New K8s clients provider for Helm plugin

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_resources.go

@@ -18,7 +18,7 @@ import (
 )
 
 func (s *Server) getPkgRepositoryResource(ctx context.Context, cluster, namespace string) (dynamic.ResourceInterface, error) {
-	_, dynClient, err := s.GetClients(ctx, cluster)
+	dynClient, err := s.clientGetter.Dynamic(ctx, cluster)
 	if err != nil {
 		return nil, err
 	}
@@ -48,7 +48,7 @@ func (s *Server) getPkgRepository(ctx context.Context, cluster, namespace, ident
 	}
 
 	// Auth and TLS
-	typedClient, _, err := s.GetClients(ctx, cluster)
+	typedClient, err := s.clientGetter.Typed(ctx, cluster)
 	if err != nil {
 		return nil, nil, nil, err
 	}

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go

@@ -65,7 +65,7 @@ type Server struct {
 	// clientGetter is a field so that it can be switched in tests for
 	// a fake client. NewServer() below sets this automatically with the
 	// non-test implementation.
-	clientGetter             clientgetter.ClientGetterFunc
+	clientGetter             clientgetter.ClientProviderInterface
 	globalPackagingNamespace string
 	globalPackagingCluster   string
 	manager                  utils.AssetManager
@@ -135,8 +135,13 @@ func NewServer(configGetter core.KubernetesConfigGetter, globalPackagingCluster
 		log.Fatalf("%s", err)
 	}
 
+	clientProvider, err := clientgetter.NewClientProvider(configGetter, clientgetter.Options{Scheme: scheme})
+	if err != nil {
+		log.Fatalf("%s", err)
+	}
+
 	return &Server{
-		clientGetter: clientgetter.NewClientGetter(configGetter, clientgetter.Options{Scheme: scheme}),
+		clientGetter: clientProvider,
 		actionConfigGetter: func(ctx context.Context, pkgContext *corev1.Context) (*action.Configuration, error) {
 			cluster := pkgContext.GetCluster()
 			// Don't force clients to send a cluster unless we are sure all use-cases
@@ -163,17 +168,19 @@ func (s *Server) GetClients(ctx context.Context, cluster string) (kubernetes.Int
 	if s.clientGetter == nil {
 		return nil, nil, status.Errorf(codes.Internal, "server not configured with configGetter")
 	}
-	// TODO (gfichtenholt) Today this function returns 2 different
-	// clients (typed and dynamic). Now if one looks at the callers, it is clear that
-	// only one client is actually needed for a given scenario.
-	// So for now, in order not to make too many changes, I am going to do more work than
-	// is actually needed by getting *all* clients and returning them.
-	// But we should think about refactoring the callers to ask for only what's needed
-	dynamicClient, err := s.clientGetter.Dynamic(ctx, cluster)
+
+	// Usually only one of the clients is used for a given scenario,
+	// but with this we keep backwards compatibility
+	clients, err := s.clientGetter.GetClients(ctx, cluster)
+	if err != nil {
+		return nil, nil, status.Errorf(codes.FailedPrecondition, fmt.Sprintf("unable to get clients : %v", err))
+	}
+
+	dynamicClient, err := clients.Dynamic()
 	if err != nil {
 		return nil, nil, status.Errorf(codes.FailedPrecondition, fmt.Sprintf("unable to get client : %v", err))
 	}
-	typedClient, err := s.clientGetter.Typed(ctx, cluster)
+	typedClient, err := clients.Typed()
 	if err != nil {
 		return nil, nil, status.Errorf(codes.FailedPrecondition, fmt.Sprintf("unable to get client : %v", err))
 	}
@@ -440,7 +447,7 @@ func (s *Server) hasAccessToNamespace(ctx context.Context, cluster, namespace st
 	if namespace == s.GetGlobalPackagingNamespace() {
 		return nil
 	}
-	client, _, err := s.GetClients(ctx, cluster)
+	client, err := s.clientGetter.Typed(ctx, cluster)
 	if err != nil {
 		return err
 	}
@@ -691,7 +698,7 @@ func installedPkgDetailFromRelease(r *release.Release, ref *corev1.InstalledPack
 func (s *Server) CreateInstalledPackage(ctx context.Context, request *corev1.CreateInstalledPackageRequest) (*corev1.CreateInstalledPackageResponse, error) {
 	log.InfoS("+helm CreateInstalledPackage", "cluster", request.GetTargetContext().GetCluster(), "namespace", request.GetTargetContext().GetNamespace())
 
-	typedClient, _, err := s.GetClients(ctx, s.globalPackagingCluster)
+	typedClient, err := s.clientGetter.Typed(ctx, s.globalPackagingCluster)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Unable to create kubernetes clientset: %v", err)
 	}
@@ -763,7 +770,7 @@ func (s *Server) UpdateInstalledPackage(ctx context.Context, request *corev1.Upd
 		return nil, status.Errorf(codes.FailedPrecondition, "Unable to find the available package used to deploy %q in the namespace %q.", releaseName, installedRef.GetContext().GetNamespace())
 	}
 
-	typedClient, _, err := s.GetClients(ctx, s.globalPackagingCluster)
+	typedClient, err := s.clientGetter.Typed(ctx, s.globalPackagingCluster)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "Unable to create kubernetes clientset: %v", err)
 	}

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server_test.go

@@ -8,10 +8,14 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	apiext "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/kubernetes"
 	"net/url"
 	"os"
 	"regexp"
 	"runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sort"
 	"strings"
 	"testing"
@@ -84,21 +88,23 @@ func TestGetClient(t *testing.T) {
 	if err != nil {
 		log.Fatalf("%s", err)
 	}
-	testClientGetter := func(ctx context.Context, cluster string) (clientgetter.ClientInterfaces, error) {
-		return clientgetter.NewBuilder().
-			WithTyped(typfake.NewSimpleClientset()).
-			WithDynamic(dynfake.NewSimpleDynamicClientWithCustomListKinds(
+
+	clientGetter := clientgetter.NewFixedClientProvider(&clientgetter.ClientGetter{
+		Typed: func() (kubernetes.Interface, error) { return typfake.NewSimpleClientset(), nil },
+		Dynamic: func() (dynamic.Interface, error) {
+			return dynfake.NewSimpleDynamicClientWithCustomListKinds(
 				k8sruntime.NewScheme(),
 				map[schema.GroupVersionResource]string{
 					{Group: "foo", Version: "bar", Resource: "baz"}: "PackageList",
 				},
-			)).Build(), nil
-	}
+			), nil
+		},
+	})
 
 	testCases := []struct {
 		name              string
 		manager           utils.AssetManager
-		clientGetter      clientgetter.ClientGetterFunc
+		clientGetter      clientgetter.ClientProviderInterface
 		statusCodeClient  codes.Code
 		statusCodeManager codes.Code
 	}{
@@ -112,7 +118,7 @@ func TestGetClient(t *testing.T) {
 		{
 			name:              "it returns internal error status when no manager configured",
 			manager:           nil,
-			clientGetter:      testClientGetter,
+			clientGetter:      clientGetter,
 			statusCodeClient:  codes.OK,
 			statusCodeManager: codes.Internal,
 		},
@@ -124,18 +130,25 @@ func TestGetClient(t *testing.T) {
 			statusCodeManager: codes.Internal,
 		},
 		{
-			name:    "it returns failed-precondition when configGetter itself errors",
+			name:    "it returns failed-precondition when clients getter function itself errors",
 			manager: manager,
-			clientGetter: func(ctx context.Context, cluster string) (clientgetter.ClientInterfaces, error) {
+			clientGetter: &clientgetter.ClientProvider{ClientsFunc: func(ctx context.Context, cluster string) (*clientgetter.ClientGetter, error) {
 				return nil, fmt.Errorf("Bang!")
-			},
+			}},
+			statusCodeClient:  codes.FailedPrecondition,
+			statusCodeManager: codes.OK,
+		},
+		{
+			name:              "it returns failed-precondition when clients getter function is not set",
+			manager:           manager,
+			clientGetter:      &clientgetter.ClientProvider{ClientsFunc: nil},
 			statusCodeClient:  codes.FailedPrecondition,
 			statusCodeManager: codes.OK,
 		},
 		{
 			name:         "it returns client without error when configured correctly",
 			manager:      manager,
-			clientGetter: testClientGetter,
+			clientGetter: clientGetter,
 		},
 	}
 
@@ -253,12 +266,11 @@ func makeServer(t *testing.T, authorized bool, actionConfig *action.Configuratio
 			Status: authorizationv1.SubjectAccessReviewStatus{Allowed: authorized},
 		}, nil
 	})
-	clientGetter := func(ctx context.Context, cluster string) (clientgetter.ClientInterfaces, error) {
-		return clientgetter.NewBuilder().
-			WithTyped(clientSet).
-			WithDynamic(dynamicClient).
-			Build(), nil
-	}
+
+	clientGetter := clientgetter.NewFixedClientProvider(&clientgetter.ClientGetter{
+		Typed:   func() (kubernetes.Interface, error) { return clientSet, nil },
+		Dynamic: func() (dynamic.Interface, error) { return dynamicClient, nil },
+	})
 
 	// Creating the SQL mock manager
 	mock, cleanup, manager := setMockManager(t)
@@ -311,25 +323,24 @@ func newServerWithSecretsAndRepos(t *testing.T, secrets []k8sruntime.Object, uns
 		log.Fatalf("%s", err)
 	}
 
-	clientGetter := func(context.Context, string) (clientgetter.ClientInterfaces, error) {
-		return clientgetter.
-			NewBuilder().
-			WithTyped(typedClient).
-			WithApiExt(apiExtIfc).
-			WithControllerRuntime(ctrlClient).
-			WithDynamic(dynfake.NewSimpleDynamicClientWithCustomListKinds(
-				scheme,
-				map[schema.GroupVersionResource]string{
-					{
-						Group:    v1alpha1.SchemeGroupVersion.Group,
-						Version:  v1alpha1.SchemeGroupVersion.Version,
-						Resource: AppRepositoryResource,
-					}: AppRepositoryResource + "List",
-				},
-				unstructuredObjs...,
-			)).
-			Build(), nil
-	}
+	dynClient := dynfake.NewSimpleDynamicClientWithCustomListKinds(
+		scheme,
+		map[schema.GroupVersionResource]string{
+			{
+				Group:    v1alpha1.SchemeGroupVersion.Group,
+				Version:  v1alpha1.SchemeGroupVersion.Version,
+				Resource: AppRepositoryResource,
+			}: AppRepositoryResource + "List",
+		},
+		unstructuredObjs...,
+	)
+
+	clientGetter := clientgetter.NewFixedClientProvider(&clientgetter.ClientGetter{
+		Typed:             func() (kubernetes.Interface, error) { return typedClient, nil },
+		Dynamic:           func() (dynamic.Interface, error) { return dynClient, nil },
+		ControllerRuntime: func() (client.WithWatch, error) { return ctrlClient, nil },
+		ApiExt:            func() (apiext.Interface, error) { return apiExtIfc, nil },
+	})
 
 	return &Server{
 		clientGetter:             clientGetter,