Allow using schema2 when pushing to OpenShift #266
Conversation
|
LGTM |
| // … if it fails, _and_ the failure is because the manifest is rejected, we may have other options. | ||
| if _, isManifestRejected := errors.Cause(err).(types.ManifestTypeRejectedError); !isManifestRejected || len(otherManifestMIMETypeCandidates) == 0 { | ||
| // We don’t have other options. | ||
| // In principle the code below would handle this as well, but the resulting error message is fairly ugly. Do |
There was a problem hiding this comment.
nit. Remove 'Do' from end of this comment.
| return errors.Wrap(err, "Error writing manifest") | ||
| } | ||
|
|
||
| writeReport("Storing signatures\n") |
There was a problem hiding this comment.
We're Storing the signatures, but we're not writing the manifest any more? Is that correct or have I not had enough tea this morning?
There was a problem hiding this comment.
NVM, I see that's it's happening further down in the code.
| } | ||
|
|
||
| // copyUpdatedConfigAndManifest updates the image per ic.manifestUpdates, if necessary, | ||
| // stores the resulting config and manifest ot the destination, and returns the stored manifest. |
There was a problem hiding this comment.
nit 'manifest ot' to 'manifest to'
| if !ic.diffIDsAreNeeded && ic.src.UpdatedImageNeedsLayerDiffIDs(*ic.manifestUpdates) { | ||
| // We have set ic.diffIDsAreNeeded based on the preferred MIME type returned by determineManifestConversion. | ||
| // So, this can only happen if we are trying to upload using one of the other MIME type candidates. | ||
| // Because UpdatedImageNeedsLayerDiffIDs only when converting from s1 to s2, this case should only arise |
There was a problem hiding this comment.
nit "Because UpdatedImageNeedsLayerDiffIDs only" to "Because UpdatedImageNeedsLayerDiffIDs is used only" perhaps?
There was a problem hiding this comment.
Modified to “$method is true only …”
| // Because UpdatedImageNeedsLayerDiffIDs only when converting from s1 to s2, this case should only arise | ||
| // when ic.dest.SupportedManifestMIMETypes() includes both s1 and s2, the upload using s1 failed, and we are now trying s2. | ||
| // Supposedly s2-only registries do not exist or are extremely rare, so failing with this error message is good enough for now. | ||
| // If handling such registries turned out to be necessary, we could compute ic.diffIDsAreNeeded based on the full list of manifest MIME type candidates. |
There was a problem hiding this comment.
nit 'turned' to 'turns'
| } | ||
|
|
||
| if err := ic.copyConfig(pendingImage); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
We've error text for the other errors, should we have an errors.Wrap(err, "Error copying configuration") here?
There was a problem hiding this comment.
errors.Wrap is not supposed to exactly mirror the call stack (errors.Wrap is automatically recording that as well, separately); it adds context for the user to see what the error was about.
The user really doesn’t care that copyConfig is a separate method (with a single caller, even—created just to keep the method sizes manageable).
copyConfig itself, and copyBlobFromtream called from it, already add context to the operation (e.g., perhaps, Error writing blob: [timeout]) and either explicitly say that they are about the config, or are related to generic blob I/O where the config is not special enough to deserve an explicit mention.
| return false | ||
| } | ||
| // ErrorCodeManifestInvalid is returned by OpenShift with acceptschema2=false. | ||
| // ErrorCodeTagInvalid is returned by docker/distribution (at least as of ec87e9b6971d831f0eff752ddb54fb64693e51cd) |
There was a problem hiding this comment.
nit I think you want a date rather than 'ec87e...' inside the parens? Or perhaps reword "at least as of" to identify what ec87e... is?
There was a problem hiding this comment.
Made more explicit using “as of commit …”
|
Other than nits, LGTM. |
Instead, just forward SupportedManifestMIMETypes to the underlying docker: transport. Now that we can do autodetection, it should be safe to declare schema2 (or, in the future, OCI) support. One notable difference is that SupportedManifestMIMETypes affects what we configure the source to accept: e.g. when streaming from docker: to s1-only atomic:, previously the conversion was done by the docker: registry server, but now the source registry gives us schema2 and we do the conversion, changing the manifest digest. Right now that is a difference, but soon(?) we will update the manifest to use the destination’s name/tag, and then the manifest digest will change in any case. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
@TomSweeneyRedHat Thanks for the review! I'm sorry I haven’t noticed that the changes actually come from #264 , and I have already merged that. I will prepare a separate PR to clean this up. |
|
👍 |
mtrmac
changed the title
|
3 participants
With #264 we can dynamically try pushing using schema2, and fall back to schema1, without relying on a static
SupportedManifestMIMETypeslist. Therefore, modify theatomic:transport to use the same set of of statically declared MIME types candidates as thedocker:transport.In practical terms, OpenShift registries configured with
acceptschema2: truewill now get schema2 images if the input is schema2 (schema1 images would be copied unmodified as schema1), whileaccpetschema2:falseregistries will continue to always get schema1.One notable difference is that
SupportedManifestMIMETypesaffects what we configure the source to accept: e.g. when streaming fromdocker:to s1-onlyatomic:, previously the conversion was done by thedocker:registry server, but now the source registry gives us schema2 and we do the conversion, changing the manifest digest. Right now that is a difference, but soon(will add link here) we will update the manifest to use the destination’s name/tag, and then the manifest digest will change in any case.This PR depends on, and includes, #264, only the last commit is actually new. I will rebase as necessary.
@aweiteka @baude FYI