Address review comments

api/v1alpha1/backstage_types.go

@@ -48,8 +48,8 @@ type Database struct {
 	//+kubebuilder:default=true
 	EnableLocalDb *bool `json:"enableLocalDb,omitempty"`
 
-	// Name of the secret for database authentication. Required for external database access.
-	// Optional for a local database (EnableLocalDb=true) and if absent a secret will be auto generated.
+	// Name of the secret for database authentication. Optional.
+	// For a local database deployment (EnableLocalDb=true), a secret will be auto generated if it does not exist.
 	// The secret shall include information used for the database access.
 	// An example for PostgreSQL DB access:
 	// "POSTGRES_PASSWORD": "rl4s3Fh4ng3M4"
@@ -100,7 +100,7 @@ type Application struct {
 
 	// Image Pull Secrets to use in all containers (including Init Containers)
 	// +optional
-	ImagePullSecrets []string `json:"imagePullSecrets,omitempty"`
+	ImagePullSecrets *[]string `json:"imagePullSecrets,omitempty"`
 
 	// Route configuration. Used for OpenShift only.
 	Route *Route `json:"route,omitempty"`

bundle/manifests/backstage-operator.clusterserviceversion.yaml

@@ -21,7 +21,7 @@ metadata:
         }
       ]
     capabilities: Basic Install
-    createdAt: "2024-01-08T02:20:13Z"
+    createdAt: "2024-01-08T19:47:18Z"
     operators.operatorframework.io/builder: operator-sdk-v1.33.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
   name: backstage-operator.v0.0.1

bundle/manifests/janus-idp.io_backstages.yaml

@@ -267,14 +267,13 @@ spec:
                 properties:
                   authSecretName:
                     description: 'Name of the secret for database authentication.
-                      Required for external database access. Optional for a local
-                      database (EnableLocalDb=true) and if absent a secret will be
-                      auto generated. The secret shall include information used for
-                      the database access. An example for PostgreSQL DB access: "POSTGRES_PASSWORD":
-                      "rl4s3Fh4ng3M4" "POSTGRES_PORT": "5432" "POSTGRES_USER": "postgres"
-                      "POSTGRESQL_ADMIN_PASSWORD": "rl4s3Fh4ng3M4" "POSTGRES_HOST":
-                      "backstage-psql-bs1"  # For local database, set to "backstage-psql-<CR
-                      name>".'
+                      Optional. For a local database deployment (EnableLocalDb=true),
+                      a secret will be auto generated if it does not exist. The secret
+                      shall include information used for the database access. An example
+                      for PostgreSQL DB access: "POSTGRES_PASSWORD": "rl4s3Fh4ng3M4"
+                      "POSTGRES_PORT": "5432" "POSTGRES_USER": "postgres" "POSTGRESQL_ADMIN_PASSWORD":
+                      "rl4s3Fh4ng3M4" "POSTGRES_HOST": "backstage-psql-bs1"  # For
+                      local database, set to "backstage-psql-<CR name>".'
                     type: string
                   enableLocalDb:
                     default: true

config/crd/bases/janus-idp.io_backstages.yaml

@@ -268,14 +268,13 @@ spec:
                 properties:
                   authSecretName:
                     description: 'Name of the secret for database authentication.
-                      Required for external database access. Optional for a local
-                      database (EnableLocalDb=true) and if absent a secret will be
-                      auto generated. The secret shall include information used for
-                      the database access. An example for PostgreSQL DB access: "POSTGRES_PASSWORD":
-                      "rl4s3Fh4ng3M4" "POSTGRES_PORT": "5432" "POSTGRES_USER": "postgres"
-                      "POSTGRESQL_ADMIN_PASSWORD": "rl4s3Fh4ng3M4" "POSTGRES_HOST":
-                      "backstage-psql-bs1"  # For local database, set to "backstage-psql-<CR
-                      name>".'
+                      Optional. For a local database deployment (EnableLocalDb=true),
+                      a secret will be auto generated if it does not exist. The secret
+                      shall include information used for the database access. An example
+                      for PostgreSQL DB access: "POSTGRES_PASSWORD": "rl4s3Fh4ng3M4"
+                      "POSTGRES_PORT": "5432" "POSTGRES_USER": "postgres" "POSTGRESQL_ADMIN_PASSWORD":
+                      "rl4s3Fh4ng3M4" "POSTGRES_HOST": "backstage-psql-bs1"  # For
+                      local database, set to "backstage-psql-<CR name>".'
                     type: string
                   enableLocalDb:
                     default: true

controllers/backstage_controller.go

