refactor: changes from review

Various changes as a result of review feedback.

Signed-off-by: Richard Case <[email protected]>

Author: richardcase

cmd/clusterawsadm/cmd/gc/disable.go

@@ -28,18 +28,20 @@ import (
 )
 
 func newDisableCmd() *cobra.Command {
-	clusterName := ""
-	namespace := ""
-	kubeConfig := ""
-	kubeConfigDefault := ""
+	var (
+		clusterName       string
+		namespace         string
+		kubeConfig        string
+		kubeConfigDefault string
+	)
 
 	if home := homedir.HomeDir(); home != "" {
 		kubeConfigDefault = filepath.Join(home, ".kube", "config")
 	}
 
 	newCmd := &cobra.Command{
 		Use:   "disable",
-		Short: "Mark a cluster as NOT requiring external resource gc",
+		Short: "Mark a cluster as NOT requiring external resource garbage collection",
 		Long: cmd.LongDesc(`
 			This command will mark the given cluster as not requiring external
 			resource garbage collection (i.e. deleting) when the cluster is
@@ -63,8 +65,7 @@ func newDisableCmd() *cobra.Command {
 				return fmt.Errorf("creating command processor: %w", err)
 			}
 
-			err = proc.Disable(cmd.Context())
-			if err != nil {
+			if err := proc.Disable(cmd.Context()); err != nil {
 				return fmt.Errorf("disabling garbage collection: %w", err)
 			}
 			fmt.Printf("Disabled garbage collection for cluster %s/%s\n", namespace, clusterName)

cmd/clusterawsadm/cmd/gc/enable.go

@@ -28,18 +28,20 @@ import (
 )
 
 func newEnableCmd() *cobra.Command {
-	clusterName := ""
-	namespace := ""
-	kubeConfig := ""
-	kubeConfigDefault := ""
+	var (
+		clusterName       string
+		namespace         string
+		kubeConfig        string
+		kubeConfigDefault string
+	)
 
 	if home := homedir.HomeDir(); home != "" {
 		kubeConfigDefault = filepath.Join(home, ".kube", "config")
 	}
 
 	newCmd := &cobra.Command{
 		Use:   "enable",
-		Short: "Mark a cluster as requiring external resource gc",
+		Short: "Mark a cluster as requiring external resource garbage collection",
 		Long: cmd.LongDesc(`
 			This command will mark the given cluster as requiring external
 			resource garbage collection (i.e. deleting) when the cluster is
@@ -64,8 +66,7 @@ func newEnableCmd() *cobra.Command {
 				return fmt.Errorf("creating command processor: %w", err)
 			}
 
-			err = proc.Enable(cmd.Context())
-			if err != nil {
+			if err := proc.Enable(cmd.Context()); err != nil {
 				return fmt.Errorf("enabling garbage collection: %w", err)
 			}
 			fmt.Printf("Enabled garbage collection for cluster %s/%s\n", namespace, clusterName)

cmd/clusterawsadm/gc/disable.go

@@ -29,8 +29,11 @@ import (
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1"
 	ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/controlplane/eks/api/v1beta1"
+	expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/exp/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-aws/pkg/annotations"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
+	"sigs.k8s.io/cluster-api/util/patch"
 )
 
 var (
@@ -100,6 +103,44 @@ func New(input GCInput, opts ...CmdProcessorOption) (*CmdProcessor, error) {
 	return cmd, nil
 }
 
+// Enable is used to enable external resource garbage collection for a cluster.
+func (c *CmdProcessor) Enable(ctx context.Context) error {
+	if err := c.setAnnotationAndPatch(ctx, "true"); err != nil {
+		return fmt.Errorf("setting gc annotation to true: %w", err)
+	}
+
+	return nil
+}
+
+// Disable is used to disable external resource garbage collection for a cluster.
+func (c *CmdProcessor) Disable(ctx context.Context) error {
+	if err := c.setAnnotationAndPatch(ctx, "false"); err != nil {
+		return fmt.Errorf("setting gc annotation to false: %w", err)
+	}
+
+	return nil
+}
+
+func (c *CmdProcessor) setAnnotationAndPatch(ctx context.Context, annotationValue string) error {
+	infraObj, err := c.getInfraCluster(ctx)
+	if err != nil {
+		return err
+	}
+
+	patchHelper, err := patch.NewHelper(infraObj, c.client)
+	if err != nil {
+		return fmt.Errorf("creating patch helper: %w", err)
+	}
+
+	annotations.Set(infraObj, expinfrav1.ExternalResourceGCAnnotation, annotationValue)
+
+	if err := patchHelper.Patch(ctx, infraObj); err != nil {
+		return fmt.Errorf("patching infra cluster with gc annotation: %w", err)
+	}
+
+	return nil
+}
+
 func (c *CmdProcessor) getInfraCluster(ctx context.Context) (*unstructured.Unstructured, error) {
 	cluster := &clusterv1.Cluster{}
 

cmd/clusterawsadm/gc/enable.go

@@ -144,7 +144,7 @@ func TestDisableGC(t *testing.T) {
 			expectError:  false,
 		},
 		{
-			name:         "with managed control plane and with annotation",
+			name:         "with awscluster and with annotation",
 			clusterName:  testClusterName,
 			existingObjs: newUnManagedClusterWithAnnotations(testClusterName, map[string]string{expinfrav1.ExternalResourceGCAnnotation: "true"}),
 			expectError:  false,

cmd/clusterawsadm/gc/gc.go

@@ -5,23 +5,25 @@
 
 ## Overview
 
-Workload clusters that have been created by CAPA may have additional resources in AWS that need to be deleted when the cluster is deleted.
+Workload clusters that CAPA has created may have additional resources in AWS that need to be deleted when the cluster is deleted.
 
-For example, if the workload cluster has `Services` of type `LoadBalancer` then there will be AWS ELB/NLB provisioned. If you try to delete the workload cluster in this example it will fail as the VPC is still being used by these load balancers.
+For example, if the workload cluster has `Services` of type `LoadBalancer` then AWS ELB/NLB are provisioned. If you try to delete the workload cluster in this example, it will fail as these load balancers are still using the VPC.
 
-This feature enables deleting these external resources as part of cluster deletion. It works by annotating the AWS infra cluster / control plane on creation. When a CAPI `Cluster` is requested to be deleted the deletion of CAPA resources is blocked depending on the status of this annotation. When the resources have been garbage collected (i.e. deleted) then the annotation is updated and normal CAPA deletion starts. This is not related to [externally managed infrastructure](https://cluster-api-aws.sigs.k8s.io/topics/bring-your-own-aws-infrastructure.html).
+This feature enables deletion of these external resources as part of cluster deletion. During the deletion of a workload cluster the external AWS resources that where created by the Cloud Controller Manager (CCM) in the workload cluster will be identified and deleted.
 
-Currently we support cleaning up the following:
+> NOTE: This is not related to [externally managed infrastructure](https://cluster-api-aws.sigs.k8s.io/topics/bring-your-own-aws-infrastructure.html).
+
+Currently, we support cleaning up the following:
 
 - AWS ELB/NLB - by deleting `Services` of type `LoadBalancer` from the workload cluster
 
-We will look to potentially supporting deleting EBS volumes in the future.
+We will look to support deleting EBS volumes in the future potentially.
 
 > Note: this feature will likely be superseded by an upstream CAPI feature in the future when [this issue](https://github.com/kubernetes-sigs/cluster-api/issues/3075) is resolved.
 
 ## Enabling
 
-To enable this you must set the `ExternalResourceGC` to `true` on the controller manager. The easiest way to do this is via an environment variable:
+To enable garbage collection, you must set the `ExternalResourceGC` feature gate to `true` on the controller manager. The easiest way to do this is via an environment variable:
 
 ```bash
 export EXP_EXTERNAL_RESOURCE_GC=true
@@ -38,7 +40,7 @@ There are 2 ways to manually disable garbage collection for an individual cluste
 
 #### Using `clusterawsadm`
 
-You can disable garbage collection for a cluster by running the following command:
+By running the following command:
 
 ```bash
 clusterawsadm gc disable --cluster-name mycluster
@@ -48,25 +50,23 @@ See the command help for more examples.
 
 #### Editing `AWSCluster\AWSManagedControlPlane`
 
-You can disable garbage collection for a cluster by editing your `AWSCluster` or `AWSManagedControlPlane`.
-
-Either remove the `aws.cluster.x-k8s.io/external-resource-gc` or set its value to **true**.
+Or, by editing your `AWSCluster` or `AWSManagedControlPlane` so that the annotation `aws.cluster.x-k8s.io/external-resource-gc` is set to **false**.
 
 ```yaml
 apiVersion: controlplane.cluster.x-k8s.io/v1beta1
 kind: AWSManagedControlPlane
 metadata:
   annotations:
-    aws.cluster.x-k8s.io/external-resource-gc: "true"
+    aws.cluster.x-k8s.io/external-resource-gc: "false"
 ```
 
-### Manually Enablind Garbage Collection for a Cluster
+### Manually Enabling Garbage Collection for a Cluster
 
 There are 2 ways to manually enable garbage collection for an individual cluster:
 
 #### Using `clusterawsadm`
 
-You can enable garbage collection for a cluster by running the following command:
+By running the following command:
 
 ```bash
 clusterawsadm gc enable --cluster-name mycluster
@@ -76,14 +76,12 @@ See the command help for more examples.
 
 #### Editing `AWSCluster\AWSManagedControlPlane`
 
-You can enable garbage collection for a cluster by editing your `AWSCluster` or `AWSManagedControlPlane`.
-
-Add the `aws.cluster.x-k8s.io/external-resource-gc` annotation if it doesn't exist and set its value to **false**.
+Or, by editing your `AWSCluster` or `AWSManagedControlPlane` o that the annotation `aws.cluster.x-k8s.io/external-resource-gc` is either removed or set to **true**.
 
 ```yaml
 apiVersion: controlplane.cluster.x-k8s.io/v1beta1
 kind: AWSManagedControlPlane
 metadata:
   annotations:
-    aws.cluster.x-k8s.io/external-resource-gc: "false"
+    aws.cluster.x-k8s.io/external-resource-gc: "true"
 ```

cmd/clusterawsadm/gc/gc_test.go

@@ -68,7 +68,7 @@ const (
 
 	// ExternalResourceGC is used to enable the garbage collection of external resources like NLB/ALB on deletion
 	// owner: @richardcase
-	// alpha: v1.4
+	// alpha: v1.5
 	ExternalResourceGC featuregate.Feature = "ExternalResourceGC"
 )