Skip to content

Add repositoryMirrors spec to ImageContentSourcePolicy #690

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Jul 7, 2021
Merged

Add repositoryMirrors spec to ImageContentSourcePolicy #690

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 189 additions & 0 deletions enhancements/api-review/add-repositoryMirrors-spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
---
title: add-repositoryMirrors-spec
authors:
- "@QiWang19"
reviewers:
- TBD
approvers:
- TBD
creation-date: 2021-03-10
last-updated: 2021-03-10
status: implementable
---

# Add repositoryMirrors spec to cluster wide ImageContentSourcePolicy

## Release Signoff Checklist

- [x] Enhancement is `implementable`
- [x] Design details are appropriately documented from clear requirements
- [x] Test plan is defined
- [ ] Operational readiness criteria is defined
- [ ] Graduation criteria for dev preview, tech preview, GA
- [ ] User-facing documentation is created in [openshift-docs](https://github.com/openshift/openshift-docs/)

## Summary

Today, the ImageContentSourcePolicy object sets up mirror with `mirror-by-digest-only` property set to true. Requires images be pulled by digest only. This enhancement plans to add new spec to ImageContentSourcePolicy with configurable field `allowMirrorByTags`, so that `mirror-by-digest-only` can be configured by ImageContentSourcePolicy.

## Motivation

Today, the ImageContentSourcePolicy object sets up mirror configuration with `mirror-by-digest-only` property set to true, which leads to using mirrors only when images are referenced by digests. However, when working with disconnected environments, sometimes multiple ImageContentSourcePolicies are needed, some of them are used by apps/manifests that don't use digests when pulling the images.

Adding the `repositoryMirrors` spec to ImageContentSourcePolicy to configure mirrors will make it possible to pull images from mirror using tags without requiring image digest reference.

### Goals

- Enable the user to pull images through ImageContentSourcePolicy configured mirrors by tags.

### Non-Goals

- This proposal does not recommend using by-tag references for OpenShift release images. Those should still be referenced by digest, regardless of whether `allowMirrorByTags` is enabled for repositories where the release images are mirrored

## Proposal

The new spec `repositoryMirrors` will be added to ImageContentSourcePolicy. The `allowMirrorByTags` property will allow for mirroring by-tag images through configured mirrors.

```go
// RepositoryMirrors holds cluster-wide information about how to handle mirrors in the registries config.
// Note: this is different from the RepositoryDigestMirrors that the mirrors only
// work when pulling the images that are referenced by their digests.
type RepositoryMirrors struct {
Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mtrmac mtrmac Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

// source is the repository that users refer to, e.g. in image pull specifications.
// +required
Source string `json:"source"`
// If true, mirrors will only be used for digest pulls. Pulling images by
// tag can potentially yield different images, depending on which endpoint
// we pull from. Forcing digest-pulls for mirrors avoids that issue.
// +optional
AllowMirrorByTags bool `json:"allowMirrorByTags"`
Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@wking wking Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor

@mtrmac mtrmac Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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,

// mirrors is one or more repositories that may also contain the same images.
// The order of mirrors in this list is treated as the user's desired priority, while source
// is by default considered lower priority than all mirrors. Other cluster configuration,
Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// including (but not limited to) other repositoryMirrors objects,
// may impact the exact order mirrors are contacted in, or some mirrors may be contacted
// in parallel, so this should be considered a preference rather than a guarantee of ordering.
// +optional
Mirrors []string `json:"mirrors"`
}
```

The ImageContentSourcePolicy CRD has the cluster-wide information about handle the mirrors only when pulling the images that are referenced by their digests.
It will now watch for mirrors configured through spec `repositoryMirrors` to not require digest reference.

An example ImageContentSourcePolicy file will look like:
```yaml
apiVersion: operator.openshift.io/v1alpha1
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
```
Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mtrmac mtrmac Oct 8, 2021

Choose a reason for hiding this comment

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

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.


### User Stories

#### As a user, I would like to pull images from mirror using tag, without digests reference
The user need to define multiple ImageContentSourcePolicies, used by apps/manifests that don't use digests to pull
the images when working with disconnected environments or pulling from registries that act as transparent pull-through proxy cache.
The user can pull image without digest by configuring `repositoryMirrors` spec in the ImageContentSourcePolicy file.
And create the ImageContentSourcePolicy project. Once this is done, the images can be pulled from the mirrors without the digest referenced.

### Implementation Details/Notes/Constraints [optional]

Implementing this enhancement requires changes in:
- openshift/api
- openshift/machine-config-operator

This is an example of the ImageContentSourcePolicy file:

```yaml
apiVersion: operator.openshift.io/v1alpha1
kind: ImageContentSourcePolicy
metadata:
name: ubi8repo
spec:
repositoryMirrors:
- mirrors:
- example.io/example/ubi-minimal
source: registry.access.redhat.com/ubi8/ubi-minimal
```

The ImageContentSourcePolicy file will create a drop-in file at `/etc/containers/registries.conf` that looks like:

```toml
unqualified-search-registries = ["registry.access.redhat.com", "docker.io"]
[[registry]]
location = "registry.access.redhat.com/ubi8/"
insecure = false
blocked = false
# No mirror-by-digest-only = true configured
prefix = ""

[[registry.mirror]]
location = "example.io/example/ubi8-minimal"
insecure = false
```

### Risks and Mitigations

Pulling images from mirror registries without the digest specifications could lead to returning different image version from different registry if the image tag mapping is out of sync. But the OpenShift core required image using digests to avoid different versions won't consume this feature at all, so it is not exposed to the risks that anyone who actually uses the feature will be exposed to.

## Design Details

### Test Plan

Update the tests that are currently in the MCO to verify that `mirror-by-digest-only` not have been set when spec repositoryMirrors of the ImageContentSourcePolicy is created.

### Graduation Criteria

GA in openshift will be looked at to
determine graduation.

#### Dev Preview -> Tech Preview

- Ability to utilize the enhancement end to end
- End user documentation, relative API stability
- Sufficient test coverage
- Gather feedback from users rather than just developers
- Enumerate service level indicators (SLIs), expose SLIs as metrics
- Write symptoms-based alerts for the component(s)

#### Tech Preview -> GA

- More testing (upgrade, downgrade, scale)
- Sufficient time for feedback
- Available by default
- Backhaul SLI telemetry
- Document SLOs for the component
- Conduct load testing

#### Removing a deprecated feature

- With `repositoryMirrors` getting in, the current `repositoryDigestMirrors` can be deprecated since its functionality will also be fully satisfied by `repositoryMirrors` and we don't have to keep duplicated APIs.
Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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.

- In operator.openshift.io/v1alpha1, gently announce to the users the `repositoryDigestMirrors` will be deprecated. In operator.openshift.io/v1alpha2, the implementation of `repositoryDigestMirrors` will be removed and replaced by `repositoryMirrors`.

### Upgrade / Downgrade Strategy

Upgrades should not be affect. But the downgrade will be affected.
Users upgraded and configured the `repositoryMirrors` spec will presumably have their CRI-O configurations clobbered and break their tag-mirroring-dependent workflows after they downgrade to a version that lacks support for the `repositoryMirrors` spec.
Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@smarterclayton smarterclayton Oct 8, 2021

Choose a reason for hiding this comment

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

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.

We will not change current default behavior of `repositoryDigestMirrors`. We are just add new spec so users can configure it through the ImageContentSourcePolicy after the upgrade.

### Version Skew Strategy

Upgrade skew will not impact this feature. The MCO does not require skew check. CRI-O with n-2 OpenShift skew will still be able to handle the new property.

## Implementation History

Major milestones in the life cycle of a proposal should be tracked in `Implementation
History`.

## Drawbacks

## Alternatives

## Infrastructure Needed [optional]