can't inject env var with a comma in it's value

[Omar-Bishtawi]

Describe the bug if you try to inject the following annotation to any deployment

appmesh.k8s.aws/sidecarEnv: '[{"sample_key_1":"value","sample_key_2":"value"}]'

you will get the follwing error

malformed annotation appmesh.k8s.aws/sidecarEnv , expected format: EnvVariableKey=EnvVariableValue

Steps to reproduce attach any json value with multiple keys to sidecar env var .

Expected outcome the env variable to have a valid json as it's value

Additional Context: I think this behavior is expected since in herethe method only split by , and we can't escape this charachter in any way.

[dileepng]

This is expected, this has to be in the format of this https://github.com/aws/aws-app-mesh-controller-for-k8s/blob/master/pkg/inject/constants.go#L54

[Omar-Bishtawi]

Thanks for the info @dileepng . are you open to the idea of supporting such feature (allow escaping of the , character if it wasn't ment to be used as a separator) ? I can try to help in this if you don't have an issue with such a feature.

[BennettJames]

Hey Omar,

We're not necessarily against improving the behavior here, but it's tricky to do in a fairly general way that's reasonably backwards compatible. We'd likely need to adapt a system that's capable of parsing the string such that ,, ", and = are special characters that control handling, and allow escaping of them. Maybe it'd be ok breaking some edge cases like if a current user is setting variables like A=ab"c\,B=efg, but I'm always pretty hesitant to make a change that can have unexpected consequences.

Perhaps the best answer is to add a new variable that has a more robust parsing system around it - maybe sidecarEnvJson - and let the user specify a json map of strings-to-values.

[Omar-Bishtawi]

@BennettJames the idea of introducing a new env variable with robust parsing system is good one and resolve any backward compatibility issues that might arise of modifying the behavior of the old sidecarEnv.

also using JSON as a way to map env vars with there values is more robust and easier to parse in a consistent expected way.