Repository: argoproj/argo-cd
Pull request #5888 (2021-03-26 21:20:10)

fix: fix incorrect OCI Helm registiries assumptions
Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Fixes https://github.com/argoproj/argo-cd/issues/5883
Fixes https://github.com/argoproj/argo-cd/issues/5558
Fixes https://github.com/argoproj/argo-cd/issues/5257


PR fixes incorrect handling of OCI Helm registries:

* #5883 Argo CD incorrectly assumes that chart is in OCI registry if it is under subdirectory. That is not correct since some OCI registries support subpaths as well. Instead of use chart path assumption should be made based on repository URL: if repository URL contains only hostname and port then it is OCI repo
* #5558 Users are struggling to register OCI registry because of scheme in the URL. PR adds explicit error message that explains that URL must include domain and port only `OCI Helm repository URL should include hostname and port only`
* #5257 Argo CD unnecessary converts non OCI and OCI charts version to semver. PR removes it for non OCI registries

codecov[bot]: # [Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=h1) Report
> Merging [#5888](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=desc) (61650d2) into [master](https://codecov.io/gh/argoproj/argo-cd/commit/1c318f39598bfe2939ca11d70a9b66fd28ec82f7?el=desc) (1c318f3) will **increase** coverage by `0.02%`.
> The diff coverage is `62.50%`.

[![Impacted file tree graph](https://codecov.io/gh/argoproj/argo-cd/pull/5888/graphs/tree.svg?width=650&height=150&src=pr&token=kf5kwuegwE)](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #5888      +/-   ##
==========================================
+ Coverage   40.88%   40.91%   +0.02%     
==========================================
  Files         147      147              
  Lines       19619    19620       +1     
==========================================
+ Hits         8021     8027       +6     
+ Misses      10493    10489       -4     
+ Partials     1105     1104       -1     
```


| [Impacted Files](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/application/v1alpha1/types.go](https://codecov.io/gh/argoproj/argo-cd/pull/5888/diff?src=pr&el=tree#diff-cGtnL2FwaXMvYXBwbGljYXRpb24vdjFhbHBoYTEvdHlwZXMuZ28=) | `57.44% <0.00%> (ø)` | |
| [util/helm/client.go](https://codecov.io/gh/argoproj/argo-cd/pull/5888/diff?src=pr&el=tree#diff-dXRpbC9oZWxtL2NsaWVudC5nbw==) | `45.02% <50.00%> (+0.38%)` | :arrow_up: |
| [reposerver/repository/repository.go](https://codecov.io/gh/argoproj/argo-cd/pull/5888/diff?src=pr&el=tree#diff-cmVwb3NlcnZlci9yZXBvc2l0b3J5L3JlcG9zaXRvcnkuZ28=) | `62.80% <80.00%> (+0.34%)` | :arrow_up: |
| [util/argo/argo.go](https://codecov.io/gh/argoproj/argo-cd/pull/5888/diff?src=pr&el=tree#diff-dXRpbC9hcmdvL2FyZ28uZ28=) | `60.75% <100.00%> (+0.88%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=footer). Last update [1c318f3...61650d2](https://codecov.io/gh/argoproj/argo-cd/pull/5888?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
index fa1359a71..1e7c5f2c3 100644
--- a/pkg/apis/application/v1alpha1/types.go
+++ b/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
diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go
index 13029b250..a332f15ea 100644
--- a/reposerver/repository/repository.go
+++ b/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)
diff --git a/util/argo/argo.go b/util/argo/argo.go
index b348b971f..d1790f5c0 100644
--- a/util/argo/argo.go
+++ b/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 {
diff --git a/util/helm/client.go b/util/helm/client.go
index c12163c0a..453750c39 100644
--- a/util/helm/client.go
+++ b/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 == ""
 }
diff --git a/util/helm/mocks/Client.go b/util/helm/mocks/Client.go
index db7a6a5b5..3eefcc771 100644
--- a/util/helm/mocks/Client.go
+++ b/util/helm/mocks/Client.go
@@ -7,8 +7,6 @@ import (
 	io "github.com/argoproj/argo-cd/util/io"
 
 	mock "github.com/stretchr/testify/mock"
-
-	semver "github.com/Masterminds/semver"
 )
 
 // Client is an autogenerated mock type for the Client type
@@ -17,11 +15,11 @@ type Client struct {
 }
 
 // CleanChartCache provides a mock function with given fields: chart, version
-func (_m *Client) CleanChartCache(chart string, version *semver.Version) error {
+func (_m *Client) CleanChartCache(chart string, version string) error {
 	ret := _m.Called(chart, version)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(string, *semver.Version) error); ok {
+	if rf, ok := ret.Get(0).(func(string, string) error); ok {
 		r0 = rf(chart, version)
 	} else {
 		r0 = ret.Error(0)
@@ -31,18 +29,18 @@ func (_m *Client) CleanChartCache(chart string, version *semver.Version) error {
 }
 
 // ExtractChart provides a mock function with given fields: chart, version
-func (_m *Client) ExtractChart(chart string, version *semver.Version) (string, io.Closer, error) {
+func (_m *Client) ExtractChart(chart string, version string) (string, io.Closer, error) {
 	ret := _m.Called(chart, version)
 
 	var r0 string
-	if rf, ok := ret.Get(0).(func(string, *semver.Version) string); ok {
+	if rf, ok := ret.Get(0).(func(string, string) string); ok {
 		r0 = rf(chart, version)
 	} else {
 		r0 = ret.Get(0).(string)
 	}
 
 	var r1 io.Closer
-	if rf, ok := ret.Get(1).(func(string, *semver.Version) io.Closer); ok {
+	if rf, ok := ret.Get(1).(func(string, string) io.Closer); ok {
 		r1 = rf(chart, version)
 	} else {
 		if ret.Get(1) != nil {
@@ -51,7 +49,7 @@ func (_m *Client) ExtractChart(chart string, version *semver.Version) (string, i
 	}
 
 	var r2 error
-	if rf, ok := ret.Get(2).(func(string, *semver.Version) error); ok {
+	if rf, ok := ret.Get(2).(func(string, string) error); ok {
 		r2 = rf(chart, version)
 	} else {
 		r2 = ret.Error(2)


Test patch:
diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go
index dcda8c189..9d163a60c 100644
--- a/reposerver/repository/repository_test.go
+++ b/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)
diff --git a/util/argo/argo_test.go b/util/argo/argo_test.go
index c7d6b13cf..066d9c35f 100644
--- a/util/argo/argo_test.go
+++ b/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())
+}
diff --git a/util/helm/client_test.go b/util/helm/client_test.go
index 9d869667a..7ac853422 100644
--- a/util/helm/client_test.go
+++ b/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"))
+}