fix: Fix ownership of ClusterAutoscaler resources (#810)

The Cluster Autoscaler addon (either HelmChartProxy or
ClusterResourceSet
) has to be deployed in the management cluster namespace and as such
cannot
be owned by the workload cluster, which commonly resides in a different
namespace.

This commit fixes that and introduces a BeforeClusterDelete hook to
delete
the HelmChartProxy or ClusterResourceSet rather than relying no
Kubernetes
GC as was previously expected.

Tested locally. Created workload cluster in different namespace to
management cluster. HCP does not have ownership set:

```
apiVersion: addons.cluster.x-k8s.io/v1alpha1
kind: HelmChartProxy
metadata:
  creationTimestamp: "2024-07-18T13:02:30Z"
  finalizers:
  - helmchartproxy.addons.cluster.x-k8s.io
  generation: 1
  name: cluster-autoscaler-my-docker-cluster
  namespace: test
  resourceVersion: "6238"
  uid: 0a5ebb11-5318-4114-bedd-9bcc9459f85e
spec:
``` 

And is properly cleaned up on deletion:

```
$ k get hcp -A
No resources found
```

Author: jimmidyson

pkg/handlers/generic/lifecycle/ccm/aws/strategy_crs.go

@@ -74,6 +74,7 @@ func (s crsStrategy) Apply(
 		ccmConfigMap.Name,
 		s.client,
 		cluster,
+		handlersutils.DefaultEnsureCRSForClusterFromObjectsOptions(),
 		ccmConfigMap,
 	)
 	if err != nil {

pkg/handlers/generic/lifecycle/clusterautoscaler/handler.go

@@ -30,6 +30,12 @@ type addonStrategy interface {
 		string,
 		logr.Logger,
 	) error
+
+	delete(
+		context.Context,
+		*clusterv1.Cluster,
+		logr.Logger,
+	) error
 }
 type Config struct {
 	*options.GlobalOptions
@@ -110,29 +116,18 @@ func (n *DefaultClusterAutoscaler) apply(
 		clusterKey,
 	)
 
-	varMap := variables.ClusterVariablesToVariablesMap(cluster.Spec.Topology.Variables)
-
-	caVar, err := variables.Get[v1alpha1.ClusterAutoscaler](
-		varMap,
-		n.variableName,
-		n.variablePath...)
+	caVar, err := n.getCAVariable(cluster)
 	if err != nil {
-		if variables.IsNotFoundError(err) {
-			log.V(5).Info(
-				"Skipping cluster-autoscaler handler, cluster does not specify request cluster-autoscaler addon deployment",
-			)
-			return
-		}
-		log.Error(
-			err,
-			"failed to read cluster-autoscaler variable from cluster definition",
-		)
+		log.Error(err, "failed to read cluster-autoscaler variable from cluster definition")
 		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
-		resp.SetMessage(
-			fmt.Sprintf("failed to read cluster-autoscaler variable from cluster definition: %v",
-				err,
-			),
+		resp.SetMessage(err.Error())
+		return
+	}
+	if caVar == nil {
+		log.V(5).Info(
+			"Skipping cluster-autoscaler handler, cluster does not specify request cluster-autoscaler addon deployment",
 		)
+		resp.SetStatus(runtimehooksv1.ResponseStatusSuccess)
 		return
 	}
 
@@ -186,3 +181,85 @@ func (n *DefaultClusterAutoscaler) apply(
 
 	resp.SetStatus(runtimehooksv1.ResponseStatusSuccess)
 }
+
+func (n *DefaultClusterAutoscaler) BeforeClusterDelete(
+	ctx context.Context,
+	req *runtimehooksv1.BeforeClusterDeleteRequest,
+	resp *runtimehooksv1.BeforeClusterDeleteResponse,
+) {
+	cluster := &req.Cluster
+
+	clusterKey := ctrlclient.ObjectKeyFromObject(cluster)
+
+	log := ctrl.LoggerFrom(ctx).WithValues(
+		"cluster",
+		clusterKey,
+	)
+
+	caVar, err := n.getCAVariable(cluster)
+	if err != nil {
+		log.Error(err, "failed to read cluster-autoscaler variable from cluster definition")
+		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+		resp.SetMessage(err.Error())
+		return
+	}
+	if caVar == nil {
+		log.V(5).Info(
+			"Skipping cluster-autoscaler before cluster delete handler, cluster does not specify request cluster-autoscaler" +
+				"addon deployment",
+		)
+		resp.SetStatus(runtimehooksv1.ResponseStatusSuccess)
+		return
+	}
+
+	var strategy addonStrategy
+	switch ptr.Deref(caVar.Strategy, "") {
+	case v1alpha1.AddonStrategyClusterResourceSet:
+		strategy = crsStrategy{
+			config: n.config.crsConfig,
+			client: n.client,
+		}
+	case v1alpha1.AddonStrategyHelmAddon:
+		strategy = helmAddonStrategy{
+			config: n.config.helmAddonConfig,
+			client: n.client,
+		}
+	case "":
+		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+		resp.SetMessage("strategy not specified for cluster-autoscaler addon")
+	default:
+		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+		resp.SetMessage(
+			fmt.Sprintf("unknown cluster-autoscaler addon deployment strategy %q", *caVar.Strategy),
+		)
+		return
+	}
+
+	if err = strategy.delete(ctx, cluster, log); err != nil {
+		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+		resp.SetMessage(err.Error())
+		return
+	}
+
+	resp.SetStatus(runtimehooksv1.ResponseStatusSuccess)
+}
+
+func (n *DefaultClusterAutoscaler) getCAVariable(
+	cluster *clusterv1.Cluster,
+) (*v1alpha1.ClusterAutoscaler, error) {
+	varMap := variables.ClusterVariablesToVariablesMap(cluster.Spec.Topology.Variables)
+
+	caVar, err := variables.Get[v1alpha1.ClusterAutoscaler](
+		varMap,
+		n.variableName,
+		n.variablePath...)
+	if err != nil {
+		if variables.IsNotFoundError(err) {
+			return nil, nil
+		}
+
+		return nil, err
+	}
+
+	return &caVar, nil
+}

pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go

@@ -13,6 +13,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/remote"
+	crsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/k8s/client"
@@ -74,7 +75,7 @@ func (s crsStrategy) apply(
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: cluster.Namespace,
-			Name:      defaultCM.Name + "-" + cluster.Name,
+			Name:      s.crsNameForCluster(cluster),
 		},
 		Data: data,
 	}
@@ -110,7 +111,17 @@ func (s crsStrategy) apply(
 		)
 	}
 
-	if err = utils.EnsureCRSForClusterFromObjects(ctx, cm.Name, s.client, targetCluster, cm); err != nil {
+	// NOTE Unlike other addons, the cluster-autoscaler ClusterResourceSet is created in the management cluster
+	// namespace and thus cannot be owned by the workload cluster which will commonly exist in a different namespace.
+	// Deletion is handled by a BeforeClusterDelete hook instead of relying on Kubernetes GC.
+	if err = utils.EnsureCRSForClusterFromObjects(
+		ctx,
+		cm.Name,
+		s.client,
+		targetCluster,
+		utils.EnsureCRSForClusterFromObjectsOptions{SetClusterOwnership: false},
+		cm,
+	); err != nil {
 		return fmt.Errorf(
 			"failed to apply cluster-autoscaler installation ClusterResourceSet: %w",
 			err,
@@ -119,3 +130,41 @@ func (s crsStrategy) apply(
 
 	return nil
 }
+
+func (s crsStrategy) delete(
+	ctx context.Context,
+	cluster *clusterv1.Cluster,
+	log logr.Logger,
+) error {
+	// The cluster-autoscaler is different from other addons.
+	// It requires all resources to be created in the management cluster,
+	// which means creating the ClusterResourceSet always targeting the management cluster.
+	targetCluster, err := findTargetCluster(ctx, s.client, cluster)
+	if err != nil {
+		return err
+	}
+
+	crs := &crsv1.ClusterResourceSet{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: crsv1.GroupVersion.String(),
+			Kind:       "ClusterResourceSet",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: targetCluster.Namespace,
+			Name:      s.crsNameForCluster(cluster),
+		},
+	}
+
+	if err := ctrlclient.IgnoreNotFound(s.client.Delete(ctx, crs)); err != nil {
+		return fmt.Errorf(
+			"failed to delete cluster-autoscaler installation ClusterResourceSet: %w",
+			err,
+		)
+	}
+
+	return nil
+}
+
+func (s crsStrategy) crsNameForCluster(cluster *clusterv1.Cluster) string {
+	return s.config.defaultClusterAutoscalerConfigMap + "-" + cluster.Name
+}

pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go

@@ -12,7 +12,6 @@ import (
 	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"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/k8s/client"
@@ -103,16 +102,48 @@ func (s helmAddonStrategy) apply(
 	}
 
 	handlersutils.SetTLSConfigForHelmChartProxyIfNeeded(hcp)
-	if err = controllerutil.SetOwnerReference(cluster, hcp, s.client.Scheme()); err != nil {
-		return fmt.Errorf(
-			"failed to set owner reference on cluster-autoscaler installation HelmChartProxy: %w",
-			err,
-		)
-	}
+
+	// NOTE Unlike other addons, the cluster-autoscaler HelmChartProxy is created in the management cluster
+	// namespace and thus cannot be owned by the workload cluster which will commonly exist in a different namespace.
+	// Deletion is handled by a BeforeClusterDelete hook instead of relying on Kubernetes GC.
 
 	if err = client.ServerSideApply(ctx, s.client, hcp, client.ForceOwnership); err != nil {
 		return fmt.Errorf("failed to apply cluster-autoscaler installation HelmChartProxy: %w", err)
 	}
 
 	return nil
 }
+
+func (s helmAddonStrategy) delete(
+	ctx context.Context,
+	cluster *clusterv1.Cluster,
+	log logr.Logger,
+) error {
+	// 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)
+	if err != nil {
+		return err
+	}
+
+	hcp := &caaphv1.HelmChartProxy{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: caaphv1.GroupVersion.String(),
+			Kind:       "HelmChartProxy",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: targetCluster.Namespace,
+			Name:      "cluster-autoscaler-" + cluster.Name,
+		},
+	}
+
+	if err := ctrlclient.IgnoreNotFound(s.client.Delete(ctx, hcp)); err != nil {
+		return fmt.Errorf(
+			"failed to delete cluster-autoscaler installation HelmChartProxy: %w",
+			err,
+		)
+	}
+
+	return nil
+}

pkg/handlers/generic/lifecycle/cni/calico/strategy_crs.go

@@ -150,7 +150,15 @@ func (s crsStrategy) ensureCNICRSForCluster(
 		)
 	}
 
-	if err := utils.EnsureCRSForClusterFromObjects(ctx, cm.Name, s.client, cluster, tigeraConfigMap, cm); err != nil {
+	if err := utils.EnsureCRSForClusterFromObjects(
+		ctx,
+		cm.Name,
+		s.client,
+		cluster,
+		utils.DefaultEnsureCRSForClusterFromObjectsOptions(),
+		tigeraConfigMap,
+		cm,
+	); err != nil {
 		return fmt.Errorf(
 			"failed to apply Calico CNI installation ClusterResourceSet: %w",
 			err,

pkg/handlers/generic/lifecycle/cni/cilium/strategy_crs.go

@@ -85,7 +85,14 @@ func (s crsStrategy) apply(
 		)
 	}
 
-	if err := utils.EnsureCRSForClusterFromObjects(ctx, cm.Name, s.client, cluster, cm); err != nil {
+	if err := utils.EnsureCRSForClusterFromObjects(
+		ctx,
+		cm.Name,
+		s.client,
+		cluster,
+		utils.DefaultEnsureCRSForClusterFromObjectsOptions(),
+		cm,
+	); err != nil {
 		return fmt.Errorf(
 			"failed to apply Cilium CNI installation ClusterResourceSet: %w",
 			err,

pkg/handlers/generic/lifecycle/csi/awsebs/strategy_crs.go

@@ -72,6 +72,7 @@ func (s crsStrategy) Apply(
 		cm.Name,
 		s.client,
 		cluster,
+		handlersutils.DefaultEnsureCRSForClusterFromObjectsOptions(),
 		cm,
 	)
 	if err != nil {

pkg/handlers/generic/lifecycle/csi/localpath/strategy_crs.go

@@ -85,7 +85,14 @@ func (s crsStrategy) Apply(
 		)
 	}
 
-	if err := utils.EnsureCRSForClusterFromObjects(ctx, cm.Name, s.client, cluster, cm); err != nil {
+	if err := utils.EnsureCRSForClusterFromObjects(
+		ctx,
+		cm.Name,
+		s.client,
+		cluster,
+		utils.DefaultEnsureCRSForClusterFromObjectsOptions(),
+		cm,
+	); err != nil {
 		return fmt.Errorf(
 			"failed to apply local-path CSI installation ClusterResourceSet: %w",
 			err,

pkg/handlers/generic/lifecycle/csi/snapshotcontroller/strategy_crs.go

@@ -85,7 +85,14 @@ func (s crsStrategy) Apply(
 		)
 	}
 
-	if err := utils.EnsureCRSForClusterFromObjects(ctx, cm.Name, s.client, cluster, cm); err != nil {
+	if err := utils.EnsureCRSForClusterFromObjects(
+		ctx,
+		cm.Name,
+		s.client,
+		cluster,
+		utils.DefaultEnsureCRSForClusterFromObjectsOptions(),
+		cm,
+	); err != nil {
 		return fmt.Errorf(
 			"failed to apply snapshot-controller installation ClusterResourceSet: %w",
 			err,

pkg/handlers/generic/lifecycle/nfd/strategy_crs.go

@@ -85,7 +85,14 @@ func (s crsStrategy) Apply(
 		)
 	}
 
-	if err := utils.EnsureCRSForClusterFromObjects(ctx, cm.Name, s.client, cluster, cm); err != nil {
+	if err := utils.EnsureCRSForClusterFromObjects(
+		ctx,
+		cm.Name,
+		s.client,
+		cluster,
+		utils.DefaultEnsureCRSForClusterFromObjectsOptions(),
+		cm,
+	); err != nil {
 		return fmt.Errorf(
 			"failed to apply NFD installation ClusterResourceSet: %w",
 			err,