Skip to content

Improve repository selection, based on prefixes in config and full image name #547

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+85 −37
Open

Improve repository selection, based on prefixes in config and full image name #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f38ed57
feat: Improve repository selection, based on prefixes in config and f…
vsychov Mar 28, 2023
149db3d
fix: linter issues
vsychov Mar 29, 2023
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
2 changes: 1 addition & 1 deletion .github/workflows/ci-tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ jobs:
- name: Run golangci-lint
uses: golangci/golangci-lint-action@v2
with:
version: v1.46.2
version: v1.52.2
args: --timeout 5m
test:
name: Ensure unit tests are passing
Expand Down
3 changes: 2 additions & 1 deletion cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/argoproj-labs/argocd-image-updater/pkg/common"
"github.com/argoproj-labs/argocd-image-updater/pkg/env"
"github.com/argoproj-labs/argocd-image-updater/pkg/health"
"github.com/argoproj-labs/argocd-image-updater/pkg/image"
"github.com/argoproj-labs/argocd-image-updater/pkg/log"
"github.com/argoproj-labs/argocd-image-updater/pkg/metrics"
"github.com/argoproj-labs/argocd-image-updater/pkg/registry"
Expand Down Expand Up @@ -343,7 +344,7 @@ func warmupImageCache(cfg *ImageUpdaterConfig) error {
entries := 0
eps := registry.ConfiguredEndpoints()
for _, ep := range eps {
r, err := registry.GetRegistryEndpoint(ep)
r, err := registry.GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ep})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg/registry has been moved to the sub-module registry-scanner.

if err == nil {
entries += r.Cache.NumEntries()
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ argocd-image-updater test nginx --allow-tags '^1.19.\d+(\-.*)*$' --update-strate
}
}

