From 6e20ef6cdef6acbb667ed04b77b6991349154320 Mon Sep 17 00:00:00 2001 From: Brian Lieberman Date: Tue, 21 Jun 2022 16:13:19 -0400 Subject: [PATCH] add webhook validations and test --- api/v1beta1/azuremachine_validation.go | 11 +++ api/v1beta1/azuremachine_webhook.go | 14 ++++ api/v1beta1/azuremachine_webhook_test.go | 82 +++++++++++++++++++ api/v1beta1/azuremachinetemplate_webhook.go | 4 + azure/scope/machine.go | 6 +- exp/api/v1beta1/azuremachinepool_webhook.go | 8 ++ .../v1beta1/azuremachinepool_webhook_test.go | 45 ++++++++++ 7 files changed, 165 insertions(+), 5 deletions(-) diff --git a/api/v1beta1/azuremachine_validation.go b/api/v1beta1/azuremachine_validation.go index 7dab64472f6..20782b3ceb9 100644 --- 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 index d27cea2774e..fd430f89e70 100644 --- 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 index a278ec92b45..2b16e7820fe 100644 --- 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 index 060cbf4eb4b..701fbd48f00 100644 --- 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 index 2f3d04e98b6..742e3b091b0 100644 --- 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 index 0b08850c19e..c5718a7eeb3 100644 --- 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 index b157fb2e7cf..ad9ce7946b1 100644 --- 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,