kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
10.99k stars 2.25k forks source link

envCommand interprets values surrounded by double quotes oddly #460

Closed cm-yamasaki-michihiro closed 5 years ago

cm-yamasaki-michihiro commented 6 years ago

Kustomize's envCommand in SecretGenerator interprets dotenv output oddly.

Some commands like aws-env generate dotenv with following format.

FOO="VAR1"
BAR="VAR2"

Values are surrounded by double quotes.

When Kustomize interprets this output with envCommand, it generates following output:

apiVersion: v1
data:
  # encoded values are surrounded by double quotes
  foo: IlZBUjEi # base64 encoded value of '"VAR1"'.
  bar: IlZBUjIi # base64 encoded value of '"VAR2"'.
kind: Secret
metadata:
  creationTimestamp: null
  name: from-aws-env-879mkbh49f
type: Opaque

Encoded values in the secret are surrounded by double quotes. Is this by design?

monopole commented 6 years ago

If its not there already, we should have regression test coverage to define behavior in both cases, so we know what we are doing.

On my machine

$ export HEY="AA BB"
$ printenv | grep HEY
HEY=AA BB

likewise:

$ export HEY=AA\ BB
$ printenv | grep HEY
HEY=AA BB

Same answer either way. And you'll have things like aws-env that apparently add the quotes regardless.

I suspect that the right answer is for kustomize to not add quotes, and assume the generator will add them if it wants them.

Liujingfang1 commented 6 years ago

I agree with @monopole that kustomize shouldn't add or remove quotes. The commands in generator need to take care of this part.

I suspect that Kustomize adds extra quotes in some cases. It might be related to converting []byte to string then to []byte when kustomize handles generators.

ryancox commented 6 years ago

Feel free to close the PR I put up against this issue. However, it doesn't seem totally unreasonable to strip them since these are values to be interpreted prior to the shell processing ( and stripping ) quotes. In this case kustomize is interpretting the value and stripping quotes seems like the principle of least surprise.

For the code path that gets the value from the environment ( which has been processed ), I agree that nothing should be stripped.

¯_(ツ)_/¯

katainaka0503 commented 6 years ago

@Liujingfang1 I think kustomize should strip quotes if envCommand is not only for echo command.

Becase tools like godotenv marshal output values with quotes and dotenv 's doc shows example with quotes. It seems very common to surround value with quotes in dotenv.

I think current implementation which only split each line with = is a little too rough to interpret output of various commands.

monopole commented 6 years ago

I'm a bit lost on this issue.

@cm-yamasaki-michihiro The answer to the question is this by design? is conclusively answered by looking for a unit test that does the thing, and if you cannot find one, write one. That establishes the actual behavior, intentional or no. And you've got better regression coverage, regardless of whether you like the behavior.

Short of a unit test, i can only make general comments.... If we attempt to quote or unquote or escape or unescape command output - modifying that output - we'll surprise someone, and as @ryancox knows, that's not good. Likewise when reading env files, we'll surprise someone if we mess with quotes or escaping, because presumably the file author is capable of saying what they want to say. We also don't want to go down the rabbit hole of trying to examine the command being used, or what shell we're running in, or what OS we are on, to try to infer some action to take with respect to quoting.

Would greatly appreciate someone writing a unit test to show the undesired behavior.

Benjamin-Dobell commented 5 years ago

I think the issue here stems from kustomize's documentation:

i.e. a Docker .env file or a .ini file

This line in the documentation is incorrect. Kubernetes env files and .ini are different and incompatible file formats.

Kubernetes .env file:

There is no special handling of quotation marks (i.e. they will be part of the ConfigMap value)).

Windows .ini file:

If the string associated with lpKeyName is enclosed in single or double quotation marks, the marks are discarded when the GetPrivateProfileString function retrieves the string.

What's perhaps more confusing is that YAML (i.e. Kubernetes config maps) will strip quotes.

Personally, I'd prefer it if Kustomize just did what it says on the tin i.e. one job - patching resources using existing Kubernetes syntax, and not overstep those boundaries.

I'm presently replacing my own custom (and extremely simple) resource patching scripts with Kustomize, to in theory simplify on-boarding, and use industry standardised tools where possible. Unfortunately, functionality like generators really muddy the waters, and issues like this introduce the kind of cognitive overhead I was trying to avoid.

Anyway, for what it's worth, I think it's best to go with Kubernete's .env file format (i.e. leave quotes verbatim in the value), even if it is itself non-standard, it's at least the context we're presently working in.

Liujingfang1 commented 5 years ago

Close this issue since envCommand was removed from SecretGenerator.