ep, err := registry.GetRegistryEndpoint(img.RegistryURL)
ep, err := registry.GetRegistryEndpoint(img)
if err != nil {
logCtx.Fatalf("could not get registry endpoint: %v", err)
}
Expand Down
12 changes: 6 additions & 6 deletions ext/git/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,12 @@ func NewClientExt(rawRepoURL string, root string, creds Creds, insecure bool, en

// Returns a HTTP client object suitable for go-git to use using the following
// pattern:
// - If insecure is true, always returns a client with certificate verification
// turned off.
// - If one or more custom certificates are stored for the repository, returns
// a client with those certificates in the list of root CAs used to verify
// the server's certificate.
// - Otherwise (and on non-fatal errors), a default HTTP client is returned.
// - If insecure is true, always returns a client with certificate verification
// turned off.
// - If one or more custom certificates are stored for the repository, returns
// a client with those certificates in the list of root CAs used to verify
// the server's certificate.
// - Otherwise (and on non-fatal errors), a default HTTP client is returned.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like only whitespace changes. I think we should avoid these changes.

func GetRepoHTTPClient(repoURL string, insecure bool, creds Creds, proxyURL string) *http.Client {
// Default HTTP client
var customHTTPClient = &http.Client{
Expand Down
2 changes: 1 addition & 1 deletion pkg/argocd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func UpdateApplication(updateConf *UpdateConfiguration, state *SyncIterationStat

imgCtx.Debugf("Considering this image for update")

rep, err := registry.GetRegistryEndpoint(applicationImage.RegistryURL)
rep, err := registry.GetRegistryEndpoint(applicationImage)
if err != nil {
imgCtx.Errorf("Could not get registry endpoint from configuration: %v", err)
result.NumErrors += 1
Expand Down
9 changes: 5 additions & 4 deletions pkg/registry/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package registry
import (
"testing"

"github.com/argoproj-labs/argocd-image-updater/pkg/image"
"github.com/argoproj-labs/argocd-image-updater/pkg/options"

"github.com/distribution/distribution/v3/manifest/schema1"
Expand All @@ -16,7 +17,7 @@ func Test_TagMetadata(t *testing.T) {
History: []schema1.History{},
},
}
ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
client, err := NewClient(ep, "", "")
require.NoError(t, err)
Expand All @@ -35,7 +36,7 @@ func Test_TagMetadata(t *testing.T) {
},
}

ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
client, err := NewClient(ep, "", "")
require.NoError(t, err)
Expand All @@ -54,7 +55,7 @@ func Test_TagMetadata(t *testing.T) {
},
}

ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
client, err := NewClient(ep, "", "")
require.NoError(t, err)
Expand All @@ -74,7 +75,7 @@ func Test_TagMetadata(t *testing.T) {
},
},
}
ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
client, err := NewClient(ep, "", "")
require.NoError(t, err)
Expand Down
7 changes: 4 additions & 3 deletions pkg/registry/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"
"time"

"github.com/argoproj-labs/argocd-image-updater/pkg/image"
"github.com/argoproj-labs/argocd-image-updater/test/fixture"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -81,15 +82,15 @@ func Test_LoadRegistryConfiguration(t *testing.T) {
err := LoadRegistryConfiguration("../../config/example-config.yaml", true)
require.NoError(t, err)
assert.Len(t, registries, 4)
reg, err := GetRegistryEndpoint("gcr.io")
reg, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "gcr.io"})
require.NoError(t, err)
assert.Equal(t, "pullsecret:foo/bar", reg.Credentials)
reg, err = GetRegistryEndpoint("ghcr.io")
reg, err = GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "ghcr.io"})
require.NoError(t, err)
assert.Equal(t, "ext:/some/script", reg.Credentials)
assert.Equal(t, 5*time.Hour, reg.CredsExpire)
RestoreDefaultRegistryConfiguration()
reg, err = GetRegistryEndpoint("gcr.io")
reg, err = GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "gcr.io"})
require.NoError(t, err)
assert.Equal(t, "", reg.Credentials)
})
Expand Down
33 changes: 29 additions & 4 deletions pkg/registry/endpoints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/argoproj-labs/argocd-image-updater/pkg/cache"
"github.com/argoproj-labs/argocd-image-updater/pkg/image"
"github.com/argoproj-labs/argocd-image-updater/pkg/log"

"go.uber.org/ratelimit"
Expand Down Expand Up @@ -179,8 +180,27 @@ func inferRegistryEndpointFromPrefix(prefix string) *RegistryEndpoint {
return NewRegistryEndpoint(prefix, prefix, apiURL, "", "", false, TagListSortUnsorted, 20, 0)
}

