Add `templateValues` to HelmApp, Bundle, etc #3267

Danil-Grigorev · 2025-01-28T13:01:06Z

Refers to #3266

Implement new field inside Helm options: templateValues, which performs templating of each individual value before converting data to yaml. This allows to use more complex template expressions then currently available to template string data, like creation of maps and lists.

Danil-Grigorev · 2025-01-28T15:12:12Z

With this change I was able to deploy helm chart with proper modifications:

apiversion: fleet.cattle.io/v1alpha1
kind: HelmApp
metadata:
  finalizers:
  - fleet.cattle.io/helmapp-finalizer
  generation: 3
  name: sample1
  namespace: default
  resourceVersion: "23694"
  uid: 511580b5-733a-4304-9cbb-229448989a3a
spec:
  helm:
    chart: tigera-operator
    releaseName: projectcalico
    repo: https://docs.tigera.io/calico/charts
    templateValues:
      installation: |-
        cni:
          type: Calico
          ipam:
            type: HostLocal
        calicoNetwork:
          bgp: Disabled
          mtu: 1350
          ipPools:
            ${- range $cidr := .ClusterValues.Cluster.spec.clusterNetwork.pods.cidrBlocks }
            - cidr: "${ $cidr }"
              encapsulation: None
              natOutgoing: Enabled
              nodeSelector: all()${- end}
  ignore: {}
  insecureSkipTLSVerify: true
  targets:
  - clusterName: docker-demo

created correct BundleDeployment:

apiversion: fleet.cattle.io/v1alpha1
kind: BundleDeployment
metadata:
  creationTimestamp: "2025-01-28T15:00:20Z"
  finalizers:
  - fleet.cattle.io/bundle-deployment-finalizer
  generation: 1
  labels:
    fleet.cattle.io/bundle-name: sample1
    fleet.cattle.io/bundle-namespace: default
    fleet.cattle.io/cluster: docker-demo
    fleet.cattle.io/cluster-namespace: default
    fleet.cattle.io/fleet-helm-name: sample1
    fleet.cattle.io/managed: "true"
  name: sample1
  namespace: cluster-default-docker-demo-9a1d57cc2ad8
  resourceVersion: "22820"
  uid: 37600a86-c9e4-4445-a757-5611c5d27ace
spec:
  deploymentID: :fb788a089566f35d87c7d4b2ed4a499a2d3e36315b44cfa9446f66996b6a7c82
  helmChartOptions:
    helmAppInsecureSkipTLSVerify: true
  options:
    helm:
      chart: tigera-operator
      releaseName: projectcalico
      repo: https://docs.tigera.io/calico/charts
      templateValues:
        installation: |-
          cni:
            type: Calico
            ipam:
              type: HostLocal
          calicoNetwork:
            bgp: Disabled
            mtu: 1350
            ipPools:
              ${- range $cidr := .ClusterValues.Cluster.spec.clusterNetwork.pods.cidrBlocks }
              - cidr: "${ $cidr }"
                encapsulation: None
                natOutgoing: Enabled
                nodeSelector: all()${- end}
      values:
        installation:
          calicoNetwork:
            bgp: Disabled
            ipPools:
            - cidr: 10.1.0.0/16
              encapsulation: None
              natOutgoing: Enabled
              nodeSelector: all()
            mtu: 1350
          cni:
            ipam:
              type: HostLocal
            type: Calico
      version: v3.29.1
    ignore: {}
  stagedDeploymentID: :fb788a089566f35d87c7d4b2ed4a499a2d3e36315b44cfa9446f66996b6a7c82
…

weyfonk

I like the idea, but I wonder if we really need both values and templateValues; perhaps we could template each member of the existing value field before serialising it to YAML, WDYT?

weyfonk · 2025-01-29T15:11:08Z

internal/cmd/controller/target/builder.go

+	if opts.Helm.Values.Data == nil {
+		opts.Helm.Values.Data = map[string]interface{}{}
+	}


Why do we still need this block? My understanding is that cmp.Or would populate the Data field if needed 🤔

weyfonk · 2025-01-29T15:13:29Z

internal/cmd/controller/target/builder.go

