From d19b4704126eec9b9da18d1b8d115191c72ae737 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 7 Jun 2022 12:25:29 +0200 Subject: [PATCH] kube: configure proper account impersonation NS Fixing a regression introduced in #480 which would always pick the namespace of the release. In addition, historically seen the configuration of the impersonation username while making use of a KubeConfig has never worked correctly, this has been adressed as well. Signed-off-by: Hidde Beydals --- controllers/helmrelease_controller.go | 9 ++++++--- internal/kube/builder.go | 21 ++++++++++++--------- internal/kube/builder_test.go | 14 ++++++++------ internal/kube/client.go | 24 +++++++++++++----------- internal/kube/client_test.go | 27 ++++++++++++++------------- 5 files changed, 53 insertions(+), 42 deletions(-) diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index e83c74015..8d3c36e64 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -474,10 +474,13 @@ func (r *HelmReleaseReconciler) checkDependencies(hr v2.HelmRelease) error { } func (r *HelmReleaseReconciler) buildRESTClientGetter(ctx context.Context, hr v2.HelmRelease) (genericclioptions.RESTClientGetter, error) { - opts := []kube.ClientGetterOption{kube.WithClientOptions(r.ClientOpts)} - if hr.Spec.ServiceAccountName != "" { - opts = append(opts, kube.WithImpersonate(hr.Spec.ServiceAccountName)) + opts := []kube.ClientGetterOption{ + kube.WithClientOptions(r.ClientOpts), + // When ServiceAccountName is empty, it will fall back to the configured default. + // If this is not configured either, this option will result in a no-op. + kube.WithImpersonate(hr.Spec.ServiceAccountName, hr.GetNamespace()), } + opts = append(opts) if hr.Spec.KubeConfig != nil { secretName := types.NamespacedName{ Namespace: hr.GetNamespace(), diff --git a/internal/kube/builder.go b/internal/kube/builder.go index dc37f0642..dcb575a6c 100644 --- a/internal/kube/builder.go +++ b/internal/kube/builder.go @@ -33,11 +33,12 @@ const ( // clientGetterOptions used to BuildClientGetter. type clientGetterOptions struct { - namespace string - kubeConfig []byte - impersonateAccount string - clientOptions client.Options - kubeConfigOptions client.KubeConfigOptions + namespace string + kubeConfig []byte + impersonateAccount string + impersonateNamespace string + clientOptions client.Options + kubeConfigOptions client.KubeConfigOptions } // ClientGetterOption configures a genericclioptions.RESTClientGetter. @@ -61,10 +62,12 @@ func WithClientOptions(opts client.Options) func(o *clientGetterOptions) { } // WithImpersonate configures the genericclioptions.RESTClientGetter to -// impersonate the provided account name. -func WithImpersonate(accountName string) func(o *clientGetterOptions) { +// impersonate with the given account name in the provided namespace. +// If the account name is empty, DefaultServiceAccountName is assumed. +func WithImpersonate(accountName, namespace string) func(o *clientGetterOptions) { return func(o *clientGetterOptions) { o.impersonateAccount = accountName + o.impersonateNamespace = namespace } } @@ -80,7 +83,7 @@ func BuildClientGetter(namespace string, opts ...ClientGetterOption) (genericcli opt(o) } if len(o.kubeConfig) > 0 { - return NewMemoryRESTClientGetter(o.kubeConfig, namespace, o.impersonateAccount, o.clientOptions, o.kubeConfigOptions), nil + return NewMemoryRESTClientGetter(o.kubeConfig, namespace, o.impersonateAccount, o.impersonateNamespace, o.clientOptions, o.kubeConfigOptions), nil } - return NewInClusterRESTClientGetter(namespace, o.impersonateAccount, &o.clientOptions) + return NewInClusterRESTClientGetter(namespace, o.impersonateAccount, o.impersonateNamespace, &o.clientOptions) } diff --git a/internal/kube/builder_test.go b/internal/kube/builder_test.go index 04785eb3c..bd2a7a837 100644 --- a/internal/kube/builder_test.go +++ b/internal/kube/builder_test.go @@ -67,7 +67,7 @@ users:`) cfgOpts := client.KubeConfigOptions{InsecureTLS: true} impersonate := "jane" - getter, err := BuildClientGetter(namespace, WithClientOptions(clientOpts), WithKubeConfig(cfg, cfgOpts), WithImpersonate(impersonate)) + getter, err := BuildClientGetter(namespace, WithClientOptions(clientOpts), WithKubeConfig(cfg, cfgOpts), WithImpersonate(impersonate, "")) g.Expect(err).ToNot(HaveOccurred()) g.Expect(getter).To(BeAssignableToTypeOf(&MemoryRESTClientGetter{})) @@ -85,7 +85,8 @@ users:`) namespace := "a-namespace" impersonate := "frank" - getter, err := BuildClientGetter(namespace, WithImpersonate(impersonate)) + impersonateNS := "other-namespace" + getter, err := BuildClientGetter(namespace, WithImpersonate(impersonate, impersonateNS)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(getter).To(BeAssignableToTypeOf(&genericclioptions.ConfigFlags{})) @@ -93,16 +94,17 @@ users:`) g.Expect(flags.Namespace).ToNot(BeNil()) g.Expect(*flags.Namespace).To(Equal(namespace)) g.Expect(flags.Impersonate).ToNot(BeNil()) - g.Expect(*flags.Impersonate).To(Equal("system:serviceaccount:a-namespace:frank")) + g.Expect(*flags.Impersonate).To(Equal("system:serviceaccount:other-namespace:frank")) }) - t.Run("with DefaultServiceAccount", func(t *testing.T) { + t.Run("with impersonate DefaultServiceAccount", func(t *testing.T) { g := NewWithT(t) ctrl.GetConfig = mockGetConfig namespace := "a-namespace" DefaultServiceAccountName = "frank" - getter, err := BuildClientGetter(namespace) + impersonateNS := "other-namespace" + getter, err := BuildClientGetter(namespace, WithImpersonate("", impersonateNS)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(getter).To(BeAssignableToTypeOf(&genericclioptions.ConfigFlags{})) @@ -110,7 +112,7 @@ users:`) g.Expect(flags.Namespace).ToNot(BeNil()) g.Expect(*flags.Namespace).To(Equal(namespace)) g.Expect(flags.Impersonate).ToNot(BeNil()) - g.Expect(*flags.Impersonate).To(Equal("system:serviceaccount:a-namespace:frank")) + g.Expect(*flags.Impersonate).To(Equal("system:serviceaccount:other-namespace:frank")) }) } diff --git a/internal/kube/client.go b/internal/kube/client.go index 65692fbcd..8312562d9 100644 --- a/internal/kube/client.go +++ b/internal/kube/client.go @@ -35,12 +35,12 @@ import ( // using genericclioptions.NewConfigFlags, and configures it with the server, // authentication, impersonation, client options, and the provided namespace. // It returns an error if it fails to retrieve a rest.Config. -func NewInClusterRESTClientGetter(namespace, impersonateAccount string, opts *client.Options) (genericclioptions.RESTClientGetter, error) { +func NewInClusterRESTClientGetter(namespace, impersonateAccount, impersonateNamespace string, opts *client.Options) (genericclioptions.RESTClientGetter, error) { cfg, err := controllerruntime.GetConfig() if err != nil { return nil, fmt.Errorf("failed to get config for in-cluster REST client: %w", err) } - SetImpersonationConfig(cfg, namespace, impersonateAccount) + SetImpersonationConfig(cfg, impersonateNamespace, impersonateAccount) flags := genericclioptions.NewConfigFlags(false) flags.APIServer = pointer.String(cfg.Host) @@ -73,6 +73,8 @@ type MemoryRESTClientGetter struct { namespace string // impersonateAccount configures the rest.ImpersonationConfig account name. impersonateAccount string + // impersonateAccount configures the rest.ImpersonationConfig account namespace. + impersonateNamespace string } // NewMemoryRESTClientGetter returns a MemoryRESTClientGetter configured with @@ -81,15 +83,17 @@ type MemoryRESTClientGetter struct { func NewMemoryRESTClientGetter( kubeConfig []byte, namespace string, - impersonate string, + impersonateAccount string, + impersonateNamespace string, clientOpts client.Options, kubeConfigOpts client.KubeConfigOptions) genericclioptions.RESTClientGetter { return &MemoryRESTClientGetter{ - kubeConfig: kubeConfig, - namespace: namespace, - impersonateAccount: impersonate, - clientOpts: clientOpts, - kubeConfigOpts: kubeConfigOpts, + kubeConfig: kubeConfig, + namespace: namespace, + impersonateAccount: impersonateAccount, + impersonateNamespace: impersonateNamespace, + clientOpts: clientOpts, + kubeConfigOpts: kubeConfigOpts, } } @@ -102,9 +106,7 @@ func (c *MemoryRESTClientGetter) ToRESTConfig() (*rest.Config, error) { return nil, err } cfg = client.KubeConfig(cfg, c.kubeConfigOpts) - if c.impersonateAccount != "" { - cfg.Impersonate = rest.ImpersonationConfig{UserName: c.impersonateAccount} - } + SetImpersonationConfig(cfg, c.impersonateNamespace, c.impersonateAccount) return cfg, nil } diff --git a/internal/kube/client_test.go b/internal/kube/client_test.go index 88e3e9ba2..53d8c2c20 100644 --- a/internal/kube/client_test.go +++ b/internal/kube/client_test.go @@ -60,7 +60,7 @@ func TestNewInClusterRESTClientGetter(t *testing.T) { ctrl.GetConfig = func() (*rest.Config, error) { return cfg, nil } - got, err := NewInClusterRESTClientGetter("", "", nil) + got, err := NewInClusterRESTClientGetter("", "", "", nil) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(BeAssignableToTypeOf(&genericclioptions.ConfigFlags{})) @@ -83,7 +83,7 @@ func TestNewInClusterRESTClientGetter(t *testing.T) { ctrl.GetConfig = func() (*rest.Config, error) { return nil, fmt.Errorf("error") } - got, err := NewInClusterRESTClientGetter("", "", nil) + got, err := NewInClusterRESTClientGetter("", "", "", nil) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("failed to get config for in-cluster REST client")) g.Expect(got).To(BeNil()) @@ -94,7 +94,7 @@ func TestNewInClusterRESTClientGetter(t *testing.T) { ctrl.GetConfig = mockGetConfig namespace := "a-space" - got, err := NewInClusterRESTClientGetter(namespace, "", nil) + got, err := NewInClusterRESTClientGetter(namespace, "", "", nil) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(BeAssignableToTypeOf(&genericclioptions.ConfigFlags{})) @@ -109,20 +109,21 @@ func TestNewInClusterRESTClientGetter(t *testing.T) { ctrl.GetConfig = mockGetConfig ns := "a-namespace" accountName := "foo" - got, err := NewInClusterRESTClientGetter(ns, accountName, nil) + accountNamespace := "another-namespace" + got, err := NewInClusterRESTClientGetter(ns, accountName, accountNamespace, nil) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(BeAssignableToTypeOf(&genericclioptions.ConfigFlags{})) flags := got.(*genericclioptions.ConfigFlags) g.Expect(flags.Impersonate).ToNot(BeNil()) - g.Expect(*flags.Impersonate).To(Equal(fmt.Sprintf("system:serviceaccount:%s:%s", ns, accountName))) + g.Expect(*flags.Impersonate).To(Equal(fmt.Sprintf("system:serviceaccount:%s:%s", accountNamespace, accountName))) }) } func TestMemoryRESTClientGetter_ToRESTConfig(t *testing.T) { t.Run("loads REST config from KubeConfig", func(t *testing.T) { g := NewWithT(t) - getter := NewMemoryRESTClientGetter(cfg, "", "", client.Options{}, client.KubeConfigOptions{}) + getter := NewMemoryRESTClientGetter(cfg, "", "", "", client.Options{}, client.KubeConfigOptions{}) got, err := getter.ToRESTConfig() g.Expect(err).ToNot(HaveOccurred()) g.Expect(got.Host).To(Equal("http://cow.org:8080")) @@ -131,11 +132,11 @@ func TestMemoryRESTClientGetter_ToRESTConfig(t *testing.T) { t.Run("sets ImpersonationConfig", func(t *testing.T) { g := NewWithT(t) - getter := NewMemoryRESTClientGetter(cfg, "", "someone", client.Options{}, client.KubeConfigOptions{}) + getter := NewMemoryRESTClientGetter(cfg, "", "someone", "a-namespace", client.Options{}, client.KubeConfigOptions{}) got, err := getter.ToRESTConfig() g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got.Impersonate.UserName).To(Equal("someone")) + g.Expect(got.Impersonate.UserName).To(Equal("system:serviceaccount:a-namespace:someone")) }) t.Run("uses KubeConfigOptions", func(t *testing.T) { @@ -143,7 +144,7 @@ func TestMemoryRESTClientGetter_ToRESTConfig(t *testing.T) { agent := "a static string forever," + "but static strings can have dreams and hope too" - getter := NewMemoryRESTClientGetter(cfg, "", "someone", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{ + getter := NewMemoryRESTClientGetter(cfg, "", "someone", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{ UserAgent: agent, }) @@ -155,7 +156,7 @@ func TestMemoryRESTClientGetter_ToRESTConfig(t *testing.T) { t.Run("invalid config", func(t *testing.T) { g := NewWithT(t) - getter := NewMemoryRESTClientGetter([]byte("invalid"), "", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) + getter := NewMemoryRESTClientGetter([]byte("invalid"), "", "", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) got, err := getter.ToRESTConfig() g.Expect(err).To(HaveOccurred()) g.Expect(got).To(BeNil()) @@ -165,7 +166,7 @@ func TestMemoryRESTClientGetter_ToRESTConfig(t *testing.T) { func TestMemoryRESTClientGetter_ToDiscoveryClient(t *testing.T) { g := NewWithT(t) - getter := NewMemoryRESTClientGetter(cfg, "", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) + getter := NewMemoryRESTClientGetter(cfg, "", "", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) got, err := getter.ToDiscoveryClient() g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).ToNot(BeNil()) @@ -174,7 +175,7 @@ func TestMemoryRESTClientGetter_ToDiscoveryClient(t *testing.T) { func TestMemoryRESTClientGetter_ToRESTMapper(t *testing.T) { g := NewWithT(t) - getter := NewMemoryRESTClientGetter(cfg, "", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) + getter := NewMemoryRESTClientGetter(cfg, "", "", "", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) got, err := getter.ToRESTMapper() g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).ToNot(BeNil()) @@ -183,7 +184,7 @@ func TestMemoryRESTClientGetter_ToRESTMapper(t *testing.T) { func TestMemoryRESTClientGetter_ToRawKubeConfigLoader(t *testing.T) { g := NewWithT(t) - getter := NewMemoryRESTClientGetter(cfg, "a-namespace", "impersonate", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) + getter := NewMemoryRESTClientGetter(cfg, "a-namespace", "impersonate", "other-namespace", client.Options{QPS: 400, Burst: 800}, client.KubeConfigOptions{}) got := getter.ToRawKubeConfigLoader() g.Expect(got).ToNot(BeNil())