Skip to content

Commit

Permalink
Merge pull request #492 from fluxcd/fix-kubecfg-impersonation-ns
Browse files Browse the repository at this point in the history 
kube: configure proper account impersonation NS
  • Loading branch information
@hiddeco
hiddeco committed Jun 7, 2022
2 parents bf7406b + d19b470 commit c10ea3c
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 42 deletions.
9 changes: 6 additions & 3 deletions controllers/helmrelease_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
21 changes: 12 additions & 9 deletions internal/kube/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}
}

Expand All @@ -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)
}
14 changes: 8 additions & 6 deletions internal/kube/builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}))

Expand All @@ -85,32 +85,34 @@ 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{}))

flags := getter.(*genericclioptions.ConfigFlags)
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{}))

flags := getter.(*genericclioptions.ConfigFlags)
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"))
})
}

Expand Down
24 changes: 13 additions & 11 deletions internal/kube/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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,
}
}

Expand All @@ -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
}

Expand Down
27 changes: 14 additions & 13 deletions internal/kube/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}))

Expand All @@ -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())
Expand All @@ -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{}))

Expand All @@ -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"))
Expand All @@ -131,19 +132,19 @@ 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) {
g := NewWithT(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,
})

Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())

Expand Down

0 comments on commit c10ea3c

Please sign in to comment.