add webhook validations and test

kubernetes-sigs · Jun 21, 2022 · 6e20ef6 · 6e20ef6
1 parent 6095d7b
commit 6e20ef6
Show file tree

Hide file tree

Showing 7 changed files with 165 additions and 5 deletions.
diff --git a/api/v1beta1/azuremachine_validation.go b/api/v1beta1/azuremachine_validation.go
@@ -54,9 +54,20 @@ func ValidateAzureMachineSpec(spec AzureMachineSpec) field.ErrorList {
 		allErrs = append(allErrs, errs...)
 	}
 
+	if errs := ValidateNetwork(spec.SubnetName, spec.NetworkInterfaces, field.NewPath("networkInterfaces")); len(errs) > 0 {
+		allErrs = append(allErrs, errs...)
+	}
+
 	return allErrs
 }
 
+func ValidateNetwork(subnetName string, networkInterfaces []AzureNetworkInterface, fldPath *field.Path) field.ErrorList {
+	if (networkInterfaces != nil) && len(networkInterfaces) > 0 && subnetName != "" {
+		return field.ErrorList{field.Invalid(fldPath, networkInterfaces, "cannot set both NetworkInterfaces and machine SubnetName")}
+	}
+	return field.ErrorList{}
+}
+
 // ValidateSSHKey validates an SSHKey.
 func ValidateSSHKey(sshKey string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}

diff --git a/api/v1beta1/azuremachine_webhook.go b/api/v1beta1/azuremachine_webhook.go
@@ -136,6 +136,20 @@ func (m *AzureMachine) ValidateUpdate(oldRaw runtime.Object) error {
 		)
 	}
 
+	if !reflect.DeepEqual(m.Spec.SubnetName, old.Spec.SubnetName) {
+		allErrs = append(allErrs,
+			field.Invalid(field.NewPath("spec", "subnetName"),
+				m.Spec.SecurityProfile, "field is immutable"),
+		)
+	}
+
+	if !reflect.DeepEqual(m.Spec.NetworkInterfaces, old.Spec.NetworkInterfaces) {
+		allErrs = append(allErrs,
+			field.Invalid(field.NewPath("spec", "networkInterfaces"),
+				m.Spec.SecurityProfile, "field is immutable"),
+		)
+	}
+
 	if len(allErrs) == 0 {
 		return nil
 	}

diff --git a/api/v1beta1/azuremachine_webhook_test.go b/api/v1beta1/azuremachine_webhook_test.go
@@ -103,6 +103,21 @@ func TestAzureMachine_ValidateCreate(t *testing.T) {
 			machine: createMachineWithOsDiskCacheType("invalid_cache_type"),
 			wantErr: true,
 		},
+		{
+			name:    "azuremachine with invalid network configuration",
+			machine: createrMachineWithNetworkConfig("subnet", []AzureNetworkInterface{{SubnetName: "subnet1"}}),
+			wantErr: true,
+		},
+		{
+			name:    "azuremachine with valid legacy network configuration",
+			machine: createrMachineWithNetworkConfig("subnet", []AzureNetworkInterface{}),
+			wantErr: false,
+		},
+		{
+			name:    "azuremachine with valid network configuration",
+			machine: createrMachineWithNetworkConfig("", []AzureNetworkInterface{{SubnetName: "subnet"}}),
+			wantErr: false,
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -509,6 +524,62 @@ func TestAzureMachine_ValidateUpdate(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "invalidTest: azuremachine.spec.subnetName is immutable",
+			oldMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					SubnetName: "subnet1",
+				},
+			},
+			newMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					SubnetName: "subnet2",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalidTest: azuremachine.spec.subnetName is immutable",
+			oldMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					SubnetName: "subnet1",
+				},
+			},
+			newMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					SubnetName: "subnet2",
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "validTest: azuremachine.spec.networkInterfaces is immutable",
+			oldMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					NetworkInterfaces: []AzureNetworkInterface{{SubnetName: "subnet"}},
+				},
+			},
+			newMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					NetworkInterfaces: []AzureNetworkInterface{{SubnetName: "subnet"}},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalidtest: azuremachine.spec.networkInterfaces is immutable",
+			oldMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					NetworkInterfaces: []AzureNetworkInterface{{SubnetName: "subnet1"}},
+				},
+			},
+			newMachine: &AzureMachine{
+				Spec: AzureMachineSpec{
+					NetworkInterfaces: []AzureNetworkInterface{{SubnetName: "subnet2"}},
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -550,6 +621,17 @@ func TestAzureMachine_Default(t *testing.T) {
 	}
 }
 
+func createrMachineWithNetworkConfig(subnetName string, interfaces []AzureNetworkInterface) *AzureMachine {
+	return &AzureMachine{
+		Spec: AzureMachineSpec{
+			SubnetName:        subnetName,
+			NetworkInterfaces: interfaces,
+			OSDisk:            validOSDisk,
+			SSHPublicKey:      validSSHPublicKey,
+		},
+	}
+}
+
 func createMachineWithSharedImage(subscriptionID, resourceGroup, name, gallery, version string) *AzureMachine {
 	image := &Image{
 		SharedGallery: &AzureSharedGalleryImage{

diff --git a/api/v1beta1/azuremachinetemplate_webhook.go b/api/v1beta1/azuremachinetemplate_webhook.go
@@ -57,6 +57,10 @@ func (r *AzureMachineTemplate) ValidateCreate() error {
 		)
 	}
 
+	if (r.Spec.Template.Spec.NetworkInterfaces != nil) && len(r.Spec.Template.Spec.NetworkInterfaces) > 0 && r.Spec.Template.Spec.SubnetName != "" {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "networkInterfaces"), r.Spec.Template.Spec.NetworkInterfaces, "cannot set both NetworkInterfaces and machine SubnetName"))
+	}
+
 	if len(allErrs) == 0 {
 		return nil
 	}

