Skip to content

Conversation

@supershal
Copy link
Contributor

What problem does this PR solve?:

  • Stacked on feat: API for custom helm chart configuration #1015
  • Implements API defined in feat: API for custom helm chart configuration #1015
  • Modified HelmChartGetter to first search chart info in the ConfigMap provided with the cluster. If the configMap does not contain component key for an Addon's chart information then fall back to default-helm-addons-config that is shipped with CAREN.
  • Preserves existing behavior where addons are installed using default-helm-addons-config configmap
  • A webhook to ensure that the referenced ConfigMap exists.

How Has This Been Tested?:

[X] unit test for addon webhook
[]unit test for HelmChartGetter (will add new commit)
[]e2e tests with custom configmap (Will file stacked PR)
[X] manual test on Nutanix Cluster

  • Custom Configmap
apiVersion: v1
data:
  nutanix-ccm: |
    ChartName: nutanix-cloud-provider
    ChartVersion: 0.5.0
    RepositoryURL: 'https://nutanix.github.io/helm/'
kind: ConfigMap
metadata:
  name: shalin-upgrade-addons-helm-addons-config
  namespace: default
  • Cluster with variable
variables:
    - name: clusterConfig
      value:
        addons:
          helmChartConfig:
            configMapRef:
              name: shalin-upgrade-addons-helm-addons-config
...
...
  • HCP for Nutanix CCM using chart info from the custom configmap:
apiVersion: addons.cluster.x-k8s.io/v1alpha1
kind: HelmChartProxy
metadata:
  generation: 1
  name: nutanix-ccm-01949566-8814-7427-bec9-eecdfae20c0a
  namespace: default
  ...
spec:
  chartName: nutanix-cloud-provider
  releaseName: nutanix-ccm
  repoURL: https://nutanix.github.io/helm/
  ...
  version: 0.5.0
status:
  conditions:
  - lastTransitionTime: "2025-01-23T23:03:32Z"
    status: "True"
    type: Ready
  ...
  matchingClusters:
  - apiVersion: cluster.x-k8s.io/v1beta1
    kind: Cluster
    name: shalin-upgrade-addons
    namespace: default
  observedGeneration: 1 
  • HCP for addon (ex. Cilium) NOT referenced in the custom configmap. It uses info from default helm chart configmap
apiVersion: addons.cluster.x-k8s.io/v1alpha1
kind: HelmChartProxy
metadata:
  generation: 1
  name: cilium-01949566-8814-7427-bec9-eecdfae20c0a
  namespace: default
  ...
spec:
  chartName: cilium
  releaseName: cilium
  repoURL: oci://helm-repository.default.svc/charts
  ...
  version: 1.16.4
status:
  conditions:
  - lastTransitionTime: "2025-01-23T23:06:12Z"
    status: "True"
    type: Ready
  ...
  matchingClusters:
  - apiVersion: cluster.x-k8s.io/v1beta1
    kind: Cluster
    name: shalin-upgrade-addons
    namespace: default
  observedGeneration: 1

Special notes for your reviewer:

The discussion for the API is still going on at #1015
While main logic would remain same, if any changes proposed in the API, I will update this PR with the implementation.

Open question:
Should we set owner reference in the referenced configmap?

@supershal supershal self-assigned this Jan 24, 2025
@supershal supershal changed the title feat: create cluster with custom helm chart config feat(stacked-PR): create cluster with custom helm chart config Jan 24, 2025
@github-actions github-actions bot added feature and removed feature labels Jan 24, 2025
@supershal supershal force-pushed the shalin/addon-upgrade-impl branch from 0563813 to 6d76d85 Compare January 24, 2025 19:16
cmNameFromVariables := helmChartConfig.ConfigMapRef.Name
clusterNamespace := cluster.Namespace
hcGetter := NewHelmChartGetterFromConfigMap(cmNameFromVariables, clusterNamespace, h.cl)
overrideHelmChart, err := hcGetter.getInfoFor(ctx, log, name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helmChartFromCustomConfigMap would be a better name IMO. I had to think about what "override" meant in this context

overrideHelmChart, err := hcGetter.getInfoFor(ctx, log, name)
if err != nil {
if errors.As(err, &HelmChartInfoNotFoundError{}) {
// HelmChart is not defined in the custom configmap. Get the HelmChart from the default configmap.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth logging a warn

@supershal supershal force-pushed the feature/addon-upgrade branch from 437e52b to ad5da85 Compare February 1, 2025 01:27
@supershal
Copy link
Contributor Author

we are going with different API design. Closing this PR. Thanks for the review @faiq

@supershal supershal closed this Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants