Proposal: Replacements and Patch value in the structured data

[koba1t]

This proposal decides the interfaces to change values in the structured data (like json,yaml) inside a Kubernetes objects' field value and implements changing function target a few formats (mainly json).

appendix

• [Feature request] Replacements value in the structured data. #4517
• proposal replacements_value_in_the_structured_data for json #4518

[k8s-ci-robot]

Hi @koba1t. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[natasha41575]

/ok-to-test

[natasha41575]

Thanks for writing this up! It looks great. Some comments, mostly that we should include the ConfigMapGenerator proposal here too, that's already written up here #680 (comment) and we should try to make sure the two syntaxes for replacements and ConfigMapGenerator make sense when we look at them side by side.

[k8s-ci-robot]

@koba1t: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[koba1t]

Hi, @natasha41575.

Sorry for the delay to reply.
I fix from your feedback. (And add details from #680 (comment))
Could you recheck it?

[natasha41575]

Thank you! Will take another look later this week

[natasha41575]

Overall this is getting close to something I am open to accepting. I have some minor wording suggestions in the motivation section so that the scope is clear, and apart from that I'd like to get more feedback about the configMapGenerator syntax from @KnVerey.

[natasha41575]

I see you've made a modification to the original proposal in #680 (comment), which looks like:

configMapGenerator:
- name: demo-settings
   behavior: merge
   patchContent: json-strategic-merge
- file: my-app-settings=patch.json
- literals:
   - foo= -|
              {
                      "logLevel": "info"
              }

I feel that the original proposal is a bit more descriptive, but yours feels a bit more closely tied with the replacements syntax. WDYT about doing this:

configMapGenerator:
- name: demo-settings
  behavior: merge                     # This function requires `behavior: merge`.
  option:
    patchContent: strategic-merge
    format: json  # Setting structured data format.
  literals:
  - config.json: |-
      {
        "config": {
          "hostname": "REPLACE_TARGET_HOSTNAME",
          "value": {
            "foo": "bar"
          }
        }
      }

?

@KnVerey would also appreciate your feedback on the right syntax here

[koba1t]

hi @KnVerey
Could you give me any feedback for this?

[koba1t]

@natasha41575

Hmm...
I thought this implementation would only be used to simply merge 2 or more json contents.
And if we're implementing json6902 , we probably need extra sintax like patchesjson6902.

So, if you have other ideas, could you please tell me?

[koba1t]

I don't think it is easy to understand what happens with this syntax.

# overlay/kustomization.yaml
resources:
- ../base
configMapGenerator:
- name: demo
  option:
    patchContent: json6902
    format: json  # Setting structured data format.
  literals:
  - config.json: |-
      - op: replace
        path: /config/value/foo
        value: qux

[KnVerey]

Sorry for the extreme delay. First, I don't think we should support json6902 or anything other than strategic merge, i.e. the behavior already implied by behavior: merge for the rest of the object. That seems sufficient to cover the use case.

Second, I think this option needs to move up a level, i.e. in GeneratorArgs rather than GeneratorOptions. The latter can be provided globally in a kustomization, and the new data here is specific to the generated object in question.

Third and most importantly, I think we need more information from the user than what the proposals so far have provided for. During overlay configmap generation, we essentially end up with a configmap from the base + a new one from the overlay's files/envs/literals. Currently, behavior: merge applies the overlay to the base as an SMP. To support this feature, it seems to me will first need to merge specific fields' values as SMPs before merging the whole object. How do we know which fields we need to do that for? We can't assume they're all JSON whenever the user wants one of them to be merged.

configMapGenerator:
- name: demo-settings
  behavior: merge
  mergeValues:  # or something like mergeOptions.yamlValues? Not under "options" at any rate
  - config.json # Value at this key MUST be YAML/JSON
  literals:
  - config.json: |-
      {
        "config": {
          "hostname": "REPLACE_TARGET_HOSTNAME",
          "value": {
            "foo": "bar"
          }
        }
      }

Finally, a bit of a sidenote: if you try to specify duplicate keys across literal, env and file sources, Kustomize currently throws an error. That behaviour should be retained. In other words, the new feature should only affect the final merge operation.

[koba1t]

@KnVerey
I fix from your comments. At d99011f
Cloud you recheck it to reflect on your suggestion?

Second, I think this option needs to move up a level, i.e. in GeneratorArgs rather than GeneratorOptions.

I move it.

Third and most importantly, How do we know which fields we need to do that for?

I think it is my insufficient consideration and agree with you. I add a parameter to choose a key for the configMap.
But, I think we need any parameter to select a value format which is JSON or YAML. because we can't discriminant clearly the value format when it is not selected with the file extension. For example, value is used by env in deployments like JSON in the environment values.
Therefore I try to add a parameter mergeValues.format to select the value format which is JSON or YAML.
d99011f#diff-2b5a81ecf356afb37e2653ac1e2daf0b6af6cd126aec835df3c25b2a8be29389R153-R155

[natasha41575]

So kustomize doesn't allow it to be manipulated. This is expected behavior, but we would like to make an exception for some structured JSON within string literal fields.

[koba1t]

/remove-lifecycle stale

[the-scott-hand]

@natasha41575 @koba1t @KnVerey Any updates or projected timeline on this feature? This feature would resolve an issue that our management regularly mentions as a big drawback for kustomize.

We often run into this scenario: a kubernetes deployment has been configured to read in a config file config.yaml mounted into the pod via a configmap's structured string data field. This multiline string data field is typically in yaml or json format. We discover that we need to set some of these config items for a particular site or particular customers deployment and immediately increase the estimated completion time/level of effort when we realize the config in question is inside a structured string. We have had to solve this issue in a multitude of "creative" ways, for example:

• If open source, we hope they added an environment variable override so we can patch that into the deployment
• If we own the code, we modify the code and rerelease with environment variable overrides for all configs that may need patching
• If there is no env override, sometimes there is a unique replacement that will work (delimit on " and replace the 42nd value for instance)
• Some scenarios we can copy and paste the entire structured string into the patch and make the small modification we need
• Use kustomize to move the configmap mount to an init container that updates the configfile based on env variables and writes out the desired config to a shared volume mount where the application was originally configured to read from

This feature would not only save time/money but also improve the readability and maintainability of our code base. We'd love to see this prioritized and merged as soon as possible!

#3787
#4517

[natasha41575]

proposal LGTM

Thank you for working on this and pinging me. I think you will have to rebase to make the test suite run so we can merge this.

/lgtm
/approve

[joebowbeer]

Once this is merged, I'll be interested to see what gaps (in existing vars functionality) remain to be filled.

I created a discussion in hopes of clarifying the current deficits, as well as plans to address them:

#5046

[natasha41575]

/lgtm cancel

Ongoing discussion in #4558 (comment), should get resolved soon

[natasha41575]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: koba1t, natasha41575

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [koba1t,natasha41575]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

We appreciate the contributions and discussion but knverey is no longer active in this project.