From a6bce167d029504a2565a0a5537753148ec461fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Andr=C3=A9?= Date: Tue, 9 Jan 2024 14:26:23 +0100 Subject: [PATCH 1/5] CI: Hide errors for expected failures The CI script should not show errors for expected failures, such as when checking if a resource exists. This commit redirects the standard error to standard output, which is in turn redirected to `/dev/null`. --- hack/ci/gce-project.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hack/ci/gce-project.sh b/hack/ci/gce-project.sh index e376015c27..22d4a059ca 100755 --- a/hack/ci/gce-project.sh +++ b/hack/ci/gce-project.sh @@ -34,12 +34,12 @@ function cloud_init { # Generate local ssh configuration # NOTE(mdbooth): This command successfully populates ssh config and then # fails for some reason I don't understand. We ignore the failure. - gcloud compute config-ssh || true + gcloud compute config-ssh >/dev/null 2>&1 || true } function init_infrastructure() { if [[ ${GCP_NETWORK_NAME} != "default" ]]; then - if ! gcloud compute networks describe "$GCP_NETWORK_NAME" --project "$GCP_PROJECT" >/dev/null; then + if ! gcloud compute networks describe "$GCP_NETWORK_NAME" --project "$GCP_PROJECT" >/dev/null 2>&1; then gcloud compute networks create --project "$GCP_PROJECT" "$GCP_NETWORK_NAME" --subnet-mode custom gcloud compute networks subnets create "$GCP_NETWORK_NAME" --project "$GCP_PROJECT" \ --network="$GCP_NETWORK_NAME" --range="$PRIVATE_NETWORK_CIDR" --region "$GCP_REGION" @@ -64,12 +64,12 @@ function init_infrastructure() { gcloud compute networks list --project="$GCP_PROJECT" gcloud compute networks describe "$GCP_NETWORK_NAME" --project="$GCP_PROJECT" - if ! gcloud compute routers describe "${CLUSTER_NAME}-myrouter" --project="$GCP_PROJECT" --region="$GCP_REGION" >/dev/null; then + if ! gcloud compute routers describe "${CLUSTER_NAME}-myrouter" --project="$GCP_PROJECT" --region="$GCP_REGION" >/dev/null 2>&1; then gcloud compute routers create "${CLUSTER_NAME}-myrouter" --project="$GCP_PROJECT" \ --region="$GCP_REGION" --network="$GCP_NETWORK_NAME" fi if ! gcloud compute routers nats describe --router="$CLUSTER_NAME-myrouter" "$CLUSTER_NAME-mynat" \ - --project="$GCP_PROJECT" --region="${GCP_REGION}" >/dev/null; then + --project="$GCP_PROJECT" --region="${GCP_REGION}" >/dev/null 2>&1; then gcloud compute routers nats create "${CLUSTER_NAME}-mynat" --project="$GCP_PROJECT" \ --router-region="$GCP_REGION" --router="${CLUSTER_NAME}-myrouter" \ --nat-all-subnet-ip-ranges --auto-allocate-nat-external-ips @@ -89,7 +89,7 @@ function create_vm { # Loop over all zones in the GCP region to ignore a full zone. # We are not able to use 'gcloud compute zones list' as the gcloud.compute.zones.list permission is missing. for GCP_ZONE in "${GCP_REGION}-a" "${GCP_REGION}-b" "${GCP_REGION}-c"; do - if ! gcloud compute instances describe "$servername" --project "$GCP_PROJECT" --zone "$GCP_ZONE" >/dev/null; then + if ! gcloud compute instances describe "$servername" --project "$GCP_PROJECT" --zone "$GCP_ZONE" >/dev/null 2>&1; then if gcloud compute instances create "$servername" \ --project "$GCP_PROJECT" \ --zone "$GCP_ZONE" \ From d5ed89e75564bad0e14718689b1a1b14b143c5aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Andr=C3=A9?= Date: Wed, 10 Jan 2024 10:42:42 +0100 Subject: [PATCH 2/5] Consolidate test names It should start with lowercase, and should not include the redundant `It`. --- test/e2e/suites/e2e/e2e_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/suites/e2e/e2e_test.go b/test/e2e/suites/e2e/e2e_test.go index bb135a580f..bab6c7dd0b 100644 --- a/test/e2e/suites/e2e/e2e_test.go +++ b/test/e2e/suites/e2e/e2e_test.go @@ -85,7 +85,7 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { }) Describe("Workload cluster (default)", func() { - It("It should be creatable and deletable", func() { + It("should be creatable and deletable", func() { shared.Logf("Creating a cluster") clusterName := fmt.Sprintf("cluster-%s", namespace.Name) configCluster := defaultConfigCluster(clusterName, namespace.Name) @@ -206,7 +206,7 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { }) Describe("Workload cluster (without lb)", func() { - It("Should create port(s) with custom options", func() { + It("should create port(s) with custom options", func() { shared.Logf("Creating a cluster") clusterName := fmt.Sprintf("cluster-%s", namespace.Name) configCluster := defaultConfigCluster(clusterName, namespace.Name) @@ -526,7 +526,7 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { }) Describe("MachineDeployment misconfigurations", func() { - It("Should fail to create MachineDeployment with invalid subnet or invalid availability zone", func() { + It("should fail to create MachineDeployment with invalid subnet or invalid availability zone", func() { shared.Logf("Creating a cluster") clusterName := fmt.Sprintf("cluster-%s", namespace.Name) configCluster := defaultConfigCluster(clusterName, namespace.Name) @@ -594,7 +594,7 @@ var _ = Describe("e2e tests [PR-Blocking]", func() { Expect(err).NotTo(HaveOccurred()) }) - It("It should be creatable and deletable", func() { + It("should be creatable and deletable", func() { workerMachines := framework.GetMachinesByMachineDeployments(ctx, framework.GetMachinesByMachineDeploymentsInput{ Lister: e2eCtx.Environment.BootstrapClusterProxy.GetClient(), ClusterName: clusterName, From 22f291582a74ac494ba6c90e3b737857c2be1dce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Andr=C3=A9?= Date: Wed, 6 Aug 2025 12:16:00 +0200 Subject: [PATCH 3/5] Bump flatcar image The previous one was deleted by mistake, use a newer one that is available. --- hack/ci/cloud-init/controller.yaml.tpl | 2 +- test/e2e/data/e2e_conf.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/ci/cloud-init/controller.yaml.tpl b/hack/ci/cloud-init/controller.yaml.tpl index 12a6d6faff..b056850af3 100644 --- a/hack/ci/cloud-init/controller.yaml.tpl +++ b/hack/ci/cloud-init/controller.yaml.tpl @@ -83,7 +83,7 @@ IMAGE_URLS+="https://storage.googleapis.com/artifacts.k8s-staging-capi-openstack.appspot.com/test/cirros/2022-12-05/cirros-0.6.1-x86_64-disk.img," IMAGE_URLS+="https://storage.googleapis.com/artifacts.k8s-staging-capi-openstack.appspot.com/test/ubuntu/2023-09-29/ubuntu-2204-kube-v1.27.2.img," IMAGE_URLS+="https://storage.googleapis.com/artifacts.k8s-staging-capi-openstack.appspot.com/test/ubuntu/2023-09-29/ubuntu-2204-kube-v1.28.2.img," - IMAGE_URLS+="https://storage.googleapis.com/artifacts.k8s-staging-capi-openstack.appspot.com/test/flatcar/flatcar-stable-3602.2.0-kube-v1.28.2.img" + IMAGE_URLS+="https://storage.googleapis.com/artifacts.k8s-staging-capi-openstack.appspot.com/test/flatcar/flatcar-stable-3815.2.0-kube-v1.28.5.img" [[post-config|$NOVA_CONF]] [DEFAULT] diff --git a/test/e2e/data/e2e_conf.yaml b/test/e2e/data/e2e_conf.yaml index b1e30eb1ad..99c53f1b0a 100644 --- a/test/e2e/data/e2e_conf.yaml +++ b/test/e2e/data/e2e_conf.yaml @@ -197,7 +197,7 @@ variables: # The default user for SSH connections from bastion to machines SSH_USER_MACHINE: "ubuntu" EXP_KUBEADM_BOOTSTRAP_FORMAT_IGNITION: "true" - OPENSTACK_FLATCAR_IMAGE_NAME: "flatcar-stable-3602.2.0-kube-v1.28.2" + OPENSTACK_FLATCAR_IMAGE_NAME: "flatcar-stable-3815.2.0-kube-v1.28.5" intervals: conformance/wait-control-plane: ["30m", "10s"] From cb3d21290eef1f1e4dcb9d0bd3a50d55bf857c3a Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 26 Jun 2025 10:15:02 +0100 Subject: [PATCH 4/5] Move webhooks into pkg/webhooks Moves webhooks from api to pkg/webhooks making only mechanical code changes except for the removal of the defaulting webhooks, because they weren't used. This results in there now being no mutating webhook configured. NOTE(stephenfin): There were a lot of conflicts here. These were mostly mitigated by faking the addition of v1alpha8, which moved the webhooks to the 'api/v1alpha8' package (commit 750b84de45), followed by the subsequent rename of this package to v1beta1 (commit e9fb53c937), for the webhook files and tests. This still resulted in some merge conflicts due the v1alpha8 changes such as 564b6bd89 and 4368c4f2be (which we obviously don't want to include here) but it made the backport much simpler. NOTE(stephenfin): We also include commit 30ba121d773b5665798500828e4ffcc467afbc15 which was a follow-up that fixed the CRD generation broken in this commit. Signed-off-by: Stephen Finucane (cherry picked from commit 750b84de45c2e884daa690143a56091e2b79a3f8) --- Makefile | 4 +- api/v1alpha7/identity_types.go | 2 - api/v1alpha7/openstackcluster_webhook.go | 153 -------------- api/v1alpha7/openstackclusterlist_webhook.go | 32 --- .../openstackclustertemplate_webhook.go | 85 -------- .../openstackmachinetemplatelist_webhook.go | 32 --- api/v1alpha7/zz_generated.deepcopy.go | 2 +- config/webhook/manifests.yaml | 69 ------- main.go | 32 +-- .../webhooks.go => pkg/webhooks/errors.go | 2 +- .../webhooks/identity_types.go | 17 +- pkg/webhooks/openstackcluster_webhook.go | 161 +++++++++++++++ .../openstackcluster_webhook_test.go | 195 +++++++++--------- .../openstackclustertemplate_webhook.go | 116 +++++++++++ .../webhooks}/openstackmachine_webhook.go | 86 +++++--- .../openstackmachinetemplate_webhook.go | 61 +++--- .../openstackmachinetemplate_webhook_test.go | 74 +++---- pkg/webhooks/register.go | 63 ++++++ 18 files changed, 582 insertions(+), 604 deletions(-) delete mode 100644 api/v1alpha7/openstackcluster_webhook.go delete mode 100644 api/v1alpha7/openstackclusterlist_webhook.go delete mode 100644 api/v1alpha7/openstackclustertemplate_webhook.go delete mode 100644 api/v1alpha7/openstackmachinetemplatelist_webhook.go rename api/v1alpha7/webhooks.go => pkg/webhooks/errors.go (98%) rename api/v1alpha7/openstackmachinelist_webhook.go => pkg/webhooks/identity_types.go (57%) create mode 100644 pkg/webhooks/openstackcluster_webhook.go rename {api/v1alpha7 => pkg/webhooks}/openstackcluster_webhook_test.go (59%) create mode 100644 pkg/webhooks/openstackclustertemplate_webhook.go rename {api/v1alpha7 => pkg/webhooks}/openstackmachine_webhook.go (52%) rename {api/v1alpha7 => pkg/webhooks}/openstackmachinetemplate_webhook.go (55%) rename {api/v1alpha7 => pkg/webhooks}/openstackmachinetemplate_webhook_test.go (61%) create mode 100644 pkg/webhooks/register.go diff --git a/Makefile b/Makefile index 479e71c9d6..8544291ee9 100644 --- a/Makefile +++ b/Makefile @@ -269,7 +269,9 @@ generate-manifests: $(CONTROLLER_GEN) ## Generate manifests e.g. CRD, RBAC etc. $(CONTROLLER_GEN) \ paths=./api/... \ crd:crdVersions=v1 \ - output:crd:dir=$(CRD_ROOT) \ + output:crd:dir=$(CRD_ROOT) + $(CONTROLLER_GEN) \ + paths=./pkg/webhooks/... \ output:webhook:dir=$(WEBHOOK_ROOT) \ webhook $(CONTROLLER_GEN) \ diff --git a/api/v1alpha7/identity_types.go b/api/v1alpha7/identity_types.go index 15cd58562b..1ea5f2fb9b 100644 --- a/api/v1alpha7/identity_types.go +++ b/api/v1alpha7/identity_types.go @@ -16,8 +16,6 @@ limitations under the License. package v1alpha7 -const defaultIdentityRefKind = "Secret" - // OpenStackIdentityReference is a reference to an infrastructure // provider identity to be used to provision cluster resources. type OpenStackIdentityReference struct { diff --git a/api/v1alpha7/openstackcluster_webhook.go b/api/v1alpha7/openstackcluster_webhook.go deleted file mode 100644 index 706d10a8c6..0000000000 --- a/api/v1alpha7/openstackcluster_webhook.go +++ /dev/null @@ -1,153 +0,0 @@ -/* -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 v1alpha7 - -import ( - "fmt" - "reflect" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/controller-runtime/pkg/builder" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -// log is for logging in this package. -var _ = logf.Log.WithName("openstackcluster-resource") - -func (r *OpenStackCluster) SetupWebhookWithManager(mgr manager.Manager) error { - return builder.WebhookManagedBy(mgr). - For(r). - Complete() -} - -// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackcluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclusters,versions=v1alpha7,name=validation.openstackcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackcluster,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclusters,versions=v1alpha7,name=default.openstackcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 - -var ( - _ webhook.Defaulter = &OpenStackCluster{} - _ webhook.Validator = &OpenStackCluster{} -) - -// Default satisfies the defaulting webhook interface. -func (r *OpenStackCluster) Default() { - if r.Spec.IdentityRef != nil && r.Spec.IdentityRef.Kind == "" { - r.Spec.IdentityRef.Kind = defaultIdentityRefKind - } -} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackCluster) ValidateCreate() (admission.Warnings, error) { - var allErrs field.ErrorList - - if r.Spec.IdentityRef != nil && r.Spec.IdentityRef.Kind != defaultIdentityRefKind { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "identityRef", "kind"), "must be a Secret")) - } - - return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackCluster) ValidateUpdate(oldRaw runtime.Object) (admission.Warnings, error) { - var allErrs field.ErrorList - old, ok := oldRaw.(*OpenStackCluster) - if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an OpenStackCluster but got a %T", oldRaw)) - } - - if r.Spec.IdentityRef != nil && r.Spec.IdentityRef.Kind != defaultIdentityRefKind { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "identityRef", "kind"), - r.Spec.IdentityRef, "must be a Secret"), - ) - } - - // Allow changes to Spec.IdentityRef.Name. - if old.Spec.IdentityRef != nil && r.Spec.IdentityRef != nil { - old.Spec.IdentityRef.Name = "" - r.Spec.IdentityRef.Name = "" - } - - // Allow changes to Spec.IdentityRef if it was unset. - if old.Spec.IdentityRef == nil && r.Spec.IdentityRef != nil { - old.Spec.IdentityRef = &OpenStackIdentityReference{} - r.Spec.IdentityRef = &OpenStackIdentityReference{} - } - - if old.Spec.IdentityRef != nil && r.Spec.IdentityRef == nil { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "identityRef"), - r.Spec.IdentityRef, "field cannot be set to nil"), - ) - } - - // Allow change only for the first time. - if old.Spec.ControlPlaneEndpoint.Host == "" { - old.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{} - r.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{} - } - - // Allow change only for the first time. - if old.Spec.DisableAPIServerFloatingIP && old.Spec.APIServerFixedIP == "" { - r.Spec.APIServerFixedIP = "" - } - - // If API Server floating IP is disabled, allow the change of the API Server port only for the first time. - if old.Spec.DisableAPIServerFloatingIP && old.Spec.APIServerPort == 0 && r.Spec.APIServerPort > 0 { - r.Spec.APIServerPort = 0 - } - - // Allow changes to the bastion spec. - old.Spec.Bastion = &Bastion{} - r.Spec.Bastion = &Bastion{} - - // Allow changes on AllowedCIDRs - if r.Spec.APIServerLoadBalancer.Enabled { - old.Spec.APIServerLoadBalancer.AllowedCIDRs = []string{} - r.Spec.APIServerLoadBalancer.AllowedCIDRs = []string{} - } - - // Allow changes to the availability zones. - old.Spec.ControlPlaneAvailabilityZones = []string{} - r.Spec.ControlPlaneAvailabilityZones = []string{} - - // Allow change to the allowAllInClusterTraffic. - old.Spec.AllowAllInClusterTraffic = false - r.Spec.AllowAllInClusterTraffic = false - - // Allow change on the spec.APIServerFloatingIP only if it matches the current api server loadbalancer IP. - if old.Status.APIServerLoadBalancer != nil && r.Spec.APIServerFloatingIP == old.Status.APIServerLoadBalancer.IP { - r.Spec.APIServerFloatingIP = "" - old.Spec.APIServerFloatingIP = "" - } - - if !reflect.DeepEqual(old.Spec, r.Spec) { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified")) - } - - return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackCluster) ValidateDelete() (admission.Warnings, error) { - return nil, nil -} diff --git a/api/v1alpha7/openstackclusterlist_webhook.go b/api/v1alpha7/openstackclusterlist_webhook.go deleted file mode 100644 index f7749b7ade..0000000000 --- a/api/v1alpha7/openstackclusterlist_webhook.go +++ /dev/null @@ -1,32 +0,0 @@ -/* -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 v1alpha7 - -import ( - "sigs.k8s.io/controller-runtime/pkg/builder" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" -) - -// log is for logging in this package. -var _ = logf.Log.WithName("openstackclusterlist-resource") - -func (r *OpenStackClusterList) SetupWebhookWithManager(mgr manager.Manager) error { - return builder.WebhookManagedBy(mgr). - For(r). - Complete() -} diff --git a/api/v1alpha7/openstackclustertemplate_webhook.go b/api/v1alpha7/openstackclustertemplate_webhook.go deleted file mode 100644 index 337928a3f1..0000000000 --- a/api/v1alpha7/openstackclustertemplate_webhook.go +++ /dev/null @@ -1,85 +0,0 @@ -/* -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 v1alpha7 - -import ( - "fmt" - "reflect" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation/field" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -const openStackClusterTemplateImmutableMsg = "OpenStackClusterTemplate spec.template.spec field is immutable. Please create new resource instead." - -func (r *OpenStackClusterTemplate) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() -} - -// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackclustertemplate,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclustertemplates,versions=v1alpha7,name=default.openstackclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackclustertemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclustertemplates,versions=v1alpha7,name=validation.openstackclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 - -var ( - _ webhook.Defaulter = &OpenStackClusterTemplate{} - _ webhook.Validator = &OpenStackClusterTemplate{} -) - -// Default implements webhook.Defaulter so a webhook will be registered for the type. -func (r *OpenStackClusterTemplate) Default() { - if r.Spec.Template.Spec.IdentityRef != nil && r.Spec.Template.Spec.IdentityRef.Kind == "" { - r.Spec.Template.Spec.IdentityRef.Kind = defaultIdentityRefKind - } -} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackClusterTemplate) ValidateCreate() (admission.Warnings, error) { - var allErrs field.ErrorList - - if r.Spec.Template.Spec.IdentityRef != nil && r.Spec.Template.Spec.IdentityRef.Kind != defaultIdentityRefKind { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "template", "spec", "identityRef", "kind"), "must be a Secret")) - } - - return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackClusterTemplate) ValidateUpdate(oldRaw runtime.Object) (admission.Warnings, error) { - var allErrs field.ErrorList - old, ok := oldRaw.(*OpenStackClusterTemplate) - if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an OpenStackClusterTemplate but got a %T", oldRaw)) - } - - if !reflect.DeepEqual(r.Spec.Template.Spec, old.Spec.Template.Spec) { - allErrs = append(allErrs, - field.Invalid(field.NewPath("OpenStackClusterTemplate", "spec", "template", "spec"), r, openStackClusterTemplateImmutableMsg), - ) - } - - return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackClusterTemplate) ValidateDelete() (admission.Warnings, error) { - return nil, nil -} diff --git a/api/v1alpha7/openstackmachinetemplatelist_webhook.go b/api/v1alpha7/openstackmachinetemplatelist_webhook.go deleted file mode 100644 index c1ff00c3dd..0000000000 --- a/api/v1alpha7/openstackmachinetemplatelist_webhook.go +++ /dev/null @@ -1,32 +0,0 @@ -/* -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 v1alpha7 - -import ( - "sigs.k8s.io/controller-runtime/pkg/builder" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" -) - -// log is for logging in this package. -var _ = logf.Log.WithName("openstackmachinetemplatelist-resource") - -func (r *OpenStackMachineTemplateList) SetupWebhookWithManager(mgr manager.Manager) error { - return builder.WebhookManagedBy(mgr). - For(r). - Complete() -} diff --git a/api/v1alpha7/zz_generated.deepcopy.go b/api/v1alpha7/zz_generated.deepcopy.go index 1d81a83410..2569c2d1b3 100644 --- a/api/v1alpha7/zz_generated.deepcopy.go +++ b/api/v1alpha7/zz_generated.deepcopy.go @@ -22,7 +22,7 @@ package v1alpha7 import ( "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" + runtime "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/errors" ) diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index b2e5ad4c2b..195ce4a34c 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -1,74 +1,5 @@ --- apiVersion: admissionregistration.k8s.io/v1 -kind: MutatingWebhookConfiguration -metadata: - name: mutating-webhook-configuration -webhooks: -- admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackcluster - failurePolicy: Fail - matchPolicy: Equivalent - name: default.openstackcluster.infrastructure.cluster.x-k8s.io - rules: - - apiGroups: - - infrastructure.cluster.x-k8s.io - apiVersions: - - v1alpha7 - operations: - - CREATE - - UPDATE - resources: - - openstackclusters - sideEffects: None -- admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackclustertemplate - failurePolicy: Fail - matchPolicy: Equivalent - name: default.openstackclustertemplate.infrastructure.cluster.x-k8s.io - rules: - - apiGroups: - - infrastructure.cluster.x-k8s.io - apiVersions: - - v1alpha7 - operations: - - CREATE - - UPDATE - resources: - - openstackclustertemplates - sideEffects: None -- admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackmachine - failurePolicy: Fail - matchPolicy: Equivalent - name: default.openstackmachine.infrastructure.cluster.x-k8s.io - rules: - - apiGroups: - - infrastructure.cluster.x-k8s.io - apiVersions: - - v1alpha7 - operations: - - CREATE - - UPDATE - resources: - - openstackmachines - sideEffects: None ---- -apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration diff --git a/main.go b/main.go index 771fc4c323..770a0e280e 100644 --- a/main.go +++ b/main.go @@ -49,6 +49,7 @@ import ( "sigs.k8s.io/cluster-api-provider-openstack/pkg/metrics" "sigs.k8s.io/cluster-api-provider-openstack/pkg/record" "sigs.k8s.io/cluster-api-provider-openstack/pkg/scope" + "sigs.k8s.io/cluster-api-provider-openstack/pkg/webhooks" "sigs.k8s.io/cluster-api-provider-openstack/version" ) @@ -297,32 +298,11 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager, caCerts []byte, sco } func setupWebhooks(mgr ctrl.Manager) { - if err := (&infrav1.OpenStackMachineTemplateWebhook{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackMachineTemplate") - os.Exit(1) - } - if err := (&infrav1.OpenStackMachineTemplateList{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackMachineTemplateList") - os.Exit(1) - } - if err := (&infrav1.OpenStackCluster{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackCluster") - os.Exit(1) - } - if err := (&infrav1.OpenStackClusterTemplate{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackClusterTemplate") - os.Exit(1) - } - if err := (&infrav1.OpenStackMachine{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackMachine") - os.Exit(1) - } - if err := (&infrav1.OpenStackMachineList{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackMachineList") - os.Exit(1) - } - if err := (&infrav1.OpenStackClusterList{}).SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackClusterList") + errs := webhooks.RegisterAllWithManager(mgr) + if len(errs) > 0 { + for i := range errs { + setupLog.Error(errs[i], "unable to register webhook") + } os.Exit(1) } } diff --git a/api/v1alpha7/webhooks.go b/pkg/webhooks/errors.go similarity index 98% rename from api/v1alpha7/webhooks.go rename to pkg/webhooks/errors.go index d58fdbb155..ad2c53aae4 100644 --- a/api/v1alpha7/webhooks.go +++ b/pkg/webhooks/errors.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha7 +package webhooks import ( apierrors "k8s.io/apimachinery/pkg/api/errors" diff --git a/api/v1alpha7/openstackmachinelist_webhook.go b/pkg/webhooks/identity_types.go similarity index 57% rename from api/v1alpha7/openstackmachinelist_webhook.go rename to pkg/webhooks/identity_types.go index 0122e1b52c..e3fcb8f45d 100644 --- a/api/v1alpha7/openstackmachinelist_webhook.go +++ b/pkg/webhooks/identity_types.go @@ -14,19 +14,6 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha7 +package webhooks -import ( - "sigs.k8s.io/controller-runtime/pkg/builder" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/manager" -) - -// log is for logging in this package. -var _ = logf.Log.WithName("openstackmachinelist-resource") - -func (r *OpenStackMachineList) SetupWebhookWithManager(mgr manager.Manager) error { - return builder.WebhookManagedBy(mgr). - For(r). - Complete() -} +const defaultIdentityRefKind = "Secret" diff --git a/pkg/webhooks/openstackcluster_webhook.go b/pkg/webhooks/openstackcluster_webhook.go new file mode 100644 index 0000000000..4827a5daa5 --- /dev/null +++ b/pkg/webhooks/openstackcluster_webhook.go @@ -0,0 +1,161 @@ +/* +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 + +import ( + "context" + "fmt" + "reflect" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" +) + +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackcluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclusters,versions=v1alpha7,name=validation.openstackcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 + +func SetupOpenStackClusterWebhook(mgr manager.Manager) error { + return builder.WebhookManagedBy(mgr). + For(&infrav1.OpenStackCluster{}). + WithValidator(&openStackClusterWebhook{}). + Complete() +} + +type openStackClusterWebhook struct{} + +// Compile-time assertion that openStackClusterWebhook implements webhook.CustomValidator. +var _ webhook.CustomValidator = &openStackClusterWebhook{} + +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackClusterWebhook) ValidateCreate(_ context.Context, objRaw runtime.Object) (admission.Warnings, error) { + var allErrs field.ErrorList + + newObj, err := castToOpenStackCluster(objRaw) + if err != nil { + return nil, err + } + + if newObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef.Kind != defaultIdentityRefKind { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "identityRef", "kind"), "must be a Secret")) + } + + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) +} + +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackClusterWebhook) ValidateUpdate(_ context.Context, oldObjRaw, newObjRaw runtime.Object) (admission.Warnings, error) { + var allErrs field.ErrorList + oldObj, err := castToOpenStackCluster(oldObjRaw) + if err != nil { + return nil, err + } + newObj, err := castToOpenStackCluster(newObjRaw) + if err != nil { + return nil, err + } + + if newObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef.Kind != defaultIdentityRefKind { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "identityRef", "kind"), + newObj.Spec.IdentityRef, "must be a Secret"), + ) + } + + // Allow changes to Spec.IdentityRef.Name. + if oldObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef != nil { + oldObj.Spec.IdentityRef.Name = "" + newObj.Spec.IdentityRef.Name = "" + } + + // Allow changes to Spec.IdentityRef if it was unset. + if oldObj.Spec.IdentityRef == nil && newObj.Spec.IdentityRef != nil { + oldObj.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{} + newObj.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{} + } + + if oldObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef == nil { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "identityRef"), + newObj.Spec.IdentityRef, "field cannot be set to nil"), + ) + } + + // Allow change only for the first time. + if oldObj.Spec.ControlPlaneEndpoint.Host == "" { + oldObj.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{} + newObj.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{} + } + + // Allow change only for the first time. + if oldObj.Spec.DisableAPIServerFloatingIP && oldObj.Spec.APIServerFixedIP == "" { + newObj.Spec.APIServerFixedIP = "" + } + + // If API Server floating IP is disabled, allow the change of the API Server port only for the first time. + if oldObj.Spec.DisableAPIServerFloatingIP && oldObj.Spec.APIServerPort == 0 && newObj.Spec.APIServerPort > 0 { + newObj.Spec.APIServerPort = 0 + } + + // Allow changes to the bastion spec. + oldObj.Spec.Bastion = &infrav1.Bastion{} + newObj.Spec.Bastion = &infrav1.Bastion{} + + // Allow changes on AllowedCIDRs + if newObj.Spec.APIServerLoadBalancer.Enabled { + oldObj.Spec.APIServerLoadBalancer.AllowedCIDRs = []string{} + newObj.Spec.APIServerLoadBalancer.AllowedCIDRs = []string{} + } + + // Allow changes to the availability zones. + oldObj.Spec.ControlPlaneAvailabilityZones = []string{} + newObj.Spec.ControlPlaneAvailabilityZones = []string{} + + // Allow change to the allowAllInClusterTraffic. + oldObj.Spec.AllowAllInClusterTraffic = false + newObj.Spec.AllowAllInClusterTraffic = false + + // Allow change on the spec.APIServerFloatingIP only if it matches the current api server loadbalancer IP. + if oldObj.Status.APIServerLoadBalancer != nil && newObj.Spec.APIServerFloatingIP == oldObj.Status.APIServerLoadBalancer.IP { + newObj.Spec.APIServerFloatingIP = "" + oldObj.Spec.APIServerFloatingIP = "" + } + + if !reflect.DeepEqual(oldObj.Spec, newObj.Spec) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified")) + } + + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) +} + +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackClusterWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, nil +} + +func castToOpenStackCluster(obj runtime.Object) (*infrav1.OpenStackCluster, error) { + cast, ok := obj.(*infrav1.OpenStackCluster) + if !ok { + return nil, fmt.Errorf("expected an OpenStackCluster but got a %T", obj) + } + return cast, nil +} diff --git a/api/v1alpha7/openstackcluster_webhook_test.go b/pkg/webhooks/openstackcluster_webhook_test.go similarity index 59% rename from api/v1alpha7/openstackcluster_webhook_test.go rename to pkg/webhooks/openstackcluster_webhook_test.go index 24e1407f2c..c3e57bae38 100644 --- a/api/v1alpha7/openstackcluster_webhook_test.go +++ b/pkg/webhooks/openstackcluster_webhook_test.go @@ -14,12 +14,15 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha7 +package webhooks import ( + "context" "testing" . "github.com/onsi/gomega" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" ) func TestOpenStackCluster_ValidateUpdate(t *testing.T) { @@ -27,25 +30,25 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { tests := []struct { name string - oldTemplate *OpenStackCluster - newTemplate *OpenStackCluster + oldTemplate *infrav1.OpenStackCluster + newTemplate *infrav1.OpenStackCluster wantErr bool }{ { name: "OpenStackCluster.Spec.IdentityRef.Kind must always be Secret", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "Secret", Name: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "foobar", Name: "foobar", }, @@ -55,19 +58,19 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.IdentityRef.Name is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "Secret", Name: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "Secret", Name: "foobarbaz", }, @@ -77,15 +80,15 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "OpenStackCluster.Spec.IdentityRef can be changed if it was unset", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "Secret", Name: "foobar", }, @@ -95,17 +98,17 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "OpenStackCluster.Spec.IdentityRef must not be removed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "Secret", Name: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", }, }, @@ -113,11 +116,11 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.Bastion is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - Bastion: &Bastion{ - Instance: OpenStackMachineSpec{ + Bastion: &infrav1.Bastion{ + Instance: infrav1.OpenStackMachineSpec{ CloudName: "foobar", Image: "foobar", Flavor: "minimal", @@ -125,17 +128,17 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { Enabled: true, }, }, - Status: OpenStackClusterStatus{ - Bastion: &BastionStatus{ + Status: infrav1.OpenStackClusterStatus{ + Bastion: &infrav1.BastionStatus{ Name: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - Bastion: &Bastion{ - Instance: OpenStackMachineSpec{ + Bastion: &infrav1.Bastion{ + Instance: infrav1.OpenStackMachineSpec{ CloudName: "foobarbaz", Image: "foobarbaz", Flavor: "medium", @@ -148,10 +151,10 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing CIDRs on the OpenStackCluster.Spec.APIServerLoadBalancer.AllowedCIDRs is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - APIServerLoadBalancer: APIServerLoadBalancer{ + APIServerLoadBalancer: infrav1.APIServerLoadBalancer{ Enabled: true, AllowedCIDRs: []string{ "0.0.0.0/0", @@ -160,10 +163,10 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - APIServerLoadBalancer: APIServerLoadBalancer{ + APIServerLoadBalancer: infrav1.APIServerLoadBalancer{ Enabled: true, AllowedCIDRs: []string{ "0.0.0.0/0", @@ -177,13 +180,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Adding OpenStackCluster.Spec.ControlPlaneAvailabilityZones is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", ControlPlaneAvailabilityZones: []string{ "alice", @@ -195,8 +198,8 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Modifying OpenStackCluster.Spec.ControlPlaneAvailabilityZones is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", ControlPlaneAvailabilityZones: []string{ "alice", @@ -204,8 +207,8 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", ControlPlaneAvailabilityZones: []string{ "alice", @@ -218,8 +221,8 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Removing OpenStackCluster.Spec.ControlPlaneAvailabilityZones is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", ControlPlaneAvailabilityZones: []string{ "alice", @@ -227,8 +230,8 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", }, }, @@ -236,13 +239,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerFixedIP is allowed when API Server Floating IP is disabled", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: true, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: true, APIServerFixedIP: "20.1.56.1", }, @@ -251,13 +254,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerFixedIP is not allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: false, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: false, APIServerFixedIP: "20.1.56.1", }, @@ -267,13 +270,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { { name: "Changing OpenStackCluster.Spec.APIServerPort is allowed when API Server Floating IP is disabled", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: true, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: true, APIServerPort: 8443, }, @@ -282,13 +285,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerPort is not allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: false, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: false, APIServerPort: 8443, }, @@ -297,22 +300,22 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerFloatingIP is allowed when it matches the current api server loadbalancer IP", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ APIServerFloatingIP: "", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ APIServerFloatingIP: "1.2.3.4", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, @@ -321,22 +324,22 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerFloatingIP is not allowed when it doesn't matches the current api server loadbalancer IP", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ APIServerFloatingIP: "", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ APIServerFloatingIP: "5.6.7.8", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, @@ -346,7 +349,9 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - warn, err := tt.newTemplate.ValidateUpdate(tt.oldTemplate) + ctx := context.TODO() + webhook := &openStackClusterWebhook{} + warn, err := webhook.ValidateUpdate(ctx, tt.oldTemplate, tt.newTemplate) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { @@ -363,15 +368,15 @@ func TestOpenStackCluster_ValidateCreate(t *testing.T) { tests := []struct { name string - template *OpenStackCluster + template *infrav1.OpenStackCluster wantErr bool }{ { name: "OpenStackCluster.Spec.IdentityRef with correct spec on create", - template: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + template: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "Secret", Name: "foobar", }, @@ -381,10 +386,10 @@ func TestOpenStackCluster_ValidateCreate(t *testing.T) { }, { name: "OpenStackCluster.Spec.IdentityRef with faulty spec on create", - template: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + template: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ CloudName: "foobar", - IdentityRef: &OpenStackIdentityReference{ + IdentityRef: &infrav1.OpenStackIdentityReference{ Kind: "foobar", Name: "foobar", }, @@ -395,7 +400,9 @@ func TestOpenStackCluster_ValidateCreate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - warn, err := tt.template.ValidateCreate() + ctx := context.TODO() + webhook := &openStackClusterWebhook{} + warn, err := webhook.ValidateCreate(ctx, tt.template) if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { diff --git a/pkg/webhooks/openstackclustertemplate_webhook.go b/pkg/webhooks/openstackclustertemplate_webhook.go new file mode 100644 index 0000000000..fb059f2f58 --- /dev/null +++ b/pkg/webhooks/openstackclustertemplate_webhook.go @@ -0,0 +1,116 @@ +/* +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 + +import ( + "context" + "fmt" + "reflect" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" +) + +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackclustertemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclustertemplates,versions=v1alpha7,name=validation.openstackclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 + +func SetupOpenStackClusterTemplateWebhook(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(&infrav1.OpenStackClusterTemplate{}). + WithValidator(&openStackClusterTemplateWebhook{}). + Complete() +} + +type openStackClusterTemplateWebhook struct{} + +// Compile-time assertion that openStackClusterTemplateWebhook implements webhook.CustomValidator. +var ( + _ webhook.CustomValidator = &openStackClusterTemplateWebhook{} + _ webhook.CustomDefaulter = &openStackClusterTemplateWebhook{} +) + +func (*openStackClusterTemplateWebhook) Default(_ context.Context, objRaw runtime.Object) error { + var allErrs field.ErrorList + newObj, err := castToOpenStackClusterTemplate(objRaw) + if err != nil { + return err + } + + if newObj.Spec.Template.Spec.IdentityRef != nil && newObj.Spec.Template.Spec.IdentityRef.Kind != defaultIdentityRefKind { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "template", "spec", "identityRef", "kind"), "must be a Secret")) + } + + if len(allErrs) == 0 { + return nil + } + + return apierrors.NewInvalid(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) +} + +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackClusterTemplateWebhook) ValidateCreate(_ context.Context, objRaw runtime.Object) (admission.Warnings, error) { + var allErrs field.ErrorList + newObj, err := castToOpenStackClusterTemplate(objRaw) + if err != nil { + return nil, err + } + + if newObj.Spec.Template.Spec.IdentityRef != nil && newObj.Spec.Template.Spec.IdentityRef.Kind != defaultIdentityRefKind { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "template", "spec", "identityRef", "kind"), "must be a Secret")) + } + + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) +} + +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackClusterTemplateWebhook) ValidateUpdate(_ context.Context, oldObjRaw, newObjRaw runtime.Object) (admission.Warnings, error) { + var allErrs field.ErrorList + oldObj, err := castToOpenStackClusterTemplate(oldObjRaw) + if err != nil { + return nil, err + } + newObj, err := castToOpenStackClusterTemplate(newObjRaw) + if err != nil { + return nil, err + } + + if !reflect.DeepEqual(newObj.Spec.Template.Spec, oldObj.Spec.Template.Spec) { + allErrs = append(allErrs, + field.Invalid(field.NewPath("OpenStackClusterTemplate", "spec", "template", "spec"), newObj, "OpenStackClusterTemplate spec.template.spec field is immutable. Please create new resource instead."), + ) + } + + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) +} + +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackClusterTemplateWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { + return nil, nil +} + +func castToOpenStackClusterTemplate(obj runtime.Object) (*infrav1.OpenStackClusterTemplate, error) { + cast, ok := obj.(*infrav1.OpenStackClusterTemplate) + if !ok { + return nil, fmt.Errorf("expected an OpenStackClusterTemplate but got a %T", obj) + } + return cast, nil +} diff --git a/api/v1alpha7/openstackmachine_webhook.go b/pkg/webhooks/openstackmachine_webhook.go similarity index 52% rename from api/v1alpha7/openstackmachine_webhook.go rename to pkg/webhooks/openstackmachine_webhook.go index 6470589a76..5a1ee496b3 100644 --- a/api/v1alpha7/openstackmachine_webhook.go +++ b/pkg/webhooks/openstackmachine_webhook.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha7 +package webhooks import ( + "context" "fmt" "reflect" @@ -24,73 +25,90 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/builder" - logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" ) -// log is for logging in this package. -var _ = logf.Log.WithName("openstackmachine-resource") +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackmachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,versions=v1alpha7,name=validation.openstackmachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -func (r *OpenStackMachine) SetupWebhookWithManager(mgr manager.Manager) error { +func SetupOpenStackMachineWebhook(mgr manager.Manager) error { return builder.WebhookManagedBy(mgr). - For(r). + For(&infrav1.OpenStackMachine{}). + WithValidator(&openStackMachineWebhook{}). Complete() } -// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackmachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,versions=v1alpha7,name=validation.openstackmachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackmachine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,versions=v1alpha7,name=default.openstackmachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 +type openStackMachineWebhook struct{} +// Compile-time assertion that openStackMachineWebhook implements webhook.CustomValidator. var ( - _ webhook.Defaulter = &OpenStackMachine{} - _ webhook.Validator = &OpenStackMachine{} + _ webhook.CustomValidator = &openStackMachineWebhook{} + _ webhook.CustomDefaulter = &openStackMachineWebhook{} ) -// Default satisfies the defaulting webhook interface. -func (r *OpenStackMachine) Default() { - if r.Spec.IdentityRef != nil && r.Spec.IdentityRef.Kind == "" { - r.Spec.IdentityRef.Kind = defaultIdentityRefKind +// Default implements webhook.CustomDefaulter. +func (*openStackMachineWebhook) Default(_ context.Context, objRaw runtime.Object) error { + newObj, err := castToOpenStackMachine(objRaw) + if err != nil { + return err + } + + if newObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef.Kind != defaultIdentityRefKind { + newObj.Spec.IdentityRef.Kind = defaultIdentityRefKind } + + return nil } -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackMachine) ValidateCreate() (admission.Warnings, error) { +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackMachineWebhook) ValidateCreate(_ context.Context, objRaw runtime.Object) (admission.Warnings, error) { var allErrs field.ErrorList + newObj, err := castToOpenStackMachine(objRaw) + if err != nil { + return nil, err + } - if r.Spec.IdentityRef != nil && r.Spec.IdentityRef.Kind != defaultIdentityRefKind { + if newObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef.Kind != defaultIdentityRefKind { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "identityRef", "kind"), "must be a Secret")) } - if r.Spec.RootVolume != nil && r.Spec.AdditionalBlockDevices != nil { - for _, device := range r.Spec.AdditionalBlockDevices { + if newObj.Spec.RootVolume != nil && newObj.Spec.AdditionalBlockDevices != nil { + for _, device := range newObj.Spec.AdditionalBlockDevices { if device.Name == "root" { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "additionalBlockDevices"), "cannot contain a device named \"root\" when rootVolume is set")) } } } - return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) } -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackMachine) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - newOpenStackMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(r) +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackMachineWebhook) ValidateUpdate(_ context.Context, oldObjRaw, newObjRaw runtime.Object) (admission.Warnings, error) { + newObj, err := castToOpenStackMachine(newObjRaw) + if err != nil { + return nil, err + } + + newOpenStackMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(newObj) if err != nil { - return nil, apierrors.NewInvalid(GroupVersion.WithKind("OpenStackMachine").GroupKind(), r.Name, field.ErrorList{ + return nil, apierrors.NewInvalid(infrav1.GroupVersion.WithKind("OpenStackMachine").GroupKind(), newObj.Name, field.ErrorList{ field.InternalError(nil, fmt.Errorf("failed to convert new OpenStackMachine to unstructured object: %w", err)), }) } - oldOpenStackMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(old) + oldOpenStackMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(oldObjRaw) if err != nil { - return nil, apierrors.NewInvalid(GroupVersion.WithKind("OpenStackMachine").GroupKind(), r.Name, field.ErrorList{ + return nil, apierrors.NewInvalid(infrav1.GroupVersion.WithKind("OpenStackMachine").GroupKind(), newObj.Name, field.ErrorList{ field.InternalError(nil, fmt.Errorf("failed to convert old OpenStackMachine to unstructured object: %w", err)), }) } var allErrs field.ErrorList - if r.Spec.IdentityRef != nil && r.Spec.IdentityRef.Kind != defaultIdentityRefKind { + if newObj.Spec.IdentityRef != nil && newObj.Spec.IdentityRef.Kind != defaultIdentityRefKind { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "identityRef", "kind"), "must be a Secret")) } @@ -113,10 +131,18 @@ func (r *OpenStackMachine) ValidateUpdate(old runtime.Object) (admission.Warning allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified")) } - return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) } -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackMachine) ValidateDelete() (admission.Warnings, error) { +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type. +func (*openStackMachineWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { return nil, nil } + +func castToOpenStackMachine(obj runtime.Object) (*infrav1.OpenStackMachine, error) { + cast, ok := obj.(*infrav1.OpenStackMachine) + if !ok { + return nil, fmt.Errorf("expected an OpenStackMachine but got a %T", obj) + } + return cast, nil +} diff --git a/api/v1alpha7/openstackmachinetemplate_webhook.go b/pkg/webhooks/openstackmachinetemplate_webhook.go similarity index 55% rename from api/v1alpha7/openstackmachinetemplate_webhook.go rename to pkg/webhooks/openstackmachinetemplate_webhook.go index b74d056ba2..a96d1a2938 100644 --- a/api/v1alpha7/openstackmachinetemplate_webhook.go +++ b/pkg/webhooks/openstackmachinetemplate_webhook.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha7 +package webhooks import ( "context" @@ -29,52 +29,51 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) -// OpenStackMachineTemplateImmutableMsg ... -const OpenStackMachineTemplateImmutableMsg = "OpenStackMachineTemplate spec.template.spec field is immutable. Please create a new resource instead. Ref doc: https://cluster-api.sigs.k8s.io/tasks/change-machine-template.html" + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" +) -// +kubebuilder:object:generate=false -type OpenStackMachineTemplateWebhook struct{} +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackmachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates,versions=v1alpha7,name=validation.openstackmachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -func (r *OpenStackMachineTemplateWebhook) SetupWebhookWithManager(mgr manager.Manager) error { +func SetupOpenStackMachineTemplateWebhook(mgr manager.Manager) error { return builder.WebhookManagedBy(mgr). - For(&OpenStackMachineTemplate{}). - WithValidator(r). + For(&infrav1.OpenStackMachineTemplate{}). + WithValidator(&openStackMachineTemplateWebhook{}). Complete() } -// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha7-openstackmachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates,versions=v1alpha7,name=validation.openstackmachinetemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 +type openStackMachineTemplateWebhook struct{} -var _ webhook.CustomValidator = &OpenStackMachineTemplateWebhook{} +// Compile-time assertion that openStackMachineTemplateWebhook implements webhook.CustomValidator. +var _ webhook.CustomValidator = &openStackMachineTemplateWebhook{} // ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type. -func (r *OpenStackMachineTemplateWebhook) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { - openStackMachineTemplate, ok := obj.(*OpenStackMachineTemplate) - if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an OpenStackMachineTemplate but got a %T", obj)) +func (*openStackMachineTemplateWebhook) ValidateCreate(_ context.Context, objRaw runtime.Object) (admission.Warnings, error) { + newObj, err := castToOpenStackMachineTemplate(objRaw) + if err != nil { + return nil, err } var allErrs field.ErrorList - if openStackMachineTemplate.Spec.Template.Spec.ProviderID != nil { + if newObj.Spec.Template.Spec.ProviderID != nil { allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "template", "spec", "providerID"), "cannot be set in templates")) } - return aggregateObjErrors(openStackMachineTemplate.GroupVersionKind().GroupKind(), openStackMachineTemplate.Name, allErrs) + return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs) } // ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type. -func (r *OpenStackMachineTemplateWebhook) ValidateUpdate(ctx context.Context, oldRaw runtime.Object, newRaw runtime.Object) (admission.Warnings, error) { +func (*openStackMachineTemplateWebhook) ValidateUpdate(ctx context.Context, oldObjRaw, newObjRaw runtime.Object) (admission.Warnings, error) { var allErrs field.ErrorList - old, ok := oldRaw.(*OpenStackMachineTemplate) - if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an OpenStackMachineTemplate but got a %T", oldRaw)) + oldObj, err := castToOpenStackMachineTemplate(oldObjRaw) + if err != nil { + return nil, err } - newObj, ok := newRaw.(*OpenStackMachineTemplate) - if !ok { - return nil, apierrors.NewBadRequest(fmt.Sprintf("expected an OpenStackMachineTemplate but got a %T", oldRaw)) + newObj, err := castToOpenStackMachineTemplate(newObjRaw) + if err != nil { + return nil, err } req, err := admission.RequestFromContext(ctx) @@ -83,9 +82,9 @@ func (r *OpenStackMachineTemplateWebhook) ValidateUpdate(ctx context.Context, ol } if !topology.ShouldSkipImmutabilityChecks(req, newObj) && - !reflect.DeepEqual(newObj.Spec.Template.Spec, old.Spec.Template.Spec) { + !reflect.DeepEqual(newObj.Spec.Template.Spec, oldObj.Spec.Template.Spec) { allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "template", "spec"), r, OpenStackMachineTemplateImmutableMsg), + field.Invalid(field.NewPath("spec", "template", "spec"), newObj.Spec.Template.Spec, "OpenStackMachineTemplate spec.template.spec field is immutable. Please create a new resource instead. Ref doc: https://cluster-api.sigs.k8s.io/tasks/change-machine-template.html"), ) } @@ -93,6 +92,14 @@ func (r *OpenStackMachineTemplateWebhook) ValidateUpdate(ctx context.Context, ol } // ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type. -func (r *OpenStackMachineTemplateWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { +func (*openStackMachineTemplateWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { return nil, nil } + +func castToOpenStackMachineTemplate(obj runtime.Object) (*infrav1.OpenStackMachineTemplate, error) { + cast, ok := obj.(*infrav1.OpenStackMachineTemplate) + if !ok { + return nil, fmt.Errorf("expected an OpenStackMachineTemplate but got a %T", obj) + } + return cast, nil +} diff --git a/api/v1alpha7/openstackmachinetemplate_webhook_test.go b/pkg/webhooks/openstackmachinetemplate_webhook_test.go similarity index 61% rename from api/v1alpha7/openstackmachinetemplate_webhook_test.go rename to pkg/webhooks/openstackmachinetemplate_webhook_test.go index d9a53adc17..891ad58f13 100644 --- a/api/v1alpha7/openstackmachinetemplate_webhook_test.go +++ b/pkg/webhooks/openstackmachinetemplate_webhook_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha7 +package webhooks import ( "context" @@ -26,6 +26,8 @@ import ( "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" ) func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { @@ -33,27 +35,27 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { tests := []struct { name string - oldTemplate *OpenStackMachineTemplate - newTemplate *OpenStackMachineTemplate + oldTemplate *infrav1.OpenStackMachineTemplate + newTemplate *infrav1.OpenStackMachineTemplate req *admission.Request wantErr bool }{ { name: "OpenStackMachineTemplate with immutable spec", - oldTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + oldTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "bar", }, }, }, }, - newTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + newTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "NewImage", }, @@ -65,10 +67,10 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { }, { name: "OpenStackMachineTemplate with mutable metadata", - oldTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + oldTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "bar", }, @@ -78,10 +80,10 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { Name: "foo", }, }, - newTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + newTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "bar", }, @@ -95,20 +97,20 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { }, { name: "don't allow modification, dry run, no skip immutability annotation set", - oldTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + oldTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "bar", }, }, }, }, - newTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + newTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "NewImage", }, @@ -120,25 +122,25 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { }, { name: "allow modification, dry run, skip immutability annotation set", - oldTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + oldTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "bar", }, }, }, }, - newTemplate: &OpenStackMachineTemplate{ + newTemplate: &infrav1.OpenStackMachineTemplate{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ clusterv1.TopologyDryRunAnnotation: "", }, }, - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", Image: "NewImage", }, @@ -154,7 +156,7 @@ func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - webhook := &OpenStackMachineTemplateWebhook{} + webhook := &openStackMachineTemplateWebhook{} ctx := admission.NewContextWithRequest(context.Background(), *tt.req) warn, err := webhook.ValidateUpdate(ctx, tt.oldTemplate, tt.newTemplate) diff --git a/pkg/webhooks/register.go b/pkg/webhooks/register.go new file mode 100644 index 0000000000..6eae031ff0 --- /dev/null +++ b/pkg/webhooks/register.go @@ -0,0 +1,63 @@ +/* +Copyright 2024 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 ( + "fmt" + + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/conversion" + "sigs.k8s.io/controller-runtime/pkg/manager" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" +) + +func RegisterAllWithManager(mgr manager.Manager) []error { + var errs []error + + // Register webhooks for all types with custom validators. + for _, webhook := range []struct { + name string + setup func(ctrl.Manager) error + }{ + {"OpenStackCluster", SetupOpenStackClusterWebhook}, + {"OpenStackClusterTemplate", SetupOpenStackClusterTemplateWebhook}, + {"OpenStackMachine", SetupOpenStackMachineWebhook}, + {"OpenStackMachineTemplate", SetupOpenStackMachineTemplateWebhook}, + } { + if err := webhook.setup(mgr); err != nil { + errs = append(errs, fmt.Errorf("creating webhook for %s: %v", webhook.name, err)) + } + } + + // Additionally register webhooks for other types so they get conversion webhooks. + for _, conversionOnlyType := range []conversion.Hub{ + &infrav1.OpenStackClusterList{}, + &infrav1.OpenStackClusterTemplateList{}, + &infrav1.OpenStackMachineList{}, + &infrav1.OpenStackMachineTemplateList{}, + } { + if err := builder.WebhookManagedBy(mgr). + For(conversionOnlyType). + Complete(); err != nil { + errs = append(errs, fmt.Errorf("creating webhook for %T: %v", conversionOnlyType, err)) + } + } + + return errs +} From 614bc67df34a1ee8ebf6632450ff6c1ab2f0a132 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Sat, 2 Mar 2024 00:24:41 +0000 Subject: [PATCH 5/5] Implement convertible for OpenStackClusterTemplateList The conversion webhook fails to register without this. Conflicts: api/v1alpha7/conversion.go NOTE(stephenfin): Conflicts are due to absence of v1beta1 API on this branch. We also need to `s/v1beta1/v1alpha7/` on the other files to handle this. (cherry picked from commit cb09d5fe452bd6c2b24dfa1b10c35b8c65b92164) --- api/v1alpha5/conversion.go | 12 ++++++++++++ api/v1alpha6/conversion.go | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/api/v1alpha5/conversion.go b/api/v1alpha5/conversion.go index 3f41962b92..9692182017 100644 --- a/api/v1alpha5/conversion.go +++ b/api/v1alpha5/conversion.go @@ -100,6 +100,18 @@ func (r *OpenStackClusterTemplate) ConvertFrom(srcRaw ctrlconversion.Hub) error return utilconversion.MarshalData(src, r) } +var _ ctrlconversion.Convertible = &OpenStackClusterTemplateList{} + +func (r *OpenStackClusterTemplateList) ConvertTo(dstRaw ctrlconversion.Hub) error { + dst := dstRaw.(*infrav1.OpenStackClusterTemplateList) + return Convert_v1alpha5_OpenStackClusterTemplateList_To_v1alpha7_OpenStackClusterTemplateList(r, dst, nil) +} + +func (r *OpenStackClusterTemplateList) ConvertFrom(srcRaw ctrlconversion.Hub) error { + src := srcRaw.(*infrav1.OpenStackClusterTemplateList) + return Convert_v1alpha7_OpenStackClusterTemplateList_To_v1alpha5_OpenStackClusterTemplateList(src, r, nil) +} + var _ ctrlconversion.Convertible = &OpenStackMachine{} func (r *OpenStackMachine) ConvertTo(dstRaw ctrlconversion.Hub) error { diff --git a/api/v1alpha6/conversion.go b/api/v1alpha6/conversion.go index ae8e343ea8..03aa3bb64e 100644 --- a/api/v1alpha6/conversion.go +++ b/api/v1alpha6/conversion.go @@ -230,6 +230,18 @@ func (r *OpenStackClusterTemplate) ConvertFrom(srcRaw ctrlconversion.Hub) error ) } +var _ ctrlconversion.Convertible = &OpenStackClusterTemplateList{} + +func (r *OpenStackClusterTemplateList) ConvertTo(dstRaw ctrlconversion.Hub) error { + dst := dstRaw.(*infrav1.OpenStackClusterTemplateList) + return Convert_v1alpha6_OpenStackClusterTemplateList_To_v1alpha7_OpenStackClusterTemplateList(r, dst, nil) +} + +func (r *OpenStackClusterTemplateList) ConvertFrom(srcRaw ctrlconversion.Hub) error { + src := srcRaw.(*infrav1.OpenStackClusterTemplateList) + return Convert_v1alpha7_OpenStackClusterTemplateList_To_v1alpha6_OpenStackClusterTemplateList(src, r, nil) +} + var _ ctrlconversion.Convertible = &OpenStackMachine{} var v1alpha6OpenStackMachineRestorer = conversion.RestorerFor[*OpenStackMachine]{