+		opts.Helm.Values.Data = map[string]interface{}{}
+	}
+
+	if opts.Helm.TemplateValues == nil && (opts.Helm.Values == nil || opts.Helm.Values.Data == nil) {


Since we've ensured that opts.Helm.Values.Data exists, wouldn't the following be enough?

Suggested change

if opts.Helm.TemplateValues == nil && (opts.Helm.Values == nil || opts.Helm.Values.Data == nil) {

if opts.Helm.TemplateValues == nil && opts.Helm.Values.Data == nil {

weyfonk · 2025-01-29T15:18:38Z

internal/cmd/controller/target/builder.go

+			return err
+		}
+
+		maps.Copy(opts.Helm.Values.Data, templatedData)


This effectively means that, if both a value and a template value exist with the same key, the latter will overwrite the former. Not a problem, but something we'll want to document, and ideally test :)

I updated the doc message on the field. Not sure we need to test this, since the method is a part of standard lib, but added a scenario

weyfonk · 2025-01-29T15:26:10Z

internal/cmd/controller/target/builder.go

+		var value interface{}
+		err = kyaml.Unmarshal(b.Bytes(), &value)
+		if err != nil {
+			return nil, fmt.Errorf("failed to interpret rendered template as helm values: %#v, %v", renderedValues, err)


Why output the whole set of rendered values here? Is the rationale to enable the user to understand where successful processing stopped? Otherwise, how about outputting b.String() (or rather a string conversion of the previous b.Bytes())?

weyfonk · 2025-01-29T15:37:39Z

internal/cmd/controller/target/target_test.go

@@ -314,6 +329,70 @@ func TestProcessTemplateValues(t *testing.T) {
 		t.Fatal("join func was not right")
 	}

+	templatedValuesData, err := processTemplateValuesData(bundle.Helm.TemplateValues, values)
+	if err != nil {
+		t.Fatalf("error during label processing %v", err)


🤨 Not sure why that error message is phrased this way (same for processTemplateValues)

Suggested change

t.Fatalf("error during label processing %v", err)

t.Fatalf("error during templated values processing %v", err)

Danil-Grigorev · 2025-01-29T17:32:49Z

I like the idea, but I wonder if we really need both values and templateValues; perhaps we could template each member of the existing value field before serialising it to YAML, WDYT?

I also thought about it, but that has the same problem as previous solution. As the value in the map is interface{}, what type it actually contains is unknown. Template processing requires string data, which is only achievable after serialization - so it has to go through yaml decoder. These template expressions do not render as valid yaml, so we can't do anything.

Even if the interface is initially checked to be string, and we can skip yaml pre-processing, it is unclear if the templated result needs to stay a sting, or it has to be a map/list. templatedValues makes an assumption that the string inside will contain raw data (map/list). So I made a field addition to make it distinct in order to avoid breaking changes in API.

Maybe there is a way around the issue I'm not seeing here.

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

manno · 2025-01-31T13:45:08Z

installation: |-

I think this is supposed to be installation:, like in the issue. Otherwise, within a string value, the old approach should work.

Danil-Grigorev · 2025-01-31T16:10:08Z

Having the value specified as

values:
  installation: |-

Results in error on HelmApp, which is only due to invalid type passed into installation, a string:

status:
  conditions:
  - lastUpdateTime: "2025-01-31T15:55:58Z"
    message: 'ErrApplied(1) [Bundle sample1: template: tigera-operator/templates/tigera-operator/02-tigera-operator.yaml:81:17:
      executing "tigera-operator/templates/tigera-operator/02-tigera-operator.yaml"
      at <.Values.installation.kubernetesProvider>: can''t evaluate field kubernetesProvider
      in type interface {}]'
    status: "False"
    type: Ready

If I instead switch to just installation: in the template and apply again, it is invalid YAML:

dgrigorev@localhost:~/Documents/work/cluster-api-addon-provider-fleet> kubectl apply -f helm.yaml 
error: error parsing helm.yaml: error converting YAML to JSON: yaml: line 21: mapping values are not allowed in this context

Then if I go to generated BundleDeployment and edit it so it contains no |- in it, the rollout succeeds. It also succeeds with correct configuration (note the cidr field):

kind: Installation
metadata:
  annotations:
    meta.helm.sh/release-name: projectcalico
    meta.helm.sh/release-namespace: default
    objectset.rio.cattle.io/id: default-sample1-fleet-addon-agent
  creationTimestamp: "2025-01-31T16:04:01Z"
  finalizers:
  - operator.tigera.io/installation-controller
  - tigera.io/operator-cleanup
  - operator.tigera.io/apiserver-controller
  generation: 4
  labels:
    app.kubernetes.io/managed-by: Helm
    objectset.rio.cattle.io/hash: cf1aceee4a248c0997592ec3509a41cb53a2fece
  name: default
  resourceVersion: "2589"
  uid: 9022fd0d-ea88-4d04-ac1a-ebc322adfc98
spec:
  calicoNetwork:
    bgp: Disabled
    hostPorts: Enabled
    ipPools:
    - allowedUses:
      - Workload
      - Tunnel
      blockSize: 26
      cidr: 10.1.0.0/16

None of this is needed in updated templateValues implementation.

manno · 2025-01-31T16:40:25Z

Having the value specified as

Yes, I think I understood that. But in your comment you are using a string field. I would have thought you want a map there.

manno

Merging this early for an alpha release

Danil-Grigorev requested a review from a team as a code owner January 28, 2025 13:01

Danil-Grigorev force-pushed the native-template-value-support branch 2 times, most recently from 21e3d46 to 5dbcbc0 Compare January 28, 2025 15:07

Danil-Grigorev changed the title ~~Add templateValues to HelmApp, Bundle, etc~~ Add templateValues to HelmApp, Bundle, etc Jan 28, 2025

kkaempf added the kind/enhancement label Jan 29, 2025

kkaempf added this to the v2.11.0 milestone Jan 29, 2025

weyfonk reviewed Jan 29, 2025

View reviewed changes

Danil-Grigorev force-pushed the native-template-value-support branch 3 times, most recently from 126b010 to f308721 Compare January 30, 2025 10:50

Add templateValues to HelmApp, Bundle, etc

86be9f3

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

Danil-Grigorev force-pushed the native-template-value-support branch from f308721 to 86be9f3 Compare January 30, 2025 10:54

Merge branch 'main' into native-template-value-support

4792bf2

manno approved these changes Jan 31, 2025

View reviewed changes

manno merged commit 44c9815 into rancher:main Jan 31, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `templateValues` to HelmApp, Bundle, etc #3267

Add `templateValues` to HelmApp, Bundle, etc #3267

Danil-Grigorev commented Jan 28, 2025

Danil-Grigorev commented Jan 28, 2025

weyfonk left a comment

weyfonk Jan 29, 2025

weyfonk Jan 29, 2025

weyfonk Jan 29, 2025

Danil-Grigorev Jan 30, 2025

weyfonk Jan 29, 2025

weyfonk Jan 29, 2025

Danil-Grigorev commented Jan 29, 2025 •

edited

Loading

manno commented Jan 31, 2025

Danil-Grigorev commented Jan 31, 2025

manno commented Jan 31, 2025

manno left a comment

	if opts.Helm.TemplateValues == nil && (opts.Helm.Values == nil \|\| opts.Helm.Values.Data == nil) {
	if opts.Helm.TemplateValues == nil && opts.Helm.Values.Data == nil {

	t.Fatalf("error during label processing %v", err)
	t.Fatalf("error during templated values processing %v", err)

Add templateValues to HelmApp, Bundle, etc #3267

Add templateValues to HelmApp, Bundle, etc #3267

Conversation

Danil-Grigorev commented Jan 28, 2025

Danil-Grigorev commented Jan 28, 2025

weyfonk left a comment

Choose a reason for hiding this comment

weyfonk Jan 29, 2025

Choose a reason for hiding this comment

weyfonk Jan 29, 2025

Choose a reason for hiding this comment

weyfonk Jan 29, 2025

Choose a reason for hiding this comment

Danil-Grigorev Jan 30, 2025

Choose a reason for hiding this comment

weyfonk Jan 29, 2025

Choose a reason for hiding this comment

weyfonk Jan 29, 2025

Choose a reason for hiding this comment

Danil-Grigorev commented Jan 29, 2025 • edited Loading

manno commented Jan 31, 2025

Danil-Grigorev commented Jan 31, 2025

manno commented Jan 31, 2025

manno left a comment

Choose a reason for hiding this comment

Add `templateValues` to HelmApp, Bundle, etc #3267

Add `templateValues` to HelmApp, Bundle, etc #3267

Danil-Grigorev commented Jan 29, 2025 •

edited

Loading