Merge pull request #2641 from jackfrancis/azuremachinetemplate-webhook-capi-1.2

AzureMachineTemplate webhooks dry-run

Author: k8s-ci-robot

api/v1beta1/azuremachinetemplate_webhook.go

@@ -17,13 +17,17 @@ limitations under the License.
 package v1beta1
 
 import (
+	"context"
+	"fmt"
 	"reflect"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"sigs.k8s.io/cluster-api/util/topology"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 // AzureMachineTemplateImmutableMsg ...
@@ -36,40 +40,50 @@ const (
 func (r *AzureMachineTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(r).
+		WithValidator(r).
+		WithDefaulter(r).
 		Complete()
 }
 
 // +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachinetemplate,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=azuremachinetemplates,versions=v1beta1,name=default.azuremachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 // +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-azuremachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=azuremachinetemplates,versions=v1beta1,name=validation.azuremachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
 
-var _ webhook.Defaulter = &AzureMachineTemplate{}
-var _ webhook.Validator = &AzureMachineTemplate{}
+var _ webhook.CustomDefaulter = &AzureMachineTemplate{}
+var _ webhook.CustomValidator = &AzureMachineTemplate{}
 
-// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
-func (r *AzureMachineTemplate) ValidateCreate() error {
-	spec := r.Spec.Template.Spec
+// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type.
+func (r *AzureMachineTemplate) ValidateCreate(ctx context.Context, obj runtime.Object) error {
+	t := obj.(*AzureMachineTemplate)
+	spec := t.Spec.Template.Spec
 
 	allErrs := ValidateAzureMachineSpec(spec)
 
-	if r.Spec.Template.Spec.RoleAssignmentName != "" {
+	if spec.RoleAssignmentName != "" {
 		allErrs = append(allErrs,
-			field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "roleAssignmentName"), r, AzureMachineTemplateRoleAssignmentNameMsg),
+			field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "roleAssignmentName"), t, AzureMachineTemplateRoleAssignmentNameMsg),
 		)
 	}
 
 	if len(allErrs) == 0 {
 		return nil
 	}
 
-	return apierrors.NewInvalid(GroupVersion.WithKind("AzureMachineTemplate").GroupKind(), r.Name, allErrs)
+	return apierrors.NewInvalid(GroupVersion.WithKind("AzureMachineTemplate").GroupKind(), t.Name, allErrs)
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (r *AzureMachineTemplate) ValidateUpdate(oldRaw runtime.Object) error {
+func (r *AzureMachineTemplate) ValidateUpdate(ctx context.Context, oldRaw runtime.Object, newRaw runtime.Object) error {
 	var allErrs field.ErrorList
 	old := oldRaw.(*AzureMachineTemplate)
+	t := newRaw.(*AzureMachineTemplate)
 
-	if !reflect.DeepEqual(r.Spec.Template.Spec, old.Spec.Template.Spec) {
+	req, err := admission.RequestFromContext(ctx)
+	if err != nil {
+		return apierrors.NewBadRequest(fmt.Sprintf("expected a admission.Request inside context: %v", err))
+	}
+
+	if !topology.ShouldSkipImmutabilityChecks(req, t) &&
+		!reflect.DeepEqual(t.Spec.Template.Spec, old.Spec.Template.Spec) {
 		// The equality failure could be because of default mismatch between v1alpha3 and v1beta1. This happens because
 		// the new object `r` will have run through the default webhooks but the old object `old` would not have so.
 		// This means if the old object was in v1alpha3, it would not get the new defaults set in v1beta1 resulting
@@ -78,35 +92,41 @@ func (r *AzureMachineTemplate) ValidateUpdate(oldRaw runtime.Object) error {
 
 		// We need to set ssh key explicitly, otherwise Default() will create a new one.
 		if old.Spec.Template.Spec.SSHPublicKey == "" {
-			old.Spec.Template.Spec.SSHPublicKey = r.Spec.Template.Spec.SSHPublicKey
+			old.Spec.Template.Spec.SSHPublicKey = t.Spec.Template.Spec.SSHPublicKey
 		}
 
-		old.Default()
+		if err := r.Default(ctx, old); err != nil {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("AzureMachineTemplate"), r, fmt.Sprintf("Unable to apply defaults: %v", err)),
+			)
+		}
 
 		// if it's still not equal, return error.
-		if !reflect.DeepEqual(r.Spec.Template.Spec, old.Spec.Template.Spec) {
+		if !reflect.DeepEqual(t.Spec.Template.Spec, old.Spec.Template.Spec) {
 			allErrs = append(allErrs,
-				field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec"), r, AzureMachineTemplateImmutableMsg),
+				field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec"), t, AzureMachineTemplateImmutableMsg),
 			)
 		}
 	}
 
 	if len(allErrs) == 0 {
 		return nil
 	}
-	return apierrors.NewInvalid(GroupVersion.WithKind("AzureMachineTemplate").GroupKind(), r.Name, allErrs)
+	return apierrors.NewInvalid(GroupVersion.WithKind("AzureMachineTemplate").GroupKind(), t.Name, allErrs)
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
-func (r *AzureMachineTemplate) ValidateDelete() error {
+func (r *AzureMachineTemplate) ValidateDelete(ctx context.Context, obj runtime.Object) error {
 	return nil
 }
 
 // Default implements webhookutil.defaulter so a webhook will be registered for the type.
-func (r *AzureMachineTemplate) Default() {
-	if err := r.Spec.Template.Spec.SetDefaultSSHPublicKey(); err != nil {
+func (r *AzureMachineTemplate) Default(ctx context.Context, obj runtime.Object) error {
+	t := obj.(*AzureMachineTemplate)
+	if err := t.Spec.Template.Spec.SetDefaultSSHPublicKey(); err != nil {
 		ctrl.Log.WithName("SetDefault").Error(err, "SetDefaultSSHPublicKey failed")
 	}
-	r.Spec.Template.Spec.SetDefaultCachingType()
-	r.Spec.Template.Spec.SetDataDisksDefaults()
+	t.Spec.Template.Spec.SetDefaultCachingType()
+	t.Spec.Template.Spec.SetDataDisksDefaults()
+	return nil
 }

api/v1beta1/azuremachinetemplate_webhook_test.go

@@ -17,12 +17,16 @@ limitations under the License.
 package v1beta1
 
 import (
+	"context"
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute"
 	"github.com/Azure/go-autorest/autorest/to"
 	. "github.com/onsi/gomega"
+	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 func TestAzureMachineTemplate_ValidateCreate(t *testing.T) {
@@ -132,8 +136,11 @@ func TestAzureMachineTemplate_ValidateCreate(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		test := test
 		t.Run(test.name, func(t *testing.T) {
-			err := test.machineTemplate.ValidateCreate()
+			t.Parallel()
+			ctx := context.Background()
+			err := test.machineTemplate.ValidateCreate(ctx, test.machineTemplate)
 			if test.wantErr {
 				g.Expect(err).To(HaveOccurred())
 			} else {
@@ -321,11 +328,27 @@ func TestAzureMachineTemplate_ValidateUpdate(t *testing.T) {
 		},
 	}
 
+	// dry-run=true
+	for _, amt := range tests {
+		amt := amt
+		t.Run(amt.name, func(t *testing.T) {
+			t.Parallel()
+			ctx := admission.NewContextWithRequest(context.Background(), admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: pointer.Bool(true)}})
+			err := amt.template.ValidateUpdate(ctx, amt.oldTemplate, amt.template)
+			if amt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+	// dry-run=false
 	for _, amt := range tests {
 		amt := amt
 		t.Run(amt.name, func(t *testing.T) {
 			t.Parallel()
-			err := amt.template.ValidateUpdate(amt.oldTemplate)
+			ctx := admission.NewContextWithRequest(context.Background(), admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{DryRun: pointer.Bool(false)}})
+			err := amt.template.ValidateUpdate(ctx, amt.oldTemplate, amt.template)
 			if amt.wantErr {
 				g.Expect(err).To(HaveOccurred())
 			} else {