From c787c5ae10f5f23d06128a215b802befffe3798b Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Fri, 17 May 2024 12:36:25 +0200 Subject: [PATCH] KCPTemplate & DockerClusterTemplate webhook: default before immutability check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../webhooks/kubeadmcontrolplanetemplate.go | 9 ++- .../kubeadmcontrolplanetemplate_test.go | 74 +++++++++++++++++++ .../webhooks/dockerclustertemplate_webhook.go | 10 ++- 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go b/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go index d0178ee92844..c74596cc413e 100644 --- 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 index 3780ec0bb190..7da19acdbb4a 100644 --- 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 index b77f0868a228..d36f09d9927f 100644 --- 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)) }