Add ImageContentSourcePolicy to operator/v1alpha1 #354

umohnani8 · 2019-06-17T18:45:39Z

Adding a new operator/v1alpha1 definition caled ImageContentSourcePolicy
which will be used when using a CR to set the registry mirror rules
for resiliency and offline installs/upgrades.
A new CRD will be created to encompass this.

Signed-off-by: Urvashi Mohnani [email protected]

umohnani8 · 2019-06-17T18:46:07Z

@mrunalp @abhinavdahiya @mtrmac PTAL

abhinavdahiya · 2019-06-18T14:59:27Z

Talking with @smarterclayton it seemed like we don't want to add this API object to config/v1 as that has very high guarantee ..

Why not it to mirror.openshift.io v1alpha1 or v1beta1 ?

umohnani8 · 2019-06-18T15:23:38Z

@abhinavdahiya should I rename the CRD to mirror then as well? Also where is v1alpha1 or v1beta1 in openshift/api? I see a v1alpha1 for operator, so should I create a new directory mirror/v1alpha1 and add this there?

mrunalp · 2019-06-18T16:55:58Z

config/v1/types_image_content_source_policy.go

+}
+
+type ImageContentSourcePolicySpec struct {
+	// DigestMappingRules holds the registry mirroring rules that are written to


We can add a line saying that we only support pulling by digests from the mirrors and hence the name.

abhinavdahiya · 2019-06-19T00:30:18Z

@abhinavdahiya should I rename the CRD to mirror then as well? Also where is v1alpha1 or v1beta1 in openshift/api? I see a v1alpha1 for operator, so should I create a new directory mirror/v1alpha1 and add this there?

I think just moving this type to a new directorymirror/v1beta1 should be sufficient.
also add mirror:v1beta1 to https://github.com/openshift/api/blob/master/hack/update-deepcopy.sh#L13

smarterclayton · 2019-06-19T01:42:21Z

Or operators.openshift.io (which is our private api) in v1alpha1? On Jun 18, 2019, at 10:59 AM, Abhinav Dahiya <[email protected]> wrote: Talking with @smarterclayton <https://github.com/smarterclayton> it seemed like we don't want to add this API object to config/v1 as that has very high guarantee .. Why not it to mirror.openshift.io v1alpha1 or v1beta1 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#354?email_source=notifications&email_token=AAI37JYM66JDDLWY434UNJLP3DZ5DA5CNFSM4HYZIPIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX65PIA#issuecomment-503175072>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI37J3XJEKXSVRC5SNQZ7DP3DZ5DANCNFSM4HYZIPIA> .

abhinavdahiya · 2019-06-19T20:38:12Z

Or operators.openshift.io (which is our private api) in v1alpha1?

+1 for operators/v1apha1

mtrmac · 2019-06-19T23:06:03Z

config/v1/types_image_content_source_policy.go

+
+// DigestMappingRules holds cluster-wide information about how to handle mirros in the registries config.
+type DigestMappingRules struct {
+	// Sources are registries that are mirrors of each other.


Should this say “registries and/or repositories [(depending on the container runtime)]”, or something like that? (Per earlier conversations, CRI-O will support this at the level of individual repositories, and containerd could support this at registry level.)

mtrmac · 2019-06-19T23:06:06Z

config/v1/types_image_content_source_policy.go

+}
+
+type ImageContentSourcePolicySpec struct {
+	// DigestMappingRules holds the registry mirroring rules that are written to


Do we want to document this as a general-purpose facility, or specifically restricted to OpenShift product images? It works just the same for both for unauthenticated registries, but we can only supply mirror credentials via a CRI-O-wide option (not via the CRI). Or maybe we don’t need to restrict this to OpenShift only, but say that non-OpenShift repositories that require credentials are {problematic,not recommended,not supported}.

abhinavdahiya · 2019-06-19T23:37:13Z

config/v1/types_image_content_source_policy.go

+	// DigestMappingRules holds the registry mirroring rules that are written to
+	// /etc/containers/registries.conf to be used by the runtime to determine which
+	// repo to fall back on, if the original repo cannot be accessed.
+	// Will also be used for offline installs/upgrades.


I think this should be changed to for example,

adambkaplan · 2019-06-20T13:21:02Z

Will the openshift apiserver need to consume this for image pullthrough?
Similarly will builds and imagestreams need to consume this for image references?

fyi @dmage

dmage · 2019-06-20T13:30:33Z

How will it be coordinated with

api/image/v1/types.go

Lines 241 to 253 in a77179b

    
           // TagReferencePolicyType describes how pull-specs for images in an image stream tag are generated when 
        
           // image change triggers are fired. 
        
           type TagReferencePolicyType string 
        
           const ( 
        
           	// SourceTagReferencePolicy indicates the image's original location should be used when the image stream tag 
        
           	// is resolved into other resources (builds and deployment configurations). 
        
           	SourceTagReferencePolicy TagReferencePolicyType = "Source" 
        
           	// LocalTagReferencePolicy indicates the image should prefer to pull via the local integrated registry, 
        
           	// falling back to the remote location if the integrated registry has not been configured. The reference will 
        
           	// use the internal DNS name or registry service IP. 
        
           	LocalTagReferencePolicy TagReferencePolicyType = "Local" 
        
           )

?

umohnani8 · 2019-06-20T13:36:10Z

@abhinavdahiya @smarterclayton moved it to operator/v1alpha1

abhinavdahiya · 2019-06-20T16:56:08Z

/lgtm

smarterclayton · 2019-06-20T17:30:42Z

operator/v1alpha1/types_image_content_source_policy.go

+	// We only support pulling by digests for mirrors, hence the name.
+	// Will also be used in cases such as offline installs/upgrades.
+	// +optional
+	DigestMappingRules DigestMappingRules `json:"digestMappingRules"`


Other possible names repositoryDigestMappings

imageDigestMappings. imageDigestRetrieval.

(To me as a non-native speaker), all of these names seem to say that digests are mapped, but they are not; digests are the only part of the image reference that don’t change.

mirroredRepositories, mirrorSets? OTOH to ensure that the digest restriction is not overlooked, that could end up with mirrorredRepositoriesInDigestReferences or something similarly horrible.

Changed it to MirrorReposForDigests

For digests is probably not accurate - you're not mirroring for digests, your mirroring digests by mirrors. We generally do not use short names inside api objects (reference instead of ref, for example).

(I don’t really want to restart the naming discussion for this field, the current name is acceptable and I don’t think we’ll get a perfect one, just to minimize the use of this figure of speech:

We are never “mirroring digests”, we may not be mirroring [complete] repositories, we are mirroring OpenShift releases = sets of individual container images.

smarterclayton · 2019-06-20T17:31:06Z

operator/v1alpha1/types_image_content_source_policy.go

+
+// DigestMappingRules holds cluster-wide information about how to handle mirros in the registries config.
+type DigestMappingRules struct {
+	// Sources are registries and/or repositories depending on the container runtime


Can we/should we document that these must be repositories (a.com/b/c) to start with, before we support wildcards in the future?

In the current implementation, there are no wildcards. There are hierarchical namespaces, i.e. you can say example.com to match example.com/*/…/*, or you can say example.com/mirror-of-red-hat to match example.com/mirror-of-red-hat/*/…/* including example.com/mirror-of-redhat/openshift.

There isn’t a way to express example.com/*/mirror-of-red-hat, and I hope we won’t need that. Do we need that?

I mean, I don't even want to support example.com from this API for now. So I would prefer to explicitly exclude that until we need it.

smarterclayton · 2019-06-20T17:43:17Z

operator/v1alpha1/types_image_content_source_policy.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// ImageContentSourcePolicy holds cluster-wide information about how to handle registry mirror rules.  The canonical name is `cluster`
+type ImageContentSourcePolicy struct {


Any other possible names we could use? Policy is fairly common. Is content source the best name? I don't want to be specific about "mirroring" or "disconnected", so trying out a name that implies "you can change where we source images" was the initial argument.

Maybe future might be "you have to have signatures from X for Y"

Just ImageContentSource or ImageSources?

Changed it to ImageContentSource

By "policy is fairly common" I meant "It's good to have Policy in there to distinguish this from a more concrete object". I don't think you have to change the name, just want to see some suggestions about what names could be that better describe what this object does.

changed the name back

You’re right that “content source” is not a commonly used term for this. I’d just say one of “image {repositories,locations,(servers,storage?)}”; the “content” word adds nothing.

Per #354 (comment):

Do we want to document this as a general-purpose facility, or specifically restricted to OpenShift product images?

do we want this to be generic for all kinds of images, or actually say something entirely specific to the contemplated use, in the ballpark of OpenShiftReleasedImageLocations?

I think making it generic will allow us to reuse it for other use cases in the future.

The resource group usually contains the product namespace (operator.openshift.io) so it is redundant in the title.

umohnani8 · 2019-06-21T13:54:57Z

@abhinavdahiya @smarterclayton fixed up based on reviews, PTAL

abhinavdahiya · 2019-06-21T18:22:55Z

/lgtm

umohnani8 · 2019-06-24T18:26:38Z

@smarterclayton @abhinavdahiya comments addressed, PTAL.

operator/v1alpha1/types_image_content_source_policy.go

abhinavdahiya · 2019-06-24T18:45:25Z

@smarterclayton @abhinavdahiya comments addressed, PTAL.

/lgtm :)

operator/v1alpha1/types_image_content_source_policy.go

mrunalp · 2019-06-24T19:46:49Z

operator/v1alpha1/types_image_content_source_policy.go

+type ImageContentSourcePolicySpec struct {
+	// repositoryDigestMirrors allows images referneced by image digests in pods to be
+	// pulled from alternative mirrored repository locations. The image pull specification
+	// provided to the pod will be compare to the source locations described in RepositoryDigestMirrors


s/compare/compared/

operator/v1alpha1/types_image_content_source_policy.go

umohnani8 · 2019-06-25T14:43:56Z

@abhinavdahiya @smarterclayton @mrunalp addressed the comments. If we are good with this, can we get it in.

smarterclayton · 2019-06-25T15:14:38Z

operator/v1alpha1/types_image_content_source_policy.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// ImageContentSourcePolicy holds cluster-wide information about how to handle registry mirror rules.
+// The canonical name is `cluster`


There should be no canonical name for this - clients may create multiple of these, it is not a singleton. We want individual components to be able to register their policies and manage them independently (trying to join a bunch of policies in a doc is very difficult to do with decelerate config models). That means that we have to define how merging happens, which should be done in the godoc for each field (repositoryDigestMirrors)

Please change the doc to "When multiple policies are defined, the outcome of the behavior is defined on each field". For the repositoryDigestMirrors field, add a sentence defining how this will work.

For instance, if I have

sources: - a - b - c

in one object (provided by the installer) and

sources: - c - d - e

in a second (provided by an operator)

we need to define how they are merged (probably by a zip merge with duplicates removed - a c b d e, although we can be loose about the ordering).

Keep in mind the use cases we're trying to target when you consider merging:

operator on cluster potentially inserts the local registry as a targeted mirror (which probably means it wants to go first)

OLM in the future may manage these objects, so it'll be adding them post-install

Merged as in when we write these mirror rules to /etc/containers/registries.conf?

Merged as when they are written and interpreted by all consumers (whether it's registries.conf today, or a future image controller use case like image import). Documenting the rules of merging makes it clear to consumers and helps clarify the design of components

As a counter, does it make sense to disallow having common repos between each config object? That way the administrator can be explicit in how they want the repos ordered.

I am thinking of a case where one user of the feature creates an ordering and then a second user comes up with an ordering that has the elements reversed. It might be better to just disallow so they can communicate with first user and come up with a commonly agreed ordering for the repos.

I find it a bit attractive to make all of this undefined (explicitly, if necessary).

Let the user declaratively define that some set of repositories is equivalent; let the cluster worry about checking whether they are up, ordering them, possibly load balancing between them, … . Sure, we do nothing of that kind now, but if we define the mirror set to have some kind of strict ordering and strict merging semantics, we’ll lose the opportunity to do something smart in the future (without an explicit opt-in, at least).

I’m especially dubious about the idea of transitively merging mirror sets; sure, it’s trivial to implement, but intuitively it seems to me that if something creates two mirror sets that have a common element but are otherwise distinct (neither a subset of the other), it’s more likely than not that something got very confused and the mirrors are not actually operated to be transitively equivalent that way.

Or is the idea that the OpenShift product will ship, and manage, a CRD object that claims the equivalence of r.a.rh.com/foo with r.rh.io/bar (and OpenShift the organization will commit to keeping the repositories in sync), while the local administrator will set up, and manage, the equivalence of r.a.rh.com/$foo with local-mirror.example.com/baz (and commit to keeping the two repositories in sync), and therefore we need merging to exist because we don’t want the administrator to edit the OpenShift-product-managed object?

If that is the case, does the administrator need to be able to say “only ever use my mirror without reaching to the internet at all”? That can only work if we define the order of accesses both within a mirror set, and between all the CRD objects (how? based on their name?). (And without an explicit “don’t use those mirrors at all”, the administrator can only put them at the end of the list, to be never contacted unless the local mirror is down. That seems good enough for me, at least in the initial version of this object; registries.conf can express “redirect all references to foo to bar without ever contacting foo”, but we’ll have enough trouble defining the semantics of merging of the ordering without having to worry about three CRD objects, setting up strict redirects of A→B, B→C, B→A.)

(Or, sure, we could leave the internet-accessible mirrors in and accept them being in the first locations, and just let pulls fail because the DNS or firewall rejects the connection attempts. It could easily be fast enough, but it will cause noise on the firewalls.)

(FWIW openshift/machine-config-operator#805 currently sets up the mirrors in registries.conf in a rotated fashion, so that different pull specs will result in different order of attempts. That’s just an initial guess, and there’s nothing at all to say that it matters because the pull specs from the different mirrors should not really exist in the cluster at the same time, it can of course be changed to any other semantics.)

Or is the idea that the OpenShift product will ship, and manage, a CRD object that claims the equivalence of r.a.rh.com/foo with r.rh.io/bar (and OpenShift the organization will commit to keeping the repositories in sync),

this is the most likely near term. The most likely case will be r.rh.io/* to registry.internal.svc/x/y for digest mirroring of core public resources.

I’m especially dubious about the idea of transitively merging mirror sets; sure, it’s trivial to implement, but intuitively it seems to me that if something creates two mirror sets that have a common element but are otherwise distinct (neither a subset of the other), it’s more likely than not that something got very confused and the mirrors are not actually operated to be transitively equivalent that way.

Since we're pulling by digest the only expense is check time (which agree can be left as weakly undefined behavior). If you say A and B are candidates for pull by digest and B and C are candidates by digest and A and B are down I would prefer to check C (this feature is about availability first, performance / local pulls second).

I am not opposed to leaving it weakly undefined, but it needs to satisfy the intent.

As far as order, I would prefer that we preserve preferential rather than round robin, the requirements for this feature are to let you preferentially use a local proxy for performance, and round robin does not satisfy that.

Adding a new operator/v1alpha1 definition caled ImageContentSource which will be used when using a CR to set the registry mirror rules for resiliency and offline installs/upgrades. A new CRD will be created to encompass this. Signed-off-by: Urvashi Mohnani <[email protected]>

umohnani8 · 2019-06-26T19:05:21Z

@abhinavdahiya @smarterclayton can we please get this in so we can continue the work that is relying on this. We can figure out the merging criteria later if that is okay? It would just be a PR to update the comments here.

smarterclayton · 2019-06-26T20:09:39Z

operator/v1alpha1/types_image_content_source_policy.go

+	// When multiple policies are defined, any overlaps found will be merged together when the mirror
+	// rules are written to `/etc/containers/registries.conf`. For example, if policy A has sources `a, b, c`
+	// and policy B has sources `c, d, e`. Then the mirror rule written to `registries.conf` will be `a, b, c, d, e`
+	// where the duplicate `c` is removed.


This comment is fine for now, I'm ok with discussing the policy in a follow up before 4.2 GA but after this feature has been prototyped.

smarterclayton · 2019-06-26T20:10:38Z

Sufficient for first round of prototyping (with the known topics still to be resolved for 4.2)

/lgtm
/approve

openshift-ci-robot · 2019-06-26T20:10:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, smarterclayton, umohnani8

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~~ [smarterclayton]

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

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 17, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan June 17, 2019 18:46

umohnani8 mentioned this pull request Jun 17, 2019

Add ImageContentSourcePolicy CRD manifest openshift/cluster-config-operator#67

Merged

mrunalp reviewed Jun 18, 2019

View reviewed changes

mtrmac reviewed Jun 19, 2019

View reviewed changes

abhinavdahiya reviewed Jun 19, 2019

View reviewed changes

umohnani8 force-pushed the mirrors branch from c712102 to e6b4380 Compare June 20, 2019 13:34

umohnani8 changed the title ~~Add ImageContentSourcePolicy to config/v1~~ Add ImageContentSourcePolicy to operator/v1alpha1 Jun 20, 2019

umohnani8 force-pushed the mirrors branch from e6b4380 to 7d5a243 Compare June 20, 2019 13:58

openshift-ci-robot assigned abhinavdahiya Jun 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2019

smarterclayton reviewed Jun 20, 2019

View reviewed changes

umohnani8 force-pushed the mirrors branch from 7d5a243 to 7fb0ddb Compare June 21, 2019 13:51

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2019

umohnani8 force-pushed the mirrors branch from 7fb0ddb to c266301 Compare June 21, 2019 13:53

umohnani8 force-pushed the mirrors branch from 69d79ba to f164cae Compare June 24, 2019 18:26

abhinavdahiya reviewed Jun 24, 2019

View reviewed changes

operator/v1alpha1/types_image_content_source_policy.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Jun 24, 2019

View reviewed changes

operator/v1alpha1/types_image_content_source_policy.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Jun 24, 2019

View reviewed changes

operator/v1alpha1/types_image_content_source_policy.go Outdated Show resolved Hide resolved

umohnani8 force-pushed the mirrors branch from f164cae to 64594b2 Compare June 24, 2019 19:09

mrunalp reviewed Jun 24, 2019

View reviewed changes

operator/v1alpha1/types_image_content_source_policy.go Outdated Show resolved Hide resolved

mrunalp reviewed Jun 24, 2019

View reviewed changes

operator/v1alpha1/types_image_content_source_policy.go Outdated Show resolved Hide resolved

umohnani8 force-pushed the mirrors branch from 64594b2 to 6ea93c5 Compare June 24, 2019 19:55

smarterclayton reviewed Jun 25, 2019

View reviewed changes

umohnani8 force-pushed the mirrors branch from 6ea93c5 to 36cf2f0 Compare June 25, 2019 18:04

smarterclayton reviewed Jun 26, 2019

View reviewed changes

openshift-ci-robot assigned smarterclayton Jun 26, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019

openshift-merge-robot merged commit da8f874 into openshift:master Jun 26, 2019

This was referenced Jun 27, 2019

Add ImageContentSourcePolicy to register.go #356

Merged

Update client-go to include ImageContentSourcePolicy api openshift/client-go#109

Merged

cgwalters mentioned this pull request Jul 3, 2019

Mirrors support openshift/machine-config-operator#805

Merged

This was referenced Jul 8, 2019

Update dependencies openshift/machine-config-operator#936

Merged

Validate ImageContentSourcePolicy openshift/machine-config-operator#949

Closed

mtrmac mentioned this pull request Jul 17, 2019

Modify ImageContentSourcePolicy to distinguish between sources (pull specs) and mirrors #384

Merged

coderabbitai bot mentioned this pull request Dec 17, 2025

NO-JIRA: Rename ImagePolicy nested types to prevent PKI collision #2626

Merged

Add ImageContentSourcePolicy to operator/v1alpha1 #354

Add ImageContentSourcePolicy to operator/v1alpha1 #354

Uh oh!

Conversation

umohnani8 commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umohnani8 commented Jun 17, 2019

Uh oh!

abhinavdahiya commented Jun 18, 2019

Uh oh!

umohnani8 commented Jun 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Jun 19, 2019

Uh oh!

smarterclayton commented Jun 19, 2019 via email

Uh oh!

abhinavdahiya commented Jun 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Jun 19, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented Jun 20, 2019

Uh oh!

dmage commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umohnani8 commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhinavdahiya commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Jun 20, 2019 • 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!

umohnani8 Jun 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

umohnani8 commented Jun 17, 2019 •

edited

Loading

abhinavdahiya Jun 19, 2019 •

edited

Loading

dmage commented Jun 20, 2019 •

edited

Loading

umohnani8 commented Jun 20, 2019 •

edited

Loading

mtrmac Jun 20, 2019 •

edited

Loading

umohnani8 Jun 21, 2019 •

edited

Loading

smarterclayton Jun 25, 2019 •

edited

Loading

umohnani8 Jun 25, 2019 •

edited

Loading

mrunalp Jun 25, 2019 •

edited

Loading