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

QiWang19 · 2022-02-28T19:06:54Z

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: [OCPNODE-747] New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags enhancements#929

QiWang19 · 2022-02-28T20:35:03Z

config/v1/0000_10_config-operator_01_imagecontentpolicy.crd.yaml

umohnani8 · 2022-03-01T20:05:53Z

config/v1/0000_10_config-operator_01_imagedigestmirrorset.crd.yaml

+                          pattern: ^((?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?)(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$
+                        x-kubernetes-list-type: set
+                      neverContactSource:
+                        description: When enabled, prevents image pull from the specified repository in the pull spec if the image pull form the mirror list fails. Default is false, the image will continue to be pulled from the pull spec if the image can not be pulled from the mirror list. neverContactSource is valid configuration only when one or more mirrors are in the mirror list.


Is pull spec the same as the source here? I think to the blear we should say something like prevents image pull from the source registry if all mirrors fail
@mtrmac wdyt?

The source here is probably a regex, something like prevents image pull from the pull spec that matches the source?

The rest of the CR documentation seems to fairly consistently say something like “from the repository specified in the pull spec”, so this should be consistent.

(In other documents we talk about source or primary locations; this CR doesn’t seem to use that terminology right now. I don’t very much care what the name is, but it should be consistent.)

(And just for the record, the source is a repo or repo namespace, or a wild-carded host name pattern, as documented in the comments; it is never a regex. Though the general point that we might not be pulling from exactly the Source string value is completely correct.)

config/v1/0000_10_config-operator_01_imagedigestmirrorset.crd.yaml

config/v1/0000_10_config-operator_01_imagetagmirrorset.crd.yaml

QiWang19 · 2022-03-06T03:32:40Z

@mtrmac PTAL.

config/v1/0000_10_config-operator_01_imagecontentpolicy.crd.yaml

config/v1/types_image_digest_mirror_set.go

deads2k · 2022-03-10T16:59:15Z

config/v1/types_image_digest_mirror_set.go

+	// https://github.com/containers/image/blob/main/docs/containers-registries.conf.5.md#choosing-a-registry-toml-table
+	// +optional
+	// +listType=set
+	Mirrors []Mirrors `json:"mirrors,omitempty"`


Is my registry authentication presented to these mirrors? Given multiple possible tokens, are tokens tried against each mirror first or is a single token tried against all mirrors first?

tokens tried against each mirror. The order of mirrors in this mirrors list is the desired order. The mirrors will be tried in this order, if you have multiple tokens including the token for the current pulling mirror, the token will be used to access the mirror.

All of these mirrors configurations are invisible to Kubelet, and the CRI (the Kubelet / CRI-O API) does not currently allow passing credentials for multiple registries.

So, what happens is that the Kubelet reads the Pod’s pull secret, if any, and extracts the credentials only matching source, and provides them in the CRI API. If the Pod’s pull secret contains any credentials for mirrors, those are completely ignored. CRI-O uses the CRI-provided credentials to contact the source only; for mirrors, it uses credentials available in the node-global config.json (in OpenShift, that’s the global cluster pull secret).

(IIRC, if the Pod’s pull secret(s) contain multiple sets of credentials for the same registry, Kubelet tries the pull operation, with each credential. So, in that case, we would try all of the credentials for the source registry; but the mirrors, if any, would all use the global cluster pull secret, repeatedly on each of those retries.)

(The inability of the user to provide mirror credentials via a pod’s pull secret is a known limitation of the mirroring mechanism, and nothing changes about that with this enhancement / these API changes.)

config/v1/types_image_tag_mirror_set.go

config/v1/0000_10_config-operator_01_imagedigestmirrorset.crd.yaml

config/v1/types_image_digest_mirror_set.go

mtrmac · 2022-03-12T16:19:22Z

config/v1/types_image_digest_mirror_set.go

+
+// SourcePolicy defines the fallback policy if fails to pull image from the mirrors.
+// +kubebuilder:validation:Enum=sourceNever;sourceLast
+type SourcePolicy string


Non-blocking: SourcePolicy feels a bit too generic of a name to use within the wide scope of of config/v1 . MirrorSourcePolicy at least? ImageMirrorSourceAccess?

config/v1/types_image_digest_mirror_set.go

mtrmac · 2022-03-12T16:26:00Z

config/v1/0000_10_config-operator_01_imagedigestmirrorset.crd.yaml

+                          pattern: ^((?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?)(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$
+                        x-kubernetes-list-type: set
+                      neverContactSource:
+                        description: When enabled, prevents image pull from the specified repository in the pull spec if the image pull form the mirror list fails. Default is false, the image will continue to be pulled from the pull spec if the image can not be pulled from the mirror list. neverContactSource is valid configuration only when one or more mirrors are in the mirror list.


The rest of the CR documentation seems to fairly consistently say something like “from the repository specified in the pull spec”, so this should be consistent.

(In other documents we talk about source or primary locations; this CR doesn’t seem to use that terminology right now. I don’t very much care what the name is, but it should be consistent.)

config/v1/types_image_digest_mirror_set.go

deads2k · 2022-03-14T20:01:38Z

config/v1/types_image_digest_mirror_set.go

+	NeverContactSource MirrorSourcePolicy = "neverContactSource"
+
+	// allowContactingSource allow falling back to the specified repository in the pull spec if the image pull from the mirror list fails.
+	AllowContactingSource MirrorSourcePolicy = "allowContactingSource"


CamelCase, "AllowContactingSource"

fixed and the above CamelCase

deads2k · 2022-03-14T20:05:07Z

After comments on case, this looks good.

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: [OCPNODE-747] New CRD ImageDigestMirrorSet and ImageTagMirrorSet to support AllowMirrByTags enhancements#929 Signed-off-by: Qi Wang <qiwan@redhat.com>

mtrmac

LGTM, to the little extent I understand K8s API rules.

(Note David’s comments though.)

openshift-ci · 2022-03-14T21:16:25Z

@QiWang19: all tests passed!

Full PR test history. Your PR dashboard.

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.

deads2k · 2022-03-15T14:08:54Z

/approve

deads2k · 2022-03-15T14:09:08Z

/lgtm

openshift-ci · 2022-03-15T14:09:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, QiWang19

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

mburke5678 · 2022-03-15T18:18:19Z

/label docs-approved

FYI: @TanyaShearonRH

mrunalp · 2022-03-15T18:43:50Z

/label

QiWang19 · 2022-03-15T18:45:40Z

/label px-approved
/label qe-approved

Update openshift/api dependency to import ImageDigest/TagMirrorSet CRD: openshift/api#1126 Signed-off-by: Qi Wang <qiwan@redhat.com>

openshift-ci bot requested review from jwforres and mfojtik February 28, 2022 19:07

QiWang19 force-pushed the mirrorset branch 2 times, most recently from 430b766 to ca942a5 Compare February 28, 2022 20:16

QiWang19 changed the title ~~Add CRD ImageDigestMirrorSet and ImageTagMirrorSet~~ [OCPNODE-521] Add CRD ImageDigestMirrorSet and ImageTagMirrorSet Feb 28, 2022

umohnani8 reviewed Mar 1, 2022

View reviewed changes

config/v1/0000_10_config-operator_01_imagecontentpolicy.crd.yaml Outdated Show resolved Hide resolved

umohnani8 reviewed Mar 1, 2022

View reviewed changes

config/v1/0000_10_config-operator_01_imagedigestmirrorset.crd.yaml Outdated Show resolved Hide resolved

umohnani8 reviewed Mar 1, 2022

View reviewed changes

config/v1/0000_10_config-operator_01_imagetagmirrorset.crd.yaml Outdated Show resolved Hide resolved

umohnani8 reviewed Mar 1, 2022

View reviewed changes

config/v1/0000_10_config-operator_01_imagetagmirrorset.crd.yaml Outdated Show resolved Hide resolved

QiWang19 force-pushed the mirrorset branch 2 times, most recently from 8f3df92 to d5b530a Compare March 6, 2022 01:46

deads2k reviewed Mar 8, 2022

View reviewed changes

config/v1/0000_10_config-operator_01_imagecontentpolicy.crd.yaml Outdated Show resolved Hide resolved

QiWang19 force-pushed the mirrorset branch from d5b530a to 4297e22 Compare March 9, 2022 17:32

deads2k reviewed Mar 10, 2022

View reviewed changes

config/v1/types_image_digest_mirror_set.go Outdated Show resolved Hide resolved

deads2k reviewed Mar 10, 2022

View reviewed changes

config/v1/types_image_digest_mirror_set.go Show resolved Hide resolved

deads2k reviewed Mar 10, 2022

View reviewed changes

config/v1/types_image_digest_mirror_set.go Outdated Show resolved Hide resolved

deads2k reviewed Mar 10, 2022

View reviewed changes

config/v1/types_image_digest_mirror_set.go Outdated Show resolved Hide resolved

deads2k reviewed Mar 10, 2022

View reviewed changes

config/v1/types_image_digest_mirror_set.go Outdated Show resolved Hide resolved

deads2k reviewed Mar 10, 2022

View reviewed changes

config/v1/types_image_tag_mirror_set.go Show resolved Hide resolved

QiWang19 force-pushed the mirrorset branch from 4297e22 to 797f0d4 Compare March 10, 2022 21:33

mtrmac reviewed Mar 12, 2022

View reviewed changes

QiWang19 force-pushed the mirrorset branch from 797f0d4 to cb2bdcd Compare March 14, 2022 19:35

deads2k reviewed Mar 14, 2022

View reviewed changes

config/v1/types_image_digest_mirror_set.go Outdated Show resolved Hide resolved

deads2k reviewed Mar 14, 2022

View reviewed changes

mtrmac reviewed Mar 14, 2022

View reviewed changes

QiWang19 force-pushed the mirrorset branch from cb2bdcd to 6785360 Compare March 14, 2022 20:59

openshift-ci bot assigned deads2k Mar 15, 2022

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

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

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Mar 15, 2022

openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR labels Mar 15, 2022

openshift-merge-robot merged commit d7c10d0 into openshift:master Mar 15, 2022

QiWang19 mentioned this pull request Mar 15, 2022

Update openshift/api dependency to import ImageDigest/TagMirrorSet CRD openshift/client-go#208

Merged

QiWang19 mentioned this pull request Mar 16, 2022

Bump openshift/api, openshit/client-go to add ImageMirrorSet CRDs openshift/cluster-config-operator#236

Merged

soltysh mentioned this pull request Apr 7, 2022

Do not register new ICSP CRDs, yet #1164

Merged

QiWang19 mentioned this pull request Aug 17, 2022

[OCPNODE-521]Register ImageDigest/TagMirrorSet CRDs #1247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

QiWang19 commented Feb 28, 2022

QiWang19 commented Feb 28, 2022

umohnani8 Mar 1, 2022

QiWang19 Mar 6, 2022 •

edited

Loading

mtrmac Mar 12, 2022

mtrmac Mar 12, 2022

QiWang19 commented Mar 6, 2022

deads2k Mar 10, 2022

QiWang19 Mar 10, 2022

mtrmac Mar 12, 2022

mtrmac Mar 12, 2022

mtrmac Mar 12, 2022

deads2k Mar 14, 2022

QiWang19 Mar 14, 2022

deads2k commented Mar 14, 2022

mtrmac left a comment

openshift-ci bot commented Mar 14, 2022

deads2k commented Mar 15, 2022

deads2k commented Mar 15, 2022

openshift-ci bot commented Mar 15, 2022

mburke5678 commented Mar 15, 2022 •

edited

Loading

mrunalp commented Mar 15, 2022

QiWang19 commented Mar 15, 2022

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

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

Conversation

QiWang19 commented Feb 28, 2022

QiWang19 commented Feb 28, 2022

Choose a reason for hiding this comment

QiWang19 Mar 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Mar 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Mar 14, 2022

mtrmac left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 14, 2022

deads2k commented Mar 15, 2022

deads2k commented Mar 15, 2022

openshift-ci bot commented Mar 15, 2022

mburke5678 commented Mar 15, 2022 • edited Loading

mrunalp commented Mar 15, 2022

QiWang19 commented Mar 15, 2022

QiWang19 Mar 6, 2022 •

edited

Loading

mburke5678 commented Mar 15, 2022 •

edited

Loading