From 9b22b9e329871900df343e468ecc22fc2ad3b923 Mon Sep 17 00:00:00 2001 From: Damyan Yordanov Date: Tue, 26 Nov 2024 16:32:22 +0100 Subject: [PATCH 1/3] Add `Endpoint` webhook validating unique MAC addresses --- PROJECT | 3 + cmd/manager/main.go | 9 ++ config/webhook/kustomization.yaml | 6 + config/webhook/kustomizeconfig.yaml | 22 +++ config/webhook/manifests.yaml | 26 +++ config/webhook/service.yaml | 15 ++ internal/webhook/v1alpha1/endpoint_webhook.go | 137 ++++++++++++++++ .../webhook/v1alpha1/endpoint_webhook_test.go | 141 +++++++++++++++++ .../webhook/v1alpha1/webhook_suite_test.go | 148 ++++++++++++++++++ 9 files changed, 507 insertions(+) create mode 100644 config/webhook/kustomization.yaml create mode 100644 config/webhook/kustomizeconfig.yaml create mode 100644 config/webhook/manifests.yaml create mode 100644 config/webhook/service.yaml create mode 100644 internal/webhook/v1alpha1/endpoint_webhook.go create mode 100644 internal/webhook/v1alpha1/endpoint_webhook_test.go create mode 100644 internal/webhook/v1alpha1/webhook_suite_test.go diff --git a/PROJECT b/PROJECT index 3f30d5a..d2be287 100644 --- a/PROJECT +++ b/PROJECT @@ -16,6 +16,9 @@ resources: kind: Endpoint path: github.com/ironcore-dev/metal-operator/api/v1alpha1 version: v1alpha1 + webhooks: + validation: true + webhookVersion: v1 - api: crdVersion: v1 controller: true diff --git a/cmd/manager/main.go b/cmd/manager/main.go index dcff0a6..a11d9a5 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -10,6 +10,8 @@ import ( "os" "time" + webhookmetalv1alpha1 "github.com/ironcore-dev/metal-operator/internal/webhook/v1alpha1" + // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" @@ -252,6 +254,13 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "ServerClaim") os.Exit(1) } + // nolint:goconst + if os.Getenv("ENABLE_WEBHOOKS") != "false" { + if err = webhookmetalv1alpha1.SetupEndpointWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "Endpoint") + os.Exit(1) + } + } //+kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml new file mode 100644 index 0000000..9cf2613 --- /dev/null +++ b/config/webhook/kustomization.yaml @@ -0,0 +1,6 @@ +resources: +- manifests.yaml +- service.yaml + +configurations: +- kustomizeconfig.yaml diff --git a/config/webhook/kustomizeconfig.yaml b/config/webhook/kustomizeconfig.yaml new file mode 100644 index 0000000..206316e --- /dev/null +++ b/config/webhook/kustomizeconfig.yaml @@ -0,0 +1,22 @@ +# the following config is for teaching kustomize where to look at when substituting nameReference. +# It requires kustomize v2.1.0 or newer to work properly. +nameReference: +- kind: Service + version: v1 + fieldSpecs: + - kind: MutatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/name + - kind: ValidatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/name + +namespace: +- kind: MutatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/namespace + create: true +- kind: ValidatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/namespace + create: true diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml new file mode 100644 index 0000000..c2b1bb8 --- /dev/null +++ b/config/webhook/manifests.yaml @@ -0,0 +1,26 @@ +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingWebhookConfiguration +metadata: + name: validating-webhook-configuration +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-metal-ironcore-dev-v1alpha1-endpoint + failurePolicy: Fail + name: vendpoint-v1alpha1.kb.io + rules: + - apiGroups: + - metal.ironcore.dev + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - endpoints + sideEffects: None diff --git a/config/webhook/service.yaml b/config/webhook/service.yaml new file mode 100644 index 0000000..156b834 --- /dev/null +++ b/config/webhook/service.yaml @@ -0,0 +1,15 @@ +apiVersion: v1 +kind: Service +metadata: + labels: + app.kubernetes.io/name: metal-operator + app.kubernetes.io/managed-by: kustomize + name: webhook-service + namespace: system +spec: + ports: + - port: 443 + protocol: TCP + targetPort: 9443 + selector: + control-plane: controller-manager diff --git a/internal/webhook/v1alpha1/endpoint_webhook.go b/internal/webhook/v1alpha1/endpoint_webhook.go new file mode 100644 index 0000000..7214842 --- /dev/null +++ b/internal/webhook/v1alpha1/endpoint_webhook.go @@ -0,0 +1,137 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha1 + +import ( + "context" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" + + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" +) + +// nolint:unused +// log is for logging in this package. +var endpointlog = logf.Log.WithName("endpoint-resource") + +// SetupEndpointWebhookWithManager registers the webhook for Endpoint in the manager. +func SetupEndpointWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr).For(&metalv1alpha1.Endpoint{}). + WithValidator(&EndpointCustomValidator{Client: mgr.GetClient()}). + Complete() +} + +// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here. +// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook. +// +kubebuilder:webhook:path=/validate-metal-ironcore-dev-v1alpha1-endpoint,mutating=false,failurePolicy=fail,sideEffects=None,groups=metal.ironcore.dev,resources=endpoints,verbs=create;update,versions=v1alpha1,name=vendpoint-v1alpha1.kb.io,admissionReviewVersions=v1 + +// EndpointCustomValidator struct is responsible for validating the Endpoint resource +// when it is created, updated, or deleted. +// +// NOTE: The +kubebuilder:object:generate=false marker prevents controller-gen from generating DeepCopy methods, +// as this struct is used only for temporary operations and does not need to be deeply copied. +type EndpointCustomValidator struct { + Client client.Client +} + +var _ webhook.CustomValidator = &EndpointCustomValidator{} + +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type Endpoint. +func (v *EndpointCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + allErrs := field.ErrorList{} + + endpoint, ok := obj.(*metalv1alpha1.Endpoint) + if !ok { + return nil, fmt.Errorf("expected a Endpoint object but got %T", obj) + } + endpointlog.Info("Validation for Endpoint upon creation", "name", endpoint.GetName()) + + allErrs = append(allErrs, ValidateMACAddressCreate(ctx, v.Client, endpoint.Spec, field.NewPath("spec"))...) + + if len(allErrs) != 0 { + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: "metal.ironcore.dev", Kind: "Endpoint"}, + endpoint.GetName(), allErrs) + } + + return nil, nil +} + +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type Endpoint. +func (v *EndpointCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + allErrs := field.ErrorList{} + + endpoint, ok := newObj.(*metalv1alpha1.Endpoint) + if !ok { + return nil, fmt.Errorf("expected a Endpoint object for the newObj but got %T", newObj) + } + endpointlog.Info("Validation for Endpoint upon update", "name", endpoint.GetName()) + + allErrs = append(allErrs, ValidateMACAddressUpdate(ctx, v.Client, endpoint, field.NewPath("spec"))...) + + if len(allErrs) != 0 { + return nil, apierrors.NewInvalid( + schema.GroupKind{Group: "metal.ironcore.dev", Kind: "Endpoint"}, + endpoint.GetName(), allErrs) + } + + return nil, nil +} + +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type Endpoint. +func (v *EndpointCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + endpoint, ok := obj.(*metalv1alpha1.Endpoint) + if !ok { + return nil, fmt.Errorf("expected a Endpoint object but got %T", obj) + } + endpointlog.Info("Validation for Endpoint upon deletion", "name", endpoint.GetName()) + + // TODO(user): fill in your validation logic upon object deletion. + + return nil, nil +} + +func ValidateMACAddressCreate(ctx context.Context, c client.Client, spec metalv1alpha1.EndpointSpec, path *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + endpoints := &metalv1alpha1.EndpointList{} + if err := c.List(ctx, endpoints); err != nil { + allErrs = append(allErrs, field.InternalError(path, fmt.Errorf("failed to list Endpoints: %w", err))) + } + + for _, e := range endpoints.Items { + if e.Spec.MACAddress == spec.MACAddress { + allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("MACAddress"), e.Spec.MACAddress)) + } + } + + return allErrs +} + +func ValidateMACAddressUpdate(ctx context.Context, c client.Client, existing *metalv1alpha1.Endpoint, path *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + endpoints := &metalv1alpha1.EndpointList{} + if err := c.List(ctx, endpoints); err != nil { + allErrs = append(allErrs, field.InternalError(path, fmt.Errorf("failed to list Endpoints: %w", err))) + } + + for _, e := range endpoints.Items { + if e.Spec.MACAddress == existing.Spec.MACAddress && e.Name != existing.Name { + allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("MACAddress"), e.Spec.MACAddress)) + } + } + + return allErrs +} diff --git a/internal/webhook/v1alpha1/endpoint_webhook_test.go b/internal/webhook/v1alpha1/endpoint_webhook_test.go new file mode 100644 index 0000000..82b09ab --- /dev/null +++ b/internal/webhook/v1alpha1/endpoint_webhook_test.go @@ -0,0 +1,141 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha1 + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" + // TODO (user): Add any additional imports if needed +) + +var _ = Describe("Endpoint Webhook", func() { + var ( + obj *metalv1alpha1.Endpoint + oldObj *metalv1alpha1.Endpoint + validator EndpointCustomValidator + ) + + BeforeEach(func() { + obj = &metalv1alpha1.Endpoint{} + oldObj = &metalv1alpha1.Endpoint{} + validator = EndpointCustomValidator{ + Client: k8sClient, + } + Expect(validator).NotTo(BeNil(), "Expected validator to be initialized") + Expect(oldObj).NotTo(BeNil(), "Expected oldObj to be initialized") + Expect(obj).NotTo(BeNil(), "Expected obj to be initialized") + }) + + Context("When creating or updating Endpoint under Validating Webhook", func() { + It("Should deny creation if an Endpoint has a duplicate MAC address", func(ctx SpecContext) { + By("creating an Endpoint") + endpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("1.1.1.1"), + MACAddress: "foo", + }, + } + Expect(k8sClient.Create(ctx, endpoint)).To(Succeed()) + DeferCleanup(k8sClient.Delete, endpoint) + + By("creating an Endpoint with existing MAC address") + existingEndpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("2.2.2.2"), + MACAddress: "foo", + }, + } + Expect(validator.ValidateCreate(ctx, existingEndpoint)).Error().To(HaveOccurred()) + }) + + It("Should allow creation if an Endpoint has an unique MAC address", func(ctx SpecContext) { + By("creating an Endpoint") + endpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("1.1.1.1"), + MACAddress: "foo", + }, + } + Expect(k8sClient.Create(ctx, endpoint)).ToNot(HaveOccurred()) + DeferCleanup(k8sClient.Delete, endpoint) + + By("creating an Endpoint with non-existing MAC address") + existingEndpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("2.2.2.2"), + MACAddress: "bar", + }, + } + Expect(validator.ValidateCreate(ctx, existingEndpoint)).Error().ToNot(HaveOccurred()) + }) + + It("Should deny update of an Endpoint with existing MAC address", func() { + By("creating an Endpoint") + endpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("1.1.1.1"), + MACAddress: "foo", + }, + } + Expect(k8sClient.Create(ctx, endpoint)).To(Succeed()) + DeferCleanup(k8sClient.Delete, endpoint) + + By("creating an Endpoint with different MAC address") + existingEndpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("2.2.2.2"), + MACAddress: "bar", + }, + } + Expect(k8sClient.Create(ctx, existingEndpoint)).To(Succeed()) + DeferCleanup(k8sClient.Delete, existingEndpoint) + + By("Updating an Endpoint to conflicting MAC address") + updatedEndpoint := endpoint.DeepCopy() + updatedEndpoint.Spec.MACAddress = "bar" + Expect(validator.ValidateUpdate(ctx, endpoint, updatedEndpoint)).Error().To(HaveOccurred()) + }) + + It("Should allow update an IP address of the same Endpoint", func() { + By("creating an Endpoint") + existingEndpoint := &metalv1alpha1.Endpoint{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.EndpointSpec{ + IP: metalv1alpha1.MustParseIP("1.1.1.1"), + MACAddress: "foo", + }, + } + Expect(k8sClient.Create(ctx, existingEndpoint)).To(Succeed()) + DeferCleanup(k8sClient.Delete, existingEndpoint) + + By("Updating an Endpoint IP address") + updatedEndpoint := existingEndpoint.DeepCopy() + updatedEndpoint.Spec.IP = metalv1alpha1.MustParseIP("2.2.2.2") + Expect(validator.ValidateUpdate(ctx, existingEndpoint, updatedEndpoint)).Error().ToNot(HaveOccurred()) + }) + }) +}) diff --git a/internal/webhook/v1alpha1/webhook_suite_test.go b/internal/webhook/v1alpha1/webhook_suite_test.go new file mode 100644 index 0000000..374feee --- /dev/null +++ b/internal/webhook/v1alpha1/webhook_suite_test.go @@ -0,0 +1,148 @@ +// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha1 + +import ( + "context" + "crypto/tls" + "fmt" + "net" + "path/filepath" + "runtime" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + admissionv1 "k8s.io/api/admission/v1" + + metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" + + // +kubebuilder:scaffold:imports + apimachineryruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + "sigs.k8s.io/controller-runtime/pkg/webhook" +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. + +const ( + pollingInterval = 100 * time.Millisecond + eventuallyTimeout = 3 * time.Second + consistentlyDuration = 3 * time.Second +) + +var ( + cancel context.CancelFunc + cfg *rest.Config + ctx context.Context + k8sClient client.Client + testEnv *envtest.Environment +) + +func TestAPIs(t *testing.T) { + SetDefaultConsistentlyPollingInterval(pollingInterval) + SetDefaultEventuallyPollingInterval(pollingInterval) + SetDefaultEventuallyTimeout(eventuallyTimeout) + SetDefaultConsistentlyDuration(consistentlyDuration) + + RegisterFailHandler(Fail) + + RunSpecs(t, "Webhook Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + ctx, cancel = context.WithCancel(context.TODO()) + + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: false, + + // The BinaryAssetsDirectory is only required if you want to run the tests directly + // without call the makefile target test. If not informed it will look for the + // default path defined in controller-runtime which is /usr/local/kubebuilder/. + // Note that you must have the required binaries setup under the bin directory to perform + // the tests directly. When we run make test it will be setup and used automatically. + BinaryAssetsDirectory: filepath.Join("..", "..", "..", "bin", "k8s", + fmt.Sprintf("1.31.0-%s-%s", runtime.GOOS, runtime.GOARCH)), + + WebhookInstallOptions: envtest.WebhookInstallOptions{ + Paths: []string{filepath.Join("..", "..", "..", "config", "webhook")}, + }, + } + + var err error + // cfg is defined in this file globally. + cfg, err = testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + scheme := apimachineryruntime.NewScheme() + err = metalv1alpha1.AddToScheme(scheme) + Expect(err).NotTo(HaveOccurred()) + + err = admissionv1.AddToScheme(scheme) + Expect(err).NotTo(HaveOccurred()) + + // +kubebuilder:scaffold:scheme + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) + + // start webhook server using Manager. + webhookInstallOptions := &testEnv.WebhookInstallOptions + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme, + WebhookServer: webhook.NewServer(webhook.Options{ + Host: webhookInstallOptions.LocalServingHost, + Port: webhookInstallOptions.LocalServingPort, + CertDir: webhookInstallOptions.LocalServingCertDir, + }), + LeaderElection: false, + Metrics: metricsserver.Options{BindAddress: "0"}, + }) + Expect(err).NotTo(HaveOccurred()) + + err = SetupEndpointWebhookWithManager(mgr) + Expect(err).NotTo(HaveOccurred()) + + // +kubebuilder:scaffold:webhook + + go func() { + defer GinkgoRecover() + err = mgr.Start(ctx) + Expect(err).NotTo(HaveOccurred()) + }() + + // wait for the webhook server to get ready. + dialer := &net.Dialer{Timeout: time.Second} + addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort) + Eventually(func() error { + conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + if err != nil { + return err + } + + return conn.Close() + }).Should(Succeed()) +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + cancel() + err := testEnv.Stop() + Expect(err).NotTo(HaveOccurred()) +}) From 25222522aca65b37185f244d54bb3fcde3234a2d Mon Sep 17 00:00:00 2001 From: Damyan Yordanov Date: Wed, 27 Nov 2024 11:19:23 +0100 Subject: [PATCH 2/3] Rename variable for readabilty --- internal/webhook/v1alpha1/endpoint_webhook.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/webhook/v1alpha1/endpoint_webhook.go b/internal/webhook/v1alpha1/endpoint_webhook.go index 7214842..85902db 100644 --- a/internal/webhook/v1alpha1/endpoint_webhook.go +++ b/internal/webhook/v1alpha1/endpoint_webhook.go @@ -119,7 +119,7 @@ func ValidateMACAddressCreate(ctx context.Context, c client.Client, spec metalv1 return allErrs } -func ValidateMACAddressUpdate(ctx context.Context, c client.Client, existing *metalv1alpha1.Endpoint, path *field.Path) field.ErrorList { +func ValidateMACAddressUpdate(ctx context.Context, c client.Client, updatedEndpoint *metalv1alpha1.Endpoint, path *field.Path) field.ErrorList { allErrs := field.ErrorList{} endpoints := &metalv1alpha1.EndpointList{} @@ -128,7 +128,7 @@ func ValidateMACAddressUpdate(ctx context.Context, c client.Client, existing *me } for _, e := range endpoints.Items { - if e.Spec.MACAddress == existing.Spec.MACAddress && e.Name != existing.Name { + if e.Spec.MACAddress == updatedEndpoint.Spec.MACAddress && e.Name != updatedEndpoint.Name { allErrs = append(allErrs, field.Duplicate(field.NewPath("spec").Child("MACAddress"), e.Spec.MACAddress)) } } From 5f3dfbdcb11f618c1d672fe07215dbea9d283e82 Mon Sep 17 00:00:00 2001 From: Damyan Yordanov Date: Wed, 27 Nov 2024 13:23:54 +0100 Subject: [PATCH 3/3] Implement review findings - improve wording - fix typos --- internal/webhook/v1alpha1/endpoint_webhook.go | 6 +++--- .../webhook/v1alpha1/endpoint_webhook_test.go | 19 +++++++++---------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/internal/webhook/v1alpha1/endpoint_webhook.go b/internal/webhook/v1alpha1/endpoint_webhook.go index 85902db..ad58b89 100644 --- a/internal/webhook/v1alpha1/endpoint_webhook.go +++ b/internal/webhook/v1alpha1/endpoint_webhook.go @@ -53,7 +53,7 @@ func (v *EndpointCustomValidator) ValidateCreate(ctx context.Context, obj runtim endpoint, ok := obj.(*metalv1alpha1.Endpoint) if !ok { - return nil, fmt.Errorf("expected a Endpoint object but got %T", obj) + return nil, fmt.Errorf("expected an Endpoint object but got %T", obj) } endpointlog.Info("Validation for Endpoint upon creation", "name", endpoint.GetName()) @@ -74,7 +74,7 @@ func (v *EndpointCustomValidator) ValidateUpdate(ctx context.Context, oldObj, ne endpoint, ok := newObj.(*metalv1alpha1.Endpoint) if !ok { - return nil, fmt.Errorf("expected a Endpoint object for the newObj but got %T", newObj) + return nil, fmt.Errorf("expected an Endpoint object for the newObj but got %T", newObj) } endpointlog.Info("Validation for Endpoint upon update", "name", endpoint.GetName()) @@ -93,7 +93,7 @@ func (v *EndpointCustomValidator) ValidateUpdate(ctx context.Context, oldObj, ne func (v *EndpointCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { endpoint, ok := obj.(*metalv1alpha1.Endpoint) if !ok { - return nil, fmt.Errorf("expected a Endpoint object but got %T", obj) + return nil, fmt.Errorf("expected an Endpoint object but got %T", obj) } endpointlog.Info("Validation for Endpoint upon deletion", "name", endpoint.GetName()) diff --git a/internal/webhook/v1alpha1/endpoint_webhook_test.go b/internal/webhook/v1alpha1/endpoint_webhook_test.go index 82b09ab..71cddd7 100644 --- a/internal/webhook/v1alpha1/endpoint_webhook_test.go +++ b/internal/webhook/v1alpha1/endpoint_webhook_test.go @@ -9,7 +9,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" - // TODO (user): Add any additional imports if needed ) var _ = Describe("Endpoint Webhook", func() { @@ -30,9 +29,9 @@ var _ = Describe("Endpoint Webhook", func() { Expect(obj).NotTo(BeNil(), "Expected obj to be initialized") }) - Context("When creating or updating Endpoint under Validating Webhook", func() { + Context("When creating or updating an Endpoint under Validating Webhook", func() { It("Should deny creation if an Endpoint has a duplicate MAC address", func(ctx SpecContext) { - By("creating an Endpoint") + By("Creating an Endpoint") endpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -45,7 +44,7 @@ var _ = Describe("Endpoint Webhook", func() { Expect(k8sClient.Create(ctx, endpoint)).To(Succeed()) DeferCleanup(k8sClient.Delete, endpoint) - By("creating an Endpoint with existing MAC address") + By("Creating an Endpoint with existing MAC address") existingEndpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -58,8 +57,8 @@ var _ = Describe("Endpoint Webhook", func() { Expect(validator.ValidateCreate(ctx, existingEndpoint)).Error().To(HaveOccurred()) }) - It("Should allow creation if an Endpoint has an unique MAC address", func(ctx SpecContext) { - By("creating an Endpoint") + It("Should allow creation if an Endpoint has a unique MAC address", func(ctx SpecContext) { + By("Creating an Endpoint") endpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -72,7 +71,7 @@ var _ = Describe("Endpoint Webhook", func() { Expect(k8sClient.Create(ctx, endpoint)).ToNot(HaveOccurred()) DeferCleanup(k8sClient.Delete, endpoint) - By("creating an Endpoint with non-existing MAC address") + By("Creating an Endpoint with non-existing MAC address") existingEndpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -86,7 +85,7 @@ var _ = Describe("Endpoint Webhook", func() { }) It("Should deny update of an Endpoint with existing MAC address", func() { - By("creating an Endpoint") + By("Creating an Endpoint") endpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -99,7 +98,7 @@ var _ = Describe("Endpoint Webhook", func() { Expect(k8sClient.Create(ctx, endpoint)).To(Succeed()) DeferCleanup(k8sClient.Delete, endpoint) - By("creating an Endpoint with different MAC address") + By("Creating an Endpoint with different MAC address") existingEndpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-", @@ -119,7 +118,7 @@ var _ = Describe("Endpoint Webhook", func() { }) It("Should allow update an IP address of the same Endpoint", func() { - By("creating an Endpoint") + By("Creating an Endpoint") existingEndpoint := &metalv1alpha1.Endpoint{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "test-",