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.

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

Add pull-from-mirror for adding per-mirror level restrictions #1411

Merged
merged 1 commit into from Mar 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions docs/containers-registries.conf.5.md
Expand Up @@ -106,20 +106,26 @@ contacted and contains the image will be used (and if none of the mirrors contai
the primary location specified by the `registry.location` field, or using the unmodified
user-specified reference, is tried last).

Each TOML table in the `mirror` array can contain the following fields, with the same semantics
as if specified in the `[[registry]]` TOML table directly:
- `location`
- `insecure`
Each TOML table in the `mirror` array can contain the following fields:
- `location`: same semantics
as specified in the `[[registry]]` TOML table
- `insecure`: same semantics
as specified in the `[[registry]]` TOML table
- `pull-from-mirror`: `all`, `digest-only` or `tag-only`. If "digest-only", mirrors will only be used for digest pulls. Pulling images by tag can potentially yield different images, depending on which endpoint we pull from. Restricting mirrors to pulls by digest avoids that issue. If "tag-only", mirrors will only be used for tag pulls. For a more up-to-date and expensive mirror that it is less likely to be out of sync if tags move, it should not be unnecessarily used for digest references. Default is "all" (or left empty), mirrors will be used for both digest pulls and tag pulls unless the mirror-by-digest-only is set for the primary registry.
Note that this per-mirror setting is allowed only when `mirror-by-digest-only` is not configured for the primary registry.

`mirror-by-digest-only`
: `true` or `false`.
If `true`, mirrors will only be used during pulling if the image reference includes a digest.
Note that if all mirrors are configured to be digest-only, images referenced by a tag will only use the primary
registry.
If all mirrors are configured to be tag-only, images referenced by a digest will only use the primary
registry.

Referencing an image by digest ensures that the same is always used
(whereas referencing an image by a tag may cause different registries to return
different images if the tag mapping is out of sync).

Note that if this is `true`, images referenced by a tag will only use the primary
registry, failing if that registry is not accessible.

*Note*: Redirection and mirrors are currently processed only when reading images, not when pushing
to a registry; that may change in the future.
Expand Down
62 changes: 54 additions & 8 deletions pkg/sysregistriesv2/system_registries_v2.go
Expand Up @@ -43,6 +43,16 @@ const builtinRegistriesConfDirPath = "/etc/containers/registries.conf.d"
// helper.
const AuthenticationFileHelper = "containers-auth.json"

const (
// configuration values for "pull-from-mirror"
// mirrors will be used for both digest pulls and tag pulls
MirrorAll = "all"
// mirrors will only be used for digest pulls
MirrorByDigestOnly = "digest-only"
// mirrors will only be used for tag pulls
MirrorByTagOnly = "tag-only"
)
vrothberg marked this conversation as resolved.
Show resolved Hide resolved

// Endpoint describes a remote location of a registry.
type Endpoint struct {
// The endpoint's remote location. Can be empty iff Prefix contains
Expand All @@ -53,6 +63,18 @@ type Endpoint struct {
// If true, certs verification will be skipped and HTTP (non-TLS)
// connections will be allowed.
Insecure bool `toml:"insecure,omitempty"`
// PullFromMirror is used for adding restrictions to image pull through the mirror.
// Set to "all", "digest-only", or "tag-only".
// If "digest-only", mirrors will only be used for digest pulls. Pulling images by
// tag can potentially yield different images, depending on which endpoint
// we pull from. Restricting mirrors to pulls by digest avoids that issue.
// If "tag-only", mirrors will only be used for tag pulls. For a more up-to-date and expensive mirror
// that it is less likely to be out of sync if tags move, it should not be unnecessarily
// used for digest references.
// Default is "all" (or left empty), mirrors will be used for both digest pulls and tag pulls unless the mirror-by-digest-only is set for the primary registry.
// This can only be set in a registry's Mirror field, not in the registry's primary Endpoint.
// This per-mirror setting is allowed only when mirror-by-digest-only is not configured for the primary registry.
PullFromMirror string `toml:"pull-from-mirror,omitempty"`
}

// userRegistriesFile is the path to the per user registry configuration file.
Expand Down Expand Up @@ -115,7 +137,7 @@ type Registry struct {
Blocked bool `toml:"blocked,omitempty"`
// If true, mirrors will only be used for digest pulls. Pulling images by
// tag can potentially yield different images, depending on which endpoint
// we pull from. Forcing digest-pulls for mirrors avoids that issue.
// we pull from. Restricting mirrors to pulls by digest avoids that issue.
MirrorByDigestOnly bool `toml:"mirror-by-digest-only,omitempty"`
}

Expand All @@ -130,17 +152,29 @@ type PullSource struct {
// reference.
func (r *Registry) PullSourcesFromReference(ref reference.Named) ([]PullSource, error) {
var endpoints []Endpoint

_, isDigested := ref.(reference.Canonical)
if r.MirrorByDigestOnly {
vrothberg marked this conversation as resolved.
Show resolved Hide resolved
// Only use mirrors when the reference is a digest one.
if _, isDigested := ref.(reference.Canonical); isDigested {
endpoints = append(r.Mirrors, r.Endpoint)
} else {
endpoints = []Endpoint{r.Endpoint}
// Only use mirrors when the reference is a digested one.
if isDigested {
endpoints = append(endpoints, r.Mirrors...)
}
} else {
endpoints = append(r.Mirrors, r.Endpoint)
for _, mirror := range r.Mirrors {
// skip the mirror if per mirror setting exists but reference does not match the restriction
switch mirror.PullFromMirror {
case MirrorByDigestOnly:
if !isDigested {
continue
}
case MirrorByTagOnly:
if isDigested {
continue
}
}
endpoints = append(endpoints, mirror)
QiWang19 marked this conversation as resolved.
Show resolved Hide resolved
}
}
endpoints = append(endpoints, r.Endpoint)

sources := []PullSource{}
for _, ep := range endpoints {
Expand Down Expand Up @@ -374,6 +408,10 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
}
}

// validate the mirror usage settings does not apply to primary registry
if reg.PullFromMirror != "" {
return fmt.Errorf("pull-from-mirror must not be set for a non-mirror registry %q", reg.Prefix)
}
// make sure mirrors are valid
for _, mir := range reg.Mirrors {
mir.Location, err = parseLocation(mir.Location)
Expand All @@ -387,6 +425,14 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
if mir.Location == "" {
return &InvalidRegistries{s: "invalid condition: mirror location is unset"}
}

if reg.MirrorByDigestOnly && mir.PullFromMirror != "" {
return &InvalidRegistries{s: fmt.Sprintf("cannot set mirror usage mirror-by-digest-only for the registry (%q) and pull-from-mirror for per-mirror (%q) at the same time", reg.Prefix, mir.Location)}
}
if mir.PullFromMirror != "" && mir.PullFromMirror != MirrorAll &&
mir.PullFromMirror != MirrorByDigestOnly && mir.PullFromMirror != MirrorByTagOnly {
return &InvalidRegistries{s: fmt.Sprintf("unsupported pull-from-mirror value %q for mirror %q", mir.PullFromMirror, mir.Location)}
}
vrothberg marked this conversation as resolved.
Show resolved Hide resolved
}
if reg.Location == "" {
regMap[reg.Prefix] = append(regMap[reg.Prefix], reg)
Expand Down
116 changes: 116 additions & 0 deletions pkg/sysregistriesv2/system_registries_v2_test.go
Expand Up @@ -671,6 +671,122 @@ func TestPullSourcesFromReference(t *testing.T) {
assert.Equal(t, 1, len(pullSources))
}

func TestPullSourcesMirrorFromReference(t *testing.T) {
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
sys := &types.SystemContext{
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it easier for the future, could TestPullSourcesFromReference be modified to use the same table-driven format, and then these tests added to that function, without adding a new config file?

It’s a bit out of scope (especially with the testing for Location/Insecure), so I’m perfectly fine with doing that work myself later, in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I'd leave that to you.

SystemRegistriesConfPath: "testdata/pull-sources-mirror-reference.conf",
SystemRegistriesConfDirPath: "testdata/this-does-not-exist",
}
registries, err := GetRegistries(sys)
require.NoError(t, err)
assert.Equal(t, 7, len(registries))

digest := "@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
tag := ":aaa"
for _, tc := range []struct {
registry string
digestSources []string
tagSources []string
}{
// Registry A has mirrors allow any kind of pull
{
"registry-a.com/foo/image",
[]string{"mirror-1.registry-a.com", "mirror-2.registry-a.com", "registry-a.com/bar"},
[]string{"mirror-1.registry-a.com", "mirror-2.registry-a.com", "registry-a.com/bar"},
},
// Registry B has mirrors allow digests pull only
{
"registry-b.com/foo/image",
[]string{"mirror-1.registry-b.com", "mirror-2.registry-b.com", "registry-b.com/bar"},
[]string{"registry-b.com/bar"},
},
// Registry C has a mirror allows digest pull only and a mirror allows any kind of pull
{
"registry-c.com/foo/image",
[]string{"mirror-1.registry-c.com", "mirror-2.registry-c.com", "registry-c.com/bar"},
[]string{"mirror-1.registry-c.com", "registry-c.com/bar"},
},
// Registry D set digest-only for registry level, allows only digest pulls
// Registry D has no digest-only set for mirrors table
{
"registry-d.com/foo/image",
[]string{"mirror-1.registry-d.com", "mirror-2.registry-d.com", "registry-d.com/bar"},
[]string{"registry-d.com/bar"},
},
// Registry E has mirrors only allows tag pull
{
"registry-e.com/foo/image",
[]string{"registry-e.com/bar"},
[]string{"mirror-1.registry-e.com", "mirror-2.registry-e.com", "registry-e.com/bar"},
},
// Registry F has one tag only mirror does not allow digest pull
{
"registry-f.com/foo/image",
[]string{"mirror-1.registry-f.com", "registry-f.com/bar"},
[]string{"mirror-1.registry-f.com", "mirror-2.registry-f.com", "registry-f.com/bar"},
},
// Registry G has one digest-only pull and one tag only pull
{
"registry-g.com/foo/image",
[]string{"mirror-1.registry-g.com", "mirror-3.registry-g.com", "mirror-4.registry-g.com", "registry-g.com/bar"},
[]string{"mirror-2.registry-g.com", "mirror-3.registry-g.com", "mirror-4.registry-g.com", "registry-g.com/bar"},
},
} {
// Digest
digestedRef := toNamedRef(t, tc.registry+digest)
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
registry, err := FindRegistry(sys, digestedRef.Name())
require.NoError(t, err)
require.NotNil(t, registry)
pullSource, err := registry.PullSourcesFromReference(digestedRef)
require.NoError(t, err)
for i, s := range tc.digestSources {
assert.Equal(t, s, pullSource[i].Endpoint.Location)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely non-blocking: Testing pullSource[i].Reference would be more directly related to the end-user-wanted behavior.

But this works just fine, and I can clean that up later when unifying the two tests.

}
// Tag
taggedRef := toNamedRef(t, tc.registry+tag)
QiWang19 marked this conversation as resolved.
Show resolved Hide resolved
registry, err = FindRegistry(sys, taggedRef.Name())
require.NoError(t, err)
require.NotNil(t, registry)
pullSource, err = registry.PullSourcesFromReference(taggedRef)
require.NoError(t, err)
for i, s := range tc.tagSources {
assert.Equal(t, s, pullSource[i].Endpoint.Location)
}
}
}

func TestInvalidMirrorConfig(t *testing.T) {
for _, tc := range []struct {
sys *types.SystemContext
expectErr string
}{
{
sys: &types.SystemContext{
SystemRegistriesConfPath: "testdata/invalid-config-level-mirror.conf",
SystemRegistriesConfDirPath: "testdata/this-does-not-exist",
},
expectErr: fmt.Sprintf("pull-from-mirror must not be set for a non-mirror registry %q", "registry-a.com/foo"),
},
{
sys: &types.SystemContext{
SystemRegistriesConfPath: "testdata/invalid-conflict-mirror.conf",
SystemRegistriesConfDirPath: "testdata/this-does-not-exist",
},
expectErr: fmt.Sprintf("cannot set mirror usage mirror-by-digest-only for the registry (%q) and pull-from-mirror for per-mirror (%q) at the same time", "registry-a.com/foo", "mirror-1.registry-a.com"),
},
{
sys: &types.SystemContext{
SystemRegistriesConfPath: "testdata/invalid-value-mirror.conf",
SystemRegistriesConfDirPath: "testdata/this-does-not-exist",
},
expectErr: fmt.Sprintf("unsupported pull-from-mirror value %q for mirror %q", "notvalid", "mirror-1.registry-a.com"),
},
} {
_, err := GetRegistries(tc.sys)
assert.ErrorContains(t, err, tc.expectErr)
}

}

func TestTryUpdatingCache(t *testing.T) {
ctx := &types.SystemContext{
SystemRegistriesConfPath: "testdata/try-update-cache-valid.conf",
Expand Down
11 changes: 11 additions & 0 deletions pkg/sysregistriesv2/testdata/invalid-config-level-mirror.conf
@@ -0,0 +1,11 @@
[[registry]]
prefix = "registry-a.com/foo"
location = "registry-a.com/bar"
pull-from-mirror = "digest-only"

[[registry.mirror]]
location = "mirror-1.registry-a.com"

[[registry.mirror]]
location = "mirror-2.registry-a.com"
insecure = true
12 changes: 12 additions & 0 deletions pkg/sysregistriesv2/testdata/invalid-conflict-mirror.conf
@@ -0,0 +1,12 @@
[[registry]]
prefix = "registry-a.com/foo"
location = "registry-a.com/bar"
mirror-by-digest-only = true

[[registry.mirror]]
pull-from-mirror = "digest-only"
location = "mirror-1.registry-a.com"

[[registry.mirror]]
location = "mirror-2.registry-a.com"
insecure = true
11 changes: 11 additions & 0 deletions pkg/sysregistriesv2/testdata/invalid-value-mirror.conf
@@ -0,0 +1,11 @@
[[registry]]
prefix = "registry-a.com/foo"
location = "registry-a.com/bar"

[[registry.mirror]]
pull-from-mirror = "notvalid"
location = "mirror-1.registry-a.com"

[[registry.mirror]]
location = "mirror-2.registry-a.com"
insecure = true
86 changes: 86 additions & 0 deletions pkg/sysregistriesv2/testdata/pull-sources-mirror-reference.conf
@@ -0,0 +1,86 @@
[[registry]]
prefix = "registry-a.com/foo"
location = "registry-a.com/bar"

[[registry.mirror]]
location = "mirror-1.registry-a.com"

[[registry.mirror]]
location = "mirror-2.registry-a.com"
insecure = true

[[registry]]
prefix = "registry-b.com/foo"
location = "registry-b.com/bar"

[[registry.mirror]]
pull-from-mirror = "digest-only"
location = "mirror-1.registry-b.com"

[[registry.mirror]]
pull-from-mirror = "digest-only"
location = "mirror-2.registry-b.com"

[[registry]]
prefix = "registry-c.com/foo"
location = "registry-c.com/bar"

[[registry.mirror]]
location = "mirror-1.registry-c.com"

[[registry.mirror]]
pull-from-mirror = "digest-only"
location = "mirror-2.registry-c.com"

[[registry]]
prefix = "registry-d.com/foo"
location = "registry-d.com/bar"
mirror-by-digest-only = true

[[registry.mirror]]
location = "mirror-1.registry-d.com"

[[registry.mirror]]
location = "mirror-2.registry-d.com"

[[registry]]
prefix = "registry-e.com/foo"
location = "registry-e.com/bar"

[[registry.mirror]]
pull-from-mirror = "tag-only"
location = "mirror-1.registry-e.com"

[[registry.mirror]]
pull-from-mirror = "tag-only"
location = "mirror-2.registry-e.com"

[[registry]]
prefix = "registry-f.com/foo"
location = "registry-f.com/bar"

[[registry.mirror]]
location = "mirror-1.registry-f.com"

[[registry.mirror]]
pull-from-mirror = "tag-only"
location = "mirror-2.registry-f.com"

[[registry]]
prefix = "registry-g.com/foo"
location = "registry-g.com/bar"

[[registry.mirror]]
pull-from-mirror = "digest-only"
location = "mirror-1.registry-g.com"

[[registry.mirror]]
pull-from-mirror = "tag-only"
location = "mirror-2.registry-g.com"

[[registry.mirror]]
location = "mirror-3.registry-g.com"

[[registry.mirror]]
pull-from-mirror = "all"
location = "mirror-4.registry-g.com"