// GetRegistryEndpoint retrieves the endpoint information for the given prefix
func GetRegistryEndpoint(prefix string) (*RegistryEndpoint, error) {
// find registry by prefix based on full image name
func findRegistryEndpointByImage(img *image.ContainerImage) (ep *RegistryEndpoint) {
log.Debugf("Try to find endpoint by image: %s/%s", img.RegistryURL, img.ImageName)
registryLock.RLock()
imgName := fmt.Sprintf("%s/%s", img.RegistryURL, img.ImageName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about moving line 187 to the first line of this func, and use var imgName in log.Debugf(...)?


for _, registry := range registries {
if (ep == nil && strings.HasPrefix(imgName, registry.RegistryPrefix)) || (strings.HasPrefix(imgName, registry.RegistryPrefix) && len(registry.RegistryPrefix) > len(ep.RegistryPrefix)) {
log.Debugf("Selected registry: '%s' (last selection in log - final)", registry.RegistryName)
ep = registry
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it break after the first match?

}
}

registryLock.RUnlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving defer RUnlock() to right after the RLock() function.

return
}

// GetRegistryEndpoint retrieves the endpoint information for the given image
func GetRegistryEndpoint(img *image.ContainerImage) (*RegistryEndpoint, error) {
prefix := img.RegistryURL

if prefix == "" {
if defaultRegistry == nil {
return nil, fmt.Errorf("no default endpoint configured")
Expand All @@ -197,7 +217,12 @@ func GetRegistryEndpoint(prefix string) (*RegistryEndpoint, error) {
return registry, nil
} else {
var err error
ep := inferRegistryEndpointFromPrefix(prefix)
ep := findRegistryEndpointByImage(img)
if ep != nil {
return ep, err
}

ep = inferRegistryEndpointFromPrefix(prefix)
if ep != nil {
err = AddRegistryEndpoint(ep)
} else {
Expand Down Expand Up @@ -235,7 +260,7 @@ func GetDefaultRegistry() *RegistryEndpoint {
// SetRegistryEndpointCredentials allows to change the credentials used for
// endpoint access for existing RegistryEndpoint configuration
func SetRegistryEndpointCredentials(prefix, credentials string) error {
registry, err := GetRegistryEndpoint(prefix)
registry, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: prefix})
if err != nil {
return err
}
Expand Down
42 changes: 31 additions & 11 deletions pkg/registry/endpoints_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"sync"
"testing"

"github.com/argoproj-labs/argocd-image-updater/pkg/image"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -13,21 +15,21 @@ func Test_GetEndpoints(t *testing.T) {
RestoreDefaultRegistryConfiguration()

t.Run("Get default endpoint", func(t *testing.T) {
ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, "docker.io", ep.RegistryPrefix)
})

t.Run("Get GCR endpoint", func(t *testing.T) {
ep, err := GetRegistryEndpoint("gcr.io")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "gcr.io"})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, ep.RegistryPrefix, "gcr.io")
})

t.Run("Infer endpoint", func(t *testing.T) {
ep, err := GetRegistryEndpoint("foobar.com")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "foobar.com"})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, "foobar.com", ep.RegistryPrefix)
Expand All @@ -43,7 +45,7 @@ func Test_AddEndpoint(t *testing.T) {
require.NoError(t, err)
})
t.Run("Get example.com endpoint", func(t *testing.T) {
ep, err := GetRegistryEndpoint("example.com")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "example.com"})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, ep.RegistryPrefix, "example.com")
Expand All @@ -56,7 +58,7 @@ func Test_AddEndpoint(t *testing.T) {
t.Run("Change existing endpoint", func(t *testing.T) {
err := AddRegistryEndpoint(NewRegistryEndpoint("example.com", "Example", "https://example.com", "", "library", true, TagListSortLatestFirst, 5, 0))
require.NoError(t, err)
ep, err := GetRegistryEndpoint("example.com")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "example.com"})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, ep.Insecure, true)
Expand All @@ -71,7 +73,7 @@ func Test_SetEndpointCredentials(t *testing.T) {
t.Run("Set credentials on default registry", func(t *testing.T) {
err := SetRegistryEndpointCredentials("", "env:FOOBAR")
require.NoError(t, err)
ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, ep.Credentials, "env:FOOBAR")
Expand All @@ -80,13 +82,31 @@ func Test_SetEndpointCredentials(t *testing.T) {
t.Run("Unset credentials on default registry", func(t *testing.T) {
err := SetRegistryEndpointCredentials("", "")
require.NoError(t, err)
ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, ep.Credentials, "")
})
}

func Test_SelectRegistryBasedOnMaxPrefixContains(t *testing.T) {
RestoreDefaultRegistryConfiguration()

t.Run("Set credentials on default registry", func(t *testing.T) {
err := SetRegistryEndpointCredentials("foo.bar/prefix1", "env:FOOBAR_1")
require.NoError(t, err)
err = SetRegistryEndpointCredentials("foo.bar/prefix2", "env:FOOBAR_2")
require.NoError(t, err)
err = SetRegistryEndpointCredentials("foo.bar/prefix1/sub-prefix", "env:FOOBAR_SUB_1")
require.NoError(t, err)

ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "foo.bar", ImageName: "prefix1/sub-prefix/image"})
require.NoError(t, err)
require.NotNil(t, ep)
assert.Equal(t, ep.Credentials, "env:FOOBAR_SUB_1")
})
}

