[OCPNODE-747] New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags #929

QiWang19 · 2021-10-10T03:38:57Z

New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags (API and design for Epic https://issues.redhat.com/browse/OCPNODE-521)
Enhancement jira card: OCPNODE-747

We can continue the discussions from the current design.

Signed-off-by: Qi Wang [email protected]

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md

QiWang19 · 2021-10-11T14:06:51Z

@mtrmac @smarterclayton PTAL

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md

QiWang19 · 2021-10-15T16:56:50Z

@mtrmac PTAL.

mtrmac

(Various questions from my previous review seem to remain outstanding.)

mtrmac · 2021-10-18T12:42:19Z

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md

+	// host[:port]
+	// host[:port]/namespace[/namespace…]
+	// host[:port]/namespace[/namespace…]/repo
+	// host[:port]/namespace[/namespace…]/repo(:_tag|@digest)


Unlike containers-registries.conf(5), https://github.com/openshift/runtime-utils/blob/8b8348d80d1d1e7b6cf06fb009d5965e0b55baa2/pkg/registries/registries.go#L19 doesn’t currently support the :tag|@digest part; it can be a host[:port][[/namespace…]/repo] only.

(I don’t think single-image mirrors are worth worrying about… unless we needed to allow them now because we won’t be allowed to change the validation rules again?)

removed host[:port]/namespace[/namespace…]/repo(:_tag|@digest)

mtrmac · 2021-10-18T12:50:07Z

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md

+	// https://github.com/containers/image/blob/main/docs/containers-registries.conf.5.md#choosing-a-registry-toml-table
+	// +required
+	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:Pattern=`(^(([a-zA-Z]|[a-zA-Z][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z]|[A-Za-z][A-Za-z0-9\-]*[A-Za-z0-9])(:[0-9]+)?(\/[^\/:\n]+)*(\/[^\/:\n]+((:[^\/:\n]+)|(@[^\n]+)))?$)|(^(([a-zA-Z\*]|[a-zA-Z\*][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)?(([a-zA-Z]|[a-zA-Z][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z]|[A-Za-z][A-Za-z0-9\-]*[A-Za-z0-9])$)`


This seems incorrect, in that it allows *a.; wildcards are supported only with the exact *. prefix.

Even with the explanation, validating the regex feels like too much work (which I didn’t do now). Does the API annotation have any mechanisms that could help?

If not, building the regex from components somehow would be nice. Maybe something similar to the way https://github.com/containers/image/blob/main/docker/reference/regexp.go does it — that’s admittedly extreme in being literal, but it does have the nice property that it results in a Go program that can be reviewed in small pieces, and then run with a value.String() to get a pattern.

I don't see the kubebuilder doc has a mechanism helper for regex. I can use some containers/image helpers in goplayground here to define the pattern https://play.golang.org/p/NO9_2LmqPiu

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md

QiWang19 · 2021-10-19T14:11:45Z

@mtrmac could you have another round of review?

kikisdeliveryservice

@QiWang19 I think it makes sense to update this enhancement at some point to pick up the new template changes (8d07520) involving API extensions since this will be adding a new CRD

Template link for ref: https://github.com/openshift/enhancements/blob/master/guidelines/enhancement_template.md

QiWang19 · 2021-11-08T22:06:59Z

@mtrmac Could you review? Updated:

Mentioned registries.conf should support digest required mirrors and tags allowed mirrors in the separated list.
ImageContentPolicy API change to have new mirrorsByTgas for using mirror by tags.

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md

QiWang19 · 2022-02-16T18:22:56Z

@umohnani8 @mrunalp @mtrmac PTAL.

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md

Add CRD ImageDigestMirrorSet and ImageTagMirrorSet to have API for epics: - https://issues.redhat.com/browse/OCPNODE-521: different API for two saprate lists for digest image pull and tag image pull using mirrors. - https://issues.redhat.com/browse/OCPNODE-810: add an option for user to choose if the source of the mirror should be denied if the mirrors pull failed. Enhancement: openshift/enhancements#929 Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2022-02-28T20:34:49Z

@umohnani8 @mrunalp PTAL

umohnani8 · 2022-03-01T20:23:51Z

Proposal around neverContactSource LGTM
Thanks @QiWang19!

mtrmac

Looks good overall, mostly trivial typos.

(Noting that more discussion is happening in openshift/api#1126 .)

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md

…AllowMirrByTags Update the enhancement to describe the work has been done based on the previous discussions for Epic https://issues.redhat.com/browse/OCPNODE-521 We can continue the discussions from the current design. Signed-off-by: Qi Wang <[email protected]>

New CRD ImageSourceDigestPolicy and ImageSourceTagPolicy to support AllowMirrByTags Signed-off-by: Qi Wang <[email protected]>

New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags Signed-off-by: Qi Wang <[email protected]>

mtrmac

LGTM.

Thanks for all the updates!

rphillips · 2022-03-28T19:02:56Z

/lgtm

Awesome Job!

openshift-ci · 2022-03-28T19:10:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mrunalp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-03-28T19:17:37Z

@QiWang19: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

soltysh · 2022-04-06T17:06:11Z

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md

+  - [Cluster-config-operator](https://github.com/openshift/cluster-config-operator)
+  - [Openshift-api-server](https://github.com/openshift/openshift-apiserver/blob/98786f917ffc7d3dc3b05893f405970b87a419b9/pkg/image/apiserver/registries/registries.go)
+  - [Runtime utils](https://github.com/openshift/runtime-utils/blob/8b8348d80d1d1e7b6cf06fb009d5965e0b55baa2/pkg/registries/registries.go#L50)
+  - [Openshift-controller-manager](https://github.com/openshift/openshift-controller-manager/blob/2a11f145ad7fcf3e92460800de1d13ba7fbb90b0/pkg/build/controller/build/build_controller.go#L20943)


@rphillips @QiWang19 oc and oc-mirror is missing here, and both of those tools heavily rely on the current ICSP implementation for all of image mirroring, this also includes our docs. Based on below note I can assume that the node team will also handle all the appropriate changes for oc and oc-mirror, is that correct?

Yes, I will handle the related changes.

openshift-ci bot requested review from dgoodwin and sttts October 10, 2021 03:39

QiWang19 mentioned this pull request Oct 10, 2021

Create ImageContentPolicy CRD to config/v1 and allowMirrorByTags support openshift/api#874

Merged

QiWang19 force-pushed the allow-mirror-by-tags branch from e86c4dd to 13729d0 Compare October 10, 2021 03:49

sttts reviewed Oct 11, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

sttts reviewed Oct 11, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

sttts reviewed Oct 11, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

sttts reviewed Oct 11, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

sttts reviewed Oct 11, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

mtrmac reviewed Oct 12, 2021

View reviewed changes

QiWang19 force-pushed the allow-mirror-by-tags branch from 13729d0 to 45efed5 Compare October 15, 2021 04:20

QiWang19 commented Oct 15, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

QiWang19 force-pushed the allow-mirror-by-tags branch from 45efed5 to b7ea2c4 Compare October 15, 2021 14:52

mtrmac reviewed Oct 18, 2021

View reviewed changes

QiWang19 force-pushed the allow-mirror-by-tags branch 2 times, most recently from a0cc302 to ecb2599 Compare October 19, 2021 04:00

kikisdeliveryservice reviewed Nov 3, 2021

View reviewed changes

QiWang19 force-pushed the allow-mirror-by-tags branch 3 times, most recently from e030049 to f325e8b Compare November 8, 2021 22:01

QiWang19 force-pushed the allow-mirror-by-tags branch from f325e8b to 70f41ac Compare November 9, 2021 16:05

bparees reviewed Nov 9, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

saschagrunert reviewed Nov 10, 2021

View reviewed changes

enhancements/api-review/add-new-CRD-ImageContentPolicy(ICP)-to-config.openshift.io.md Outdated Show resolved Hide resolved

QiWang19 force-pushed the allow-mirror-by-tags branch from 1e36840 to bec341f Compare November 11, 2021 15:25

QiWang19 changed the title ~~[OCPNODE-521] New CRD ImageContentPolicy(ICP) to support AllowMirrByTags~~ [OCPNODE-521] New CRD ImageSourceDigestPolicy and ImageSourceTagPolicy to support AllowMirrByTags Nov 11, 2021

QiWang19 force-pushed the allow-mirror-by-tags branch from 555a3de to 37f4713 Compare November 11, 2021 23:41

QiWang19 force-pushed the allow-mirror-by-tags branch 2 times, most recently from 2738eed to abf342e Compare February 15, 2022 21:44

kikisdeliveryservice reviewed Feb 16, 2022

View reviewed changes

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md Outdated Show resolved Hide resolved

kikisdeliveryservice reviewed Feb 16, 2022

View reviewed changes

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md Outdated Show resolved Hide resolved

kikisdeliveryservice reviewed Feb 16, 2022

View reviewed changes

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md Outdated Show resolved Hide resolved

kikisdeliveryservice reviewed Feb 16, 2022

View reviewed changes

.../api-review/add-new-CRD-ImageDigestMirrorSet-and-ImageTagMirrorSet-to-config.openshift.io.md Show resolved Hide resolved

QiWang19 force-pushed the allow-mirror-by-tags branch from abf342e to 62b98af Compare February 28, 2022 18:18

QiWang19 mentioned this pull request Feb 28, 2022

[OCPNODE-521] Add CRD ImageDigestMirrorSet and ImageTagMirrorSet openshift/api#1126

Merged

mtrmac mentioned this pull request Mar 4, 2022

Use github.com/openshift/runtime-utils/pkg/registries openshift/openshift-apiserver#205

Merged

mtrmac reviewed Mar 12, 2022

View reviewed changes

QiWang19 added 2 commits March 14, 2022 16:26

CRD ImageSourceDigestPolicy and ImageSourceTagPolicy

74d816c

New CRD ImageSourceDigestPolicy and ImageSourceTagPolicy to support AllowMirrByTags Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the allow-mirror-by-tags branch from 62b98af to c0504c9 Compare March 14, 2022 20:28

CRD ImageDigestMirrorSet and ImageTagMirrorSet

cc9b39e

New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the allow-mirror-by-tags branch from c0504c9 to cc9b39e Compare March 14, 2022 20:35

mtrmac reviewed Mar 14, 2022

View reviewed changes

mrunalp approved these changes Mar 28, 2022

View reviewed changes

openshift-ci bot assigned rphillips Mar 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2022

openshift-merge-robot merged commit ad0d7b2 into openshift:master Mar 28, 2022

soltysh reviewed Apr 6, 2022

View reviewed changes

[OCPNODE-747] New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags #929

[OCPNODE-747] New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags #929

Uh oh!

Conversation

QiWang19 commented Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QiWang19 commented Oct 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QiWang19 commented Oct 15, 2021

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

QiWang19 Oct 27, 2021

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

QiWang19 Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QiWang19 commented Oct 19, 2021

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

QiWang19 commented Nov 8, 2021

Uh oh!

Uh oh!

Uh oh!

QiWang19 commented Feb 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

QiWang19 commented Feb 28, 2022

Uh oh!

umohnani8 commented Mar 1, 2022

Uh oh!

mtrmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

rphillips commented Mar 28, 2022

QiWang19 commented Oct 10, 2021 •

edited

Loading

QiWang19 Oct 19, 2021 •

edited

Loading

mtrmac left a comment •

edited

Loading