Refactor the helm and OCI chart clients and respective fakes.

cmd/kubeops/internal/handler/handler.go

@@ -52,13 +52,13 @@ type Options struct {
 // Config represents data needed by each handler to be able to create Helm 3 actions.
 // It cannot be created without a bearer token, so a new one must be created upon each HTTP request.
 type Config struct {
-	ActionConfig  *action.Configuration
-	Options       Options
-	KubeHandler   kube.AuthHandler
-	Resolver      handlerutil.ResolverFactory
-	Cluster       string
-	Token         string
-	userClientSet kubernetes.Interface
+	ActionConfig       *action.Configuration
+	Options            Options
+	KubeHandler        kube.AuthHandler
+	ChartClientFactory chartUtils.ChartClientFactoryInterface
+	Cluster            string
+	Token              string
+	userClientSet      kubernetes.Interface
 }
 
 // WithHandlerConfig takes a dependentHandler and creates a regular (WithParams) handler that,
@@ -110,13 +110,13 @@ func WithHandlerConfig(storageForDriver agent.StorageForDriver, options Options)
 			}
 
 			cfg := Config{
-				Options:       options,
-				ActionConfig:  actionConfig,
-				KubeHandler:   kubeHandler,
-				Cluster:       cluster,
-				Token:         token,
-				Resolver:      &handlerutil.ClientResolver{},
-				userClientSet: userKubeClient,
+				Options:            options,
+				ActionConfig:       actionConfig,
+				KubeHandler:        kubeHandler,
+				Cluster:            cluster,
+				Token:              token,
+				ChartClientFactory: &chartUtils.ChartClientFactory{},
+				userClientSet:      userKubeClient,
 			}
 			f(cfg, w, req, params)
 		}
@@ -191,7 +191,7 @@ func CreateRelease(cfg Config, w http.ResponseWriter, req *http.Request, params
 		chartDetails,
 		appRepo,
 		caCertSecret, authSecret,
-		cfg.Resolver.New(appRepo.Spec.Type, cfg.Options.UserAgent),
+		cfg.ChartClientFactory.New(appRepo.Spec.Type, cfg.Options.UserAgent),
 	)
 	if err != nil {
 		returnErrMessage(err, w)
@@ -245,7 +245,7 @@ func upgradeRelease(cfg Config, w http.ResponseWriter, req *http.Request, params
 		chartDetails,
 		appRepo,
 		caCertSecret, authSecret,
-		cfg.Resolver.New(appRepo.Spec.Type, cfg.Options.UserAgent),
+		cfg.ChartClientFactory.New(appRepo.Spec.Type, cfg.Options.UserAgent),
 	)
 	registrySecrets, err := chartUtils.RegistrySecretsPerDomain(req.Context(), appRepo.Spec.DockerRegistrySecrets, appRepo.Namespace, cfg.userClientSet)
 	if err != nil {

cmd/kubeops/internal/handler/handler_test.go

@@ -13,7 +13,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/apis/apprepository/v1alpha1"
-	fakeHandlerUtils "github.com/kubeapps/kubeapps/pkg/handlerutil/fake"
+	fakeChartUtils "github.com/kubeapps/kubeapps/pkg/chart/fake"
 	kubeappsKube "github.com/kubeapps/kubeapps/pkg/kube"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
@@ -57,7 +57,7 @@ func newConfigFixture(t *testing.T, k *kubefake.FailingKubeClient) *Config {
 				},
 			},
 		},
-		Resolver: &fakeHandlerUtils.ClientResolver{},
+		ChartClientFactory: &fakeChartUtils.ChartClientFactory{},
 		Options: Options{
 			ListLimit: defaultListLimit,
 		},

pkg/agent/agent_test.go

