Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable subdomain matching in registries.conf #1191
Enable subdomain matching in registries.conf #1191
Changes from all commits
da93a53
1f072d1
1adbe71
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens on user input (prefix=
*.foo
, location unset)? AFAICS that’s converted into (prefix=*.foo
, location=*.foo
), and this rewrite function then turnsdomain.foo/bar
into*.foo/bar
, breaking the format.(There should be a test for this as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I have re-formatted the above to show asterisks instead of being interpreted as italic.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, just to confirm, do you mean this:
So, unset location doesn't seem to be accepted to begin with and
make test
panics along with a:Error: Expected nil, but got: error loading registries configuration "testdata/find-registry.conf": invalid location: cannot be empty
I'll look at adding a test for panics if we need it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err .. let me double check on this ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading more of the RFE, it’s unclear to me whether they actually need the insecure flag or whether it was incidental, and primarily then wanted the allowed/blocked registries. Either way, the “blocked registries” option shows up both in
policy.json
andregistries.conf
per https://github.com/openshift/runtime-utils/blob/master/pkg/registries/registries.go and https://github.com/openshift/machine-config-operator/blob/ca8fcb887f153604556f2706cee8a33165879797/pkg/controller/container-runtime-config/helpers.go#L381 , so I think we do need need to have wildcard matching without triggering a remapping.It feels unlikely to me the primary feature that must get in before the deadline is “remap
*.foo/…
tobar/…
”, but it’s possible; I’m afraid I can’t now review the full history of the related RFEs. Either way prioritization is not up to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your points convince me. Thanks, @mtrmac!
@lsm5. I agree that we need to support the
location=""
case in which case no remapping will happen (i.e., the provided input will be used) but the matching should work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, a
podman pull xxx.internal.registry.com/image:tag
would pull fromxxx. ...
without TLS (insecure).blocked=true
would work implicitlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, I'll add a check for this and may also need code change I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrothberg I first added and then removed the check for
insecure=true
based on @mtrmac's later review comment and @umohnani8 said openshift/mco didn't need it. Current version only checks for empty location and wildcarded prefix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit too broad — it now allows empty mirrors, or empty entries in
unqualifiedSearchRegistries
. I’d expect this check to remain, the callers could recognize the various special situations and decide not to parse an empty string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad. changing ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Earlier there was a reference to a deadline today — if things had to be cut, this removed check “only” allows invalid input in, and does not prevent correct operation.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deadline is tomorrow, so I guess I still have time to do the right thing.
If you don't mind dumbing things down for me, would you prefer that this check remain and I also add a condition to check that it's not a
location
field? Otherwise anything test that requires alocation=""
fails currently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, tomorrow is only feature freeze and there's another month for final code freeze, which will give time for bugs and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that also makes me wonder if this func should have a name other than
parseLocation
, because I tend to assume it's only for the Location field. Maybe that's just me though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrmac For now, I left the check commented out along with a note about allowing invalid input and not preventing correct operation. let me know what's the best way to proceed. ^ /cc @vrothberg .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ideally there should be a
parsePrefix
(which allows*.
, and rejects/@:
the way the current later check does, and maybe validates against the charset limitations of ofreference.DomainRegexp
), and aparseLocation
(which rejects*.
, and allows/@:
, and allows areference.DomainRegexp
or areference.NameRegexp
, or…).I think it’s quite fine to defer that till later (especially because too strict validation of non-wildcard locations could break users, and tightening that validation is really a quite separate feature).
This might be a clean way (or I might be missing something):
parseLocation
reject empty input.parseLocation
can remain unchanged.postProcessRegistries
could go something like:reg.Location != ""
check needs to happen anyway for error checking/defaulting, and only the first one exists purely to implement the “location can be empty” situation, exactly in the one case where we allow an empty value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrmac On second thoughts, I have some other commitments this evening and also considering @vrothberg's timezone for tomorrow and such, I would like to defer this part to the bugfix phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is consistent with the current documentation and fine for now.
For future fix-ups:
*.
being a special case helps; allowing configuration authors to always use Prefix would simplify them. Compare Parse registries for wildcard entries openshift/runtime-utils#11 .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted as FIXME.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the loop below?
AFAICS with a
{ prefix = *.example; location = ""; insecure = true }; { prefix = *.other; location = ""; blocked = true }
the loop will complain about conflicting insecure/blocked settings.I think
regMap
should usereg.Prefix
ifreg.Location
is unset.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrmac PTAL ^ .. hope that looks ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err likely not :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regMap[reg.Location] = append(regMap[reg.Prefix], reg)
should use `reg.Prefix in both casesregMap
needs to considerreg.Prefix
as well ifreg.Location
is empty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, ack, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, PTAL. Other comments added as FIXMEs. Thanks.