From a9e710e78ffd3ff51bbd90eb18319e2cc025acc7 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 1 Mar 2024 18:14:07 +0000 Subject: [PATCH] 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. --- Makefile | 2 +- api/v1beta1/openstackcluster_webhook.go | 156 --------- api/v1beta1/openstackclusterlist_webhook.go | 32 -- .../openstackclustertemplate_webhook.go | 78 ----- api/v1beta1/openstackmachinelist_webhook.go | 32 -- .../openstackmachinetemplatelist_webhook.go | 32 -- config/webhook/manifests.yaml | 69 ---- docs/book/src/api/v1beta1/api.md | 4 - main.go | 32 +- .../webhooks.go => pkg/webhooks/errors.go | 2 +- pkg/webhooks/openstackcluster_webhook.go | 167 ++++++++++ .../openstackcluster_webhook_test.go | 309 +++++++++--------- .../openstackclustertemplate_webhook.go | 90 +++++ .../webhooks}/openstackmachine_webhook.go | 72 ++-- .../openstackmachinetemplate_webhook.go | 61 ++-- .../openstackmachinetemplate_webhook_test.go | 90 ++--- pkg/webhooks/register.go | 63 ++++ test/e2e/suites/apivalidations/suite_test.go | 10 +- 18 files changed, 610 insertions(+), 691 deletions(-) delete mode 100644 api/v1beta1/openstackcluster_webhook.go delete mode 100644 api/v1beta1/openstackclusterlist_webhook.go delete mode 100644 api/v1beta1/openstackclustertemplate_webhook.go delete mode 100644 api/v1beta1/openstackmachinelist_webhook.go delete mode 100644 api/v1beta1/openstackmachinetemplatelist_webhook.go rename api/v1beta1/webhooks.go => pkg/webhooks/errors.go (98%) create mode 100644 pkg/webhooks/openstackcluster_webhook.go rename {api/v1beta1 => pkg/webhooks}/openstackcluster_webhook_test.go (56%) create mode 100644 pkg/webhooks/openstackclustertemplate_webhook.go rename {api/v1beta1 => pkg/webhooks}/openstackmachine_webhook.go (57%) rename {api/v1beta1 => pkg/webhooks}/openstackmachinetemplate_webhook.go (55%) rename {api/v1beta1 => pkg/webhooks}/openstackmachinetemplate_webhook_test.go (55%) create mode 100644 pkg/webhooks/register.go diff --git a/Makefile b/Makefile index 4eb9f5d757..7a0b864d05 100644 --- a/Makefile +++ b/Makefile @@ -274,7 +274,7 @@ generate-conversion-gen: $(CONVERSION_GEN) .PHONY: generate-manifests generate-manifests: $(CONTROLLER_GEN) ## Generate manifests e.g. CRD, RBAC etc. $(CONTROLLER_GEN) \ - paths=./api/... \ + paths=./pkg/webhooks/... \ crd:crdVersions=v1 \ output:crd:dir=$(CRD_ROOT) \ output:webhook:dir=$(WEBHOOK_ROOT) \ diff --git a/api/v1beta1/openstackcluster_webhook.go b/api/v1beta1/openstackcluster_webhook.go deleted file mode 100644 index 97781ea2c6..0000000000 --- a/api/v1beta1/openstackcluster_webhook.go +++ /dev/null @@ -1,156 +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 v1beta1 - -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-v1beta1-openstackcluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclusters,versions=v1beta1,name=validation.openstackcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-openstackcluster,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclusters,versions=v1beta1,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() { -} - -// 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.ManagedSecurityGroups != nil { - for _, rule := range r.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules { - if rule.RemoteManagedGroups != nil && (rule.RemoteGroupID != nil || rule.RemoteIPPrefix != nil) { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSecurityGroups", "allNodesSecurityGroupRules"), "remoteManagedGroups cannot be used with remoteGroupID or remoteIPPrefix")) - } - if rule.RemoteGroupID != nil && (rule.RemoteManagedGroups != nil || rule.RemoteIPPrefix != nil) { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSecurityGroups", "allNodesSecurityGroupRules"), "remoteGroupID cannot be used with remoteManagedGroups or remoteIPPrefix")) - } - if rule.RemoteIPPrefix != nil && (rule.RemoteManagedGroups != nil || rule.RemoteGroupID != nil) { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSecurityGroups", "allNodesSecurityGroupRules"), "remoteIPPrefix cannot be used with remoteManagedGroups or remoteGroupID")) - } - } - } - - 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)) - } - - // Allow changes to Spec.IdentityRef - old.Spec.IdentityRef = OpenStackIdentityReference{} - r.Spec.IdentityRef = OpenStackIdentityReference{} - - // 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 to remove the bastion spec only if it was disabled before. - if r.Spec.Bastion == nil { - if old.Spec.Bastion != nil && old.Spec.Bastion.Enabled { - allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "bastion"), "cannot be removed before disabling it")) - } - } - - // Allow changes to the bastion spec. - old.Spec.Bastion = &Bastion{} - r.Spec.Bastion = &Bastion{} - - // Allow changes to the managed allNodesSecurityGroupRules. - if r.Spec.ManagedSecurityGroups != nil { - old.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules = []SecurityGroupRuleSpec{} - r.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules = []SecurityGroupRuleSpec{} - - // Allow change to the allowAllInClusterTraffic. - old.Spec.ManagedSecurityGroups.AllowAllInClusterTraffic = false - r.Spec.ManagedSecurityGroups.AllowAllInClusterTraffic = false - } - - // 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 the scheduling to be changed from CAPI managed to Nova and - // vice versa. - old.Spec.ControlPlaneOmitAvailabilityZone = false - r.Spec.ControlPlaneOmitAvailabilityZone = 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/v1beta1/openstackclusterlist_webhook.go b/api/v1beta1/openstackclusterlist_webhook.go deleted file mode 100644 index c765ad7cb8..0000000000 --- a/api/v1beta1/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 v1beta1 - -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/v1beta1/openstackclustertemplate_webhook.go b/api/v1beta1/openstackclustertemplate_webhook.go deleted file mode 100644 index 700dac06e2..0000000000 --- a/api/v1beta1/openstackclustertemplate_webhook.go +++ /dev/null @@ -1,78 +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 v1beta1 - -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-v1beta1-openstackclustertemplate,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclustertemplates,versions=v1beta1,name=default.openstackclustertemplate.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-openstackclustertemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclustertemplates,versions=v1beta1,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() { -} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenStackClusterTemplate) ValidateCreate() (admission.Warnings, error) { - var allErrs field.ErrorList - - 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/v1beta1/openstackmachinelist_webhook.go b/api/v1beta1/openstackmachinelist_webhook.go deleted file mode 100644 index 9e6a551f2d..0000000000 --- a/api/v1beta1/openstackmachinelist_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 v1beta1 - -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() -} diff --git a/api/v1beta1/openstackmachinetemplatelist_webhook.go b/api/v1beta1/openstackmachinetemplatelist_webhook.go deleted file mode 100644 index 03d438e925..0000000000 --- a/api/v1beta1/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 v1beta1 - -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/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 260778c5cf..032b3756be 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-v1beta1-openstackcluster - failurePolicy: Fail - matchPolicy: Equivalent - name: default.openstackcluster.infrastructure.cluster.x-k8s.io - rules: - - apiGroups: - - infrastructure.cluster.x-k8s.io - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - openstackclusters - sideEffects: None -- admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-infrastructure-cluster-x-k8s-io-v1beta1-openstackclustertemplate - failurePolicy: Fail - matchPolicy: Equivalent - name: default.openstackclustertemplate.infrastructure.cluster.x-k8s.io - rules: - - apiGroups: - - infrastructure.cluster.x-k8s.io - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - openstackclustertemplates - sideEffects: None -- admissionReviewVersions: - - v1beta1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-infrastructure-cluster-x-k8s-io-v1beta1-openstackmachine - failurePolicy: Fail - matchPolicy: Equivalent - name: default.openstackmachine.infrastructure.cluster.x-k8s.io - rules: - - apiGroups: - - infrastructure.cluster.x-k8s.io - apiVersions: - - v1beta1 - operations: - - CREATE - - UPDATE - resources: - - openstackmachines - sideEffects: None ---- -apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration diff --git a/docs/book/src/api/v1beta1/api.md b/docs/book/src/api/v1beta1/api.md index 9dc7667830..a0f4caf74b 100644 --- a/docs/book/src/api/v1beta1/api.md +++ b/docs/book/src/api/v1beta1/api.md @@ -3477,10 +3477,6 @@ OpenStackMachineTemplateResource -

