Add repositoryMirrors spec to ImageContentSourcePolicy

[QiWang19]

Request for enhancement: https://issues.redhat.com/browse/RFE-1608
Epic: https://issues.redhat.com/browse/OCPNODE-521
Ref: openshift/api#636

Registry mirror set by ImageContentSourcePolicy only will be used if the image
is referenced by digest because the mirror-by-digest of /etc/containers/registries.conf is set to true.
This causes issue since there are use cases where no digests are available.

In ImageContentSourcePolicy, add repositoryMirrors spec, so user can
easily configure it in the same spot they set post-installation mirror configuration.

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

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign adambkaplan after the PR has been reviewed.
You can assign the PR to them by writing /assign @adambkaplan in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[QiWang19]

@umohnani8 PTAL

[dmesser]

@QiWang19 Another impactful use case outside of disconnected environments is the use of registries that act as transparent pull-through proxy cache for upstream registries. Especially in light of some public registries introducing severe pull rate limits.

[wking]

Users with the new configuration who downgrade to a version that lacks support for the new feature will presumably have their CRI-O configurations clobbered and break their tag-mirroring-dependent workflows.

[wking]

Probably worth saying something about the age and stability of the mirror-by-digest-only CRI-O configuration property, to demonstrate CRI-O with n-2 OpenShift skew will still be able to handle the new property. Or maybe it's just the existing property being removed, in which case it should be even easier to make that case.

[wking]

Seems like the body of this section should be replaced with "Not applicable, because we are not deprecating any features." or similar

[wking]

Looks like you have a lot of boilerplate template content in this section. If you're going to be born GA, probably just say that?

[wking]

This and later in this section look like template boilerplate that can be dropped.

[QiWang19]

/assign @adambkaplan

[QiWang19]

@wking PTAL

[QiWang19]

@mrunalp PTAL

[wking]

Defaulting to the unsafe false is riskier than I'd like. If this is a true boolean in registries.conf (I don't know), I'd prefer AllowTags or some such here to default safe.

[adambkaplan]

Isn't mirror by digest the default behavior today? If that is the case then then new API needs to be opt-in, as suggested with AllowMirrorByTags

[QiWang19]

These lines of API definition is current registries.conf. Not the ImageCententSourcePolicy that this enhancement try to implement.

[QiWang19]

Isn't mirror by digest the default behavior today? If that is the case then then new API needs to be opt-in, as suggested with AllowMirrorByTags

I have updated this enhancement to add a new spec of repositoryMirrros per comment #690 (comment)

[wking]

With a property for digest-ness under repositoryMirrors, maybe time to go v1alpha2 and drop repositoryDigestMirrors?

[QiWang19]

If the repositoryDigestMirrors dropped, the digest requirement will not exist. Is it possible the customer still wants to limit the use of mirrors? This looks like a very rare case, I am not sure.

[wking]

mirror-by-digest-only: true is sane, and protects you from malicious/compromised registries by forcing you to use by-digest pullspecs. But you can ask for one if those with:

apiVersion: operator.openshift.io/v1alpha2
kind: ImageContentSourcePolicy
metadata:
  name: ubi8repo
spec:
  repositoryMirrors:
  - mirrors:
    - example.io/example/ubi-minimal 
    source: registry.access.redhat.com/ubi8/ubi-minimal
    allowMirrorByTags: false # or equivalently, by leaving allowMirrorByTags unset

without needing a repositoryDigestMirrors entry.

[QiWang19]

With a property for digest-ness under repositoryMirrors, maybe time to go v1alpha2 and drop repositoryDigestMirrors?

@wking How we do the API version change and the drop for repositoryDigestMirrors? Do we add the new repositoryMirrors and drop the old one at the same time in openshift/api?

Should this enhancement mention we are going to drop the repositoryDigestMirrors?

[adambkaplan]

Isn't mirror by digest the default behavior today? If that is the case then then new API needs to be opt-in, as suggested with AllowMirrorByTags

[QiWang19]

The comment has disagreements about https://issues.redhat.com/browse/RFE-1608. Adding a new spec RepositoryMirrors to ImageContentSourcePolicySpecvs exposing a new bool field to users. What is our consensus?

[QiWang19]

@wking @adambkaplan PTAL.

[QiWang19]

@mrunalp PTAL

[QiWang19]

@wking @mrunalp @adambkaplan PTAL. can we merge this enhancement?

[wking]

nit: unbalanced backticks around the second mirror-by-digest-only.

[mrunalp]

/lgtm
/approve

We can fix the nits in follow-on PRs.

[openshift-ci]

[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

[smarterclayton]

Why is this necessary? If I want digest only mirroring, I would already just use RepositoryDigestMirrors. Why would I ever use RepositoryMirrors with this false?

[smarterclayton]

Another reason this is a problematic design, the order of precedence for digest mirrors and tag mirrors provides wildly different behavior. We specifically specced RepositoryDigestMirrors to provide ordering, including putting source at the lowest priority. That does not work for tag users - for a tag user, source is generally prefered. So already this is missing one of the key distinctions between a desgin focused on ensuring users get the right content to a "users may get the wrong behavior".

So I probably don't think that these should be represenetd in a single struct.

[wking]

If I want digest only mirroring, I would already just use RepositoryDigestMirrors.

Having one set of mirrors where both tags and digests are allowed and another set where only digests are allowed seemed redundant, so I'd suggested (or maybe Miloslav did 🤷) the new slice allow for both. Argument about different priorities sounds... like it needs more detail. But say you prefer registry B over registry A, because B is closer. But A is more canonical, so is likely to have more current tags. Hopefully the latency in tag-mirroring from A -> B isn't too bad, but it's possible there's significant delay because it requires manual steps. However, if you're comfortable falling back to B's tag, when you can't reach A, you risk rollbacks like:

1. v1 content pushed to some-tag on A.
2. v1 content mirrored to some-tag on B.
3. v2 content pushed to some-tag on A.
4. Cluster can reach A, pulls v2 content from some-tag.
5. Workload drained off the node; rescheduled elsewhere. Or drained and uses Always pull policy, or whatever.
6. Cluster tries to reach A, but network hiccup, so it fails.
7. Cluster falls back to B, pulls v1 content from some-tag.
8. Workload mysteriously regresses from v2 to v1.

Now, if you have multiple mirrors B, C, ..., there's no reason that B would always have newer content than C. And maybe you have flows where v2 has bin mirrored to B, but then someone rolls back to v1 for the tag on A. It's all a big mess that's hard to reason about generically (by-digest pullspecs for the win). Is the argument for by-tag-specific mirror ordering that "no generic reason, but admin Alice knows that for her cluster there will be no rollbacks on A, and B will always lag, but never lead, in promotion for all tags covered by this mirroring stanza"?

[mtrmac]

I think the general assumption for allowing tag references in mirrors is that this would just never happen; tags must be immutable (e.g. v1.2.3, as opposed to a floating latest or v1).

I don’t see that there’s any way to achieve consistency if we assume all of:

1. Workloads are defined by tags and tags are looked up by individual nodes, not resolved into digests at a single authoritative point (e.g. when a Deployment creates a ReplicaSet)
2. The same tag can point to different images over time
3. Different mirrors of the same repo can get out of sync about what image the tag points to
4. Mirrors can become unreachable over time.

3. and 4. are AFAICS always true in practice, 1. seems to be true as well (I can’t find a single non-test reference to Image in openshift/kubernetes/pkg/controller, at least); so the only “give” is 2. [Actually, 3. and 4. is not even relevant, 2. is sufficient even for non-mirrored single-registry clusters: if a Deployment refers to a tag, and the tag changes in the middle of a rollout, half of the replicas is going to be on v1 and half on v2, isn’t it?]

I don’t see that worrying about the priority of tag resolution between mirrors helps. Tags must not move, and if they do, it’s (given current workload controllers AFAICS), a user error that can’t be fixed by mirror priority,

[smarterclayton]

If a source is already defined with RepositoryDigestMirrors, by definition it ignores tags. So this section implies "we will be mirroring tags", since digests are already covered.

[mtrmac]

I think the overall idea was to replace v1alpha1.RepositoryDigestMirrors with v1.RepositoryMirrors which includes the allowMirrorByTags boolean; RepositoryMirrors was not intended to define an exclusive tag-only mirror configuration.

Originally the CR had a RepositoryDigestMirrors with a allowMirrorByTags boolean, which is a confusing naming, so I proposed renaming to just RepositoryMirrors which fits both.

---

Not having the allowMirrorByTags boolean, and instead defining separate RepositoryDigestMirrors and RepositoryTagMirrors is certainly an option. A difference is that with two separate arrays, it’s not possible to create a CR that says “Here’s a mirror, use it, I am not defining a tag/digest policy for how it’s used” (which I imagine a mirror-maintenance/replication operator might want to do, but that’s pure speculation).

[smarterclayton]

This isn't a very strong argument, changing behavior without understanding the difference in behavior is a bug.

So I would probably put a hold on this feature with this as is without stronger justification.

[smarterclayton]

In a single version, during an upgrade, both new and old api versions should be ready and transformed into crio config. After a release has gone by, the old version can be removed. The flow described here doesn't meet our historical guarantees for upgrade.

[smarterclayton]

In fact, the better approach is to simply read the old objects as long as their are available, then read new objects, and the merge order is that the behavior provided by the old object is honored as long as old objects are described.

[smarterclayton]

Specifically "source being last" only makes sense for digest, it DOES not make sense for tag in all use cases. Tag is fundamentally dependent on hoping you get the most up to date version of a tag during upgrade scenarios.

Both image stream import and crio can use repository digest order consistently. However, I'm not sure without an understanding of image import tag mirroring what we do here.

Is there another enhancement that covers ImageStream import behavior with repository tag design? We should not be introducing a V1 api without a full design covering how import will interpret this, because mirroring of tags may actually need these fields to be designed. If there is no additional design, I'd like to see one before I withdraw my hold on the implementation.

[smarterclayton]

Expect to see a section here describing how to compose RepositoryMirrors of type digest and non-digest. Also, expect to see a defined order of composition for order (so that different nodes have consistent orders).

Finally, there is a missing section here on the security implications of repository tag mirroring - tag mirroring allows anyone who can craft a source ICSP to hijack all workloads, which existing digest mirroring does not. So right there that may point to the need to define a separate object with clearly differentiated security characteristics.

Digest mirroring has some attack vectors (for instance, someone could define a bunch of useless mirrors that return error continuously), but tag mirroring could allow escalation.

[smarterclayton]

I would probably recommend defining composition of mirrors in name order (ICSP A mirrors go before ICSP B mirrors)

[mtrmac]

The v1alpha1 behavior is, when composing mirrors for the same repo, to honor the relative ordering if possible (and a stable, but unspecified, handling of ordering loops).

E.g. ICSP1: source -> A,B; ICSP2: source -> B,C is defined to compose to A,B,C, in that order.