Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Add] Allow specifying helm secret namespace for cluster scope objects #323

Closed
wants to merge 1 commit into from
Closed

[Add] Allow specifying helm secret namespace for cluster scope objects #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions pkg/client/actionclient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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())
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand Down
22 changes: 21 additions & 1 deletion pkg/client/actionconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,22 @@
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)
Expand All @@ -50,6 +54,7 @@
baseRestConfig: baseRestConfig,
restMapper: rm,
discoveryClient: cdc,
scheme: scheme,
}
for _, o := range opts {
o(acg)
Expand Down Expand Up @@ -98,13 +103,15 @@
}
}

func getObjectNamespace(obj client.Object) (string, error) {

Check failure on line 106 in pkg/client/actionconfig.go

View workflow job for this annotation

GitHub Actions / Lint 

unnecessary leading newline (whitespace)

return obj.GetNamespace(), nil
}

type actionConfigGetter struct {
baseRestConfig *rest.Config
restMapper meta.RESTMapper
scheme *runtime.Scheme
discoveryClient discovery.CachedDiscoveryInterface

objectToClientNamespace ObjectToStringMapper
Expand All @@ -119,6 +126,19 @@
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)
Expand Down
22 changes: 14 additions & 8 deletions pkg/client/actionconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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

Expand All @@ -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())
})
Expand All @@ -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())
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
Loading