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.

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

Add pull-from-mirror for adding per-mirror level restrictions #1411

Merged
merged 1 commit into from Mar 31, 2022

Conversation

QiWang19
Copy link
Collaborator

@QiWang19 QiWang19 commented Nov 19, 2021

Close: #1407
Add pull-from-mirror: all, digest-only, tag-only for adding per-mirror level restrictions

The mirror-by-digest-only for primary is still allowed configuring,
and it is honored for compatibility

Signed-off-by: Qi Wang qiwan@redhat.com

@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Nov 19, 2021

@mtrmac @vrothberg Could you review?

Copy link
Contributor

@mtrmac mtrmac left a comment

(A quick first glance only.)

pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Show resolved Hide resolved
@QiWang19 QiWang19 changed the title set mirror-by-digest-only for each mirror instead of registry Set digest-only for each mirror Nov 19, 2021
docs/containers-registries.conf.5.md Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
@QiWang19 QiWang19 force-pushed the digest-set-for-mirror branch 2 times, most recently from 33e1e10 to cd80244 Compare Nov 30, 2021
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Nov 30, 2021

@vrothberg @mtrmac PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Other than the nit, LGTM

pkg/sysregistriesv2/system_registries_v2_test.go Outdated Show resolved Hide resolved
@QiWang19 QiWang19 force-pushed the digest-set-for-mirror branch from cd80244 to dff5d35 Compare Dec 3, 2021
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Dec 3, 2021

@vrothberg PTAL.

Copy link
Member

@vrothberg vrothberg left a comment

LGTM
@mtrmac PTAL

@QiWang19 QiWang19 force-pushed the digest-set-for-mirror branch 2 times, most recently from f23b1f1 to d3363ea Compare Dec 3, 2021
@rhatdan
Copy link
Member

@rhatdan rhatdan commented Dec 3, 2021

@mtrmac PTAL

@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Dec 10, 2021

@mtrmac Could you review?

@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Jan 5, 2022

@mtrmac Could you review?

Copy link
Contributor

@mtrmac mtrmac left a comment

I apologize for the late response.

This does work fine, and it’s perfectly sufficient for correctness.

OTOH it doesn’t implement openshift/enhancements#929 as currently proposed. That enhancement has a separate list of per-digest mirrors and per-tag mirrors, i.e. it is possible to have a tag-only mirror.


I can imagine, with absolutely no data, that some users might want to use “cheap” mirrors for digested references, and only use “more expensive”, better-secured, mirrors for tagged references. But even in that hypothetical case, I find it fairly hard to argue that they’d rather have a failed pull than use the “more expensive” mirror.

So, for c/image, I think this could well be sufficient — and users might set up the configuration so that digest-only mirrors are listed before the digest+tag mirrors, if they wanted to avoid use of a “costly” digest+tag mirror.

But it’s pretty surprising to introduce the OpenShift feature, and this underlying design for an implementation of the OpenShift feature, at the same time, but not have them do exactly the same thing. So either c/image should allow configuring tag-only mirrors, or the OpenShift CR should be changed so that the “tag” mirrors are documented to also be used for digested references.


It’s somewhat likely that there was an earlier conversation about this that I can’t remember/find; is there?

Except I can find #1407 (comment) , so it’s quite possible that the current inconsistent state of things is my fault. If that’s the case, I’m sorry.

docs/containers-registries.conf.5.md Outdated Show resolved Hide resolved
Also note that if all mirrors in the `mirror` array have `digest-only = true`, images referenced by a tag will only use the primary
registry, failing if that registry is not accessible.
Copy link
Contributor

@mtrmac mtrmac Jan 15, 2022

Choose a reason for hiding this comment

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

I don’t think we need the note about only using the primary twice; it’s a side point and not that important.

Maybe leave the note in the previous location (which is “outside” of the mirror-by-digest-only definition list entry anyway), and just write “if all mirrors are configured to be digest-only”, without mentioning a specific field name.

Copy link
Contributor

@mtrmac mtrmac Jan 22, 2022

Choose a reason for hiding this comment

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

The digest-only copy will, I assume be removed — then the only copy of this note should be a separate paragraph, not a part of the mirrors-by-digest-only paragraph.

pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
@@ -675,6 +675,72 @@ func TestPullSourcesFromReference(t *testing.T) {
assert.Equal(t, 1, len(pullSources))
}

