remove strict AKS create validations for spec.controlPlaneEndpoint

kubernetes-sigs · Mar 24, 2023 · 62df033 · 62df033
1 parent dcd004c
commit 62df033
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 191 deletions.
diff --git a/api/v1beta1/azuremanagedcluster_webhook.go b/api/v1beta1/azuremanagedcluster_webhook.go
@@ -25,7 +25,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/cluster-api-provider-azure/feature"
 	"sigs.k8s.io/cluster-api-provider-azure/util/maps"
-	webhookutils "sigs.k8s.io/cluster-api-provider-azure/util/webhook"
 	capifeature "sigs.k8s.io/cluster-api/feature"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -52,18 +51,6 @@ func (r *AzureManagedCluster) ValidateCreate() error {
 			"can be set only if the Cluster API 'MachinePool' feature flag is enabled",
 		)
 	}
-	if r.Spec.ControlPlaneEndpoint.Host != "" {
-		return field.Forbidden(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Host"),
-			controlPlaneEndpointErrorMessage,
-		)
-	}
-	if r.Spec.ControlPlaneEndpoint.Port != 0 {
-		return field.Forbidden(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Port"),
-			controlPlaneEndpointErrorMessage,
-		)
-	}
 	return nil
 }
 
@@ -83,24 +70,6 @@ func (r *AzureManagedCluster) ValidateUpdate(oldRaw runtime.Object) error {
 				fmt.Sprintf("annotations with '%s' prefix are immutable", CustomHeaderPrefix)))
 	}
 
-	if old.Spec.ControlPlaneEndpoint.Host != "" {
-		if err := webhookutils.ValidateImmutable(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Host"),
-			old.Spec.ControlPlaneEndpoint.Host,
-			r.Spec.ControlPlaneEndpoint.Host); err != nil {
-			allErrs = append(allErrs, err)
-		}
-	}
-
-	if old.Spec.ControlPlaneEndpoint.Port != 0 {
-		if err := webhookutils.ValidateImmutable(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Port"),
-			old.Spec.ControlPlaneEndpoint.Port,
-			r.Spec.ControlPlaneEndpoint.Port); err != nil {
-			allErrs = append(allErrs, err)
-		}
-	}
-
 	if len(allErrs) != 0 {
 		return apierrors.NewInvalid(GroupVersion.WithKind("AzureManagedCluster").GroupKind(), r.Name, allErrs)
 	}

diff --git a/api/v1beta1/azuremanagedcluster_webhook_test.go b/api/v1beta1/azuremanagedcluster_webhook_test.go
@@ -122,13 +122,12 @@ func TestAzureManagedCluster_ValidateUpdate(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ControlPlaneEndpoint.Port is immutable",
+			name: "ControlPlaneEndpoint.Port update (AKS API-derived update scenario)",
 			oldAMC: &AzureManagedCluster{
 				ObjectMeta: metav1.ObjectMeta{},
 				Spec: AzureManagedClusterSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
 						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
-						Port: 443,
 					},
 				},
 			},
@@ -137,42 +136,22 @@ func TestAzureManagedCluster_ValidateUpdate(t *testing.T) {
 				Spec: AzureManagedClusterSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
 						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
-						Port: 444,
+						Port: 443,
 					},
 				},
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
-			name: "ControlPlaneEndpoint.Host is immutable",
+			name: "ControlPlaneEndpoint.Host update (AKS API-derived update scenario)",
 			oldAMC: &AzureManagedCluster{
 				ObjectMeta: metav1.ObjectMeta{},
 				Spec: AzureManagedClusterSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
 						Port: 443,
 					},
 				},
 			},
-			amc: &AzureManagedCluster{
-				ObjectMeta: metav1.ObjectMeta{},
-				Spec: AzureManagedClusterSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "this-is-not-allowed",
-						Port: 443,
-					},
-				},
-			},
-			wantErr: true,
-		},
-		{
-			name: "ControlPlaneEndpoint update from zero values are allowed",
-			oldAMC: &AzureManagedCluster{
-				ObjectMeta: metav1.ObjectMeta{},
-				Spec: AzureManagedClusterSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{},
-				},
-			},
 			amc: &AzureManagedCluster{
 				ObjectMeta: metav1.ObjectMeta{},
 				Spec: AzureManagedClusterSpec{
@@ -211,26 +190,26 @@ func TestAzureManagedCluster_ValidateCreate(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "can't set Spec.ControlPlaneEndpoint.Host during create",
+			name: "can set Spec.ControlPlaneEndpoint.Host during create (clusterctl move scenario)",
 			amc: &AzureManagedCluster{
 				Spec: AzureManagedClusterSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
 						Host: "my-host",
 					},
 				},
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
-			name: "can't set Spec.ControlPlaneEndpoint.Port during create",
+			name: "can set Spec.ControlPlaneEndpoint.Port during create (clusterctl move scenario)",
 			amc: &AzureManagedCluster{
 				Spec: AzureManagedClusterSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
 						Port: 4443,
 					},
 				},
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 	}
 	for _, tc := range tests {

diff --git a/api/v1beta1/azuremanagedcontrolplane_webhook.go b/api/v1beta1/azuremanagedcontrolplane_webhook.go
@@ -41,12 +41,11 @@ import (
 )
 
 var (
-	kubeSemver                       = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)
-	controlPlaneEndpointErrorMessage = "can not be set by the user, will be set automatically by AKS after the cluster is Ready"
-	rMaxNodeProvisionTime            = regexp.MustCompile(`^(\d+)m$`)
-	rScaleDownTime                   = regexp.MustCompile(`^(\d+)m$`)
-	rScaleDownDelayAfterDelete       = regexp.MustCompile(`^(\d+)s$`)
-	rScanInterval                    = regexp.MustCompile(`^(\d+)s$`)
+	kubeSemver                 = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)
+	rMaxNodeProvisionTime      = regexp.MustCompile(`^(\d+)m$`)
+	rScaleDownTime             = regexp.MustCompile(`^(\d+)m$`)
+	rScaleDownDelayAfterDelete = regexp.MustCompile(`^(\d+)s$`)
+	rScanInterval              = regexp.MustCompile(`^(\d+)s$`)
 )
 
 // SetupWebhookWithManager sets up and registers the webhook with the manager.