@@ -189,7 +189,7 @@ func TestCreateReleases(t *testing.T) {
 			// Initialize environment for test
 			actionConfig := newActionConfigFixture(t)
 			makeReleases(t, actionConfig, tc.existingReleases)
-			fakechart := chartFake.Client{}
+			fakechart := chartFake.ChartClient{}
 			ch, _ := fakechart.GetChart(&kubechart.Details{
 				ChartName: tc.chartName,
 			}, "")
@@ -678,7 +678,7 @@ func TestUpgradeRelease(t *testing.T) {
 		t.Run(tc.description, func(t *testing.T) {
 			cfg := newActionConfigFixture(t)
 			makeReleases(t, cfg, tc.releases)
-			fakechart := chartFake.Client{}
+			fakechart := chartFake.ChartClient{}
 			ch, _ := fakechart.GetChart(&kubechart.Details{
 				ChartName: tc.chartName,
 			}, "")

pkg/chart/chart.go

@@ -82,34 +82,34 @@ type Details struct {
 // LoadHelmChart returns a helm3 Chart struct from an IOReader
 type LoadHelmChart func(in io.Reader) (*helm3chart.Chart, error)
 
-// Resolver for exposed funcs
-type Resolver interface {
-	InitClient(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error
+// ChartClient for exposed funcs
+type ChartClient interface {
+	Init(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error
 	GetChart(details *Details, repoURL string) (*helm3chart.Chart, error)
 }
 
-// Client struct contains the clients required to retrieve charts info
-type Client struct {
+// HelmRepoClient struct contains the clients required to retrieve charts info
+type HelmRepoClient struct {
 	userAgent string
 	netClient httpclient.Client
 }
 
 // NewChartClient returns a new ChartClient
-func NewChartClient(userAgent string) Resolver {
-	return &Client{
+func NewChartClient(userAgent string) ChartClient {
+	return &HelmRepoClient{
 		userAgent: userAgent,
 	}
 }
 
-// OCIClient struct contains the clients required to retrieve charts info from an OCI registry
-type OCIClient struct {
+// OCIRepoClient struct contains the clients required to retrieve charts info from an OCI registry
+type OCIRepoClient struct {
 	userAgent string
 	puller    helm.ChartPuller
 }
 
 // NewOCIClient returns a new OCIClient
-func NewOCIClient(userAgent string) Resolver {
-	return &OCIClient{
+func NewOCIClient(userAgent string) ChartClient {
+	return &OCIRepoClient{
 		userAgent: userAgent,
 	}
 }
@@ -313,19 +313,19 @@ func GetAppRepoAndRelatedSecrets(appRepoName, appRepoNamespace string, handler k
 	return appRepo, caCertSecret, authSecret, nil
 }
 
-// InitClient returns an HTTP client based on the chart details loading a
+// Init initialises the HTTP client based on the chart details loading a
 // custom CA if provided (as a secret)
-func (c *Client) InitClient(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error {
+func (c *HelmRepoClient) Init(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error {
 	var err error
 	c.netClient, err = kube.InitNetClient(appRepo, caCertSecret, authSecret, http.Header{"User-Agent": []string{c.userAgent}})
 	return err
 }
 
 // GetChart retrieves and loads a Chart from a registry in both
 // v2 and v3 formats.
-func (c *Client) GetChart(details *Details, repoURL string) (*helm3chart.Chart, error) {
+func (c *HelmRepoClient) GetChart(details *Details, repoURL string) (*helm3chart.Chart, error) {
 	if c.netClient == nil {
-		return nil, fmt.Errorf("unable to retrieve chart, InitClient should be called first")
+		return nil, fmt.Errorf("unable to retrieve chart, Init should be called first")
 	}
 	var chart *helm3chart.Chart
 	indexURL := strings.TrimSuffix(strings.TrimSpace(repoURL), "/") + "/index.yaml"
@@ -381,10 +381,10 @@ func RegistrySecretsPerDomain(ctx context.Context, appRepoSecrets []string, name
 	return secretsPerDomain, nil
 }
 
-// InitClient returns an HTTP client based on the chart details loading a
+// Init initialises the HTTP client based on the chart details loading a
 // custom CA if provided (as a secret)
 // TODO(andresmgot): Using a custom CA cert is not supported by ORAS (neither helm), only using the insecure flag
-func (c *OCIClient) InitClient(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error {
+func (c *OCIRepoClient) Init(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error {
 	var err error
 	headers := http.Header{
 		"User-Agent": []string{c.userAgent},
@@ -407,9 +407,9 @@ func (c *OCIClient) InitClient(appRepo *appRepov1.AppRepository, caCertSecret *c
 }
 
 // GetChart retrieves and loads a Chart from a OCI registry
-func (c *OCIClient) GetChart(details *Details, repoURL string) (*helm3chart.Chart, error) {
+func (c *OCIRepoClient) GetChart(details *Details, repoURL string) (*helm3chart.Chart, error) {
 	if c.puller == nil {
-		return nil, fmt.Errorf("unable to retrieve chart, InitClient should be called first")
+		return nil, fmt.Errorf("unable to retrieve chart, Init should be called first")
 	}
 	url, err := url.ParseRequestURI(strings.TrimSpace(repoURL))
 	if err != nil {
@@ -424,3 +424,28 @@ func (c *OCIClient) GetChart(details *Details, repoURL string) (*helm3chart.Char
 
 	return helm3loader.LoadArchive(chartBuffer)
 }
+
+// ChartClientFactoryInterface defines how a ChartClientFactory implementation
+// can return a chart client.
+//
+// This can be implemented with a fake for tests.
+type ChartClientFactoryInterface interface {
+	New(repoType, userAgent string) ChartClient
+}
+
+// ChartClientFactory provides a real implementation of the ChartClientFactory interface
+// returning either an OCI repository client or a traditional helm repository chart client.
+type ChartClientFactory struct{}
+
+// New for ClientResolver
+func (c *ChartClientFactory) New(repoType, userAgent string) ChartClient {
+	var client ChartClient
+	switch repoType {
+	case "oci":
+		client = NewOCIClient(userAgent)
+		break
+	default:
+		client = NewChartClient(userAgent)
+	}
+	return client
+}

pkg/chart/chart_test.go

@@ -546,7 +546,7 @@ func TestGetChart(t *testing.T) {
 		}
 		t.Run(tc.name, func(t *testing.T) {
 			httpClient := newHTTPClient(repoURL, []Details{target}, tc.userAgent)
-			chUtils := Client{
+			chUtils := HelmRepoClient{
 				userAgent: tc.userAgent,
 			}
 			chUtils.netClient = httpClient
@@ -593,7 +593,7 @@ func TestGetChart(t *testing.T) {
 	t.Run("it should fail if the netClient is not instantiated", func(t *testing.T) {
 		cli := NewChartClient("")
 		_, err := cli.GetChart(nil, "")
-		assert.Err(t, fmt.Errorf("unable to retrieve chart, InitClient should be called first"), err)
+		assert.Err(t, fmt.Errorf("unable to retrieve chart, Init should be called first"), err)
 	})
 }
 
@@ -848,8 +848,8 @@ func TestGetRegistrySecretsPerDomain(t *testing.T) {
 func TestOCIClient(t *testing.T) {
 	t.Run("InitClient - Creates puller with User-Agent header", func(t *testing.T) {
 		cli := NewOCIClient("foo")
-		cli.InitClient(&appRepov1.AppRepository{}, &corev1.Secret{}, &corev1.Secret{})
-		helmtest.CheckHeader(t, cli.(*OCIClient).puller, "User-Agent", "foo")
+		cli.Init(&appRepov1.AppRepository{}, &corev1.Secret{}, &corev1.Secret{})
+		helmtest.CheckHeader(t, cli.(*OCIRepoClient).puller, "User-Agent", "foo")
 	})
 
 	t.Run("InitClient - Creates puller with Authorization", func(t *testing.T) {
@@ -871,8 +871,8 @@ func TestOCIClient(t *testing.T) {
 				"custom-secret-key": []byte("Basic Auth"),
 			},
 		}
-		cli.InitClient(appRepo, &corev1.Secret{}, authSecret)
-		helmtest.CheckHeader(t, cli.(*OCIClient).puller, "Authorization", "Basic Auth")
+		cli.Init(appRepo, &corev1.Secret{}, authSecret)
+		helmtest.CheckHeader(t, cli.(*OCIRepoClient).puller, "Authorization", "Basic Auth")
 	})
 
 	t.Run("InitClient - Creates puller with Docker Creds Authorization", func(t *testing.T) {
@@ -894,23 +894,23 @@ func TestOCIClient(t *testing.T) {
 				".dockerconfigjson": []byte(`{"auths":{"foo":{"username":"foo","password":"bar"}}}`),
 			},
 		}
-		err := cli.InitClient(appRepo, &corev1.Secret{}, authSecret)
+		err := cli.Init(appRepo, &corev1.Secret{}, authSecret)
 		if err != nil {
 			t.Fatalf("Unexpected error: %v", err)
 		}
 		// Authorization: Basic base64('foo:bar')
-		helmtest.CheckHeader(t, cli.(*OCIClient).puller, "Authorization", "Basic Zm9vOmJhcg==")
+		helmtest.CheckHeader(t, cli.(*OCIRepoClient).puller, "Authorization", "Basic Zm9vOmJhcg==")
 	})
 
 	t.Run("GetChart - Fails if the puller has not been instantiated", func(t *testing.T) {
 		cli := NewOCIClient("foo")
 		_, err := cli.GetChart(nil, "")
-		assert.Err(t, fmt.Errorf("unable to retrieve chart, InitClient should be called first"), err)
+		assert.Err(t, fmt.Errorf("unable to retrieve chart, Init should be called first"), err)
 	})
 
 	t.Run("GetChart - Fails if the URL is not valid", func(t *testing.T) {
 		cli := NewOCIClient("foo")
-		cli.(*OCIClient).puller = &helmfake.OCIPuller{}
+		cli.(*OCIRepoClient).puller = &helmfake.OCIPuller{}
 		_, err := cli.GetChart(nil, "foo")
 		if !strings.Contains(err.Error(), "invalid URI for request") {
 			t.Errorf("Unexpected error %v", err)
@@ -921,7 +921,7 @@ func TestOCIClient(t *testing.T) {
 		cli := NewOCIClient("foo")
 		data, err := ioutil.ReadFile("./testdata/nginx-5.1.1-apiVersionV2.tgz")
 		assert.NoErr(t, err)
-		cli.(*OCIClient).puller = &helmfake.OCIPuller{
+		cli.(*OCIRepoClient).puller = &helmfake.OCIPuller{
 			ExpectedName: "foo/bar/nginx:5.1.1",
 			Content:      map[string]*bytes.Buffer{"5.1.1": bytes.NewBuffer(data)},
 		}

pkg/chart/fake/chart.go

@@ -24,11 +24,11 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
-// Client implements Resolver inteface
-type Client struct{}
+// ChartClient implements Resolver inteface
+type ChartClient struct{}
 
 // GetChart fake
-func (f *Client) GetChart(details *chartUtils.Details, repoURL string) (*chart3.Chart, error) {
+func (f *ChartClient) GetChart(details *chartUtils.Details, repoURL string) (*chart3.Chart, error) {
 	vals, err := getValues([]byte(details.Values))
 	if err != nil {
 		return nil, err
@@ -50,7 +50,15 @@ func getValues(raw []byte) (map[string]interface{}, error) {
 	return values, nil
 }
 
-// InitClient fake
-func (f *Client) InitClient(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error {
+// Init fake
+func (f *ChartClient) Init(appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret) error {
 	return nil
 }
+
+// ChartClientFactory is a fake implementation of the ChartClientFactory interface.
+type ChartClientFactory struct{}
+
+// New returns a fake ChartClient
+func (c *ChartClientFactory) New(repoType, userAgent string) chartUtils.ChartClient {
+	return &ChartClient{}
+}

pkg/handlerutil/handlerutil.go

@@ -83,34 +83,13 @@ func ParseRequest(req *http.Request) (*chartUtils.Details, error) {
 	return chartDetails, nil
 }
 
-// ResolverFactory interface to return a resolver
-type ResolverFactory interface {
-	New(repoType, userAgent string) chartUtils.Resolver
-}
-
-// ClientResolver implements ResolverFactory
-type ClientResolver struct{}
-
-// New for ClientResolver
-func (c *ClientResolver) New(repoType, userAgent string) chartUtils.Resolver {
-	var cu chartUtils.Resolver
-	switch repoType {
-	case "oci":
-		cu = chartUtils.NewOCIClient(userAgent)
-		break
-	default:
-		cu = chartUtils.NewChartClient(userAgent)
-	}
-	return cu
-}
-
 // GetChart retrieves a chart
-func GetChart(chartDetails *chartUtils.Details, appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret, resolver chartUtils.Resolver) (*chart.Chart, error) {
-	err := resolver.InitClient(appRepo, caCertSecret, authSecret)
+func GetChart(chartDetails *chartUtils.Details, appRepo *appRepov1.AppRepository, caCertSecret *corev1.Secret, authSecret *corev1.Secret, chartClient chartUtils.ChartClient) (*chart.Chart, error) {
+	err := chartClient.Init(appRepo, caCertSecret, authSecret)
 	if err != nil {
 		return nil, err
 	}
-	ch, err := resolver.GetChart(chartDetails, appRepo.Spec.URL)
+	ch, err := chartClient.GetChart(chartDetails, appRepo.Spec.URL)
 	if err != nil {
 		return nil, err
 	}