From bc7155896c622635869e9107064cc1cc5360d7fc Mon Sep 17 00:00:00 2001 From: Nuckal777 Date: Fri, 8 Nov 2024 09:57:16 +0100 Subject: [PATCH] Migrate to CRD validation rules --- PROJECT | 3 - api/v1alpha1/bmc_types.go | 5 + api/v1alpha1/bmc_webhook.go | 84 --------- api/v1alpha1/bmc_webhook_test.go | 162 ----------------- api/v1alpha1/serverclaim_types.go | 6 + api/v1alpha1/serverclaim_webhook.go | 76 -------- api/v1alpha1/serverclaim_webhook_test.go | 111 ------------ api/v1alpha1/webhook_suite_test.go | 163 ------------------ api/v1alpha1/zz_generated.deepcopy.go | 4 +- cmd/manager/main.go | 12 -- config/crd/bases/metal.ironcore.dev_bmcs.yaml | 9 + .../metal.ironcore.dev_serverclaims.yaml | 11 ++ config/webhook/kustomization.yaml | 6 - config/webhook/kustomizeconfig.yaml | 22 --- config/webhook/manifests.yaml | 46 ----- config/webhook/service.yaml | 19 -- internal/controller/bmc_controller_test.go | 150 ++++++++++++++++ .../controller/serverclaim_controller_test.go | 99 +++++++++++ 18 files changed, 282 insertions(+), 706 deletions(-) delete mode 100644 api/v1alpha1/bmc_webhook.go delete mode 100644 api/v1alpha1/bmc_webhook_test.go delete mode 100644 api/v1alpha1/serverclaim_webhook.go delete mode 100644 api/v1alpha1/serverclaim_webhook_test.go delete mode 100644 api/v1alpha1/webhook_suite_test.go delete mode 100644 config/webhook/kustomization.yaml delete mode 100644 config/webhook/kustomizeconfig.yaml delete mode 100644 config/webhook/manifests.yaml delete mode 100644 config/webhook/service.yaml diff --git a/PROJECT b/PROJECT index e0e9448..3f30d5a 100644 --- a/PROJECT +++ b/PROJECT @@ -57,7 +57,4 @@ resources: kind: ServerClaim path: github.com/ironcore-dev/metal-operator/api/v1alpha1 version: v1alpha1 - webhooks: - validation: true - webhookVersion: v1 version: "3" diff --git a/api/v1alpha1/bmc_types.go b/api/v1alpha1/bmc_types.go index 3ea664a..ff31ca1 100644 --- a/api/v1alpha1/bmc_types.go +++ b/api/v1alpha1/bmc_types.go @@ -16,15 +16,20 @@ const ( ) // BMCSpec defines the desired state of BMC +// +kubebuilder:validation:XValidation:rule="has(self.access) != has(self.endpointRef)",message="exactly one of access or endpointRef needs to be set" type BMCSpec struct { // EndpointRef is a reference to the Kubernetes object that contains the endpoint information for the BMC. // This reference is typically used to locate the BMC endpoint within the cluster. // +optional + // +kubebuilder:validation:Optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="endpointRef is immutable" EndpointRef *v1.LocalObjectReference `json:"endpointRef"` // Endpoint allows inline configuration of network access details for the BMC. // Use this field if access settings like address are to be configured directly within the BMC resource. // +optional + // +kubebuilder:validation:Optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="access is immutable" Endpoint *InlineEndpoint `json:"access,omitempty"` // BMCSecretRef is a reference to the Kubernetes Secret object that contains the credentials diff --git a/api/v1alpha1/bmc_webhook.go b/api/v1alpha1/bmc_webhook.go deleted file mode 100644 index 415058f..0000000 --- a/api/v1alpha1/bmc_webhook.go +++ /dev/null @@ -1,84 +0,0 @@ -// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors -// SPDX-License-Identifier: Apache-2.0 - -package v1alpha1 - -import ( - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/validation/field" - 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" -) - -// log is for logging in this package. -var bmclog = logf.Log.WithName("bmc-resource") - -// SetupWebhookWithManager will setup the manager to manage the webhooks -func (r *BMC) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() -} - -//+kubebuilder:webhook:path=/validate-metal-ironcore-dev-v1alpha1-bmc,mutating=false,failurePolicy=fail,sideEffects=None,groups=metal.ironcore.dev,resources=bmcs,verbs=create;update,versions=v1alpha1,name=vbmc.kb.io,admissionReviewVersions=v1 - -var _ webhook.Validator = &BMC{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type -func (r *BMC) ValidateCreate() (admission.Warnings, error) { - bmclog.Info("validate create", "name", r.Name) - - allErrs := field.ErrorList{} - allErrs = append(allErrs, ValidateCreateBMCSpec(r.Spec, field.NewPath("spec"))...) - - if len(allErrs) != 0 { - return nil, apierrors.NewInvalid( - schema.GroupKind{Group: "metal.ironcore.dev", Kind: "BMC"}, - r.Name, allErrs) - } - - return nil, nil -} - -func ValidateCreateBMCSpec(spec BMCSpec, fldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} - - if spec.EndpointRef != nil && spec.Endpoint != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("endpointRef"), spec.EndpointRef, "only one of 'endpointRef' or 'endpoint' should be specified")) - allErrs = append(allErrs, field.Invalid(fldPath.Child("endpoint"), spec.Endpoint, "only one of 'endpointRef' or 'endpoint' should be specified")) - } - if spec.EndpointRef == nil && spec.Endpoint == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("endpointRef"), spec.EndpointRef, "either 'endpointRef' or 'endpoint' must be specified")) - allErrs = append(allErrs, field.Invalid(fldPath.Child("endpoint"), spec.Endpoint, "either 'endpointRef' or 'endpoint' must be specified")) - } - - return allErrs -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (r *BMC) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - bmclog.Info("validate update", "name", r.Name) - allErrs := field.ErrorList{} - - allErrs = append(allErrs, ValidateCreateBMCSpec(r.Spec, field.NewPath("spec"))...) - - if len(allErrs) != 0 { - return nil, apierrors.NewInvalid( - schema.GroupKind{Group: "metal.ironcore.dev", Kind: "BMC"}, - r.Name, allErrs) - } - - return nil, nil -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type -func (r *BMC) ValidateDelete() (admission.Warnings, error) { - bmclog.Info("validate delete", "name", r.Name) - - // TODO(user): fill in your validation logic upon object deletion. - return nil, nil -} diff --git a/api/v1alpha1/bmc_webhook_test.go b/api/v1alpha1/bmc_webhook_test.go deleted file mode 100644 index 8a47742..0000000 --- a/api/v1alpha1/bmc_webhook_test.go +++ /dev/null @@ -1,162 +0,0 @@ -// 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" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" -) - -var _ = Describe("BMC Webhook", func() { - _ = SetupTest() - - It("Should deny if the BMC has EndpointRef and InlineEndpoint spec fields", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-invalid", - }, - Spec: BMCSpec{ - EndpointRef: &v1.LocalObjectReference{Name: "foo"}, - Endpoint: &InlineEndpoint{ - IP: MustParseIP("127.0.0.1"), - MACAddress: "aa:bb:cc:dd:ee:ff", - }, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(HaveOccurred()) - Eventually(Get(bmc)).Should(Satisfy(errors.IsNotFound)) - }) - - It("Should deny if the BMC has no EndpointRef and InlineEndpoint spec fields", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-empty", - }, - Spec: BMCSpec{}, - } - Expect(k8sClient.Create(ctx, bmc)).To(HaveOccurred()) - Eventually(Get(bmc)).Should(Satisfy(errors.IsNotFound)) - }) - - It("Should admit if the BMC has an EndpointRef but no InlineEndpoint spec field", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: BMCSpec{ - EndpointRef: &v1.LocalObjectReference{Name: "foo"}, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) - DeferCleanup(k8sClient.Delete, bmc) - }) - - It("Should deny if the BMC EndpointRef spec field has been removed", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: BMCSpec{ - EndpointRef: &v1.LocalObjectReference{Name: "foo"}, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) - DeferCleanup(k8sClient.Delete, bmc) - - Eventually(Update(bmc, func() { - bmc.Spec.EndpointRef = nil - })).Should(Not(Succeed())) - - Eventually(Object(bmc)).Should(SatisfyAll(HaveField( - "Spec.EndpointRef", &v1.LocalObjectReference{Name: "foo"}))) - }) - - It("Should admit if the BMC is changing EndpointRef to InlineEndpoint spec field", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: BMCSpec{ - EndpointRef: &v1.LocalObjectReference{Name: "foo"}, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) - DeferCleanup(k8sClient.Delete, bmc) - - Eventually(Update(bmc, func() { - bmc.Spec.EndpointRef = nil - bmc.Spec.Endpoint = &InlineEndpoint{ - IP: MustParseIP("127.0.0.1"), - MACAddress: "aa:bb:cc:dd:ee:ff", - } - })).Should(Succeed()) - }) - - It("Should admit if the BMC has no EndpointRef but an InlineEndpoint spec field", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: BMCSpec{ - Endpoint: &InlineEndpoint{ - IP: MustParseIP("127.0.0.1"), - MACAddress: "aa:bb:cc:dd:ee:ff", - }, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) - DeferCleanup(k8sClient.Delete, bmc) - }) - - It("Should deny if the BMC InlineEndpoint spec field has been removed", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: BMCSpec{ - Endpoint: &InlineEndpoint{ - IP: MustParseIP("127.0.0.1"), - MACAddress: "aa:bb:cc:dd:ee:ff", - }, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) - DeferCleanup(k8sClient.Delete, bmc) - - Eventually(Update(bmc, func() { - bmc.Spec.Endpoint = nil - })).Should(Not(Succeed())) - - Eventually(Object(bmc)).Should(SatisfyAll( - HaveField("Spec.Endpoint.IP", MustParseIP("127.0.0.1")), - HaveField("Spec.Endpoint.MACAddress", "aa:bb:cc:dd:ee:ff"), - )) - }) - - It("Should admit if the BMC has is changing to an EndpointRef from an InlineEndpoint spec field", func() { - bmc := &BMC{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - Spec: BMCSpec{ - Endpoint: &InlineEndpoint{ - IP: MustParseIP("127.0.0.1"), - MACAddress: "aa:bb:cc:dd:ee:ff", - }, - }, - } - Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) - DeferCleanup(k8sClient.Delete, bmc) - - Eventually(Update(bmc, func() { - bmc.Spec.EndpointRef = &v1.LocalObjectReference{Name: "foo"} - bmc.Spec.Endpoint = nil - })).Should(Succeed()) - }) - -}) diff --git a/api/v1alpha1/serverclaim_types.go b/api/v1alpha1/serverclaim_types.go index 49a1696..4006845 100644 --- a/api/v1alpha1/serverclaim_types.go +++ b/api/v1alpha1/serverclaim_types.go @@ -9,16 +9,22 @@ import ( ) // ServerClaimSpec defines the desired state of ServerClaim. +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.serverRef) || has(self.serverRef)", message="serverRef is required once set" +// +kubebuilder:validation:XValidation:rule="!has(oldSelf.serverSelector) || has(self.serverSelector)", message="serverSelector is required once set" type ServerClaimSpec struct { // Power specifies the desired power state of the server. Power Power `json:"power"` // ServerRef is a reference to a specific server to be claimed. // This field is optional and can be omitted if the server is to be selected using ServerSelector. + // +kubebuilder:validation:Optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="serverRef is immutable" ServerRef *v1.LocalObjectReference `json:"serverRef,omitempty"` // ServerSelector specifies a label selector to identify the server to be claimed. // This field is optional and can be omitted if a specific server is referenced using ServerRef. + // +kubebuilder:validation:Optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="serverSelector is immutable" ServerSelector *metav1.LabelSelector `json:"serverSelector,omitempty"` // IgnitionSecretRef is a reference to the Kubernetes Secret object that contains diff --git a/api/v1alpha1/serverclaim_webhook.go b/api/v1alpha1/serverclaim_webhook.go deleted file mode 100644 index 87a802c..0000000 --- a/api/v1alpha1/serverclaim_webhook.go +++ /dev/null @@ -1,76 +0,0 @@ -// SPDX-FileCopyrightText: 2024 SAP SE or an SAP affiliate company and IronCore contributors -// SPDX-License-Identifier: Apache-2.0 - -package v1alpha1 - -import ( - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/validation" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/validation/field" - 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" -) - -// log is for logging in this package. -var serverclaimlog = logf.Log.WithName("serverclaim-resource") - -// SetupWebhookWithManager will setup the manager to manage the webhooks -func (r *ServerClaim) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() -} - -//+kubebuilder:webhook:path=/validate-metal-ironcore-dev-v1alpha1-serverclaim,mutating=false,failurePolicy=fail,sideEffects=None,groups=metal.ironcore.dev,resources=serverclaims,verbs=create;update,versions=v1alpha1,name=vserverclaim.kb.io,admissionReviewVersions=v1 - -var _ webhook.Validator = &ServerClaim{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type -func (r *ServerClaim) ValidateCreate() (admission.Warnings, error) { - serverclaimlog.Info("validate create", "name", r.Name) - - // TODO(user): fill in your validation logic upon object creation. - return nil, nil -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (r *ServerClaim) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - serverclaimlog.Info("validate update", "name", r.Name) - allErrs := field.ErrorList{} - oldClaim := old.(*ServerClaim) - - allErrs = append(allErrs, ValidateUpdateSpecUpdate(r.Spec, oldClaim.Spec, field.NewPath("spec"))...) - - if len(allErrs) != 0 { - return nil, apierrors.NewInvalid( - schema.GroupKind{Group: "metal.ironcore.dev", Kind: "ServerClaim"}, - r.Name, allErrs) - } - - return nil, nil -} - -func ValidateUpdateSpecUpdate(newSpec, oldSpec ServerClaimSpec, fldPath *field.Path) field.ErrorList { - var allErrs field.ErrorList - - if oldSpec.ServerRef != nil { - allErrs = append(allErrs, validation.ValidateImmutableField(newSpec.ServerRef, oldSpec.ServerRef, fldPath.Child("serverRef"))...) - } - if oldSpec.ServerSelector != nil { - allErrs = append(allErrs, validation.ValidateImmutableField(newSpec.ServerSelector, oldSpec.ServerSelector, fldPath.Child("serverSelector"))...) - } - - return allErrs -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type -func (r *ServerClaim) ValidateDelete() (admission.Warnings, error) { - serverclaimlog.Info("validate delete", "name", r.Name) - - // TODO(user): fill in your validation logic upon object deletion. - return nil, nil -} diff --git a/api/v1alpha1/serverclaim_webhook_test.go b/api/v1alpha1/serverclaim_webhook_test.go deleted file mode 100644 index 836185d..0000000 --- a/api/v1alpha1/serverclaim_webhook_test.go +++ /dev/null @@ -1,111 +0,0 @@ -// 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" - v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" -) - -var _ = Describe("ServerClaim Webhook", func() { - ns := SetupTest() - - var claim *ServerClaim - var claimWithSelector *ServerClaim - - BeforeEach(func(ctx SpecContext) { - By("creating a new ServerClaim") - claim = &ServerClaim{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: ns.Name, - GenerateName: "claim-", - }, - } - Expect(k8sClient.Create(ctx, claim)).To(Succeed()) - DeferCleanup(k8sClient.Delete, claim) - - By("updating the ServerRef to claim a Server") - Eventually(Update(claim, func() { - claim.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"} - })).Should(Succeed()) - - By("creating a new ServerClaim") - claimWithSelector = &ServerClaim{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: ns.Name, - GenerateName: "claim-", - }, - } - Expect(k8sClient.Create(ctx, claimWithSelector)).To(Succeed()) - DeferCleanup(k8sClient.Delete, claimWithSelector) - - By("updating the ServerSelector to claim a Server") - Eventually(Update(claimWithSelector, func() { - claimWithSelector.Spec.ServerSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "foo": "bar", - }, - } - })).Should(Succeed()) - }) - - It("Should deny if the ServerRef changes", func() { - By("updating the ServerRef to claim a different Server") - Eventually(Update(claim, func() { - claim.Spec.ServerRef = &v1.LocalObjectReference{Name: "bar"} - })).Should(HaveOccurred()) - - By("ensuring that the ServerRef did not change") - Consistently(Object(claim)).Should(HaveField("Spec.ServerRef.Name", Equal("foo"))) - }) - - It("Should allow a change of ServerClaim by not changing the ServerRef", func() { - By("updating the ServerRef to claim a different Server") - Eventually(Update(claim, func() { - claim.Spec.Power = PowerOn - claim.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"} - })).Should(Succeed()) - - By("ensuring that the PowerState changed") - Consistently(Object(claim)).Should(SatisfyAll( - HaveField("Spec.Power", Equal(PowerOn)), - HaveField("Spec.ServerRef.Name", Equal("foo")), - )) - }) - - It("Should deny if the ServerSelector changes", func() { - By("updating the ServerRef to claim a different Server") - Eventually(Update(claimWithSelector, func() { - claimWithSelector.Spec.ServerSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "bar": "foo", - }, - } - })).Should(HaveOccurred()) - - By("ensuring that the ServerRef did not change") - Consistently(Object(claimWithSelector)).Should( - HaveField("Spec.ServerSelector.MatchLabels", Equal(map[string]string{"foo": "bar"}))) - }) - - It("Should allow a change of ServerClaim by not changing the ServerSelector", func() { - By("updating the ServerRef to claim a different Server") - Eventually(Update(claimWithSelector, func() { - claimWithSelector.Spec.Power = PowerOn - claimWithSelector.Spec.ServerSelector = &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "foo": "bar", - }, - } - })).Should(Succeed()) - - By("ensuring that the PowerState changed") - Consistently(Object(claimWithSelector)).Should(SatisfyAll( - HaveField("Spec.Power", Equal(PowerOn)), - HaveField("Spec.ServerSelector.MatchLabels", Equal(map[string]string{"foo": "bar"})))) - }) -}) diff --git a/api/v1alpha1/webhook_suite_test.go b/api/v1alpha1/webhook_suite_test.go deleted file mode 100644 index 4449ccf..0000000 --- a/api/v1alpha1/webhook_suite_test.go +++ /dev/null @@ -1,163 +0,0 @@ -// 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" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - 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" - . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" - 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" - //+kubebuilder:scaffold:imports -) - -const ( - pollingInterval = 100 * time.Millisecond - eventuallyTimeout = 3 * time.Second - consistentlyDuration = 3 * time.Second -) - -var ( - cfg *rest.Config - k8sClient client.Client - testEnv *envtest.Environment - ctx context.Context - cancel context.CancelFunc -) - -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 = AddToScheme(scheme) - Expect(corev1.AddToScheme(scheme)).To(Succeed()) - Expect(err).NotTo(HaveOccurred()) - - Expect(admissionv1.AddToScheme(scheme)).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()) - - Expect((&ServerClaim{}).SetupWebhookWithManager(mgr)).NotTo(HaveOccurred()) - Expect((&BMC{}).SetupWebhookWithManager(mgr)).NotTo(HaveOccurred()) - - //+kubebuilder:scaffold:webhook - - go func() { - defer GinkgoRecover() - Expect(mgr.Start(ctx)).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()) - - // set komega client - SetClient(k8sClient) -}) - -func SetupTest() *corev1.Namespace { - ns := &corev1.Namespace{} - - BeforeEach(func(ctx SpecContext) { - *ns = corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-", - }, - } - Expect(k8sClient.Create(ctx, ns)).To(Succeed(), "failed to create test namespace") - DeferCleanup(k8sClient.Delete, ns) - }) - - return ns -} - -var _ = AfterSuite(func() { - cancel() - By("tearing down the test environment") - err := testEnv.Stop() - Expect(err).NotTo(HaveOccurred()) -}) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 3828a77..2ac9af0 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -8,9 +8,9 @@ package v1alpha1 import ( - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" + runtime "k8s.io/apimachinery/pkg/runtime" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 8a05242..dd8ab3e 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -241,18 +241,6 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "ServerClaim") os.Exit(1) } - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = (&metalv1alpha1.ServerClaim{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "ServerClaim") - os.Exit(1) - } - } - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = (&metalv1alpha1.BMC{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "BMC") - os.Exit(1) - } - } //+kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/config/crd/bases/metal.ironcore.dev_bmcs.yaml b/config/crd/bases/metal.ironcore.dev_bmcs.yaml index 4adf1f0..d310379 100644 --- a/config/crd/bases/metal.ironcore.dev_bmcs.yaml +++ b/config/crd/bases/metal.ironcore.dev_bmcs.yaml @@ -85,6 +85,9 @@ spec: - ip - macAddress type: object + x-kubernetes-validations: + - message: access is immutable + rule: self == oldSelf bmcSecretRef: description: |- BMCSecretRef is a reference to the Kubernetes Secret object that contains the credentials @@ -137,6 +140,9 @@ spec: type: string type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: endpointRef is immutable + rule: self == oldSelf protocol: description: |- Protocol specifies the protocol to be used for communicating with the BMC. @@ -161,6 +167,9 @@ spec: - bmcSecretRef - protocol type: object + x-kubernetes-validations: + - message: exactly one of access or endpointRef needs to be set + rule: has(self.access) != has(self.endpointRef) status: description: BMCStatus defines the observed state of BMC. properties: diff --git a/config/crd/bases/metal.ironcore.dev_serverclaims.yaml b/config/crd/bases/metal.ironcore.dev_serverclaims.yaml index e9ab877..fe97561 100644 --- a/config/crd/bases/metal.ironcore.dev_serverclaims.yaml +++ b/config/crd/bases/metal.ironcore.dev_serverclaims.yaml @@ -93,6 +93,9 @@ spec: type: string type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: serverRef is immutable + rule: self == oldSelf serverSelector: description: |- ServerSelector specifies a label selector to identify the server to be claimed. @@ -141,10 +144,18 @@ spec: type: object type: object x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: serverSelector is immutable + rule: self == oldSelf required: - image - power type: object + x-kubernetes-validations: + - message: serverRef is required once set + rule: '!has(oldSelf.serverRef) || has(self.serverRef)' + - message: serverSelector is required once set + rule: '!has(oldSelf.serverSelector) || has(self.serverSelector)' status: description: ServerClaimStatus defines the observed state of ServerClaim. properties: diff --git a/config/webhook/kustomization.yaml b/config/webhook/kustomization.yaml deleted file mode 100644 index 9cf2613..0000000 --- a/config/webhook/kustomization.yaml +++ /dev/null @@ -1,6 +0,0 @@ -resources: -- manifests.yaml -- service.yaml - -configurations: -- kustomizeconfig.yaml diff --git a/config/webhook/kustomizeconfig.yaml b/config/webhook/kustomizeconfig.yaml deleted file mode 100644 index 206316e..0000000 --- a/config/webhook/kustomizeconfig.yaml +++ /dev/null @@ -1,22 +0,0 @@ -# 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 deleted file mode 100644 index b6d7f3e..0000000 --- a/config/webhook/manifests.yaml +++ /dev/null @@ -1,46 +0,0 @@ ---- -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-bmc - failurePolicy: Fail - name: vbmc.kb.io - rules: - - apiGroups: - - metal.ironcore.dev - apiVersions: - - v1alpha1 - operations: - - CREATE - - UPDATE - resources: - - bmcs - sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-metal-ironcore-dev-v1alpha1-serverclaim - failurePolicy: Fail - name: vserverclaim.kb.io - rules: - - apiGroups: - - metal.ironcore.dev - apiVersions: - - v1alpha1 - operations: - - CREATE - - UPDATE - resources: - - serverclaims - sideEffects: None diff --git a/config/webhook/service.yaml b/config/webhook/service.yaml deleted file mode 100644 index 2b12aa9..0000000 --- a/config/webhook/service.yaml +++ /dev/null @@ -1,19 +0,0 @@ -apiVersion: v1 -kind: Service -metadata: - labels: - app.kubernetes.io/name: service - app.kubernetes.io/instance: webhook-service - app.kubernetes.io/component: webhook - app.kubernetes.io/created-by: metal-operator - app.kubernetes.io/part-of: metal-operator - app.kubernetes.io/managed-by: kustomize - name: webhook-service - namespace: system -spec: - ports: - - port: 443 - protocol: TCP - targetPort: 9445 - selector: - control-plane: controller-manager diff --git a/internal/controller/bmc_controller_test.go b/internal/controller/bmc_controller_test.go index 4edff43..0b3c6e7 100644 --- a/internal/controller/bmc_controller_test.go +++ b/internal/controller/bmc_controller_test.go @@ -8,6 +8,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" @@ -168,3 +169,152 @@ var _ = Describe("BMC Controller", func() { }) }) + +var _ = Describe("BMC Validation", func() { + _ = SetupTest() + + It("Should deny if the BMC has EndpointRef and InlineEndpoint spec fields", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-invalid", + }, + Spec: metalv1alpha1.BMCSpec{ + EndpointRef: &v1.LocalObjectReference{Name: "foo"}, + Endpoint: &metalv1alpha1.InlineEndpoint{ + IP: metalv1alpha1.MustParseIP("127.0.0.1"), + MACAddress: "aa:bb:cc:dd:ee:ff", + }, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(HaveOccurred()) + Eventually(Get(bmc)).Should(Satisfy(errors.IsNotFound)) + }) + + It("Should deny if the BMC has no EndpointRef and InlineEndpoint spec fields", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-empty", + }, + Spec: metalv1alpha1.BMCSpec{}, + } + Expect(k8sClient.Create(ctx, bmc)).To(HaveOccurred()) + Eventually(Get(bmc)).Should(Satisfy(errors.IsNotFound)) + }) + + It("Should admit if the BMC has an EndpointRef but no InlineEndpoint spec field", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.BMCSpec{ + EndpointRef: &v1.LocalObjectReference{Name: "foo"}, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) + DeferCleanup(k8sClient.Delete, bmc) + }) + + It("Should deny if the BMC EndpointRef spec field has been removed", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.BMCSpec{ + EndpointRef: &v1.LocalObjectReference{Name: "foo"}, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) + DeferCleanup(k8sClient.Delete, bmc) + + Eventually(Update(bmc, func() { + bmc.Spec.EndpointRef = nil + })).Should(Not(Succeed())) + + Eventually(Object(bmc)).Should(SatisfyAll(HaveField( + "Spec.EndpointRef", &v1.LocalObjectReference{Name: "foo"}))) + }) + + It("Should admit if the BMC is changing EndpointRef to InlineEndpoint spec field", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.BMCSpec{ + EndpointRef: &v1.LocalObjectReference{Name: "foo"}, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) + DeferCleanup(k8sClient.Delete, bmc) + + Eventually(Update(bmc, func() { + bmc.Spec.EndpointRef = nil + bmc.Spec.Endpoint = &metalv1alpha1.InlineEndpoint{ + IP: metalv1alpha1.MustParseIP("127.0.0.1"), + MACAddress: "aa:bb:cc:dd:ee:ff", + } + })).Should(Succeed()) + }) + + It("Should admit if the BMC has no EndpointRef but an InlineEndpoint spec field", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.BMCSpec{ + Endpoint: &metalv1alpha1.InlineEndpoint{ + IP: metalv1alpha1.MustParseIP("127.0.0.1"), + MACAddress: "aa:bb:cc:dd:ee:ff", + }, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) + DeferCleanup(k8sClient.Delete, bmc) + }) + + It("Should deny if the BMC InlineEndpoint spec field has been removed", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.BMCSpec{ + Endpoint: &metalv1alpha1.InlineEndpoint{ + IP: metalv1alpha1.MustParseIP("127.0.0.1"), + MACAddress: "aa:bb:cc:dd:ee:ff", + }, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) + DeferCleanup(k8sClient.Delete, bmc) + + Eventually(Update(bmc, func() { + bmc.Spec.Endpoint = nil + })).Should(Not(Succeed())) + + Eventually(Object(bmc)).Should(SatisfyAll( + HaveField("Spec.Endpoint.IP", metalv1alpha1.MustParseIP("127.0.0.1")), + HaveField("Spec.Endpoint.MACAddress", "aa:bb:cc:dd:ee:ff"), + )) + }) + + It("Should admit if the BMC has is changing to an EndpointRef from an InlineEndpoint spec field", func(ctx SpecContext) { + bmc := &metalv1alpha1.BMC{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: metalv1alpha1.BMCSpec{ + Endpoint: &metalv1alpha1.InlineEndpoint{ + IP: metalv1alpha1.MustParseIP("127.0.0.1"), + MACAddress: "aa:bb:cc:dd:ee:ff", + }, + }, + } + Expect(k8sClient.Create(ctx, bmc)).To(Succeed()) + DeferCleanup(k8sClient.Delete, bmc) + + Eventually(Update(bmc, func() { + bmc.Spec.EndpointRef = &v1.LocalObjectReference{Name: "foo"} + bmc.Spec.Endpoint = nil + })).Should(Succeed()) + }) + +}) diff --git a/internal/controller/serverclaim_controller_test.go b/internal/controller/serverclaim_controller_test.go index c39a195..046f762 100644 --- a/internal/controller/serverclaim_controller_test.go +++ b/internal/controller/serverclaim_controller_test.go @@ -448,3 +448,102 @@ var _ = Describe("ServerClaim Controller", func() { Eventually(Get(claim)).Should(Satisfy(apierrors.IsNotFound)) }) }) + +var _ = Describe("ServerClaim Validation", func() { + ns := SetupTest() + + var claim *metalv1alpha1.ServerClaim + var claimWithSelector *metalv1alpha1.ServerClaim + + BeforeEach(func(ctx SpecContext) { + By("creating a new ServerClaim") + claim = &metalv1alpha1.ServerClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "claim-", + }, + } + Expect(k8sClient.Create(ctx, claim)).To(Succeed()) + DeferCleanup(k8sClient.Delete, claim) + + By("updating the ServerRef to claim a Server") + Eventually(Update(claim, func() { + claim.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"} + })).Should(Succeed()) + + By("creating a new ServerClaim") + claimWithSelector = &metalv1alpha1.ServerClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "claim-", + }, + } + Expect(k8sClient.Create(ctx, claimWithSelector)).To(Succeed()) + DeferCleanup(k8sClient.Delete, claimWithSelector) + + By("updating the ServerSelector to claim a Server") + Eventually(Update(claimWithSelector, func() { + claimWithSelector.Spec.ServerSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + } + })).Should(Succeed()) + }) + + It("Should deny if the ServerRef changes", func() { + By("updating the ServerRef to claim a different Server") + Eventually(Update(claim, func() { + claim.Spec.ServerRef = &v1.LocalObjectReference{Name: "bar"} + })).Should(HaveOccurred()) + + By("ensuring that the ServerRef did not change") + Consistently(Object(claim)).Should(HaveField("Spec.ServerRef.Name", Equal("foo"))) + }) + + It("Should allow a change of ServerClaim by not changing the ServerRef", func() { + By("updating the ServerRef to claim a different Server") + Eventually(Update(claim, func() { + claim.Spec.Power = metalv1alpha1.PowerOn + claim.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"} + })).Should(Succeed()) + + By("ensuring that the PowerState changed") + Consistently(Object(claim)).Should(SatisfyAll( + HaveField("Spec.Power", Equal(metalv1alpha1.PowerOn)), + HaveField("Spec.ServerRef.Name", Equal("foo")), + )) + }) + + It("Should deny if the ServerSelector changes", func() { + By("updating the ServerRef to claim a different Server") + Eventually(Update(claimWithSelector, func() { + claimWithSelector.Spec.ServerSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "bar": "foo", + }, + } + })).Should(HaveOccurred()) + + By("ensuring that the ServerRef did not change") + Consistently(Object(claimWithSelector)).Should( + HaveField("Spec.ServerSelector.MatchLabels", Equal(map[string]string{"foo": "bar"}))) + }) + + It("Should allow a change of ServerClaim by not changing the ServerSelector", func() { + By("updating the ServerRef to claim a different Server") + Eventually(Update(claimWithSelector, func() { + claimWithSelector.Spec.Power = metalv1alpha1.PowerOn + claimWithSelector.Spec.ServerSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", + }, + } + })).Should(Succeed()) + + By("ensuring that the PowerState changed") + Consistently(Object(claimWithSelector)).Should(SatisfyAll( + HaveField("Spec.Power", Equal(metalv1alpha1.PowerOn)), + HaveField("Spec.ServerSelector.MatchLabels", Equal(map[string]string{"foo": "bar"})))) + }) +})