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

Allow config authors to always use Prefix #1368

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow config authors to always use Prefix #1368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions docs/containers-registries.conf.5.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ location = "internal-registry-for-example.net/bar"
requests for the image `example.com/foo/myimage:latest` will actually work with the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Above, in

By default, this equal to prefix (in which case prefix can be omitted …

the parenthesized aside doesn’t make sense, if location is unset, prefix can’t be omitted.

Basically I think we should document it this way:

  • prefix is how the right section is chosen. (It is ~assumed to be always set.)
  • location is how we redirect; if unset, no redirection happens.
  • As a special case, (prefix unset + location set) is interpreted the same as (prefix set + location unset)

We already document the special case in the “Choosing a [[registry]] TOML table” section, so maybe we can just not discuss it here at all.

`internal-registry-for-example.net/bar/myimage:latest` image.

With a `prefix` containing a wildcard in the format: "*.example.com" for subdomain matching,
the location can be empty. In such a case,
With any valid `prefix`, the location can be emtpy. In such a case,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
With any valid `prefix`, the location can be emtpy. In such a case,
With any valid `prefix`, the location can be empty. In such a case,

but we don’t exactly mean “empty”, we mean “missing” / “not provided by the user”; that this shows up as "" is just an implementation detail of our Go implementation (our lazy Go implementation, it would probably be possible to accurate… and reject "" values).

prefix matching will occur, but no reference rewrite will occur. The
Comment on lines +85 to 86
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ultimately, the “Choosing a [[registry]] TOML table” already describes the matching semantics, and that’s where it should be documented.

This is a paragraph about the location field, in a “Remapping and mirroring registries” section; maybe all this needs to say is something like “if only one of prefix and location is provided, the original requested image reference will be used as-is without redirection”.

original requested image string will be used as-is. But other settings like
`insecure` / `blocked` / `mirrors` will be applied to matching images.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m not sure we even need the example for doing nothing; we almost certainly don’t need two.

Also the “But other settings like” sentence is duplicated above and below the example, probably unnecessarily.

Expand All @@ -92,6 +91,10 @@ Example: Given
```
prefix = "*.example.com"
```
OR
```
prefix = "blah.example.com"
```
requests for the image `blah.example.com/foo/myimage:latest` will be used
as-is. But other settings like insecure/blocked/mirrors will be applied to matching images

Expand Down
33 changes: 16 additions & 17 deletions pkg/sysregistriesv2/system_registries_v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,7 @@ func (e *Endpoint) rewriteReference(ref reference.Named, prefix string) (referen
}
// In the case of an empty `location` field, simply return the original
// input ref as-is.
//
// FIXME: already validated in postProcessRegistries, so check can probably
// be dropped.
// https://github.com/containers/image/pull/1191#discussion_r610621608
if e.Location == "" {
if prefix[:2] != "*." {
return nil, fmt.Errorf("invalid prefix '%v' for empty location, should be in the format: *.example.com", prefix)
}
return ref, nil
}
newNamedRef = e.Location + refString[prefixLen:]
Expand Down Expand Up @@ -357,21 +350,27 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
return err
}

if reg.Prefix == "" {
if reg.Location == "" {
return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}
}
reg.Prefix = reg.Location
} else {
// Prefix and Location cannot both be empty.
// Either one at least must be set.
if reg.Prefix != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be simpler as

if reg.Prefix == "" && reg.Location == "" { fail }
if reg.Location != "" { reg.Location, err = parseLocation(…); if err … }
if reg.Prefix == "" { reg.Prefix = reg.Location }
else { reg.Prefix, err = parseLocation(…); if err … }

or perhaps

// Validate individual fields
if reg.Location != "" { reg.Location, err = parseLocation(…); if err … }
if reg.Prefix != "" { reg.Prefix, err = parseLocation(…); if err … } 
// Validate the combination, normalize to have reg.Prefix always set
if reg.Prefix == "" {
    if reg.Location == "" { fail }
    else { reg.Prefix = reg.Location }
}

Either way, please remove the parseLocation call above this line, and make parseLocation fail on "" again.

reg.Prefix, err = parseLocation(reg.Prefix)
if err != nil {
return err
}
// FIXME: allow config authors to always use Prefix.
// https://github.com/containers/image/pull/1191#discussion_r610622495
if reg.Prefix[:2] != "*." && reg.Location == "" {
return &InvalidRegistries{s: "invalid condition: location is unset and prefix is not in the format: *.example.com"}
if reg.Location != "" {
reg.Location, err = parseLocation(reg.Location)
if err != nil {
return err
}
}
} else if reg.Location != "" {
reg.Prefix, err = parseLocation(reg.Location)
if err != nil {
return err
}
reg.Location = reg.Prefix
} else {
return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}
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, this is pre-existing and has more instances, i.e. basically a note to self to improve this later: invalid condition is not very helpful in a user-facing message.)

}

// make sure mirrors are valid
Expand Down
17 changes: 15 additions & 2 deletions pkg/sysregistriesv2/system_registries_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func TestFindRegistry(t *testing.T) {

registries, err := GetRegistries(sys)
assert.Nil(t, err)
assert.Equal(t, 19, len(registries))
assert.Equal(t, 20, len(registries))

reg, err := FindRegistry(sys, "simple-prefix.com/foo/bar:latest")
assert.Nil(t, err)
Expand Down Expand Up @@ -383,6 +383,12 @@ func TestFindRegistry(t *testing.T) {
assert.Equal(t, "empty-prefix.com", reg.Prefix)
assert.Equal(t, "empty-prefix.com", reg.Location)

reg, err = FindRegistry(sys, "empty-location.set-prefix.example.com/namespace/repo")
assert.Nil(t, err)
assert.NotNil(t, reg)
Comment on lines +387 to +388
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Non-blocking: These should be require.. OTOH this is consistent with the rest, and only matters on test failures.)

Copy link
Member Author

Choose a reason for hiding this comment

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

so would you prefer require be used for all other existing test cases too? Or only for the missing-location + set-prefix case?

I could make a separate PR if we need to change it throughout the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly don’t think this is worth much worrying about; if you find updating pre-existing test cases fun, sure, go for it, but it’s not work that results in anything users could notice and consider valuable while using the software. As far as “clean code” goes, require is a bit more correct but consistency is also nice, so to me that’s not a strong reason to prefer either.

(Just to be explicit: require should be used for testing prerequisites that must be true for other checks to be even valid to do, e.g.

ptr, err := testSubject()
require.NoError(t, err)
assert.Equal(t, …, *ptr) // would crash if testSubject returned (nil, err) and the above were assert.NoError(…)

But there is a minimal practical difference; with assert.NoError(t, err) the test crashes with a backtrace, with require.NoError(t, err) the test fails with an error message containing err. It doesn’t matter at all if the test completely succeeds. If the test fails because err is set, in both cases the test fails, it’s easy enough to identify the location of the failure, and probably something about testSubject needs to be updated. require is only better in that it includes the actual error text in the test failure output, and that could help with a inconsistent rarely-occurring failure in CI.)

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, that's really helpful thanks, I'll consider introducing that in a future PR, will make a note in the test file for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But there is a minimal practical difference; with assert.NoError(t, err) the test crashes with a backtrace, with require.NoError(t, err) the test fails with an error message containing err.

I’m sorry, this is nonsense of course; assert.NoError(t, err) does report the unexpected error value first, it’s just the later use of *ptr that crashes. So really the difference between failures of the two implementations is basically cosmetic.

assert.NotNil(t, "empty-location.set-prefix.example.com", reg.Prefix)
assert.Equal(t, "", reg.Location)

_, err = FindRegistry(&types.SystemContext{SystemRegistriesConfPath: "testdata/this-does-not-exist.conf"}, "example.com")
assert.Error(t, err)
}
Expand Down Expand Up @@ -579,6 +585,14 @@ func TestRewriteReferenceSuccess(t *testing.T) {
{"sub.example.io/library/prefix/image", "*.example.io", "example.com", "example.com/library/prefix/image"},
{"another.sub.example.io:5000/library/prefix/image:latest", "*.sub.example.io", "example.com", "example.com:5000/library/prefix/image:latest"},
{"foo.bar.io/ns1/ns2/ns3/ns4", "*.bar.io", "omg.bbq.com/roflmao", "omg.bbq.com/roflmao/ns1/ns2/ns3/ns4"},
// Empty location with non-wildcard prefix examples. Essentially, no
// rewrite occurs and original reference is used as-is.
{"registry.com/foo", "registry.com", "", "registry.com/foo"},
{"abc.internal.registry.com/foo:bar", "abc.internal.registry.com", "", "abc.internal.registry.com/foo:bar"},
{"abc.internal.registry.com/foo/bar:baz", "abc.internal.registry.com", "", "abc.internal.registry.com/foo/bar:baz"},
{"alien.vs.predator.foobar.io:5000/foo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "alien.vs.predator.foobar.io", "",
"alien.vs.predator.foobar.io:5000/foo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},
{"alien.vs.predator.foobar.io:5000/omg:bbq", "alien.vs.predator.foobar.io", "", "alien.vs.predator.foobar.io:5000/omg:bbq"},
// Empty location with wildcard prefix examples. Essentially, no
// rewrite occurs and original reference is used as-is.
{"abc.internal.registry.com/foo:bar", "*.internal.registry.com", "", "abc.internal.registry.com/foo:bar"},
Expand All @@ -602,7 +616,6 @@ func TestRewriteReferenceFailedDuringParseNamed(t *testing.T) {
{"example.com/foo/image:latest", "example.com/foo", "example.com/path/"},
{"example.com/foo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"example.com/foo", "example.com"},
{"example.com:5000/image:latest", "example.com", ""},
{"example.com:5000/image:latest", "example.com", "example.com:5000"},
// Malformed prefix
{"example.com/foo/image:latest", "example.com//foo", "example.com/path"},
Expand Down
3 changes: 3 additions & 0 deletions pkg/sysregistriesv2/testdata/find-registry.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,6 @@ prefix="*.com"

[[registry]]
prefix="*.foobar.io"

[[registry]]
prefix="empty-location.set-prefix.example.com"