Merge pull request #1463 from abays/osversion_check

[OSPRH-17106] Improve detection of pre-existing OpenStackVersion

Author: openshift-merge-bot[bot]

apis/core/v1beta1/conditions.go

@@ -152,6 +152,9 @@ const (
 
 	// OpenStackControlPlaneCertCleanupReadyCondition Status=True condition which indicates global certification cleanup is Ready
 	OpenStackControlPlaneCertCleanupReadyCondition condition.Type = "OpenStackControlPlaneCertCleanupReadyCondition"
+
+	// OpenStackControlPlaneOpenStackVersionInitializationReadyCondition Status=True condition which indicates if OpenStackVersion is initialized
+	OpenStackControlPlaneOpenStackVersionInitializationReadyCondition condition.Type = "OpenStackControlPlaneOpenStackVersionInitializationReadyCondition"
 )
 
 // Common Messages used by API objects.
@@ -471,6 +474,18 @@ const (
 
 	// OpenStackControlPlaneInstanceHaCMReadyMessage
 	OpenStackControlPlaneInstanceHaCMReadyMessage = "OpenStackControlPlane InstanceHa CM is available"
+
+	// OpenStackControlPlaneOpenStackVersionInitializationReadyInitMessage
+	OpenStackControlPlaneOpenStackVersionInitializationReadyInitMessage = "OpenStackControlPlane OpenStackVersion initialization not started"
+
+	// OpenStackControlPlaneOpenStackVersionInitializationReadyMessage
+	OpenStackControlPlaneOpenStackVersionInitializationReadyMessage = "OpenStackControlPlane OpenStackVersion initialized"
+
+	// OpenStackControlPlaneOpenStackVersionInitializationReadyRunningMessage
+	OpenStackControlPlaneOpenStackVersionInitializationReadyRunningMessage = "OpenStackControlPlane OpenStackVersion initialization in progress"
+
+	// OpenStackControlPlaneOpenStackVersionInitializationReadyErrorMessage
+	OpenStackControlPlaneOpenStackVersionInitializationReadyErrorMessage = "OpenStackControlPlane OpenStackVersion initialization error occured %s"
 )
 
 // Version Conditions used by to drive minor updates

apis/core/v1beta1/openstackcontrolplane_types.go