OpenStackMachineTemplateWebhook -

-

-

PortOpts

diff --git a/main.go b/main.go index 9fa9fc5eb0..c170553eeb 100644 --- a/main.go +++ b/main.go @@ -54,6 +54,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" ) @@ -353,32 +354,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/v1beta1/webhooks.go b/pkg/webhooks/errors.go similarity index 98% rename from api/v1beta1/webhooks.go rename to pkg/webhooks/errors.go index 3df43589ba..ad2c53aae4 100644 --- a/api/v1beta1/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 v1beta1 +package webhooks import ( apierrors "k8s.io/apimachinery/pkg/api/errors" diff --git a/pkg/webhooks/openstackcluster_webhook.go b/pkg/webhooks/openstackcluster_webhook.go new file mode 100644 index 0000000000..6541e93681 --- /dev/null +++ b/pkg/webhooks/openstackcluster_webhook.go @@ -0,0 +1,167 @@ +/* +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/v1beta1" +) + +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-openstackcluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclusters,versions=v1beta1,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.ManagedSecurityGroups != nil { + for _, rule := range newObj.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules { + if rule.RemoteManagedGroups != nil && (rule.RemoteGroupID != nil || rule.RemoteIPPrefix != nil) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSecurityGroups", "allNodesSecurityGroupRules"), "remoteManagedGroups cannot be used with remoteGroupID or remoteIPPrefix")) + } + if rule.RemoteGroupID != nil && (rule.RemoteManagedGroups != nil || rule.RemoteIPPrefix != nil) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSecurityGroups", "allNodesSecurityGroupRules"), "remoteGroupID cannot be used with remoteManagedGroups or remoteIPPrefix")) + } + if rule.RemoteIPPrefix != nil && (rule.RemoteManagedGroups != nil || rule.RemoteGroupID != nil) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "managedSecurityGroups", "allNodesSecurityGroupRules"), "remoteIPPrefix cannot be used with remoteManagedGroups or remoteGroupID")) + } + } + } + + 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 + } + + // Allow changes to Spec.IdentityRef + oldObj.Spec.IdentityRef = infrav1.OpenStackIdentityReference{} + newObj.Spec.IdentityRef = infrav1.OpenStackIdentityReference{} + + // 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 to remove the bastion spec only if it was disabled before. + if newObj.Spec.Bastion == nil { + if oldObj.Spec.Bastion != nil && oldObj.Spec.Bastion.Enabled { + allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "bastion"), "cannot be removed before disabling it")) + } + } + + // Allow changes to the bastion spec. + oldObj.Spec.Bastion = &infrav1.Bastion{} + newObj.Spec.Bastion = &infrav1.Bastion{} + + // Allow changes to the managed allNodesSecurityGroupRules. + if newObj.Spec.ManagedSecurityGroups != nil { + oldObj.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules = []infrav1.SecurityGroupRuleSpec{} + newObj.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules = []infrav1.SecurityGroupRuleSpec{} + + // Allow change to the allowAllInClusterTraffic. + oldObj.Spec.ManagedSecurityGroups.AllowAllInClusterTraffic = false + newObj.Spec.ManagedSecurityGroups.AllowAllInClusterTraffic = false + } + + // 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 the scheduling to be changed from CAPI managed to Nova and + // vice versa. + oldObj.Spec.ControlPlaneOmitAvailabilityZone = false + newObj.Spec.ControlPlaneOmitAvailabilityZone = 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/v1beta1/openstackcluster_webhook_test.go b/pkg/webhooks/openstackcluster_webhook_test.go similarity index 56% rename from api/v1beta1/openstackcluster_webhook_test.go rename to pkg/webhooks/openstackcluster_webhook_test.go index 4a0213dd90..5a4c44269c 100644 --- a/api/v1beta1/openstackcluster_webhook_test.go +++ b/pkg/webhooks/openstackcluster_webhook_test.go @@ -14,13 +14,16 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1beta1 +package webhooks import ( + "context" "testing" . "github.com/onsi/gomega" "k8s.io/utils/pointer" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" ) func TestOpenStackCluster_ValidateUpdate(t *testing.T) { @@ -28,23 +31,23 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { tests := []struct { name string - oldTemplate *OpenStackCluster - newTemplate *OpenStackCluster + oldTemplate *infrav1.OpenStackCluster + newTemplate *infrav1.OpenStackCluster wantErr bool }{ { name: "Changing OpenStackCluster.Spec.IdentityRef.Name is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobarbaz", CloudName: "foobar", }, @@ -54,17 +57,17 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.IdentityRef.CloudName is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobarbaz", }, @@ -74,35 +77,35 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.Bastion is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - Bastion: &Bastion{ - Instance: OpenStackMachineSpec{ - Image: ImageFilter{Name: "foobar"}, + Bastion: &infrav1.Bastion{ + Instance: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{Name: "foobar"}, Flavor: "minimal", }, Enabled: true, }, }, - Status: OpenStackClusterStatus{ - Bastion: &BastionStatus{ + Status: infrav1.OpenStackClusterStatus{ + Bastion: &infrav1.BastionStatus{ Name: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - Bastion: &Bastion{ - Instance: OpenStackMachineSpec{ - Image: ImageFilter{Name: "foobarbaz"}, + Bastion: &infrav1.Bastion{ + Instance: infrav1.OpenStackMachineSpec{ + Image: infrav1.ImageFilter{Name: "foobarbaz"}, Flavor: "medium", }, Enabled: true, @@ -113,41 +116,41 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing security group rules on the OpenStackCluster.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - ManagedSecurityGroups: &ManagedSecurityGroups{ - AllNodesSecurityGroupRules: []SecurityGroupRuleSpec{ + ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{ + AllNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ { Name: "foobar", Description: pointer.String("foobar"), PortRangeMin: pointer.Int(80), PortRangeMax: pointer.Int(80), Protocol: pointer.String("tcp"), - RemoteManagedGroups: []ManagedSecurityGroupName{"controlplane"}, + RemoteManagedGroups: []infrav1.ManagedSecurityGroupName{"controlplane"}, }, }, }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - ManagedSecurityGroups: &ManagedSecurityGroups{ - AllNodesSecurityGroupRules: []SecurityGroupRuleSpec{ + ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{ + AllNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ { Name: "foobar", Description: pointer.String("foobar"), PortRangeMin: pointer.Int(80), PortRangeMax: pointer.Int(80), Protocol: pointer.String("tcp"), - RemoteManagedGroups: []ManagedSecurityGroupName{"controlplane", "worker"}, + RemoteManagedGroups: []infrav1.ManagedSecurityGroupName{"controlplane", "worker"}, }, }, }, @@ -157,13 +160,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing CIDRs on the OpenStackCluster.Spec.APIServerLoadBalancer.AllowedCIDRs is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - APIServerLoadBalancer: APIServerLoadBalancer{ + APIServerLoadBalancer: infrav1.APIServerLoadBalancer{ Enabled: true, AllowedCIDRs: []string{ "0.0.0.0/0", @@ -172,13 +175,13 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - APIServerLoadBalancer: APIServerLoadBalancer{ + APIServerLoadBalancer: infrav1.APIServerLoadBalancer{ Enabled: true, AllowedCIDRs: []string{ "0.0.0.0/0", @@ -192,17 +195,17 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Adding OpenStackCluster.Spec.ControlPlaneAvailabilityZones is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -216,9 +219,9 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Modifying OpenStackCluster.Spec.ControlPlaneAvailabilityZones is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -228,9 +231,9 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -245,9 +248,9 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Removing OpenStackCluster.Spec.ControlPlaneAvailabilityZones is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -257,9 +260,9 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -269,17 +272,17 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Modifying OpenstackCluster.Spec.ControlPlaneOmitAvailabilityZone is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -290,18 +293,18 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerFixedIP is allowed when API Server Floating IP is disabled", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, DisableAPIServerFloatingIP: true, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -313,18 +316,18 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerFixedIP is not allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, DisableAPIServerFloatingIP: false, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -337,17 +340,17 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { { name: "Changing OpenStackCluster.Spec.APIServerPort is allowed when API Server Floating IP is disabled", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, DisableAPIServerFloatingIP: true, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ DisableAPIServerFloatingIP: true, APIServerPort: 8443, }, @@ -356,18 +359,18 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Changing OpenStackCluster.Spec.APIServerPort is not allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, DisableAPIServerFloatingIP: false, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -379,30 +382,30 @@ 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{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, APIServerFloatingIP: "", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, APIServerFloatingIP: "1.2.3.4", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, @@ -411,30 +414,30 @@ 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{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, APIServerFloatingIP: "", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, APIServerFloatingIP: "5.6.7.8", }, - Status: OpenStackClusterStatus{ - APIServerLoadBalancer: &LoadBalancer{ + Status: infrav1.OpenStackClusterStatus{ + APIServerLoadBalancer: &infrav1.LoadBalancer{ IP: "1.2.3.4", }, }, @@ -443,24 +446,24 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Removing OpenStackCluster.Spec.Bastion when it is enabled is not allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - Bastion: &Bastion{ + Bastion: &infrav1.Bastion{ Enabled: true, - Instance: OpenStackMachineSpec{ + Instance: infrav1.OpenStackMachineSpec{ Flavor: "m1.small", - Image: ImageFilter{Name: "ubuntu"}, + Image: infrav1.ImageFilter{Name: "ubuntu"}, }, }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -470,24 +473,24 @@ func TestOpenStackCluster_ValidateUpdate(t *testing.T) { }, { name: "Removing OpenStackCluster.Spec.Bastion when it is disabled is allowed", - oldTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + oldTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - Bastion: &Bastion{ + Bastion: &infrav1.Bastion{ Enabled: false, - Instance: OpenStackMachineSpec{ + Instance: infrav1.OpenStackMachineSpec{ Flavor: "m1.small", - Image: ImageFilter{Name: "ubuntu"}, + Image: infrav1.ImageFilter{Name: "ubuntu"}, }, }, }, }, - newTemplate: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + newTemplate: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -498,7 +501,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 { @@ -515,14 +520,14 @@ 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{ - IdentityRef: OpenStackIdentityReference{ + template: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, @@ -532,14 +537,14 @@ func TestOpenStackCluster_ValidateCreate(t *testing.T) { }, { name: "OpenStackCluster.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules with correct spec on create", - template: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + template: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - ManagedSecurityGroups: &ManagedSecurityGroups{ - AllNodesSecurityGroupRules: []SecurityGroupRuleSpec{ + ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{ + AllNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ { Name: "foobar", Description: pointer.String("foobar"), @@ -555,21 +560,21 @@ func TestOpenStackCluster_ValidateCreate(t *testing.T) { }, { name: "OpenStackCluster.Spec.ManagedSecurityGroups.AllNodesSecurityGroupRules with mutually exclusive fields on create", - template: &OpenStackCluster{ - Spec: OpenStackClusterSpec{ - IdentityRef: OpenStackIdentityReference{ + template: &infrav1.OpenStackCluster{ + Spec: infrav1.OpenStackClusterSpec{ + IdentityRef: infrav1.OpenStackIdentityReference{ Name: "foobar", CloudName: "foobar", }, - ManagedSecurityGroups: &ManagedSecurityGroups{ - AllNodesSecurityGroupRules: []SecurityGroupRuleSpec{ + ManagedSecurityGroups: &infrav1.ManagedSecurityGroups{ + AllNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ { Name: "foobar", Description: pointer.String("foobar"), PortRangeMin: pointer.Int(80), PortRangeMax: pointer.Int(80), Protocol: pointer.String("tcp"), - RemoteManagedGroups: []ManagedSecurityGroupName{"controlplane"}, + RemoteManagedGroups: []infrav1.ManagedSecurityGroupName{"controlplane"}, RemoteGroupID: pointer.String("foobar"), }, }, @@ -581,7 +586,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..b82f510ca8 --- /dev/null +++ b/pkg/webhooks/openstackclustertemplate_webhook.go @@ -0,0 +1,90 @@ +/* +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" + 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/v1beta1" +) + +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-openstackclustertemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackclustertemplates,versions=v1beta1,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{} + +// 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 + } + + 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/v1beta1/openstackmachine_webhook.go b/pkg/webhooks/openstackmachine_webhook.go similarity index 57% rename from api/v1beta1/openstackmachine_webhook.go rename to pkg/webhooks/openstackmachine_webhook.go index 93c91c5ffa..cfee9e5c74 100644 --- a/api/v1beta1/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 v1beta1 +package webhooks import ( + "context" "fmt" "reflect" @@ -24,59 +25,62 @@ 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/v1beta1" ) -// 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-v1beta1-openstackmachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,versions=v1beta1,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-v1beta1-openstackmachine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,versions=v1beta1,name=validation.openstackmachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 -// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-openstackmachine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachines,versions=v1beta1,name=default.openstackmachine.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1 +type openStackMachineWebhook struct{} -var ( - _ webhook.Defaulter = &OpenStackMachine{} - _ webhook.Validator = &OpenStackMachine{} -) +// Compile-time assertion that openStackMachineWebhook implements webhook.CustomValidator. +var _ webhook.CustomValidator = &openStackMachineWebhook{} -// Default satisfies the defaulting webhook interface. -func (r *OpenStackMachine) Default() { -} - -// 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.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)), }) } @@ -106,10 +110,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/v1beta1/openstackmachinetemplate_webhook.go b/pkg/webhooks/openstackmachinetemplate_webhook.go similarity index 55% rename from api/v1beta1/openstackmachinetemplate_webhook.go rename to pkg/webhooks/openstackmachinetemplate_webhook.go index 24f27a937e..4d11008836 100644 --- a/api/v1beta1/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 v1beta1 +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/v1beta1" +) -// +kubebuilder:object:generate=false -type OpenStackMachineTemplateWebhook struct{} +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-openstackmachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates,versions=v1beta1,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-v1beta1-openstackmachinetemplate,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=openstackmachinetemplates,versions=v1beta1,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/v1beta1/openstackmachinetemplate_webhook_test.go b/pkg/webhooks/openstackmachinetemplate_webhook_test.go similarity index 55% rename from api/v1beta1/openstackmachinetemplate_webhook_test.go rename to pkg/webhooks/openstackmachinetemplate_webhook_test.go index 95fd2c7ccf..b6d30791c6 100644 --- a/api/v1beta1/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 v1beta1 +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/v1beta1" ) func TestOpenStackMachineTemplate_ValidateUpdate(t *testing.T) { @@ -33,29 +35,29 @@ 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: ImageFilter{Name: "bar"}, + Image: infrav1.ImageFilter{Name: "bar"}, }, }, }, }, - newTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + newTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", - Image: ImageFilter{Name: "NewImage"}, + Image: infrav1.ImageFilter{Name: "NewImage"}, }, }, }, @@ -65,12 +67,12 @@ 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: ImageFilter{Name: "bar"}, + Image: infrav1.ImageFilter{Name: "bar"}, }, }, }, @@ -78,12 +80,12 @@ 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: ImageFilter{Name: "bar"}, + Image: infrav1.ImageFilter{Name: "bar"}, }, }, }, @@ -95,22 +97,22 @@ 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: ImageFilter{Name: "bar"}, + Image: infrav1.ImageFilter{Name: "bar"}, }, }, }, }, - newTemplate: &OpenStackMachineTemplate{ - Spec: OpenStackMachineTemplateSpec{ - Template: OpenStackMachineTemplateResource{ - Spec: OpenStackMachineSpec{ + newTemplate: &infrav1.OpenStackMachineTemplate{ + Spec: infrav1.OpenStackMachineTemplateSpec{ + Template: infrav1.OpenStackMachineTemplateResource{ + Spec: infrav1.OpenStackMachineSpec{ Flavor: "foo", - Image: ImageFilter{Name: "NewImage"}, + Image: infrav1.ImageFilter{Name: "NewImage"}, }, }, }, @@ -120,27 +122,27 @@ 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: ImageFilter{Name: "bar"}, + Image: infrav1.ImageFilter{Name: "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: ImageFilter{Name: "NewImage"}, + Image: infrav1.ImageFilter{Name: "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..e950a183a9 --- /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" + + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/manager" + + infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" +) + +func RegisterAllWithManager(mgr manager.Manager) []error { + 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 _, listType := range []runtime.Object{ + &infrav1.OpenStackClusterList{}, + &infrav1.OpenStackClusterTemplateList{}, + &infrav1.OpenStackMachineList{}, + &infrav1.OpenStackMachineTemplateList{}, + } { + if err := builder.WebhookManagedBy(mgr). + For(listType). + Complete(); err != nil { + errs = append(errs, fmt.Errorf("creating webhook for %T: %v", listType, err)) + } + } + + return errs +} diff --git a/test/e2e/suites/apivalidations/suite_test.go b/test/e2e/suites/apivalidations/suite_test.go index 65a8e69136..70eef5e7ec 100644 --- a/test/e2e/suites/apivalidations/suite_test.go +++ b/test/e2e/suites/apivalidations/suite_test.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-openstack/pkg/webhooks" ) var ( @@ -119,14 +120,7 @@ var _ = BeforeSuite(func() { }), }) Expect(err).ToNot(HaveOccurred(), "Manager setup should succeed") - - Expect((&infrav1.OpenStackMachineTemplateWebhook{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachineTemplate webhook should be registered with manager") - Expect((&infrav1.OpenStackMachineTemplateList{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachineTemplateList webhook should be registered with manager") - Expect((&infrav1.OpenStackCluster{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackCluster webhook should be registered with manager") - Expect((&infrav1.OpenStackClusterTemplate{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackClusterTemplate webhook should be registered with manager") - Expect((&infrav1.OpenStackMachine{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachine webhook should be registered with manager") - Expect((&infrav1.OpenStackMachineList{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackMachineList webhook should be registered with manager") - Expect((&infrav1.OpenStackClusterList{}).SetupWebhookWithManager(mgr)).To(Succeed(), "OpenStackClusterList webhook should be registered with manager") + Expect(webhooks.RegisterAllWithManager(mgr)).To(BeEmpty(), "Failed to register webhooks") By("Starting manager") var mgrCtx context.Context