func TestPullSourcesMirrorFromReference(t *testing.T) {
sys := &types.SystemContext{
Copy link
Contributor

@mtrmac mtrmac Jan 15, 2022

Choose a reason for hiding this comment

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

To make it easier for the future, could TestPullSourcesFromReference be modified to use the same table-driven format, and then these tests added to that function, without adding a new config file?

It’s a bit out of scope (especially with the testing for Location/Insecure), so I’m perfectly fine with doing that work myself later, in a separate PR.

Copy link
Collaborator Author

@QiWang19 QiWang19 Jan 18, 2022

Choose a reason for hiding this comment

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

ok, I'd leave that to you.

pkg/sysregistriesv2/system_registries_v2_test.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2_test.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2_test.go Outdated Show resolved Hide resolved
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Jan 18, 2022

So either c/image should allow configuring tag-only mirrors, or the OpenShift CR should be changed so that the “tag” mirrors are documented to also be used for digested references.

Does the tag-only mirrors mean the registry reference without tag should be rejected in the case? The openshift/enhancements#929 proposed to allow using tags, not to limit the use to tag-only.

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Jan 19, 2022

By tag-only mirrors I mean mirrors that aren’t used for digested references. Digested references would only use digest-only mirrors (if any), not the tag-only mirrors, and the primary (non-mirror) location.

The enhancement’s ImageSourceTagPolicy AFAICS only documents affecting tagged references; with this PR, that can’t be implemented as specified because the ISTP mirrors would be used for digest-only references as well.

Given ISDP (source -> mirrorDigest), ISTP (source -> mirrorTag), AFAICS OpenShift documents that source:latest should be looked up in (mirrorTag:latest, source:latest), and source@sha256… should be looked up in (mirrorDigest@sha256…, source@sha256…). With this PR, the former can’t be configured, the best we could do is (mirrorTag:latest, mirrorDigest:latest, source:latest).

I don’t have a very strong opinion on whether to change this implementation or the OpenShift CR.

@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Jan 19, 2022

By tag-only mirrors I mean mirrors that aren’t used for digested references. Digested references would only use digest-only mirrors (if any), not the tag-only mirrors, and the primary (non-mirror) location.

With this PR, the former can’t be configured, the best we could do is (mirrorTag:latest, mirrorDigest:latest, source:latest).

The former one follows current implementation, like the testcase

I think you mean the digest example, source@sha256… should be looked up in (mirrorDigest@sha256…, source@sha256…) cannot be configured,
Current PR does (mirrorDigest@sha256, mirrorTag@sha256, source@sha256).

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Jan 19, 2022

I think you mean the digest example

Yes, my mistake. Thanks.

@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Jan 19, 2022

@mtrmac I fixed the reviews, PTAL.

Copy link
Contributor

@mtrmac mtrmac left a comment

Thanks! The implementation LGTM, just some test and documentation nits.


I don’t think it would be reasonable to block this PR on the inconsistency with OpenShift plans, per #1411 (review) . But I do hope there’s some plan to address that.

I suppose another c/image PR that adds another per-mirror option can land later, or something.

pkg/sysregistriesv2/system_registries_v2_test.go Outdated Show resolved Hide resolved
pullSource, err := registry.PullSourcesFromReference(digestedRef)
assert.Nil(t, err)
for i, s := range tc.digestSources {
assert.Equal(t, s, pullSource[i].Endpoint.Location)
Copy link
Contributor

@mtrmac mtrmac Jan 22, 2022

Choose a reason for hiding this comment

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

Absolutely non-blocking: Testing pullSource[i].Reference would be more directly related to the end-user-wanted behavior.

But this works just fine, and I can clean that up later when unifying the two tests.

@QiWang19 QiWang19 force-pushed the digest-set-for-mirror branch 2 times, most recently from e73f234 to 71f0bfa Compare Mar 24, 2022
@QiWang19 QiWang19 changed the title Set digest-only for each mirror Add pull-from-mirror for adding per-mirror level restrictions Mar 24, 2022
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Mar 24, 2022

@mtrmac @vrothberg PTAL

pkg/sysregistriesv2/system_registries_v2.go Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
@QiWang19 QiWang19 force-pushed the digest-set-for-mirror branch 3 times, most recently from 84f966d to 165cb39 Compare Mar 24, 2022
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Mar 24, 2022

@vrothberg @mtrmac PTAL.

@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Mar 28, 2022

@mtrmac Could you review it?

pkg/sysregistriesv2/system_registries_v2.go Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Mar 30, 2022

@vrothberg @mtrmac PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Other than the nit, LGTM

switch mirror.PullFromMirror {
case MirrorByDigestOnly:
if !isDigested {
continue
}
endpoints = append(endpoints, mirror)
case MirrorByTagOnly:
if isDigested {
continue
}
endpoints = append(endpoints, mirror)
default:
endpoints = append(endpoints, mirror)
}
Copy link
Member

@vrothberg vrothberg Mar 30, 2022

Choose a reason for hiding this comment

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

Suggested change
switch mirror.PullFromMirror {
case MirrorByDigestOnly:
if !isDigested {
continue
}
endpoints = append(endpoints, mirror)
case MirrorByTagOnly:
if isDigested {
continue
}
endpoints = append(endpoints, mirror)
default:
endpoints = append(endpoints, mirror)
}
switch mirror.PullFromMirror {
case MirrorByDigestOnly:
if !isDigested {
continue
}
case MirrorByTagOnly:
if isDigested {
continue
}
}
endpoints = append(endpoints, mirror)

Copy link
Contributor

@mtrmac mtrmac left a comment

Design and overall implementation LGTM.

pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2.go Outdated Show resolved Hide resolved
pkg/sysregistriesv2/system_registries_v2_test.go Outdated Show resolved Hide resolved
docs/containers-registries.conf.5.md Outdated Show resolved Hide resolved
Close: containers#1407
Add pull-from-mirror: all, digest-only, tag-only for adding per-mirror level restrictions
to image pull through mirrors.

The `mirror-by-digest-only` for primary is still allowed configuring,
and it is honored for compatibility

Signed-off-by: Qi Wang <qiwan@redhat.com>
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Mar 30, 2022

@vrothberg @mtrmac PTAL. This is ready to get merged.

mtrmac
mtrmac approved these changes Mar 31, 2022
Copy link
Contributor

@mtrmac mtrmac left a comment

LGTM. Thanks!

Copy link
Member

@vrothberg vrothberg left a comment

LGTM

@vrothberg vrothberg merged commit f3198c2 into containers:main Mar 31, 2022
9 checks passed
@QiWang19
Copy link
Collaborator Author

@QiWang19 QiWang19 commented Mar 31, 2022

@mtrmac @vrothberg can we have a new release for this feature?

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Apr 1, 2022

Sure, a new release sounds fine. @vrothberg WDYT? Is there any reason not to, or anything urgent we should finish first?

@vrothberg
Copy link
Member

@vrothberg vrothberg commented Apr 4, 2022

Releasing now SGTM. Thanks for working on this, @QiWang19!

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Apr 4, 2022

#1514 filed to release v5.21.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants