From d5b3bbd716feb8ea9c7d1c38427d9c1144f1a996 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Tue, 15 Aug 2023 16:05:39 +1000 Subject: [PATCH] Refactor repo validation so as to not require http-only validators. Signed-off-by: Michael Nelson --- .../packages/v1alpha1/repositories_test.go | 2 +- .../v1alpha1/repositories_validation.go | 71 +++++++++++-------- .../v1alpha1/repositories_validation_test.go | 30 +++++--- .../plugins/helm/packages/v1alpha1/server.go | 4 +- .../helm/packages/v1alpha1/test_util_test.go | 2 +- 5 files changed, 67 insertions(+), 42 deletions(-) diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go index a92322173ff..cd1659766a1 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go +++ b/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 }{ { diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation.go index 19fe6bb2817..ec8b962bfa2 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation.go +++ b/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()} } diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation_test.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation_test.go index 33bb16e2f65..18a6206850c 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_validation_test.go +++ b/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,27 +34,24 @@ 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"}, }, @@ -61,13 +59,11 @@ func TestNonOCIValidate(t *testing.T) { 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) } diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go index f71bf9fdac9..70debbb9712 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go +++ b/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 } diff --git a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/test_util_test.go b/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/test_util_test.go index 624a2a89ec2..1b3ae486e42 100644 --- a/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/test_util_test.go +++ b/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,