diff --git a/azure/scope/machine.go b/azure/scope/machine.go
@@ -283,11 +283,7 @@ func (m *MachineScope) NICSpecs() []azure.ResourceSpecGetter {
 			IPConfigs:             []networkinterfaces.IPConfig{},
 		}
 
-		if n.SubnetName == "" {
-			spec.SubnetName = m.Subnet().Name
-		} else {
-			spec.SubnetName = n.SubnetName
-		}
+		spec.SubnetName = n.SubnetName
 
 		// Check for control plane interface setup on interface 0
 		if m.Role() == infrav1.ControlPlane && i == 0 {

diff --git a/exp/api/v1beta1/azuremachinepool_webhook.go b/exp/api/v1beta1/azuremachinepool_webhook.go
@@ -77,6 +77,7 @@ func (amp *AzureMachinePool) Validate(old runtime.Object) error {
 		amp.ValidateUserAssignedIdentity,
 		amp.ValidateStrategy(),
 		amp.ValidateSystemAssignedIdentity(old),
+		amp.ValidateNetwork,
 	}
 
 	var errs []error
@@ -89,6 +90,13 @@ func (amp *AzureMachinePool) Validate(old runtime.Object) error {
 	return kerrors.NewAggregate(errs)
 }
 
+func (amp *AzureMachinePool) ValidateNetwork() error {
+	if (amp.Spec.Template.NetworkInterfaces != nil) && len(amp.Spec.Template.NetworkInterfaces) > 0 && amp.Spec.Template.SubnetName != "" {
+		return errors.New("cannot set both NetworkInterfaces and machine SubnetName")
+	}
+	return nil
+}
+
 // ValidateImage of an AzureMachinePool.
 func (amp *AzureMachinePool) ValidateImage() error {
 	if amp.Spec.Template.Image != nil {

diff --git a/exp/api/v1beta1/azuremachinepool_webhook_test.go b/exp/api/v1beta1/azuremachinepool_webhook_test.go
@@ -28,6 +28,7 @@ import (
 	"golang.org/x/crypto/ssh"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/uuid"
+	"sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 )
 
@@ -135,6 +136,21 @@ func TestAzureMachinePool_ValidateCreate(t *testing.T) {
 			}),
 			wantErr: false,
 		},
+		{
+			name:    "azuremachinepool with valid legacy network configuration",
+			amp:     createMachinePoolWithNetworkConfig("testSubnet", []v1beta1.AzureNetworkInterface{}),
+			wantErr: false,
+		},
+		{
+			name:    "azuremachinepool with invalid legacy network configuration",
+			amp:     createMachinePoolWithNetworkConfig("testSubnet", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet"}}),
+			wantErr: true,
+		},
+		{
+			name:    "azuremachinepool with valid networkinterface configuration",
+			amp:     createMachinePoolWithNetworkConfig("", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet"}}),
+			wantErr: false,
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -210,6 +226,24 @@ func TestAzureMachinePool_ValidateUpdate(t *testing.T) {
 			}),
 			wantErr: false,
 		},
+		{
+			name:    "azuremachinepool with valid network interface config",
+			oldAMP:  createMachinePoolWithNetworkConfig("", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet"}}),
+			amp:     createMachinePoolWithNetworkConfig("", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet2"}}),
+			wantErr: false,
+		},
+		{
+			name:    "azuremachinepool with valid network interface config",
+			oldAMP:  createMachinePoolWithNetworkConfig("", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet"}}),
+			amp:     createMachinePoolWithNetworkConfig("subnet", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet2"}}),
+			wantErr: true,
+		},
+		{
+			name:    "azuremachinepool with valid network interface config",
+			oldAMP:  createMachinePoolWithNetworkConfig("subnet", []v1beta1.AzureNetworkInterface{}),
+			amp:     createMachinePoolWithNetworkConfig("subnet", []v1beta1.AzureNetworkInterface{{SubnetName: "testSubnet2"}}),
+			wantErr: true,
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -303,6 +337,17 @@ func createMachinePoolWithSharedImage(subscriptionID, resourceGroup, name, galle
 	}
 }
 
+func createMachinePoolWithNetworkConfig(subnetName string, interfaces []v1beta1.AzureNetworkInterface) *AzureMachinePool {
+	return &AzureMachinePool{
+		Spec: AzureMachinePoolSpec{
+			Template: AzureMachinePoolMachineTemplate{
+				SubnetName:        subnetName,
+				NetworkInterfaces: interfaces,
+			},
+		},
+	}
+}
+
 func createMachinePoolWithImageByID(imageID string, terminateNotificationTimeout *int) *AzureMachinePool {
 	image := infrav1.Image{
 		ID: &imageID,