@@ -131,7 +131,7 @@ func (r *BackstageReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			return ctrl.Result{}, fmt.Errorf("failed to sync Database Service: %w", err)
 		}
 		setStatusCondition(&backstage, bs.LocalDbSynced, v1.ConditionTrue, bs.SyncOK, "")
-	} else if isLocalDbDeployed(backstage) { // EnableLocalDb is off but local db has been deployed. Clean up the deployed local db resources
+	} else { // Clean up the deployed local db resources if any
 		if err := r.cleanupLocalDbResources(ctx, backstage); err != nil {
 			setStatusCondition(&backstage, bs.LocalDbSynced, v1.ConditionFalse, bs.SyncFailed, fmt.Sprintf("failed to delete Database Services:%s", err.Error()))
 			return ctrl.Result{}, fmt.Errorf("failed to delete Database Service: %w", err)
@@ -263,28 +263,7 @@ func setStatusCondition(backstage *bs.Backstage, condType string, status v1.Cond
 	})
 }
 
-func isSynced(backstage bs.Backstage) bool {
-	if cond := meta.FindStatusCondition(backstage.Status.Conditions, bs.RuntimeConditionSynced); cond != nil {
-		return cond.Status == v1.ConditionTrue
-	}
-	return false
-}
-
-func isRouteDeployed(backstage bs.Backstage) bool {
-	if cond := meta.FindStatusCondition(backstage.Status.Conditions, bs.RouteSynced); cond != nil {
-		return cond.Status == v1.ConditionTrue && cond.Reason == bs.SyncOK
-	}
-	return false
-}
-
-func isLocalDbDeployed(backstage bs.Backstage) bool {
-	if cond := meta.FindStatusCondition(backstage.Status.Conditions, bs.LocalDbSynced); cond != nil {
-		return cond.Status == v1.ConditionTrue && cond.Reason == bs.SyncOK
-	}
-	return false
-}
-
-// cleanupResource deletes the resource from the cluster if exists
+// cleanupResource deletes the resource that was previously deployed by the operator from the cluster
 func (r *BackstageReconciler) cleanupResource(ctx context.Context, obj client.Object, backstage bs.Backstage) (bool, error) {
 	err := r.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, obj)
 	if err != nil {

controllers/backstage_controller_test.go

@@ -25,6 +25,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/pointer"
@@ -1345,7 +1346,7 @@ plugins: []
 		BeforeEach(func() {
 			backstage = buildBackstageCR(bsv1alpha1.BackstageSpec{
 				Application: &bsv1alpha1.Application{
-					ImagePullSecrets: []string{ips1, ips2},
+					ImagePullSecrets: &[]string{ips1, ips2},
 				},
 			})
 			err := k8sClient.Create(ctx, backstage)
@@ -1507,7 +1508,7 @@ plugins: []
 				verifyBackstageInstance(ctx)
 			})
 		})
-		It("should fail to reconcile a custom resource for default Backstage without existing secret", func() {
+		It("should reconcile a custom resource for default Backstage without existing secret", func() {
 			backstage := buildBackstageCR(bsv1alpha1.BackstageSpec{
 				Database: bsv1alpha1.Database{
 					EnableLocalDb: pointer.Bool(false),
@@ -1526,8 +1527,7 @@ plugins: []
 			_, err = backstageReconciler.Reconcile(ctx, reconcile.Request{
 				NamespacedName: types.NamespacedName{Name: backstageName, Namespace: ns},
 			})
-			Expect(err).Should(HaveOccurred())
-			Expect(err.Error()).Should(ContainSubstring("authSecretName is required if enableLocalDb is false"))
+			Expect(err).Should(Not(HaveOccurred()))
 		})
 	})
 })
@@ -1540,3 +1540,17 @@ func findElementsByPredicate[T any](l []T, predicate func(t T) bool) (result []T
 	}
 	return result
 }
+
+func isLocalDbDeployed(backstage bsv1alpha1.Backstage) bool {
+	if cond := meta.FindStatusCondition(backstage.Status.Conditions, bsv1alpha1.LocalDbSynced); cond != nil {
+		return cond.Status == metav1.ConditionTrue && cond.Reason == bsv1alpha1.SyncOK
+	}
+	return false
+}
+
+func isSynced(backstage bsv1alpha1.Backstage) bool {
+	if cond := meta.FindStatusCondition(backstage.Status.Conditions, bsv1alpha1.RuntimeConditionSynced); cond != nil {
+		return cond.Status == metav1.ConditionTrue
+	}
+	return false
+}

controllers/backstage_deployment.go

