From 0bb8ac876e639cf89abc546a07df1df1889795f5 Mon Sep 17 00:00:00 2001 From: Andrew Bays Date: Mon, 6 Mar 2023 14:28:51 +0000 Subject: [PATCH] Validation and mutation webhook call-out pattern --- ....openstack.org_openstackcontrolplanes.yaml | 2 +- .../v1beta1/openstackcontrolplane_types.go | 6 +- .../v1beta1/openstackcontrolplane_webhook.go | 95 ++++++++++++++++--- cmd/csv-merger/csv-merger.go | 59 +++++++++++- ....openstack.org_openstackcontrolplanes.yaml | 2 +- config/webhook/kustomizeconfig.yaml | 7 ++ config/webhook/manifests.yaml | 27 ++++++ hack/configure_local_webhook.sh | 28 ++++++ main.go | 10 ++ 9 files changed, 213 insertions(+), 23 deletions(-) diff --git a/apis/bases/core.openstack.org_openstackcontrolplanes.yaml b/apis/bases/core.openstack.org_openstackcontrolplanes.yaml index 670f10d0c..7ea126060 100644 --- a/apis/bases/core.openstack.org_openstackcontrolplanes.yaml +++ b/apis/bases/core.openstack.org_openstackcontrolplanes.yaml @@ -13969,7 +13969,7 @@ spec: description: Galera - Parameters related to the Galera services properties: enabled: - default: true + default: false description: Enabled - Whether Galera services should be deployed and managed type: boolean diff --git a/apis/core/v1beta1/openstackcontrolplane_types.go b/apis/core/v1beta1/openstackcontrolplane_types.go index 2808dbf6a..ae33f802a 100644 --- a/apis/core/v1beta1/openstackcontrolplane_types.go +++ b/apis/core/v1beta1/openstackcontrolplane_types.go @@ -103,7 +103,7 @@ type OpenStackControlPlaneSpec struct { // However, if extraVolumes are specified within the single operator // template Section, the globally defined ExtraMounts are ignored and // overridden for the operator which has this section already. - ExtraMounts []OpenStackExtraVolMounts `json:"extraMounts"` + ExtraMounts []OpenStackExtraVolMounts `json:"extraMounts,omitempty"` } // KeystoneSection defines the desired state of Keystone service @@ -169,7 +169,7 @@ type MariadbSection struct { // GaleraSection defines the desired state of Galera services type GaleraSection struct { // +kubebuilder:validation:Optional - // +kubebuilder:default=true + // +kubebuilder:default=false // Enabled - Whether Galera services should be deployed and managed Enabled bool `json:"enabled,omitempty"` @@ -198,7 +198,7 @@ type RabbitmqTemplate struct { // +kubebuilder:validation:Optional // ExternalEndpoint, expose a VIP via MetalLB on the pre-created address pool - ExternalEndpoint *MetalLBConfig `json:"externalEndpoint"` + ExternalEndpoint *MetalLBConfig `json:"externalEndpoint,omitempty"` } // MetalLBConfig to configure the MetalLB loadbalancer service diff --git a/apis/core/v1beta1/openstackcontrolplane_webhook.go b/apis/core/v1beta1/openstackcontrolplane_webhook.go index b83b865bf..1d8b1bf53 100644 --- a/apis/core/v1beta1/openstackcontrolplane_webhook.go +++ b/apis/core/v1beta1/openstackcontrolplane_webhook.go @@ -19,7 +19,10 @@ package v1beta1 import ( "fmt" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -35,9 +38,6 @@ func (r *OpenStackControlPlane) SetupWebhookWithManager(mgr ctrl.Manager) error Complete() } -// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! - -// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. //+kubebuilder:webhook:path=/validate-core-openstack-org-v1beta1-openstackcontrolplane,mutating=false,failurePolicy=Fail,sideEffects=None,groups=core.openstack.org,resources=openstackcontrolplanes,verbs=create;update,versions=v1beta1,name=vopenstackcontrolplane.kb.io,admissionReviewVersions=v1 var _ webhook.Validator = &OpenStackControlPlane{} @@ -46,14 +46,38 @@ var _ webhook.Validator = &OpenStackControlPlane{} func (r *OpenStackControlPlane) ValidateCreate() error { openstackcontrolplanelog.Info("validate create", "name", r.Name) - return r.ValidateServices() + var allErrs field.ErrorList + basePath := field.NewPath("spec") + if err := r.ValidateServices(basePath); err != nil { + allErrs = append(allErrs, err...) + } + + if len(allErrs) != 0 { + return apierrors.NewInvalid( + schema.GroupKind{Group: "core.openstack.org", Kind: "OpenStackControlPlane"}, + r.Name, allErrs) + } + + return nil } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type func (r *OpenStackControlPlane) ValidateUpdate(old runtime.Object) error { openstackcontrolplanelog.Info("validate update", "name", r.Name) - return r.ValidateServices() + var allErrs field.ErrorList + basePath := field.NewPath("spec") + if err := r.ValidateServices(basePath); err != nil { + allErrs = append(allErrs, err...) + } + + if len(allErrs) != 0 { + return apierrors.NewInvalid( + schema.GroupKind{Group: "core.openstack.org", Kind: "OpenStackControlPlane"}, + r.Name, allErrs) + } + + return nil } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type @@ -91,39 +115,80 @@ func (r *OpenStackControlPlane) checkDepsEnabled(name string) bool { } // ValidateServices implements common function for validating services -func (r *OpenStackControlPlane) ValidateServices() error { +func (r *OpenStackControlPlane) ValidateServices(basePath *field.Path) field.ErrorList { + var allErrs field.ErrorList // Temporary check until MariaDB is deprecated if r.Spec.Mariadb.Enabled && r.Spec.Galera.Enabled { - return fmt.Errorf("Mariadb and Galera are mutually exclusive") + err := field.Invalid(basePath.Child("mariaDB").Child("enabled"), r.Spec.Mariadb.Enabled, "Mariadb and Galera are mutually exclusive") + allErrs = append(allErrs, err) } // Add service dependency validations - errorMsg := "%s service dependencies are not enabled." + depErrorMsg := "%s service dependencies are not enabled." if r.Spec.Keystone.Enabled && !r.checkDepsEnabled("Keystone") { - return fmt.Errorf(errorMsg, "Keystone") + err := field.Invalid(basePath.Child("keystone").Child("enabled"), r.Spec.Keystone.Enabled, fmt.Sprintf(depErrorMsg, "Keystone")) + allErrs = append(allErrs, err) } if r.Spec.Glance.Enabled && !r.checkDepsEnabled("Glance") { - return fmt.Errorf(errorMsg, "Glance") + err := field.Invalid(basePath.Child("glance").Child("enabled"), r.Spec.Glance.Enabled, fmt.Sprintf(depErrorMsg, "Glance")) + allErrs = append(allErrs, err) } if r.Spec.Cinder.Enabled && !r.checkDepsEnabled("Cinder") { - return fmt.Errorf(errorMsg, "Cinder") + err := field.Invalid(basePath.Child("cinder").Child("enabled"), r.Spec.Cinder.Enabled, fmt.Sprintf(depErrorMsg, "Cinder")) + allErrs = append(allErrs, err) } if r.Spec.Placement.Enabled && !r.checkDepsEnabled("Placement") { - return fmt.Errorf(errorMsg, "Placement") + err := field.Invalid(basePath.Child("placement").Child("enabled"), r.Spec.Placement.Enabled, fmt.Sprintf(depErrorMsg, "Placement")) + allErrs = append(allErrs, err) } if r.Spec.Neutron.Enabled && !r.checkDepsEnabled("Neutron") { - return fmt.Errorf(errorMsg, "Neutron") + err := field.Invalid(basePath.Child("neutron").Child("enabled"), r.Spec.Neutron.Enabled, fmt.Sprintf(depErrorMsg, "Neutron")) + allErrs = append(allErrs, err) } if r.Spec.Nova.Enabled && !r.checkDepsEnabled("Nova") { - return fmt.Errorf(errorMsg, "Nova") + err := field.Invalid(basePath.Child("nova").Child("enabled"), r.Spec.Nova.Enabled, fmt.Sprintf(depErrorMsg, "Nova")) + allErrs = append(allErrs, err) } - return nil + // Checks which call internal validation logic for individual service operators + + // Ironic + if r.Spec.Ironic.Enabled { + if err := r.Spec.Ironic.Template.ValidateCreate(basePath.Child("ironic").Child("template")); err != nil { + allErrs = append(allErrs, err...) + } + } + + return allErrs +} + +//+kubebuilder:webhook:path=/mutate-core-openstack-org-v1beta1-openstackcontrolplane,mutating=true,failurePolicy=fail,sideEffects=None,groups=core.openstack.org,resources=openstackcontrolplanes,verbs=create;update,versions=v1beta1,name=mopenstackcontrolplane.kb.io,admissionReviewVersions=v1 + +var _ webhook.Defaulter = &OpenStackControlPlane{} + +// Default implements webhook.Defaulter so a webhook will be registered for the type +func (r *OpenStackControlPlane) Default() { + openstackcontrolplanelog.Info("default", "name", r.Name) + + r.DefaultServices() +} + +// DefaultServices - common function for calling individual services' defaulting functions +func (r *OpenStackControlPlane) DefaultServices() { + // Glance + if r.Spec.Glance.Enabled { + r.Spec.Glance.Template.Default() + } + + // Ironic + if r.Spec.Ironic.Enabled { + r.Spec.Ironic.Template.Default() + } } diff --git a/cmd/csv-merger/csv-merger.go b/cmd/csv-merger/csv-merger.go index 47842709f..45daef318 100644 --- a/cmd/csv-merger/csv-merger.go +++ b/cmd/csv-merger/csv-merger.go @@ -29,6 +29,7 @@ import ( csvv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -123,7 +124,6 @@ func main() { Status: csvBase.Status} installStrategyBase := csvBase.Spec.InstallStrategy.StrategySpec - webhookdefinitions := csvBase.Spec.WebhookDefinitions for _, csvFile := range csvs { if csvFile != "" { @@ -139,10 +139,64 @@ func main() { panic(err) } + // 1. We need to add the "env" section from this Service Operator deployment in case there + // are default values configured there that are needed for use with defaulting webhooks + // + // - DeploymentSpecs[0] is always the base deployment for OpenStack Operator + // - Container at index 1 in DeploymentSpecs[0].Spec.Template.Spec.Containers list is + // always the OpenStack Operator controller-manager + // - We need to find the Service Operator's controller-manager container in + // csvStruct.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Containers + // + // TODO: What about "env" list keys that overlap between Service Operators (i.e. non-unique + // names)? + // + // 2. We also need to inject "ENABLE_WEBHOOKS=false" into the env vars for the Service Operators' + // deployments, and then remove their webhook server's cert's volume mount + + for index, container := range csvStruct.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Containers { + // Copy env vars from the Service Operator into the OpenStack Operator + if container.Name == "manager" { + installStrategyBase.DeploymentSpecs[0].Spec.Template.Spec.Containers[1].Env = append( + // OpenStack Operator controller-manager container env vars + installStrategyBase.DeploymentSpecs[0].Spec.Template.Spec.Containers[1].Env, + // Service Operator controller-manager container env vars + container.Env..., + ) + + // Now we also need to turn off any "internal" webhooks that belong to the service + // operator, as we are now using "external" webhooks that live in the OpenStack + // operator. These "external" webhooks will eventually call the mutating/validating + // logic that was previously housed within the "internal" Service Operator webhook + // logic. + container.Env = append(container.Env, + v1.EnvVar{ + Name: "ENABLE_WEBHOOKS", + Value: "false", + }, + ) + + // And finally we need to remove the webhook server's cert volume mount + for volMountIndex, volMount := range container.VolumeMounts { + if volMount.Name == "cert" { + container.VolumeMounts[volMountIndex] = container.VolumeMounts[len(container.VolumeMounts)-1] + container.VolumeMounts = container.VolumeMounts[:len(container.VolumeMounts)-1] + // Found the target mount, so stop iterating + break + } + } + + // Need to replace the container in the Deployment since this local variable is a copy + csvStruct.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Containers[index] = container + + // We found the controller-manager container, so no need to continue iterating + break + } + } + installStrategyBase.DeploymentSpecs = append(installStrategyBase.DeploymentSpecs, csvStruct.Spec.InstallStrategy.StrategySpec.DeploymentSpecs...) installStrategyBase.ClusterPermissions = append(installStrategyBase.ClusterPermissions, csvStruct.Spec.InstallStrategy.StrategySpec.ClusterPermissions...) installStrategyBase.Permissions = append(installStrategyBase.Permissions, csvStruct.Spec.InstallStrategy.StrategySpec.Permissions...) - webhookdefinitions = append(webhookdefinitions, csvStruct.Spec.WebhookDefinitions...) for _, owned := range csvStruct.Spec.CustomResourceDefinitions.Owned { csvExtended.Spec.CustomResourceDefinitions.Owned = append( @@ -201,7 +255,6 @@ func main() { csvExtended.Annotations["operators.operatorframework.io/internal-objects"] = string(hiddenCrdsJ) csvExtended.Spec.InstallStrategy.StrategyName = "deployment" - csvExtended.Spec.WebhookDefinitions = webhookdefinitions csvExtended.Spec.InstallStrategy = csvv1alpha1.NamedInstallStrategy{ StrategyName: "deployment", StrategySpec: installStrategyBase, diff --git a/config/crd/bases/core.openstack.org_openstackcontrolplanes.yaml b/config/crd/bases/core.openstack.org_openstackcontrolplanes.yaml index 670f10d0c..7ea126060 100644 --- a/config/crd/bases/core.openstack.org_openstackcontrolplanes.yaml +++ b/config/crd/bases/core.openstack.org_openstackcontrolplanes.yaml @@ -13969,7 +13969,7 @@ spec: description: Galera - Parameters related to the Galera services properties: enabled: - default: true + default: false description: Enabled - Whether Galera services should be deployed and managed type: boolean diff --git a/config/webhook/kustomizeconfig.yaml b/config/webhook/kustomizeconfig.yaml index e809f7820..25e21e3c9 100644 --- a/config/webhook/kustomizeconfig.yaml +++ b/config/webhook/kustomizeconfig.yaml @@ -4,11 +4,18 @@ nameReference: - kind: Service version: v1 fieldSpecs: + - kind: MutatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/name - kind: ValidatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/name namespace: +- kind: MutatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/namespace + create: true - kind: ValidatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/namespace diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 266473bc8..2c9410106 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -1,5 +1,32 @@ --- apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + creationTimestamp: null + name: mutating-webhook-configuration +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-core-openstack-org-v1beta1-openstackcontrolplane + failurePolicy: Fail + name: mopenstackcontrolplane.kb.io + rules: + - apiGroups: + - core.openstack.org + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - openstackcontrolplanes + sideEffects: None +--- +apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: creationTimestamp: null diff --git a/hack/configure_local_webhook.sh b/hack/configure_local_webhook.sh index eb08ab633..f67c31cb3 100755 --- a/hack/configure_local_webhook.sh +++ b/hack/configure_local_webhook.sh @@ -56,6 +56,34 @@ webhooks: scope: '*' sideEffects: None timeoutSeconds: 10 +--- +apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + name: mopenstackcontrolplane.kb.io +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + caBundle: ${CA_BUNDLE} + url: https://${CRC_IP}:9443/mutate-core-openstack-org-v1beta1-openstackcontrolplane + failurePolicy: Fail + matchPolicy: Equivalent + name: mopenstackcontrolplane.kb.io + objectSelector: {} + rules: + - apiGroups: + - core.openstack.org + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - openstackcontrolplanes + scope: '*' + sideEffects: None + timeoutSeconds: 10 EOF_CAT oc apply -n openstack -f ${TMPDIR}/patch_webhook_configurations.yaml diff --git a/main.go b/main.go index 40b0d8db8..a025f1218 100644 --- a/main.go +++ b/main.go @@ -147,6 +147,16 @@ func main() { os.Exit(1) } + // Defaults for service operators + + // Acquire environmental defaults and initialize Glance and GlanceAPI defaults with them + glanceDefaults := glancev1.GlanceDefaults{ + ContainerImageURL: os.Getenv("GLANCE_API_IMAGE_URL_DEFAULT"), + } + + (&glancev1.Glance{}).Spec.SetupDefaults(glanceDefaults) + + // Webhooks if strings.ToLower(os.Getenv("ENABLE_WEBHOOKS")) != "false" { if err = (&corev1beta1.OpenStackControlPlane{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "OpenStackControlPlane")