-
Notifications
You must be signed in to change notification settings - Fork 531
Improve oc awareness of ImageContentSourcePolicy #334
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.
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
Conversation
| ## Summary | ||
|
|
||
| ICSP allows OpenShift (CVO, CRI-O) to check down a list of possible mirrors to find an image with the matching digest it is | ||
| looking for. `oc` should do the same. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oc seems to use hard-coded image references, for example oc adm must-gather just tries to pull quay.io/openshift/origin-must-gather:latest. How will oc use ICSP to find right image? oc's going to have to know the pullspec for the release and then do whatever oc adm release info --image-for=must-gather would do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must gather should use the tag in the openshift image stream (it should be like debug, which tries to look up openshift/tools:latest and then falls back to a hardcoded constant).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense but since must-gather is in the release image, can't that be added as a fallback option?
We don't deploy the samples operator on baremetal by default, and even if we did and it failed, I'd still expect must-gather to work in the disconnected case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For disconnected oc adm must-gather I believe you have to pass the
oc adm must-gather --image=disco-repo/release:4.5.0-0.ci-2020-05-26-081827-must-gather flag, otherwise it will try to launch a pod using the imagestream from -n openshift and that still has reference to the quay.io registry from which the release image was mirrored. When that fails must-gather will then try the hard-coded origin-must-gather:latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but as an end user I'd just expect oc adm must-gather to work in a disconnected environment. Specifying the image isn't very intuitive.... we do it like this: https://github.com/openshift-metal3/dev-scripts/blob/master/must_gather.sh#L19-L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stbenjam must-gather will do the following:
- resolve images passed through --image flag
- resolve images streams passed through --image-stream flag
- resolve must-gather:latest IStag from openshift ns
all of the above built the set of plugins/images that are being run during must-gather. Only if the last step fails we will resolve to quay.io image.
see https://github.com/openshift/oc/blob/f30826ef04e45eea1745276dee8e693a08e29dba/pkg/cli/admin/mustgather/mustgather.go#L141-L158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stbenjam similar logic applies to oc debug it will look for tools:latest in openshift ns, and fallback to registry only if none is found.
|
This also applies to a bunch of other commands, can we enumerate all the things that |
| * Add logic to `oc adm release` and/or openshift/library-go/pkg/image to become aware of ICSP in cluster | ||
| * Add logic to `oc adm release` and/or openshift/library-go/pkg/image to use the ICSP to complete extracts, mirroring, must-gather | ||
|
|
||
| ### User Stories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks, i'll incorporate debug as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for oc debug, in disconnected, is it expected that you'd pass --image myrepo/tools:mytag to debug command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug has similar flow like must-gather see my earlier comment.
| retain references to the mirrored registry, usually something like | ||
| `quay.io/openshift-release-dev/ocp-v4.0-art-dev`. | ||
|
|
||
| There needs to be logic in `oc` to look for `ImageContentSourcePolicy` either from a file or from a cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a file is required, although might be nice.
All oc image and oc adm release commands need to support this. However, oc image commands are not allowed to talk to API servers (they are commands that are cluster agnostic).
So this solution needs to implement something that works for oc image without the logic described about looking up ICSP from server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any flags added for oc image mirror can we also add to oc adm catalog mirror?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll add any function across the board for all oc adm cmds to handle ICS information.
The file will be created during the mirror, so oc can then use the info from the ICSP file for other cmds, if it exists, like so: openshift/oc#439 that the oc adm catalog mirror already writes.
|
|
||
| ## Proposal | ||
|
|
||
| * Add logic to `oc adm release` and/or openshift/library-go/pkg/image to become aware of ICSP file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to add a third option and discuss tradeoffs:
oc adm release infono extra arguments - IF a cluster is available, TRY to find ICSP and if you can't assume none are possibleoc adm release info --icsp-file XwhereXis a file on disk that has image content policy source options in it (note we don't do anything like this anywhere else, so this is wierd)oc adm release info --alternate-repository XwhereXis an image repository that is searched (like ICSP would be) for any image looked up
The first one is fine (as long as the error logic is taken into account). Given a choice between the second and third one, there are times I want both. As a user of both commands, the third option is simpler and works in more scenarios - I don't need to craft a YAML, I just say
oc image info quay.io/openshift-release-dev/ocp-release@sha256:abcdef123456 --alternate-repository myregistry.com/repo/mirror
would lookup the digest in the first link and then fall back to the second. So that's simpler for someone who is used to dealing with sources and destinations.
The counter argument is that for an admin doing this they will be working with ICSP. So the correct behavior is to do oc get imagecontentsourcepolicy -o yaml > /foo and then oc adm release info --image-content /foo. I kind of like this flow for oc adm release info, and i think it's probably ok for oc image info too.
Given that, can we define the behavior of 1 and 2 of flags better and then talk about error fallback on 1 (i.e. we can't fail if no ICSP exists).
I can't think of another command that cares about ICSP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i'll add that to the proposal - option 3 similar to my --force-prefix PR openshift/oc#427 it is simpler because a user knows exactly what they want, they don't need oc to go down a list. They know they have the images in myrepo/myrelease:tag and so they should be able to tell oc to only look there.
The oc get imagesourcecontentpolicy -o yaml only works if you have a cluster. We need to give disconnected users who don't have the cluster yet the ability to extract from a mirrored release - right now, a mirrored release still has the reference to quay.io/openshift-release-dev/ocp-release this is what prohibits oc adm release extract from working in disconnected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this proposal, to reflect my opinion that we need the following:
- A user can specify an ICSP file - if set, oc will use that.
- oc will look for/try to use ICSP from cluster
- oc will try user-given registry/repo/release image
- if those don't succeed, oc will use image-reference
oc adm release mirrorwill write icsp.yaml file to either --icsp-to path (or whatever flag we decide on) or local directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why we can't support both no 2 and 3 from Clayton's list here. The overlap, so the underlying functionality will be the same, the reading from ICSP or from flag will be the only difference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With so many flags already for the oc adm release commands, I was thinking the
--alternative-repository flag could be avoided. I will add it, though, if we need it (and since @smarterclayton suggested it, he's probably thinking we do). I am thinking a user would give their repository and expect to either run the cmd against that exact repository, or from the image reference, or from the ICSP data. These are already there, without the extra flag (in this PR).
Example:
oc adm release extract --tools quay.io/sallyom/test-release:4.5.0-0.nightly-2020-06-22-125424 -v=2 will extract from quay.io/sallyom/test-release if built w/ this PR: - this fails from master unless you have the credentials for
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:d960fc054c342.... since that is where the quay.io/sallyom image is mirrored from. (w v=2 you can see where extracted from)
I don't think a user would run
oc adm release extract --tools quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:d960fc05... --alternative-repository quay.io/sallyom/test-release that seems like it's not necessary to me, but I'll add it if we want it.
873900d to
9c6ad68
Compare
3b9364e to
5586da4
Compare
| the following commands do not succeed when in a disconnected environment: | ||
|
|
||
| ```console | ||
| $ oc adm release extract --tools aprivateregistry/repo/name:tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: aprivateregistry -> example.com or registry.example.com or some such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to registry.example.com (all instances of aprivateregistry)
| 3. `oc adm release mirror --icsp-to /path/to/file aregistry/arepo/release:tag --to anotherreg/arepo/release` | ||
| * `oc` will write an ICSP file to the given path. If no --icsp-to flag, `oc` will write the ICSP file to the current directory. | ||
|
|
||
| ## Alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no guarantee that the local oc can access the mirrors configured by the cluster's ICSP. For example, maybe the host where oc is running doesn't have whatever proxy config the cluster nodes are using to reach a mirror registry. Or maybe there is a completely different DNS config so whatever.example.com resolves to different registries for the cluster and oc. For must-gather, we could work around that by having an in-cluster service that spit out the extracted images (e.g. by populating a well-known ImageStream with tags extracted from the current release image target). I thought we had that ImageStream already, and am not clear on why it is not being populated (or is being populated but not used?) in the restricted-network case.
| ```console | ||
| $ oc adm release extract --tools registry.example.com/repo/name:tag | ||
| $ oc adm release mirror registry.example.com/repo/name:tag --to someregistry/repo/name | ||
| $ oc adm must-gather |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, although it requires certain Image Streams to exist, so that's not a problem with functionality but with missing docs, tbh.
|
|
||
| ## Proposal | ||
|
|
||
| * Add logic to `oc adm release` and/or openshift/library-go/pkg/image to become aware of ICSP file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why we can't support both no 2 and 3 from Clayton's list here. The overlap, so the underlying functionality will be the same, the reading from ICSP or from flag will be the only difference here.
| 3. (The current flow): If the above fall through, try the image-reference from user-given image - use it - may or may not succeed | ||
| if disconnected or not authorized to access image-reference registry | ||
| * FLAGS: | ||
| * *--icsp-to* will define where to write an ICSP file to. If unset, `oc adm release mirror` will write to current directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we just write the iscp when --to-dir is specified or just print it like we already do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ICSP isn't formed if mirroring to a directory. To get the ICSP, mirror from 1 registry to another:
oc adm release mirror someregistry/repo/release:tag --to anotherregistry/repo/release
When mirroring to a local directory, ICSP isn't created.
The reason for saving to file is so a user has the copy, so they can apply to a cluster if they lose that stdout. Or, they can pass --icsp-file=path to oc adm release extract.
| `oc adm release extract --command oc myreg:5000/myrepo/release:tag --image-content-source myreg:5000/myrepo/release`. | ||
| * Instead, a boolean `--set-prefix` would allow a user to specify "I want to use the prefix of the release image I have specified, | ||
| rather than any underlying image reference." | ||
| * This proposal is for the `FLAGS`, that `oc` will try ICSP, user-given image, then fall back on underlying image-reference from user-given image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think explicit flags, w/o too much hidden logic what-if. Iow:
- read ICSP from cluster, none available - fail with suggestion
- read from file explicitly passed through flag
- use alternate repository explicitly passed through flag
This should ensure we address Clayton's comments.
| ## User Stories | ||
|
|
||
| A user runs: | ||
| 1. `oc adm must-gather` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave must-gather out of ICSP, for starters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed must-gather use-case
| 2. `oc adm release extract --tools registry.example.com/repo/release:tag` | ||
| * `oc` will look for ICSP and if found, will extract from the ICSP mirror (aregistry/arepo/release:tag-toolsha) rather than from the image reference that the user will not | ||
| have access to if in disconnected environment. If ICSP not found, will extract from the user-given registry.example.com/repo/tool:digest. | ||
| If neither of these succeed, will proceed to use the image-reference from the user-given image. The extract will succeed if user has access and permission to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure which fallback this covers, you mentioned in the previous sentence that if ISCP is not found we'll extract from user-given image. So I'm a bit confused with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if a user has mirrored an image from quay.io/openshift-release-dev/ocp-release:tag to repo/release/ocp:tag then w/out this PR, when you extract from repo/release/ocp:tag oc adm release extract only extracts from the original, quay.io/openshift-release-dev/ocp-release:tag.
With this PR, result of a user running:
$ oc adm release extract --tools repo/release/ocp:tag will be:
- Try to extract from ICSP file or ICSP from connected cluster
- Try to extract from user-given release (
repo/release/ocp:tag) - If those fail, then fall to extract from the image reference (the original image) . This may or may not succeed, depending on if you have access to that registry thru pull-secret/reg credentials.
| 2. `oc adm release extract --icsp-file /path/to/icsp.yaml --tools anightly/release:tag` | ||
| * `oc` will try to use data from the icsp file, it will extract from the ICSP mirror (aregistry/arepo/release:tag-toolsha) rather than from the image reference. | ||
| It will not try other sources, if the --icsp-file flag is provided. | ||
| 3. `oc adm release mirror --icsp-to /path/to/file aregistry/arepo/release:tag --to anotherreg/arepo/release` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... this is weird, why we'd write ISCP to an image? The current logic in release mirror just prints ISCP, iirc, why not just leaving that as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're writing ICSP to a file, so it can be used later to extract from, to apply to a cluster, etc. oc adm catalog mirror already does this.
| * `oc` will try to use data from the icsp file, it will extract from the ICSP mirror (aregistry/arepo/release:tag-toolsha) rather than from the image reference. | ||
| It will not try other sources, if the --icsp-file flag is provided. | ||
| 3. `oc adm release mirror --icsp-to /path/to/file aregistry/arepo/release:tag --to anotherreg/arepo/release` | ||
| * `oc` will write an ICSP file to the given path. If no --icsp-to flag, `oc` will write the ICSP file to the current directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current directory or if --to-dir is specified write there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
soltysh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm, but I'm leaving Clayton final saying.
0b642bc to
3e721fc
Compare
| ## Summary | ||
|
|
||
| ICSP allows OpenShift (CVO, CRI-O) to check down a list of possible mirrors to find an image with the matching digest it is | ||
| looking for. `oc` should do the same. If no ICSP found, then try user-given image. If that doesn't succeed, then use underlying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oc should do the same because users expect that when they interact with a cluster, the cluster's context informs their action (i.e. oc adm release info will lookup the current cluster). In general, letting oc default to the behavior of the connected cluster is the right thing to do when the current cluster is the target - but not when the context is unclear or may be a different target.
This may help clarify this summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarifications I'd like added to the proposal (maybe as "design points") for future reference:
occan use context of the cluster when the action the user is taking is clearly expected to talk to the cluster (oc get foo)- When the user's action is not obviously expected to talk to the cluster (
oc adm release info foo), the cli MAY attempt to load ICSP info from an existing clusters, but if that fails, the cli MUST ignore that failure (log at a high debug level) - In the case where the CLI attempts to lookup the ICSP in order to help the user, that should happen after the first attempt to retrieve content from the location fails, in which case the ICSP should be looked up and an attempt made to find the alternate location from those sources - if all of those fail, then the original error must be returned.
- In general, when a user specifies an explicit ICSP, the CLI MUST fail if that ICSP cannot be loaded, and SHOULD follow the order defined in the ICSP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
| * `oc` will try to use data from the icsp file, it will extract from the ICSP mirror (aregistry/arepo/release:tag-toolsha) rather than from the image reference. | ||
| It will not try other sources, if the --icsp-file flag is provided. | ||
| 3. `oc adm release mirror --icsp-to /path/to/file aregistry/arepo/release:tag --to anotherreg/arepo/release` | ||
| * `oc` will write an ICSP file to a configured path (same directory as the signature file), or to provided path via an --icsp-to (or similar) flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion was whether to introduce a new, more general flag to mirror. I think for now, let's just add a parallel --release-icsp flag that writes the ICSP to a specific file, and then a user should be able to apply the same file given the same inputs and have the same ICSP on cluster.
| - "@smarterclayton" | ||
| creation-date: 2020-05-19 | ||
| last-updated: 2020-07-29 | ||
| status: provisional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with those comments addressed this is ready for implementation, if you can make the updates I'll approve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated w/ your feedback.
a6beff5 to
5a28c26
Compare
|
Please squash and add a quick description in the body of the first commit comment (i.e. what the change is, why, high level summary) and then I'll tag it. |
done, thanks |
This proposal is to add logic in `oc` to look for ImageContentSourcePolicy from a cluster or from a file to be used in `oc adm release` commands. If using a mirrored image and the mirrored source registry is disconnected, `oc adm release extract|mirror` do not succeed. This is because the mirrored image tags (the individual component images from a payload) retain references to the mirrored registry, usually something like `quay.io/openshift-release-dev/ocp-v4.0-art-dev`, and `oc adm release` only looks up the image reference that a disconnected user would not have access to. Implementing this proposal will solve this issue. `oc` should look for ICSP in the cluster/current context if connected, if the user has permission to access ICSPs, and if the current flow of using the image-reference from a given image fails. When a cluster context is unclear or there's the possibility that the connected cluster could be a different target than expected, `oc` should not default to connecting to the cluster.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sallyom, smarterclayton, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @soltysh
/cc @smarterclayton
oc adm release info|mirror|extractshould attempt lookup in following order:If 1,2,3 fail, user will get the failure from 1 (same as they do currently)
Also, oc adm release mirror will write an ICSP file to a user's local env, similar to the release-image-signature-file that is written now.