func Test_EndpointConcurrentAccess(t *testing.T) {
RestoreDefaultRegistryConfiguration()
const numRuns = 50
Expand All @@ -96,7 +116,7 @@ func Test_EndpointConcurrentAccess(t *testing.T) {
wg.Add(numRuns)
for i := 0; i < numRuns; i++ {
go func() {
ep, err := GetRegistryEndpoint("gcr.io")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "gcr.io"})
require.NoError(t, err)
require.NotNil(t, ep)
wg.Done()
Expand All @@ -114,7 +134,7 @@ func Test_EndpointConcurrentAccess(t *testing.T) {
creds := fmt.Sprintf("secret:foo/secret-%d", i)
err := SetRegistryEndpointCredentials("", creds)
require.NoError(t, err)
ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
require.NotNil(t, ep)
wg.Done()
Expand All @@ -132,7 +152,7 @@ func Test_SetDefault(t *testing.T) {
assert.Equal(t, "docker.io", dep.RegistryPrefix)
assert.True(t, dep.IsDefault)

ep, err := GetRegistryEndpoint("ghcr.io")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "ghcr.io"})
require.NoError(t, err)
require.NotNil(t, ep)
require.False(t, ep.IsDefault)
Expand All @@ -146,7 +166,7 @@ func Test_SetDefault(t *testing.T) {

func Test_DeepCopy(t *testing.T) {
t.Run("DeepCopy endpoint object", func(t *testing.T) {
ep, err := GetRegistryEndpoint("docker.pkg.github.com")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "docker.pkg.github.com"})
require.NoError(t, err)
require.NotNil(t, ep)
newEp := ep.DeepCopy()
Expand Down
10 changes: 5 additions & 5 deletions pkg/registry/registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func Test_GetTags(t *testing.T) {
regClient.On("NewRepository", mock.Anything).Return(nil)
regClient.On("Tags", mock.Anything).Return([]string{"1.2.0", "1.2.1", "1.2.2"}, nil)

ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)

img := image.NewFromIdentifier("foo/bar:1.2.0")
Expand All @@ -42,7 +42,7 @@ func Test_GetTags(t *testing.T) {
regClient.On("NewRepository", mock.Anything).Return(nil)
regClient.On("Tags", mock.Anything).Return([]string{"1.2.0", "1.2.1", "1.2.2"}, nil)

ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)

img := image.NewFromIdentifier("foo/bar:1.2.0")
Expand All @@ -65,7 +65,7 @@ func Test_GetTags(t *testing.T) {
regClient.On("NewRepository", mock.Anything).Return(nil)
regClient.On("Tags", mock.Anything).Return([]string{"1.2.0", "1.2.1", "1.2.2"}, nil)

ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)

img := image.NewFromIdentifier("foo/bar:1.2.0")
Expand Down Expand Up @@ -97,7 +97,7 @@ func Test_GetTags(t *testing.T) {
regClient.On("ManifestForTag", mock.Anything, mock.Anything).Return(meta1, nil)
regClient.On("TagMetadata", mock.Anything, mock.Anything).Return(&tag.TagInfo{}, nil)

ep, err := GetRegistryEndpoint("")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: ""})
require.NoError(t, err)
ep.Cache.ClearCache()

Expand Down Expand Up @@ -132,7 +132,7 @@ registries:
// New registry configuration
err = AddRegistryEndpointFromConfig(epl.Items[0])
require.NoError(t, err)
ep, err := GetRegistryEndpoint("ghcr.io")
ep, err := GetRegistryEndpoint(&image.ContainerImage{RegistryURL: "ghcr.io"})
require.NoError(t, err)
require.NotEqual(t, 0, ep.CredsExpire)

Expand Down