Merge pull request #10638 from sbueringer/pr-fix-webhooks

🐛 KCPTemplate & DockerClusterTemplate webhook: default before immutability check
kubernetes-sigs · May 17, 2024 · 760964a · 760964a
2 parents b46a7b5 + 155de2c
commit 760964a
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 2 deletions.
diff --git a/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go b/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go
@@ -95,7 +95,7 @@ func (webhook *KubeadmControlPlaneTemplate) ValidateCreate(_ context.Context, ob
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (webhook *KubeadmControlPlaneTemplate) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+func (webhook *KubeadmControlPlaneTemplate) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
 	var allErrs field.ErrorList
 
 	oldK, ok := oldObj.(*controlplanev1.KubeadmControlPlaneTemplate)
@@ -108,6 +108,13 @@ func (webhook *KubeadmControlPlaneTemplate) ValidateUpdate(_ context.Context, ol
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a KubeadmControlPlaneTemplate but got a %T", newObj))
 	}
 
+	if err := webhook.Default(ctx, oldK); err != nil {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new KubeadmControlPlaneTemplate: failed to default old object: %v", err))
+	}
+	if err := webhook.Default(ctx, newK); err != nil {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new KubeadmControlPlaneTemplate: failed to default new object: %v", err))
+	}
+
 	if !reflect.DeepEqual(newK.Spec.Template.Spec, oldK.Spec.Template.Spec) {
 		allErrs = append(allErrs,
 			field.Invalid(field.NewPath("spec", "template", "spec"), newK, kubeadmControlPlaneTemplateImmutableMsg),

diff --git a/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate_test.go b/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate_test.go
@@ -156,3 +156,77 @@ func TestKubeadmControlPlaneTemplateValidationMetadata(t *testing.T) {
 		g.Expect(warnings).To(BeEmpty())
 	})
 }
+
+func TestKubeadmControlPlaneTemplateUpdateValidation(t *testing.T) {
+	t.Run("update KubeadmControlPlaneTemplate should pass if only defaulted fields are different", func(t *testing.T) {
+		g := NewWithT(t)
+		oldKCPTemplate := &controlplanev1.KubeadmControlPlaneTemplate{
+			Spec: controlplanev1.KubeadmControlPlaneTemplateSpec{
+				Template: controlplanev1.KubeadmControlPlaneTemplateResource{
+					Spec: controlplanev1.KubeadmControlPlaneTemplateResourceSpec{
+						MachineTemplate: &controlplanev1.KubeadmControlPlaneTemplateMachineTemplate{
+							NodeDrainTimeout: &metav1.Duration{Duration: time.Duration(10) * time.Minute},
+						},
+					},
+				},
+			},
+		}
+		newKCPTemplate := &controlplanev1.KubeadmControlPlaneTemplate{
+			Spec: controlplanev1.KubeadmControlPlaneTemplateSpec{
+				Template: controlplanev1.KubeadmControlPlaneTemplateResource{
+					Spec: controlplanev1.KubeadmControlPlaneTemplateResourceSpec{
+						KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
+							// Only this field is different, but defaulting will set it as well, so this should pass the immutability check.
+							Format: bootstrapv1.CloudConfig,
+						},
+						MachineTemplate: &controlplanev1.KubeadmControlPlaneTemplateMachineTemplate{
+							NodeDrainTimeout: &metav1.Duration{Duration: time.Duration(10) * time.Minute},
+						},
+					},
+				},
+			},
+		}
+		webhook := &KubeadmControlPlaneTemplate{}
+		warnings, err := webhook.ValidateUpdate(ctx, oldKCPTemplate, newKCPTemplate)
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(warnings).To(BeEmpty())
+	})
+	t.Run("update kubeadmcontrolplanetemplate should not pass if fields are different", func(t *testing.T) {
+		g := NewWithT(t)
+		oldKCPTemplate := &controlplanev1.KubeadmControlPlaneTemplate{
+			Spec: controlplanev1.KubeadmControlPlaneTemplateSpec{
+				Template: controlplanev1.KubeadmControlPlaneTemplateResource{
+					Spec: controlplanev1.KubeadmControlPlaneTemplateResourceSpec{
+						MachineTemplate: &controlplanev1.KubeadmControlPlaneTemplateMachineTemplate{
+							NodeDrainTimeout: &metav1.Duration{Duration: time.Duration(10) * time.Minute},
+						},
+					},
+				},
+			},
+		}
+		newKCPTemplate := &controlplanev1.KubeadmControlPlaneTemplate{
+			Spec: controlplanev1.KubeadmControlPlaneTemplateSpec{
+				Template: controlplanev1.KubeadmControlPlaneTemplateResource{
+					Spec: controlplanev1.KubeadmControlPlaneTemplateResourceSpec{
+						KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
+							// Defaulting will set this field as well.
+							Format: bootstrapv1.CloudConfig,
+							// This will fail the immutability check.
+							PreKubeadmCommands: []string{
+								"new-cmd",
+							},
+						},
+						MachineTemplate: &controlplanev1.KubeadmControlPlaneTemplateMachineTemplate{
+							NodeDrainTimeout: &metav1.Duration{Duration: time.Duration(10) * time.Minute},
+						},
+					},
+				},
+			},
+		}
+		webhook := &KubeadmControlPlaneTemplate{}
+		warnings, err := webhook.ValidateUpdate(ctx, oldKCPTemplate, newKCPTemplate)
+		g.Expect(err).To(HaveOccurred())
+		g.Expect(err.Error()).To(ContainSubstring("KubeadmControlPlaneTemplate spec.template.spec field is immutable"))
+		g.Expect(warnings).To(BeEmpty())
+	})
+}
diff --git a/test/infrastructure/docker/internal/webhooks/dockerclustertemplate_webhook.go b/test/infrastructure/docker/internal/webhooks/dockerclustertemplate_webhook.go
@@ -91,7 +91,7 @@ func (webhook *DockerClusterTemplate) ValidateCreate(_ context.Context, obj runt
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (webhook *DockerClusterTemplate) ValidateUpdate(_ context.Context, oldRaw, newRaw runtime.Object) (admission.Warnings, error) {
+func (webhook *DockerClusterTemplate) ValidateUpdate(ctx context.Context, oldRaw, newRaw runtime.Object) (admission.Warnings, error) {
 	var allErrs field.ErrorList
 	oldTemplate, ok := oldRaw.(*infrav1.DockerClusterTemplate)
 	if !ok {
@@ -101,6 +101,14 @@ func (webhook *DockerClusterTemplate) ValidateUpdate(_ context.Context, oldRaw,
 	if !ok {
 		return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a DockerClusterTemplate but got a %T", newRaw))
 	}
+
+	if err := webhook.Default(ctx, oldTemplate); err != nil {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new DockerClusterTemplate: failed to default old object: %v", err))
+	}
+	if err := webhook.Default(ctx, newTemplate); err != nil {
+		return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new DockerClusterTemplate: failed to default new object: %v", err))
+	}
+
 	if !reflect.DeepEqual(newTemplate.Spec.Template.Spec, oldTemplate.Spec.Template.Spec) {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "template", "spec"), newTemplate, dockerClusterTemplateImmutableMsg))
 	}