Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically convert JSON patch paths in MeshProxyPatches from snake_case to camelCase #8510

Closed
bartsmykla opened this issue Dec 2, 2023 · 5 comments · Fixed by #9519
Closed
Assignees
Labels
area/policies kind/improvement Improvement on an existing feature triage/accepted The issue was reviewed and is complete enough to start working on it
Milestone

Comments

@bartsmykla
Copy link
Contributor

bartsmykla commented Dec 2, 2023

Description

Currently, Envoy's config dump presents paths in snake_case format, while JSON patches in MeshProxyPatch require camelCase format. Instead of documenting this discrepancy, we can implement logic to automatically convert paths from snake_case to camelCase when encountered. This approach would eliminate the need for users to manually convert paths, improving the overall usability of the system.


xref. kumahq/kuma-website#1539

@bartsmykla bartsmykla added triage/pending This issue will be looked at on the next triage meeting kind/improvement Improvement on an existing feature area/policies labels Dec 2, 2023
@jakubdyszkiewicz jakubdyszkiewicz added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Dec 4, 2023
@lahabana lahabana added this to the 2.6.x milestone Jan 9, 2024
@lahabana lahabana modified the milestones: 2.6.x, 2.7.x Jan 17, 2024
@lukidzi
Copy link
Contributor

lukidzi commented Mar 12, 2024

I did some debugging, it is caused by not setting OrigName to true https://github.com/kumahq/kuma/blob/master/pkg/util/proto/proto.go#L44
if we change it to this we are going to have proto fields name (like in a config_dump)

MeshProxyPatch allows the creation of JSON patch and accepts JSON formatted fields camelCase; if we change the dump, a user might have to rewrite fields from the dump to match json format.

Envoy's config_dump shows only snake_case when directly requesting the envoy, but not through the Control-plane.

I think this issue might not be the case if we agree that what shows UI is a source of truth, but if we decide that we should stick to the same format as Envoy(which would be the best for cross docs reading) then we need to rewrite a lot of things and we need to keep backward compatibility for old patches in JSON format

@lukidzi lukidzi added triage/needs-information Reviewed and some extra information was asked to the reporter and removed triage/accepted The issue was reviewed and is complete enough to start working on it labels Mar 12, 2024
@jakubdyszkiewicz
Copy link
Contributor

Triage: let's change API so the config dump is snake_cased (just like 9901) not camelCased

@jakubdyszkiewicz jakubdyszkiewicz added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/needs-information Reviewed and some extra information was asked to the reporter labels Mar 12, 2024
@lahabana
Copy link
Contributor

This because of the sanitization:

func (a *envoyAdminClient) ConfigDump(ctx context.Context, proxy core_model.ResourceWithAddress) ([]byte, error) {
configDump, err := a.executeRequest(ctx, proxy, "config_dump")
if err != nil {
return nil, err
}
cd := &envoy_admin_v3.ConfigDump{}
if err := util_proto.FromJSON(configDump, cd); err != nil {
return nil, err
}
if err := Sanitize(cd); err != nil {
return nil, err
}
return util_proto.ToJSONIndent(cd, " ")
}

How about we retrieve the tokens that needs to be redacted in the first step and then redacted from the raw payload? This would avoid the reserialization.

@lukidzi
Copy link
Contributor

lukidzi commented Mar 12, 2024

We are sanitizing DataplaneToken from the output. We don't store it anywhere so we won't be able to retrieve it.

@lahabana
Copy link
Contributor

lahabana commented Mar 13, 2024

What I mean is something like:

	configDump, err := a.executeRequest(ctx, proxy, "config_dump")
	if err != nil {
		return nil, err
	}

        toReplace := []string{}
	cd := &envoy_admin_v3.ConfigDump{}
	if err := util_proto.FromJSON(configDump, cd); err != nil {
		return nil, err
	}
        // Retrieve strings that need to be sanitized
	for _, config := range configDump.Configs {
		if config.MessageIs(&envoy_admin_v3.BootstrapConfigDump{}) {
			bootstrapConfigDump := &envoy_admin_v3.BootstrapConfigDump{}
			if err := config.UnmarshalTo(bootstrapConfigDump); err != nil {
				return err
			}

			for _, grpcService := range bootstrapConfigDump.GetBootstrap().GetDynamicResources().GetAdsConfig().GetGrpcServices() {
				for i, initMeta := range grpcService.InitialMetadata {
					if initMeta.Key == "authorization" {
						toReplace = append(toReplace, grpcService.InitialMetadata[i].Value, "[redacted]")
					}
				}
			}

			for _, grpcService := range bootstrapConfigDump.GetBootstrap().GetHdsConfig().GetGrpcServices() {
				for i, initMeta := range grpcService.InitialMetadata {
					if initMeta.Key == "authorization" {
						toReplace = append(toReplace, grpcService.InitialMetadata[i].Value, "[redacted]")
					}
				}
			}
		}
	}
        return strings.NewReplacer(toReplace).Replace(configDump)

It seems to also avoid multiple occurences of serialization which likely makes this quicker too :)

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/policies kind/improvement Improvement on an existing feature triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants