Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d8fd8b
Unify semver resolution in new versions subpackage
PaulSonOfLars Oct 3, 2024
1ff1030
rename back to entries instead of tagslist
PaulSonOfLars Oct 3, 2024
16ff1c7
add IsConstraint method
PaulSonOfLars Oct 4, 2024
bbe0191
replace entries struct with string arrays
PaulSonOfLars Oct 4, 2024
02626c9
regen mocks
PaulSonOfLars Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 17 additions & 22 deletions reposerver/repository/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -61,6 +60,7 @@ import (
"github.com/argoproj/argo-cd/v2/util/kustomize"
"github.com/argoproj/argo-cd/v2/util/manifeststream"
"github.com/argoproj/argo-cd/v2/util/text"
"github.com/argoproj/argo-cd/v2/util/versions"
)

const (
Expand Down Expand Up @@ -2419,39 +2419,34 @@ 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) {
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
}
tags, err = index.GetTags(chart)
if err != nil {
return nil, "", err
}
return helmClient, version.String(), nil
}

index, err := helmClient.GetIndex(noRevisionCache, s.initConstants.HelmRegistryMaxIndexSize)
version, err := versions.MaxVersion(revision, tags)
if err != nil {
return nil, "", err
}
entries, err := index.GetEntries(chart)
if err != nil {
return nil, "", err
}
version, err := entries.MaxVersion(constraints)
if err != nil {
return nil, "", err
return nil, "", fmt.Errorf("no version for constraints: %w", err)
}

return helmClient, version.String(), nil
}

Expand Down Expand Up @@ -2540,8 +2535,8 @@ func (s *Service) GetHelmCharts(ctx context.Context, q *apiclient.HelmChartsRequ
chart := apiclient.HelmChart{
Name: chartName,
}
for _, entry := range entries {
chart.Versions = append(chart.Versions, entry.Version)
for _, version := range entries.Tags {
chart.Versions = append(chart.Versions, version)
}
res.Items = append(res.Items, &chart)
}
Expand Down
4 changes: 2 additions & 2 deletions reposerver/repository/repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"github.com/argoproj/argo-cd/v2/util/git"
gitmocks "github.com/argoproj/argo-cd/v2/util/git/mocks"
"github.com/argoproj/argo-cd/v2/util/helm"
helmmocks "github.com/argoproj/argo-cd/v2/util/helm/mocks"

Check failure on line 49 in reposerver/repository/repository_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

could not import github.com/argoproj/argo-cd/v2/util/helm/mocks (-: # github.com/argoproj/argo-cd/v2/util/helm/mocks
"github.com/argoproj/argo-cd/v2/util/io"
iomocks "github.com/argoproj/argo-cd/v2/util/io/mocks"
)
Expand Down Expand Up @@ -123,8 +123,8 @@
oobChart := "out-of-bounds-chart"
version := "1.1.0"
helmClient.On("GetIndex", mock.AnythingOfType("bool"), mock.Anything).Return(&helm.Index{Entries: map[string]helm.Entries{
chart: {{Version: "1.0.0"}, {Version: version}},
oobChart: {{Version: "1.0.0"}, {Version: version}},
chart: {Tags: []string{"1.0.0", version}},
oobChart: {Tags: []string{"1.0.0", version}},
}}, nil)
helmClient.On("ExtractChart", chart, version, "", false, int64(0), false).Return("./testdata/my-chart", io.NopCloser, nil)
helmClient.On("ExtractChart", oobChart, version, "", false, int64(0), false).Return("./testdata2/out-of-bounds-chart", io.NopCloser, nil)
Expand Down
72 changes: 14 additions & 58 deletions util/git/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -39,6 +37,7 @@ import (
"github.com/argoproj/argo-cd/v2/util/env"
executil "github.com/argoproj/argo-cd/v2/util/exec"
"github.com/argoproj/argo-cd/v2/util/proxy"
"github.com/argoproj/argo-cd/v2/util/versions"
)

var ErrInvalidRepoURL = fmt.Errorf("repo URL is invalid")
Expand Down Expand Up @@ -617,6 +616,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
Expand All @@ -631,9 +640,9 @@ func (m *nativeGitClient) lsRemote(revision string) (string, error) {
revision = "HEAD"
}

semverSha := m.resolveSemverRevision(revision, refs)
if semverSha != "" {
return semverSha, nil
version, err := versions.MaxVersion(revision, getGitTags(refs))
if err == nil {
revision = version.Original()
}

// refToHash keeps a maps of remote refs to their hash
Expand Down Expand Up @@ -682,59 +691,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")
Expand Down
2 changes: 1 addition & 1 deletion util/git/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,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"],
}} {
Expand Down
19 changes: 11 additions & 8 deletions util/helm/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
"strings"
"time"

executil "github.com/argoproj/argo-cd/v2/util/exec"

"github.com/argoproj/pkg/sync"
log "github.com/sirupsen/logrus"
"gopkg.in/yaml.v2"
Expand All @@ -28,6 +26,7 @@ import (
"oras.land/oras-go/v2/registry/remote/credentials"

"github.com/argoproj/argo-cd/v2/util/cache"
executil "github.com/argoproj/argo-cd/v2/util/exec"
argoio "github.com/argoproj/argo-cd/v2/util/io"
"github.com/argoproj/argo-cd/v2/util/io/files"
"github.com/argoproj/argo-cd/v2/util/proxy"
Expand Down Expand Up @@ -58,7 +57,7 @@ type Client interface {
CleanChartCache(chart string, version string, project string) error
ExtractChart(chart string, version string, project 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)
}

Expand Down Expand Up @@ -414,7 +413,11 @@ func getIndexURL(rawURL string) (string, error) {
return repoURL.String(), nil
}

func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error) {
type Entries struct {
blakepettersson marked this conversation as resolved.
Show resolved Hide resolved
Tags []string
}

func (c *nativeHelmChart) GetTags(chart string, noCache bool) ([]string, error) {
if !c.enableOci {
return nil, OCINotEnabledErr
}
Expand All @@ -430,7 +433,7 @@ func (c *nativeHelmChart) GetTags(chart string, noCache bool) (*TagsList, error)
}
}

tags := &TagsList{}
entries := &Entries{}
if len(data) == 0 {
start := time.Now()
repo, err := remote.NewRepository(tagsURL)
Expand Down Expand Up @@ -472,7 +475,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
Expand All @@ -490,11 +493,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
}
14 changes: 7 additions & 7 deletions util/helm/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func TestIndex(t *testing.T) {
})

t.Run("Cached", func(t *testing.T) {
fakeIndex := Index{Entries: map[string]Entries{"fake": {}}}
fakeIndex := Index{Entries: map[string]Entries{"fake": {Tags: []string{}}}}
data := bytes.Buffer{}
err := yaml.NewEncoder(&data).Encode(fakeIndex)
require.NoError(t, err)
Expand Down Expand Up @@ -169,7 +169,7 @@ 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{}
responseTags := Entries{}
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))
Expand All @@ -195,7 +195,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",
Expand Down Expand Up @@ -231,7 +231,7 @@ func TestGetTagsFromURLPrivateRepoAuthentication(t *testing.T) {

assert.Equal(t, expectedAuthorization, authorization)

responseTags := TagsList{
responseTags := Entries{
Tags: []string{
"2.8.0",
"2.8.0-prerelease",
Expand Down Expand Up @@ -286,7 +286,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",
Expand All @@ -312,7 +312,7 @@ func TestGetTagsFromURLEnvironmentAuthentication(t *testing.T) {

assert.Equal(t, expectedAuthorization, authorization)

responseTags := TagsList{
responseTags := Entries{
Tags: []string{
"2.8.0",
"2.8.0-prerelease",
Expand Down Expand Up @@ -372,7 +372,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",
Expand Down
Loading
Loading