From 62df0338de5b01cb4d9e7ecda592d487a3aa02c1 Mon Sep 17 00:00:00 2001 From: Jack Francis Date: Fri, 24 Mar 2023 16:43:14 -0700 Subject: [PATCH] remove strict AKS create validations for spec.controlPlaneEndpoint --- api/v1beta1/azuremanagedcluster_webhook.go | 31 ------- .../azuremanagedcluster_webhook_test.go | 37 ++------- .../azuremanagedcontrolplane_webhook.go | 42 ++-------- .../azuremanagedcontrolplane_webhook_test.go | 83 +------------------ azure/scope/managedcontrolplane.go | 8 +- controllers/azuremanagedcluster_controller.go | 9 +- docs/book/src/topics/managedcluster.md | 5 +- 7 files changed, 24 insertions(+), 191 deletions(-) diff --git a/api/v1beta1/azuremanagedcluster_webhook.go b/api/v1beta1/azuremanagedcluster_webhook.go index 0db03ba4544..a6d51588cbb 100644 --- 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 index 7131b3c83a5..1298bf8f18a 100644 --- 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,7 +190,7 @@ 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{ @@ -219,10 +198,10 @@ func TestAzureManagedCluster_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)", amc: &AzureManagedCluster{ Spec: AzureManagedClusterSpec{ ControlPlaneEndpoint: clusterv1.APIEndpoint{ @@ -230,7 +209,7 @@ func TestAzureManagedCluster_ValidateCreate(t *testing.T) { }, }, }, - wantErr: true, + wantErr: false, }, } for _, tc := range tests { diff --git a/api/v1beta1/azuremanagedcontrolplane_webhook.go b/api/v1beta1/azuremanagedcontrolplane_webhook.go index 6fc6ed54c95..401bff2e703 100644 --- 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 index c62161c6906..c4bd7e4a021 100644 --- 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 index 6907e42bb7d..a48baa86636 100644 --- 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 index 7f70ca0d13d..4411beeb779 100644 --- 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 index 400450ffb5e..067e651fc0b 100644 --- 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 | |