From 0c9d956a2b2b86814bdab1dde265985f362d5f9b Mon Sep 17 00:00:00 2001 From: gazarenkov Date: Tue, 23 Jul 2024 08:42:51 +0300 Subject: [PATCH 1/3] retry on conflict CM/Secret update Signed-off-by: gazarenkov --- controllers/backstage_controller.go | 2 +- controllers/spec_preprocessor.go | 65 ++++++++++++++---------- integration_tests/config-refresh_test.go | 6 ++- integration_tests/rhdh-config_test.go | 2 +- 4 files changed, 43 insertions(+), 32 deletions(-) diff --git a/controllers/backstage_controller.go b/controllers/backstage_controller.go index 5c14fc6a..2102d103 100644 --- a/controllers/backstage_controller.go +++ b/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{} } diff --git a/controllers/spec_preprocessor.go b/controllers/spec_preprocessor.go index fd52f545..38a74cd2 100644 --- a/controllers/spec_preprocessor.go +++ b/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 } diff --git a/integration_tests/config-refresh_test.go b/integration_tests/config-refresh_test.go index 92fed3e8..015850ff 100644 --- a/integration_tests/config-refresh_test.go +++ b/integration_tests/config-refresh_test.go @@ -64,7 +64,7 @@ var _ = When("create backstage with external configuration", func() { backstageName := generateRandName("") - generateConfigMap(ctx, k8sClient, appConfig1, ns, map[string]string{"key11": "app:", "key12": "app:"}, nil, nil) + generateConfigMap(ctx, k8sClient, appConfig1, ns, map[string]string{"key11": "app:", "key12": "app:", "app": "myApp"}, nil, nil) generateSecret(ctx, k8sClient, secretEnv1, ns, map[string]string{"sec11": "val11"}, nil, nil) bs := bsv1.BackstageSpec{ @@ -94,7 +94,9 @@ var _ = When("create backstage with external configuration", func() { err = k8sClient.List(ctx, podList, client.InNamespace(ns), client.MatchingLabels{model.BackstageAppLabel: utils.BackstageAppLabelValue(backstageName)}) g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(len(podList.Items)).To(Equal(1)) + // + //g.Expect(len(podList.Items)).To(Equal(1)) + g.Expect(len(podList.Items) >= 1).Should(BeTrue()) podName := podList.Items[0].Name out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/key11") g.Expect(err).ShouldNot(HaveOccurred()) diff --git a/integration_tests/rhdh-config_test.go b/integration_tests/rhdh-config_test.go index 18175958..53450a91 100644 --- a/integration_tests/rhdh-config_test.go +++ b/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). From fd951f7f3015b8093e8345f83e5c92fb3b45f4bb Mon Sep 17 00:00:00 2001 From: gazarenkov Date: Mon, 29 Jul 2024 13:54:57 +0300 Subject: [PATCH 2/3] remove check for label/anno in AddToSyncedConfig, fix test Signed-off-by: gazarenkov --- integration_tests/config-refresh_test.go | 166 ----------------------- integration_tests/utils.go | 7 - pkg/model/externalconfig.go | 4 - 3 files changed, 177 deletions(-) delete mode 100644 integration_tests/config-refresh_test.go diff --git a/integration_tests/config-refresh_test.go b/integration_tests/config-refresh_test.go deleted file mode 100644 index 015850ff..00000000 --- a/integration_tests/config-refresh_test.go +++ /dev/null @@ -1,166 +0,0 @@ -// -// Copyright (c) 2023 Red Hat, Inc. -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package integration_tests - -import ( - "context" - "fmt" - "redhat-developer/red-hat-developer-hub-operator/pkg/utils" - "strings" - "time" - - "sigs.k8s.io/controller-runtime/pkg/client" - - corev1 "k8s.io/api/core/v1" - - appsv1 "k8s.io/api/apps/v1" - - "redhat-developer/red-hat-developer-hub-operator/pkg/model" - - bsv1 "redhat-developer/red-hat-developer-hub-operator/api/v1alpha2" - - "k8s.io/apimachinery/pkg/types" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = When("create backstage with external configuration", func() { - - var ( - ctx context.Context - ns string - ) - - BeforeEach(func() { - ctx = context.Background() - ns = createNamespace(ctx) - }) - - AfterEach(func() { - deleteNamespace(ctx, ns) - }) - - It("refresh config", func() { - - if !*testEnv.UseExistingCluster { - Skip("Skipped for not real cluster") - } - - appConfig1 := "app-config1" - secretEnv1 := "secret-env1" - - backstageName := generateRandName("") - - generateConfigMap(ctx, k8sClient, appConfig1, ns, map[string]string{"key11": "app:", "key12": "app:", "app": "myApp"}, nil, nil) - generateSecret(ctx, k8sClient, secretEnv1, ns, map[string]string{"sec11": "val11"}, nil, nil) - - bs := bsv1.BackstageSpec{ - Application: &bsv1.Application{ - AppConfig: &bsv1.AppConfig{ - MountPath: "/my/mount/path", - ConfigMaps: []bsv1.ObjectKeyRef{ - {Name: appConfig1}, - }, - }, - ExtraEnvs: &bsv1.ExtraEnvs{ - Secrets: []bsv1.ObjectKeyRef{ - {Name: secretEnv1, Key: "sec11"}, - }, - }, - }, - } - - createAndReconcileBackstage(ctx, ns, bs, backstageName) - - Eventually(func(g Gomega) { - deploy := &appsv1.Deployment{} - err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DeploymentName(backstageName)}, deploy) - g.Expect(err).ShouldNot(HaveOccurred()) - - podList := &corev1.PodList{} - err = k8sClient.List(ctx, podList, client.InNamespace(ns), client.MatchingLabels{model.BackstageAppLabel: utils.BackstageAppLabelValue(backstageName)}) - g.Expect(err).ShouldNot(HaveOccurred()) - - // - //g.Expect(len(podList.Items)).To(Equal(1)) - g.Expect(len(podList.Items) >= 1).Should(BeTrue()) - podName := podList.Items[0].Name - out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "cat /my/mount/path/key11") - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(out).To(Equal("app:")) - - 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()) - - 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} - 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)) - - // 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") - 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()) - - }, 10*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage()) - - sec := &corev1.Secret{} - err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: secretEnv1}, sec) - Expect(err).ShouldNot(HaveOccurred()) - newEnv := "val22" - sec.StringData = map[string]string{"sec11": newEnv} - err = k8sClient.Update(ctx, sec) - Expect(err).ShouldNot(HaveOccurred()) - - Eventually(func(g Gomega) { - - // 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", "echo $sec11") - g.Expect(err).ShouldNot(HaveOccurred()) - g.Expect(fmt.Sprintf("%s%s", newEnv, "\r\n")).To(Equal(out)) - - }, 10*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage()) - - }) - -}) diff --git a/integration_tests/utils.go b/integration_tests/utils.go index 1d99443e..6e1cb1ca 100644 --- a/integration_tests/utils.go +++ b/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, diff --git a/pkg/model/externalconfig.go b/pkg/model/externalconfig.go index 13843d76..5cc0a7c8 100644 --- a/pkg/model/externalconfig.go +++ b/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 From a471f3f209d825908bb69d6fc4fc2c9b3410509c Mon Sep 17 00:00:00 2001 From: gazarenkov Date: Mon, 29 Jul 2024 14:44:47 +0300 Subject: [PATCH 3/3] remove check for label/anno in AddToSyncedConfig, fix test Signed-off-by: gazarenkov --- integration_tests/config-refresh_test.go | 177 +++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 integration_tests/config-refresh_test.go diff --git a/integration_tests/config-refresh_test.go b/integration_tests/config-refresh_test.go new file mode 100644 index 00000000..467bfee4 --- /dev/null +++ b/integration_tests/config-refresh_test.go @@ -0,0 +1,177 @@ +// +// Copyright (c) 2023 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package integration_tests + +import ( + "context" + "fmt" + "redhat-developer/red-hat-developer-hub-operator/pkg/utils" + "strings" + "time" + + "sigs.k8s.io/controller-runtime/pkg/client" + + corev1 "k8s.io/api/core/v1" + + appsv1 "k8s.io/api/apps/v1" + + "redhat-developer/red-hat-developer-hub-operator/pkg/model" + + bsv1 "redhat-developer/red-hat-developer-hub-operator/api/v1alpha2" + + "k8s.io/apimachinery/pkg/types" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = When("create backstage with external configuration", func() { + + var ( + ctx context.Context + ns string + ) + + BeforeEach(func() { + ctx = context.Background() + ns = createNamespace(ctx) + }) + + AfterEach(func() { + deleteNamespace(ctx, ns) + }) + + It("refresh config", func() { + + if !*testEnv.UseExistingCluster { + Skip("Skipped for not real cluster") + } + + appConfig1 := "app-config1" + secretEnv1 := "secret-env1" + + backstageName := generateRandName("") + + 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{ + Application: &bsv1.Application{ + AppConfig: &bsv1.AppConfig{ + MountPath: "/my/mount/path", + ConfigMaps: []bsv1.ObjectKeyRef{ + {Name: appConfig1}, + }, + }, + ExtraEnvs: &bsv1.ExtraEnvs{ + Secrets: []bsv1.ObjectKeyRef{ + {Name: secretEnv1, Key: "sec11"}, + }, + }, + }, + } + + createAndReconcileBackstage(ctx, ns, bs, backstageName) + + Eventually(func(g Gomega) { + deploy := &appsv1.Deployment{} + err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DeploymentName(backstageName)}, deploy) + g.Expect(err).ShouldNot(HaveOccurred()) + + podList := &corev1.PodList{} + err = k8sClient.List(ctx, podList, client.InNamespace(ns), client.MatchingLabels{model.BackstageAppLabel: utils.BackstageAppLabelValue(backstageName)}) + g.Expect(err).ShouldNot(HaveOccurred()) + + 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/appconfig11") + g.Expect(err).ShouldNot(HaveOccurred()) + 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)) + + }, 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()) + + // 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["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/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()) + + }, 10*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage()) + + sec := &corev1.Secret{} + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: secretEnv1}, sec) + Expect(err).ShouldNot(HaveOccurred()) + newEnv := "val22" + sec.StringData = map[string]string{"sec11": newEnv} + err = k8sClient.Update(ctx, sec) + Expect(err).ShouldNot(HaveOccurred()) + + 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)}) + g.Expect(err).ShouldNot(HaveOccurred()) + + podName := podList.Items[0].Name + + out, _, err := executeRemoteCommand(ctx, ns, podName, "backstage-backend", "echo $sec11") + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(fmt.Sprintf("%s%s", newEnv, "\r\n")).To(Equal(out)) + + }, 10*time.Minute, 10*time.Second).Should(Succeed(), controllerMessage()) + + }) + +})