Merge pull request openstack-k8s-operators#220 from abays/glance_imag…

…e_defaults

Validation and mutation webhook call-out pattern

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
@@ -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

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()
+	}
 }

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,

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

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

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

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")