Refactor repo validation so as to not require http-only validators.

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

@@ -248,7 +248,7 @@ func TestAddPackageRepository(t *testing.T) {
 		expectedAuthCreatedSecret   *apiv1.Secret
 		expectedDockerCreatedSecret *apiv1.Secret
 		userManagedSecrets          bool
-		repoClientGetter            newRepoClient
+		repoClientGetter            repositoryClientGetter
 		expectedGlobalSecret        *apiv1.Secret
 	}{
 		{

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

@@ -37,15 +37,11 @@ func (s *Server) ValidateRepository(appRepo *apprepov1alpha1.AppRepository, secr
 		return connect.NewError(connect.CodeFailedPrecondition, fmt.Errorf("docker registry secrets cannot be set for app repositories available in all namespaces"))
 	}
 
-	client, err := s.repoClientGetter(appRepo, secret)
+	validator, err := getValidator(appRepo, secret, s.repoClientGetter)
 	if err != nil {
 		return err
 	}
-	httpValidator, err := getValidator(appRepo)
-	if err != nil {
-		return err
-	}
-	resp, err := httpValidator.Validate(client)
+	resp, err := validator.Validate()
 	if err != nil {
 		return err
 	} else if resp.Code >= 400 {
@@ -56,26 +52,21 @@ func (s *Server) ValidateRepository(appRepo *apprepov1alpha1.AppRepository, secr
 	}
 }
 
-// getValidator return appropriate HttpValidator interface for OCI and non-OCI Repos
-func getValidator(appRepo *apprepov1alpha1.AppRepository) (HttpValidator, error) {
+// getValidator return appropriate RepositoryValidator interface for OCI and
+// non-OCI Repos
+func getValidator(appRepo *apprepov1alpha1.AppRepository, secret *corev1.Secret, clientGetter repositoryClientGetter) (RepositoryValidator, error) {
 	if appRepo.Spec.Type == "oci" {
 		// For the OCI case, we want to validate that all the given repositories are valid
 		return HelmOCIValidator{
-			AppRepo: appRepo,
+			AppRepo:      appRepo,
+			Secret:       secret,
+			ClientGetter: clientGetter,
 		}, nil
 	} else {
-		repoURL := strings.TrimSuffix(strings.TrimSpace(appRepo.Spec.URL), "/")
-		parsedURL, err := url.ParseRequestURI(repoURL)
-		if err != nil {
-			return nil, err
-		}
-		parsedURL.Path = path.Join(parsedURL.Path, "index.yaml")
-		req, err := http.NewRequest("GET", parsedURL.String(), nil)
-		if err != nil {
-			return nil, err
-		}
 		return HelmNonOCIValidator{
-			Req: req,
+			AppRepo:      appRepo,
+			Secret:       secret,
+			ClientGetter: clientGetter,
 		}, nil
 	}
 }
@@ -256,20 +247,36 @@ func ValidateOCIAppRepository(appRepo *apprepov1alpha1.AppRepository, cli httpcl
 	return true, nil
 }
 
-// HttpValidator is an interface for checking the validity of an AppRepo via Http requests.
-type HttpValidator interface {
+// RepositoryValidator is an interface for checking the validity of an AppRepository
+type RepositoryValidator interface {
 	// Validate returns a validation response.
-	Validate(cli httpclient.Client) (*ValidationResponse, error)
+	Validate() (*ValidationResponse, error)
 }
 
 // HelmNonOCIValidator is an HttpValidator for non-OCI Helm repositories.
 type HelmNonOCIValidator struct {
-	Req *http.Request
+	AppRepo      *apprepov1alpha1.AppRepository
+	Secret       *corev1.Secret
+	ClientGetter repositoryClientGetter
 }
 
-func (r HelmNonOCIValidator) Validate(cli httpclient.Client) (*ValidationResponse, error) {
+func (r HelmNonOCIValidator) Validate() (*ValidationResponse, error) {
+	repoURL := strings.TrimSuffix(strings.TrimSpace(r.AppRepo.Spec.URL), "/")
+	parsedURL, err := url.ParseRequestURI(repoURL)
+	if err != nil {
+		return nil, err
+	}
+	parsedURL.Path = path.Join(parsedURL.Path, "index.yaml")
+	req, err := http.NewRequest("GET", parsedURL.String(), nil)
+	if err != nil {
+		return nil, err
+	}
+	cli, err := r.ClientGetter(r.AppRepo, r.Secret)
+	if err != nil {
+		return nil, err
+	}
 
-	res, err := cli.Do(r.Req)
+	res, err := cli.Do(req)
 	if err != nil {
 		// If the request fail, it's not an internal error
 		return &ValidationResponse{Code: 400, Message: err.Error()}, nil
@@ -287,16 +294,22 @@ func (r HelmNonOCIValidator) Validate(cli httpclient.Client) (*ValidationRespons
 }
 
 type HelmOCIValidator struct {
-	AppRepo *apprepov1alpha1.AppRepository
+	AppRepo      *apprepov1alpha1.AppRepository
+	Secret       *corev1.Secret
+	ClientGetter repositoryClientGetter
 }
 
-func (r HelmOCIValidator) Validate(cli httpclient.Client) (*ValidationResponse, error) {
+func (v HelmOCIValidator) Validate() (*ValidationResponse, error) {
 
 	var response *ValidationResponse
 	response = &ValidationResponse{Code: 200, Message: "OK"}
 
+	cli, err := v.ClientGetter(v.AppRepo, v.Secret)
+	if err != nil {
+		return nil, err
+	}
 	// If there was an error validating the OCI repository, it's not an internal error.
-	isValidRepo, err := ValidateOCIAppRepository(r.AppRepo, cli)
+	isValidRepo, err := ValidateOCIAppRepository(v.AppRepo, cli)
 	if err != nil || !isValidRepo {
 		response = &ValidationResponse{Code: 400, Message: err.Error()}
 	}

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

@@ -18,6 +18,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/vmware-tanzu/kubeapps/cmd/apprepository-controller/pkg/apis/apprepository/v1alpha1"
 	httpclient "github.com/vmware-tanzu/kubeapps/pkg/http-client"
+	corev1 "k8s.io/api/core/v1"
 	log "k8s.io/klog/v2"
 )
 
@@ -33,41 +34,36 @@ func (f *fakeHTTPCli) Do(r *http.Request) (*http.Response, error) {
 }
 
 func TestNonOCIValidate(t *testing.T) {
-	validRequest, err := http.NewRequest("GET", "http://example.com/index.yaml", strings.NewReader(""))
+	validRequest, err := http.NewRequest("GET", "https://example.com/index.yaml", nil)
 	if err != nil {
 		t.Fatalf("%+v", err)
 	}
 
 	testCases := []struct {
 		name             string
-		httpValidator    HelmNonOCIValidator
 		fakeHttpError    error
 		fakeRepoResponse *http.Response
 		expectedResponse *ValidationResponse
 	}{
 		{
 			name:             "it returns 200 OK validation response if there is no error and the external response is 200",
-			httpValidator:    HelmNonOCIValidator{Req: validRequest},
 			fakeRepoResponse: &http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader([]byte("OK")))},
 			expectedResponse: &ValidationResponse{Code: 200, Message: "OK"},
 		},
 		{
 			name:             "it does not include the body of the upstream response when validation succeeds",
-			httpValidator:    HelmNonOCIValidator{Req: validRequest},
 			fakeRepoResponse: &http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader([]byte("10 Mb of data")))},
 			expectedResponse: &ValidationResponse{Code: 200, Message: "OK"},
 		},
 		{
 			name:             "it returns an error from the response with the body text if validation fails",
 			fakeRepoResponse: &http.Response{StatusCode: 401, Body: io.NopCloser(bytes.NewReader([]byte("It failed because of X and Y")))},
 			expectedResponse: &ValidationResponse{Code: 401, Message: "It failed because of X and Y"},
-			httpValidator:    HelmNonOCIValidator{Req: validRequest},
 		},
 		{
 			name:             "it returns a 400 error if the validation cannot be run",
 			fakeHttpError:    fmt.Errorf("client.Do returns an error"),
 			expectedResponse: &ValidationResponse{Code: 400, Message: "client.Do returns an error"},
-			httpValidator:    HelmNonOCIValidator{Req: validRequest},
 		},
 	}
 
@@ -81,12 +77,24 @@ func TestNonOCIValidate(t *testing.T) {
 				err:      tc.fakeHttpError,
 			}
 
-			response, err := tc.httpValidator.Validate(fakeClient)
+			httpValidator := HelmNonOCIValidator{
+				ClientGetter: func(*v1alpha1.AppRepository, *corev1.Secret) (httpclient.Client, error) {
+					return fakeClient, nil
+				},
+				AppRepo: &v1alpha1.AppRepository{
+					Spec: v1alpha1.AppRepositorySpec{
+						Type: "oci",
+						URL:  "https://example.com",
+					},
+				},
+			}
+
+			response, err := httpValidator.Validate()
 			if err != nil {
 				t.Errorf("Unexpected error %v", err)
 			}
 
-			if got, want := fakeClient.request, tc.httpValidator.Req; !cmp.Equal(want, got, cmpOpts...) {
+			if got, want := fakeClient.request, validRequest; !cmp.Equal(want, got, cmpOpts...) {
 				t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, cmpOpts...))
 			}
 
@@ -346,10 +354,14 @@ func TestOCIValidate(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			ts := makeTestOCIServer(t, registryName, tc.repos, "")
 			defer ts.Close()
+
 			// Use the test servers host/port as repo url.
 			tc.validator.AppRepo.Spec.URL = fmt.Sprintf("%s/%s", ts.URL, registryName)
 
-			response, err := tc.validator.Validate(httpclient.New())
+			// Use the actual client getter since we're using a test double.
+			tc.validator.ClientGetter = newRepositoryClient
+
+			response, err := tc.validator.Validate()
 			if err != nil {
 				t.Fatalf("%+v", err)
 			}

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

@@ -59,7 +59,7 @@ const (
 )
 
 type createRelease func(*action.Configuration, string, string, string, *chart.Chart, map[string]string, int32) (*release.Release, error)
-type newRepoClient func(appRepo *appRepov1.AppRepository, secret *corek8sv1.Secret) (httpclient.Client, error)
+type repositoryClientGetter func(appRepo *appRepov1.AppRepository, secret *corek8sv1.Secret) (httpclient.Client, error)
 
 // Server implements the helm packages v1alpha1 interface.
 type Server struct {
@@ -80,7 +80,7 @@ type Server struct {
 	kubeappsCluster                 string // Specifies the cluster on which Kubeapps is installed.
 	kubeappsNamespace               string // Namespace in which Kubeapps is installed
 	pluginConfig                    *common.HelmPluginConfig
-	repoClientGetter                newRepoClient
+	repoClientGetter                repositoryClientGetter
 	clientQPS                       float32
 }
 

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

@@ -160,7 +160,7 @@ func addTlsToSecret(secret *apiv1.Secret, pub, priv, ca []byte) *apiv1.Secret {
 	return secret
 }
 
-func newRepoHttpClient(responses map[string]*http.Response) newRepoClient {
+func newRepoHttpClient(responses map[string]*http.Response) repositoryClientGetter {
 	return func(appRepo *appRepov1.AppRepository, secret *apiv1.Secret) (httpclient.Client, error) {
 		return &fakeHTTPClient{
 			responses: responses,