fix: Handle long cluster names

This commit starts fixing a bug that means
addons fail to be fully deployed if the cluster name
is longer than 44 characters. This is caused by the name
of the HCP being over 63 characters. This name is then
used in HRP labels which have a maximum length of 63
characters, so the HRPs are rejected by the API server
when CAAPH applies them.

The fix is to use generate name and labels on the
HCP to ensure uniqueness by lookup rather than by using
a deterministic name.

Author: jimmidyson

pkg/handlers/generic/lifecycle/addons/helmaddon.go

@@ -5,6 +5,7 @@ package addons
 
 import (
 	"context"
+	"crypto/sha256"
 	"fmt"
 
 	"github.com/go-logr/logr"
@@ -20,6 +21,11 @@ import (
 	handlersutils "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/utils"
 )
 
+var (
+	HelmReleaseNameHashLabel = "addons.cluster.x-k8s.io/helm-release-name-hash"
+	ClusterNamespaceLabel    = clusterv1.ClusterNamespaceAnnotation
+)
+
 type HelmAddonConfig struct {
 	defaultValuesTemplateConfigMapName string
 
@@ -52,6 +58,7 @@ type helmAddonApplier struct {
 	config    *HelmAddonConfig
 	client    ctrlclient.Client
 	helmChart *lifecycleconfig.HelmChart
+	opts      []applyOption
 }
 
 var _ Applier = &helmAddonApplier{}
@@ -68,12 +75,48 @@ func NewHelmAddonApplier(
 	}
 }
 
+type applyOptions struct {
+	valueTemplater func(cluster *clusterv1.Cluster, valuesTemplate string) (string, error)
+	targetCluster  *clusterv1.Cluster
+}
+
+type applyOption func(*applyOptions)
+
+func (a *helmAddonApplier) WithValueTemplater(
+	valueTemplater func(cluster *clusterv1.Cluster, valuesTemplate string) (string, error),
+) *helmAddonApplier {
+	a.opts = append(a.opts, func(o *applyOptions) {
+		o.valueTemplater = valueTemplater
+	})
+
+	return a
+}
+
+func (a *helmAddonApplier) WithTargetCluster(cluster *clusterv1.Cluster) *helmAddonApplier {
+	a.opts = append(a.opts, func(o *applyOptions) {
+		o.targetCluster = cluster
+	})
+
+	return a
+}
+
 func (a *helmAddonApplier) Apply(
 	ctx context.Context,
 	cluster *clusterv1.Cluster,
 	defaultsNamespace string,
 	log logr.Logger,
 ) error {
+	applyOpts := &applyOptions{}
+	for _, opt := range a.opts {
+		opt(applyOpts)
+	}
+
+	log.Info("Checking for existing HelmChartProxy for cluster")
+	chartProxy, err := a.FindExistingHelmChartProxy(ctx, cluster)
+	if err != nil {
+		return fmt.Errorf("failed to lookup existing HelmChartProxy for cluster: %w", err)
+	}
+
 	log.Info("Retrieving installation values template for cluster")
 	values, err := handlersutils.RetrieveValuesTemplate(
 		ctx,
@@ -88,39 +131,115 @@ func (a *helmAddonApplier) Apply(
 		)
 	}
 
-	chartProxy := &caaphv1.HelmChartProxy{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: caaphv1.GroupVersion.String(),
-			Kind:       "HelmChartProxy",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: cluster.Namespace,
-			Name:      a.config.defaultHelmReleaseName + "-" + cluster.Name,
-		},
-		Spec: caaphv1.HelmChartProxySpec{
-			RepoURL:   a.helmChart.Repository,
-			ChartName: a.helmChart.Name,
-			ClusterSelector: metav1.LabelSelector{
-				MatchLabels: map[string]string{clusterv1.ClusterNameLabel: cluster.Name},
+	if applyOpts.valueTemplater != nil {
+		values, err = applyOpts.valueTemplater(cluster, values)
+		if err != nil {
+			return fmt.Errorf("failed to template Helm values: %w", err)
+		}
+	}
+
+	targetCluster := cluster
+	if applyOpts.targetCluster != nil {
+		targetCluster = applyOpts.targetCluster
+	}
+
+	if chartProxy == nil {
+		chartProxy = &caaphv1.HelmChartProxy{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: caaphv1.GroupVersion.String(),
+				Kind:       "HelmChartProxy",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: targetCluster.Namespace,
+				Labels: map[string]string{
+					clusterv1.ClusterNameLabel: cluster.Name,
+					ClusterNamespaceLabel:      cluster.Namespace,
+					// Label values have a maximum length of 63 characters so hash the release name to
+					// ensure it fits within the limit.
+					HelmReleaseNameHashLabel: hashReleaseName(a.config.defaultHelmReleaseName),
+				},
+				GenerateName: fmt.Sprintf("%s-", a.config.defaultHelmReleaseName),
 			},
-			ReleaseNamespace: a.config.defaultHelmReleaseNamespace,
-			ReleaseName:      a.config.defaultHelmReleaseName,
-			Version:          a.helmChart.Version,
-			ValuesTemplate:   values,
+		}
+	}
+
+	chartProxy.Spec = caaphv1.HelmChartProxySpec{
+		RepoURL:   a.helmChart.Repository,
+		ChartName: a.helmChart.Name,
+		ClusterSelector: metav1.LabelSelector{
+			MatchLabels: map[string]string{clusterv1.ClusterNameLabel: targetCluster.Name},
 		},
+		ReleaseNamespace: a.config.defaultHelmReleaseNamespace,
+		ReleaseName:      a.config.defaultHelmReleaseName,
+		Version:          a.helmChart.Version,
+		ValuesTemplate:   values,
 	}
+
 	handlersutils.SetTLSConfigForHelmChartProxyIfNeeded(chartProxy)
-	if err = controllerutil.SetOwnerReference(cluster, chartProxy, a.client.Scheme()); err != nil {
+	if err = controllerutil.SetOwnerReference(targetCluster, chartProxy, a.client.Scheme()); err != nil {
 		return fmt.Errorf(
 			"failed to set owner reference on HelmChartProxy %q: %w",
 			chartProxy.Name,
 			err,
 		)
 	}
 
-	if err = k8sclient.ServerSideApply(ctx, a.client, chartProxy, k8sclient.ForceOwnership); err != nil {
-		return fmt.Errorf("failed to apply HelmChartProxy %q: %w", chartProxy.Name, err)
+	// Only server-side apply the HelmChartProxy if it already existed, i.e. that metadata.name is non-empty.
+	// This allows to use metadata.generateName for the first creation to avoid naming collisions.
+	if chartProxy.Name == "" {
+		if err = a.client.Create(ctx, chartProxy); err != nil {
+			return fmt.Errorf("failed to create HelmChartProxy %q: %w", chartProxy.Name, err)
+		}
+	} else {
+		// metadata.managedFields must be nil when using server-side apply.
+		chartProxy.ManagedFields = nil
+		if err = k8sclient.ServerSideApply(ctx, a.client, chartProxy, k8sclient.ForceOwnership); err != nil {
+			return fmt.Errorf("failed to apply HelmChartProxy %q: %w", chartProxy.Name, err)
+		}
 	}
 
 	return nil
 }
+
+func (a *helmAddonApplier) FindExistingHelmChartProxy(
+	ctx context.Context, cluster *clusterv1.Cluster,
+) (*caaphv1.HelmChartProxy, error) {
+	applyOpts := &applyOptions{}
+	for _, opt := range a.opts {
+		opt(applyOpts)
+	}
+
+	targetCluster := cluster
+	if applyOpts.targetCluster != nil {
+		targetCluster = applyOpts.targetCluster
+	}
+
+	chartProxyList := &caaphv1.HelmChartProxyList{}
+	if err := a.client.List(
+		ctx,
+		chartProxyList,
+		ctrlclient.MatchingLabels{
+			clusterv1.ClusterNameLabel: cluster.Name,
+			ClusterNamespaceLabel:      cluster.Namespace,
+			HelmReleaseNameHashLabel:   hashReleaseName(a.config.defaultHelmReleaseName),
+		},
+		ctrlclient.InNamespace(targetCluster.Namespace),
+	); err != nil {
+		return nil, fmt.Errorf("failed to list HelmChartProxies: %w", err)
+	}
+
+	if len(chartProxyList.Items) == 0 {
+		return nil, nil
+	}
+
+	if len(chartProxyList.Items) > 1 {
+		return nil, fmt.Errorf("found multiple HelmChartProxies for cluster %q", cluster.Name)
+	}
+
+	return &chartProxyList.Items[0], nil
+}
+
+func hashReleaseName(releaseName string) string {
+	// Use Sum224 to ensure the hash is 56 characters long.
+	return fmt.Sprintf("%x", sha256.Sum224([]byte(releaseName)))
+}

pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go

@@ -12,14 +12,11 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
-	caaphv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/external/sigs.k8s.io/cluster-api-addon-provider-helm/api/v1alpha1"
 	apivariables "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/variables"
-	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/k8s/client"
+	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/lifecycle/addons"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/lifecycle/config"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/options"
 	handlersutils "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/utils"
@@ -80,25 +77,11 @@ func (p *provider) Apply(
 		return ErrMissingCredentials
 	}
 
-	log.Info("Retrieving Nutanix CCM installation values template for cluster")
-	values, err := handlersutils.RetrieveValuesTemplate(
-		ctx,
-		p.client,
-		p.config.defaultValuesTemplateConfigMapName,
-		p.config.DefaultsNamespace(),
-	)
-	if err != nil {
-		return fmt.Errorf(
-			"failed to retrieve Nutanix CCM installation values template for cluster: %w",
-			err,
-		)
-	}
-
 	// It's possible to have the credentials Secret be created by the Helm chart.
 	// However, that would leave the credentials visible in the HelmChartProxy.
 	// Instead, we'll create the Secret on the remote cluster and reference it in the Helm values.
 	if clusterConfig.Addons.CCM.Credentials != nil {
-		err = handlersutils.EnsureOwnerReferenceForSecret(
+		err := handlersutils.EnsureOwnerReferenceForSecret(
 			ctx,
 			p.client,
 			clusterConfig.Addons.CCM.Credentials.SecretRef.Name,
@@ -134,77 +117,56 @@ func (p *provider) Apply(
 		return fmt.Errorf("failed to get values for nutanix-ccm-config: %w", err)
 	}
 
-	// The configMap will contain the Helm values, but templated with fields that need to be filled in.
-	values, err = templateValues(clusterConfig, values)
-	if err != nil {
-		return fmt.Errorf("failed to template Helm values read from ConfigMap: %w", err)
-	}
-
-	hcp := &caaphv1.HelmChartProxy{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: caaphv1.GroupVersion.String(),
-			Kind:       "HelmChartProxy",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: cluster.Namespace,
-			Name:      "nutanix-ccm-" + cluster.Name,
-		},
-		Spec: caaphv1.HelmChartProxySpec{
-			RepoURL:   helmChart.Repository,
-			ChartName: helmChart.Name,
-			ClusterSelector: metav1.LabelSelector{
-				MatchLabels: map[string]string{clusterv1.ClusterNameLabel: cluster.Name},
-			},
-			ReleaseNamespace: defaultHelmReleaseNamespace,
-			ReleaseName:      defaultHelmReleaseName,
-			Version:          helmChart.Version,
-			ValuesTemplate:   values,
-		},
-	}
-	handlersutils.SetTLSConfigForHelmChartProxyIfNeeded(hcp)
-	if err = controllerutil.SetOwnerReference(cluster, hcp, p.client.Scheme()); err != nil {
-		return fmt.Errorf(
-			"failed to set owner reference on nutanix-ccm installation HelmChartProxy: %w",
-			err,
-		)
-	}
+	applier := addons.NewHelmAddonApplier(
+		addons.NewHelmAddonConfig(
+			p.config.defaultValuesTemplateConfigMapName,
+			defaultHelmReleaseNamespace,
+			defaultHelmReleaseName,
+		),
+		p.client,
+		helmChart,
+	).WithValueTemplater(templateValuesFunc(clusterConfig))
 
-	if err = client.ServerSideApply(ctx, p.client, hcp, client.ForceOwnership); err != nil {
+	if err = applier.Apply(ctx, cluster, p.config.DefaultsNamespace(), log); err != nil {
 		return fmt.Errorf("failed to apply nutanix-ccm installation HelmChartProxy: %w", err)
 	}
 
 	return nil
 }
 
-func templateValues(clusterConfig *apivariables.ClusterConfigSpec, text string) (string, error) {
-	helmValuesTemplate, err := template.New("").Parse(text)
-	if err != nil {
-		return "", fmt.Errorf("failed to parse Helm values template: %w", err)
-	}
+func templateValuesFunc(
+	clusterConfig *apivariables.ClusterConfigSpec,
+) func(*clusterv1.Cluster, string) (string, error) {
+	return func(_ *clusterv1.Cluster, valuesTemplate string) (string, error) {
+		helmValuesTemplate, err := template.New("").Parse(valuesTemplate)
+		if err != nil {
+			return "", fmt.Errorf("failed to parse Helm values template: %w", err)
+		}
 
-	type input struct {
-		PrismCentralHost                  string
-		PrismCentralPort                  int32
-		PrismCentralInsecure              bool
-		PrismCentralAdditionalTrustBundle string
-	}
+		type input struct {
+			PrismCentralHost                  string
+			PrismCentralPort                  int32
+			PrismCentralInsecure              bool
+			PrismCentralAdditionalTrustBundle string
+		}
 
-	address, port, err := clusterConfig.Nutanix.PrismCentralEndpoint.ParseURL()
-	if err != nil {
-		return "", err
-	}
-	templateInput := input{
-		PrismCentralHost:                  address,
-		PrismCentralPort:                  port,
-		PrismCentralInsecure:              clusterConfig.Nutanix.PrismCentralEndpoint.Insecure,
-		PrismCentralAdditionalTrustBundle: clusterConfig.Nutanix.PrismCentralEndpoint.AdditionalTrustBundle,
-	}
+		address, port, err := clusterConfig.Nutanix.PrismCentralEndpoint.ParseURL()
+		if err != nil {
+			return "", err
+		}
+		templateInput := input{
+			PrismCentralHost:                  address,
+			PrismCentralPort:                  port,
+			PrismCentralInsecure:              clusterConfig.Nutanix.PrismCentralEndpoint.Insecure,
+			PrismCentralAdditionalTrustBundle: clusterConfig.Nutanix.PrismCentralEndpoint.AdditionalTrustBundle,
+		}
 
-	var b bytes.Buffer
-	err = helmValuesTemplate.Execute(&b, templateInput)
-	if err != nil {
-		return "", fmt.Errorf("failed setting PrismCentral configuration in template: %w", err)
-	}
+		var b bytes.Buffer
+		err = helmValuesTemplate.Execute(&b, templateInput)
+		if err != nil {
+			return "", fmt.Errorf("failed setting PrismCentral configuration in template: %w", err)
+		}
 
-	return b.String(), nil
+		return b.String(), nil
+	}
 }

pkg/handlers/generic/lifecycle/ccm/nutanix/handler_test.go

@@ -123,7 +123,7 @@ func Test_templateValues(t *testing.T) {
 		tt := tests[idx]
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			out, err := templateValues(tt.clusterConfig, tt.in)
+			out, err := templateValuesFunc(tt.clusterConfig)(nil, tt.in)
 			require.NoError(t, err)
 			assert.Equal(t, tt.expected, out)
 		})