refactor: standardize the code for getting Helm values (#500)

I noticed there were 3 copies of `retrieveValuesTemplateConfigMap()` in addition to the one generic one in `lifecycleutils`, refactored all of these methods to reuse the existing function. While refactoring I saw that for NFD the handler was adding a value, moved the templating to the helm values. This copies the pattern in other addons, but also makes it easier to override at cluster creation time (kind of as a last resort if there is a bug in the handler code) I also added `RetrieveValuesTemplate` to get the data instead of having `valuesTemplateConfigMap.Data["values.yaml"]` in every handler.
nutanix-cloud-native · Apr 17, 2024 · 366321f · 366321f
1 parent e1c7b91
commit 366321f
Show file tree

Hide file tree

Showing 12 changed files with 72 additions and 132 deletions.
diff --git a/pkg/handlers/generic/lifecycle/ccm/aws/handler.go b/pkg/handlers/generic/lifecycle/ccm/aws/handler.go
@@ -8,11 +8,11 @@ import (
 	"fmt"
 
 	"github.com/blang/semver/v4"
+	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
@@ -58,11 +58,8 @@ func (a *AWSCCM) Apply(
 	ctx context.Context,
 	cluster *clusterv1.Cluster,
 	_ *v1alpha1.ClusterConfigSpec,
+	log logr.Logger,
 ) error {
-	log := ctrl.LoggerFrom(ctx).WithValues(
-		"cluster",
-		cluster.Name,
-	)
 	log.Info("Creating AWS CCM ConfigMap for Cluster")
 	version, err := semver.ParseTolerant(cluster.Spec.Topology.Version)
 	if err != nil {

diff --git a/pkg/handlers/generic/lifecycle/ccm/handler.go b/pkg/handlers/generic/lifecycle/ccm/handler.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/go-logr/logr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -25,7 +26,12 @@ const (
 )
 
 type CCMProvider interface {
-	Apply(context.Context, *clusterv1.Cluster, *v1alpha1.ClusterConfigSpec) error
+	Apply(
+		context.Context,
+		*clusterv1.Cluster,
+		*v1alpha1.ClusterConfigSpec,
+		logr.Logger,
+	) error
 }
 
 type CCMHandler struct {
@@ -120,7 +126,7 @@ func (c *CCMHandler) AfterControlPlaneInitialized(
 		return
 	}
 
-	err = handler.Apply(ctx, &req.Cluster, &clusterConfigVar)
+	err = handler.Apply(ctx, &req.Cluster, &clusterConfigVar, log)
 	if err != nil {
 		log.Error(
 			err,

diff --git a/pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go b/pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go
@@ -10,10 +10,10 @@ import (
 	"fmt"
 	"text/template"
 
+	"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"
-	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
@@ -73,21 +73,23 @@ func (p *provider) Apply(
 	ctx context.Context,
 	cluster *clusterv1.Cluster,
 	clusterConfig *v1alpha1.ClusterConfigSpec,
+	log logr.Logger,
 ) error {
 	// No need to check for nil values in the struct, this function will only be called if CCM is not nil
 	if clusterConfig.Addons.CCM.Credentials == nil {
 		return ErrMissingCredentials
 	}
 
-	valuesTemplateConfigMap, err := lifecycleutils.RetrieveValuesTemplateConfigMap(
+	log.Info("Retrieving Nutanix CCM installation values template for cluster")
+	values, err := lifecycleutils.RetrieveValuesTemplate(
 		ctx,
 		p.client,
 		p.config.defaultValuesTemplateConfigMapName,
 		p.config.DefaultsNamespace(),
 	)
 	if err != nil {
 		return fmt.Errorf(
-			"failed to retrieve Nutanix CCM installation values template ConfigMap for cluster: %w",
+			"failed to retrieve Nutanix CCM installation values template for cluster: %w",
 			err,
 		)
 	}
@@ -115,16 +117,11 @@ func (p *provider) Apply(
 		}
 	}
 
-	log := ctrl.LoggerFrom(ctx).WithValues(
-		"cluster",
-		ctrlclient.ObjectKeyFromObject(cluster),
-	)
 	helmChart, err := p.helmChartInfoGetter.For(ctx, log, config.NutanixCCM)
 	if err != nil {
 		return fmt.Errorf("failed to get values for nutanix-ccm-config %w", err)
 	}
 
-	values := valuesTemplateConfigMap.Data["values.yaml"]
 	// 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 {

diff --git a/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go b/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go
@@ -20,19 +20,10 @@ import (
 )
 
 type crsConfig struct {
-	defaultsNamespace string
-
 	defaultClusterAutoscalerConfigMap string
 }
 
 func (c *crsConfig) AddFlags(prefix string, flags *pflag.FlagSet) {
-	flags.StringVar(
-		&c.defaultsNamespace,
-		prefix+".defaults-namespace",
-		corev1.NamespaceDefault,
-		"namespace of the ConfigMap used to deploy cluster-autoscaler",
-	)
-
 	flags.StringVar(
 		&c.defaultClusterAutoscalerConfigMap,
 		prefix+".default-cluster-autoscaler-configmap-name",

diff --git a/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go
@@ -18,7 +18,7 @@ import (
 	caaphv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/external/sigs.k8s.io/cluster-api-addon-provider-helm/api/v1alpha1"
 	"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/config"
-	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/lifecycle/utils"
+	lifecycleutils "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/lifecycle/utils"
 )
 
 const (
@@ -52,27 +52,23 @@ func (s helmAddonStrategy) apply(
 	log logr.Logger,
 ) error {
 	log.Info("Retrieving cluster-autoscaler installation values template for cluster")
-	valuesTemplateConfigMap, err := utils.RetrieveValuesTemplateConfigMap(
+	values, err := lifecycleutils.RetrieveValuesTemplate(
 		ctx,
 		s.client,
 		s.config.defaultValuesTemplateConfigMapName,
 		defaultsNamespace,
 	)
 	if err != nil {
 		return fmt.Errorf(
-			"failed to retrieve cluster-autoscaler installation values template ConfigMap for cluster: %w",
+			"failed to retrieve cluster-autoscaler installation values template for cluster: %w",
 			err,
 		)
 	}
 
-	cluster := &req.Cluster
-
-	values := valuesTemplateConfigMap.Data["values.yaml"]
-
 	// The cluster-autoscaler is different from other addons.
 	// It requires all resources to be created in the management cluster,
 	// which means creating the HelmChartProxy always targeting the management cluster.
-	targetCluster, err := findTargetCluster(ctx, s.client, cluster)
+	targetCluster, err := findTargetCluster(ctx, s.client, &req.Cluster)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
@@ -19,6 +18,7 @@ import (
 	caaphv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/external/sigs.k8s.io/cluster-api-addon-provider-helm/api/v1alpha1"
 	"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/config"
+	lifecycleutils "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/lifecycle/utils"
 )
 
 const (
@@ -68,14 +68,15 @@ func (s helmAddonStrategy) apply(
 	}
 
 	log.Info("Retrieving Calico installation values template for cluster")
-	valuesTemplateConfigMap, err := s.retrieveValuesTemplateConfigMap(
+	values, err := lifecycleutils.RetrieveValuesTemplate(
 		ctx,
-		defaultsNamespace,
+		s.client,
 		defaultInstallationConfigMapName,
+		defaultsNamespace,
 	)
 	if err != nil {
 		return fmt.Errorf(
-			"failed to retrieve Calico installation values template ConfigMap for cluster: %w",
+			"failed to retrieve Calico installation values template for cluster: %w",
 			err,
 		)
 	}
@@ -98,7 +99,7 @@ func (s helmAddonStrategy) apply(
 			ReleaseNamespace: defaultTigerOperatorNamespace,
 			ReleaseName:      defaultTigeraOperatorReleaseName,
 			Version:          s.helmChart.Version,
-			ValuesTemplate:   valuesTemplateConfigMap.Data["values.yaml"],
+			ValuesTemplate:   values,
 		},
 	}
 
@@ -115,29 +116,3 @@ func (s helmAddonStrategy) apply(
 
 	return nil
 }
-
-func (s helmAddonStrategy) retrieveValuesTemplateConfigMap(
-	ctx context.Context,
-	defaultsNamespace string,
-	configMapName string,
-) (*corev1.ConfigMap, error) {
-	configMap := &corev1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: defaultsNamespace,
-			Name:      configMapName,
-		},
-	}
-	configMapObjName := ctrlclient.ObjectKeyFromObject(
-		configMap,
-	)
-	err := s.client.Get(ctx, configMapObjName, configMap)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to retrieve installation values template ConfigMap %q: %w",
-			configMapObjName,
-			err,
-		)
-	}
-
-	return configMap, nil
-}
diff --git a/pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
@@ -19,6 +18,7 @@ import (
 	caaphv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/external/sigs.k8s.io/cluster-api-addon-provider-helm/api/v1alpha1"
 	"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/config"
+	lifecycleutils "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/lifecycle/utils"
 )
 
 const (
@@ -52,10 +52,15 @@ func (s helmAddonStrategy) apply(
 	log logr.Logger,
 ) error {
 	log.Info("Retrieving Cilium installation values template for cluster")
-	valuesTemplateConfigMap, err := s.retrieveValuesTemplateConfigMap(ctx, defaultsNamespace)
+	values, err := lifecycleutils.RetrieveValuesTemplate(
+		ctx,
+		s.client,
+		s.config.defaultValuesTemplateConfigMapName,
+		defaultsNamespace,
+	)
 	if err != nil {
 		return fmt.Errorf(
-			"failed to retrieve Cilium installation values template ConfigMap for cluster: %w",
+			"failed to retrieve Cilium installation values template for cluster: %w",
 			err,
 		)
 	}
@@ -78,7 +83,7 @@ func (s helmAddonStrategy) apply(
 			ReleaseNamespace: defaultCiliumNamespace,
 			ReleaseName:      defaultCiliumReleaseName,
 			Version:          s.helmChart.Version,
-			ValuesTemplate:   valuesTemplateConfigMap.Data["values.yaml"],
+			ValuesTemplate:   values,
 		},
 	}
 
@@ -95,28 +100,3 @@ func (s helmAddonStrategy) apply(
 
 	return nil
 }
-
-func (s helmAddonStrategy) retrieveValuesTemplateConfigMap(
-	ctx context.Context,
-	defaultsNamespace string,
-) (*corev1.ConfigMap, error) {
-	configMap := &corev1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: defaultsNamespace,
-			Name:      s.config.defaultValuesTemplateConfigMapName,
-		},
-	}
-	configMapObjName := ctrlclient.ObjectKeyFromObject(
-		configMap,
-	)
-	err := s.client.Get(ctx, configMapObjName, configMap)
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to retrieve installation values template ConfigMap %q: %w",
-			configMapObjName,
-			err,
-		)
-	}
-
-	return configMap, nil
-}
diff --git a/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go b/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	"github.com/spf13/pflag"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -60,6 +61,7 @@ func (a *AWSEBS) Apply(
 	provider v1alpha1.CSIProvider,
 	defaultStorageConfig *v1alpha1.DefaultStorage,
 	req *runtimehooksv1.AfterControlPlaneInitializedRequest,
+	_ logr.Logger,
 ) error {
 	strategy := provider.Strategy
 	switch strategy {

diff --git a/pkg/handlers/generic/lifecycle/csi/handler.go b/pkg/handlers/generic/lifecycle/csi/handler.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -28,6 +29,7 @@ type CSIProvider interface {
 		v1alpha1.CSIProvider,
 		*v1alpha1.DefaultStorage,
 		*runtimehooksv1.AfterControlPlaneInitializedRequest,
+		logr.Logger,
 	) error
 }
 
@@ -124,6 +126,7 @@ func (c *CSIHandler) AfterControlPlaneInitialized(
 			provider,
 			csiProviders.DefaultStorage,
 			req,
+			log,
 		)
 		if err != nil {
 			log.Error(