-
Notifications
You must be signed in to change notification settings - Fork 236
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
[Additional Layer Store] Use TOCDigest as ID of each layer (patch for c/storage) #1924
Conversation
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 didn’t read the caller containers/image#2416 yet.)
// reference. Note that this hasn't been stored to this store yet: the actual creation of | ||
// a usable layer is done by calling the returned object's PutAs() method. After creating | ||
// a layer, the caller must then call the object's Release() method to free any temporary | ||
// resources which were allocated for the object by this method or the object's PutAs() | ||
// method. | ||
// This API is experimental and can be changed without bumping the major version number. | ||
LookupAdditionalLayer(d digest.Digest, imageref string) (AdditionalLayer, error) | ||
LookupAdditionalLayer(tocDigest digest.Digest, imageref string) (AdditionalLayer, error) |
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.
In general, changing the semantics of a security-critical parameter in a caller-invisible way like this feels rather risky to me.
In this case, assuming there’s exactly the one caller in c/image, and because this breaks the API of AdditionalLayer
, I suppose I can live with it.
This does not, AFAICS, change the visible API. What is the migration path to ensure that c/storage and the FUSE backends are updated in lockstep? Or are there no real-world deployments at all, to worry about? |
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
After this PR is merged, we'll merge containerd/stargz-snapshotter#1673 and release a new version as soon as possible. |
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, ktock The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
// UncompressedDigest returns the uncompressed digest of this layer | ||
UncompressedDigest() digest.Digest | ||
// TOCDigest returns the digest of TOC of this layer. Returns "" if unknown. | ||
TOCDigest() digest.Digest |
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.
Compare containers/image#2416 (comment) / containerd/stargz-snapshotter#1673 (comment) ; do we need this provided by the ALS backend at all?
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 actually, sort of, do: if this returns ""
, we know the FUSE server is older than this PR expects, and interprets the digest as non-TOC compressed digest.
AFAICS the proposed stargz-snapshotter
implementation never returns a different digest than the one we looked up, but at least the “supported/unsupported” bit seems valuable.
Needs: containers/image#2416
This commit changes the store implementation to use TOCDigset as the ID of each layer. This ensures the runtime to pass TOCDigest to the FUSE backend everytime requiring a layer.
This commit modifies c/storage's
LookupAdditionalLayer
to receive a TOCDigest as one of the arguments. Then that TOCDigest is passed to Additional Layer Store as one of the path elements of the FUSE fs. For example, the extracted view of a layer diff contents is provided in the following path:Additional Layer Store implementation attempts lazy pulling of the layer that has the specified TOCDigest in the image.
If that image doesn't contain the layer matching to the TOCDigest, accessing to the layer directly results in an error.
Example draft implementation of Additional Layer Store is at containerd/stargz-snapshotter#1673