Merge pull request #4392 from nawazkh/update_AzureManagedMachinePool_…

…Webhook AzureManagedMachinePool ValidateDelete Webhook will check AMMP for DeleteForMoveAnnotation annotation
kubernetes-sigs · Dec 15, 2023 · 73b3cf1 · 73b3cf1
2 parents ad30f63 + cd5ba59
commit 73b3cf1
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 22 deletions.
diff --git a/api/v1beta1/azuremanagedmachinepool_webhook.go b/api/v1beta1/azuremanagedmachinepool_webhook.go
@@ -34,6 +34,7 @@ import (
 	azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure"
 	webhookutils "sigs.k8s.io/cluster-api-provider-azure/util/webhook"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	clusterctlv1alpha3 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	capifeature "sigs.k8s.io/cluster-api/feature"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -223,7 +224,7 @@ func (mw *azureManagedMachinePoolWebhook) ValidateUpdate(ctx context.Context, ol
 
 	if m.Spec.Mode != string(NodePoolModeSystem) && old.Spec.Mode == string(NodePoolModeSystem) {
 		// validate for last system node pool
-		if err := validateLastSystemNodePool(mw.Client, m.Labels, m.Namespace); err != nil {
+		if err := validateLastSystemNodePool(mw.Client, m.Labels, m.Namespace, m.Annotations); err != nil {
 			allErrs = append(allErrs, field.Forbidden(
 				field.NewPath("Spec", "Mode"),
 				"Cannot change node pool mode to User, you must have at least one System node pool in your cluster"))
@@ -308,12 +309,12 @@ func (mw *azureManagedMachinePoolWebhook) ValidateDelete(ctx context.Context, ob
 		return nil, nil
 	}
 
-	return nil, errors.Wrapf(validateLastSystemNodePool(mw.Client, m.Labels, m.Namespace), "if the delete is triggered via owner MachinePool please refer to trouble shooting section in https://capz.sigs.k8s.io/topics/managedcluster.html")
+	return nil, errors.Wrapf(validateLastSystemNodePool(mw.Client, m.Labels, m.Namespace, m.Annotations), "if the delete is triggered via owner MachinePool please refer to trouble shooting section in https://capz.sigs.k8s.io/topics/managedcluster.html")
 }
 
 // validateLastSystemNodePool is used to check if the existing system node pool is the last system node pool.
 // If it is a last system node pool it cannot be deleted or mutated to user node pool as AKS expects min 1 system node pool.
-func validateLastSystemNodePool(cli client.Client, labels map[string]string, namespace string) error {
+func validateLastSystemNodePool(cli client.Client, labels map[string]string, namespace string, annotations map[string]string) error {
 	ctx := context.Background()
 
 	// Fetch the Cluster.
@@ -336,7 +337,8 @@ func validateLastSystemNodePool(cli client.Client, labels map[string]string, nam
 		return nil
 	}
 
-	if ownerCluster.Spec.Paused {
+	// checking if this AzureManagedMachinePool is going to be deleted for clusterctl move operation
+	if _, ok := annotations[clusterctlv1alpha3.DeleteForMoveAnnotation]; ok {
 		return nil
 	}
 

diff --git a/api/v1beta1/azuremanagedmachinepool_webhook_test.go b/api/v1beta1/azuremanagedmachinepool_webhook_test.go
@@ -28,6 +28,7 @@ import (
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/cluster-api-provider-azure/feature"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	clusterctlv1alpha3 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	capifeature "sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -1328,30 +1329,37 @@ func TestAzureManagedMachinePool_validateLastSystemNodePool(t *testing.T) {
 	deletionTime := metav1.Now()
 	finalizers := []string{"test"}
 	systemMachinePool := getManagedMachinePoolWithSystemMode()
+	systemMachinePoolWithDeletionAnnotation := getAzureManagedMachinePoolWithChanges(
+		// Add the DeleteForMoveAnnotation annotation to the AMMP
+		func(azureManagedMachinePool *AzureManagedMachinePool) {
+			azureManagedMachinePool.Annotations = map[string]string{
+				clusterctlv1alpha3.DeleteForMoveAnnotation: "true",
+			}
+		},
+	)
 	tests := []struct {
 		name    string
 		ammp    *AzureManagedMachinePool
 		cluster *clusterv1.Cluster
 		wantErr bool
 	}{
 		{
-			name: "Test with paused cluster without deletion timestamp having one system pool node(valid delete:move operation)",
-			ammp: systemMachinePool,
+			// AzureManagedMachinePool will be deleted since AMMP has DeleteForMoveAnnotation annotation
+			// Note that Owner Cluster's deletion timestamp is nil and Owner cluster being paused does not matter anymore.
+			name: "AzureManagedMachinePool (AMMP) should be deleted if this AMMP has the annotation 'cluster.x-k8s.io/move-to-delete' with the owner cluster being paused and 'No' deletion timestamp",
+			ammp: systemMachinePoolWithDeletionAnnotation,
 			cluster: &clusterv1.Cluster{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:              systemMachinePool.GetLabels()[clusterv1.ClusterNameLabel],
-					Namespace:         systemMachinePool.Namespace,
-					DeletionTimestamp: &deletionTime,
-					Finalizers:        finalizers,
-				},
-				Spec: clusterv1.ClusterSpec{
-					Paused: true,
+					Name:       systemMachinePool.GetLabels()[clusterv1.ClusterNameLabel],
+					Namespace:  systemMachinePool.Namespace,
+					Finalizers: finalizers,
 				},
 			},
 			wantErr: false,
 		},
 		{
-			name: "Test with paused cluster with deletion timestamp having one system pool node(valid delete)",
+			// AzureManagedMachinePool will be deleted since Owner Cluster has been marked for deletion
+			name: "AzureManagedMachinePool should be deleted since the Cluster is paused with a deletion timestamp",
 			ammp: systemMachinePool,
 			cluster: &clusterv1.Cluster{
 				ObjectMeta: metav1.ObjectMeta{
@@ -1360,14 +1368,11 @@ func TestAzureManagedMachinePool_validateLastSystemNodePool(t *testing.T) {
 					DeletionTimestamp: &deletionTime,
 					Finalizers:        finalizers,
 				},
-				Spec: clusterv1.ClusterSpec{
-					Paused: true,
-				},
 			},
 			wantErr: false,
 		},
 		{
-			name: "Test with running cluster without deletion timestamp having one system pool node(invalid delete)",
+			name: "AzureManagedMachinePool should not be deleted without a deletion timestamp on Owner Cluster and having one system pool node(invalid delete)",
 			ammp: systemMachinePool,
 			cluster: &clusterv1.Cluster{
 				ObjectMeta: metav1.ObjectMeta{
@@ -1378,7 +1383,7 @@ func TestAzureManagedMachinePool_validateLastSystemNodePool(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "Test with running cluster with deletion timestamp having one system pool node(valid delete)",
+			name: "AzureManagedMachinePool should be deleted when Cluster is set with a deletion timestamp having one system pool node(valid delete)",
 			ammp: systemMachinePool,
 			cluster: &clusterv1.Cluster{
 				ObjectMeta: metav1.ObjectMeta{
@@ -1399,7 +1404,7 @@ func TestAzureManagedMachinePool_validateLastSystemNodePool(t *testing.T) {
 			_ = AddToScheme(scheme)
 			_ = clusterv1.AddToScheme(scheme)
 			fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tc.cluster, tc.ammp).Build()
-			err := validateLastSystemNodePool(fakeClient, tc.ammp.Spec.NodeLabels, tc.ammp.Namespace)
+			err := validateLastSystemNodePool(fakeClient, tc.ammp.Spec.NodeLabels, tc.ammp.Namespace, tc.ammp.Annotations)
 			if tc.wantErr {
 				g.Expect(err).To(HaveOccurred())
 			} else {
@@ -1438,3 +1443,11 @@ func getManagedMachinePoolWithSystemMode() *AzureManagedMachinePool {
 		},
 	}
 }
+
+func getAzureManagedMachinePoolWithChanges(changes ...func(*AzureManagedMachinePool)) *AzureManagedMachinePool {
+	ammp := getManagedMachinePoolWithSystemMode().DeepCopy()
+	for _, change := range changes {
+		change(ammp)
+	}
+	return ammp
+}
diff --git a/api/v1beta1/azuremanagedmachinepooltemplate_webhook.go b/api/v1beta1/azuremanagedmachinepooltemplate_webhook.go
@@ -199,7 +199,7 @@ func (mpw *azureManagedMachinePoolTemplateWebhook) ValidateUpdate(ctx context.Co
 
 	if mp.Spec.Template.Spec.Mode != string(NodePoolModeSystem) && old.Spec.Template.Spec.Mode == string(NodePoolModeSystem) {
 		// validate for last system node pool
-		if err := validateLastSystemNodePool(mpw.Client, mp.Spec.Template.Spec.NodeLabels, mp.Namespace); err != nil {
+		if err := validateLastSystemNodePool(mpw.Client, mp.Spec.Template.Spec.NodeLabels, mp.Namespace, mp.Annotations); err != nil {
 			allErrs = append(allErrs, field.Forbidden(
 				field.NewPath("Spec", "Template", "Spec", "Mode"),
 				"Cannot change node pool mode to User, you must have at least one System node pool in your cluster"))
@@ -283,5 +283,5 @@ func (mpw *azureManagedMachinePoolTemplateWebhook) ValidateDelete(ctx context.Co
 		return nil, nil
 	}
 
-	return nil, errors.Wrapf(validateLastSystemNodePool(mpw.Client, mp.Spec.Template.Spec.NodeLabels, mp.Namespace), "if the delete is triggered via owner MachinePool please refer to trouble shooting section in https://capz.sigs.k8s.io/topics/managedcluster.html")
+	return nil, errors.Wrapf(validateLastSystemNodePool(mpw.Client, mp.Spec.Template.Spec.NodeLabels, mp.Namespace, mp.Annotations), "if the delete is triggered via owner MachinePool please refer to trouble shooting section in https://capz.sigs.k8s.io/topics/managedcluster.html")
 }