-
Notifications
You must be signed in to change notification settings - Fork 377
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
Implement private.ImageDestination in all transports, improve transport helpers #1589
Conversation
Declare the data structure before using it, so that reading the file top-down is a bit easier. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The purpose is to let a transport only implement these methods, and provide a generic fallback for the public methods. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…tion Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that we can have two lines instead of copy&pasted methods. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
(Updated the PR description after an accidental early submit.) |
For now, this does not change behavior; but it will allow using BlobInfoCache2 next. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This should not change behavior, but it changes the overhead: - Transports (like dockerImageDestination) now don't need to care about the old interface - ... it's now handled by internal/imagedestination/impl - whether the transport uses the cache or not. Overall, this should make the internal callers and internal implementations a bit simpler and faster, at the cost of moving the overhead to the compat infrastructure, and paying the cost for more external callers. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This makes it clearer that dirImageDestination is only used in a valid state. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will allow using it in error messages in the future. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This sets up the precedent that all transports should primarily implement the private interface; that will allow us to make future changes to the private interface easier, because we can just change the public interface wrappers in a single place instead of modifying transports - especially as more stubs are added soonish. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is mostly a direct forwarder to the underlying transport (for consistency, even for calls that the underlying transport implements with stubs). We also use imagedestination.FromPublic although we know that the transport implements the private interface. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is not locally worth it, but it is a proof of the concept. Goals for similar optional transport features: - Don't use an interface type cast; require a SupportsPutBlobPartial method, so that every private transport implementor needs to make a decision whether to implment the feature. This will lead to required reviews if we add new features. - Minimize copy&pasted boilerplate methods: transports that don't implement the feature only need to add (and initialize) one No... stub. Transports that do implement the feature need to add an Implements... stub, and the actual implementation. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…n transports Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…rplate It's very annoying that Go allows silent zero-initilization, and we have neither mandatory constructors nor parameter labels, i.e. a forgotten field does not cause a compile-time error. For _private_ transport implementations, relying on human review is just about acceptable. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
aa40cc9
to
85db725
Compare
impl.Compat | ||
impl.PropertyMethodsInitialize | ||
stubs.NoPutBlobPartialInitialize | ||
stubs.AlwaysSupportsSignatures |
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 would very much appreciate an independent opinion. I’m really not sure whether using mix-ins like this is is more succinct and convenient, or surprising and inscrutable.
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.
To be honest, I found it exotic at the beginning but started to like it. I still have concerns how approachable the code is for new pairs of eyes though. I think that one central structure with boolean fields would be easier to digest. At the current state, properties are implicitly embedded into the structures.
In other words: I am completely OK to merge it as is. It's a dramatic improvement!
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 that one central structure with boolean fields would be easier to digest. At the current state, properties are implicitly embedded into the structures.
I’m not quite sure what this refers to. My best guess is that this says that it’s very hard to find the SupportsPutBlobPartial
value for a transport just by text search (quite true), and perhaps that it would be nice to have something broadly like
type private.ImageSource interface {
HasThreadSafeGetBlob bool
SupportsPutBlobPartial bool
}
…
func /*docker.*/newImageSource(…) private.ImageSource {
return &dockerImageSource{
HasThreadSafeGetBlob: true,
SupportsPutBlobPartial: true,
}
}
Is that what you mean? The immediate problem is that Go interfaces only contain methods, not fields. Secondarily, and less importantly, SupportsPutBlobPartial
could get out of sync with the actual implementation.[1]
The immediate problem could be handled by making private.ImageSource
a struct
type private.ImageSource struct {
HasThreadSafeGetBlob bool
SupportsPutBlobPartial bool
BasicImageSourceOperations // an interface
PutBlobPartialOperations // an interface, or even a *PutBlobPartialOperations and remove a separate SupportsPutBlobPartial
}
which is quite attractive in some respects, OTOH calling methods would be more annoying, and to an extent this is replacing compile-time interfaces with run-time dynamic function pointers. We would also have a separate private.ImageSource
struct and some separate dockerImageSource
struct carrying the actual transport’s state, so that doesn’t help tracing the relationship between transport’s properties and methods much.
With the mix-ins embedded into interface implementations, questions like “what implements $method for $transport” are compiler-visible and can be statically answered, though probably hard to follow with an IDE. With a struct pointing to separate interfaces, all of that data only exists dynamically at runtime.
[1] The primary motivation for the No
/Implements
… stubs is to make it a compile-time failure to forget to add a method, or to fail to keep up with an interface change. Without that, it’s IIRC possible to change the type of a parameter in an interface, and implementations suddenly don’t match without any notice, so a run-time check like source.(private.ImageDestinationWithPutBlobPartial)
can fail. With the stubs, and a single compile-time type check against a private.ImageDestination
, we can guarantee that one of the two stubs are present and that the type signatures are correct.
Admittedly this might be overthinking it — there are only 10 transports after all, and we could just do careful type checks in unit tests, and review API additions carefully. Then we wouldn’t the Supports…
methods, and we could just do interface type checks.
WRT the non-Supports
values, one possible alternative is
type Properties struct {
HasThreadSafeGetBlob bool
…
}
type ImageSource interface {
func Properties() Properties
}
and then we don’t have a PropertyMethodsInitialize
, just a single method implemented in every transport (+ compat.Impl
providing the single-value getters in the existing interface).
One downside to this is that actual users of the transport would be more clumsy — everything would be source.Properties().SomeValue
. A larger downside is that it would be harder and more expensive to implement “forwarding” transports like oci/archive
forwarding to oci/layout
, or BlobCache
, especially if the forwarding transport only wanted to override some of the values — and the forwarding transport would have to collect all of the values although the caller only wants one of them.
I’m fairly certain there is a better design to be found, and I’m very open to suggestions.
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.
(Also, the …Initialize
convention kind of drives me nuts. It seems somewhat attractive to me: to move that to a run-time check (allow each of those stubs to check if it has been initialized as a private method, and run that check in a unit test of the transport):
type privateSourceInitializationChecks interface {
initializedPutBlobPartialStub()
initialized…
}
func (stub ImplementsPutBlobPartial) initializedPutBlobPartialStub() bool {}
func (stub NoPutBlobPartialInitialize) initializedPutBlobPartialStub() bool {
if !initialized { panic() }
}
func ValidatePrivateSourceImplementation(src private.ImageSource) { // called from a unit test
inits, ok := src.(privateSourceInitializationChecks)
inits.initializedPutBlobPartialStub()
inits.initialized…
}
That has some run-time cost, but much worse it only works for transports where we can ~successfully create the source/destination object in a unit test. Which is awkward for remote clients like docker-daemon
.
Oh well, I suppose hating Go might count as a hobby…)
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.
Thanks for the detailed elaboration, @mtrmac.
I am supportive of the idea - and think it's a really good one. I very much enjoy the fact that build tests can help review changes :)
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
Thanks for putting the changes into easy to digest small(er) commits.
Feel free to merge at will.
LGTM |
copy.Image can now copy non-image OCI artifacts. Added support for sigstore signatures: they (and related cosign attachments) can be copied along with images after opt-in in registries.d. Signatures can be created by copy.Image and enforced via policy.json (currently with public/private key pairs only). Now requires Go 1.17. GPGMe now must be new enough to be visible via pkg-config. github.com/pkg/errors is no longer used; that might affect caller-observable error types (in particular, errors.{As,Is} might need to be used instead of pkg/errors.Cause). Changes default paths on FreeBSD. - Remove unused Makefile variables - Config files should live in /usr/local on FreeBSD - docker: validate received parts - Use go env to fetch the go path - docker: add workaround for CloudFront - Improve errors messages when image missing from list - Stop calling gpgme-config - Fix codespell errors - Make sure github.com/opencontainers/runc >= 1.1.2 is used - Cirrus: use Ubuntu 22.04 LTS - Merge pull request containers#1576 from mtrmac/private-image - Merge pull request containers#1577 from mtrmac/mocks - Merge pull request containers#1571 from mtrmac/go1.17 - Merge pull request containers#1578 from mtrmac/sourced-image-struct - Fix error on parallel multiple image pullings with additionallayerstore - Merge pull request containers#1579 from mtrmac/copy-layers-refactor - Reject OCI artifacts in manifest.OCI1.ImageID - Reject OCI artifacts in manifest.OCI1.Inspect - Refuse to convert non-image OCI artifacts to Docker formats - Reject OCI artifacts in image.manifestOCI1.OCIConfig - Introduce SourcedImage.CanChangeLayerCompression, use it in copy.Image - Use an updated CI image - Use strings.ReplaceAll instead of strings.Replace(..., -1) - Move the main helper removal case to the main path on RemoveAllAuthentication - Merge pull request containers#1588 from mtrmac/pkg_errors - Merge pull request containers#1589 from mtrmac/private-dest-impls - Merge pull request containers#1590 from mtrmac/private-src-impls - Merge pull request containers#1592 from mtrmac/blobcache-wrap-private - Use "io.ReadAll" instead of "os.ReadAll" - Merge pull request containers#1596 from mtrmac/cosign-payload - Generalize copy.Image to be able to copy signatures with any format - Merge pull request containers#1593 from mtrmac/cosign-sigs - Introduce signature.Cosign as a format - Add use-cosign-attachments to registries.d/*.yaml - Add support for reading and writing Cosign attachments, incl. signatures - Merge pull request containers#1595 from mtrmac/cosign-docker - Add support for creating Cosign signatures - Fix a long-standing incorrect comment - Fix JSON syntax in the policy.json(5) man page - Correctly decode Cosign-generated payloads - Add Cosign verification support - s/sigstore/lookaside/g in comments and documentation - Refer to lookasideStorage instead of signatureStorage in code - Add lookaside and lookaside-staging, hide sigstore and sigstore-staging - Merge pull request containers#1605 from mtrmac/sigstore - Fix a typo in error messages - Remove a copy&pasted test entry - Add context to some test failures - Use more valid data in TestPRSignedByIsSignatureAuthorAccepted - Generalize keyPath/keyData exclusivity checks - Remove repetition in tests - Accept multiple keyrings in newEphemeralGPGSigningMechanism - Allow accepting multiple GPG keyrings via signedBy.keyPaths - Switch to golang native error wrapping - Point out use-sigstore-registries in sigstoreSigned documentation - Use .pub extension for public keys in sigstoreSigned examples - copy: print copy info once when writer==io.Discard - Silence a "potentially unused parameter" warning - Read signatures from UnparsedImage instead of ImageSource directly - Consolidate reading messages, and checking for support, into a helper - build(deps): bump github.com/containers/storage from 1.40.0 to 1.40.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/klauspost/compress from 1.15.2 to 1.15.3 - build(deps): bump github.com/klauspost/compress from 1.15.3 to 1.15.4 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/proglottis/gpgme from 0.1.1 to 0.1.2 - build(deps): bump github.com/vbauerster/mpb/v7 from 7.4.1 to 7.4.2 - build(deps): bump github.com/imdario/mergo from 0.3.12 to 0.3.13 - build(deps): bump github.com/klauspost/compress from 1.15.4 to 1.15.5 - build(deps): bump github.com/sylabs/sif/v2 from 2.7.0 to 2.7.1 - build(deps): bump github.com/klauspost/compress from 1.15.5 to 1.15.6 - build(deps): bump github.com/stretchr/testify from 1.7.1 to 1.7.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/stretchr/testify from 1.7.2 to 1.7.4 - build(deps): bump github.com/stretchr/testify from 1.7.4 to 1.7.5 - build(deps): bump github.com/stretchr/testify from 1.7.5 to 1.8.0 - build(deps): bump github.com/klauspost/compress from 1.15.6 to 1.15.7 - build(deps): bump github.com/proglottis/gpgme from 0.1.2 to 0.1.3 - build(deps): bump github.com/klauspost/compress from 1.15.7 to 1.15.8 - build(deps): bump github.com/sirupsen/logrus from 1.8.1 to 1.9.0 - build(deps): bump github.com/theupdateframework/go-tuf - build(deps): bump github.com/BurntSushi/toml from 1.1.0 to 1.2.0 Signed-off-by: Miloslav Trmač <mitr@redhat.com>
copy.Image can now copy non-image OCI artifacts. Added support for sigstore signatures: they (and related cosign attachments) can be copied along with images after opt-in in registries.d. Signatures can be created by copy.Image and enforced via policy.json (currently with public/private key pairs only). Now requires Go 1.17. GPGMe now must be new enough to be visible via pkg-config. github.com/pkg/errors is no longer used; that might affect caller-observable error types (in particular, errors.{As,Is} might need to be used instead of pkg/errors.Cause). Changes default paths on FreeBSD. - Remove unused Makefile variables - Config files should live in /usr/local on FreeBSD - docker: validate received parts - Use go env to fetch the go path - docker: add workaround for CloudFront - Improve errors messages when image missing from list - Stop calling gpgme-config - Fix codespell errors - Make sure github.com/opencontainers/runc >= 1.1.2 is used - Cirrus: use Ubuntu 22.04 LTS - Merge pull request containers#1576 from mtrmac/private-image - Merge pull request containers#1577 from mtrmac/mocks - Merge pull request containers#1571 from mtrmac/go1.17 - Merge pull request containers#1578 from mtrmac/sourced-image-struct - Fix error on parallel multiple image pullings with additionallayerstore - Merge pull request containers#1579 from mtrmac/copy-layers-refactor - Reject OCI artifacts in manifest.OCI1.ImageID - Reject OCI artifacts in manifest.OCI1.Inspect - Refuse to convert non-image OCI artifacts to Docker formats - Reject OCI artifacts in image.manifestOCI1.OCIConfig - Introduce SourcedImage.CanChangeLayerCompression, use it in copy.Image - Use an updated CI image - Use strings.ReplaceAll instead of strings.Replace(..., -1) - Move the main helper removal case to the main path on RemoveAllAuthentication - Merge pull request containers#1588 from mtrmac/pkg_errors - Merge pull request containers#1589 from mtrmac/private-dest-impls - Merge pull request containers#1590 from mtrmac/private-src-impls - Merge pull request containers#1592 from mtrmac/blobcache-wrap-private - Use "io.ReadAll" instead of "os.ReadAll" - Merge pull request containers#1596 from mtrmac/cosign-payload - Generalize copy.Image to be able to copy signatures with any format - Merge pull request containers#1593 from mtrmac/cosign-sigs - Introduce signature.Cosign as a format - Add use-cosign-attachments to registries.d/*.yaml - Add support for reading and writing Cosign attachments, incl. signatures - Merge pull request containers#1595 from mtrmac/cosign-docker - Add support for creating Cosign signatures - Fix a long-standing incorrect comment - Fix JSON syntax in the policy.json(5) man page - Correctly decode Cosign-generated payloads - Add Cosign verification support - s/sigstore/lookaside/g in comments and documentation - Refer to lookasideStorage instead of signatureStorage in code - Add lookaside and lookaside-staging, hide sigstore and sigstore-staging - Merge pull request containers#1605 from mtrmac/sigstore - Fix a typo in error messages - Remove a copy&pasted test entry - Add context to some test failures - Use more valid data in TestPRSignedByIsSignatureAuthorAccepted - Generalize keyPath/keyData exclusivity checks - Remove repetition in tests - Accept multiple keyrings in newEphemeralGPGSigningMechanism - Allow accepting multiple GPG keyrings via signedBy.keyPaths - Switch to golang native error wrapping - Point out use-sigstore-registries in sigstoreSigned documentation - Use .pub extension for public keys in sigstoreSigned examples - copy: print copy info once when writer==io.Discard - Silence a "potentially unused parameter" warning - Read signatures from UnparsedImage instead of ImageSource directly - Consolidate reading messages, and checking for support, into a helper - build(deps): bump github.com/containers/storage from 1.40.0 to 1.40.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/klauspost/compress from 1.15.2 to 1.15.3 - build(deps): bump github.com/klauspost/compress from 1.15.3 to 1.15.4 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/proglottis/gpgme from 0.1.1 to 0.1.2 - build(deps): bump github.com/vbauerster/mpb/v7 from 7.4.1 to 7.4.2 - build(deps): bump github.com/imdario/mergo from 0.3.12 to 0.3.13 - build(deps): bump github.com/klauspost/compress from 1.15.4 to 1.15.5 - build(deps): bump github.com/sylabs/sif/v2 from 2.7.0 to 2.7.1 - build(deps): bump github.com/klauspost/compress from 1.15.5 to 1.15.6 - build(deps): bump github.com/stretchr/testify from 1.7.1 to 1.7.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/stretchr/testify from 1.7.2 to 1.7.4 - build(deps): bump github.com/stretchr/testify from 1.7.4 to 1.7.5 - build(deps): bump github.com/stretchr/testify from 1.7.5 to 1.8.0 - build(deps): bump github.com/klauspost/compress from 1.15.6 to 1.15.7 - build(deps): bump github.com/proglottis/gpgme from 0.1.2 to 0.1.3 - build(deps): bump github.com/klauspost/compress from 1.15.7 to 1.15.8 - build(deps): bump github.com/sirupsen/logrus from 1.8.1 to 1.9.0 - build(deps): bump github.com/theupdateframework/go-tuf - build(deps): bump github.com/BurntSushi/toml from 1.1.0 to 1.2.0 Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is a superset of the remaining part of #1439.
private.ImageDestination
, leaving the task of compatibility with publictypes.ImageDestination
to aninternal/imagedestination/impl.Compat
helper. All transports will now think purely in terms of the private interface.BlobInfoCache
is not yet included: that needs to happen, but that will be a later PR (notably because it needs to deal with forwarding between private and public-only users).imagedestination/impl.Properties
, and let the transport just provide the values to return.The move to private interfaces is, I think, generally desirable. We will soon need to extend the signatures interface, so consolidating the compatibility concerns into a single location will help.
The boilerplate reduction part is RFC. Go is really not well suited handling some of the design concerns, primarily because it allows silent zero-initialization of struct members (all of simple values, structs, and interface pointers), without requiring the user to provide a value or to call a constructor. The previously-used interface with mandatory method implementation is really the only way to force a transport author to deal with a value;
impl.Properties
weakens that compile-time enforcement. I’m not quite sure that it is worth it.