Merge pull request #2637 from nojnhuh/aks-public-ip

k8s-ci-robot · web-flow · commit 7a8cd67bb2b2 · 2022-09-08T13:49:23.000-07:00
add enableNodePublicIP to managed machine pools
diff --git a/azure/converters/managedagentpool.go b/azure/converters/managedagentpool.go
@@ -42,5 +42,6 @@ func AgentPoolToManagedClusterAgentPoolProfile(pool containerservice.AgentPool)
 		OsDiskType:          properties.OsDiskType,
 		NodeLabels:          properties.NodeLabels,
 		EnableUltraSSD:      properties.EnableUltraSSD,
+		EnableNodePublicIP:  properties.EnableNodePublicIP,
 	}
 }
diff --git a/azure/scope/managedmachinepool.go b/azure/scope/managedmachinepool.go
@@ -169,12 +169,13 @@ func buildAgentPoolSpec(managedControlPlane *infrav1exp.AzureManagedControlPlane
 			managedControlPlane.Spec.VirtualNetwork.Name,
 			managedControlPlane.Spec.VirtualNetwork.Subnet.Name,
 		),
-		Mode:              managedMachinePool.Spec.Mode,
-		MaxPods:           managedMachinePool.Spec.MaxPods,
-		AvailabilityZones: managedMachinePool.Spec.AvailabilityZones,
-		OsDiskType:        managedMachinePool.Spec.OsDiskType,
-		EnableUltraSSD:    managedMachinePool.Spec.EnableUltraSSD,
-		Headers:           maps.FilterByKeyPrefix(agentPoolAnnotations, azure.CustomHeaderPrefix),
+		Mode:               managedMachinePool.Spec.Mode,
+		MaxPods:            managedMachinePool.Spec.MaxPods,
+		AvailabilityZones:  managedMachinePool.Spec.AvailabilityZones,
+		OsDiskType:         managedMachinePool.Spec.OsDiskType,
+		EnableUltraSSD:     managedMachinePool.Spec.EnableUltraSSD,
+		Headers:            maps.FilterByKeyPrefix(agentPoolAnnotations, azure.CustomHeaderPrefix),
+		EnableNodePublicIP: managedMachinePool.Spec.EnableNodePublicIP,
 	}
 
 	if managedMachinePool.Spec.OSDiskSizeGB != nil {
diff --git a/azure/services/agentpools/spec.go b/azure/services/agentpools/spec.go
@@ -89,6 +89,9 @@ type AgentPoolSpec struct {
 
 	// Headers is the list of headers to add to the HTTP requests to update this resource.
 	Headers map[string]string
+
+	// EnableNodePublicIP controls whether or not nodes in the agent pool each have a public IP address.
+	EnableNodePublicIP *bool `json:"enableNodePublicIP,omitempty"`
 }
 
 // ResourceName returns the name of the agent pool.
@@ -206,6 +209,7 @@ func (s *AgentPoolSpec) Parameters(existing interface{}) (params interface{}, er
 			Type:                containerservice.AgentPoolTypeVirtualMachineScaleSets,
 			VMSize:              sku,
 			VnetSubnetID:        vnetSubnetID,
+			EnableNodePublicIP:  s.EnableNodePublicIP,
 		},
 	}, nil
 }
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedmachinepools.yaml
@@ -205,6 +205,10 @@ spec:
                 items:
                   type: string
                 type: array
+              enableNodePublicIP:
+                description: EnableNodePublicIP controls whether or not nodes in the
+                  pool each have a public IP address.
+                type: boolean
               enableUltraSSD:
                 description: EnableUltraSSD enables the storage type UltraSSD_LRS
                   for the agent pool.
diff --git a/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go b/exp/api/v1alpha3/azuremanagedmachinepool_conversion.go
@@ -45,6 +45,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
 	dst.Spec.OSType = restored.Spec.OSType
 	dst.Spec.NodeLabels = restored.Spec.NodeLabels
 	dst.Spec.EnableUltraSSD = restored.Spec.EnableUltraSSD
+	dst.Spec.EnableNodePublicIP = restored.Spec.EnableNodePublicIP
 
 	dst.Status.LongRunningOperationStates = restored.Status.LongRunningOperationStates
 	dst.Status.Conditions = restored.Status.Conditions
