feat: API for custom helm chart configuration

[supershal]

What problem does this PR solve?:
API to support custom helm chart configuration for addons

The addons can be installed and upgraded only during AfterControlPlaneInitialization or BeforeClusterUpgrade hook.
If an addon needs to upgraded without upgrading kubernetes cluster, the cluster object has to be modified so that it could trigger reconciliation of addons.
A separate controller will manage the upgrade of addons outside kubernetes upgrade. Check draft PR #740

Custom helm chart configuration allows to upgrade addons without kubernetes version upgrade.
The API clients can create custom ConfigMap including some or all addons configuration and update the Cluster variable clusterConfig.addons.helmChartConfig.configMapRef.name with the name of the local ConfigMap.

To install addons using custom helm configuration, specify following values:

apiVersion: cluster.x-k8s.io/v1beta1
kind: Cluster
metadata:
  name: <NAME>
spec:
  topology:
    variables:
      - name: clusterConfig
        value:
          addonsMetadata:
              configMapRef:
                name: <NAME>-helm-addons-config
          addons:

The format of the custom configmap

apiVersion: v1
kind: ConfigMap
metadata:
  name: <CLUSTER_NAME>-helm-addons-config-<uuid>
  namespace: <CLUSTER_NAMESPACE>
immutable: true
data:
  nutanix-ccm: |
    ChartName: nutanix-cloud-provider
    ChartVersion: 0.4.2
    RepositoryURL: https://nutanix.github.io/helm/
  nutanix-storage-csi: |
    ChartName: nutanix-csi-storage
    ChartVersion: 3.2.0
    RepositoryURL: https://nutanix.github.io/helm-releases/

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

Lets finalize API in this PR. I plan to maintain feature/addon-upgrade branch until next release and create all related changes as stack PRs.

[supershal]

not sure if this label is strictly needed. According to the Move documentation, object with either clusterctl.cluster.x-k8s.io/move: "" or direct/indirect ownerReference will be moved.
I am not sure if ownerReference is not explicitly added, it will be added indirectly when the configmap is referenced by the Cluster object.

[dkoshkin]

The ownerRef won't be added automatically, but I think the controller in CAREN should add it.

This is similar to what CAREN does with CCM/CSI Secrets and adds an ownerRef.
Then when all clusters that own are deleted the Secret (CM in this case) would be garbageCollected.

[supershal]

Agree.
is CAREN mutating webhook is the right place to the owner reference?

[manoj-nutanix]

Would it make sense to also define generic struct for addon config like -

type AddonConfig struct {
	ChartName string `json:"chartName,omitempty"`
	ChartVersion string `json:"chartVersion,omitempty"`
	RepositoryURL string `json:"repositoryURL,omitempty"`
}

Users can define above field in cluster spec and konvoy can use these values and generate ConfigMap for the same.

[manoj-nutanix]

nit: we should description for this field.

[dkoshkin]

I initially commented on this in the design doc and seeing it in code I'm still not sure about this field being part of the GenericAddons struct, which is inlined in Docker/AWS/NutanixAddons structs, that also have other addons structs (CSI/COSI) that would inherit this configuration.

I really think we should consider a higher level API for configuration, maybe something like:

addonsConfiguration:
  helm: # ie HelmAddon strategy
    configMapRef:

[supershal]

let me start a thread on this.

[dlipovetsky]

+1 to making this field a sibling of the addons field.

[dlipovetsky]

Seeing the two fields, I wouldn't understand which one is for addon configuration:

addons:
  cni:
addonsConfiguration:
  configMapRef:

Can we find another name for addonsConfiguration, something that communicates that it's a list of sources for all addons (even one's we're not enabling for the cluster).

Maybe addonsSources?

[dlipovetsky]

If we can represent multiple strategies (helm, CRS, etc) in the configmap itself, then we can avoid the helm field.

So, instead of

addonsConfiguration:
  helm:
    configMapRef:

We have

addonsConfiguration:
  configMapRef:

And we put all sources, for all strategies, in this one ConfigMap.

[dlipovetsky]

If addonsSources limits us too much, maybe addonsMetadata?

[supershal]

thanks for the feedback and inputs. I like addonsMetadata and top level field suggestion. I will update the PR with what we discussed.

[supershal]

Would it make sense to also define generic struct for addon config like -

type AddonConfig struct {
	ChartName string `json:"chartName,omitempty"`
	ChartVersion string `json:"chartVersion,omitempty"`
	RepositoryURL string `json:"repositoryURL,omitempty"`
}

Users can define above field in cluster spec and konvoy can use these values and generate ConfigMap for the same.

Initially we wanted to keep the API simple and ensure that the Chart versions that shipped with current version are compatible with helm values shipped with it and use sane defaults in the default-helm-config that we test with.
Also we template RepositoryURL for airgap env by default for addons management.
if we ask chart info in the API, it will be upto the Clients to figure out correct URL, version, name and values etc or they will have to parse sane defaults from default-helm-config and pass it here in the API.

We can revisit above suggestion in wake of upgrade usecase. I love to hear other opinions.

[faiq]

I wonder if we can enforce this format somehow without needing to jump to a whole CRD 🤔

just a thought i wanted to note down

[supershal]

as we discussed:

• The new CRD would look like HelmChartProxy and CAREN's job would be take data from this new CRD and set it in HelmChartProxy.
• We want to keep using default configurationdefault-helm-chart-config to make it easy for customers to not worry about chart retrival details and making mistakes.
• CRD will be additional API on top of CAREN API which we think might not worth to maintain or overkill at this moment.

We decided to have a structured types instead of map of string values in the configmap and expose it though Go external library for GO clients. We can document the type schema for the API client.
I will create a follow up documentation and PR to demo this.

[supershal]

Would it make sense to also define generic struct for addon config like -

type AddonConfig struct {
	ChartName string `json:"chartName,omitempty"`
	ChartVersion string `json:"chartVersion,omitempty"`
	RepositoryURL string `json:"repositoryURL,omitempty"`
}

Users can define above field in cluster spec and konvoy can use these values and generate ConfigMap for the same.

Initially we wanted to keep the API simple and ensure that the Chart versions that shipped with current version are compatible with helm values shipped with it and use sane defaults in the default-helm-config that we test with. Also we template RepositoryURL for airgap env by default for addons management. if we ask chart info in the API, it will be upto the Clients to figure out correct URL, version, name and values etc or they will have to parse sane defaults from default-helm-config and pass it here in the API.

We can revisit above suggestion in wake of upgrade usecase. I love to hear other opinions.

@manoj-nutanix We discuss above design in huddle (Sorry could not talk with you due to timezone difference, I can discuss outcome with you on Monday PST).
We decided to still keep the current design to use default helm chart configmap and its format instead of adding that details in the API explicitly. The user can very easily make mistake with providing chart name, URL and version and endup with a botched cluster. This is what team has learned in past with customers who use advanced features and provide untested versions, urls and end up with botched cluster.
We (CAREN) want to have control of the chart URLs and versions that it test and ships with. So we will keep API as simple as possible and use custom configmap for the Advanced Users.

[supershal]

We recently added changes to upgrade/update cluster with new cluster class to force reconcile. We will leverage this mechanism instead of creating new API variable. Closing this PR.