[RHCLOUD-21235] Code Refactor (#108)

* Refactor code

controllers/cloud.redhat.com/clowdenvironment_controller.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	clowder "github.com/RedHatInsights/clowder/apis/cloud.redhat.com/v1alpha1"
+	"github.com/RedHatInsights/ephemeral-namespace-operator/controllers/cloud.redhat.com/helpers"
 	"github.com/go-logr/logr"
 	"github.com/prometheus/client_golang/prometheus"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -42,12 +43,6 @@ type ClowdenvironmentReconciler struct {
 //+kubebuilder:rbac:groups=cloud.redhat.com,resources=clowdenvironments,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=cloud.redhat.com,resources=clowdenvironments/status,verbs=get
 
-const (
-	ENV_STATUS_READY = "ready"
-	ENV_STATUS_ERROR = "error"
-	COMPLETION_TIME  = "completion-time"
-)
-
 func (r *ClowdenvironmentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	env := clowder.ClowdEnvironment{}
 	if err := r.Client.Get(ctx, req.NamespacedName, &env); err != nil {
@@ -61,38 +56,46 @@ func (r *ClowdenvironmentReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		"deployments", fmt.Sprintf("%d / %d", env.Status.Deployments.ReadyDeployments, env.Status.Deployments.ManagedDeployments),
 	)
 
-	if ready, _ := VerifyClowdEnvReady(env); ready {
-		nsName := env.Spec.TargetNamespace
-		r.Log.Info("Clowdenvironment ready", "ns-name", nsName)
+	if ready, err := helpers.VerifyClowdEnvReady(env); !ready {
+		return ctrl.Result{Requeue: true}, err
+	}
 
-		if err := CreateFrontendEnv(ctx, r.Client, nsName, env); err != nil {
-			r.Log.Error(err, "Error encountered with frontend environment", "ns-name", nsName)
-			UpdateAnnotations(ctx, r.Client, map[string]string{"env-status": ENV_STATUS_ERROR, "status": "error"}, nsName)
-		} else {
-			r.Log.Info("Namespace ready", "ns-name", nsName)
-			UpdateAnnotations(ctx, r.Client, map[string]string{"env-status": ENV_STATUS_READY, "status": "ready"}, nsName)
+	nsName := env.Spec.TargetNamespace
+	r.Log.Info("clowdenvironment ready", "namespace", nsName)
 
-			ns, err := GetNamespace(ctx, r.Client, nsName)
-			if err != nil {
-				r.Log.Error(err, "Could not retrieve newly created namespace", "ns-name", nsName)
-			}
+	if err := helpers.CreateFrontendEnv(ctx, r.Client, nsName, env); err != nil {
+		r.Log.Error(err, "error encountered with frontend environment", "namespace", nsName)
+		helpers.UpdateAnnotations(ctx, r.Client, nsName, helpers.AnnotationEnvError.ToMap())
+	}
 
-			if _, ok := ns.Annotations["completion-time"]; !ok {
-				nsCompletionTime := time.Now()
-				ns.Annotations["completion-time"] = nsCompletionTime.String()
+	r.Log.Info("namespace ready", "namespace", nsName)
+	helpers.UpdateAnnotations(ctx, r.Client, nsName, helpers.AnnotationEnvReady.ToMap())
+
+	ns, err := helpers.GetNamespace(ctx, r.Client, nsName)
+	if err != nil {
+		r.Log.Error(err, "could not retrieve newly created namespace", "namespace", nsName)
+	}
 
-				if err := r.Client.Update(ctx, &ns); err != nil {
-					return ctrl.Result{}, err
-				}
+	if _, ok := ns.Annotations[helpers.COMPLETION_TIME]; ok {
+		return ctrl.Result{}, nil
+	}
 
-				elapsed := nsCompletionTime.Sub(ns.CreationTimestamp.Time)
+	nsCompletionTime := time.Now()
+	var AnnotationCompletionTime = helpers.CustomAnnotation{Annotation: helpers.COMPLETION_TIME, Value: nsCompletionTime.String()}
 
-				averageNamespaceCreationMetrics.With(prometheus.Labels{"pool": ns.Labels["pool"]}).Observe(float64(elapsed.Seconds()))
-			}
+	err = helpers.UpdateAnnotations(ctx, r.Client, ns.Name, AnnotationCompletionTime.ToMap())
+	if err != nil {
+		r.Log.Error(err, "could not update annotation with completion time", "namespace", nsName)
+	}
 
-		}
+	if err := r.Client.Update(ctx, &ns); err != nil {
+		return ctrl.Result{}, err
 	}
 
+	elapsed := nsCompletionTime.Sub(ns.CreationTimestamp.Time)
+
+	averageNamespaceCreationMetrics.With(prometheus.Labels{"pool": ns.Labels["pool"]}).Observe(float64(elapsed.Seconds()))
+
 	return ctrl.Result{}, nil
 }
 
@@ -122,7 +125,7 @@ func poolFilter(ctx context.Context, cl client.Client) predicate.Predicate {
 }
 
 func isOwnedByPool(ctx context.Context, cl client.Client, nsName string) bool {
-	ns, err := GetNamespace(ctx, cl, nsName)
+	ns, err := helpers.GetNamespace(ctx, cl, nsName)
 	if err != nil {
 		return false
 	}
@@ -136,7 +139,7 @@ func isOwnedByPool(ctx context.Context, cl client.Client, nsName string) bool {
 }
 
 func isOwnedBySpecificPool(ctx context.Context, cl client.Client, nsName string, uid types.UID) bool {
-	ns, err := GetNamespace(ctx, cl, nsName)
+	ns, err := helpers.GetNamespace(ctx, cl, nsName)
 	if err != nil {
 		return false
 	}

controllers/cloud.redhat.com/clowdenvs.go → controllers/cloud.redhat.com/helpers/clowdenvs.go

@@ -1,4 +1,4 @@
-package controllers
+package helpers
 
 import (
 	"context"
@@ -58,7 +58,7 @@ func WaitForClowdEnv(ctx context.Context, cl client.Client, log logr.Logger, nsN
 		if err != nil {
 			msg = msg + fmt.Sprintf(" (%s)", err)
 		}
-		log.Info(msg, "ns-name", nsName)
+		log.Info(msg, "namespace", nsName)
 	}
 
 	return clowdEnv

controllers/cloud.redhat.com/helpers/constants.go

@@ -0,0 +1,27 @@
+package helpers
+
+const (
+	ANNOTATION_ENV_STATUS = "env-status"
+	ANNOTATION_RESERVED   = "reserved"
+
+	COMPLETION_TIME = "completion-time"
+
+	ENV_STATUS_CREATING = "creating"
+	ENV_STATUS_DELETING = "deleting"
+	ENV_STATUS_ERROR    = "error"
+	ENV_STATUS_READY    = "ready"
+
+	KIND_NAMESPACEPOOL = "NamespacePool"
+
+	LABEL_OPERATOR_NS = "operator-ns"
+	LABEL_POOL        = "pool"
+
+	NAMESPACE_EPHEMERAL_BASE = "ephemeral-base"
+
+	BONFIRE_IGNORE_SECRET          = "bonfire.ignore"
+	OPENSHIFT_VAULT_SECRETS_SECRET = "openshift-vault-secrets"
+	QONTRACT_INTEGRATION_SECRET    = "qontract.integration"
+
+	TRUE_VALUE  = "true"
+	FALSE_VALUE = "false"
+)

controllers/cloud.redhat.com/frontends.go → controllers/cloud.redhat.com/helpers/frontends.go

@@ -1,4 +1,4 @@
-package controllers
+package helpers
 
 import (
 	"context"

controllers/cloud.redhat.com/namespaces.go → controllers/cloud.redhat.com/helpers/namespaces.go

@@ -1,8 +1,7 @@
-package controllers
+package helpers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 
@@ -19,27 +18,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-var initialAnnotations = map[string]string{
-	"env-status": "creating",
-	"reserved":   "false",
-}
-
-var initialLabels = map[string]string{
-	"operator-ns": "true",
-}
-
 func CreateNamespace(ctx context.Context, cl client.Client, pool *crd.NamespacePool) (string, error) {
-	// Create project or namespace depending on environment
 	ns := core.Namespace{}
 
-	labels := map[string]string{}
-	for k, v := range initialLabels {
-		labels[k] = v
-	}
+	ns.Name = fmt.Sprintf("ephemeral-%s", strings.ToLower(utils.RandString(6)))
 
-	labels["pool"] = pool.Name
-
-	ns.Name = fmt.Sprintf("ephemeral-%s", strings.ToLower(randString(6)))
+	utils.UpdateAnnotations(&ns, CreateInitialAnnotations())
+	utils.UpdateLabels(&ns, CreateInitialLabels(pool.Name))
 
 	if pool.Spec.Local {
 		if err := cl.Create(ctx, &ns); err != nil {
@@ -60,22 +45,6 @@ func CreateNamespace(ctx context.Context, cl client.Client, pool *crd.NamespaceP
 		return ns.Name, err
 	}
 
-	if len(ns.Annotations) == 0 {
-		ns.SetAnnotations(initialAnnotations)
-	} else {
-		for k, v := range initialAnnotations {
-			ns.Annotations[k] = v
-		}
-	}
-
-	if len(ns.Labels) == 0 {
-		ns.SetLabels(labels)
-	} else {
-		for k, v := range labels {
-			ns.Labels[k] = v
-		}
-	}
-
 	ns.SetOwnerReferences([]metav1.OwnerReference{pool.MakeOwnerReference()})
 
 	if err := cl.Update(ctx, &ns); err != nil {
@@ -84,29 +53,29 @@ func CreateNamespace(ctx context.Context, cl client.Client, pool *crd.NamespaceP
 
 	// Create ClowdEnvironment
 	if err := CreateClowdEnv(ctx, cl, pool.Spec.ClowdEnvironment, ns.Name); err != nil {
-		return "", errors.New("Error creating ClowdEnvironment: " + err.Error())
+		return "", fmt.Errorf("error creating ClowdEnvironment: %s", err.Error())
 	}
 
 	// Create LimitRange
 	limitRange := pool.Spec.LimitRange
 	limitRange.SetNamespace(ns.Name)
 
 	if err := cl.Create(ctx, &limitRange); err != nil {
-		return "", errors.New("Error creating LimitRange: " + err.Error())
+		return "", fmt.Errorf("error creating LimitRange: %s", err.Error())
 	}
 
 	// Create ResourceQuotas
 	resourceQuotas := pool.Spec.ResourceQuotas
 	for _, quota := range resourceQuotas.Items {
 		quota.SetNamespace(ns.Name)
 		if err := cl.Create(ctx, &quota); err != nil {
-			return "", errors.New("Error creating ResourceQuota: " + err.Error())
+			return "", fmt.Errorf("error creating ResourceQuota: %s", err.Error())
 		}
 	}
 
 	// Copy secrets
 	if err := CopySecrets(ctx, cl, ns.Name); err != nil {
-		return "", errors.New("Error copying secrets: " + err.Error())
+		return "", fmt.Errorf("error copying secrets from ephemeral-base namespace: %s", err.Error())
 	}
 
 	return ns.Name, nil
@@ -131,11 +100,11 @@ func GetNamespace(ctx context.Context, cl client.Client, nsName string) (core.Na
 	return ns, nil
 }
 
-func GetReadyNamespaces(ctx context.Context, cl client.Client, pool string) ([]core.Namespace, error) {
+func GetReadyNamespaces(ctx context.Context, cl client.Client, poolName string) ([]core.Namespace, error) {
 	nsList := core.NamespaceList{}
 
-	validatedSelector, _ := labels.ValidatedSelectorFromSet(
-		map[string]string{"operator-ns": "true", "pool": pool})
+	var LabelPoolType = CustomLabel{Label: LABEL_POOL, Value: poolName}
+	validatedSelector, _ := labels.ValidatedSelectorFromSet(LabelPoolType.ToMap())
 
 	nsListOptions := &client.ListOptions{LabelSelector: validatedSelector}
 
@@ -147,8 +116,8 @@ func GetReadyNamespaces(ctx context.Context, cl client.Client, pool string) ([]c
 
 	for _, ns := range nsList.Items {
 		for _, owner := range ns.GetOwnerReferences() {
-			if owner.Kind == "NamespacePool" {
-				ready = CheckReadyStatus(pool, ns, ready)
+			if owner.Kind == KIND_NAMESPACEPOOL {
+				ready = CheckReadyStatus(poolName, ns, ready)
 			}
 		}
 	}
@@ -157,28 +126,22 @@ func GetReadyNamespaces(ctx context.Context, cl client.Client, pool string) ([]c
 }
 
 func CheckReadyStatus(pool string, ns core.Namespace, ready []core.Namespace) []core.Namespace {
-	if val := ns.ObjectMeta.Labels["pool"]; val == pool {
-		if val, ok := ns.ObjectMeta.Annotations["env-status"]; ok && val == "ready" {
+	if val := ns.ObjectMeta.Labels[LABEL_POOL]; val == pool {
+		if val, ok := ns.ObjectMeta.Annotations[ANNOTATION_ENV_STATUS]; ok && val == ENV_STATUS_READY {
 			ready = append(ready, ns)
 		}
 	}
 
 	return ready
 }
 
-func UpdateAnnotations(ctx context.Context, cl client.Client, annotations map[string]string, nsName string) error {
+func UpdateAnnotations(ctx context.Context, cl client.Client, nsName string, annotations map[string]string) error {
 	ns, err := GetNamespace(ctx, cl, nsName)
 	if err != nil {
 		return err
 	}
 
-	if len(ns.Annotations) == 0 {
-		ns.SetAnnotations(annotations)
-	} else {
-		for k, v := range annotations {
-			ns.Annotations[k] = v
-		}
-	}
+	utils.UpdateAnnotations(&ns, annotations)
 
 	if err := cl.Update(ctx, &ns); err != nil {
 		return err
@@ -189,22 +152,22 @@ func UpdateAnnotations(ctx context.Context, cl client.Client, annotations map[st
 
 func CopySecrets(ctx context.Context, cl client.Client, nsName string) error {
 	secrets := core.SecretList{}
-	if err := cl.List(ctx, &secrets, client.InNamespace("ephemeral-base")); err != nil {
+	if err := cl.List(ctx, &secrets, client.InNamespace(NAMESPACE_EPHEMERAL_BASE)); err != nil {
 		return err
 	}
 
 	for _, secret := range secrets.Items {
 		// Filter which secrets should be copied
 		// All secrets with the "qontract" annotations are defined in app-interface
-		if val, ok := secret.Annotations["qontract.integration"]; !ok {
+		if val, ok := secret.Annotations[QONTRACT_INTEGRATION_SECRET]; !ok {
 			continue
 		} else {
-			if val != "openshift-vault-secrets" {
+			if val != OPENSHIFT_VAULT_SECRETS_SECRET {
 				continue
 			}
 		}
 
-		if val, ok := secret.Annotations["bonfire.ignore"]; ok {
+		if val, ok := secret.Annotations[BONFIRE_IGNORE_SECRET]; ok {
 			if val == "true" {
 				continue
 			}
@@ -234,7 +197,7 @@ func CopySecrets(ctx context.Context, cl client.Client, nsName string) error {
 }
 
 func DeleteNamespace(ctx context.Context, cl client.Client, nsName string) error {
-	UpdateAnnotations(ctx, cl, map[string]string{"env-status": "deleting"}, nsName)
+	UpdateAnnotations(ctx, cl, nsName, AnnotationEnvDeleting.ToMap())
 
 	ns, err := GetNamespace(ctx, cl, nsName)
 	if err != nil {

controllers/cloud.redhat.com/helpers/types.go

@@ -0,0 +1,43 @@
+package helpers
+
+type CustomAnnotation struct {
+	Annotation string
+	Value      string
+}
+
+type CustomLabel struct {
+	Label string
+	Value string
+}
+
+func CreateInitialAnnotations() map[string]string {
+	return map[string]string{
+		ANNOTATION_ENV_STATUS: ENV_STATUS_CREATING,
+		ANNOTATION_RESERVED:   FALSE_VALUE,
+	}
+}
+
+func CreateInitialLabels(poolName string) map[string]string {
+	return map[string]string{
+		LABEL_OPERATOR_NS: TRUE_VALUE,
+		LABEL_POOL:        poolName,
+	}
+}
+
+func (a *CustomAnnotation) ToMap() map[string]string {
+	return map[string]string{a.Annotation: a.Value}
+}
+
+func (l *CustomLabel) ToMap() map[string]string {
+	return map[string]string{l.Label: l.Value}
+}
+
+var AnnotationEnvReady = CustomAnnotation{Annotation: ANNOTATION_ENV_STATUS, Value: ENV_STATUS_READY}
+var AnnotationEnvCreating = CustomAnnotation{Annotation: ANNOTATION_ENV_STATUS, Value: ENV_STATUS_CREATING}
+var AnnotationEnvError = CustomAnnotation{Annotation: ANNOTATION_ENV_STATUS, Value: ENV_STATUS_ERROR}
+var AnnotationEnvDeleting = CustomAnnotation{Annotation: ANNOTATION_ENV_STATUS, Value: ENV_STATUS_DELETING}
+
+var AnnotationReservedTrue = CustomAnnotation{Annotation: ANNOTATION_RESERVED, Value: TRUE_VALUE}
+var AnnotationReservedFalse = CustomAnnotation{Annotation: ANNOTATION_RESERVED, Value: FALSE_VALUE}
+
+var LabelOperatorNamespaceTrue = CustomLabel{Label: LABEL_OPERATOR_NS, Value: TRUE_VALUE}