Optimize pod-refresh-on-external-config preprocessing and tests

controllers/backstage_controller.go

@@ -301,7 +301,7 @@ func (r *BackstageReconciler) requestByLabel(ctx context.Context, object client.
 
 	backstageName := object.GetAnnotations()[model.BackstageNameAnnotation]
 	if backstageName == "" {
-		lg.V(1).Info(fmt.Sprintf("warning: %s annotation is not defined for %s, Backstage instances will not be reconciled in this loop", model.BackstageNameAnnotation, object.GetName()))
+		//lg.V(1).Info(fmt.Sprintf("warning: %s annotation is not defined for %s, Backstage instances will not be reconciled in this loop", model.BackstageNameAnnotation, object.GetName()))
 		return []reconcile.Request{}
 	}
 

controllers/spec_preprocessor.go

@@ -20,6 +20,8 @@ import (
 	"os"
 	"strconv"
 
+	"k8s.io/client-go/util/retry"
+
 	"k8s.io/apimachinery/pkg/api/errors"
 
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -143,40 +145,47 @@ func (r *BackstageReconciler) addExtConfig(config *model.ExternalConfig, ctx con
 
 	lg := log.FromContext(ctx)
 
-	if err := r.Get(ctx, types.NamespacedName{Name: objectName, Namespace: ns}, obj); err != nil {
-		if _, ok := obj.(*corev1.Secret); ok && errors.IsForbidden(err) {
-			return fmt.Errorf("warning: Secrets GET is forbidden, updating Secrets may not cause Pod recreating")
+	// use RetryOnConflict to avoid possible Conflict error which may be caused mostly by other call of this function
+	// https://pkg.go.dev/k8s.io/client-go/util/retry#RetryOnConflict.
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+
+		if err := r.Get(ctx, types.NamespacedName{Name: objectName, Namespace: ns}, obj); err != nil {
+			if _, ok := obj.(*corev1.Secret); ok && errors.IsForbidden(err) {
+				return fmt.Errorf("warning: Secrets GET is forbidden, updating Secrets may not cause Pod recreating")
+			}
+			return fmt.Errorf("failed to get external config from %s: %s", objectName, err)
 		}
-		return fmt.Errorf("failed to get external config from %s: %s", objectName, err)
-	}
 
-	if err := config.AddToSyncedConfig(obj); err != nil {
-		return fmt.Errorf("failed to add to synced %s: %s", obj.GetName(), err)
-	}
+		if err := config.AddToSyncedConfig(obj); err != nil {
+			return fmt.Errorf("failed to add to synced %s: %s", obj.GetName(), err)
+		}
 
-	if obj.GetLabels() == nil {
-		obj.SetLabels(map[string]string{})
-	}
-	if obj.GetAnnotations() == nil {
-		obj.SetAnnotations(map[string]string{})
-	}
+		if obj.GetLabels() == nil {
+			obj.SetLabels(map[string]string{})
+		}
+		if obj.GetAnnotations() == nil {
+			obj.SetAnnotations(map[string]string{})
+		}
 
-	autoSync := true
-	autoSyncStr, ok := os.LookupEnv(AutoSyncEnvVar)
-	if ok {
-		autoSync, _ = strconv.ParseBool(autoSyncStr)
-	}
+		autoSync := true
+		autoSyncStr, ok := os.LookupEnv(AutoSyncEnvVar)
+		if ok {
+			autoSync, _ = strconv.ParseBool(autoSyncStr)
+		}
 
-	if obj.GetLabels()[model.ExtConfigSyncLabel] == "" || obj.GetAnnotations()[model.BackstageNameAnnotation] == "" ||
-		obj.GetLabels()[model.ExtConfigSyncLabel] != strconv.FormatBool(autoSync) {
+		if obj.GetLabels()[model.ExtConfigSyncLabel] == "" || obj.GetAnnotations()[model.BackstageNameAnnotation] == "" ||
+			obj.GetLabels()[model.ExtConfigSyncLabel] != strconv.FormatBool(autoSync) {
 
-		obj.GetLabels()[model.ExtConfigSyncLabel] = strconv.FormatBool(autoSync)
-		obj.GetAnnotations()[model.BackstageNameAnnotation] = backstageName
-		if err := r.Update(ctx, obj); err != nil {
-			return fmt.Errorf("failed to update external config %s: %s", objectName, err)
+			obj.GetLabels()[model.ExtConfigSyncLabel] = strconv.FormatBool(autoSync)
+			obj.GetAnnotations()[model.BackstageNameAnnotation] = backstageName
+			if err := r.Update(ctx, obj); err != nil {
+				return err
+			}
+			lg.V(1).Info(fmt.Sprintf("update external config %s with label %s=%s and annotation %s=%s", obj.GetName(), model.ExtConfigSyncLabel, strconv.FormatBool(autoSync), model.BackstageNameAnnotation, backstageName))
 		}
-		lg.V(1).Info(fmt.Sprintf("update external config %s with label %s=%s and annotation %s=%s", obj.GetName(), model.ExtConfigSyncLabel, strconv.FormatBool(autoSync), model.BackstageNameAnnotation, backstageName))
-	}
 
-	return nil
+		return nil
+
+	})
+	return err
 }

integration_tests/config-refresh_test.go

@@ -64,7 +64,12 @@ var _ = When("create backstage with external configuration", func() {
 
 		backstageName := generateRandName("")
 
-		generateConfigMap(ctx, k8sClient, appConfig1, ns, map[string]string{"key11": "app:", "key12": "app:"}, nil, nil)
+		conf := `
+organization:
+  name: "my org"
+`
+
+		generateConfigMap(ctx, k8sClient, appConfig1, ns, map[string]string{"appconfig11": conf}, nil, nil)
 		generateSecret(ctx, k8sClient, secretEnv1, ns, map[string]string{"sec11": "val11"}, nil, nil)
 
 		bs := bsv1.BackstageSpec{
@@ -96,43 +101,48 @@ var _ = When("create backstage with external configuration", func() {
 
 			g.Expect(len(podList.Items)).To(Equal(1))
 			podName := podList.Items[0].Name
-			out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/key11")
+			out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/appconfig11")
 			g.Expect(err).ShouldNot(HaveOccurred())
-			g.Expect(out).To(Equal("app:"))
+			out = strings.Replace(out, "\r", "", -1)
+			g.Expect(out).To(Equal(conf))
 
 			out, _, err = executeRemoteCommand(ctx, ns, podName, "backstage-backend", "echo $sec11")
 			g.Expect(err).ShouldNot(HaveOccurred())
 			g.Expect("val11\r\n").To(Equal(out))
 
-		}, 10*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage())
+		}, 5*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage())
 
 		cm := &corev1.ConfigMap{}
 		err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: appConfig1}, cm)
 		Expect(err).ShouldNot(HaveOccurred())
 
-		newData := "app:\n  backend:"
-		cm.Data = map[string]string{"key11": newData}
+		// update appconfig11
+		newData := `
+organization:
+  name: "another org"
+`
+		cm.Data = map[string]string{"appconfig11": newData}
 		err = k8sClient.Update(ctx, cm)
 		Expect(err).ShouldNot(HaveOccurred())
 
 		Eventually(func(g Gomega) {
 			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: appConfig1}, cm)
 			g.Expect(err).ShouldNot(HaveOccurred())
-			g.Expect(cm.Data["key11"]).To(Equal(newData))
+			g.Expect(cm.Data["appconfig11"]).To(Equal(newData))
 
 			// Pod replaced so have to re-ask
 			podList := &corev1.PodList{}
 			err = k8sClient.List(ctx, podList, client.InNamespace(ns), client.MatchingLabels{model.BackstageAppLabel: utils.BackstageAppLabelValue(backstageName)})
 			g.Expect(err).ShouldNot(HaveOccurred())
 
 			podName := podList.Items[0].Name
-			out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/key11")
+			out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/appconfig11")
 			g.Expect(err).ShouldNot(HaveOccurred())
 			// TODO nicer method to compare file content with added '\r'
 			g.Expect(strings.ReplaceAll(out, "\r", "")).To(Equal(newData))
 
-			_, _, err = executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/key12")
-			g.Expect(err).Should(HaveOccurred())
+			//_, _, err = executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/key12")
+			//g.Expect(err).Should(HaveOccurred())
 
 		}, 10*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage())
 
