From 67bc61433ae02a4b70d90bcc29f58e537af1ce4e Mon Sep 17 00:00:00 2001 From: Varsha Prasad Narsing Date: Fri, 26 Apr 2024 18:07:54 -0700 Subject: [PATCH] [Add] Allow specifying helm secret namespace for cluster scope objects Currently, the helm secret namespace is taken from the namespace of the object which is being passed to `ActionClientFor`. When the action client is created for cluster scoped objects, the namespace has to explicitly set in their metadata. This PR introduces an option to specify the namespace through the context instead. --- pkg/client/actionclient_test.go | 13 ++++++++----- pkg/client/actionconfig.go | 22 +++++++++++++++++++++- pkg/client/actionconfig_test.go | 22 ++++++++++++++-------- pkg/reconciler/reconciler.go | 2 +- pkg/reconciler/reconciler_test.go | 2 +- 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/pkg/client/actionclient_test.go b/pkg/client/actionclient_test.go index f1dd3a2f..c0fe448c 100644 --- a/pkg/client/actionclient_test.go +++ b/pkg/client/actionclient_test.go @@ -59,7 +59,8 @@ const mockTestDesc = "Test Description" var _ = Describe("ActionClient", func() { var ( - rm meta.RESTMapper + rm meta.RESTMapper + sch *runtime.Scheme ) BeforeEach(func() { var err error @@ -69,10 +70,12 @@ var _ = Describe("ActionClient", func() { rm, err = apiutil.NewDynamicRESTMapper(cfg, httpClient) Expect(err).ToNot(HaveOccurred()) + + sch = runtime.NewScheme() }) var _ = Describe("NewActionClientGetter", func() { It("should return a valid ActionConfigGetter", func() { - actionConfigGetter, err := NewActionConfigGetter(cfg, rm) + actionConfigGetter, err := NewActionConfigGetter(cfg, rm, sch) Expect(err).ShouldNot(HaveOccurred()) acg, err := NewActionClientGetter(actionConfigGetter) Expect(err).ToNot(HaveOccurred()) @@ -89,7 +92,7 @@ var _ = Describe("ActionClient", func() { ) BeforeEach(func() { var err error - actionConfigGetter, err = NewActionConfigGetter(cfg, rm) + actionConfigGetter, err = NewActionConfigGetter(cfg, rm, sch) Expect(err).ShouldNot(HaveOccurred()) dc, err := discovery.NewDiscoveryClientForConfig(cfg) Expect(err).ShouldNot(HaveOccurred()) @@ -310,7 +313,7 @@ var _ = Describe("ActionClient", func() { obj = testutil.BuildTestCR(gvk) }) It("should return a valid ActionClient", func() { - actionConfGetter, err := NewActionConfigGetter(cfg, rm) + actionConfGetter, err := NewActionConfigGetter(cfg, rm, sch) Expect(err).ShouldNot(HaveOccurred()) acg, err := NewActionClientGetter(actionConfGetter) Expect(err).ToNot(HaveOccurred()) @@ -330,7 +333,7 @@ var _ = Describe("ActionClient", func() { BeforeEach(func() { obj = testutil.BuildTestCR(gvk) - actionConfigGetter, err := NewActionConfigGetter(cfg, rm) + actionConfigGetter, err := NewActionConfigGetter(cfg, rm, sch) Expect(err).ShouldNot(HaveOccurred()) acg, err := NewActionClientGetter(actionConfigGetter) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/client/actionconfig.go b/pkg/client/actionconfig.go index 120febf1..888ba351 100644 --- a/pkg/client/actionconfig.go +++ b/pkg/client/actionconfig.go @@ -28,18 +28,22 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/discovery" "k8s.io/client-go/discovery/cached/memory" v1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) +const StorageNamespaceKey = "helm-secret-namespace" + type ActionConfigGetter interface { ActionConfigFor(ctx context.Context, obj client.Object) (*action.Configuration, error) } -func NewActionConfigGetter(baseRestConfig *rest.Config, rm meta.RESTMapper, opts ...ActionConfigGetterOption) (ActionConfigGetter, error) { +func NewActionConfigGetter(baseRestConfig *rest.Config, rm meta.RESTMapper, scheme *runtime.Scheme, opts ...ActionConfigGetterOption) (ActionConfigGetter, error) { dc, err := discovery.NewDiscoveryClientForConfig(baseRestConfig) if err != nil { return nil, fmt.Errorf("create discovery client: %v", err) @@ -50,6 +54,7 @@ func NewActionConfigGetter(baseRestConfig *rest.Config, rm meta.RESTMapper, opts baseRestConfig: baseRestConfig, restMapper: rm, discoveryClient: cdc, + scheme: scheme, } for _, o := range opts { o(acg) @@ -99,12 +104,14 @@ func RestConfigMapper(f func(context.Context, client.Object, *rest.Config) (*res } func getObjectNamespace(obj client.Object) (string, error) { + return obj.GetNamespace(), nil } type actionConfigGetter struct { baseRestConfig *rest.Config restMapper meta.RESTMapper + scheme *runtime.Scheme discoveryClient discovery.CachedDiscoveryInterface objectToClientNamespace ObjectToStringMapper @@ -119,6 +126,19 @@ func (acg *actionConfigGetter) ActionConfigFor(ctx context.Context, obj client.O return nil, fmt.Errorf("get storage namespace for object: %v", err) } + isNamespaced, err := apiutil.IsObjectNamespaced(obj, acg.scheme, acg.restMapper) + if err != nil { + return nil, fmt.Errorf("failed to determine scope of the object: %v", err) + } + + if storageNs == "" || !isNamespaced { + ns, ok := ctx.Value(StorageNamespaceKey).(string) + if !ok || ns == "" { + return nil, fmt.Errorf("empty namespace for creating helm secret") + } + storageNs = ns + } + restConfig, err := acg.objectToRestConfig(ctx, obj, acg.baseRestConfig) if err != nil { return nil, fmt.Errorf("get rest config for object: %v", err) diff --git a/pkg/client/actionconfig_test.go b/pkg/client/actionconfig_test.go index 6a3ee470..490d888e 100644 --- a/pkg/client/actionconfig_test.go +++ b/pkg/client/actionconfig_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" "k8s.io/cli-runtime/pkg/resource" @@ -42,8 +43,10 @@ import ( var _ = Describe("ActionConfig", func() { var _ = Describe("NewActionConfigGetter", func() { - var rm meta.RESTMapper - + var ( + rm meta.RESTMapper + sch *runtime.Scheme + ) BeforeEach(func() { var err error @@ -52,10 +55,12 @@ var _ = Describe("ActionConfig", func() { rm, err = apiutil.NewDynamicRESTMapper(cfg, httpClient) Expect(err).ToNot(HaveOccurred()) + + sch = runtime.NewScheme() }) It("should return a valid ActionConfigGetter", func() { - acg, err := NewActionConfigGetter(cfg, nil) + acg, err := NewActionConfigGetter(cfg, nil, nil) Expect(err).ShouldNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) }) @@ -77,7 +82,7 @@ var _ = Describe("ActionConfig", func() { It("should use a custom client namespace", func() { clientNs := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("client-%s", rand.String(8))}} clientNsMapper := func(_ client.Object) (string, error) { return clientNs.Name, nil } - acg, err := NewActionConfigGetter(cfg, rm, ClientNamespaceMapper(clientNsMapper)) + acg, err := NewActionConfigGetter(cfg, rm, sch, ClientNamespaceMapper(clientNsMapper)) Expect(err).ToNot(HaveOccurred()) ac, err := acg.ActionConfigFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) @@ -99,7 +104,7 @@ metadata: It("should use a custom storage namespace", func() { storageNs := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("storage-%s", rand.String(8))}} storageNsMapper := func(_ client.Object) (string, error) { return storageNs.Name, nil } - acg, err := NewActionConfigGetter(cfg, rm, StorageNamespaceMapper(storageNsMapper)) + acg, err := NewActionConfigGetter(cfg, rm, sch, StorageNamespaceMapper(storageNsMapper)) Expect(err).ToNot(HaveOccurred()) ac, err := acg.ActionConfigFor(context.Background(), obj) @@ -134,7 +139,7 @@ metadata: }) It("should disable storage owner ref injection", func() { - acg, err := NewActionConfigGetter(cfg, rm, DisableStorageOwnerRefInjection(true)) + acg, err := NewActionConfigGetter(cfg, rm, sch, DisableStorageOwnerRefInjection(true)) Expect(err).ToNot(HaveOccurred()) ac, err := acg.ActionConfigFor(context.Background(), obj) @@ -168,7 +173,7 @@ metadata: BearerToken: obj.GetName(), }, nil } - acg, err := NewActionConfigGetter(cfg, rm, RestConfigMapper(restConfigMapper)) + acg, err := NewActionConfigGetter(cfg, rm, sch, RestConfigMapper(restConfigMapper)) Expect(err).ToNot(HaveOccurred()) testObject := func(name string) client.Object { @@ -199,8 +204,9 @@ metadata: rm, err := apiutil.NewDynamicRESTMapper(cfg, httpClient) Expect(err).ToNot(HaveOccurred()) + sch := runtime.NewScheme() - acg, err := NewActionConfigGetter(cfg, rm) + acg, err := NewActionConfigGetter(cfg, rm, sch) Expect(err).ShouldNot(HaveOccurred()) ac, err := acg.ActionConfigFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 272c8c75..e5439117 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -917,7 +917,7 @@ func (r *Reconciler) addDefaults(mgr ctrl.Manager, controllerName string) error r.log = ctrl.Log.WithName("controllers").WithName("Helm") } if r.actionClientGetter == nil { - actionConfigGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper()) + actionConfigGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), mgr.GetScheme()) if err != nil { return fmt.Errorf("creating action config getter: %w", err) } diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 4a5e9455..e6a020d6 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -977,7 +977,7 @@ var _ = Describe("Reconciler", func() { var actionConf *action.Configuration BeforeEach(func() { By("getting the current release and config", func() { - acg, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper()) + acg, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), mgr.GetScheme()) Expect(err).ShouldNot(HaveOccurred()) actionConf, err = acg.ActionConfigFor(ctx, obj) Expect(err).ToNot(HaveOccurred())