feat: emitStringData boolean for secretGenerator

[stealthybox]

This allows users to pass emitStringData: true to the secretGenerator, alongside the type option.
When enabled, UTF-8 strings are output in plainText stringData, and non-UTF strings and any binary data fallback to the default behavior of being base64 encoded in data.

This is very similar to the default, kyaml behavior of loading values into ConfigMap's data and binaryData fields.

This feature provides a general U/X improvement for people using kustomize interactively, and also allows Flux users to template into generated secrets.
(Flux users already can and do template into ConfigMaps, so we want to give people more secure mechanisms in kustomize-controller)

• test: add secretGenerator testcases
• feat: support emitStringData in secretGenerator
• test: test emitStringData in secretGenerator

resolves #5142 #1444 #1261 #793

[k8s-ci-robot]

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-sigs/prow repository.

[koba1t]

/assign

[matheuscscp]

LGTM 🚀

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: matheuscscp, stealthybox
Once this PR has been reviewed and has the lgtm label, please ask for approval from koba1t. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[stealthybox]

I've just done a fresh rebase.
@koba1t, is there any way I can help out in reviewing this patch?
Thanks :)

[koba1t]

@stealthybox
So, Sorry for the late review.

• The field name stringData: true is associated with the field name of a secret resource, but is it possible to change it to a more descriptive field name, such as noEncode: true?
• As a test scenario, could you verify that when adding a secondary secretGenerator with stringData: false to a secretGenerator configured with stringData: true using the behavior: merge option, the resulting data is properly base64 encoded?

[matheuscscp]

How about outputStringData?

[koba1t]

@matheuscscp
What I'm trying to say is that field names should be clear enough that users can immediately understand how they'll behave when seeing them in this PR.
The name stringData likely comes from the existence of a stringData field in secret resources, but it doesn't actually explain the behavior in kustomize.

For example, in the case of stringData, it's not clear whether this refers to string input or what exactly is being stringified, or in the case of outputStringData, it's not clear whether this controls the output destination or something else.

[stealthybox]

could you verify that when adding a secondary secretGenerator with stringData: false to a secretGenerator configured with stringData: true using the behavior: merge option, the resulting data is properly base64 encoded?

@koba1t, thanks for pointing out this out.
I've added tests for the following cases in test: field overrides for configMapGenerator + secretGenerator
67fc288:

• 1. merge encoded data fields onto a generated secret with stringData
• 2. override encoded data fields with stringData for secretGenerator
     producing a Secret that successfully uses the new value in the Kubernetes API
• 3. override stringData fields with encoded data for secretGenerator
     producing a Secret that silently fails to use the new value in the Kubernetes API
• 4. override encoded binaryData fields with data for configMapGenerator
     producing an invalid ConfigMap
• 5. override data fields with encoded binaryData for configMapGenerator
     producing an invalid ConfigMap

The 4 cases where fields override using a different encoding output duplicate fields.
This is existing ConfigMapGenerator behavior that is undesirable.
This existing buggy behavior becomes new Secret-gen behavior.
I can fix both of these in a followup PR as discussed privately in our Slack dm.

Here is an example of what that fix could look like:
stealthybox@e95e854

[stealthybox]

Regarding the field name, I understand the concern of stringData as a bool not fully expressing the behavior.
I would hope the doc-string helps clarify this beyond just the name.

In the most recent issue,
an end-user requests a stringData bool in the generatorOptions. #5142 (comment),
@seh and @annasong20 confirm the boolean field stringData should be added to SecretArgs and exposed directly on secretGenerator. #5142 (comment)

A separate end-user suggests the boolean field should be called useStringData. #5142 (comment)

The title of unrelated issue (#1261) is "Support StringData option in SecretArgs".

Considering end-user, contributor, and maintainer responses, stringData seems to be the agreed upon field name that is most natural. I think the word "stringData" must at least be contained within the field name.

noEncode implies that there isn't a fallback encoding behavior when there is.
I don't think noEncode works.

I'd also challenge that stringData doesn't make sense.
Most of the fields on generator options across the project are passthrough fields that end up in the output:

• labels
• annotations
• immutable
• type (for Secrets only -- defaults to "Opaque" when not supplied)

The new field isn't a passthrough value field -- it changes generator behavior. disableNameSuffixHash is a similar behavioral field that describes the generated output implicitly.
That's why secretGenerator[].stringData = true in my head automatically talks about the output of the generator.

in the case of stringData, it's not clear whether this refers to string input or what exactly is being stringified

This is fair. there are a lot of fields when you consider input fields like env, envs ,files, and literals mixing with output fields like type and annotations and behavioral fields like behavior and disableNameSuffixHash.
Disambiguation would need to come from the doc-string and examples.
I'm happy to add examples to:

• https://kubernetes.io/docs/tasks/configmap-secret/managing-secret-using-kustomize/#create-a-secret.
• site/content/en/docs/Tasks/secret_generator.md

in the case of outputStringData, it's not clear whether this controls the output destination or something else.

I disagree that outputStringData is unclear. There is only one output of a generator.

I think these are decent options for field names:

• stringData
• outputStringData
• generateStringData
• setStringData
• preferStringDataOutput
• disableBase64ForStringData

The default behavior of a configMapGenerator already includes heuristic auto-encoding behavior for binaryData, and that doesn't show up in any field names. It's just the way a kustomize generator works.

Thanks for considering these points.
I don't want to be overly pedantic, and I know the behavior is complex.
Naming this field is tricky.
I think examples and the doc-strings on the field name should help people understand what it does exactly.
I don't think we should give this field any name that inaccurately implies that it performs or prevents behaviors that it does not. Because the behavior is complex, the name either needs to be somewhat ambiguous, or very detailed and lengthy.

Do you like stringData or any of the other names I suggested?

[matheuscscp]

Yeah this field has to explain itself, it has to have stringData on its name somehow. Anything else would be garbage for explaining what it does.

[stealthybox]

unrelated to field names, here's one way to fix the existing broken configMapGenerator behavior:
stealthybox@e95e854
I would send this in a followup PR.

[seh]

I think "stringData" would be a deceiving name for this new field. My suggestion today is "emitStringData".

[stealthybox]

I've proactively renamed the field to emitStringData in the types, docs, and test names.
Tests are passing.
Now that the field name is disambiguated from the stringData fields in the test output and docstrings, it's easy to rename it to whatever we decide 👍.

[stealthybox]

@koba1t and I considered in our Slack dm's whether a string value field could work:

  field: "data" # "stringData" | "binaryData"

This could be a generic way to solve this problem for both ConfigMap and Secret generators, but it also creates more error states and is harder to grep for in existing code + discover in auto-complete.

I prefer the emitStringData boolean field.
If we want support for explicit encoding in ConfigMap generator, I propose we do so by adding emitBinaryData as a boolean in another PR.

[stealthybox]

Hi friends, if there's no further contest on the field name, I'd love to get this reviewed for a merge.
I'm happy to write some docs examples.

I want to draft a change for the merge behavior bugfix in stealthybox@e95e854 .

After these changes make their way into kustomize, kubectl needs to update kustomize before we can pull this feature into Flux.
I'd like to be able to do that before KubeCon NA and the Flux 2.7 release.

cc @koba1t @seh