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 <max@e13.dev>

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

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",

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)

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())