feat: helm api support for reading valuesFiles from ConfigMaps

[natasha41575]

• Adds support for reading valuesFiles from in-cluster ConfigMaps.
• Watches ConfigMaps in the RSync namespace, and rehydrate/resync when modifications are detected.
• Changes the 'commit' to helm charts to specify just the version, rather than chart:version.

Design and context: go/cs-helm-values-from-cm

[google-oss-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[natasha41575]

/test all

[natasha41575]

/assign @sdowell

[natasha41575]

/assign @sdowell

[natasha41575]

/hold

[natasha41575]

/retest

[karlkfi]

Not a fan of the naming decisions here. See inline comments.

[karlkfi]

I've been informed that many of these naming decisions were documented in the design doc and approved by other people. So I will summarize my concerns here for posterity.

1. I think copying the valuesFrom syntax from Flux is a mistake. We're not support values from Secrets, so we don't need the complexity of specifying the Kind or Key. We could provide a much simpler API instead. IF we DO plan to add support for Secrets, then we should just do that now. It's not that much work and it makes the API decisions make more sense.
2. I generally prefer "Strategy" over "Mode". I did some research and found examples of both in the K8s API, but the design pattern is called the "strategy pattern". "Mode" is better used when selecting between multiple models.
3. The reason Kubernetes uses the strategy pattern so often (instead of booleans) is to allow extension after initial implementation. With this in mind, the "merge" value is a bad choice of value, because it makes the API mergeMode: merge, which i both redundant and all encompassing. You look at it and think "what other possible values could there be?".
4. Not only is "merge" bad for the above reasons, but it's also confusing because it's only actually merging some values types. Scalars are not being merged. Lists are being concatonated/joined. Only maps/objects are actually being merged... We need a better strategy name.
5. If we are going to copy flux's API for valuesFrom, we should at least copy the whole thing and use valuesKey. Why rename it "key"?
6. If we're not going to copy flux's API exactly, I think "key" is an inaccurate name. Maps have keys. Resource objects have fields. If we really want to allow future extension, with other resources than just ConfigMap and Secret, we should use "field" and support JSONPath or at least period separated field path syntax. For example, if using a ConfigMap, you might set valuesField: data["values.yaml"] (since you can't use dot-syntax with a key that contains a dot).
7. And if we really want to allow support from other resources as inputs, we might also want to avoid hard coding Secret kind values as needing base64 decoding. That could be done by adding an optional valuesEncoding: base64 field.
8. Whether we do or don't keep the valueFrom syntax, we should make the command line flag more clear. It's not obvious from the flag name or the description how to use it or what the values should be.
9. If we don't need to copy Flux, then I would recommend not using -From in a field name. The name of a list field should in most cases include the type of the entries it expects, for readability and discoverability. -Sources or -Objects might be more clear. Then the list field can be plural and then entry object can be singlular. That avoids the plural and singular having the same name (e.g. valuesFrom = []ValuesFrom{}).

[janetkuo]

The goal isn't to replicate Flux's API, so we don't need to follow them. We also don't need to support Secrets now given that no users ask for it. I prefer strategy over mode as well.

What should the second strategy be called, if not "merge"? It's already a term that's used by Helm users in the feature request for this functionality. How do you expect Scalars and Lists to be merged? If the concern is about the redundant mention of "merge", we can call it something like valuesFilesApplyStrategy (the strategy to apply multiple values files).

Regarding 9, I prefer Sources over Objects.

[natasha41575]

Based on the above comments, discussed with @janetkuo and we are going to make the following naming changes:

valuesKeyMergeMode -> valuesFileApplyStrategy
valuesFrom -> valuesSources
valuesSources.Kind will be optional, and default to ConfigMap
valuesSources.Key -> valuesSources.valuesFile

[karlkfi]

Can we just leave out the kind and key?
Do they need to be configurable?
If that capability wasn’t asked for by the CX can we punt on how to configure it?
Having a source just be an object with a name field seems nice and simple. Tho I’m also ok with having a kind that defaults to ConfigMap, because that makes the API slightly more readable without needing to see the reference docs.
However, leaving out the Kind lets us punt on whether we only support resource objects as sources. We might eventually want to add a URL to a values.yaml, for example.

Other alternative names:

• valuesKeyMergeMode -> valuesFileMergeStrategy (keys aren’t merging, files are merging)
• valuesFrom -> valuesFileSources (The ConfigMap doesn’t contain values. There’s only one data key. It contains a values file encoded as a yaml string.)

[karlkfi]

Another consideration:
Can I use values & valuesFrom? What order do they merge/override?

[natasha41575]

Can we just leave out the kind and key?
Do they need to be configurable?
If that capability wasn’t asked for by the CX can we punt on how to configure it?

The customer didn't ask for it, so we could leave it out for now if we want to punt on the design. The key allows the user to use the same ConfigMap for multiple valuesFiles, which seems convenient to me but it is something we could theoretically add later. Same with kind, we could add it later if we don't want to do it now. I very slightly prefer keeping them but am OK to take them out. I'll defer to @janetkuo to be the tiebreaker on this.

valuesKeyMergeMode -> valuesFileMergeStrategy (keys aren’t merging, files are merging)

This still has the same issue that you mentioned above with merge being duplicated in the field name and the value. I made a typo above (in case you saw the comment before I edited it), Janet actually suggested valuesFilesApplyStrategy as it is the strategy of how we apply the valuesFiles to the chart; either they "override" each other or we "merge" them together. I think of the options discussed so far I still prefer this.

valuesFrom -> valuesFileSources

This SGTM. I think it is more accurate than the above valuesSources that I suggested.

Can I use values & valuesFrom? What order do they merge/override?

Yes you can. values is treated as the last valuesFile (so will override any valuesFile from valuesFileSource). There's an explanation in the CRD description and godoc, would appreciate feedback if the wording is clear from there.

[karlkfi]

As for the “merge” strategy enum… I think there are a few approaches.

1. Consider what other types of possible strategies we might want to add later and how you might name them all to be unique.
2. Look for other yaml/json merge tools and see what options they expose.

For example, in the future, we may want to allow lists to merge using a unique key field, because that’s a very common pattern in k8s config: unordered lists of maps (e.g. containers). Concatenating is only one way to merge lists and probably not the most useful.

Another consideration is that current merge strategy is actually using “override” on maps. So it’s not super clear what the distinction is between those modes.

As for prior art, consider CRD merge strategies: https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy

So maybe what we really need is type-specified strategy options… I have a couple ideas:

1. mapMergeStrategy, listMergeStrategy, stringMergeStrategy, scalarMergeStrategy
2. a list of type strategy maps, like fieldMergeStrategies: [{type: map, strategy: overwrite}, {type: list, strategy: listMapKey, key: blah}]
3. a list of default and/or field-specific strategy maps, like fieldMergeStrategies: [{type: list, strategy: atomic}, {type: list, strategy: map, listMapKey: blah, fieldPath: “a.b.c”}]

Option 3 would allow for specifying default and path specific merge strategies for the same type, in case that’s desired. That would more closely mirror the flexibility provided by CRD schema.

[janetkuo]

Good point that we can add kind later, so it's fine to remove that field now and we can add it when we need to support more types (and keep the default as ConfigMap).

We're handling values.yaml instead of KRM yaml. Does it allow those merge strategies? The merge strategy proposed in this PR seems good for our use case.

The valuesSources.valuesFile field is useful if users want to use a name other than values.yaml.

[karlkfi]

We're handling values.yaml instead of KRM yaml. Does it allow those merge strategies? The merge strategy proposed in this PR seems good for our use case.

AFAIK, helm values can be any data structure supported by yaml. You could have a value named spec that was the entire multi-layer resource spec map. It wouldn't even surprise me if it supports non-string keys and non-json-compatible fields, even tho those aren't allowed in KRM yaml. Helm templates use Go-templating to translate values for interpolation, so you can do almost anything with them.

[janetkuo]

I don't think we need to support other types of merge strategies - I would rather it to be supported by Helm. Natasha implemented this for our own use case because of the lack of support in Helm. I'm fine with renaming the current strategy from "merge" to something more descriptive.

[karlkfi]

Are we certain that the merge behavior, as described, meets the needs of the Fleet Tenancy team?

I'd doubtful that list concatenation is at all useful.

I'm also unsure if "override" works like "set" or "atomic" or some new behavior unlike any existing CRD merge strategy.

[natasha41575]

I have changed merge to just do list concatenation, and changed its name to listConcatenate as that's all that is required for now.

[janetkuo]

/lgtm

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo

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 [janetkuo]

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

[natasha41575]

/hold cancel

changes addressed