Refactor c/i/image #122
Merged
Refactor c/i/image #122
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
👍 I like these distinctions |
In particular, do not even look at the manifest type; we might not event want to fetch the manifest if required signatures are missing, so loading it and possibly guessing the MIME type is not needed either. Signed-off-by: Miloslav Trmač <[email protected]>
Currently we use a types.Image, which supports lots of parsing, for verification processing in in signature.PolicyContext. In the future, we will want that types.Image to do significantly more processing at initialization time (e.g. determine manifest type and fully parse it), which is undesirable for signature verification — there we would _really_ prefer to first find a signature which cryptographically verifies, before even _downloading_ the manifest, let alone processing it in any way. So, split the minimum functionality desired for processing unsigned images (manifest and signature caching) into a separate UnparsedImage type. Right now, this does not affect any Image or UnparsedImage implementation (apart from dropping a few panic()ing mock functions). (Note that for some more advanced processing, signature/* may create a types.Image out of the given types.UnparsedImage in the future — but that would be an intentional action after the signature code determines that there is enough presumed trust to even start parsing anything.) Signed-off-by: Miloslav Trmač <[email protected]>
Now that types.UnparsedImage is split from types.Image, also split the implementations. In particular, copy.Image uses an UnparsedImage for signature verification. This structural separation allows us to remove the “It is essential for signature verification”… comments all over the place in favor of a single one in the choke point where an UnparsedImage turns into a full genericImage. Also, split the manifest type guessing (which involves parsing) so that it does not happen in UnparsedImage. This needs ugly fields like trueManifestMIMETypeSet, which will go away momentarily. Signed-off-by: Miloslav Trmač <[email protected]>
image/manifest.go now contains genericManifest without caring about genericImage, and image/image.go contains genericImage only. Signed-off-by: Miloslav Trmač <[email protected]>
Just a bit of pointless pedantry, sorry :) Signed-off-by: Miloslav Trmač <[email protected]>
This means that image.FromUnparsedImage can now fail, OTOH later methods which refer to a manifest cannot. This is an intermediate step to also parsing the manifest at that time already. Signed-off-by: Miloslav Trmač <[email protected]>
…Image Another step towards simplifying the types.Image/genericManifest relationship: Now every genericImage always has a genericManifest. Signed-off-by: Miloslav Trmač <[email protected]>
Update types.Image to drop unnecessary error return values, and modify genericManifest to be more suitable; then embed a genericManfest into genericImage as an anonymous member implementing most of the interface. genericImage has now become a tiny wrapper over UnparsedImage and genericManifest. Signed-off-by: Miloslav Trmač <[email protected]>
… immediately before we introduce another kind of image. Signed-off-by: Miloslav Trmač <[email protected]>
Right now this does not really do anything, except add a whole lot of do-nothing infrastructure in memoryImage, but it will make it much easier to return extra data (like a newly generated config.json) from UpdatedImage. Signed-off-by: Miloslav Trmač <[email protected]>
Instead, make it a responsibility of genericManifest to provide the MIME type. This simplifies creation of a memoryImage, and removes a field which may be redundant, e.g. with manifestSchema2.MediaType. Signed-off-by: Miloslav Trmač <[email protected]>
Collaborator
Author
|
(Rebased, added 9e7a5bf , and skopeo tests are running in containers/skopeo#230 ). |
mtrmac
force-pushed
the
image-refactor
branch
from
9e7a5bf to
1e99e60
Compare
Collaborator
Author
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@runcom PTAL.
This is a pretty big refactoring of
image/, see individual commits for details. The overall ideas:UnparsedImage, which structurally prevents interpreting too much before we know the image is admissible.genericImagebecomessourcedImage, a very thin layer overUnparsedImageandgenericManifestUpdatedManifest, we now haveUpdatedImage. This does not change much now, but will be a way to return alsoconfig.jsonand other objects.I haven’t actually updated the schema conversion work to take advantage of
UpdatedImageyet, but I expect it to help with the API shape noticeably.Tests pass in https://github.com/mtrmac/skopeo/tree/image-refactor-test , although note that that includes mtrmac@364c110 as a defeat device for #115, not to be merged.