fix: fix incorrect OCI Helm registiries assumptions (#5888)

Signed-off-by: Alexander Matyushentsev <[email protected]>

pkg/apis/application/v1alpha1/types.go

@@ -191,7 +191,7 @@ func (a *ApplicationSource) IsHelmOci() bool {
 	if a.Chart == "" {
 		return false
 	}
-	return helm.IsHelmOciChart(a.Chart)
+	return helm.IsHelmOciRepo(a.RepoURL)
 }
 
 // IsZero returns true if the application source is considered empty

reposerver/repository/repository.go

@@ -232,17 +232,13 @@ func (s *Service) runRepoOperation(
 	}
 
 	if source.IsHelm() {
-		version, err := semver.NewVersion(revision)
-		if err != nil {
-			return nil, err
-		}
 		if settings.noCache {
-			err = helmClient.CleanChartCache(source.Chart, version)
+			err = helmClient.CleanChartCache(source.Chart, revision)
 			if err != nil {
 				return nil, err
 			}
 		}
-		chartPath, closer, err := helmClient.ExtractChart(source.Chart, version)
+		chartPath, closer, err := helmClient.ExtractChart(source.Chart, revision)
 		if err != nil {
 			return nil, err
 		}
@@ -1301,13 +1297,12 @@ func (s *Service) newClientResolveRevision(repo *v1alpha1.Repository, revision s
 }
 
 func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revision string, chart string, noCache bool) (helm.Client, string, error) {
-	helmClient := s.newHelmClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI || helm.IsHelmOciChart(chart))
-	if helm.IsVersion(revision) {
+	enableOCI := repo.EnableOCI || helm.IsHelmOciRepo(repo.Repo)
+	helmClient := s.newHelmClient(repo.Repo, repo.GetHelmCreds(), enableOCI)
+	// OCI helm registers don't support semver ranges. Assuming that given revision is exact version
+	if helm.IsVersion(revision) || enableOCI {
 		return helmClient, revision, nil
 	}
-	if repo.EnableOCI {
-		return nil, "", errors.New("OCI helm registers don't support semver ranges. Exact revision must be specified.")
-	}
 	constraints, err := semver.NewConstraint(revision)
 	if err != nil {
 		return nil, "", fmt.Errorf("invalid revision '%s': %v", revision, err)

reposerver/repository/repository_test.go

@@ -14,7 +14,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/Masterminds/semver"
 	"github.com/ghodss/yaml"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -64,10 +63,6 @@ func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client)
 }
 
 func newServiceWithOpt(cf clientFunc) (*Service, *gitmocks.Client) {
-	// root, err := filepath.Abs(root)
-	// if err != nil {
-	// 	panic(err)
-	// }
 	helmClient := &helmmocks.Client{}
 	gitClient := &gitmocks.Client{}
 	cf(gitClient)
@@ -77,9 +72,9 @@ func newServiceWithOpt(cf clientFunc) (*Service, *gitmocks.Client) {
 	), RepoServerInitConstants{ParallelismLimit: 1})
 
 	chart := "my-chart"
-	version := semver.MustParse("1.1.0")
+	version := "1.1.0"
 	helmClient.On("GetIndex", true).Return(&helm.Index{Entries: map[string]helm.Entries{
-		chart: {{Version: "1.0.0"}, {Version: version.String()}},
+		chart: {{Version: "1.0.0"}, {Version: version}},
 	}}, nil)
 	helmClient.On("ExtractChart", chart, version).Return("./testdata/my-chart", io.NopCloser, nil)
 	helmClient.On("CleanChartCache", chart, version).Return(nil)

util/argo/argo.go

@@ -3,6 +3,7 @@ package argo
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -172,6 +173,9 @@ func TestRepo(repo *argoappv1.Repository) error {
 		},
 		"helm": func() error {
 			if repo.EnableOCI {
+				if !helm.IsHelmOciRepo(repo.Repo) {
+					return errors.New("OCI Helm repository URL should include hostname and port only")
+				}
 				_, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI).TestHelmOCI()
 				return err
 			} else {

util/argo/argo_test.go

@@ -743,3 +743,9 @@ func TestFilterByName(t *testing.T) {
 		assert.Len(t, res, 0)
 	})
 }
+
+func TestTestRepoOCI(t *testing.T) {
+	err := TestRepo(&argoappv1.Repository{Repo: "https://demo.goharbor.io", Type: "helm", EnableOCI: true})
+	assert.Error(t, err)
+	assert.Equal(t, "OCI Helm repository URL should include hostname and port only", err.Error())
+}

util/helm/client.go

@@ -16,16 +16,13 @@ import (
 	"strings"
 	"time"
 
-	"github.com/argoproj/argo-cd/common"
-
-	"github.com/argoproj/argo-cd/util/env"
-
-	"github.com/Masterminds/semver"
 	"github.com/argoproj/pkg/sync"
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/yaml.v2"
 
+	"github.com/argoproj/argo-cd/common"
 	"github.com/argoproj/argo-cd/util/cache"
+	"github.com/argoproj/argo-cd/util/env"
 	executil "github.com/argoproj/argo-cd/util/exec"
 	"github.com/argoproj/argo-cd/util/io"
 )
