Allow config authors to always use Prefix #1368

lsm5 · 2021-09-01T02:10:51Z

Previously an unset Location was only allowed for wildcarded Prefixes.
This commit will allow any valid Prefix.

Signed-off-by: Lokesh Mandvekar lsm5@fedoraproject.org

vrothberg

Needs tests and changes to docs. AFAIR the docs state the location must be set, and we need to explain how rewriting works when location is empty.

The tests should make sure that rewriting the reference will return the input.

lsm5 · 2021-09-01T12:47:09Z

Done. PTAL.

TomSweeneyRedHat · 2021-09-01T14:05:17Z

LGTM

lsm5 · 2021-09-01T14:15:25Z

@mtrmac removing the entire check for empty location from rewriteReference seemed to cause failures in TestRewriteReferenceFailedDuringParseNamed . So, let me know if the FIXME comment still needs to exist there.

vrothberg

I'd love a test with at least one registries.conf file with an empty location and set prefix.

vrothberg · 2021-09-02T08:25:51Z

docs/containers-registries.conf.5.md

@@ -82,7 +82,7 @@ location = "internal-registry-for-example.net/bar"
 requests for the image `example.com/foo/myimage:latest` will actually work with the
 `internal-registry-for-example.net/bar/myimage:latest` image.

-With a `prefix` containing a wildcard in the format: "*.example.com" for subdomain matching,
+With any valid `prefix` with or without a wildcard in the format: "*.example.com" for subdomain matching,
 the location can be empty. In such a case,


This sentence seems a bit too complicated. I suggest: "The location can be empty if the prefix is set."

vrothberg · 2021-09-02T08:28:14Z

pkg/sysregistriesv2/system_registries_v2.go

-			}
-			reg.Prefix = reg.Location
-		} else {
+		// Allow config authors to always use Prefix.


I don't find the comment helpful. I'd prefer a comment stating that either prefix OR location OR both must be set.

rhatdan · 2021-09-16T08:03:28Z

@lsm5 any update on this PR?

lsm5 · 2021-09-16T14:24:29Z

@rhatdan working on it..

lsm5 · 2021-09-22T19:26:35Z

@mtrmac @vrothberg PTAL

vrothberg

LGTM
@mtrmac PTAL

lsm5 · 2021-09-27T13:13:58Z

@mtrmac ping PTAL

vrothberg · 2021-09-28T08:01:15Z

@mtrmac ping PTAL

On PTO until next week.

Previously an unset Location was only allowed for wildcarded Prefixes. This commit will allow configs with emtpy location or empty prefix, so long as the other is valid. Remove wildcard prefix check for empty location in rewriteReference. Fixes: 1. containers#1191 (comment) 2. containers#1191 (comment) Co-authored-by: Miloslav Trmač <mitr@redhat.com> Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>

lsm5 · 2021-10-05T13:33:33Z

@mtrmac PTAL

mtrmac

Thanks for the PR, and I apologize for the late review.

ACK overall; a few more simplifications seem possible.

mtrmac · 2021-10-05T17:43:19Z

docs/containers-registries.conf.5.md

@@ -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
 `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,


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).

mtrmac · 2021-10-05T17:48:14Z

docs/containers-registries.conf.5.md

+With any valid `prefix`, the location can be emtpy. In such a case,
 prefix matching will occur, but no reference rewrite will occur. The


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”.

mtrmac · 2021-10-05T17:49:14Z

docs/containers-registries.conf.5.md

@@ -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
 `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,
 prefix matching will occur, but no reference rewrite will occur. The
 original requested image string will be used as-is. But other settings like
 `insecure` / `blocked` / `mirrors` will be applied to matching images.


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.

mtrmac · 2021-10-05T17:53:15Z

docs/containers-registries.conf.5.md

@@ -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


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.

mtrmac · 2021-10-05T17:55:54Z

pkg/sysregistriesv2/system_registries_v2.go

 			}
+			reg.Location = reg.Prefix
+		} else {
+			return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}


(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.)

mtrmac · 2021-10-05T18:04:38Z

pkg/sysregistriesv2/system_registries_v2.go

-		} else {
+		// Prefix and Location cannot both be empty.
+		// Either one at least must be set.
+		if reg.Prefix != "" {


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.

mtrmac · 2021-10-05T18:07:47Z

pkg/sysregistriesv2/system_registries_v2_test.go

+	assert.Nil(t, err)
+	assert.NotNil(t, reg)


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

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.

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.)

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.

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.

This was referenced Sep 1, 2021

Fix subdomain matching FIXMEs in commit 373440662 #1366

Closed

Move prefix validation to postProcessRegistries #1369

Draft

vrothberg reviewed Sep 1, 2021

View reviewed changes

lsm5 force-pushed the wildcard-cert-fixes-2 branch from 4c1ec49 to 03c954d Compare September 1, 2021 12:32

lsm5 force-pushed the wildcard-cert-fixes-2 branch from 03c954d to 3c68593 Compare September 1, 2021 13:56

lsm5 force-pushed the wildcard-cert-fixes-2 branch from 3c68593 to 9e12044 Compare September 1, 2021 14:12

vrothberg reviewed Sep 2, 2021

View reviewed changes

lsm5 force-pushed the wildcard-cert-fixes-2 branch 2 times, most recently from 38823ca to 48ddd0e Compare September 20, 2021 14:39

vrothberg reviewed Sep 23, 2021

View reviewed changes

lsm5 force-pushed the wildcard-cert-fixes-2 branch from 48ddd0e to 1fadd42 Compare October 4, 2021 19:43

lsm5 force-pushed the wildcard-cert-fixes-2 branch from 1fadd42 to dcc2e8f Compare October 5, 2021 13:33

mtrmac reviewed Oct 5, 2021

View reviewed changes

lsm5 mentioned this pull request Oct 13, 2021

Golang panic since upgrade from 3.0.2 to 3.2.3 containers/podman#11934

Closed

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow config authors to always use Prefix #1368

Allow config authors to always use Prefix #1368

lsm5 commented Sep 1, 2021

vrothberg left a comment

lsm5 commented Sep 1, 2021

TomSweeneyRedHat commented Sep 1, 2021

lsm5 commented Sep 1, 2021

vrothberg left a comment

vrothberg Sep 2, 2021

vrothberg Sep 2, 2021

rhatdan commented Sep 16, 2021

lsm5 commented Sep 16, 2021

lsm5 commented Sep 22, 2021

vrothberg left a comment

lsm5 commented Sep 27, 2021

vrothberg commented Sep 28, 2021

lsm5 commented Oct 5, 2021

mtrmac left a comment

mtrmac Oct 5, 2021

mtrmac Oct 5, 2021

mtrmac Oct 5, 2021

mtrmac Oct 5, 2021

mtrmac Oct 5, 2021

mtrmac Oct 5, 2021

mtrmac Oct 5, 2021

lsm5 Oct 13, 2021

mtrmac Oct 13, 2021

lsm5 Oct 13, 2021

mtrmac Oct 14, 2021

	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,

		With any valid `prefix`, the location can be emtpy. In such a case,
		prefix matching will occur, but no reference rewrite will occur. The

		@@ -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

Allow config authors to always use Prefix #1368

Are you sure you want to change the base?

Allow config authors to always use Prefix #1368

Conversation

lsm5 commented Sep 1, 2021

vrothberg left a comment

Choose a reason for hiding this comment

lsm5 commented Sep 1, 2021

TomSweeneyRedHat commented Sep 1, 2021

lsm5 commented Sep 1, 2021

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Sep 16, 2021

lsm5 commented Sep 16, 2021

lsm5 commented Sep 22, 2021

vrothberg left a comment

Choose a reason for hiding this comment

lsm5 commented Sep 27, 2021

vrothberg commented Sep 28, 2021

lsm5 commented Oct 5, 2021

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment