chore(repo-server): unify semver resolution in new versions subpackage (#20216) (#23310)

Signed-off-by: Paul Larsen <[email protected]>
Signed-off-by: Blake Pettersson <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Co-authored-by: Paul Larsen <[email protected]>
Co-authored-by: Blake Pettersson <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>

Author: 4 people

reposerver/repository/repository.go

@@ -16,7 +16,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/Masterminds/semver/v3"
 	"github.com/TomOnTime/utfutil"
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
 	textutils "github.com/argoproj/gitops-engine/pkg/utils/text"
@@ -60,6 +59,7 @@ import (
 	"github.com/argoproj/argo-cd/v3/util/kustomize"
 	"github.com/argoproj/argo-cd/v3/util/manifeststream"
 	"github.com/argoproj/argo-cd/v3/util/text"
+	"github.com/argoproj/argo-cd/v3/util/versions"
 )
 
 const (
@@ -2403,40 +2403,37 @@ func (s *Service) newClientResolveRevision(repo *v1alpha1.Repository, revision s
 func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revision string, chart string, noRevisionCache bool) (helm.Client, string, error) {
 	enableOCI := repo.EnableOCI || helm.IsHelmOciRepo(repo.Repo)
 	helmClient := s.newHelmClient(repo.Repo, repo.GetHelmCreds(), enableOCI, repo.Proxy, repo.NoProxy, helm.WithIndexCache(s.cache), helm.WithChartPaths(s.chartPaths))
-	if helm.IsVersion(revision) {
+
+	// Note: This check runs the risk of returning a version which is not found in the helm registry.
+	if versions.IsVersion(revision) {
 		return helmClient, revision, nil
 	}
-	constraints, err := semver.NewConstraint(revision)
-	if err != nil {
-		return nil, "", fmt.Errorf("invalid revision '%s': %w", revision, err)
-	}
 
+	var tags []string
 	if enableOCI {
-		tags, err := helmClient.GetTags(chart, noRevisionCache)
+		var err error
+		tags, err = helmClient.GetTags(chart, noRevisionCache)
 		if err != nil {
 			return nil, "", fmt.Errorf("unable to get tags: %w", err)
 		}
-
-		version, err := tags.MaxVersion(constraints)
+	} else {
+		index, err := helmClient.GetIndex(noRevisionCache, s.initConstants.HelmRegistryMaxIndexSize)
 		if err != nil {
-			return nil, "", fmt.Errorf("no version for constraints: %w", err)
+			return nil, "", err
 		}
-		return helmClient, version.String(), nil
+		entries, err := index.GetEntries(chart)
+		if err != nil {
+			return nil, "", err
+		}
+		tags = entries.Tags()
 	}
 
-	index, err := helmClient.GetIndex(noRevisionCache, s.initConstants.HelmRegistryMaxIndexSize)
+	maxV, err := versions.MaxVersion(revision, tags)
 	if err != nil {
-		return nil, "", err
+		return nil, "", fmt.Errorf("invalid revision: %w", err)
 	}
-	entries, err := index.GetEntries(chart)
-	if err != nil {
-		return nil, "", err
-	}
-	version, err := entries.MaxVersion(constraints)
-	if err != nil {
-		return nil, "", err
-	}
-	return helmClient, version.String(), nil
+
+	return helmClient, maxV, nil
 }
 
 // directoryPermissionInitializer ensures the directory has read/write/execute permissions and returns
@@ -2564,13 +2561,10 @@ func (s *Service) GetHelmCharts(_ context.Context, q *apiclient.HelmChartsReques
 	}
 	res := apiclient.HelmChartsResponse{}
 	for chartName, entries := range index.Entries {
-		chart := apiclient.HelmChart{
-			Name: chartName,
-		}
-		for _, entry := range entries {
-			chart.Versions = append(chart.Versions, entry.Version)
-		}
-		res.Items = append(res.Items, &chart)
+		res.Items = append(res.Items, &apiclient.HelmChart{
+			Name:     chartName,
+			Versions: entries.Tags(),
+		})
 	}
 	return &res, nil
 }

reposerver/repository/repository_test.go

@@ -1828,11 +1828,11 @@ func TestService_newHelmClientResolveRevision(t *testing.T) {
 
 	t.Run("EmptyRevision", func(t *testing.T) {
 		_, _, err := service.newHelmClientResolveRevision(&v1alpha1.Repository{}, "", "my-chart", true)
-		assert.EqualError(t, err, "invalid revision '': improper constraint: ")
+		assert.EqualError(t, err, "invalid revision: failed to determine semver constraint: improper constraint: ")
 	})
 	t.Run("InvalidRevision", func(t *testing.T) {
 		_, _, err := service.newHelmClientResolveRevision(&v1alpha1.Repository{}, "???", "my-chart", true)
-		assert.EqualError(t, err, "invalid revision '???': improper constraint: ???")
+		assert.EqualError(t, err, "invalid revision: failed to determine semver constraint: improper constraint: ???")
 	})
 }
 

util/git/client.go

@@ -17,8 +17,6 @@ import (
 	"syscall"
 	"time"
 
-	"github.com/Masterminds/semver/v3"
-
 	argoexec "github.com/argoproj/pkg/exec"
 	"github.com/bmatcuk/doublestar/v4"
 	"github.com/go-git/go-git/v5"
@@ -39,6 +37,7 @@ import (
 	"github.com/argoproj/argo-cd/v3/util/env"
 	executil "github.com/argoproj/argo-cd/v3/util/exec"
 	"github.com/argoproj/argo-cd/v3/util/proxy"
+	"github.com/argoproj/argo-cd/v3/util/versions"
 )
 
 var ErrInvalidRepoURL = errors.New("repo URL is invalid")
@@ -645,6 +644,16 @@ func (m *nativeGitClient) LsRemote(revision string) (res string, err error) {
 	return
 }
 
+func getGitTags(refs []*plumbing.Reference) []string {
+	var tags []string
+	for _, ref := range refs {
+		if ref.Name().IsTag() {
+			tags = append(tags, ref.Name().Short())
+		}
+	}
+	return tags
+}
+
 func (m *nativeGitClient) lsRemote(revision string) (string, error) {
 	if IsCommitSHA(revision) {
 		return revision, nil
@@ -659,9 +668,9 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) {
 		revision = "HEAD"
 	}
 
-	semverSha := m.resolveSemverRevision(revision, refs)
-	if semverSha != "" {
-		return semverSha, nil
+	maxV, err := versions.MaxVersion(revision, getGitTags(refs))
+	if err == nil {
+		revision = maxV
 	}
 
 	// refToHash keeps a maps of remote refs to their hash
@@ -710,59 +719,6 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) {
 	return "", fmt.Errorf("unable to resolve '%s' to a commit SHA", revision)
 }
 
-// resolveSemverRevision is a part of the lsRemote method workflow.
-// When the user correctly configures the Git repository revision, and that revision is a valid semver constraint, we
-// use this logic path rather than the standard lsRemote revision resolution loop.
-// Some examples to illustrate the actual behavior - if the revision is:
-// * "v0.1.2"/"0.1.2" or "v0.1"/"0.1", then this is not a constraint, it's a pinned version - so we fall back to the standard tag matching in the lsRemote loop.
-// * "v0.1.*"/"0.1.*", and there's a tag matching that constraint, then we find the latest matching version and return its commit hash.
-// * "v0.1.*"/"0.1.*", and there is *no* tag matching that constraint, then we fall back to the standard tag matching in the lsRemote loop.
-// * "custom-tag", only the lsRemote loop will run - because that revision is an invalid semver;
-// * "master-branch", only the lsRemote loop will run because that revision is an invalid semver;
-func (m *nativeGitClient) resolveSemverRevision(revision string, refs []*plumbing.Reference) string {
-	if _, err := semver.NewVersion(revision); err == nil {
-		// If the revision is a valid version, then we know it isn't a constraint; it's just a pin.
-		// In which case, we should use standard tag resolution mechanisms.
-		return ""
-	}
-
-	constraint, err := semver.NewConstraint(revision)
-	if err != nil {
-		log.Debugf("Revision '%s' is not a valid semver constraint, skipping semver resolution.", revision)
-		return ""
-	}
-
-	maxVersion := semver.New(0, 0, 0, "", "")
-	maxVersionHash := plumbing.ZeroHash
-	for _, ref := range refs {
-		if !ref.Name().IsTag() {
-			continue
-		}
-
-		tag := ref.Name().Short()
-		version, err := semver.NewVersion(tag)
-		if err != nil {
-			log.Debugf("Error parsing version for tag: '%s': %v", tag, err)
-			// Skip this tag and continue to the next one
-			continue
-		}
-
-		if constraint.Check(version) {
-			if version.GreaterThan(maxVersion) {
-				maxVersion = version
-				maxVersionHash = ref.Hash()
-			}
-		}
-	}
-
-	if maxVersionHash.IsZero() {
-		return ""
-	}
-
-	log.Debugf("Semver constraint '%s' resolved to tag '%s', at reference '%s'", revision, maxVersion.Original(), maxVersionHash.String())
-	return maxVersionHash.String()
-}
-
 // CommitSHA returns current commit sha from `git rev-parse HEAD`
 func (m *nativeGitClient) CommitSHA() (string, error) {
 	out, err := m.runCmd("rev-parse", "HEAD")

util/git/client_test.go

@@ -315,7 +315,7 @@ func Test_SemverTags(t *testing.T) {
 		// However, if one specifies the minor/patch versions, semver constraints can be used to match non-semver tags.
 		// 2024-banana is considered as "2024.0.0-banana" in semver-ish, and banana > apple, so it's a match.
 		// Note: this is more for documentation and future reference than real testing, as it seems like quite odd behaviour.
-		name:     "semver constraints on non-semver tags",
+		name:     "semver constraints on semver tags",
 		ref:      "> 2024.0.0-apple",
 		expected: mapTagRefs["2024-banana"],
 	}} {

util/helm/client.go

@@ -49,7 +49,7 @@ type Client interface {
 	CleanChartCache(chart string, version string) error
 	ExtractChart(chart string, version string, passCredentials bool, manifestMaxExtractedSize int64, disableManifestMaxExtractedSize bool) (string, argoio.Closer, error)
 	GetIndex(noCache bool, maxIndexSize int64) (*Index, error)
-	GetTags(chart string, noCache bool) (*TagsList, error)
+	GetTags(chart string, noCache bool) ([]string, error)
 	TestHelmOCI() (bool, error)
 }
 
@@ -416,7 +416,7 @@ func getIndexURL(rawURL string) (string, error) {
 	return repoURL.String(), nil
 }
 
-func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error) {
+func (c *nativeHelmChart) GetTags(chart string, noCache bool) ([]string, error) {
 	if !c.enableOci {
 		return nil, OCINotEnabledErr
 	}
@@ -432,7 +432,11 @@ func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error)
 		}
 	}
 
-	tags := &TagsList{}
+	type entriesStruct struct {
+		Tags []string
+	}
+
+	entries := &entriesStruct{}
 	if len(data) == 0 {
 		start := time.Now()
 		repo, err := remote.NewRepository(tagsURL)
@@ -479,7 +483,7 @@ func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error)
 			for _, tag := range tagsResult {
 				// By convention: Change underscore (_) back to plus (+) to get valid SemVer
 				convertedTag := strings.ReplaceAll(tag, "_", "+")
-				tags.Tags = append(tags.Tags, convertedTag)
+				entries.Tags = append(entries.Tags, convertedTag)
 			}
 
 			return nil
@@ -497,11 +501,11 @@ func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error)
 			}
 		}
 	} else {
-		err := json.Unmarshal(data, tags)
+		err := json.Unmarshal(data, entries)
 		if err != nil {
 			return nil, fmt.Errorf("failed to decode tags: %w", err)
 		}
 	}
 
