enhancements/oc/mirroring-release-signatures: Propose a new enhancement #283
Conversation
wking
force-pushed
the
oc-adm-release-mirror-signatures
branch
from
225d24f to
f2a52d9
Compare
| Users mirroring a release before creating a cluster may want to [push both container images and the release image signature ConfigMap manifest to disk](#pushing-to-disk). | ||
| But they might also want to push container images directly to a mirror repository (as in [the *applying directly to the target cluster* case](#applying-directly-to-the-target-cluster)) while still pushing the release image signature ConfigMap to disk (because there is not yet a cluster into which that manifest can be pushed). | ||
| While you could address this use-case with [the *pushing to disk* flow](#pushing-to-disk) followed by an immediate `oc image mirror ...`, it is more convenient to have `oc adm release mirror ...` push to the registry directly. | ||
| Because setting `--to-dir` adjusts the image source, this enhancement extends the command with a new `--release-image-signature-to-dir` that can be used to set a configuration manifest output directory without setting a container image output directory. |
There was a problem hiding this comment.
Ref prior comment #188 (comment)
"...consistent with our naming patterns (--to-dir, --to-release)", so to adhere make this --to-release-image-signature-dir
There was a problem hiding this comment.
My pushback against naming the directory (and instead focusing on what we are pushing into the directory) was not convincing? Expanding just in case, this option is about putting release image signatures in an unclassified directory, while --to-release-image-signature-dir sounds more like putting unspecified things in a directory that is specific to release image signatures.
There was a problem hiding this comment.
I don't feel strongly about it or disagree with your pushback but rather am highlighting Clayton's comment to be sure you dodn;t overlook in this re-roll. Again, I just want to reach agreement so I can push my implementation :)
| ## Alternatives | ||
|
|
||
| Narrowly for release signatures, we have discussed serving signatures via the Cincinnati graph JSON. | ||
| However, even in that case, users would have to gather information from the wider internet (e.g. graph snapshots from Red Hat's Cincinnati service) to feed their [in-cluster Cincinnati](#in-cluster-cincinnati), so I don't see a way around something like this enhancement to facilitate that capture. |
There was a problem hiding this comment.
We do not have a section called #in-cluster-cincinnati
There was a problem hiding this comment.
s/something like this enhancement/ for something like that in this enhancement/
There was a problem hiding this comment.
We do not have a section called
#in-cluster-cincinnati
Oops. Fixed with 3cd02e0 -> 528e93d.
s/something like this enhancement/ for something like that in this enhancement/
I left this alone, because it doesn't matter which enhancement lands the feature. I just think that in-cluster Cincinnati is going to want to run something that captures information from the wider internet, and that gathering information is what this enhancement is about (although now it is scoped narrowly to gathering release image signatures).
wking
force-pushed
the
oc-adm-release-mirror-signatures
branch
2 times, most recently
from
3cd02e0 to
528e93d
Compare
|
|
||
| ### Graduation Criteria | ||
|
|
||
| This will move straight to GA, because `oc` doesn't seem to have a history of a `--tech-preview` flag or similar to allow for graduated stability of new features. |
There was a problem hiding this comment.
This seems weird and out of place. I would rephrase this to be consistent with our general statement:
This feature will be GA and will follow normal CLI policy for two releases of backwards compatibility.
There was a problem hiding this comment.
This feature will be GA and will follow normal CLI policy for two releases of backwards compatibility.
I assume this is two minor bumps, like 4.5 -> 4.7. But "falling out of GA support on the third minor bump" is distinct from "we are born in GA, because oc provides no mechanism to tech-preview new features". I've pushed 528e93d -> 367e32d with an attempt to cover both sides.
| * There's no way to implement it in [the *pushing to disk* flow](#pushing-to-disk). | ||
| * New versions of `oc` could create [custom resources][custom-resources] instead of using generic types such as ConfigMaps. | ||
| This way, configuration manifest application would generate errors when applying manifests to clusters which were too old to understand them. | ||
| But it is too late to use this approach for release image signatures, because the ConfigMap support has [already landed][cvo-config-map-signatures]. |
There was a problem hiding this comment.
We generally, in practice, accept then risk of version skew by instructing users some functions may not be available in older versions. While there are downsides, they have never risen to the level of concern to justify expensive server interaction.
There was a problem hiding this comment.
We generally, in practice, accept then risk of version skew by instructing users some functions may not be available in older versions.
This is covered above in my:
Users in that situation might be surprised that the cluster was unable to locate release image signatures, not realizing that the older cluster was ignoring the unknown-to-it signature ConfigMaps.
line, right? Also, is just "using a custom resource" an expensive server interaction? Even if you push a ConfigMap, you're still connecting to the cluster and performing some server-side operations.
There was a problem hiding this comment.
Talking to the server is an expensive (in dev time) operation. Basically we have sufficient experience that it's not worth it.
There was a problem hiding this comment.
Even with ConfigMap it's pretty straight forward, the cluster works with the items it knows about ignore everything above. Iow. if CM has fields signatures and somefancyfeature, older cluster will only work with signatures ignoring the other, whereas newer will work fine with both. That has proven fine in the past. I agree with Clayton here that client-side checking for version didn't work well. In CLIs we prefer to apply the current and fallback if that's not known to the current cluster.
|
One comment I'd like to see changed and then this is LGTM /approve in advance |
openshift-ci-robot
added
the
approved
This PR implements the openshift/enhancements#283 Local package pkg/verify was reused with minor changes from CVO so there is PR openshift/library-go#725 to move package verify into library-go.
Describing how we're going to feed restricted-network clusters the release-signature ConfigMaps that the cluster-version operator reads since openshift/cluster-version-operator#279. Personally, I prefer 'manifests' naming here, so: * manifests directory * --apply-manifests and --manifests-to-dir options. But there was some concern that the Kubernetes/OpenShift manifests would be confused with the container image manifests in v2/openshift/release/manifests [1]. kubernetes-manifests was suggested [2], which would to a kubernetes-manifests directory and --apply-kubernetes-manifests and --kubernetes-manifests-to-dir options. But there was some concern about using Kubernetes anywhere [3], so the commit goes with a config directory and options which leave the format implicit (--apply-release-image-signature and --release-image-signature-to-dir). The truncated digest hash in the example signature filename avoids any concerns about filename length limitations on restrictive operating systems or filesystems [4]. The narrow scoping vs openshift#188 accommodates pushback from Clayton in [5] and similar out of band discussion. The two-releases of backwards compatibility is based on [6]. I'm extrapolating from Clayton's comment to assume that "releases" means 4.(y+2), not 4.y -> 6.0. [1]: openshift#238 (comment) [2]: openshift#188 (comment) [3]: openshift#188 (comment) [4]: openshift#188 (comment) [5]: openshift#188 (comment) [6]: openshift#283 (comment)
wking
force-pushed
the
oc-adm-release-mirror-signatures
branch
from
528e93d to
367e32d
Compare
soltysh
left a comment
soltysh
left a comment
There was a problem hiding this comment.
Based on the slack discussion and my reading this is ok to merge as is.
/lgtm
|
|
||
| ### Graduation Criteria | ||
|
|
||
| This feature will move straight to GA, because `oc` doesn't seem to have a history of a `--tech-preview` flag or similar to allow for graduated stability of new features. |
There was a problem hiding this comment.
That's not true, there are several options, including naming the flag as alpha --alpha-myfancy-flag and/or hiding it so it's not presented to users MarkHidden on a flag. We rarely use it, but it's there, so this claims is too far fetched.
| * There's no way to implement it in [the *pushing to disk* flow](#pushing-to-disk). | ||
| * New versions of `oc` could create [custom resources][custom-resources] instead of using generic types such as ConfigMaps. | ||
| This way, configuration manifest application would generate errors when applying manifests to clusters which were too old to understand them. | ||
| But it is too late to use this approach for release image signatures, because the ConfigMap support has [already landed][cvo-config-map-signatures]. |
There was a problem hiding this comment.
Even with ConfigMap it's pretty straight forward, the cluster works with the items it knows about ignore everything above. Iow. if CM has fields signatures and somefancyfeature, older cluster will only work with signatures ignoring the other, whereas newer will work fine with both. That has proven fine in the past. I agree with Clayton here that client-side checking for version didn't work well. In CLIs we prefer to apply the current and fallback if that's not known to the current cluster.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LalatenduMohanty, smarterclayton, soltysh, wking 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 |
7 participants
Describing how we're going to feed restricted-network clusters the release-signature ConfigMaps that the cluster-version operator reads since openshift/cluster-version-operator#279.
The narrow scoping vs #188 accommodates pushback from @smarterclayton here and similar out of band discussion. Personally, I still prefer #188, because I expect us to grow many manifests as we attempt to gather more information from the wider internet to make life consistent in restricted-network clusters, and I like the convenience of a simple mirror command that gathers as much as possible with a single call.