@@ -98,19 +97,6 @@ func (m *AzureManagedControlPlane) ValidateCreate(client client.Client) error {
 		)
 	}
 
-	if m.Spec.ControlPlaneEndpoint.Host != "" {
-		return field.Forbidden(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Host"),
-			controlPlaneEndpointErrorMessage,
-		)
-	}
-	if m.Spec.ControlPlaneEndpoint.Port != 0 {
-		return field.Forbidden(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Port"),
-			controlPlaneEndpointErrorMessage,
-		)
-	}
-
 	return m.Validate(client)
 }
 
@@ -207,24 +193,6 @@ func (m *AzureManagedControlPlane) ValidateUpdate(oldRaw runtime.Object, client
 		}
 	}
 
-	if old.Spec.ControlPlaneEndpoint.Host != "" {
-		if err := webhookutils.ValidateImmutable(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Host"),
-			old.Spec.ControlPlaneEndpoint.Host,
-			m.Spec.ControlPlaneEndpoint.Host); err != nil {
-			allErrs = append(allErrs, err)
-		}
-	}
-
-	if old.Spec.ControlPlaneEndpoint.Port != 0 {
-		if err := webhookutils.ValidateImmutable(
-			field.NewPath("Spec", "ControlPlaneEndpoint", "Port"),
-			old.Spec.ControlPlaneEndpoint.Port,
-			m.Spec.ControlPlaneEndpoint.Port); err != nil {
-			allErrs = append(allErrs, err)
-		}
-	}
-
 	// Consider removing this once moves out of preview
 	// Updating outboundType after cluster creation (PREVIEW)
 	// https://learn.microsoft.com/en-us/azure/aks/egress-outboundtype#updating-outboundtype-after-cluster-creation-preview