@@ -912,6 +912,7 @@ func (instance *OpenStackControlPlane) InitConditions() {
 		condition.UnknownCondition(OpenStackControlPlaneBarbicanReadyCondition, condition.InitReason, OpenStackControlPlaneBarbicanReadyInitMessage),
 		condition.UnknownCondition(OpenStackControlPlaneRedisReadyCondition, condition.InitReason, OpenStackControlPlaneRedisReadyInitMessage),
 		condition.UnknownCondition(OpenStackControlPlaneCAReadyCondition, condition.InitReason, OpenStackControlPlaneCAReadyInitMessage),
+		condition.UnknownCondition(OpenStackControlPlaneOpenStackVersionInitializationReadyCondition, condition.InitReason, OpenStackControlPlaneOpenStackVersionInitializationReadyInitMessage),
 
 		// Also add the overall status condition as Unknown
 		condition.UnknownCondition(condition.ReadyCondition, condition.InitReason, condition.ReadyInitMessage),

apis/core/v1beta1/openstackcontrolplane_webhook.go

@@ -83,7 +83,6 @@ var _ webhook.Validator = &OpenStackControlPlane{}
 func (r *OpenStackControlPlane) ValidateCreate() (admission.Warnings, error) {
 	openstackcontrolplanelog.Info("validate create", "name", r.Name)
 
-	var allErrs field.ErrorList
 	var allWarn []string
 	basePath := field.NewPath("spec")
 
@@ -118,7 +117,15 @@ func (r *OpenStackControlPlane) ValidateCreate() (admission.Warnings, error) {
 		)
 	}
 
-	allWarn, allErrs = r.ValidateCreateServices(basePath)
+	allErrs, err := r.ValidateVersion()
+
+	// Version validation can generate non-field errors, so we consider those first
+	if err != nil {
+		return nil, err
+	}
+
+	allWarn, errs := r.ValidateCreateServices(basePath)
+	allErrs = append(allErrs, errs...)
 
 	if err := r.ValidateTopology(basePath); err != nil {
 		allErrs = append(allErrs, err)
@@ -672,6 +679,56 @@ func (r *OpenStackControlPlane) ValidateServiceDependencies(basePath *field.Path
 	return allErrs
 }
 
+func (r *OpenStackControlPlane) ValidateVersion() (field.ErrorList, error) {
+	var allErrs field.ErrorList
+
+	openStackVersionList, err := GetOpenStackVersions(r.Namespace, ctlplaneWebhookClient)
+
+	if err != nil {
+		return allErrs, apierrors.NewForbidden(
+			schema.GroupResource{
+				Group:    GroupVersion.WithKind("OpenStackControlPlane").Group,
+				Resource: GroupVersion.WithKind("OpenStackControlPlane").Kind,
+			}, r.GetName(), &field.Error{
+				Type:     field.ErrorTypeForbidden,
+				Field:    "",
+				BadValue: r.Name,
+				Detail:   err.Error(),
+			},
+		)
+	}
+
+	if len(openStackVersionList.Items) > 0 {
+		if len(openStackVersionList.Items) > 1 {
+			return allErrs, apierrors.NewForbidden(
+				schema.GroupResource{
+					Group:    GroupVersion.WithKind("OpenStackControlPlane").Group,
+					Resource: GroupVersion.WithKind("OpenStackControlPlane").Kind,
+				}, r.GetName(), &field.Error{
+					Type:     field.ErrorTypeForbidden,
+					Field:    "",
+					BadValue: r.Name,
+					Detail: fmt.Sprintf(
+						"multiple (%d) OpenStackVersions found in namespace %s: only one may be present.  Please rectify before creating OpenStackControlPlane",
+						len(openStackVersionList.Items), r.Namespace),
+				},
+			)
+
+		}
+
+		openStackVersion := openStackVersionList.Items[0]
+
+		if openStackVersion.Name != r.Name {
+			err := field.Invalid(field.NewPath("metadata").Child("name"),
+				r.Name, fmt.Sprintf("OpenStackControlPlane '%s' must have same name as the existing '%s' OpenStackVersion",
+					r.Name, openStackVersion.Name))
+			allErrs = append(allErrs, err)
+		}
+	}
+
+	return allErrs, nil
+}
+
 // +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{}

apis/core/v1beta1/openstackversion_types.go

@@ -17,11 +17,13 @@ limitations under the License.
 package v1beta1
 
 import (
+	"context"
 	"regexp"
 
 	condition "github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -266,3 +268,18 @@ func GetOpenStackReleaseVersion(envVars []string) string {
 	)
 
 }
+
+// GetOpenStackVersions - returns the OpenStackVersion resource(s) associated with the namespace
+func GetOpenStackVersions(namespace string, k8sClient client.Client) (*OpenStackVersionList, error) {
+	versionList := &OpenStackVersionList{}
+
+	listOpts := []client.ListOption{
+		client.InNamespace(namespace),
+	}
+
+	if err := k8sClient.List(context.TODO(), versionList, listOpts...); err != nil {
+		return nil, err
+	}
+
+	return versionList, nil
+}

apis/core/v1beta1/openstackversion_webhook.go

@@ -17,7 +17,6 @@ limitations under the License.
 package v1beta1
 
 import (
-	"context"
 	"os"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -26,7 +25,6 @@ import (
 
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	goClient "sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -100,13 +98,9 @@ func (r *OpenStackVersion) ValidateCreate() (admission.Warnings, error) {
 		)
 	}
 
-	versionList := &OpenStackVersionList{}
+	versionList, err := GetOpenStackVersions(r.Namespace, versionWebhookClient)
 
-	listOpts := []client.ListOption{
-		client.InNamespace(r.Namespace),
-	}
-
-	if err := versionWebhookClient.List(context.TODO(), versionList, listOpts...); err != nil {
+	if err != nil {
 
 		return nil, apierrors.NewForbidden(
 			schema.GroupResource{

controllers/core/openstackcontrolplane_controller.go

@@ -203,8 +203,21 @@ func (r *OpenStackControlPlaneReconciler) Reconcile(ctx context.Context, req ctr
 	Log.Info("Looking up the current OpenStackVersion")
 	ctrlResult, version, err := openstack.ReconcileVersion(ctx, instance, helper)
 	if err != nil {
+		instance.Status.Conditions.Set(condition.FalseCondition(
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyErrorMessage,
+			err.Error()))
 		return ctrl.Result{}, err
 	} else if (ctrlResult != ctrl.Result{}) {
+		// It appears that openstack.ReconcileVersion never returns a non-empty ctrlResult,
+		// but we'll put this condition update here just in case it ever changes
+		instance.Status.Conditions.Set(condition.FalseCondition(
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyCondition,
+			condition.RequestedReason,
+			condition.SeverityInfo,
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyRunningMessage))
 		return ctrlResult, nil
 	}
 
@@ -216,7 +229,13 @@ func (r *OpenStackControlPlaneReconciler) Reconcile(ctx context.Context, req ctr
 		Log,
 	)
 	if err != nil {
-		Log.Error(err, "unable to create helper")
+		Log.Error(err, "unable to create version helper")
+		instance.Status.Conditions.Set(condition.FalseCondition(
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyErrorMessage,
+			err.Error()))
 		return ctrl.Result{}, err
 	}
 
@@ -227,9 +246,16 @@ func (r *OpenStackControlPlaneReconciler) Reconcile(ctx context.Context, req ctr
 
 	// wait until the version is initialized so we have images on the version.Status
 	if !version.Status.Conditions.IsTrue(corev1beta1.OpenStackVersionInitialized) {
+		instance.Status.Conditions.Set(condition.FalseCondition(
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyCondition,
+			condition.RequestedReason,
+			condition.SeverityInfo,
+			corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyRunningMessage))
 		return ctrlResult, nil
 	}
 
+	instance.Status.Conditions.MarkTrue(corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyCondition, corev1beta1.OpenStackControlPlaneOpenStackVersionInitializationReadyMessage)
+
 	if instance.Status.DeployedVersion == nil || version.Spec.TargetVersion == *instance.Status.DeployedVersion { //revive:disable:indent-error-flow
 		// green field deployment or no minor update in progress
 		ctrlResult, err := r.reconcileNormal(ctx, instance, version, helper)

tests/functional/ctlplane/base_test.go

@@ -47,6 +47,7 @@ type Names struct {
 	Namespace                          string
 	OpenStackControlplaneName          types.NamespacedName
 	OpenStackVersionName               types.NamespacedName
+	OpenStackVersionName2              types.NamespacedName
 	KeystoneAPIName                    types.NamespacedName
 	MemcachedName                      types.NamespacedName
 	MemcachedCertName                  types.NamespacedName
@@ -98,6 +99,10 @@ func CreateNames(openstackControlplaneName types.NamespacedName) Names {
 			Namespace: openstackControlplaneName.Namespace,
 			Name:      openstackControlplaneName.Name, // same name as controlplane
 		},
+		OpenStackVersionName2: types.NamespacedName{
+			Namespace: openstackControlplaneName.Namespace,
+			Name:      "arbitrary-openstackversion", // different name than controlplane
+		},
 		RootCAPublicName: types.NamespacedName{
 			Namespace: openstackControlplaneName.Namespace,
 			Name:      "rootca-public"},

tests/functional/ctlplane/openstackoperator_controller_test.go

@@ -1832,6 +1832,78 @@ var _ = Describe("OpenStackOperator controller", func() {
 		})
 	})
 
+	When("OpenStackControlplane instance is created and an OpenStackVersion already exists with an arbitrary name", func() {
+		BeforeEach(func() {
+			DeferCleanup(
+				th.DeleteInstance,
+				CreateOpenStackVersion(names.OpenStackVersionName2, GetDefaultOpenStackVersionSpec()),
+			)
+		})
+
+		It("rejects the OpenStackControlPlane if its name is not that same as the OpenStackVersion's name", func() {
+			raw := map[string]interface{}{
+				"apiVersion": "core.openstack.org/v1beta1",
+				"kind":       "OpenStackControlPlane",
+				"metadata": map[string]interface{}{
+					"name":      names.OpenStackControlplaneName.Name,
+					"namespace": names.Namespace,
+				},
+				"spec": GetDefaultOpenStackControlPlaneSpec(),
+			}
+
+			unstructuredObj := &unstructured.Unstructured{Object: raw}
+			_, err := controllerutil.CreateOrPatch(
+				th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
+			Expect(err).Should(HaveOccurred())
+			var statusError *k8s_errors.StatusError
+			Expect(errors.As(err, &statusError)).To(BeTrue())
+			Expect(statusError.ErrStatus.Details.Kind).To(Equal("OpenStackControlPlane"))
+			Expect(statusError.ErrStatus.Message).To(
+				ContainSubstring(
+					"must have same name as the existing"),
+			)
+
+			// we remove the finalizer as this is needed when the OpenStackControlplane
+			// does not create the OpenStackVersion itself
+			DeferCleanup(
+				OpenStackVersionRemoveFinalizer,
+				ctx,
+				names.OpenStackVersionName2,
+			)
+		})
+
+		It("accepts the OpenStackControlPlane if its name is the same as the OpenStackVersion's name", func() {
+			raw := map[string]interface{}{
+				"apiVersion": "core.openstack.org/v1beta1",
+				"kind":       "OpenStackControlPlane",
+				"metadata": map[string]interface{}{
+					"name":      names.OpenStackVersionName2.Name,
+					"namespace": names.Namespace,
+				},
+				"spec": GetDefaultOpenStackControlPlaneSpec(),
+			}
+
+			unstructuredObj := &unstructured.Unstructured{Object: raw}
+			_, err := controllerutil.CreateOrPatch(
+				th.Ctx, th.K8sClient, unstructuredObj, func() error { return nil })
+			Expect(err).ShouldNot(HaveOccurred())
+
+			openStackControlPlane := &corev1.OpenStackControlPlane{}
+			openStackControlPlane.ObjectMeta.Namespace = names.Namespace
+			openStackControlPlane.Name = names.OpenStackVersionName2.Name
+			err = k8sClient.Delete(ctx, openStackControlPlane)
+			Expect(err).ShouldNot(HaveOccurred())
+
+			// we remove the finalizer as this is needed when the OpenStackControlplane
+			// does not create the OpenStackVersion itself
+			DeferCleanup(
+				OpenStackVersionRemoveFinalizer,
+				ctx,
+				names.OpenStackVersionName2,
+			)
+		})
+	})
+
 	When("An OpenStackControlplane instance is created with nodeSelector", func() {
 		BeforeEach(func() {
 			spec := GetDefaultOpenStackControlPlaneSpec()

tests/kuttl/common/assert-sample-deployment.yaml

@@ -282,6 +282,10 @@ status:
     reason: Ready
     status: "True"
     type: OpenStackControlPlaneOVNReady
+  - message: OpenStackControlPlane OpenStackVersion initialized
+    reason: Ready
+    status: "True"
+    type: OpenStackControlPlaneOpenStackVersionInitializationReadyCondition
   - message: OpenStackControlPlane PlacementAPI completed
     reason: Ready
     status: "True"

tests/kuttl/tests/ctlplane-basic-deployment/03-assert-deploy-custom-cacert.yaml

@@ -91,6 +91,10 @@ status:
     reason: Ready
     status: "True"
     type: OpenStackControlPlaneOVNReady
+  - message: OpenStackControlPlane OpenStackVersion initialized
+    reason: Ready
+    status: "True"
+    type: OpenStackControlPlaneOpenStackVersionInitializationReadyCondition
   - message: OpenStackControlPlane PlacementAPI completed
     reason: Ready
     status: "True"