-	return tags, nil
+	return entries.Tags, nil
 }

util/helm/client_test.go

@@ -26,6 +26,10 @@ type fakeIndexCache struct {
 	data []byte
 }
 
+type fakeTagsList struct {
+	Tags []string `json:"tags"`
+}
+
 func (f *fakeIndexCache) SetHelmIndex(_ string, indexData []byte) error {
 	f.data = indexData
 	return nil
@@ -171,19 +175,23 @@ func TestGetTagsFromUrl(t *testing.T) {
 	t.Run("should return tags correctly while following the link header", func(t *testing.T) {
 		server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			t.Logf("called %s", r.URL.Path)
-			responseTags := TagsList{}
+			var responseTags fakeTagsList
 			w.Header().Set("Content-Type", "application/json")
 			if !strings.Contains(r.URL.String(), "token") {
 				w.Header().Set("Link", fmt.Sprintf("<https://%s%s?token=next-token>; rel=next", r.Host, r.URL.Path))
-				responseTags.Tags = []string{"first"}
+				responseTags = fakeTagsList{
+					Tags: []string{"first"},
+				}
 			} else {
-				responseTags.Tags = []string{
-					"second",
-					"2.8.0",
-					"2.8.0-prerelease",
-					"2.8.0_build",
-					"2.8.0-prerelease_build",
-					"2.8.0-prerelease.1_build.1234",
+				responseTags = fakeTagsList{
+					Tags: []string{
+						"second",
+						"2.8.0",
+						"2.8.0-prerelease",
+						"2.8.0_build",
+						"2.8.0-prerelease_build",
+						"2.8.0-prerelease.1_build.1234",
+					},
 				}
 			}
 			w.WriteHeader(http.StatusOK)
@@ -194,7 +202,7 @@ func TestGetTagsFromUrl(t *testing.T) {
 
 		tags, err := client.GetTags("mychart", true)
 		require.NoError(t, err)
-		assert.ElementsMatch(t, tags.Tags, []string{
+		assert.ElementsMatch(t, tags, []string{
 			"first",
 			"second",
 			"2.8.0",
@@ -230,7 +238,7 @@ func TestGetTagsFromURLPrivateRepoAuthentication(t *testing.T) {
 
 		assert.Equal(t, expectedAuthorization, authorization)
 
-		responseTags := TagsList{
+		responseTags := fakeTagsList{
 			Tags: []string{
 				"2.8.0",
 				"2.8.0-prerelease",
@@ -282,7 +290,7 @@ func TestGetTagsFromURLPrivateRepoAuthentication(t *testing.T) {
 			tags, err := client.GetTags("mychart", true)
 
 			require.NoError(t, err)
-			assert.ElementsMatch(t, tags.Tags, []string{
+			assert.ElementsMatch(t, tags, []string{
 				"2.8.0",
 				"2.8.0-prerelease",
 				"2.8.0+build",
@@ -327,7 +335,7 @@ func TestGetTagsFromURLPrivateRepoWithAzureWorkloadIdentityAuthentication(t *tes
 
 			assert.Equal(t, expectedAuthorization, authorization)
 
-			responseTags := TagsList{
+			responseTags := fakeTagsList{
 				Tags: []string{
 					"2.8.0",
 					"2.8.0-prerelease",
@@ -380,7 +388,7 @@ func TestGetTagsFromURLPrivateRepoWithAzureWorkloadIdentityAuthentication(t *tes
 			tags, err := client.GetTags("mychart", true)
 
 			require.NoError(t, err)
-			assert.ElementsMatch(t, tags.Tags, []string{
+			assert.ElementsMatch(t, tags, []string{
 				"2.8.0",
 				"2.8.0-prerelease",
 				"2.8.0+build",
@@ -406,7 +414,7 @@ func TestGetTagsFromURLEnvironmentAuthentication(t *testing.T) {
 
 		assert.Equal(t, expectedAuthorization, authorization)
 
-		responseTags := TagsList{
+		responseTags := fakeTagsList{
 			Tags: []string{
 				"2.8.0",
 				"2.8.0-prerelease",
@@ -463,7 +471,7 @@ func TestGetTagsFromURLEnvironmentAuthentication(t *testing.T) {
 			tags, err := client.GetTags("mychart", true)
 
 			require.NoError(t, err)
-			assert.ElementsMatch(t, tags.Tags, []string{
+			assert.ElementsMatch(t, tags, []string{
 				"2.8.0",
 				"2.8.0-prerelease",
 				"2.8.0+build",