@@ -146,6 +156,9 @@ var _ = When("create backstage with external configuration", func() {
 
 		Eventually(func(g Gomega) {
 
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: secretEnv1}, sec)
+			g.Expect(err).ShouldNot(HaveOccurred())
+
 			// Pod replaced so have to re-ask
 			podList := &corev1.PodList{}
 			err = k8sClient.List(ctx, podList, client.InNamespace(ns), client.MatchingLabels{model.BackstageAppLabel: utils.BackstageAppLabelValue(backstageName)})

integration_tests/rhdh-config_test.go

@@ -60,7 +60,7 @@ var _ = When("create default backstage", func() {
 			g.Expect(initCont.VolumeMounts[1].MountPath).To(Equal("/opt/app-root/src/.npmrc.dynamic-plugins"))
 			g.Expect(initCont.VolumeMounts[1].SubPath).To(Equal(".npmrc"))
 			g.Expect(initCont.VolumeMounts[2].MountPath).To(Equal("/opt/app-root/src/.npm/_cacache"))
-			g.Expect(initCont.VolumeMounts[2].SubPath).To(BeEmpty())	
+			g.Expect(initCont.VolumeMounts[2].SubPath).To(BeEmpty())
 			g.Expect(initCont.VolumeMounts[3].MountPath).To(Equal("/opt/app-root/src/dynamic-plugins.yaml"))
 			g.Expect(initCont.VolumeMounts[3].SubPath).To(Equal("dynamic-plugins.yaml"))
 			g.Expect(initCont.VolumeMounts[3].Name).

integration_tests/utils.go

@@ -49,13 +49,6 @@ func generateConfigMap(ctx context.Context, k8sClient client.Client, name string
 }
 
 func generateSecret(ctx context.Context, k8sClient client.Client, name, namespace string, data, labels, annotations map[string]string) string {
-	if data == nil {
-		data = map[string]string{}
-	}
-
-	for _, v := range data {
-		data[v] = fmt.Sprintf("value-%s", v)
-	}
 	Expect(k8sClient.Create(ctx, &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        name,

pkg/model/externalconfig.go

@@ -61,10 +61,6 @@ func (e *ExternalConfig) GetHash() string {
 
 func (e *ExternalConfig) AddToSyncedConfig(content client.Object) error {
 
-	if content.GetLabels()[ExtConfigSyncLabel] == "" || content.GetAnnotations()[BackstageNameAnnotation] == "" {
-		return nil
-	}
-
 	d, err := json.Marshal(content)
 	if err != nil {
 		return err