I think this should set SystemRegistriesConfDirPath to something non-existent or not-a-directory, so that the controller doesn’t read current configuration of the node/container where it is currently running.

(In both places, ideally by sharing the SystemContext)

Thanks for the explanation.

(Non-blocking: The entry for the primary repo is not necessary if NeverContactSource … but it also doesn’t really hurt; and otherwise we would need to special-case the “no configuration for registry” code path, i.e. this is a bit simpler than being 100% precise.)

[As a general rule, prefer ParseNormalizedNamed. It shouldn’t make a difference for valid scopes, … but do we actually validate that? IsValidRegistriesConfScope seems rather loose, and I’m not sure that there is any other location validating scopes more strictly.

I’m … worried what happens in the “scope == host name” case — AFAICS reference.Parse turns that into "[nothing]/$input". But it might work well enough.

OTOH with ParseNormalizedNamed, the dummyPath part would really have to happen before this parsing. See elsewhere.]

On second thought, ParseNormalizedNamed("docker.io/library") would produce an unwanted result (or we would have to work around that even more); reference.Parse() is really better here.

I think this can be just scope = scopeRef.Name(), or scopeRef = reference.TrimNamed(scopeRef), without specifically checking for tagged/digested.

Ugh… I didn’t realize there’s this corner case.

It’s safe to do because we have already looked up reg, but, AFAICS, if we add dummyPath here, we must also strip it from the returned endpoints.

… or, alternatively, unconditionally add dummyPath, and unconditionally remove it. Either way, ugly…

(Non-blocking? This does work, but it does a bit more parsing than necessary. I think:

// dummyPrefix
if !strings.Contains(scope, "/") { // implies no tag and no digest, so this should be safe
   scope += dummyPath
   addedDummy = true
}
repo := reference.ParseNormalizedNamed(scope).TrimNamed()
digestRef = reference.WithDigest(repo, digest.Parse(dummyDigest))
tagRef = reference.WithTag(repo, dummyTag)

should work.

AFAICS this should be strings.TrimSuffix(s.Reference.Name(), dummyPathIfUsed); s.Endpoint is the “root” of the mirror, not the final remapped repo (consider an original scope of example.com/foo/bar, and a mirror defined for example.com/foo)

a.com/a1/a2 is mirrored to a-a1.mirror/a2/a2, so that should be the scope. (See Reference vs. Endpoint in the previous review brach.)

I don’t think this should be added (probably Endpoint vs. Reference?)

This doesn’t hurt but also doesn’t need to be added — *.x.example.com is a subset of *.example.com

Doesn’t hurt but doesn’t need to be added.

@mtrmac Could you review this? We also need to add a check for dummyPrefix.

(I’m afraid that my suggestion to use the c/image matching code has not been simpler — certainly not much simpler.)

To be explicit, the situation is a config entry with

• prefix: *.example.com
• location not set — OCP never does that for wild-carded scopes
• and perhaps some mirrors, perhaps not, e.g. this entry might exist because there was an “insecure” bool set

And in that case the source reference is returned as is.

Here, unlike the added dummyPath, I’m worried about collisions with user-defined names (e.g. a mirroring setup from *.example.com to matched.example.com). I don’t think just a string comparison can resolve that.

It seems to me that this needs a condition && s.Endpoint.Location == "" to rule out this case: mirror endpoints must, by definition, have a mirror location. So if we have Location set, the host name was user-supplied; if Location is unset, the host name comes from dummyPrefix.

(Alternatively, I think the Location == "" check could be the only condition here, but then all primary endpoints would be ruled out and would have to be added at the top level. That would actually match the addScopeMirrorsSigstoreRegistriesdConfig function name a bit better — it would be adding only mirrors, and that would be good enough for currently OCP-generated configs, OTOH it would be less correct for processing registries.conf in general, because top-level redirection (`prefix: "logical"; location: "actualServer") would not be handled. I think we can handle top-level redirection here “for free”, so it’s better to do so than to risk future surprises.)

Please make sure there’s a unit test for this situation, and document what the situation and what the condition is checking for. (Something vaguely like “if we had to add a dummyPrefix to process a wildcard Registry entry, we only want to configure the mirror endpoints, if any; not to add a new entry for exactly dummyPrefix+originalScope”).

Also, this code would now depend on the caller unconditionally configuring attachments for registriesDockerConfig[policyScope] — that also seems worth having a comment.

This place also needs updating with SystemRegistriesConfDirPath.

(Please create a single SystemContext variable to share that.)

Non-blocking: This cast probably can’t fail, but double-checking might be easier than worrying about crashes.

assert.ElementsMatch(…, expected, maps.Keys(gotRegistriesCfg.Docker)) would check not just a subset relationship, but equality.

@mtrmac PTAL. Test cases for #4449 (comment)

Thanks.

Also a test for policy scope foo.example.com/ns/repo vs *.example.com -> our.mirror/example (expected scopes foo.example.com/ns/repo. our.mirror/example/ns/repo) might be a interesting because it might be closer to real-world configurations (with fairly coarse mirror maps but fairly small policy scopes); I don’t expect it would change code coverage…

We got foo.example.com/ns/repo, our.mirror/example/ns/repo, our.mirror/example
https://github.com/openshift/machine-config-operator/pull/4449/files#diff-e75cbabf033d7cdc22ac1877e6e0134549c848c7226729274f806c3ab943f37eR1366

Thanks!

(This is only used on the registriesTOML != nil path, so it can be initialized there. A context with SystemRegistriesConfPath set but SystemRegistriesConfPath does not make all that much sense. But this works fine as is.)