From 1edc9ed1d462d70a5323cab3b123e23a40d9d131 Mon Sep 17 00:00:00 2001 From: Tesshu Flower Date: Thu, 19 Dec 2024 13:11:28 -0500 Subject: [PATCH] Use acm rh registry pull request secret for volsync-system (#513) Allows non-Openshift clusters to be able to access the images in the redhat registry in the volsync-system namespace - Use ACM label that will copy the pull request secret to the volsync-system namespace (this is similar to other ACM addons) - Update the volsync service account to use this pull request secret Signed-off-by: Tesshu Flower --- controllers/addoncontroller.go | 8 +++- controllers/addoncontroller_test.go | 25 +++++++--- controllers/helmutils/helmutils.go | 48 ++++++++++++++----- controllers/helmutils/helmutils_test.go | 2 +- .../helmutils/helmutilstest/testhelper.go | 9 ++++ controllers/manifesthelper_helmdeploy.go | 2 +- .../manifests/helm-chart/namespace.yaml | 2 + 7 files changed, 73 insertions(+), 23 deletions(-) diff --git a/controllers/addoncontroller.go b/controllers/addoncontroller.go index d2e7795..7cee41e 100644 --- a/controllers/addoncontroller.go +++ b/controllers/addoncontroller.go @@ -80,6 +80,13 @@ const ( AnnotationVolSyncAddonDeployTypeOverrideOLMValue = "olm" AnnotationHelmChartKey = "helm-chart-key" + + // This is the name of the pull secret that is copied to the namespace (volsync-system) on the managed + // cluster. This will allow pulls to the redhat registry. + // (Other addons get this copied to open-cluster-management-agent-addon namespace on the mgd cluster) + // Note this secret is automatically copied to volsync-system via putting the + // label "addon.open-cluster-management.io/namespace":"true" on the volsync-system ns + RHRegistryPullSecretName = "open-cluster-management-image-pull-credentials" ) func init() { @@ -156,7 +163,6 @@ func (h *volsyncAgent) GetAgentAddonOptions() agent.AgentAddonOptions { Resource: "deployments", Name: "volsync", Namespace: "*", - //Namespace: "volsync-system", }, ProbeRules: []workapiv1.FeedbackRule{ { diff --git a/controllers/addoncontroller_test.go b/controllers/addoncontroller_test.go index b2b05f6..4b39c95 100644 --- a/controllers/addoncontroller_test.go +++ b/controllers/addoncontroller_test.go @@ -40,6 +40,8 @@ var _ = Describe("Addoncontroller - helm deployment tests", func() { genericCodecs := serializer.NewCodecFactory(scheme.Scheme) genericCodec := genericCodecs.UniversalDeserializer() + expectedVolSyncNamespace := controllers.DefaultHelmInstallNamespace + // Make sure a ClusterManagementAddOn exists for volsync or addon-framework will not reconcile // VolSync ManagedClusterAddOns var clusterManagementAddon *addonv1alpha1.ClusterManagementAddOn @@ -194,8 +196,6 @@ var _ = Describe("Addoncontroller - helm deployment tests", func() { } Context(fmt.Sprintf("When the managed cluster %s", whenText), func() { - expectedNamespace := controllers.DefaultHelmInstallNamespace - var namespaceObj *corev1.Namespace var operatorPolicyObj *policyv1beta1.OperatorPolicy var operatorPolicyAggregateClusterRoleObj *rbacv1.ClusterRole @@ -302,18 +302,29 @@ var _ = Describe("Addoncontroller - helm deployment tests", func() { } // In all cases here we expect the namespace to be the default - Expect(namespaceObj.GetName()).To(Equal(expectedNamespace)) + Expect(namespaceObj.GetName()).To(Equal(expectedVolSyncNamespace)) + // Check that special label is set on the namespace to indicate that ACM should copy over the + // redhat registry pull secret (allows us to pull volsync images from registry.redhat.io in + // volsync-system) + nsLabels := namespaceObj.GetLabels() + Expect(len(nsLabels)).To(Equal(1)) + pullSecretCopyLabel, ok := nsLabels["addon.open-cluster-management.io/namespace"] + Expect(ok).To(BeTrue()) + Expect(pullSecretCopyLabel).To(Equal("true")) }) Context("When the ManagedClusterAddOn spec does not set an installNamespace", func() { It("Should install to default namespace", func() { // should this get set in the managedclusteraddon.spec.InstallNamespace as well? helmutilstest.VerifyHelmRenderedVolSyncObjects(helmChartObjs, - expectedNamespace, mgdClusterIsOpenShift) + expectedVolSyncNamespace, mgdClusterIsOpenShift) }) }) Context("When the ManagedClusterAddOn spec sets an installNamespace", func() { + // ManagedClusterAddon.Spec.InstallNamespace is essentially deprecated and should not be used + // See: https://github.com/open-cluster-management-io/ocm/issues/298 + // volsync-addon-controller Code should ignore it BeforeEach(func() { // Override to specifically set the ns in the spec - all the tests above in JustBeforeEach // should still be valid here @@ -325,7 +336,7 @@ var _ = Describe("Addoncontroller - helm deployment tests", func() { Expect(mcAddon.Spec.InstallNamespace).To(Equal("test1234")) helmutilstest.VerifyHelmRenderedVolSyncObjects(helmChartObjs, - expectedNamespace, mgdClusterIsOpenShift) + expectedVolSyncNamespace, mgdClusterIsOpenShift) }) }) }) @@ -474,7 +485,7 @@ var _ = Describe("Addoncontroller - helm deployment tests", func() { var err error volsyncDeployment, err = getVolSyncDeploymentFromManifestWork(manifestWork, genericCodec) Expect(err).NotTo(HaveOccurred()) - Expect(volsyncDeployment.GetNamespace()).To(Equal("volsync-system")) + Expect(volsyncDeployment.GetNamespace()).To(Equal(expectedVolSyncNamespace)) Expect(volsyncDeployment.GetName()).To(Equal("volsync")) }) @@ -1019,7 +1030,7 @@ var _ = Describe("Addoncontroller - helm deployment tests", func() { Expect(manifestWork).ToNot(BeNil()) Expect(volsyncDeployment).NotTo(BeNil()) - Expect(volsyncDeployment.GetNamespace()).To(Equal("volsync-system")) + Expect(volsyncDeployment.GetNamespace()).To(Equal(expectedVolSyncNamespace)) }) Context("When a ManagedClusterAddOn is created with no addonConfig specified (the default)", func() { diff --git a/controllers/helmutils/helmutils.go b/controllers/helmutils/helmutils.go index 76f6768..ba433a0 100644 --- a/controllers/helmutils/helmutils.go +++ b/controllers/helmutils/helmutils.go @@ -8,6 +8,7 @@ import ( "sort" "sync" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" @@ -24,6 +25,7 @@ import ( const ( defaultEmbeddedChartsDir = "/helmcharts" crdKind = "CustomResourceDefinition" + serviceAccountKind = "ServiceAccount" ) // List of kinds of objects in the manifestwork - anything in this list will not have @@ -88,6 +90,7 @@ func RenderManifestsFromChart( clusterIsOpenShift bool, chartValues map[string]interface{}, runtimeDecoder runtime.Decoder, + pullSecretForServiceAccount string, ) ([]runtime.Object, error) { helmObjs := []runtime.Object{} @@ -164,20 +167,39 @@ func RenderManifestsFromChart( return nil, err } - if gvk != nil { - if !slices.Contains(globalKinds, gvk.Kind) { - // Helm rendering does not set namespace on the templates, it will rely on the kubectl install/apply - // to do it (which does not happen since these objects end up directly in our manifestwork). - // So set the namespace ourselves for any object with kind not in our globalKinds list - templateObj.(metav1.Object).SetNamespace(releaseOptions.Namespace) - } + if gvk == nil { + gvkErr := fmt.Errorf("no gvk for template") + klog.Error(gvkErr, "Error decoding gvk from rendered template", "fileName", fileName) + return nil, gvkErr + } + + if !slices.Contains(globalKinds, gvk.Kind) { + // Helm rendering does not set namespace on the templates, it will rely on the kubectl install/apply + // to do it (which does not happen since these objects end up directly in our manifestwork). + // So set the namespace ourselves for any object with kind not in our globalKinds list + templateObj.(metav1.Object).SetNamespace(releaseOptions.Namespace) + } - if gvk.Kind == crdKind { - // Add annotation to indicate we do not want the CRD deleted when the manifestwork is deleted - // (i.e. when the managedclusteraddon is deleted) - crdAnnotations := templateObj.(metav1.Object).GetAnnotations() - crdAnnotations[addonapiv1alpha1.DeletionOrphanAnnotationKey] = "" - templateObj.(metav1.Object).SetAnnotations(crdAnnotations) + // Special cases for specific resource kinds + switch gvk.Kind { + case crdKind: + // Add annotation to indicate we do not want the CRD deleted when the manifestwork is deleted + // (i.e. when the managedclusteraddon is deleted) + crdAnnotations := templateObj.(metav1.Object).GetAnnotations() + crdAnnotations[addonapiv1alpha1.DeletionOrphanAnnotationKey] = "" + templateObj.(metav1.Object).SetAnnotations(crdAnnotations) + case serviceAccountKind: + // Add pull secret to svc account + if pullSecretForServiceAccount != "" { + svcAccount, ok := templateObj.(*corev1.ServiceAccount) + if !ok { + svcAcctErr := fmt.Errorf("unable to decode service account resource") + klog.Error(svcAcctErr, "Error rendering helm chart") + return nil, svcAcctErr + } + svcAccount.ImagePullSecrets = append(svcAccount.ImagePullSecrets, corev1.LocalObjectReference{ + Name: pullSecretForServiceAccount, + }) } } diff --git a/controllers/helmutils/helmutils_test.go b/controllers/helmutils/helmutils_test.go index a85da9e..751ed88 100644 --- a/controllers/helmutils/helmutils_test.go +++ b/controllers/helmutils/helmutils_test.go @@ -60,7 +60,7 @@ var _ = Describe("Helmutils", func() { } renderedObjs, err = helmutils.RenderManifestsFromChart(chart, testNamespace, testCluster, clusterIsOpenShift, - chartValues, genericCodec) + chartValues, genericCodec, controllers.RHRegistryPullSecretName) Expect(err).NotTo(HaveOccurred()) Expect(renderedObjs).NotTo(BeNil()) }) diff --git a/controllers/helmutils/helmutilstest/testhelper.go b/controllers/helmutils/helmutilstest/testhelper.go index 08f3823..9d27bbb 100644 --- a/controllers/helmutils/helmutilstest/testhelper.go +++ b/controllers/helmutils/helmutilstest/testhelper.go @@ -10,6 +10,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" + + "github.com/stolostron/volsync-addon-controller/controllers" ) //nolint:funlen @@ -131,6 +133,13 @@ func VerifyHelmRenderedVolSyncObjects(objs []runtime.Object, Expect(*deployment.Spec.Template.Spec.SecurityContext.RunAsUser).To(Equal(int64(65534))) } + // Check ServiceAccount + // It should have the pull secret set + Expect(len(serviceAccount.ImagePullSecrets)).To(Equal(1)) + Expect(serviceAccount.ImagePullSecrets[0]).To(Equal(corev1.LocalObjectReference{ + Name: controllers.RHRegistryPullSecretName, + })) + // Return the deployment (can be checked further by tests) return deployment } diff --git a/controllers/manifesthelper_helmdeploy.go b/controllers/manifesthelper_helmdeploy.go index 1d7ff47..bcb3b29 100644 --- a/controllers/manifesthelper_helmdeploy.go +++ b/controllers/manifesthelper_helmdeploy.go @@ -103,7 +103,7 @@ func (mh *manifestHelperHelmDeploy) loadManifestsFromHelmRepo(values addonfactor } return helmutils.RenderManifestsFromChart(chart, installNamespace, - mh.cluster, mh.clusterIsOpenShift, values, genericCodec) + mh.cluster, mh.clusterIsOpenShift, values, genericCodec, RHRegistryPullSecretName) } func (mh *manifestHelperHelmDeploy) getValuesForManifest() (addonfactory.Values, error) { diff --git a/controllers/manifests/helm-chart/namespace.yaml b/controllers/manifests/helm-chart/namespace.yaml index 88156e6..30190aa 100644 --- a/controllers/manifests/helm-chart/namespace.yaml +++ b/controllers/manifests/helm-chart/namespace.yaml @@ -2,3 +2,5 @@ apiVersion: v1 kind: Namespace metadata: name: {{ .InstallNamespace }} + labels: + "addon.open-cluster-management.io/namespace": "true"