From 9cbd56ea5d59b461fe2ec3c8bdb18ad8af83feb1 Mon Sep 17 00:00:00 2001 From: swamyan Date: Wed, 13 Sep 2023 19:43:39 +0530 Subject: [PATCH] Move experimental API v1beta1 webhooks to separate package --- exp/internal/webhooks/doc.go | 18 + exp/internal/webhooks/machinepool.go | 181 ++++++++ exp/internal/webhooks/machinepool_test.go | 395 ++++++++++++++++++ internal/test/envtest/environment.go | 2 +- internal/webhooks/exp/doc.go | 18 + .../webhooks/exp/machinepool.go | 73 ++-- .../webhooks/exp/machinepool_test.go | 137 +++--- main.go | 2 +- webhooks/alias.go | 9 + 9 files changed, 747 insertions(+), 88 deletions(-) create mode 100644 exp/internal/webhooks/doc.go create mode 100644 exp/internal/webhooks/machinepool.go create mode 100644 exp/internal/webhooks/machinepool_test.go create mode 100644 internal/webhooks/exp/doc.go rename exp/api/v1beta1/machinepool_webhook.go => internal/webhooks/exp/machinepool.go (62%) rename exp/api/v1beta1/machinepool_webhook_test.go => internal/webhooks/exp/machinepool_test.go (74%) diff --git a/exp/internal/webhooks/doc.go b/exp/internal/webhooks/doc.go new file mode 100644 index 000000000000..ad3cd005e864 --- /dev/null +++ b/exp/internal/webhooks/doc.go @@ -0,0 +1,18 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package webhooks contains external webhook implementations for some of our API types. +package webhooks diff --git a/exp/internal/webhooks/machinepool.go b/exp/internal/webhooks/machinepool.go new file mode 100644 index 000000000000..d06fcf778367 --- /dev/null +++ b/exp/internal/webhooks/machinepool.go @@ -0,0 +1,181 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhooks + +import ( + "context" + "fmt" + "strings" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/utils/pointer" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" + "sigs.k8s.io/cluster-api/feature" + "sigs.k8s.io/cluster-api/util/version" +) + +// MachinePool implements a validation and defaulting webhook for MachinePool. +type MachinePool struct { + *expv1.MachinePool +} + +func (webhook *MachinePool) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(&expv1.MachinePool{}). + WithDefaulter(webhook). + WithValidator(webhook). + Complete() +} + +// +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machinepool,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinepools,versions=v1beta1,name=validation.machinepool.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 +// +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machinepool,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinepools,versions=v1beta1,name=default.machinepool.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 + +var _ webhook.CustomDefaulter = &MachinePool{} +var _ webhook.CustomValidator = &MachinePool{} + +// Default implements webhook.Defaulter so a webhook will be registered for the type. +func (webhook *MachinePool) Default(_ context.Context, obj runtime.Object) error { + m, ok := obj.(*expv1.MachinePool) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", obj)) + } + + if m.Labels == nil { + m.Labels = make(map[string]string) + } + m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName + + if m.Spec.Replicas == nil { + m.Spec.Replicas = pointer.Int32(1) + } + + if m.Spec.MinReadySeconds == nil { + m.Spec.MinReadySeconds = pointer.Int32(0) + } + + if m.Spec.Template.Spec.Bootstrap.ConfigRef != nil && m.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace == "" { + m.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace = m.Namespace + } + + if m.Spec.Template.Spec.InfrastructureRef.Namespace == "" { + m.Spec.Template.Spec.InfrastructureRef.Namespace = m.Namespace + } + + // tolerate version strings without a "v" prefix: prepend it if it's not there. + if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") { + normalizedVersion := "v" + *m.Spec.Template.Spec.Version + m.Spec.Template.Spec.Version = &normalizedVersion + } + return nil +} + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. +func (webhook *MachinePool) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, webhook.validate(nil, nil) +} + +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. +func (webhook *MachinePool) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldMP, ok := oldObj.(*MachinePool) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", oldObj)) + } + newMP, ok := newObj.(*MachinePool) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", newObj)) + } + return nil, webhook.validate(oldMP, newMP) +} + +// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. +func (webhook *MachinePool) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, webhook.validate(nil, nil) +} + +func (webhook *MachinePool) validate(oldObj, newObj *MachinePool) error { + // NOTE: MachinePool is behind MachinePool feature gate flag; the web hook + // must prevent creating newObj objects when the feature flag is disabled. + specPath := field.NewPath("spec") + if !feature.Gates.Enabled(feature.MachinePool) { + return field.Forbidden( + specPath, + "can be set only if the MachinePool feature flag is enabled", + ) + } + var allErrs field.ErrorList + if newObj.Spec.Template.Spec.Bootstrap.ConfigRef == nil && newObj.Spec.Template.Spec.Bootstrap.DataSecretName == nil { + allErrs = append( + allErrs, + field.Required( + specPath.Child("template", "spec", "bootstrap", "data"), + "expected either spec.bootstrap.dataSecretName or spec.bootstrap.configRef to be populated", + ), + ) + } + + if newObj.Spec.Template.Spec.Bootstrap.ConfigRef != nil && newObj.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace != newObj.Namespace { + allErrs = append( + allErrs, + field.Invalid( + specPath.Child("template", "spec", "bootstrap", "configRef", "namespace"), + newObj.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace, + "must match metadata.namespace", + ), + ) + } + + if newObj.Spec.Template.Spec.InfrastructureRef.Namespace != newObj.Namespace { + allErrs = append( + allErrs, + field.Invalid( + specPath.Child("infrastructureRef", "namespace"), + newObj.Spec.Template.Spec.InfrastructureRef.Namespace, + "must match metadata.namespace", + ), + ) + } + + if oldObj != nil && oldObj.Spec.ClusterName != newObj.Spec.ClusterName { + allErrs = append( + allErrs, + field.Forbidden( + specPath.Child("clusterName"), + "field is immutable"), + ) + } + + if newObj.Spec.Template.Spec.Version != nil { + if !version.KubeSemver.MatchString(*newObj.Spec.Template.Spec.Version) { + allErrs = append(allErrs, field.Invalid(specPath.Child("template", "spec", "version"), *newObj.Spec.Template.Spec.Version, "must be a valid semantic version")) + } + } + + // Validate the metadata of the MachinePool template. + allErrs = append(allErrs, newObj.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) + + if len(allErrs) == 0 { + return nil + } + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("MachinePool").GroupKind(), newObj.Name, allErrs) +} diff --git a/exp/internal/webhooks/machinepool_test.go b/exp/internal/webhooks/machinepool_test.go new file mode 100644 index 000000000000..e2c8a58c5218 --- /dev/null +++ b/exp/internal/webhooks/machinepool_test.go @@ -0,0 +1,395 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package webhooks + +import ( + "strings" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilfeature "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/pointer" + ctrl "sigs.k8s.io/controller-runtime" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" + "sigs.k8s.io/cluster-api/feature" + "sigs.k8s.io/cluster-api/internal/webhooks/util" +) + +var ctx = ctrl.SetupSignalHandler() + +func TestMachinePoolDefault(t *testing.T) { + // NOTE: MachinePool feature flag is disabled by default, thus preventing to create or update MachinePool. + // Enabling the feature flag temporarily for this test. + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)() + + g := NewWithT(t) + + m := &MachinePool{ + &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foobar", + }, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Version: pointer.String("1.20.0"), + }, + }, + }, + }, + } + webhook := &MachinePool{} + t.Run("for Machine", util.CustomDefaultValidateTest(ctx, m, webhook)) + g.Expect(m.Default(ctx, m)).To(Succeed()) + + g.Expect(m.Labels[clusterv1.ClusterNameLabel]).To(Equal(m.Spec.ClusterName)) + g.Expect(m.Spec.Replicas).To(Equal(pointer.Int32(1))) + g.Expect(m.Spec.MinReadySeconds).To(Equal(pointer.Int32(0))) + g.Expect(m.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace).To(Equal(m.Namespace)) + g.Expect(m.Spec.Template.Spec.InfrastructureRef.Namespace).To(Equal(m.Namespace)) + g.Expect(m.Spec.Template.Spec.Version).To(Equal(pointer.String("v1.20.0"))) +} + +func TestMachinePoolBootstrapValidation(t *testing.T) { + // NOTE: MachinePool feature flag is disabled by default, thus preventing to create or update MachinePool. + // Enabling the feature flag temporarily for this test. + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)() + tests := []struct { + name string + bootstrap clusterv1.Bootstrap + expectErr bool + }{ + { + name: "should return error if configref and data are nil", + bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: nil}, + expectErr: true, + }, + { + name: "should not return error if dataSecretName is set", + bootstrap: clusterv1.Bootstrap{ConfigRef: nil, DataSecretName: pointer.String("test")}, + expectErr: false, + }, + { + name: "should not return error if config ref is set", + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}, DataSecretName: nil}, + expectErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + m := &MachinePool{ + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: tt.bootstrap, + }, + }, + }, + }, + } + + if tt.expectErr { + warnings, err := m.ValidateCreate(ctx, m) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = m.ValidateUpdate(ctx, m, m) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } else { + warnings, err := m.ValidateCreate(ctx, m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = m.ValidateUpdate(ctx, m, m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } + }) + } +} + +func TestMachinePoolNamespaceValidation(t *testing.T) { + // NOTE: MachinePool feature flag is disabled by default, thus preventing to create or update MachinePool. + // Enabling the feature flag temporarily for this test. + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)() + tests := []struct { + name string + expectErr bool + bootstrap clusterv1.Bootstrap + infraRef corev1.ObjectReference + namespace string + }{ + { + name: "should succeed if all namespaces match", + expectErr: false, + namespace: "foobar", + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar"}}, + infraRef: corev1.ObjectReference{Namespace: "foobar"}, + }, + { + name: "should return error if namespace and bootstrap namespace don't match", + expectErr: true, + namespace: "foobar", + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar123"}}, + infraRef: corev1.ObjectReference{Namespace: "foobar"}, + }, + { + name: "should return error if namespace and infrastructure ref namespace don't match", + expectErr: true, + namespace: "foobar", + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar"}}, + infraRef: corev1.ObjectReference{Namespace: "foobar123"}, + }, + { + name: "should return error if no namespaces match", + expectErr: true, + namespace: "foobar1", + bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{Namespace: "foobar2"}}, + infraRef: corev1.ObjectReference{Namespace: "foobar3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + m := &MachinePool{ + &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{Namespace: tt.namespace}, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: tt.bootstrap, + InfrastructureRef: tt.infraRef, + }, + }, + }, + }, + } + + if tt.expectErr { + warnings, err := m.ValidateCreate(ctx, m) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = m.ValidateUpdate(ctx, m, m) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } else { + warnings, err := m.ValidateCreate(ctx, m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = m.ValidateUpdate(ctx, m, m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } + }) + } +} + +func TestMachinePoolClusterNameImmutable(t *testing.T) { + // NOTE: MachinePool feature flag is disabled by default, thus preventing to create or update MachinePool. + // Enabling the feature flag temporarily for this test. + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)() + tests := []struct { + name string + oldClusterName string + newClusterName string + expectErr bool + }{ + { + name: "when the cluster name has not changed", + oldClusterName: "foo", + newClusterName: "foo", + expectErr: false, + }, + { + name: "when the cluster name has changed", + oldClusterName: "foo", + newClusterName: "bar", + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + newMP := &MachinePool{ + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + ClusterName: tt.newClusterName, + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + }, + }, + }, + }, + } + + oldMP := &MachinePool{ + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + ClusterName: tt.oldClusterName, + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + }, + }, + }, + }, + } + + warnings, err := newMP.ValidateUpdate(ctx, oldMP, newMP) + if tt.expectErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + g.Expect(warnings).To(BeEmpty()) + }) + } +} + +func TestMachinePoolVersionValidation(t *testing.T) { + // NOTE: MachinePool feature flag is disabled by default, thus preventing to create or update MachinePool. + // Enabling the feature flag temporarily for this test. + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.MachinePool, true)() + tests := []struct { + name string + expectErr bool + version string + }{ + { + name: "should succeed version is a valid kube semver", + expectErr: false, + version: "v1.23.3", + }, + { + name: "should succeed version is a valid pre-release", + expectErr: false, + version: "v1.19.0-alpha.1", + }, + { + name: "should fail if version is not a valid semver", + expectErr: true, + version: "v1.1", + }, + { + name: "should fail if version is missing a v prefix", + expectErr: true, + version: "1.20.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + m := &MachinePool{ + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Version: &tt.version, + }, + }, + }, + }, + } + + if tt.expectErr { + warnings, err := m.ValidateCreate(ctx, m) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = m.ValidateUpdate(ctx, m, m) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } else { + warnings, err := m.ValidateCreate(ctx, m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = m.ValidateUpdate(ctx, m, m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } + }) + } +} + +func TestMachinePoolMetadataValidation(t *testing.T) { + tests := []struct { + name string + labels map[string]string + annotations map[string]string + expectErr bool + }{ + { + name: "should return error for invalid labels and annotations", + labels: map[string]string{ + "foo": "$invalid-key", + "bar": strings.Repeat("a", 64) + "too-long-value", + "/invalid-key": "foo", + }, + annotations: map[string]string{ + "/invalid-key": "foo", + }, + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + mp := &MachinePool{ + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ + Labels: tt.labels, + Annotations: tt.annotations, + }, + }, + }, + }, + } + if tt.expectErr { + warnings, err := mp.ValidateCreate(ctx, mp) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = mp.ValidateUpdate(ctx, mp, mp) + g.Expect(err).To(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } else { + warnings, err := mp.ValidateCreate(ctx, mp) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + warnings, err = mp.ValidateUpdate(ctx, mp, mp) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(warnings).To(BeEmpty()) + } + }) + } +} diff --git a/internal/test/envtest/environment.go b/internal/test/envtest/environment.go index e6028ce1f0ca..22de5d4f6732 100644 --- a/internal/test/envtest/environment.go +++ b/internal/test/envtest/environment.go @@ -309,7 +309,7 @@ func newEnvironment(uncachedObjs ...client.Object) *Environment { if err := (&addonsv1.ClusterResourceSetBinding{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook for ClusterResourceSetBinding: %+v", err) } - if err := (&expv1.MachinePool{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachinePool{}).SetupWebhookWithManager(mgr); err != nil { klog.Fatalf("unable to create webhook for machinepool: %+v", err) } if err := (&runtimewebhooks.ExtensionConfig{}).SetupWebhookWithManager(mgr); err != nil { diff --git a/internal/webhooks/exp/doc.go b/internal/webhooks/exp/doc.go new file mode 100644 index 000000000000..8d303cca428b --- /dev/null +++ b/internal/webhooks/exp/doc.go @@ -0,0 +1,18 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package exp contains external webhook implementations for some of our API types. +package exp diff --git a/exp/api/v1beta1/machinepool_webhook.go b/internal/webhooks/exp/machinepool.go similarity index 62% rename from exp/api/v1beta1/machinepool_webhook.go rename to internal/webhooks/exp/machinepool.go index 8dfd0d6b07e9..eab9d73c5400 100644 --- a/exp/api/v1beta1/machinepool_webhook.go +++ b/internal/webhooks/exp/machinepool.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package exp import ( + "context" "fmt" "strings" @@ -29,24 +30,37 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/util/version" ) -func (m *MachinePool) SetupWebhookWithManager(mgr ctrl.Manager) error { +// MachinePool implements a validation and defaulting webhook for MachinePool. +type MachinePool struct { + *expv1.MachinePool +} + +func (webhook *MachinePool) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). - For(m). + For(&expv1.MachinePool{}). + WithDefaulter(webhook). + WithValidator(webhook). Complete() } // +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machinepool,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinepools,versions=v1beta1,name=validation.machinepool.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 // +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machinepool,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machinepools,versions=v1beta1,name=default.machinepool.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 -var _ webhook.Defaulter = &MachinePool{} -var _ webhook.Validator = &MachinePool{} +var _ webhook.CustomDefaulter = &MachinePool{} +var _ webhook.CustomValidator = &MachinePool{} // Default implements webhook.Defaulter so a webhook will be registered for the type. -func (m *MachinePool) Default() { +func (webhook *MachinePool) Default(_ context.Context, obj runtime.Object) error { + m, ok := obj.(*expv1.MachinePool) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", obj)) + } + if m.Labels == nil { m.Labels = make(map[string]string) } @@ -73,30 +87,35 @@ func (m *MachinePool) Default() { normalizedVersion := "v" + *m.Spec.Template.Spec.Version m.Spec.Template.Spec.Version = &normalizedVersion } + return nil } // ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachinePool) ValidateCreate() (admission.Warnings, error) { - return nil, m.validate(nil) +func (webhook *MachinePool) ValidateCreate(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, webhook.validate(nil, nil) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (m *MachinePool) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - oldMP, ok := old.(*MachinePool) +func (webhook *MachinePool) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + oldMP, ok := oldObj.(*MachinePool) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", oldObj)) + } + newMP, ok := newObj.(*MachinePool) if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", old)) + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a MachinePool but got a %T", newObj)) } - return nil, m.validate(oldMP) + return nil, webhook.validate(oldMP, newMP) } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (m *MachinePool) ValidateDelete() (admission.Warnings, error) { - return nil, m.validate(nil) +func (webhook *MachinePool) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, webhook.validate(nil, nil) } -func (m *MachinePool) validate(old *MachinePool) error { +func (webhook *MachinePool) validate(oldObj, newObj *MachinePool) error { // NOTE: MachinePool is behind MachinePool feature gate flag; the web hook - // must prevent creating new objects when the feature flag is disabled. + // must prevent creating newObj objects when the feature flag is disabled. specPath := field.NewPath("spec") if !feature.Gates.Enabled(feature.MachinePool) { return field.Forbidden( @@ -105,7 +124,7 @@ func (m *MachinePool) validate(old *MachinePool) error { ) } var allErrs field.ErrorList - if m.Spec.Template.Spec.Bootstrap.ConfigRef == nil && m.Spec.Template.Spec.Bootstrap.DataSecretName == nil { + if newObj.Spec.Template.Spec.Bootstrap.ConfigRef == nil && newObj.Spec.Template.Spec.Bootstrap.DataSecretName == nil { allErrs = append( allErrs, field.Required( @@ -115,29 +134,29 @@ func (m *MachinePool) validate(old *MachinePool) error { ) } - if m.Spec.Template.Spec.Bootstrap.ConfigRef != nil && m.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace != m.Namespace { + if newObj.Spec.Template.Spec.Bootstrap.ConfigRef != nil && newObj.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace != newObj.Namespace { allErrs = append( allErrs, field.Invalid( specPath.Child("template", "spec", "bootstrap", "configRef", "namespace"), - m.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace, + newObj.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace, "must match metadata.namespace", ), ) } - if m.Spec.Template.Spec.InfrastructureRef.Namespace != m.Namespace { + if newObj.Spec.Template.Spec.InfrastructureRef.Namespace != newObj.Namespace { allErrs = append( allErrs, field.Invalid( specPath.Child("infrastructureRef", "namespace"), - m.Spec.Template.Spec.InfrastructureRef.Namespace, + newObj.Spec.Template.Spec.InfrastructureRef.Namespace, "must match metadata.namespace", ), ) } - if old != nil && old.Spec.ClusterName != m.Spec.ClusterName { + if oldObj != nil && oldObj.Spec.ClusterName != newObj.Spec.ClusterName { allErrs = append( allErrs, field.Forbidden( @@ -146,17 +165,17 @@ func (m *MachinePool) validate(old *MachinePool) error { ) } - if m.Spec.Template.Spec.Version != nil { - if !version.KubeSemver.MatchString(*m.Spec.Template.Spec.Version) { - allErrs = append(allErrs, field.Invalid(specPath.Child("template", "spec", "version"), *m.Spec.Template.Spec.Version, "must be a valid semantic version")) + if newObj.Spec.Template.Spec.Version != nil { + if !version.KubeSemver.MatchString(*newObj.Spec.Template.Spec.Version) { + allErrs = append(allErrs, field.Invalid(specPath.Child("template", "spec", "version"), *newObj.Spec.Template.Spec.Version, "must be a valid semantic version")) } } // Validate the metadata of the MachinePool template. - allErrs = append(allErrs, m.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) + allErrs = append(allErrs, newObj.Spec.Template.ObjectMeta.Validate(specPath.Child("template", "metadata"))...) if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("MachinePool").GroupKind(), m.Name, allErrs) + return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("MachinePool").GroupKind(), newObj.Name, allErrs) } diff --git a/exp/api/v1beta1/machinepool_webhook_test.go b/internal/webhooks/exp/machinepool_test.go similarity index 74% rename from exp/api/v1beta1/machinepool_webhook_test.go rename to internal/webhooks/exp/machinepool_test.go index 3b115bd2d1c1..0bcb2ee6fb26 100644 --- a/exp/api/v1beta1/machinepool_webhook_test.go +++ b/internal/webhooks/exp/machinepool_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package exp import ( "strings" @@ -25,12 +25,16 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" + ctrl "sigs.k8s.io/controller-runtime" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/cluster-api/feature" - utildefaulting "sigs.k8s.io/cluster-api/util/defaulting" + "sigs.k8s.io/cluster-api/internal/webhooks/util" ) +var ctx = ctrl.SetupSignalHandler() + func TestMachinePoolDefault(t *testing.T) { // NOTE: MachinePool feature flag is disabled by default, thus preventing to create or update MachinePool. // Enabling the feature flag temporarily for this test. @@ -39,20 +43,23 @@ func TestMachinePoolDefault(t *testing.T) { g := NewWithT(t) m := &MachinePool{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "foobar", - }, - Spec: MachinePoolSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: clusterv1.MachineSpec{ - Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, - Version: pointer.String("1.20.0"), + &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foobar", + }, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Version: pointer.String("1.20.0"), + }, }, }, }, } - t.Run("for MachinePool", utildefaulting.DefaultValidateTest(m)) - m.Default() + webhook := &MachinePool{} + t.Run("for Machine", util.CustomDefaultValidateTest(ctx, m, webhook)) + g.Expect(m.Default(ctx, m)).To(Succeed()) g.Expect(m.Labels[clusterv1.ClusterNameLabel]).To(Equal(m.Spec.ClusterName)) g.Expect(m.Spec.Replicas).To(Equal(pointer.Int32(1))) @@ -92,27 +99,29 @@ func TestMachinePoolBootstrapValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) m := &MachinePool{ - Spec: MachinePoolSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: clusterv1.MachineSpec{ - Bootstrap: tt.bootstrap, + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: tt.bootstrap, + }, }, }, }, } if tt.expectErr { - warnings, err := m.ValidateCreate() + warnings, err := m.ValidateCreate(ctx, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = m.ValidateUpdate(ctx, m, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := m.ValidateCreate() + warnings, err := m.ValidateCreate(ctx, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = m.ValidateUpdate(ctx, m, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -166,29 +175,31 @@ func TestMachinePoolNamespaceValidation(t *testing.T) { g := NewWithT(t) m := &MachinePool{ - ObjectMeta: metav1.ObjectMeta{Namespace: tt.namespace}, - Spec: MachinePoolSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: clusterv1.MachineSpec{ - Bootstrap: tt.bootstrap, - InfrastructureRef: tt.infraRef, + &expv1.MachinePool{ + ObjectMeta: metav1.ObjectMeta{Namespace: tt.namespace}, + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: tt.bootstrap, + InfrastructureRef: tt.infraRef, + }, }, }, }, } if tt.expectErr { - warnings, err := m.ValidateCreate() + warnings, err := m.ValidateCreate(ctx, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = m.ValidateUpdate(ctx, m, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := m.ValidateCreate() + warnings, err := m.ValidateCreate(ctx, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = m.ValidateUpdate(ctx, m, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -225,28 +236,32 @@ func TestMachinePoolClusterNameImmutable(t *testing.T) { g := NewWithT(t) newMP := &MachinePool{ - Spec: MachinePoolSpec{ - ClusterName: tt.newClusterName, - Template: clusterv1.MachineTemplateSpec{ - Spec: clusterv1.MachineSpec{ - Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + ClusterName: tt.newClusterName, + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + }, }, }, }, } oldMP := &MachinePool{ - Spec: MachinePoolSpec{ - ClusterName: tt.oldClusterName, - Template: clusterv1.MachineTemplateSpec{ - Spec: clusterv1.MachineSpec{ - Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + ClusterName: tt.oldClusterName, + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + }, }, }, }, } - warnings, err := newMP.ValidateUpdate(oldMP) + warnings, err := newMP.ValidateUpdate(ctx, oldMP, newMP) if tt.expectErr { g.Expect(err).To(HaveOccurred()) } else { @@ -293,28 +308,30 @@ func TestMachinePoolVersionValidation(t *testing.T) { g := NewWithT(t) m := &MachinePool{ - Spec: MachinePoolSpec{ - Template: clusterv1.MachineTemplateSpec{ - Spec: clusterv1.MachineSpec{ - Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, - Version: &tt.version, + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Bootstrap: clusterv1.Bootstrap{ConfigRef: &corev1.ObjectReference{}}, + Version: &tt.version, + }, }, }, }, } if tt.expectErr { - warnings, err := m.ValidateCreate() + warnings, err := m.ValidateCreate(ctx, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = m.ValidateUpdate(ctx, m, m) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := m.ValidateCreate() + warnings, err := m.ValidateCreate(ctx, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = m.ValidateUpdate(m) + warnings, err = m.ValidateUpdate(ctx, m, m) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } @@ -347,27 +364,29 @@ func TestMachinePoolMetadataValidation(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) mp := &MachinePool{ - Spec: MachinePoolSpec{ - Template: clusterv1.MachineTemplateSpec{ - ObjectMeta: clusterv1.ObjectMeta{ - Labels: tt.labels, - Annotations: tt.annotations, + &expv1.MachinePool{ + Spec: expv1.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ + Labels: tt.labels, + Annotations: tt.annotations, + }, }, }, }, } if tt.expectErr { - warnings, err := mp.ValidateCreate() + warnings, err := mp.ValidateCreate(ctx, mp) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mp.ValidateUpdate(mp) + warnings, err = mp.ValidateUpdate(ctx, mp, mp) g.Expect(err).To(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } else { - warnings, err := mp.ValidateCreate() + warnings, err := mp.ValidateCreate(ctx, mp) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) - warnings, err = mp.ValidateUpdate(mp) + warnings, err = mp.ValidateUpdate(ctx, mp, mp) g.Expect(err).ToNot(HaveOccurred()) g.Expect(warnings).To(BeEmpty()) } diff --git a/main.go b/main.go index 1e3ce3d974d2..b360d5e77cd3 100644 --- a/main.go +++ b/main.go @@ -588,7 +588,7 @@ func setupWebhooks(mgr ctrl.Manager) { // NOTE: MachinePool is behind MachinePool feature gate flag; the webhook // is going to prevent creating or updating new objects in case the feature flag is disabled - if err := (&expv1.MachinePool{}).SetupWebhookWithManager(mgr); err != nil { + if err := (&webhooks.MachinePool{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "MachinePool") os.Exit(1) } diff --git a/webhooks/alias.go b/webhooks/alias.go index ee0646bdc19e..5b394281629e 100644 --- a/webhooks/alias.go +++ b/webhooks/alias.go @@ -22,6 +22,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "sigs.k8s.io/cluster-api/internal/webhooks" + "sigs.k8s.io/cluster-api/internal/webhooks/exp" ) // Cluster implements a validating and defaulting webhook for Cluster. @@ -83,3 +84,11 @@ type MachineHealthCheck struct{} func (webhook *MachineHealthCheck) SetupWebhookWithManager(mgr ctrl.Manager) error { return (&webhooks.MachineHealthCheck{}).SetupWebhookWithManager(mgr) } + +// MachinePool implements a validating and defaulting webhook for MachinePool. +type MachinePool struct{} + +// SetupWebhookWithManager sets up MachinePool webhooks. +func (webhook *MachinePool) SetupWebhookWithManager(mgr ctrl.Manager) error { + return (&exp.MachinePool{}).SetupWebhookWithManager(mgr) +}