From 6e9815980fcecfa845c52585c26bbfd16ce891ce Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Tue, 23 Apr 2024 14:08:23 -0400 Subject: [PATCH] actionConfigGetter: allow custom mapping from object to rest.Config (#317) Signed-off-by: Joe Lanford --- go.mod | 2 +- pkg/client/actionclient.go | 13 ++- pkg/client/actionclient_test.go | 38 ++++--- pkg/client/actionconfig.go | 103 ++++++++++------- pkg/client/actionconfig_test.go | 50 ++++++--- pkg/client/postrenderer_test.go | 8 +- pkg/client/restclientgetter.go | 30 +---- pkg/client/restclientgetter_test.go | 110 +++++++++---------- pkg/reconciler/internal/fake/actionclient.go | 3 +- pkg/reconciler/reconciler.go | 7 +- pkg/reconciler/reconciler_test.go | 17 ++- testdata/hybrid/memcached-operator/go.mod | 2 +- 12 files changed, 203 insertions(+), 180 deletions(-) diff --git a/go.mod b/go.mod index 41ee4cea..91248e65 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/operator-framework/helm-operator-plugins go 1.21 -toolchain go1.21.8 +toolchain go1.21.9 require ( github.com/blang/semver/v4 v4.0.0 diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index e9ac654b..4b6c6de0 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -18,6 +18,7 @@ package client import ( "bytes" + "context" "encoding/json" "errors" "fmt" @@ -40,13 +41,13 @@ import ( ) type ActionClientGetter interface { - ActionClientFor(obj client.Object) (ActionInterface, error) + ActionClientFor(ctx context.Context, obj client.Object) (ActionInterface, error) } -type ActionClientGetterFunc func(obj client.Object) (ActionInterface, error) +type ActionClientGetterFunc func(ctx context.Context, obj client.Object) (ActionInterface, error) -func (acgf ActionClientGetterFunc) ActionClientFor(obj client.Object) (ActionInterface, error) { - return acgf(obj) +func (acgf ActionClientGetterFunc) ActionClientFor(ctx context.Context, obj client.Object) (ActionInterface, error) { + return acgf(ctx, obj) } type ActionInterface interface { @@ -140,8 +141,8 @@ type actionClientGetter struct { var _ ActionClientGetter = &actionClientGetter{} -func (hcg *actionClientGetter) ActionClientFor(obj client.Object) (ActionInterface, error) { - actionConfig, err := hcg.acg.ActionConfigFor(obj) +func (hcg *actionClientGetter) ActionClientFor(ctx context.Context, obj client.Object) (ActionInterface, error) { + actionConfig, err := hcg.acg.ActionConfigFor(ctx, obj) if err != nil { return nil, err } diff --git a/pkg/client/actionclient_test.go b/pkg/client/actionclient_test.go index 1812ff28..f1dd3a2f 100644 --- a/pkg/client/actionclient_test.go +++ b/pkg/client/actionclient_test.go @@ -24,7 +24,6 @@ import ( "strconv" "time" - "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "helm.sh/helm/v3/pkg/action" @@ -45,6 +44,8 @@ import ( apitypes "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" "k8s.io/cli-runtime/pkg/resource" + "k8s.io/client-go/discovery" + "k8s.io/client-go/discovery/cached/memory" "k8s.io/client-go/rest" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -71,7 +72,7 @@ var _ = Describe("ActionClient", func() { }) var _ = Describe("NewActionClientGetter", func() { It("should return a valid ActionConfigGetter", func() { - actionConfigGetter, err := NewActionConfigGetter(cfg, rm, logr.Discard()) + actionConfigGetter, err := NewActionConfigGetter(cfg, rm) Expect(err).ShouldNot(HaveOccurred()) acg, err := NewActionClientGetter(actionConfigGetter) Expect(err).ToNot(HaveOccurred()) @@ -88,9 +89,12 @@ var _ = Describe("ActionClient", func() { ) BeforeEach(func() { var err error - actionConfigGetter, err = NewActionConfigGetter(cfg, rm, logr.Discard()) + actionConfigGetter, err = NewActionConfigGetter(cfg, rm) Expect(err).ShouldNot(HaveOccurred()) - cli = kube.New(newRESTClientGetter(cfg, rm, "")) + dc, err := discovery.NewDiscoveryClientForConfig(cfg) + Expect(err).ShouldNot(HaveOccurred()) + cdc := memory.NewMemCacheClient(dc) + cli = kube.New(newRESTClientGetter(cfg, rm, cdc, "")) Expect(err).ShouldNot(HaveOccurred()) obj = testutil.BuildTestCR(gvk) }) @@ -110,7 +114,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) @@ -131,7 +135,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) @@ -152,7 +156,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) @@ -173,7 +177,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) @@ -194,7 +198,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) @@ -226,7 +230,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) @@ -254,7 +258,7 @@ var _ = Describe("ActionClient", func() { Expect(err).ToNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) _, err = ac.Install(obj.GetName(), obj.GetNamespace(), &chrt, chartutil.Values{}) @@ -291,11 +295,11 @@ var _ = Describe("ActionClient", func() { expectedObj := &unstructured.Unstructured{} expectedObj.SetGroupVersionKind(gvk) var actualObj client.Object - f := ActionClientGetterFunc(func(obj client.Object) (ActionInterface, error) { + f := ActionClientGetterFunc(func(_ context.Context, obj client.Object) (ActionInterface, error) { actualObj = obj return nil, nil }) - _, _ = f.ActionClientFor(expectedObj) + _, _ = f.ActionClientFor(context.Background(), expectedObj) Expect(actualObj.GetObjectKind().GroupVersionKind()).To(Equal(gvk)) }) }) @@ -306,11 +310,11 @@ var _ = Describe("ActionClient", func() { obj = testutil.BuildTestCR(gvk) }) It("should return a valid ActionClient", func() { - actionConfGetter, err := NewActionConfigGetter(cfg, rm, logr.Discard()) + actionConfGetter, err := NewActionConfigGetter(cfg, rm) Expect(err).ShouldNot(HaveOccurred()) acg, err := NewActionClientGetter(actionConfGetter) Expect(err).ToNot(HaveOccurred()) - ac, err := acg.ActionClientFor(obj) + ac, err := acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) }) @@ -326,11 +330,11 @@ var _ = Describe("ActionClient", func() { BeforeEach(func() { obj = testutil.BuildTestCR(gvk) - actionConfigGetter, err := NewActionConfigGetter(cfg, rm, logr.Discard()) + actionConfigGetter, err := NewActionConfigGetter(cfg, rm) Expect(err).ShouldNot(HaveOccurred()) acg, err := NewActionClientGetter(actionConfigGetter) Expect(err).ToNot(HaveOccurred()) - ac, err = acg.ActionClientFor(obj) + ac, err = acg.ActionClientFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) cl, err = client.New(cfg, client.Options{}) diff --git a/pkg/client/actionconfig.go b/pkg/client/actionconfig.go index a2deb3ee..120febf1 100644 --- a/pkg/client/actionconfig.go +++ b/pkg/client/actionconfig.go @@ -28,38 +28,28 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" + "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" ) type ActionConfigGetter interface { - ActionConfigFor(obj client.Object) (*action.Configuration, error) + ActionConfigFor(ctx context.Context, obj client.Object) (*action.Configuration, error) } -func NewActionConfigGetter(cfg *rest.Config, rm meta.RESTMapper, log logr.Logger, opts ...ActionConfigGetterOption) (ActionConfigGetter, error) { - rcg := newRESTClientGetter(cfg, rm, "") - // Setup the debug log function that Helm will use - debugLog := func(format string, v ...interface{}) { - if log.GetSink() != nil { - log.V(1).Info(fmt.Sprintf(format, v...)) - } - } - - kc := kube.New(rcg) - kc.Log = debugLog - - kcs, err := kc.Factory.KubernetesClientSet() +func NewActionConfigGetter(baseRestConfig *rest.Config, rm meta.RESTMapper, opts ...ActionConfigGetterOption) (ActionConfigGetter, error) { + dc, err := discovery.NewDiscoveryClientForConfig(baseRestConfig) if err != nil { - return nil, fmt.Errorf("creating kubernetes client set: %w", err) + return nil, fmt.Errorf("create discovery client: %v", err) } + cdc := memory.NewMemCacheClient(dc) acg := &actionConfigGetter{ - kubeClient: kc, - kubeClientSet: kcs, - debugLog: debugLog, - restClientGetter: rcg.restClientGetter, + baseRestConfig: baseRestConfig, + restMapper: rm, + discoveryClient: cdc, } for _, o := range opts { o(acg) @@ -70,6 +60,11 @@ func NewActionConfigGetter(cfg *rest.Config, rm meta.RESTMapper, log logr.Logger if acg.objectToStorageNamespace == nil { acg.objectToStorageNamespace = getObjectNamespace } + if acg.objectToRestConfig == nil { + acg.objectToRestConfig = func(_ context.Context, _ client.Object, baseRestConfig *rest.Config) (*rest.Config, error) { + return rest.CopyConfig(baseRestConfig), nil + } + } return acg, nil } @@ -97,28 +92,56 @@ func DisableStorageOwnerRefInjection(v bool) ActionConfigGetterOption { } } +func RestConfigMapper(f func(context.Context, client.Object, *rest.Config) (*rest.Config, error)) ActionConfigGetterOption { + return func(getter *actionConfigGetter) { + getter.objectToRestConfig = f + } +} + func getObjectNamespace(obj client.Object) (string, error) { return obj.GetNamespace(), nil } type actionConfigGetter struct { - kubeClient *kube.Client - kubeClientSet kubernetes.Interface - debugLog func(string, ...interface{}) - restClientGetter *restClientGetter + baseRestConfig *rest.Config + restMapper meta.RESTMapper + discoveryClient discovery.CachedDiscoveryInterface objectToClientNamespace ObjectToStringMapper objectToStorageNamespace ObjectToStringMapper + objectToRestConfig func(context.Context, client.Object, *rest.Config) (*rest.Config, error) disableStorageOwnerRefInjection bool } -func (acg *actionConfigGetter) ActionConfigFor(obj client.Object) (*action.Configuration, error) { +func (acg *actionConfigGetter) ActionConfigFor(ctx context.Context, obj client.Object) (*action.Configuration, error) { storageNs, err := acg.objectToStorageNamespace(obj) if err != nil { - return nil, fmt.Errorf("get storage namespace from object: %v", err) + return nil, fmt.Errorf("get storage namespace for object: %v", err) + } + + restConfig, err := acg.objectToRestConfig(ctx, obj, acg.baseRestConfig) + if err != nil { + return nil, fmt.Errorf("get rest config for object: %v", err) + } + + clientNamespace, err := acg.objectToClientNamespace(obj) + if err != nil { + return nil, fmt.Errorf("get client namespace for object: %v", err) } - secretClient := acg.kubeClientSet.CoreV1().Secrets(storageNs) + rcg := newRESTClientGetter(restConfig, acg.restMapper, acg.discoveryClient, clientNamespace) + kc := kube.New(rcg) + kc.Namespace = clientNamespace + + kcs, err := kc.Factory.KubernetesClientSet() + if err != nil { + return nil, fmt.Errorf("create kubernetes clientset: %v", err) + } + + // Setup the debug log function that Helm will use + debugLog := getDebugLogger(ctx) + + secretClient := kcs.CoreV1().Secrets(storageNs) if !acg.disableStorageOwnerRefInjection { ownerRef := metav1.NewControllerRef(obj, obj.GetObjectKind().GroupVersionKind()) secretClient = &ownerRefSecretClient{ @@ -127,27 +150,29 @@ func (acg *actionConfigGetter) ActionConfigFor(obj client.Object) (*action.Confi } } d := driver.NewSecrets(secretClient) - - // Also, use the debug log for the storage driver - d.Log = acg.debugLog + d.Log = debugLog // Initialize the storage backend s := storage.Init(d) - kubeClient := *acg.kubeClient - kubeClient.Namespace, err = acg.objectToClientNamespace(obj) - if err != nil { - return nil, fmt.Errorf("get client namespace from object: %v", err) - } - return &action.Configuration{ - RESTClientGetter: acg.restClientGetter.ForNamespace(kubeClient.Namespace), + RESTClientGetter: rcg, Releases: s, - KubeClient: &kubeClient, - Log: acg.debugLog, + KubeClient: kc, + Log: debugLog, }, nil } +func getDebugLogger(ctx context.Context) func(format string, v ...interface{}) { + logger, err := logr.FromContext(ctx) + if err != nil { + return func(format string, v ...interface{}) {} + } + return func(format string, v ...interface{}) { + logger.V(1).Info(fmt.Sprintf(format, v...)) + } +} + var _ v1.SecretInterface = &ownerRefSecretClient{} type ownerRefSecretClient struct { diff --git a/pkg/client/actionconfig_test.go b/pkg/client/actionconfig_test.go index 020bc555..6a3ee470 100644 --- a/pkg/client/actionconfig_test.go +++ b/pkg/client/actionconfig_test.go @@ -21,7 +21,6 @@ import ( "context" "fmt" - "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "helm.sh/helm/v3/pkg/action" @@ -29,6 +28,7 @@ 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/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" "k8s.io/cli-runtime/pkg/resource" @@ -55,7 +55,7 @@ var _ = Describe("ActionConfig", func() { }) It("should return a valid ActionConfigGetter", func() { - acg, err := NewActionConfigGetter(cfg, nil, logr.Discard()) + acg, err := NewActionConfigGetter(cfg, nil) Expect(err).ShouldNot(HaveOccurred()) Expect(acg).NotTo(BeNil()) }) @@ -77,11 +77,9 @@ 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, logr.Discard(), - ClientNamespaceMapper(clientNsMapper), - ) + acg, err := NewActionConfigGetter(cfg, rm, ClientNamespaceMapper(clientNsMapper)) Expect(err).ToNot(HaveOccurred()) - ac, err := acg.ActionConfigFor(obj) + ac, err := acg.ActionConfigFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac.KubeClient.(*kube.Client).Namespace).To(Equal(clientNs.Name)) Expect(ac.RESTClientGetter.(*namespacedRCG).namespaceConfig.Namespace()).To(Equal(clientNs.Name)) @@ -101,12 +99,10 @@ 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, logr.Discard(), - StorageNamespaceMapper(storageNsMapper), - ) + acg, err := NewActionConfigGetter(cfg, rm, StorageNamespaceMapper(storageNsMapper)) Expect(err).ToNot(HaveOccurred()) - ac, err := acg.ActionConfigFor(obj) + ac, err := acg.ActionConfigFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) By("Creating the storage namespace") @@ -138,12 +134,10 @@ metadata: }) It("should disable storage owner ref injection", func() { - acg, err := NewActionConfigGetter(cfg, rm, logr.Discard(), - DisableStorageOwnerRefInjection(true), - ) + acg, err := NewActionConfigGetter(cfg, rm, DisableStorageOwnerRefInjection(true)) Expect(err).ToNot(HaveOccurred()) - ac, err := acg.ActionConfigFor(obj) + ac, err := acg.ActionConfigFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) By("Installing a release") @@ -167,6 +161,30 @@ metadata: _, err = action.NewUninstall(ac).Run(i.ReleaseName) Expect(err).ToNot(HaveOccurred()) }) + + It("should use a custom rest config mapping", func() { + restConfigMapper := func(ctx context.Context, obj client.Object, cfg *rest.Config) (*rest.Config, error) { + return &rest.Config{ + BearerToken: obj.GetName(), + }, nil + } + acg, err := NewActionConfigGetter(cfg, rm, RestConfigMapper(restConfigMapper)) + Expect(err).ToNot(HaveOccurred()) + + testObject := func(name string) client.Object { + u := unstructured.Unstructured{} + u.SetName(name) + return &u + } + + ac1, err := acg.ActionConfigFor(context.Background(), testObject("test1")) + Expect(err).ToNot(HaveOccurred()) + Expect(ac1.RESTClientGetter.ToRESTConfig()).To(WithTransform(func(c *rest.Config) string { return c.BearerToken }, Equal("test1"))) + + ac2, err := acg.ActionConfigFor(context.Background(), testObject("test2")) + Expect(err).ToNot(HaveOccurred()) + Expect(ac2.RESTClientGetter.ToRESTConfig()).To(WithTransform(func(c *rest.Config) string { return c.BearerToken }, Equal("test2"))) + }) }) }) @@ -182,9 +200,9 @@ metadata: rm, err := apiutil.NewDynamicRESTMapper(cfg, httpClient) Expect(err).ToNot(HaveOccurred()) - acg, err := NewActionConfigGetter(cfg, rm, logr.Discard()) + acg, err := NewActionConfigGetter(cfg, rm) Expect(err).ShouldNot(HaveOccurred()) - ac, err := acg.ActionConfigFor(obj) + ac, err := acg.ActionConfigFor(context.Background(), obj) Expect(err).ToNot(HaveOccurred()) Expect(ac).NotTo(BeNil()) }) diff --git a/pkg/client/postrenderer_test.go b/pkg/client/postrenderer_test.go index f7ef4fe3..e3b28782 100644 --- a/pkg/client/postrenderer_test.go +++ b/pkg/client/postrenderer_test.go @@ -9,6 +9,8 @@ import ( "helm.sh/helm/v3/pkg/kube" "helm.sh/helm/v3/pkg/postrender" corev1 "k8s.io/api/core/v1" + "k8s.io/client-go/discovery" + "k8s.io/client-go/discovery/cached/memory" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -171,13 +173,17 @@ var _ = Describe("ownerPostRenderer", func() { rm, err := apiutil.NewDynamicRESTMapper(cfg, httpClient) Expect(err).ToNot(HaveOccurred()) + dc, err := discovery.NewDiscoveryClientForConfig(cfg) + Expect(err).ToNot(HaveOccurred()) + cdc := memory.NewMemCacheClient(dc) + owner = newTestDeployment([]corev1.Container{{ Name: "test", }}) pr = ownerPostRenderer{ owner: owner, rm: rm, - kubeClient: kube.New(newRESTClientGetter(cfg, rm, owner.GetNamespace())), + kubeClient: kube.New(newRESTClientGetter(cfg, rm, cdc, owner.GetNamespace())), } }) diff --git a/pkg/client/restclientgetter.go b/pkg/client/restclientgetter.go index 818c2f8c..7aab76f5 100644 --- a/pkg/client/restclientgetter.go +++ b/pkg/client/restclientgetter.go @@ -17,32 +17,28 @@ limitations under the License. package client import ( - "sync" - "k8s.io/apimachinery/pkg/api/meta" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/discovery" - cached "k8s.io/client-go/discovery/cached" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ) -func newRESTClientGetter(cfg *rest.Config, rm meta.RESTMapper, ns string) *namespacedRCG { +func newRESTClientGetter(cfg *rest.Config, rm meta.RESTMapper, cachedDiscoveryClient discovery.CachedDiscoveryInterface, ns string) *namespacedRCG { return &namespacedRCG{ restClientGetter: &restClientGetter{ - restConfig: cfg, - restMapper: rm, + restConfig: cfg, + restMapper: rm, + cachedDiscoveryClient: cachedDiscoveryClient, }, namespaceConfig: namespaceClientConfig{ns}, } } type restClientGetter struct { - restConfig *rest.Config - restMapper meta.RESTMapper - - setupDiscoveryClient sync.Once + restConfig *rest.Config + restMapper meta.RESTMapper cachedDiscoveryClient discovery.CachedDiscoveryInterface } @@ -51,20 +47,6 @@ func (c *restClientGetter) ToRESTConfig() (*rest.Config, error) { } func (c *restClientGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { - var ( - dc discovery.DiscoveryInterface - err error - ) - c.setupDiscoveryClient.Do(func() { - dc, err = discovery.NewDiscoveryClientForConfig(c.restConfig) - if err != nil { - return - } - c.cachedDiscoveryClient = cached.NewMemCacheClient(dc) - }) - if err != nil { - return nil, err - } return c.cachedDiscoveryClient, nil } diff --git a/pkg/client/restclientgetter_test.go b/pkg/client/restclientgetter_test.go index 688c4750..ab73a754 100644 --- a/pkg/client/restclientgetter_test.go +++ b/pkg/client/restclientgetter_test.go @@ -21,6 +21,8 @@ import ( . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/discovery" + "k8s.io/client-go/discovery/cached/memory" "k8s.io/client-go/rest" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -32,83 +34,69 @@ var _ = Describe("restClientGetter", func() { rcg genericclioptions.RESTClientGetter ) - When("the config is invalid", func() { - BeforeEach(func() { - rcg = newRESTClientGetter(&rest.Config{ - Host: "ftp:///path/to/foobar", - }, rm, "test-ns") - Expect(rcg).NotTo(BeNil()) - }) + BeforeEach(func() { + var err error - It("returns an error getting the discovery client", func() { - cdc, err := rcg.ToDiscoveryClient() - Expect(err).To(HaveOccurred()) - Expect(cdc).To(BeNil()) - }) + httpClient, err := rest.HTTPClientFor(cfg) + Expect(err).NotTo(HaveOccurred()) + + rm, err = apiutil.NewDynamicRESTMapper(cfg, httpClient) + Expect(err).ToNot(HaveOccurred()) + + dc, err := discovery.NewDiscoveryClientForConfig(cfg) + Expect(err).ToNot(HaveOccurred()) + cdc := memory.NewMemCacheClient(dc) + + rcg = newRESTClientGetter(cfg, rm, cdc, "test-ns") + Expect(rcg).NotTo(BeNil()) }) - When("the config is valid", func() { - BeforeEach(func() { - var err error + It("returns the configured rest config", func() { + restConfig, err := rcg.ToRESTConfig() + Expect(err).ToNot(HaveOccurred()) + Expect(restConfig).To(Equal(cfg)) + }) - httpClient, err := rest.HTTPClientFor(cfg) - Expect(err).NotTo(HaveOccurred()) + It("returns a valid discovery client", func() { + cdc, err := rcg.ToDiscoveryClient() + Expect(err).ToNot(HaveOccurred()) + Expect(cdc).NotTo(BeNil()) - rm, err = apiutil.NewDynamicRESTMapper(cfg, httpClient) - Expect(err).ToNot(HaveOccurred()) + vers, err := cdc.ServerVersion() + Expect(err).ToNot(HaveOccurred()) + Expect(vers.GitTreeState).To(Equal("clean")) + }) - rcg = newRESTClientGetter(cfg, rm, "test-ns") - Expect(rcg).NotTo(BeNil()) - }) + It("returns the configured rest mapper", func() { + restMapper, err := rcg.ToRESTMapper() + Expect(err).ToNot(HaveOccurred()) + Expect(restMapper).To(Equal(rm)) + }) - It("returns the configured rest config", func() { - restConfig, err := rcg.ToRESTConfig() - Expect(err).ToNot(HaveOccurred()) - Expect(restConfig).To(Equal(cfg)) - }) + It("returns a minimal raw kube config loader", func() { + rkcl := rcg.ToRawKubeConfigLoader() + Expect(rkcl).NotTo(BeNil()) - It("returns a valid discovery client", func() { - cdc, err := rcg.ToDiscoveryClient() + By("verifying the namespace", func() { + ns, _, err := rkcl.Namespace() Expect(err).ToNot(HaveOccurred()) - Expect(cdc).NotTo(BeNil()) + Expect(ns).To(Equal("test-ns")) + }) - vers, err := cdc.ServerVersion() + By("verifying raw config is empty", func() { + rc, err := rkcl.RawConfig() Expect(err).ToNot(HaveOccurred()) - Expect(vers.GitTreeState).To(Equal("clean")) + Expect(rc).To(Equal(clientcmdapi.Config{})) }) - It("returns the configured rest mapper", func() { - restMapper, err := rcg.ToRESTMapper() + By("verifying client config is empty", func() { + cc, err := rkcl.ClientConfig() Expect(err).ToNot(HaveOccurred()) - Expect(restMapper).To(Equal(rm)) + Expect(cc).To(BeNil()) }) - It("returns a minimal raw kube config loader", func() { - rkcl := rcg.ToRawKubeConfigLoader() - Expect(rkcl).NotTo(BeNil()) - - By("verifying the namespace", func() { - ns, _, err := rkcl.Namespace() - Expect(err).ToNot(HaveOccurred()) - Expect(ns).To(Equal("test-ns")) - }) - - By("verifying raw config is empty", func() { - rc, err := rkcl.RawConfig() - Expect(err).ToNot(HaveOccurred()) - Expect(rc).To(Equal(clientcmdapi.Config{})) - }) - - By("verifying client config is empty", func() { - cc, err := rkcl.ClientConfig() - Expect(err).ToNot(HaveOccurred()) - Expect(cc).To(BeNil()) - }) - - By("verifying config access is nil", func() { - Expect(rkcl.ConfigAccess()).To(BeNil()) - }) + By("verifying config access is nil", func() { + Expect(rkcl.ConfigAccess()).To(BeNil()) }) }) - }) diff --git a/pkg/reconciler/internal/fake/actionclient.go b/pkg/reconciler/internal/fake/actionclient.go index 92b7da63..e35cb3f4 100644 --- a/pkg/reconciler/internal/fake/actionclient.go +++ b/pkg/reconciler/internal/fake/actionclient.go @@ -17,6 +17,7 @@ limitations under the License. package fake import ( + "context" "errors" "helm.sh/helm/v3/pkg/chart" @@ -40,7 +41,7 @@ type fakeActionClientGetter struct { var _ client.ActionClientGetter = &fakeActionClientGetter{} -func (hcg *fakeActionClientGetter) ActionClientFor(_ crclient.Object) (client.ActionInterface, error) { +func (hcg *fakeActionClientGetter) ActionClientFor(_ context.Context, _ crclient.Object) (client.ActionInterface, error) { if hcg.returnErr != nil { return nil, hcg.returnErr } diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 9602a719..272c8c75 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -24,10 +24,9 @@ import ( "sync" "time" - errs "github.com/pkg/errors" - "github.com/go-logr/logr" sdkhandler "github.com/operator-framework/operator-lib/handler" + errs "github.com/pkg/errors" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" @@ -574,7 +573,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } }() - actionClient, err := r.actionClientGetter.ActionClientFor(obj) + actionClient, err := r.actionClientGetter.ActionClientFor(ctx, obj) if err != nil { u.UpdateStatus( updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonErrorGettingClient, err)), @@ -918,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(), r.log) + actionConfigGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper()) 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 968f5fa5..4a5e9455 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -27,6 +27,7 @@ import ( "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + sdkhandler "github.com/operator-framework/operator-lib/handler" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" @@ -52,8 +53,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" "sigs.k8s.io/yaml" - sdkhandler "github.com/operator-framework/operator-lib/handler" - "github.com/operator-framework/helm-operator-plugins/internal/sdk/controllerutil" "github.com/operator-framework/helm-operator-plugins/pkg/annotation" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -541,7 +540,7 @@ var _ = Describe("Reconciler", func() { Expect(err).ToNot(HaveOccurred()) Expect(r.SetupWithManager(mgr)).To(Succeed()) - ac, err = r.actionClientGetter.ActionClientFor(obj) + ac, err = r.actionClientGetter.ActionClientFor(ctx, obj) Expect(err).ToNot(HaveOccurred()) }) @@ -579,7 +578,7 @@ var _ = Describe("Reconciler", func() { acgErr := errors.New("broken action client getter: error getting action client") By("creating a reconciler with a broken action client getter", func() { - r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) { + r.actionClientGetter = helmclient.ActionClientGetterFunc(func(context.Context, client.Object) (helmclient.ActionInterface, error) { return nil, acgErr }) }) @@ -615,7 +614,7 @@ var _ = Describe("Reconciler", func() { }) It("returns an error getting the release", func() { By("creating a reconciler with a broken action client getter", func() { - r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) { + r.actionClientGetter = helmclient.ActionClientGetterFunc(func(context.Context, client.Object) (helmclient.ActionInterface, error) { cl := helmfake.NewActionClient() return &cl, nil }) @@ -825,7 +824,7 @@ var _ = Describe("Reconciler", func() { acgErr := errors.New("broken action client getter: error getting action client") By("creating a reconciler with a broken action client getter", func() { - r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) { + r.actionClientGetter = helmclient.ActionClientGetterFunc(func(context.Context, client.Object) (helmclient.ActionInterface, error) { return nil, acgErr }) }) @@ -861,7 +860,7 @@ var _ = Describe("Reconciler", func() { }) It("returns an error getting the release", func() { By("creating a reconciler with a broken action client getter", func() { - r.actionClientGetter = helmclient.ActionClientGetterFunc(func(client.Object) (helmclient.ActionInterface, error) { + r.actionClientGetter = helmclient.ActionClientGetterFunc(func(context.Context, client.Object) (helmclient.ActionInterface, error) { cl := helmfake.NewActionClient() return &cl, nil }) @@ -978,9 +977,9 @@ 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(), logr.Discard()) + acg, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper()) Expect(err).ShouldNot(HaveOccurred()) - actionConf, err = acg.ActionConfigFor(obj) + actionConf, err = acg.ActionConfigFor(ctx, obj) Expect(err).ToNot(HaveOccurred()) }) }) diff --git a/testdata/hybrid/memcached-operator/go.mod b/testdata/hybrid/memcached-operator/go.mod index d655241f..826106ed 100644 --- a/testdata/hybrid/memcached-operator/go.mod +++ b/testdata/hybrid/memcached-operator/go.mod @@ -2,7 +2,7 @@ module github.com/example/memcached-operator go 1.21 -toolchain go1.21.8 +toolchain go1.21.9 require ( github.com/onsi/ginkgo/v2 v2.14.0