@@ -47,8 +44,8 @@ type Creds struct {
 }
 
 type Client interface {
-	CleanChartCache(chart string, version *semver.Version) error
-	ExtractChart(chart string, version *semver.Version) (string, io.Closer, error)
+	CleanChartCache(chart string, version string) error
+	ExtractChart(chart string, version string) (string, io.Closer, error)
 	GetIndex(noCache bool) (*Index, error)
 	TestHelmOCI() (bool, error)
 }
@@ -97,11 +94,11 @@ func (c *nativeHelmChart) ensureHelmChartRepoPath() error {
 	return nil
 }
 
-func (c *nativeHelmChart) CleanChartCache(chart string, version *semver.Version) error {
+func (c *nativeHelmChart) CleanChartCache(chart string, version string) error {
 	return os.RemoveAll(c.getCachedChartPath(chart, version))
 }
 
-func (c *nativeHelmChart) ExtractChart(chart string, version *semver.Version) (string, io.Closer, error) {
+func (c *nativeHelmChart) ExtractChart(chart string, version string) (string, io.Closer, error) {
 	err := c.ensureHelmChartRepoPath()
 	if err != nil {
 		return "", nil, err
@@ -158,13 +155,13 @@ func (c *nativeHelmChart) ExtractChart(chart string, version *semver.Version) (s
 			}
 
 			// 'helm chart pull' ensures that chart is downloaded into local repository cache
-			_, err = helmCmd.ChartPull(c.repoURL, chart, version.String())
+			_, err = helmCmd.ChartPull(c.repoURL, chart, version)
 			if err != nil {
 				return "", nil, err
 			}
 
 			// 'helm chart export' copies cached chart into temp directory
-			_, err = helmCmd.ChartExport(c.repoURL, chart, version.String(), tempDest)
+			_, err = helmCmd.ChartExport(c.repoURL, chart, version, tempDest)
 			if err != nil {
 				return "", nil, err
 			}
@@ -177,7 +174,7 @@ func (c *nativeHelmChart) ExtractChart(chart string, version *semver.Version) (s
 				return "", nil, err
 			}
 		} else {
-			_, err = helmCmd.Fetch(c.repoURL, chart, version.String(), tempDest, c.creds)
+			_, err = helmCmd.Fetch(c.repoURL, chart, version, tempDest, c.creds)
 			if err != nil {
 				return "", nil, err
 			}
@@ -352,11 +349,16 @@ func normalizeChartName(chart string) string {
 	return nc
 }
 
-func (c *nativeHelmChart) getCachedChartPath(chart string, version *semver.Version) string {
-	return path.Join(c.repoPath, fmt.Sprintf("%s-%v.tgz", strings.ReplaceAll(chart, "/", "_"), version))
+func (c *nativeHelmChart) getCachedChartPath(chart string, version string) string {
+	return path.Join(c.repoPath, fmt.Sprintf("%s-%s.tgz", strings.ReplaceAll(chart, "/", "_"), version))
 }
 
-// Only OCI registries support storing charts under sub-directories.
-func IsHelmOciChart(chart string) bool {
-	return strings.Contains(chart, "/")
+// Ensures that given OCI registries URL does not have protocol
+func IsHelmOciRepo(repoURL string) bool {
+	if repoURL == "" {
+		return false
+	}
+	parsed, err := url.Parse(repoURL)
+	// the URL parser treat hostname as either path or opaque if scheme is not specified, so hostname must be empty
+	return err == nil && parsed.Host == ""
 }

util/helm/client_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/Masterminds/semver"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/argoproj/argo-cd/util/io"
@@ -50,7 +49,7 @@ func TestIndex(t *testing.T) {
 
 func Test_nativeHelmChart_ExtractChart(t *testing.T) {
 	client := NewClient("https://argoproj.github.io/argo-helm", Creds{}, false)
-	path, closer, err := client.ExtractChart("argo-cd", semver.MustParse("0.7.1"))
+	path, closer, err := client.ExtractChart("argo-cd", "0.7.1")
 	assert.NoError(t, err)
 	defer io.Close(closer)
 	info, err := os.Stat(path)
@@ -88,3 +87,10 @@ func Test_normalizeChartName(t *testing.T) {
 		assert.Equal(t, n, "myorg/..")
 	})
 }
+
+func TestIsHelmOciRepo(t *testing.T) {
+	assert.True(t, IsHelmOciRepo("demo.goharbor.io"))
+	assert.True(t, IsHelmOciRepo("demo.goharbor.io:8080"))
+	assert.False(t, IsHelmOciRepo("https://demo.goharbor.io"))
+	assert.False(t, IsHelmOciRepo("https://demo.goharbor.io:8080"))
+}