diff --git a/api/v1beta1/azuremanagedcontrolplane_webhook_test.go b/api/v1beta1/azuremanagedcontrolplane_webhook_test.go
@@ -630,7 +630,7 @@ func TestAzureManagedControlPlane_ValidateCreate(t *testing.T) {
 			errorLen: 1,
 		},
 		{
-			name: "can't set Spec.ControlPlaneEndpoint.Host during create",
+			name: "can set Spec.ControlPlaneEndpoint.Host during create (clusterctl move scenario)",
 			amcp: &AzureManagedControlPlane{
 				Spec: AzureManagedControlPlaneSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
@@ -647,10 +647,10 @@ func TestAzureManagedControlPlane_ValidateCreate(t *testing.T) {
 					},
 				},
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
-			name: "can't set Spec.ControlPlaneEndpoint.Port during create",
+			name: "can set Spec.ControlPlaneEndpoint.Port during create (clusterctl move scenario)",
 			amcp: &AzureManagedControlPlane{
 				Spec: AzureManagedControlPlaneSpec{
 					ControlPlaneEndpoint: clusterv1.APIEndpoint{
@@ -667,7 +667,7 @@ func TestAzureManagedControlPlane_ValidateCreate(t *testing.T) {
 					},
 				},
 			},
-			wantErr: true,
+			wantErr: false,
 		},
 	}
 	for _, tc := range tests {
@@ -1224,81 +1224,6 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		{
-			name: "AzureManagedControlPlane ControlPlaneEndpoint.Port is immutable",
-			oldAMCP: &AzureManagedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-cluster",
-				},
-				Spec: AzureManagedControlPlaneSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
-						Port: 443,
-					},
-				},
-			},
-			amcp: &AzureManagedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-cluster",
-				},
-				Spec: AzureManagedControlPlaneSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
-						Port: 444,
-					},
-				},
-			},
-			wantErr: true,
-		},
-		{
-			name: "AzureManagedControlPlane ControlPlaneEndpoint.Host is immutable",
-			oldAMCP: &AzureManagedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-cluster",
-				},
-				Spec: AzureManagedControlPlaneSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
-						Port: 443,
-					},
-				},
-			},
-			amcp: &AzureManagedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-cluster",
-				},
-				Spec: AzureManagedControlPlaneSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "this-is-not-allowed",
-						Port: 443,
-					},
-				},
-			},
-			wantErr: true,
-		},
-		{
-			name: "ControlPlaneEndpoint update from zero values are allowed",
-			oldAMCP: &AzureManagedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-cluster",
-				},
-				Spec: AzureManagedControlPlaneSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{},
-				},
-			},
-			amcp: &AzureManagedControlPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-cluster",
-				},
-				Spec: AzureManagedControlPlaneSpec{
-					ControlPlaneEndpoint: clusterv1.APIEndpoint{
-						Host: "aks-8622-h4h26c44.hcp.eastus.azmk8s.io",
-						Port: 443,
-					},
-				},
-			},
-			wantErr: true,
-		},
 		{
 			name: "OutboundType update",
 			oldAMCP: &AzureManagedControlPlane{

diff --git a/azure/scope/managedcontrolplane.go b/azure/scope/managedcontrolplane.go
@@ -578,12 +578,8 @@ func (s *ManagedControlPlaneScope) GetAllAgentPoolSpecs() ([]azure.ResourceSpecG
 
 // SetControlPlaneEndpoint sets a control plane endpoint.
 func (s *ManagedControlPlaneScope) SetControlPlaneEndpoint(endpoint clusterv1.APIEndpoint) {
-	if s.ControlPlane.Spec.ControlPlaneEndpoint.Host == "" {
-		s.ControlPlane.Spec.ControlPlaneEndpoint.Host = endpoint.Host
-	}
-	if s.ControlPlane.Spec.ControlPlaneEndpoint.Port == 0 {
-		s.ControlPlane.Spec.ControlPlaneEndpoint.Port = endpoint.Port
-	}
+	s.ControlPlane.Spec.ControlPlaneEndpoint.Host = endpoint.Host
+	s.ControlPlane.Spec.ControlPlaneEndpoint.Port = endpoint.Port
 }
 
 // MakeEmptyKubeConfigSecret creates an empty secret object that is used for storing kubeconfig secret data.

diff --git a/controllers/azuremanagedcluster_controller.go b/controllers/azuremanagedcluster_controller.go
@@ -161,14 +161,7 @@ func (amcr *AzureManagedClusterReconciler) Reconcile(ctx context.Context, req ct
 	// Infrastructure must be ready before control plane. We should also enqueue
 	// requests from control plane to infra cluster to keep control plane endpoint accurate.
 	aksCluster.Status.Ready = true
-	// We only expect to set the apiserver endpoint values once;
-	// if we attempted to update existing ControlPlaneEndpoint values, they would be rejected via webhook enforcement.
-	if aksCluster.Spec.ControlPlaneEndpoint.Host == "" {
-		aksCluster.Spec.ControlPlaneEndpoint.Host = controlPlane.Spec.ControlPlaneEndpoint.Host
-	}
-	if aksCluster.Spec.ControlPlaneEndpoint.Port == 0 {
-		aksCluster.Spec.ControlPlaneEndpoint.Port = controlPlane.Spec.ControlPlaneEndpoint.Port
-	}
+	aksCluster.Spec.ControlPlaneEndpoint = controlPlane.Spec.ControlPlaneEndpoint
 
 	if err := patchhelper.Patch(ctx, aksCluster); err != nil {
 		return reconcile.Result{}, err

diff --git a/docs/book/src/topics/managedcluster.md b/docs/book/src/topics/managedcluster.md
@@ -194,6 +194,8 @@ spec:
   sku: Standard_D2s_v4
 ```
 
+Please note that we don't declare a configuration for the apiserver endpoint. This configuration data will be populated automatically based on the data returned from AKS API during cluster create as `.spec.controlPlaneEndpoint.Host` and `.spec.controlPlaneEndpoint.Port` in both the `AzureManagedCluster` and `AzureManagedControlPlane` resources. Any user-provided data will be ignored and overwritten by data returned from the AKS API.
+
 The main features for configuration are:
 
 - [networkPolicy](https://docs.microsoft.com/en-us/azure/aks/concepts-network#network-policies)
@@ -717,7 +719,8 @@ Following is the list of immutable fields for managed clusters:
 
 | CRD                       | jsonPath                     | Comment                   |
 |---------------------------|------------------------------|---------------------------|
-| AzureManagedControlPlane  | .name                        |                           |
+| AzureManagedCluster       | .spec.controlPlaneEndpoint   | populated by the AKS API during cluster create |
+| AzureManagedControlPlane  | .spec.controlPlaneEndpoint   | populated by the AKS API during cluster create |
 | AzureManagedControlPlane  | .spec.subscriptionID         |                           |
 | AzureManagedControlPlane  | .spec.resourceGroupName      |                           |
 | AzureManagedControlPlane  | .spec.nodeResourceGroupName  |                           |