Skip to content
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

Use a SourcedImage struct in c/image/copy #1578

Merged
merged 10 commits into from
Jun 15, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jun 14, 2022

Another part of #1574 (but note that this approach is rather different from an earlier version):

Use an internal/image.SourcedImage directly instead of a types.Image interface for most of c/image/copy. The implementation/tests of copy.determineManifestConversion currently mock the types.Image interface; instead, turn determineManifestConversion into a ~pure function, with all inputs and outputs as fields in a struct.

Also add the first benefit of using the non-public SourcedImage: access the manifest and manifest’s MIME type directly without having to worry about (impossible) failures.

Should not change behavior overall. See individual commit messages for details.

mtrmac added 10 commits June 14, 2022 16:48
…mage

This will allow us to add more methods to SourcedImage without breaking
external implementations of types.Image, if any.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We know these values are available without having to worry about
errors, which allows simplifying users in c/image/copy.  We don't really
need accessor functions, all users are internal (and a public field
is actually better protected than a public method, because a public
method can be used by third parties through a caller-defined interface).

It's also usefo to make them available as individual values; various
users only need one or the other.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
To make determineManifestConversion easier to test,
we'll want to separate it from imageCopier and make it a pure function.

To do that, start with introducing a struct for its return value;
that way we won't need three unnamed return values, and three
variables with long names at the call site.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Have determineManifestConversion just set a flag in the returned plan;
then copier.copyOneImage will actually act on that flag.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…estConversion

This is not smaller, but a bit more readable and consistent
with the real call site.

Also, on test failures we can see the full returned plan as a single unit,
making it possibly easier to see what went wrong.

Should not change (test) behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and split the actual logic (determineManifestConversion)
from imageCopier (imageCopier.determineManifestConversion).

determineManifestConversion is now a pure function (well,
apart from logging).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Keep the "reading manifest failed" case, for now.

Should not change (test) behavior, except that the test coverage
is now a tiny bit smaller - but then the test not depending
on imageCopier is the primary reason for this refactoring.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is surprisingly somewhat convoluted.

We've had to do a bunch of work to make determineManifestConversion
not depend on a complete (and mostly valid!) image, to be able
to make this change.

Then, so that we don't lose test coverage (and to actually benefit
from the new private type), use SourcedImage.ManifestMIMEType
in imageCopier.determineManifestConversion.  That allows us to get
rid of the last imageCopier.determineManifestConversion call
in tests.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This eliminates some unreachable error paths.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It's now basically a trivial call forwarder we don't need for
anything, so just have the caller set determineManifestConversionInputs
directly (and let it benefit from labeling the inputs).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
merge at will

@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2022

LGTM

@rhatdan rhatdan merged commit a78a007 into containers:main Jun 15, 2022
@mtrmac mtrmac deleted the sourced-image-struct branch June 15, 2022 15:41
mtrmac added a commit to mtrmac/image that referenced this pull request Jul 21, 2022
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>
@mtrmac mtrmac mentioned this pull request Jul 21, 2022
mtrmac added a commit to mtrmac/image that referenced this pull request Jul 21, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants