Move prefix validation to postProcessRegistries

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

pkg/sysregistriesv2/system_registries_v2.go

@@ -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:]
@@ -271,16 +264,6 @@ func (e *InvalidRegistries) Error() string {
 func parseLocation(input string) (string, error) {
 	trimmed := strings.TrimRight(input, "/")
 
-	// FIXME: This check needs to exist but fails for empty Location field with
-	// wildcarded prefix. Removal of this check "only" allows invalid input in,
-	// and does not prevent correct operation.
-	// https://github.com/containers/image/pull/1191#discussion_r610122617
-	//
-	//	if trimmed == "" {
-	//		return "", &InvalidRegistries{s: "invalid location: cannot be empty"}
-	//	}
-	//
-
 	if strings.HasPrefix(trimmed, "http://") || strings.HasPrefix(trimmed, "https://") {
 		msg := fmt.Sprintf("invalid location '%s': URI schemes are not supported", input)
 		return "", &InvalidRegistries{s: msg}
@@ -357,21 +340,31 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
 			return err
 		}
 
-		if reg.Prefix == "" {
-			if reg.Location == "" {
-				return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}
+		// Prefix and Location cannot both be empty.
+		// Either one at least must be set.
+		if reg.Prefix != "" {
+			if isWildcardedPrefix(reg.Prefix) && strings.ContainsAny(reg.Prefix, "/@:") {
+				msg := fmt.Sprintf("Wildcarded prefix should be in the format: *.example.com. Current prefix %q is incorrectly formatted", reg.Prefix)
+				return &InvalidRegistries{s: msg}
 			}
-			reg.Prefix = reg.Location
-		} else {
 			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"}
 		}
 
 		// make sure mirrors are valid
@@ -768,6 +761,11 @@ func CredentialHelpers(sys *types.SystemContext) ([]string, error) {
 	return config.partialV2.CredentialHelpers, nil
 }
 
+// isWildcardedPrefix only checks if the first two characters match "*.".
+func isWildcardedPrefix(prefix string) bool {
+	return prefix[:2] == "*."
+}
+
 // refMatchingSubdomainPrefix returns the length of ref
 // iff ref, which is a registry, repository namespace, repository or image reference (as formatted by
 // reference.Domain(), reference.Named.Name() or reference.Reference.String()
@@ -804,7 +802,7 @@ func refMatchingSubdomainPrefix(ref, prefix string) int {
 // (This is split from the caller primarily to make testing easier.)
 func refMatchingPrefix(ref, prefix string) int {
 	switch {
-	case prefix[0:2] == "*.":
+	case isWildcardedPrefix(prefix):
 		return refMatchingSubdomainPrefix(ref, prefix)
 	case len(ref) < len(prefix):
 		return -1
@@ -919,17 +917,6 @@ func loadConfigFile(path string, forceV2 bool) (*parsedConfig, error) {
 		res.shortNameMode = types.ShortNameModeInvalid
 	}
 
-	// Valid wildcarded prefixes must be in the format: *.example.com
-	// FIXME: Move to postProcessRegistries
-	// https://github.com/containers/image/pull/1191#discussion_r610623829
-	for i := range res.partialV2.Registries {
-		prefix := res.partialV2.Registries[i].Prefix
-		if prefix[:2] == "*." && strings.ContainsAny(prefix, "/@:") {
-			msg := fmt.Sprintf("Wildcarded prefix should be in the format: *.example.com. Current prefix %q is incorrectly formatted", prefix)
-			return nil, &InvalidRegistries{s: msg}
-		}
-	}
-
 	// Parse and validate short-name aliases.
 	cache, err := newShortNameAliasCache(path, &res.partialV2.shortNameAliasConf)
 	if err != nil {

pkg/sysregistriesv2/system_registries_v2_test.go

@@ -134,6 +134,25 @@ func TestMirrors(t *testing.T) {
 	assert.True(t, reg.Mirrors[1].Insecure)
 }
 
+func TestWildcardedPrefix(t *testing.T) {
+	for _, c := range []struct {
+		prefix   string
+		expected bool
+	}{
+		// Only check if the first two characters are "*."
+		{"*.io", true},
+		{"*.com/foo@bar", true},
+		{"foo.com/bar", false},
+		{"*foo.com/bar", false},
+		{"foo*.com/bar", false},
+		{".foo*.com/bar", false},
+		{"*.foo*.com/bar", true},
+	} {
+		isValid := isWildcardedPrefix(c.prefix)
+		assert.Equal(t, c.expected, isValid)
+	}
+}
+
 func TestRefMatchingSubdomainPrefix(t *testing.T) {
 	for _, c := range []struct {
 		ref, prefix string
@@ -291,7 +310,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)
@@ -383,6 +402,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)
+	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)
 }
@@ -579,6 +604,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"},
@@ -602,7 +635,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"},

pkg/sysregistriesv2/testdata/find-registry.conf

@@ -60,3 +60,6 @@ prefix="*.com"
 
 [[registry]]
 prefix="*.foobar.io"
+
+[[registry]]
+prefix="empty-location.set-prefix.example.com"