kpt/kyaml doesn't parse yaml sequence nodes correctly

[phanimarupaka]

The following is expected workflow

@seh So summarizing your request...

If you start with

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- target:
    kind: Something
  patch: |-
    - op: add
      path: placeholder
      value: known

and do

kpt cfg create-setter . some-json-pointer placeholder

The output should be

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- target:
    kind: Something
  patch: |-
    - op: add
      path: placeholder # {"$kpt-set":"some-json-pointer"}
      value: known

and later on

kpt cfg set . some-json-pointer new-placeholder

should give output

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- target:
    kind: Something
  patch: |-
    - op: add
      path: new-placeholder # {"$kpt-set":"some-json-pointer"}
      value: known

But it doesn't work as expected. Investigate and fix it.

[phanimarupaka]

@natasha41575 @etefera This is a good candidate to understand more about kyaml parsing. If you don't have cycles I can pick this up.

cc @mikebz

[natasha41575]

@phanimarupaka if you have cycles feel free to take it. If not, I'm happy to take a look next week

[phanimarupaka]

@seh Have you tried modifying the yaml like this

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- target:
    kind: Something
  patch:
  - op: add
    path: placeholder
    value: known

This works

[msonnleitner]

@phanimarupaka Does this really work? I don't think this is a valid Kustomization... according to https://kubectl.docs.kubernetes.io/references/kustomize/patches/

[seh]

No, I haven't tried that, because , as @phanimarupaka noted, the documentation prescribes embedding the patch content as a string (emphasis mine):

Each patch may:
[....]

• be either a file, or an inline string

I'm willing to try it, but if that works, then we need to fix the documentation. Also, if that does work, then It's weird that patches work (without interpolation) as embedded strings now.

[msonnleitner]

@seh well I have tried that, could not get it work though (which means that the documentation is correct)
it fails with

json: cannot unmarshal array into Go struct field Patch.patches.patch of type string"

[phanimarupaka]

@seh @msonnleitner There are 2 issues here.

1. kpt can't interpret value strings as nested yaml nodes.
2. kustomize needs patch to be a string.

@monopole @Shell32-Natsu Can patches be yaml nodes instead of enforcing them to be strings. This solves a bigger problem and can help users to easily parameterize patches. Enforcing them to be strings makes it difficult for tools to parse them as yaml and query them.

[seh]

Yes, with the current state of kustomize and kpt, using JSON Patch is difficult. We can't use setters and substitutions due to the these conflicting constraints:

1. Inline patches are strings, within which these tools don't interpolate setters or substitutions.
2. Separate JSON Patch files must be top-level sequences (be they JSON or YAML).
3. Using setters or substitutions requires use of YAML (since JSON does not accommodate comments).
4. These tools can't parse a top-level sequence—at least within a YAML file.

My real goal is to use a parametric JSON Patch. At present, I have to write a script to generate the patch file, interpolating the dynamic values before kustomize ever sees it, which then eliminates the use of setters for the patch content.

[Shell32-Natsu]

What kustomize supports is either a embedded string of JSON patch in kustomization or a YAML/JSON patch in a separate file. @monopole I think it's ok to accept a list of structured JSON patch. WDYT?

While IMO the real problem is kpt/kustomize is trying to process all YAML files in the directory but assuming all YAML files are k8s resource files. That's not true.

Sorry, the statement above is about another issue.

[seh]

There is also the "patchesJson6902" field. That one can also refer to separate files or accept the patch as an inline string, which is subject to the same limitations we face with the "patches" field. Do you intend to retain both of these fields in the future?

The documentation encourages writing the content as either JSON or YAML, which leads users into this defect's waiting arms.

[krafts]

+1, running into the same issue.

For my use I tried to get around this by using strategicMergePatch with a $patch: merge for rolebinding subjects but so far have been unsuccessful.

[natasha41575]

For anyone who picks this up, I believe ultimately the fix will be in the kustomize/kyaml code; feel free to submit the fix there if needed and we can still take a look at it.

[yuwenma]

Some pointers for new contributors who pick up this issue:

1. kpt cfg is deprecated, we want to verify if the apply-setters function has this problem.
2. The apply-setters code base is here https://github.com/GoogleContainerTools/kpt-functions-catalog/tree/master/functions/go/apply-setters

[annasong20]

@yuwenma I'd like to confirm the intended behavior with apply-setters.

For the kustomization.yaml in the issue description, when I run:

kpt fn eval --image gcr.io/kpt-fn/create-setters:v0.1.0 -- some-json-pointer=placeholder

The kustomization.yaml is mutated to the following:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- target:
    kind: Something
  patch: |- # kpt-set: - op: add
    #   path: ${some-json-pointer}
    #   value: known
    - op: add
      path: placeholder
      value: known

However, running apply-setters:

kpt fn eval --image gcr.io/kpt-fn/apply-setters:v0.2.0 -- some-json-pointer=new-placeholder

does not find any matches in kustomization.yaml and thus leaves kustomization.yaml unchanged.

Questions:

1. Is the current create-setters behavior acceptable or does the kpt-set comment have to be kpt-set:${some-json-pointer} on the same line as path as described in the issue?
   Given my rudimentary understanding of kpt, I assume apply-setters can replace the entire inline string with the comment string? I guess the downside may be that if the user changes part of the json op, say value: other-value, our kpt-set comment won't reflect that.
2. I assume we want to generalize the implementation to parse inline strings as yaml for all fields, not just patches? I also assume we want to try to match all inlined fields, not just path? Feedback by @phanimarupaka prompted this question: https://github.com/kubernetes-sigs/kustomize/pull/3656/files#r585855671

[annasong20]

/assign

[yuwenma]

Thanks for looking at this issue @annasong20

1. I'll treat the create-setters as a handy functions to add setters. Ideally I'd expect it to only add the comment at the end of the line which as setters. But we can make progress step by step. That means if commenting the entire patches content is much easier, let's fix it that way.

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
patches:
- target:
    kind: Something
  patch: |- 
    - op: add
      path: placeholder # {"$kpt-set":"some-json-pointer"}
      value: known

2. Exactly. The fix should be a more generic solution for multi-line parsing and matching.

[annasong20]

Task List

• Add kpt-functions-catalog test demonstrating desired behavior in create-setters and apply-setters
• Add kustomize test in kyaml demonstrating desired behavior
• File kustomize issue including implementation plan
• Implement fix
• Add e2e test in kpt-functions-catalog

EDIT: no longer relevant

[natasha41575]

To clarify, if you file an issue in the kustomize repo, the issue doesn't need to include implementation details (we can discuss that in the PR). But the issue should have a quick explanation of the problematic behavior vs the desired behavior.

[annasong20]

I will no longer be working on this issue,. Please find a summary of my findings below.

• From my limited understanding of the kpt-function-catalog create-setters, apply-setters code, I don't believe that the fix is in the kustomize kyaml code. The kpt-function-catalog node traversal logic seems to have diverged from its kyaml counterpart.
• I believe the fix is either to support
  • multi-line comments (please reference the above comment kpt/kyaml doesn't parse yaml sequence nodes correctly #1342 (comment)), the code for which should probably live in visitScalar, where the current comment-handling code is
  • parsing inline yaml, the code for which should probably live where the scalar string is processed