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

yaml.Marshal() can not be forced to leave strings quoted #108

Open
spkane opened this issue Feb 29, 2024 · 8 comments · May be fixed by #115
Open

yaml.Marshal() can not be forced to leave strings quoted #108

spkane opened this issue Feb 29, 2024 · 8 comments · May be fixed by #115

Comments

@spkane
Copy link

spkane commented Feb 29, 2024

Originally reported here: kubernetes-sigs/kustomize#5558

What happened?

We have a YAML string in a ConfigMap that contains a variable (e.g. ${TEST}) which will be replaced with a string AFTER running kustomize --build .

The problem is that the quotes are removed from the string by kustomize and then if that variable is replaced with something like true, the ConfigMap will be invalid since true is interpreted as a boolean value instead of a string and that is not allowed in a ConfigMap.

What did you expect to happen?

I expect kustomize to leave a quoted string quoted.

How can we reproduce it (as minimally and precisely as possible)?

# kustomization.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - "configmap.yaml"
# resources.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: test-object
data:
  pci: '${TEST}'

Expected output

apiVersion: v1
data:
  pci: '${TEST}'
kind: ConfigMap
metadata:
  name: test-object

Actual output

apiVersion: v1
data:
  pci: ${TEST}
kind: ConfigMap
metadata:
  name: test-object
  • Note the missing quotes. If that variable is then templated with a boolean or integer, this is a significant problem.

Kustomize version

v5.3.0

Operating system

MacOS

@sibucan
Copy link

sibucan commented Feb 29, 2024

This problem kinda sucks because the original YAML will have quotes that are not preserved between the conversion of kustomize template->JSON->YAML, and if we desire to keep them, there's no way to indicate to kustomize that they shouldn't be removed. Suppose we start with this YAML kustomize template:

apiVersion: v1
kind: ConfigMap
data:
  test1: "${TEMPLATE_VAR1}"
  test2: "{TEMPLATE_VAR2}"
  test3: "true"
  test4: "test4"
metadata:
  name: config

When executing kustomize build, it'll read the kustomization.yaml file and render the manifest. I've narrowed the path down to the JSONToYAML() function used by the build command here: https://github.com/kubernetes-sigs/kustomize/blob/33caee50cb25954e1889cea30e3a5b6283e0bfef/api/resource/resource.go#L371

This function is supposed to take a JSON byte array and turns it into valid YAML:

JSON:

{"apiVersion":"v1","data":{"test1":"${TEMPLATE_VAR1}","test2":"{TEMPLATE_VAR2}","test3":"true","test4":"test4"},"kind":"ConfigMap","metadata":{"name":"config"}}

FINAL RESULT

apiVersion: v1
data:
  test1: ${TEMPLATE_VAR1}
  test2: '{TEMPLATE_VAR2}'
  test3: "true"
  test4: test4
kind: ConfigMap
metadata:
  name: config

My assumption is that any string that isn’t already a valid JSON type (such as a sub structure with {} or a boolean true) gets its quotes stripped (You can test the code out in this playground link: https://go.dev/play/p/rFUUlFjPk5f):

@spkane spkane changed the title yaml.Marshal() is incorrectly removing quotes from strings under various hard-to-predict situations yaml.Marshal() can not be forced to leave string quoted Feb 29, 2024
@spkane spkane changed the title yaml.Marshal() can not be forced to leave string quoted yaml.Marshal() can not be forced to leave strings quoted Feb 29, 2024
@spkane
Copy link
Author

spkane commented Feb 29, 2024

Our investigation suggests that the YAML parser is actually doing the "right thing", but the fact that quotes are removed when we explicitly need them, and there appears to be no way to force them to be kept, is very problematic in our use case.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2024
@Red-M
Copy link

Red-M commented Jun 18, 2024

I'm also seeing this but its causing "1234" to be turned into 1234 in resource annotations and labels, which is ironically making kubernetes' own YAML parser not compliant with its own resource manifest schemas.

@Red-M
Copy link

Red-M commented Jun 20, 2024

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 20, 2024
@waddles
Copy link

waddles commented Aug 7, 2024

This could be solved if the output of JSONToYAML() included explicit yaml type tags, although that is a pretty ugly solution eg.

apiVersion: v1
data:
  test1: !!str ${TEMPLATE_VAR1}
  test2: !!str '{TEMPLATE_VAR2}'
  test3: !!str "true"
  test4: !!str test4
kind: ConfigMap
metadata:
  name: config

Kustomize at least knows that the values must all be strings

---
apiVersion: v1
kind: ConfigMap
data:
  test1: !!str "${TEMPLATE_VAR1}"
  test2: "{TEMPLATE_VAR2}"
  test3: "true"
  test4: "test4"
  test5: !!str no
  test6: !!bool no
metadata:
  name: config

❯ kustomize build .
Error: map[string]interface {}(nil): yaml: cannot decode !!str `no` as a !!bool

Could you instead prepend !!str to the value of your ${TEMPLATE_VAR1} as a workaround like this?

❯ kustomize build . | TEMPLATE_VAR1='!!str 1234' envsubst
apiVersion: v1
data:
  test1: !!str 1234
  test2: '{TEMPLATE_VAR2}'
  test3: "true"
  test4: test4
  test5: "no"
kind: ConfigMap
metadata:
  name: config

@mikebveil
Copy link

mikebveil commented Aug 15, 2024

So yaml.encode.stringv() has an internal variable canUsePlain, that's set for multi-line strings or for strings aliasing reserved words like "true". We'd like to pass in on override that always sets canUsePlain to false.

So we'd need to have a parameters object passed through yaml.JSONToYAML(), then yaml.Marshal(), then encoder.newEncoder(). And I guess we'd need a new top-level function name like yaml.JSONToYAMLWithOptions() to keep compatibility with callers, since Go doesn't have optional arguments?

My concern is that while we do use a fork of go-yaml, there's this notice:

In this fork, we plan to support only critical changes required for kubernetes, such as small bug fixes and regressions. Larger, general-purpose feature requests should be made in the upstream go-yaml library, and we will reject such changes in this fork unless we are pulling them from upstream.

I doubt the upstream maintainer would want to support a relative niche feature in their general-purpose library. But on the other hand, I don't see how kustomize can do the right thing without it, and the point of having this library is to support projects like kustomize. Could we make this change in the fork anyway?

@mikebveil
Copy link

mikebveil commented Aug 15, 2024

Tagging @niemeyer as the upstream maintainer, about whether either of the changes mentioned in my last comment would be accepted upstream.

  1. Pass a params object to yaml.Marshal() (e.g. yaml.MarshalWithOptions()) with a parameter to always enclose string values in quotes.
  2. Improve behavior of encoder.stringv() for special cases like "- this is not a list, it's a string". [ignore, this actually works]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants