helm-controller: providing values from sources

[hiddeco]

The Helm Operator supports values from sources, for example via a Secret or ConfigMap resource. Given many charts run with some secret values, this is a requirement for users to be able to migrate to the helm-controller.

The current design in the Helm Operator could be ported as it has proven to be useful, but there also have been feature requests to extend the current functionalities:

• To watch for changes on resources and reconcile the release Watch for ConfigMaps and Secrets values changes to trigger a sync helm-operator#151
• The option to provide the key the values should be merged at valuesFromFile to create key's value sourced from file content helm-operator#49
• The ability to provide an HTTP header and/or payload for the externalSourceRef Ability to pass body and headers in externalSourceRef helm-operator#348
• To append array items instead of replacing them Merging secrets does not work as expected helm-operator#126

The goal of this discussion is to:

• Gather information on if there are any additional features that should be taken into consideration
• What features we actually do want to support
• Define an API (outline)

[stefanprodan]

There is also:

• Providing Values via chartFileRef

[hiddeco]

While this relates to the features the helm-controller would support, it would be part of the source-controller API. I do think we would want to (eventually) support it, but this discussion is scoped around the helm-controller API.

[hiddeco]

I have an objection against two of the proposals.

The ability to provide an HTTP header and/or payload for the externalSourceRef fluxcd/helm-operator#348

While I do think it would be useful to support HTTP/S credentials (provided using a secret, and not embedded in the HelmRelease). I would not include the option to provide a payload for now, as this would also mean people likely want to configure the type of request made (POST, PUT, or even PATCH), which seems out-of-scope for a "getter" functionality.

If the question arises again, we can always reconsider including it based on the use-cases provided by users.

To append array items instead of replacing them fluxcd/helm-operator#126

I feel nothing for including this, it goes against the default Helm merging behaviour and will hurt more people (by not being able to overwrite a complete list) than help them, while making it a configurable option would be cumbersome.

[stefanprodan]

I agree with you on both cases. I would remove externalSourceRef altogether since it goes against GitOps, you can't reliably restore the cluster state from Git repositories if the configuration of a service relies on some URL being available. Having the optional param is even more problematic, as the cluster state will change if the URL is briefly unavailable during reconciliation.

[hiddeco]

Given the fact that externalSourceRef was introduced to be able to use e.g. values-prod.yaml files from some public chart, which would now be covered by fluxcd/helm-controller#4. I think I agree.

[stefanprodan]

I propose a simpler approche to values:

spec:
  valuesFrom:
  - kind: ConfigMap
    name: config-values
  - kind: Secret
    name: secret-values

• no key selector, like with GitRepository secretRef.identity, we expect for values.yaml to be present
• no optional flag since one can add/remove items with a kustomize patch prior to syncing
• no URL ref since it doesn't fit with GitOps
• no File ref since we don't support Git repos but only Helm repos
• no namespace for better security

[stefanprodan]

How would you create the Kubernetes secret with kubectl assuming you have the certs file on disk?

[hiddeco]

kubectl create secret generic cert-values \
  --from-file=ca.crt=./certs/ca.crt \
  --from-file=issuer.crt=./certs/issuer.crt \
  --from-file=issuer.key=./certs/issuer.key \
  --from-literal=crtExpiry=$(date -d '+8760 hour' +"%Y-%m-%dT%H:%M:%SZ")

[stefanprodan]

This is great, thanks for showcasing the usage! I would mark you proposal as the answer.

@squaremo do you have any concerns regarding Hidde's proposal?

[squaremo]

Solid, yep. I suggest the name targetKey or targetPath for the place in the values where the data ends up -- "subsetKey" could refer to projecting the source values. And if you consider that, consider also sourceKey / sourcePath.

[hiddeco]

We considered source while brainstorming in private, but in the context of the toolkit "source" is a pretty overloaded term. Given this, and due to the fact that it indeed accepts a key path, I think your targetPath might be the term we were looking for.

[seaneagan]

As long as values continues to be supported this can be avoided, but watching valuesFrom items for updates could lead to a similar scenario as mentioned in fluxcd/helm-controller#3. Imagine if you have 10 valuesFrom items, then that's 11 items (including the HelmRelease) that need to be coordinated. The pattern of creating new ConfigMaps and Secrets each time using a hash in the name may be needed, so that may need to be documented.

[hiddeco]

Besides using values, you can avoid this by putting the values that need to be applied at the same time into the same Secret or ConfigMap (potentially combined with valuesKey and targetPath).

[hiddeco]

To combine #100 (reply in thread) and #100 (reply in thread), and settle on an answer.

Proposed spec changes

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index 5c13e45..086f624 100644
--- a/docs/spec/v2alpha1/helmreleases.md
+++ b/docs/spec/v2alpha1/helmreleases.md
@@ -70,6 +70,10 @@ type HelmReleaseSpec struct {
 	// +optional
 	Uninstall Uninstall `json:"uninstall,omitempty"`
 
+	// ValuesFrom holds references to values in Secrets and ConfigMaps.
+	// +optional
+	ValuesFrom []ValuesReference `json:"valuesFrom,omitempty"`
+
 	// Values holds the values for this Helm release.
 	// +optional
 	Values apiextensionsv1.JSON `json:"values,omitempty"`
@@ -96,6 +100,30 @@ type HelmChartTemplate struct {
 	Interval *metav1.Duration `json:"interval,omitempty"`
 }
 
+// ValuesReference contains enough information to let you locate the
+// values reference at namespace level, and where the (sub)set of
+// values should be merged at.
+type ValuesReference struct {
+	// Kind of the values referent.
+	// +kubebuilder:validation:Enum=Secret,ConfigMap
+	// +required
+	Kind string `json:"kind"`
+
+	// Name of the values referent.
+	// +required
+	Name string `json:"name"`
+
+	// ValuesKey is the key in the referent the values can be found at.
+	// Defaults to 'values.yaml'.
+	// +optional
+	ValuesKey string `json:"valuesKey,omitempty"`
+
+	// TargetPath is the YAML dot notation path the values should be merged at.
+	// Defaults to 'None', which results in the values getting merged at the root.
+	// +optional	
+    TargetPath string `json:"targetPath,omitempty"`
+}
+
 // Install holds the configuration for Helm install actions.
 type Install struct {
 	// Timeout is the time to wait for any individual Kubernetes operation (like Jobs

[jbilliau5668]

What keeps biting my team is the fact we can't inject Secret or ConfigMap values if we are pointing at a tarball release; that only works if we point at a raw Helm template repo. For things like Prometheus, I need to point to a "cluster-info" ConfigMap to get remote_write authentication since we are sending our data to a remote datasource. The only way to do this is to get the raw Helm YAML for Prometheus and store it myself, which is obviously not ideal. I'd rather just point to version 11.0.0 of the official chart, and during apply, have my own values derived from the cluster applied.

Is this possible?

[carlpett]

Coming in a bit late here, but removing the namespace support in configmap/secrets will be a bad breakage in our setup.
Since we have a significant number of clusters, we're creating a "cluster-info" configmap in our clusters to inject eg domains for ingress etc.
It'd be semantically weird to create the HelmReleases in the namespace where we put the configmap.

We had this discussion of information disclosure back when namespace support was added (helm-operator #120), and at the time Kubernetes RBAC was considered sufficient. I don't really understand how this changed with the new design?

[stefanprodan]

I think we can add the namespace back and highlight in our docs that this can be used to steal secrets cross-namespaces in a multi-tenant setup. To make this safe, we should create OPA Gatekeeper rules and deny HelmReleases that have secrets ref to other namespaces.

[carlpett]

Sounds like a good approach to me, esp good with the actionable advice for how to handle it in the environments where it is a concern 👍

[hiddeco]

I am still on the fence about this, not so much because it is preventable by RBAC or OPA Gatekeeper rules, but because being able to refer to resources in another namespace than the resource itself is not what I would call a 'Kubernetes best practice'. Which is something we are trying to get right with(in) the GitOps Toolkit components, as laid out in the announcement blog post.

What would be a more interesting approach to solve your specific issue in my opinion, would be to see if it is possible using for example Kustomize to apply the ConfigMap into any namespace that requires the values. This would allow you to keep a single configuration file while still adhering to the above rule.

[carlpett]

Sure, it might be best to take a step back and see if we're really in "holding it wrong" territory. We'd probably be able to shift away from Helm to Kustomize if we can make it work well in with our requirements.
So, I'll lay out our situation and as stripped down requirements as possible and we can see if there's a better way?

Our organization runs multiple clusters per environment, currently ~20 of them. Clusters are provisioned on-demand by development teams using Terraform modules, which among many other things sets up the cluster and DNS zones, and kicks off installation of all "infra deployments" (log collection, metrics, etc etc). The last step is currently flux+helm-operator, but that can be kept open. From this we have two items:

• We must be able to create new clusters without involvement from the infra team
• There are some parameters to deployments that are only known once the cluster is provisioned
  After the cluster is available, the team will deploy their services. This can happen in a multitude of ways, with or without flux, so that isn't too relevant for the discussion atm.

How would you approach this? A non-goal is having to create new repos/branches/files whenever a cluster is created, but apart from that I do not think we have many hard requirements.

[jeff-minard-ck]

I want to chime in here that this is also a fully breaking change for our upgrade path to v2 of the operator as well. I'll give an example of how we utilize this in case there are alternatives which can provide the same level of utility.

We keep a config map like this:

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: namespace-cluster
  namespace: default
data:
  values.yaml: |-
    nodeCidr: 1.2.3.4/28

Then we have HelmReleases configured to reference that singular configmap from, basically, every namespace. This allows those charts to create NetworkPolicy's that need the nodepool cidr range, as an example. We have lots of other "cluster info" configuration values here as well, much like I suspect Carl is doing as well. This pattern is used across a dozen+ clusters to make it significantly more easy to update each cluster with a single configmap in each.

I pretty well grok the rationale for the removal -- our ecosystem has other controls in place around who/what is allowed to make HelmRelease objects to protect us from rouge secret stealing. Having a "you better know what you're doing" feature flag for this would be great.

[nij4t]

Supplying values from flux sources would be useful too.

We are currently using Flux v1 with Helm Operator. We use git repo as chart source and that git repo stores the umbrella charts which refer to the charts that are located in our chart museum and each chart has its own values file. This allows us to separate helm values files from helm release files. We use this approach because we want helm values to remain its original form, moreover having helm values in helm-release files seems bulky (in case your configuration is very descriptive), the same goes with configMaps and secrets. In addition, with helm values stored in clusters as configMaps and secrets, the namespaces look cluttered and busy.

[hiddeco]

This allows us to separate helm values files from helm release files.

This is precisely what the helm-controller's ConfigMap and Secret values feature was designed to solve.

In addition, with helm values stored in clusters as configMaps and secrets, the namespaces look cluttered and busy.

Is not a strong argument to make any changes. Helm storage gets put into Secret resources, so you are already cluttering your cluster by making use of Helm, and those resource types where specifically designed to solve the issues you are describing in Kubernetes. In addition, Kubernetes namespaces are designed to be cluttered, and it is advised to de-clutter them by making use of labels:

It is not necessary to use multiple namespaces just to separate slightly different resources, such as different versions of the same software: use labels to distinguish resources within the same namespace.
— Kubernetes documentation

[nij4t]

Is not a strong argument to make any changes. Helm storage gets put into Secret resources, so you are already cluttering your cluster by making use of Helm, and those resource types where specifically designed to solve the issues you are describing in Kubernetes. In addition, Kubernetes namespaces are designed to be cluttered, and it is advised to de-clutter them by making use of labels:

It is not necessary to use multiple namespaces just to separate slightly different resources, such as different versions of the same software: use labels to distinguish resources within the same namespace.
— Kubernetes documentation

Totally agreed with that statement. Yet, having configMaps and secrets for helm values that are used to generate configMaps and secrets seems kind of odd, doesn't it? 😃

Though, with old helm release spec we were able to reference a file in git repo and that is something that feels right. I think, in the context of gitops the state of the cluster must be stored in the git repo rather than in the cluster. Of course, those values could be stored as configMaps/secrets within the git repo and those resources would then be applied to the cluster. However, because of those odd specifics, my teammates are always getting confused. Even myself. I've been reading the docs to refresh my knowledge about those specifics.

I can't imagine with how many technical difficulties you have stumbled upon and what compromises you've made. Overall, that won't stop us from migrating all our infra repos to Flux v2. Currently, we are waiting for automation controller. That automation feature was, by the way, was the reason for why we have chosen Flux. We really like what you guys are doing with Flux, and we believe that you are making the right decisions.

[hiddeco]

Yet, having configMaps and secrets for helm values that are used to generate configMaps and secrets seems kind of odd, doesn't it?

I can't imagine with how many technical difficulties you have stumbled upon and what compromises you've made.

The helm-controller is in many ways the odd one out in the GitOps driven world, as it is designed to serve user needs (being able to make use of Helm charts), while also adhering to how Helm works (that being: integrating with the Helm storage and maintaining (almost full) feature parity with the helm binary, instead of just using helm template like some others do). This means that we inherit some things of Helm that are not really "GitOpsy".

The reason I strongly believe that this is justified is because it makes it both easy to migrate to the helm-controller, and move away from it, at any given moment. However, this also means you have to give your user a limited set of tools. Because if you continue to provide them new patterns for the issues they are trying to solve, they may end up attempting to use a spoon as a knife to cut their solution, instead of looking for better tooling (like for example Kustomize).

I've been reading the docs to refresh my knowledge about those specifics.

I think in the source-controller and helm-controller we have now created a fair balance in available options:

• You can now merge single values at a given path;
• We still support values from ConfigMap and Secret references;
• We support alternative values files available in the chart (directory)

Those combined should give you, as I read what you are trying to achieve, sufficient capabilities, and I think that (with maybe some slight restructuring) you even should be able to create an understandable solution.

We really like what you guys are doing with Flux, and we believe that you are making the right decisions.

Thank you very much 🌞

[nij4t]

Thank you for clarification. Indeed, we still are able to define the values file name. It is just moved to helm charts spec.

type HelmChartTemplateSpec struct {
        ...
	// Alternative values file to use as the default chart values, expected to be a
	// relative path in the SourceRef. Ignored when omitted.
	// +optional
	ValuesFile string `json:"valuesFile,omitempty"`
}

[arbourd]

It would be nice to be able to provide more than one values file if the chart is sourced from a GitRepository. For example:

apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: example
  namespace: default
spec:
  releaseName: example
  chart:
    spec:
      chart: charts/example
      sourceRef:
        kind: GitRepository
        name: example-repo
        namespace: flux-system
      valuesFile: 
        - charts/example/values.yaml
        - charts/example/values-extend.yaml
        - charts/example/values-production.yaml
  interval: 5m

The HelmRelease create function in the CLI has a many-values-files mapper:

flux2/cmd/flux/create_helmrelease.go

Lines 182 to 203 in 24fe74f

	for _, v := range helmReleaseArgs.valuesFile {
	data, err := ioutil.ReadFile(v)
	if err != nil {
	return fmt.Errorf("reading values from %s failed: %w", v, err)
	}
	jsonBytes, err := yaml.YAMLToJSON(data)
	if err != nil {
	return fmt.Errorf("converting values to JSON from %s failed: %w", v, err)
	}
	jsonMap := make(map[string]interface{})
	if err := json.Unmarshal(jsonBytes, &jsonMap); err != nil {
	return fmt.Errorf("unmarshaling values from %s failed: %w", v, err)
	}
	if valuesMap == nil {
	valuesMap = jsonMap
	} else {
	valuesMap = utils.MergeMaps(valuesMap, jsonMap)
	}
	}

Any reason why we couldn't encourage merging from multiple files from the same GitRepository (as Helm Release/Chart source)?

If I understand correctly the alternatives are:

• Merge the above 3 yaml files into a single file
  This is different than what we do now and makes it harder to diff during CI
• Use the ConfigMap generator
  This is pretty elegant but requires additional logic, storage and work to store the state of something that is already declared and available in the GitRepository.
• Declare overrides in values: in a HelmRelease
  This doesn't work well for us because we do lots of CI on the raw YAML.

Anything I'm missing? Thanks!

[hiddeco]

@arbourd can you please create a new issue for this in the helm-controller repository?

[arbourd]

For anyone else who comes here looking for the issue you can find it here: fluxcd/source-controller#291