diff --git a/exp/api/v1alpha3/zz_generated.conversion.go b/exp/api/v1alpha3/zz_generated.conversion.go
diff --git a/exp/api/v1alpha4/azuremanagedmachinepool_conversion.go b/exp/api/v1alpha4/azuremanagedmachinepool_conversion.go
@@ -45,6 +45,7 @@ func (src *AzureManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error {
 	dst.Spec.OSType = restored.Spec.OSType
 	dst.Spec.NodeLabels = restored.Spec.NodeLabels
 	dst.Spec.EnableUltraSSD = restored.Spec.EnableUltraSSD
+	dst.Spec.EnableNodePublicIP = restored.Spec.EnableNodePublicIP
 
 	dst.Status.LongRunningOperationStates = restored.Status.LongRunningOperationStates
 	dst.Status.Conditions = restored.Status.Conditions
diff --git a/exp/api/v1alpha4/zz_generated.conversion.go b/exp/api/v1alpha4/zz_generated.conversion.go
diff --git a/exp/api/v1beta1/azuremanagedmachinepool_types.go b/exp/api/v1beta1/azuremanagedmachinepool_types.go
@@ -98,6 +98,10 @@ type AzureManagedMachinePoolSpec struct {
 	// +kubebuilder:validation:Enum=Linux;Windows
 	// +optional
 	OSType *string `json:"osType,omitempty"`
+
+	// EnableNodePublicIP controls whether or not nodes in the pool each have a public IP address.
+	// +optional
+	EnableNodePublicIP *bool `json:"enableNodePublicIP,omitempty"`
 }
 
 // ManagedMachinePoolScaling specifies scaling options.
diff --git a/exp/api/v1beta1/azuremanagedmachinepool_webhook.go b/exp/api/v1beta1/azuremanagedmachinepool_webhook.go
@@ -197,32 +197,19 @@ func (m *AzureManagedMachinePool) ValidateUpdate(oldRaw runtime.Object, client c
 		}
 	}
 
-	if old.Spec.EnableUltraSSD != nil {
-		// Prevent EnabledUltraSSD modification if it was already set to some value
-		if m.Spec.EnableUltraSSD == nil {
-			// unsetting the field is not allowed
-			allErrs = append(allErrs,
-				field.Invalid(
-					field.NewPath("Spec", "EnableUltraSSD"),
-					m.Spec.EnableUltraSSD,
-					"field is immutable, unsetting is not allowed"))
-		} else if *m.Spec.EnableUltraSSD != *old.Spec.EnableUltraSSD {
-			// changing the field is not allowed
-			allErrs = append(allErrs,
-				field.Invalid(
-					field.NewPath("Spec", "EnableUltraSSD"),
-					m.Spec.EnableUltraSSD,
-					"field is immutable"))
-		}
-	} else {
-		if m.Spec.EnableUltraSSD != nil {
-			allErrs = append(allErrs,
-				field.Invalid(
-					field.NewPath("Spec", "EnableUltraSSD"),
-					m.Spec.EnableUltraSSD,
-					"field is immutable, unsetting is not allowed"))
-		}
+	if err := validateBoolPtrImmutable(
+		field.NewPath("Spec", "EnableUltraSSD"),
+		old.Spec.EnableUltraSSD,
+		m.Spec.EnableUltraSSD); err != nil {
+		allErrs = append(allErrs, err)
+	}
+	if err := validateBoolPtrImmutable(
+		field.NewPath("Spec", "EnableNodePublicIP"),
+		old.Spec.EnableNodePublicIP,
+		m.Spec.EnableNodePublicIP); err != nil {
+		allErrs = append(allErrs, err)
 	}
+
 	if len(allErrs) != 0 {
 		return apierrors.NewInvalid(GroupVersion.WithKind("AzureManagedMachinePool").GroupKind(), m.Name, allErrs)
 	}
@@ -336,3 +323,21 @@ func ensureStringSlicesAreEqual(a []string, b []string) bool {
 	}
 	return true
 }
+
+func validateBoolPtrImmutable(path *field.Path, oldVal, newVal *bool) *field.Error {
+	if oldVal != nil {
+		// Prevent modification if it was already set to some value
+		if newVal == nil {
+			// unsetting the field is not allowed
+			return field.Invalid(path, newVal, "field is immutable, unsetting is not allowed")
+		}
+		if *newVal != *oldVal {
+			// changing the field is not allowed
+			return field.Invalid(path, newVal, "field is immutable")
+		}
+	} else if newVal != nil {
+		return field.Invalid(path, newVal, "field is immutable, setting is not allowed")
+	}
+
+	return nil
+}
diff --git a/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go b/exp/api/v1beta1/azuremanagedmachinepool_webhook_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -393,20 +394,6 @@ func TestAzureManagedMachinePoolUpdatingWebhook(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		{
-			name: "Unexpected error, value EnableUltraSSD is unchanged",
-			new: &AzureManagedMachinePool{
-				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: to.BoolPtr(false),
-				},
-			},
-			old: &AzureManagedMachinePool{
-				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: to.BoolPtr(false),
-				},
-			},
-			wantErr: false,
-		},
 		{
 			name: "EnableUltraSSD feature is immutable and currently enabled on this agentpool",
 			new: &AzureManagedMachinePool{
@@ -422,43 +409,29 @@ func TestAzureManagedMachinePoolUpdatingWebhook(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "EnableUltraSSD feature is immutable and currently disabled on this agentpool",
+			name: "Unexpected error, value EnableNodePublicIP is unchanged",
 			new: &AzureManagedMachinePool{
 				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: to.BoolPtr(true),
+					EnableNodePublicIP: to.BoolPtr(true),
 				},
 			},
 			old: &AzureManagedMachinePool{
 				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: to.BoolPtr(false),
+					EnableNodePublicIP: to.BoolPtr(true),
 				},
 			},
-			wantErr: true,
-		},
-		{
-			name: "EnableUltraSSD feature is immutable and currently disabled on this agentpool",
-			new: &AzureManagedMachinePool{
-				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: nil,
-				},
-			},
-			old: &AzureManagedMachinePool{
-				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: to.BoolPtr(false),
-				},
-			},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
-			name: "EnableUltraSSD feature is immutable and currently enabled on this agentpool",
+			name: "EnableNodePublicIP feature is immutable and currently enabled on this agentpool",
 			new: &AzureManagedMachinePool{
 				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: nil,
+					EnableNodePublicIP: to.BoolPtr(false),
 				},
 			},
 			old: &AzureManagedMachinePool{
 				Spec: AzureManagedMachinePoolSpec{
-					EnableUltraSSD: to.BoolPtr(true),
+					EnableNodePublicIP: to.BoolPtr(true),
 				},
 			},
 			wantErr: true,
