FR: Add spec.values to the HelmChart CRD for structured YAML support

[aofei]

Introduction

Currently, the HelmChart CRD supports specifying Chart values via spec.valuesContent as a raw string. While effective, this method lacks the ability to leverage YAML syntax checking when editing manifests with editors. This proposal suggests an enhancement to the HelmChart CRD to address this limitation.

The primary goal of this proposal is to introduce a new field, spec.values, to the HelmChart CRD. This field will allow users to provide Chart values as a YAML document rather than a raw string. This enhancement aims to improve the user experience by enabling YAML syntax validation during the editing process.

Proposed Changes

1. Introduction of spec.values

Add a new field spec.values to the HelmChart CRD. This field should accept a YAML document representing the Chart values.

2. Precedence rule

In cases where both spec.values and spec.valuesContent are provided, spec.valuesContent will take precedence (just like spec.chart and spec.chartContent). This ensures backward compatibility and provides flexibility for users who prefer the raw string format.

3. Implementation

Perhaps introduce a new field Values to the pkg/apis/helm.cattle.io/v1. HelmChartSpec struct:

import "encoding/json"

type HelmChartSpec struct {
	// ...
	Values *json.RawMessage `json:"values,omitempty"`
}

[brandond]

this method lacks the ability to leverage YAML syntax checking when editing manifests with editors

Values *json.RawMessage `json:"values,omitempty"`

How is this handled in the generated OpenAPI spec? What specific editors handle this field type, and how does that enhance the display and validation of the resource as a whole?

[tdemin]

+1. This would be particularly useful when using Kustomize to generate/override complex host/cluster-specific configurations (it can merge YAML documents, but not strings containing YAML). The existing CRDs only cover up to two levels of YAML-level merging, at HelmChart.spec.valuesContent and HelmChartConfig.spec.valuesContent respectively.

One more thing: this should apply not just to helm.cattle.io/v1.HelmChart but to HelmChartConfig too.

[brandond]

Can Kustomize merge arbitrary yaml, or does it need the document to have an OpenAPI Spec? If it requires a spec to know how to handle merging, then just changing it to json.RawMessage won't improve anything, as the values yaml does not have a fixed schema.

[tdemin]

@brandond Kustomize can merge arbitrary YAML, provided it's YAML, and not a string. While having specs improves strategic merge for complex cases, it is in no way necessary and Kustomize will happily do without it.

[aofei]

How is this handled in the generated OpenAPI spec?

Yeah, I was wrong. json.RawMessage will be represented as a string type in the generated OpenAPI spec:

helmcharts.helm.cattle.io

spec:
  versions:
    - name: v1
      schema:
        openAPIV3Schema:
          properties:
            spec:
              type: object
              properties:
                values:
                  nullable: true
                  type: string

Referring to this, it seems that HelmChartSpec.Values should be of type runtime.RawExtension. However, I don't know how to set x-kubernetes-preserve-unknown-fields: true for it.

What specific editors handle this field type, and how does that enhance the display and validation of the resource as a whole?

Actually my motivation stemmed from a YAML syntax error in spec.valuesContent that I overlooked, as raw string values lacked syntax checking. So all I want initially was basic YAML syntax checking and highlighting. But @tdemin came up with something more interesting.

[brandond]

I'm not sure there is an unstructured type that will get you exactly what you want. Can you provide an example of another customer resource definition that does what you want in your editor of choice?

[aofei]

Cert-manager has something similar. Their webhook Issuer has a config field that supports arbitrary YAML. But instead of using runtime.RawExtension, it uses k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON. (webhook Issuer related code in cert-manager)

I believe this is what we want as x-kubernetes-preserve-unknown-fields: true will be added automatically.

import apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"

type HelmChartSpec struct {
	// ...
	Values *apiextensionsv1.JSON `json:"values,omitempty"`
}

spec:
  versions:
    - name: v1
      schema:
        openAPIV3Schema:
          properties:
            spec:
              type: object
              properties:
                values:
                  nullable: true
                  type: object
                  x-kubernetes-preserve-unknown-fields: true

[tdemin]

ArgoCD does it like that for their argoproj.io/v1alpha1.Application.spec.source.helm.valuesObject (and a multitude of fields elsewhere):

                                    valuesObject:
                                      description: ValuesObject specifies Helm values
                                        to be passed to helm template, defined as
                                        a map. This takes precedence over Values.
                                      type: object
                                      x-kubernetes-preserve-unknown-fields: true

They seem to be using *runtime.RawExtension:

type ApplicationSourceHelm struct {
...
	// ValuesObject specifies Helm values to be passed to helm template, defined as a map. This takes precedence over Values.
	// +kubebuilder:pruning:PreserveUnknownFields
	ValuesObject *runtime.RawExtension `json:"valuesObject,omitempty" protobuf:"bytes,10,opt,name=valuesObject"`
}

[aofei]

From my testing, the biggest problem with runtime.RawExtension is that it requires the +kubebuilder:pruning:PreserveUnknownFields annotation to work, which is not supported by helm-controller.

So k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON should be what we're looking for.

[brandond]

We use rancher/wrangler to generate the CRDs, and there is a way to get that annotation set on the OpenAPI spec for a field using tags. I will take a look at it when I have some time.