From f915dd0984d33907472e4659e4bb2f3ca399d403 Mon Sep 17 00:00:00 2001 From: Max Jonas Werner Date: Tue, 26 Jul 2022 17:10:24 +0200 Subject: [PATCH] don't fetch tags when exact version is used in HelmRepository Taking this shortcut has two benefits: 1. It allows charts to be fetched from AWS's public container registry at public.ecr.aws 2. It makes reconciling a HelmChart faster by skipping one or more potentially expensive API calls to the registry. I adapted the unit tests to the new behavior that the OCIChartRepository doesn't fail anymore for the case where a specific chart version has been requested that doesn't actually exist in the registry. refs #845 Signed-off-by: Max Jonas Werner fix tests --- internal/helm/chart/builder_remote_test.go | 19 ++++- .../helm/chart/dependency_manager_test.go | 8 +- .../helm/repository/oci_chart_repository.go | 19 ++++- .../repository/oci_chart_repository_test.go | 85 ++++++++++--------- 4 files changed, 84 insertions(+), 47 deletions(-) diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go index e76503e43..bf59219ed 100644 --- a/internal/helm/chart/builder_remote_test.go +++ b/internal/helm/chart/builder_remote_test.go @@ -64,10 +64,14 @@ func (m *mockRegistryClient) Logout(url string, opts ...registry.LogoutOption) e type mockIndexChartGetter struct { IndexResponse []byte ChartResponse []byte + ErrorResponse error requestedURL string } func (g *mockIndexChartGetter) Get(u string, _ ...helmgetter.Option) (*bytes.Buffer, error) { + if g.ErrorResponse != nil { + return nil, g.ErrorResponse + } g.requestedURL = u r := g.ChartResponse if strings.HasSuffix(u, "index.yaml") { @@ -248,6 +252,15 @@ func TestRemoteBuilder_BuildFromOCIChatRepository(t *testing.T) { RegistryClient: registryClient, } } + mockRepoWithoutChart := func() *repository.OCIChartRepository { + return &repository.OCIChartRepository{ + URL: *u, + Client: &mockIndexChartGetter{ + ErrorResponse: fmt.Errorf("chart doesn't exist"), + }, + RegistryClient: registryClient, + } + } tests := []struct { name string @@ -278,8 +291,8 @@ func TestRemoteBuilder_BuildFromOCIChatRepository(t *testing.T) { { name: "chart version not in repository", reference: RemoteReference{Name: "grafana", Version: "1.1.1"}, - repository: mockRepo(), - wantErr: "failed to get chart version for remote reference", + repository: mockRepoWithoutChart(), + wantErr: "failed to download chart for remote reference", }, { name: "invalid version metadata", @@ -334,7 +347,7 @@ func TestRemoteBuilder_BuildFromOCIChatRepository(t *testing.T) { cb, err := b.Build(context.TODO(), tt.reference, targetPath, tt.buildOpts) if tt.wantErr != "" { - g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(HaveOccurred(), "expected error '%s'", tt.wantErr) g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) g.Expect(cb).To(BeZero()) return diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go index 8a66c9797..d63e5f153 100644 --- a/internal/helm/chart/dependency_manager_test.go +++ b/internal/helm/chart/dependency_manager_test.go @@ -604,6 +604,8 @@ func TestDependencyManager_addRemoteOCIDependency(t *testing.T) { }, wantFunc: func(g *WithT, c *helmchart.Chart) { g.Expect(c.Dependencies()).To(HaveLen(1)) + dep := c.Dependencies()[0] + g.Expect(dep).NotTo(BeNil()) }, }, { @@ -633,9 +635,7 @@ func TestDependencyManager_addRemoteOCIDependency(t *testing.T) { Scheme: "oci", Host: "example.com", }, - Client: &mockGetter{ - Response: chartB, - }, + Client: &mockGetter{}, RegistryClient: &mockTagsGetter{ tags: map[string][]string{ "helmchart": {"0.1.0"}, @@ -648,7 +648,7 @@ func TestDependencyManager_addRemoteOCIDependency(t *testing.T) { Version: "0.2.0", Repository: "oci://example.com", }, - wantErr: "could not locate a version matching provided version string 0.2.0", + wantErr: "failed to load downloaded archive of version '0.2.0'", }, { name: "chart load error", diff --git a/internal/helm/repository/oci_chart_repository.go b/internal/helm/repository/oci_chart_repository.go index b9bb21312..417a52818 100644 --- a/internal/helm/repository/oci_chart_repository.go +++ b/internal/helm/repository/oci_chart_repository.go @@ -131,10 +131,25 @@ func NewOCIChartRepository(repositoryURL string, chartRepoOpts ...OCIChartReposi // stable version will be returned and prerelease versions will be ignored. // adapted from https://github.com/helm/helm/blob/49819b4ef782e80b0c7f78c30bd76b51ebb56dc8/pkg/downloader/chart_downloader.go#L162 func (r *OCIChartRepository) GetChartVersion(name, ver string) (*repo.ChartVersion, error) { - // Find chart versions matching the given name. - // Either in an index file or from a registry. + cpURL := r.URL cpURL.Path = path.Join(cpURL.Path, name) + + // if ver is a valid semver version, take a shortcut here so we don't need to list all tags which can be an + // expensive operation. + if _, err := version.ParseVersion(ver); err == nil { + return &repo.ChartVersion{ + URLs: []string{fmt.Sprintf("%s:%s", cpURL.String(), ver)}, + Metadata: &chart.Metadata{ + Name: name, + Version: ver, + }, + }, nil + } + + // ver doesn't denote a concrete version so we interpret it as a semver range and try to find the best-matching + // version from the list of tags in the registry. + cvs, err := r.getTags(cpURL.String()) if err != nil { return nil, fmt.Errorf("could not get tags for %q: %s", name, err) diff --git a/internal/helm/repository/oci_chart_repository_test.go b/internal/helm/repository/oci_chart_repository_test.go index 89e7b470e..1ef12a860 100644 --- a/internal/helm/repository/oci_chart_repository_test.go +++ b/internal/helm/repository/oci_chart_repository_test.go @@ -118,59 +118,68 @@ func TestOCIChartRepository_Get(t *testing.T) { testURL := "oci://localhost:5000/my_repo" testCases := []struct { - name string - url string - version string - expected string - expectedErr string + name string + registryClient RegistryClient + url string + version string + expected string + expectedErr string }{ { - name: "should return latest stable version", - version: "", - url: testURL, - expected: "1.0.0", + name: "should return latest stable version", + registryClient: registryClient, + version: "", + url: testURL, + expected: "1.0.0", }, { - name: "should return latest stable version (asterisk)", - version: "*", - url: testURL, - expected: "1.0.0", + name: "should return latest stable version (asterisk)", + registryClient: registryClient, + version: "*", + url: testURL, + expected: "1.0.0", }, { - name: "should return latest stable version (semver range)", - version: ">=0.1.5", - url: testURL, - expected: "1.0.0", + name: "should return latest stable version (semver range)", + registryClient: registryClient, + version: ">=0.1.5", + url: testURL, + expected: "1.0.0", }, { - name: "should return 0.2.0 (semver range)", - version: "0.2.x", - url: testURL, - expected: "0.2.0", + name: "should return 0.2.0 (semver range)", + registryClient: registryClient, + version: "0.2.x", + url: testURL, + expected: "0.2.0", }, { - name: "should return a perfect match", - version: "0.1.0", - url: testURL, - expected: "0.1.0", + name: "should return a perfect match", + registryClient: nil, + version: "0.1.0", + url: testURL, + expected: "0.1.0", }, { - name: "should return 0.10.0", - version: "0.*", - url: testURL, - expected: "0.10.0", + name: "should return 0.10.0", + registryClient: registryClient, + version: "0.*", + url: testURL, + expected: "0.10.0", }, { - name: "should an error for unfunfilled range", - version: ">2.0.0", - url: testURL, - expectedErr: "could not locate a version matching provided version string >2.0.0", + name: "should an error for unfulfilled range", + registryClient: registryClient, + version: ">2.0.0", + url: testURL, + expectedErr: "could not locate a version matching provided version string >2.0.0", }, { - name: "shouldn't error out with trailing slash", - version: "", - url: "oci://localhost:5000/my_repo/", - expected: "1.0.0", + name: "shouldn't error out with trailing slash", + registryClient: registryClient, + version: "", + url: "oci://localhost:5000/my_repo/", + expected: "1.0.0", }, } @@ -178,7 +187,7 @@ func TestOCIChartRepository_Get(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - r, err := NewOCIChartRepository(tc.url, WithOCIRegistryClient(registryClient), WithOCIGetter(providers)) + r, err := NewOCIChartRepository(tc.url, WithOCIRegistryClient(tc.registryClient), WithOCIGetter(providers)) g.Expect(err).ToNot(HaveOccurred()) g.Expect(r).ToNot(BeNil())