From 1da6ad4a648641e478a29b12ed6544929156a92c Mon Sep 17 00:00:00 2001 From: Michele Baldessari Date: Tue, 22 Sep 2026 10:13:10 +0200 Subject: [PATCH] Fix singleargo secret copying To do this we actually parse the set of value files (and extraParams) and we verify if global.singleArgoCD is true or not. If it is true there is no need to copy the git secret to the namespaced argo instance as there is only a single "clusterwide" argo instance. Tested as follows: 1. No secrets public repo single argo -> OK 2. No secrets public repo multi argo -> OK 3. One secret private repo single argo -> OK ``` ./pattern.sh make TARGET_ORIGIN=mcg-private-http TOKEN_SECRET=private-repo TOKEN_NAMESPACE=openshift-operators install ``` 4. One secret private repo multi argo -> OK ``` ./pattern.sh make TARGET_ORIGIN=mcg-private-http TOKEN_SECRET=private-repo TOKEN_NAMESPACE=openshift-operators install ``` Fixes: #838 --- internal/controller/pattern_controller.go | 20 ++- .../controller/pattern_controller_test.go | 129 ++++++++++++++++++ internal/controller/values.go | 39 ++++++ internal/controller/values_test.go | 83 +++++++++++ 4 files changed, 266 insertions(+), 5 deletions(-) diff --git a/internal/controller/pattern_controller.go b/internal/controller/pattern_controller.go index f6f21910d..2292d916a 100644 --- a/internal/controller/pattern_controller.go +++ b/internal/controller/pattern_controller.go @@ -244,7 +244,7 @@ func (r *PatternReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return result, argoErr } - // Copy the bootstrap secret to the namespaced argo namespace + // Copy the bootstrap secret to the clusterwide argo namespace if qualifiedInstance.Spec.GitConfig.TokenSecret != "" { if err = r.copyAuthGitSecret(qualifiedInstance.Spec.GitConfig.TokenSecretNamespace, qualifiedInstance.Spec.GitConfig.TokenSecret, getClusterWideArgoNamespace(), "vp-private-repo-credentials"); err != nil { @@ -280,11 +280,21 @@ func (r *PatternReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return result, appErr } - // Copy the bootstrap secret to the namespaced argo namespace + // Copy the bootstrap secret to the per-pattern namespaced argo namespace. + // Under singleArgoCD the application lives in the clusterwide ArgoCD namespace + // (which already received the secret above), so skip this copy. if qualifiedInstance.Spec.GitConfig.TokenSecret != "" { - if err = r.copyAuthGitSecret(qualifiedInstance.Spec.GitConfig.TokenSecretNamespace, - qualifiedInstance.Spec.GitConfig.TokenSecret, applicationName(qualifiedInstance), "vp-private-repo-credentials"); err != nil { - return r.actionPerformed(qualifiedInstance, "copying clusterwide git auth secret to namespaced argo", err) + singleArgo := false + if mergedValues, valErr := getPatternMergedValues(qualifiedInstance); valErr == nil { + if v := getGlobalValue("singleArgoCD", mergedValues); v != nil { + singleArgo = v == true || v == boolTrue + } + } + if !singleArgo { + if err = r.copyAuthGitSecret(qualifiedInstance.Spec.GitConfig.TokenSecretNamespace, + qualifiedInstance.Spec.GitConfig.TokenSecret, applicationName(qualifiedInstance), "vp-private-repo-credentials"); err != nil { + return r.actionPerformed(qualifiedInstance, "copying git auth secret to namespaced argo", err) + } } } // Perform validation of the site values file(s) diff --git a/internal/controller/pattern_controller_test.go b/internal/controller/pattern_controller_test.go index 2f2f122eb..472ef7854 100644 --- a/internal/controller/pattern_controller_test.go +++ b/internal/controller/pattern_controller_test.go @@ -32,6 +32,7 @@ import ( operatorclient "github.com/openshift/client-go/operator/clientset/versioned/fake" olmclient "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" gomock "go.uber.org/mock/gomock" + "gopkg.in/yaml.v3" kubeclient "k8s.io/client-go/kubernetes/fake" @@ -619,6 +620,134 @@ var _ = Describe("pattern controller - buildPatternManifest helpers", func() { }) }) +var _ = Describe("pattern controller - singleArgoCD secret copy", func() { + const ( + srcNamespace = "openshift-operators" + srcSecretName = "private-repo" + destSecretName = "vp-private-repo-credentials" //nolint:gosec + ) + + var reconciler *PatternReconciler + + createSourceSecret := func(r *PatternReconciler) { + srcSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: srcSecretName, + Namespace: srcNamespace, + }, + Data: map[string][]byte{ + "sshPrivateKey": []byte("fake-key"), + "url": []byte("git@github.com:example/repo.git"), + "type": []byte("git"), + }, + } + _, err := r.fullClient.CoreV1().Secrets(srcNamespace).Create( + context.TODO(), srcSecret, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + } + + secretExistsIn := func(r *PatternReconciler, ns string) bool { + _, err := r.fullClient.CoreV1().Secrets(ns).Get( + context.TODO(), destSecretName, metav1.GetOptions{}) + return err == nil + } + + writeValuesGlobal := func(dir string, singleArgoCD any) { + Expect(os.MkdirAll(dir, 0755)).To(Succeed()) + content := map[string]any{"global": map[string]any{}} + if singleArgoCD != nil { + content["global"] = map[string]any{"singleArgoCD": singleArgoCD} + } + data, err := yaml.Marshal(content) + Expect(err).NotTo(HaveOccurred()) + Expect(os.WriteFile(filepath.Join(dir, "values-global.yaml"), data, 0600)).To(Succeed()) + } + + BeforeEach(func() { + nsOperators := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + reconciler = newFakeReconciler(nsOperators, buildPatternManifest()) + createSourceSecret(reconciler) + activeArgoNamespace = ApplicationNamespace + activeArgoName = ClusterWideArgoName + }) + + Context("singleArgoCD is true", func() { + It("skips copying the secret to the application-name namespace", func() { + patternDir := filepath.Join(tempDir, "single-argo-test") + writeValuesGlobal(patternDir, true) + + p := buildPatternManifest() + enabled := true + p.Spec.MultiSourceConfig.Enabled = &enabled + p.Spec.ClusterGroupName = "standalone" + p.Spec.GitConfig.TokenSecret = srcSecretName + p.Spec.GitConfig.TokenSecretNamespace = srcNamespace + p.Status.LocalCheckoutPath = patternDir + + singleArgo := false + mergedValues, err := getPatternMergedValues(p) + Expect(err).NotTo(HaveOccurred()) + if v := getGlobalValue("singleArgoCD", mergedValues); v != nil { + singleArgo = v == true || v == "true" + } + Expect(singleArgo).To(BeTrue()) + + appNS := applicationName(p) + Expect(appNS).To(Equal("foo-standalone")) + Expect(secretExistsIn(reconciler, appNS)).To(BeFalse()) + }) + }) + + Context("singleArgoCD is false or absent (multi-Argo)", func() { + It("copies the secret to the application-name namespace", func() { + patternDir := filepath.Join(tempDir, "multi-argo-test") + writeValuesGlobal(patternDir, nil) + + p := buildPatternManifest() + enabled := true + p.Spec.MultiSourceConfig.Enabled = &enabled + p.Spec.ClusterGroupName = "standalone" + p.Spec.GitConfig.TokenSecret = srcSecretName + p.Spec.GitConfig.TokenSecretNamespace = srcNamespace + p.Status.LocalCheckoutPath = patternDir + + singleArgo := false + mergedValues, err := getPatternMergedValues(p) + Expect(err).NotTo(HaveOccurred()) + if v := getGlobalValue("singleArgoCD", mergedValues); v != nil { + singleArgo = v == true || v == "true" + } + Expect(singleArgo).To(BeFalse()) + + appNS := applicationName(p) + err = reconciler.copyAuthGitSecret(srcNamespace, srcSecretName, appNS, destSecretName) + Expect(err).NotTo(HaveOccurred()) + Expect(secretExistsIn(reconciler, appNS)).To(BeTrue()) + }) + }) + + Context("legacy ArgoCD namespace", func() { + BeforeEach(func() { + activeArgoNamespace = LegacyApplicationNamespace + activeArgoName = LegacyClusterWideArgoName + }) + + AfterEach(func() { + activeArgoNamespace = ApplicationNamespace + activeArgoName = ClusterWideArgoName + }) + + It("application-name namespace differs from legacy argo namespace", func() { + p := buildPatternManifest() + p.Spec.ClusterGroupName = "standalone" + appNS := applicationName(p) + Expect(appNS).To(Equal("foo-standalone")) + Expect(appNS).NotTo(Equal(getClusterWideArgoNamespace())) + Expect(getClusterWideArgoNamespace()).To(Equal(LegacyApplicationNamespace)) + }) + }) +}) + var _ = Describe("pattern controller - reconciler creation", func() { It("should create a reconciler with all required clients", func() { nsOperators := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} diff --git a/internal/controller/values.go b/internal/controller/values.go index 8fc576159..a6cb0a980 100644 --- a/internal/controller/values.go +++ b/internal/controller/values.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" + api "github.com/hybrid-cloud-patterns/patterns-operator/api/v1alpha1" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/engine" @@ -147,3 +148,41 @@ func countApplicationsAndSets(a any) (appCount, appSetsCount int) { } return applicationCount, applicationSetsCount } + +func getPatternMergedValues(p *api.Pattern) (map[string]any, error) { + gitDir := p.Status.LocalCheckoutPath + if _, err := os.Stat(gitDir); err != nil { + return nil, fmt.Errorf("%s path does not exist", gitDir) + } + + useVariantsDir := HasVariantsFolderLayout(gitDir) + valueFiles := newApplicationValueFiles(p, gitDir, useVariantsDir) + + mergedValues, err := mergeHelmValues(valueFiles...) + if err != nil { + return nil, fmt.Errorf("could not merge value files: %w", err) + } + + extraParams := convertArgoHelmParametersToMap(newApplicationParameters(p)) + mergedValues = chartutil.CoalesceTables(extraParams, mergedValues) + + return mergedValues, nil +} + +func getGlobalValue(key string, values map[string]any) any { //nolint:unparam + global, ok := values["global"] + if !ok { + return nil + } + + globalMap, ok := global.(map[string]any) + if !ok { + return nil + } + + v, ok := globalMap[key] + if !ok { + return nil + } + return v +} diff --git a/internal/controller/values_test.go b/internal/controller/values_test.go index f029a293d..c3c1abf4b 100644 --- a/internal/controller/values_test.go +++ b/internal/controller/values_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" + api "github.com/hybrid-cloud-patterns/patterns-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "gopkg.in/yaml.v3" @@ -405,3 +406,85 @@ var _ = Describe("CountApplicationsAndSets", func() { }) }) }) + +var _ = Describe("getGlobalValue", func() { + It("returns the value when global key exists", func() { + values := map[string]any{ + "global": map[string]any{"singleArgoCD": true}, + } + Expect(getGlobalValue("singleArgoCD", values)).To(BeTrue()) + }) + + It("returns nil when global key is missing", func() { + values := map[string]any{ + "global": map[string]any{"otherKey": "val"}, + } + Expect(getGlobalValue("singleArgoCD", values)).To(BeNil()) + }) + + It("returns nil when global section is absent", func() { + values := map[string]any{"clusterGroup": map[string]any{"foo": "bar"}} + Expect(getGlobalValue("singleArgoCD", values)).To(BeNil()) + }) + + It("returns nil when global is not a map", func() { + values := map[string]any{"global": "notAMap"} + Expect(getGlobalValue("singleArgoCD", values)).To(BeNil()) + }) + + It("returns string value for string-typed booleans", func() { + values := map[string]any{ + "global": map[string]any{"singleArgoCD": "true"}, + } + Expect(getGlobalValue("singleArgoCD", values)).To(Equal("true")) + }) +}) + +var _ = Describe("getPatternMergedValues", func() { + It("merges values-global.yaml and extraParameters", func() { + patternDir := filepath.Join(tempDir, "merge-test") + Expect(os.MkdirAll(patternDir, 0755)).To(Succeed()) + + createTempValueFileAt(patternDir, "values-global.yaml", map[string]any{ + "global": map[string]any{ + "singleArgoCD": true, + "pattern": "from-file", + }, + }) + + p := &api.Pattern{} + enabled := true + p.Spec.MultiSourceConfig.Enabled = &enabled + p.Name = "test-pattern" + p.Spec.ClusterGroupName = "default" + p.Status.LocalCheckoutPath = patternDir + + merged, err := getPatternMergedValues(p) + Expect(err).NotTo(HaveOccurred()) + + global, ok := merged["global"].(map[string]any) + Expect(ok).To(BeTrue()) + Expect(global["singleArgoCD"]).To(BeTrue()) + // extraParameters override file values for global.pattern + Expect(global["pattern"]).To(Equal("test-pattern")) + }) + + It("returns error when checkout path does not exist", func() { + p := &api.Pattern{} + p.Status.LocalCheckoutPath = "/nonexistent/path" + + _, err := getPatternMergedValues(p) + Expect(err).To(HaveOccurred()) + }) +}) + +func createTempValueFileAt(dir, name string, content any) string { + filePath := filepath.Join(dir, name) + data, err := yaml.Marshal(content) + Expect(err).NotTo(HaveOccurred()) + + err = os.WriteFile(filePath, data, 0600) + Expect(err).NotTo(HaveOccurred()) + + return filePath +}