@@ -22,7 +22,6 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	bs "janus-idp.io/backstage-operator/api/v1alpha1"
@@ -161,8 +160,11 @@ func (r *BackstageReconciler) reconcileBackstageDeployment(ctx context.Context,
 	return nil
 }
 
-func (r *BackstageReconciler) deploymentObjectMutFun(ctx context.Context, deployment *appsv1.Deployment, backstage bs.Backstage, ns string) controllerutil.MutateFn {
+func (r *BackstageReconciler) deploymentObjectMutFun(ctx context.Context, targetDeployment *appsv1.Deployment, backstage bs.Backstage, ns string) controllerutil.MutateFn {
 	return func() error {
+		deployment := &appsv1.Deployment{}
+		targetDeployment.ObjectMeta.DeepCopyInto(&deployment.ObjectMeta)
+
 		err := r.readConfigMapOrDefault(ctx, backstage.Spec.RawRuntimeConfig.BackstageConfigName, "deployment.yaml", ns, deployment)
 		if err != nil {
 			return fmt.Errorf("failed to read config: %s", err)
@@ -202,6 +204,9 @@ func (r *BackstageReconciler) deploymentObjectMutFun(ctx context.Context, deploy
 				return fmt.Errorf("failed to set owner reference: %s", err)
 			}
 		}
+
+		deployment.ObjectMeta.DeepCopyInto(&targetDeployment.ObjectMeta)
+		deployment.Spec.DeepCopyInto(&targetDeployment.Spec)
 		return nil
 	}
 }
@@ -255,9 +260,6 @@ func (r *BackstageReconciler) addEnvVars(backstage bs.Backstage, ns string, depl
 }
 
 func (r *BackstageReconciler) validateAndUpdatePsqlSecretRef(backstage bs.Backstage, deployment *appsv1.Deployment) error {
-	if len(backstage.Spec.Database.AuthSecretName) == 0 && !pointer.BoolDeref(backstage.Spec.Database.EnableLocalDb, true) {
-		return fmt.Errorf("authSecretName is required if enableLocalDb is false")
-	}
 	for i, c := range deployment.Spec.Template.Spec.Containers {
 		if c.Name != _defaultBackstageMainContainerName {
 			continue
@@ -301,13 +303,14 @@ func (r *BackstageReconciler) applyApplicationParamsFromCR(backstage bs.Backstag
 				container.Image = *backstage.Spec.Application.Image
 			})
 		}
-
-		if len(backstage.Spec.Application.ImagePullSecrets) > 0 { // use image pull secrets from the CR spec
+		if backstage.Spec.Application.ImagePullSecrets != nil { // use image pull secrets from the CR spec
 			deployment.Spec.Template.Spec.ImagePullSecrets = nil
-			for _, imagePullSecret := range backstage.Spec.Application.ImagePullSecrets {
-				deployment.Spec.Template.Spec.ImagePullSecrets = append(deployment.Spec.Template.Spec.ImagePullSecrets, v1.LocalObjectReference{
-					Name: imagePullSecret,
-				})
+			if len(*backstage.Spec.Application.ImagePullSecrets) > 0 {
+				for _, imagePullSecret := range *backstage.Spec.Application.ImagePullSecrets {
+					deployment.Spec.Template.Spec.ImagePullSecrets = append(deployment.Spec.Template.Spec.ImagePullSecrets, v1.LocalObjectReference{
+						Name: imagePullSecret,
+					})
+				}
 			}
 		}
 	}

controllers/backstage_route.go

@@ -38,14 +38,11 @@ func (r *BackstageReconciler) reconcileBackstageRoute(ctx context.Context, backs
 	}
 
 	if !shouldCreateRoute(*backstage) {
-		if isRouteDeployed(*backstage) {
-			deleted, err := r.cleanupResource(ctx, route, *backstage)
-			if err == nil && deleted {
-				setStatusCondition(backstage, bs.RouteSynced, metav1.ConditionTrue, bs.Deleted, "")
-			}
-			return err
+		deleted, err := r.cleanupResource(ctx, route, *backstage)
+		if err == nil && deleted {
+			setStatusCondition(backstage, bs.RouteSynced, metav1.ConditionTrue, bs.Deleted, "")
 		}
-		return nil
+		return err
 	}
 
 	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, route, r.routeObjectMutFun(ctx, route, *backstage, ns)); err != nil {
@@ -59,8 +56,11 @@ func (r *BackstageReconciler) reconcileBackstageRoute(ctx context.Context, backs
 	return nil
 }
 
-func (r *BackstageReconciler) routeObjectMutFun(ctx context.Context, route *openshift.Route, backstage bs.Backstage, ns string) controllerutil.MutateFn {
+func (r *BackstageReconciler) routeObjectMutFun(ctx context.Context, targetRoute *openshift.Route, backstage bs.Backstage, ns string) controllerutil.MutateFn {
 	return func() error {
+		route := &openshift.Route{}
+		targetRoute.ObjectMeta.DeepCopyInto(&route.ObjectMeta)
+
 		err := r.readConfigMapOrDefault(ctx, backstage.Spec.RawRuntimeConfig.BackstageConfigName, "route.yaml", ns, route)
 		if err != nil {
 			return err
@@ -80,6 +80,9 @@ func (r *BackstageReconciler) routeObjectMutFun(ctx context.Context, route *open
 				return fmt.Errorf("failed to set owner reference: %s", err)
 			}
 		}
+
+		route.ObjectMeta.DeepCopyInto(&targetRoute.ObjectMeta)
+		route.Spec.DeepCopyInto(&targetRoute.Spec)
 		return nil
 	}
 }

controllers/backstage_service.go

@@ -44,9 +44,11 @@ func (r *BackstageReconciler) reconcileBackstageService(ctx context.Context, bac
 
 // selector for deploy.spec.template.spec.meta.label
 // targetPort: http for deploy.spec.template.spec.containers.ports.name=http
-func (r *BackstageReconciler) serviceObjectMutFun(ctx context.Context, service *corev1.Service, backstage bs.Backstage, ns string) controllerutil.MutateFn {
+func (r *BackstageReconciler) serviceObjectMutFun(ctx context.Context, targetService *corev1.Service, backstage bs.Backstage, ns string) controllerutil.MutateFn {
 	return func() error {
-		tmp := service.DeepCopy()
+		service := &corev1.Service{}
+		targetService.ObjectMeta.DeepCopyInto(&service.ObjectMeta)
+
 		err := r.readConfigMapOrDefault(ctx, backstage.Spec.RawRuntimeConfig.BackstageConfigName, "service.yaml", ns, service)
 		if err != nil {
 			return err
@@ -64,18 +66,21 @@ func (r *BackstageReconciler) serviceObjectMutFun(ctx context.Context, service *
 			}
 		}
 
-		if len(tmp.Spec.ClusterIP) > 0 && service.Spec.ClusterIP != "" && service.Spec.ClusterIP != "None" && service.Spec.ClusterIP != tmp.Spec.ClusterIP {
-			return fmt.Errorf("backstage service IP can not be updated: %s, %s, %s", tmp.Name, tmp.Spec.ClusterIP, service.Spec.ClusterIP)
+		if len(targetService.Spec.ClusterIP) > 0 && service.Spec.ClusterIP != "" && service.Spec.ClusterIP != "None" && service.Spec.ClusterIP != targetService.Spec.ClusterIP {
+			return fmt.Errorf("backstage service IP can not be updated: %s, current: %s, new: %s", targetService.Name, targetService.Spec.ClusterIP, service.Spec.ClusterIP)
 		}
-		service.Spec.ClusterIP = tmp.Spec.ClusterIP
-		for _, ip1 := range tmp.Spec.ClusterIPs {
+		service.Spec.ClusterIP = targetService.Spec.ClusterIP
+		for _, ip1 := range targetService.Spec.ClusterIPs {
 			for _, ip2 := range service.Spec.ClusterIPs {
 				if len(ip1) > 0 && ip2 != "" && ip2 != "None" && ip1 != ip2 {
-					return fmt.Errorf("backstage service IPs can not be updated: %s, %v, %v", tmp.Name, tmp.Spec.ClusterIPs, service.Spec.ClusterIPs)
+					return fmt.Errorf("backstage service IPs can not be updated: %s, current: %v, new: %v", targetService.Name, targetService.Spec.ClusterIPs, service.Spec.ClusterIPs)
 				}
 			}
 		}
-		service.Spec.ClusterIPs = tmp.Spec.ClusterIPs
+		service.Spec.ClusterIPs = targetService.Spec.ClusterIPs
+
+		service.ObjectMeta.DeepCopyInto(&targetService.ObjectMeta)
+		service.Spec.DeepCopyInto(&targetService.Spec)
 		return nil
 	}
 }

controllers/local_db_services.go

@@ -88,14 +88,15 @@ func (r *BackstageReconciler) reconcilePsqlService(ctx context.Context, backstag
 	return nil
 }
 
-func (r *BackstageReconciler) psqlServiceObjectMutFun(ctx context.Context, service *corev1.Service, backstage bs.Backstage, label, ns, key string) controllerutil.MutateFn {
+func (r *BackstageReconciler) psqlServiceObjectMutFun(ctx context.Context, targetService *corev1.Service, backstage bs.Backstage, label, ns, key string) controllerutil.MutateFn {
 	return func() error {
-		tmp := service.DeepCopy()
+		service := &corev1.Service{}
+		targetService.ObjectMeta.DeepCopyInto(&service.ObjectMeta)
 		err := r.readConfigMapOrDefault(ctx, backstage.Spec.RawRuntimeConfig.LocalDbConfigName, key, ns, service)
 		if err != nil {
 			return err
 		}
-		service.SetName(tmp.Name)
+		service.SetName(targetService.Name)
 		setBackstageLocalDbLabel(&service.ObjectMeta.Labels, label)
 		setBackstageLocalDbLabel(&service.Spec.Selector, label)
 
@@ -104,19 +105,21 @@ func (r *BackstageReconciler) psqlServiceObjectMutFun(ctx context.Context, servi
 				return fmt.Errorf(ownerRefFmt, err)
 			}
 		}
-		if len(tmp.Spec.ClusterIP) > 0 && service.Spec.ClusterIP != "" && service.Spec.ClusterIP != "None" && service.Spec.ClusterIP != tmp.Spec.ClusterIP {
-			return fmt.Errorf("db service IP can not be updated: %s, %s, %s", tmp.Name, tmp.Spec.ClusterIP, service.Spec.ClusterIP)
+		if len(targetService.Spec.ClusterIP) > 0 && service.Spec.ClusterIP != "" && service.Spec.ClusterIP != "None" && service.Spec.ClusterIP != targetService.Spec.ClusterIP {
+			return fmt.Errorf("db service IP can not be updated: %s, %s, %s", targetService.Name, targetService.Spec.ClusterIP, service.Spec.ClusterIP)
 		}
-		service.Spec.ClusterIP = tmp.Spec.ClusterIP
-		for _, ip1 := range tmp.Spec.ClusterIPs {
+		service.Spec.ClusterIP = targetService.Spec.ClusterIP
+		for _, ip1 := range targetService.Spec.ClusterIPs {
 			for _, ip2 := range service.Spec.ClusterIPs {
 				if len(ip1) > 0 && ip2 != "" && ip2 != "None" && ip1 != ip2 {
-					return fmt.Errorf("db service IPs can not be updated: %s, %v, %v", tmp.Name, tmp.Spec.ClusterIPs, service.Spec.ClusterIPs)
+					return fmt.Errorf("db service IPs can not be updated: %s, %v, %v", targetService.Name, targetService.Spec.ClusterIPs, service.Spec.ClusterIPs)
 				}
 			}
 		}
-		service.Spec.ClusterIPs = tmp.Spec.ClusterIPs
+		service.Spec.ClusterIPs = targetService.Spec.ClusterIPs
 
+		service.ObjectMeta.DeepCopyInto(&targetService.ObjectMeta)
+		service.Spec.DeepCopyInto(&targetService.Spec)
 		return nil
 	}
 }

controllers/local_db_statefulset.go

@@ -153,8 +153,10 @@ func (r *BackstageReconciler) reconcileLocalDbStatefulSet(ctx context.Context, b
 	return nil
 }
 
-func (r *BackstageReconciler) localDBStatefulSetMutFun(ctx context.Context, statefulSet *appsv1.StatefulSet, backstage bs.Backstage, ns string) controllerutil.MutateFn {
+func (r *BackstageReconciler) localDBStatefulSetMutFun(ctx context.Context, targetStatefulSet *appsv1.StatefulSet, backstage bs.Backstage, ns string) controllerutil.MutateFn {
 	return func() error {
+		statefulSet := &appsv1.StatefulSet{}
+		targetStatefulSet.ObjectMeta.DeepCopyInto(&statefulSet.ObjectMeta)
 		err := r.readConfigMapOrDefault(ctx, backstage.Spec.RawRuntimeConfig.LocalDbConfigName, "db-statefulset.yaml", ns, statefulSet)
 		if err != nil {
 			return err
@@ -194,6 +196,9 @@ func (r *BackstageReconciler) localDBStatefulSetMutFun(ctx context.Context, stat
 				return fmt.Errorf(ownerRefFmt, err)
 			}
 		}
+
+		statefulSet.ObjectMeta.DeepCopyInto(&targetStatefulSet.ObjectMeta)
+		statefulSet.Spec.DeepCopyInto(&targetStatefulSet.Spec)
 		return nil
 	}
 }