@@ -477,6 +450,76 @@ func TestAzureManagedMachinePoolUpdatingWebhook(t *testing.T) {
 	}
 }
 
+func TestValidateBoolPtrImmutable(t *testing.T) {
+	tests := []struct {
+		name    string
+		oldVal  *bool
+		newVal  *bool
+		wantErr bool
+	}{
+		{
+			name:    "true to true ok",
+			oldVal:  to.BoolPtr(true),
+			newVal:  to.BoolPtr(true),
+			wantErr: false,
+		},
+		{
+			name:    "false to false ok",
+			oldVal:  to.BoolPtr(false),
+			newVal:  to.BoolPtr(false),
+			wantErr: false,
+		},
+		{
+			name:    "true to false bad",
+			oldVal:  to.BoolPtr(true),
+			newVal:  to.BoolPtr(false),
+			wantErr: true,
+		},
+		{
+			name:    "false to true bad",
+			oldVal:  to.BoolPtr(false),
+			newVal:  to.BoolPtr(true),
+			wantErr: true,
+		},
+		{
+			name:    "false to nil bad",
+			oldVal:  to.BoolPtr(false),
+			newVal:  nil,
+			wantErr: true,
+		},
+		{
+			name:    "true to nil bad",
+			oldVal:  to.BoolPtr(true),
+			newVal:  nil,
+			wantErr: true,
+		},
+		{
+			name:    "nil to false bad",
+			oldVal:  nil,
+			newVal:  to.BoolPtr(false),
+			wantErr: true,
+		},
+		{
+			name:    "nil to true bad",
+			oldVal:  nil,
+			newVal:  to.BoolPtr(true),
+			wantErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			g := NewWithT(t)
+			err := validateBoolPtrImmutable(field.NewPath("test"), test.oldVal, test.newVal)
+			if test.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+}
+
 func TestAzureManagedMachinePool_ValidateCreate(t *testing.T) {
 	g := NewWithT(t)
 
diff --git a/exp/api/v1beta1/zz_generated.deepcopy.go b/exp/api/v1beta1/zz_generated.deepcopy.go
diff --git a/templates/cluster-template-aks.yaml b/templates/cluster-template-aks.yaml
diff --git a/templates/flavors/aks/cluster-template.yaml b/templates/flavors/aks/cluster-template.yaml
@@ -74,6 +74,7 @@ spec:
   mode: System
   osDiskSizeGB: 30
   sku: "${AZURE_NODE_MACHINE_TYPE}"
+  enableNodePublicIP: false
 ---
 # Deploy a second agent pool with the same number of machines, but using potentially different infrastructure.
 apiVersion: cluster.x-k8s.io/v1beta1
@@ -104,3 +105,4 @@ spec:
   mode: User
   osDiskSizeGB: 40
   sku: "${AZURE_NODE_MACHINE_TYPE}"
+  enableNodePublicIP: false
diff --git a/templates/test/ci/cluster-template-prow-aks.yaml b/templates/test/ci/cluster-template-prow-aks.yaml

Original file line number	Diff line number	Diff line change
`@@ -42,5 +42,6 @@ func AgentPoolToManagedClusterAgentPoolProfile(pool containerservice.AgentPool)`
`42`	`42`	`OsDiskType: properties.OsDiskType,`
`43`	`43`	`NodeLabels: properties.NodeLabels,`
`44`	`44`	`EnableUltraSSD: properties.EnableUltraSSD,`
	`45`	`+ EnableNodePublicIP: properties.EnableNodePublicIP,`
`45`	`46`	`}`
`46`	`47`	`}`