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

Remove users of pkg/errors.WithStack, pkg/errors.Cause #1588

Merged
merged 15 commits into from
Jul 1, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jun 30, 2022

Motivated by containers/podman#14784 removing pkg/errors.

This does not remove all of it: We still use pkg/errors.Wrap* to provide an unchanged interface to callers that rely on pkg/errors.Cause.

  • Stop attaching stack traces to errors, other than as implied by pkg/errors.Wrap*. I.e. use errors.New instead of pkg/errors.New, fmt.Errorf instead of errors.Errorf, remove a few explicit pkg/errors.WithStack. Those were never promised (OTOH Podman does have at least one fmt(…%+v…, err), so this might break something).
    • We could drop stack traces also from errors.Wrap* by replacing that with errors.WithMessage*, but that’s a mouthful, and we want to eventually eliminate those fully…
  • Eliminate almost all uses of pkg/errors.Cause; use errors.{As,Is}. Note that this could, in principle, change behavior, and I didn’t examine the call stack in detail. c/storage is probably the most risky in this respect — OTOH I have a hard time thinking of a specific scenario where this would break, and we need c/image to update this before c/storage can eliminate pkg/errors.Wrap*. We retain one pkg/errors.Cause in tests, at least for now.
  • Consistently import errors as errors, and pkg/errors as perrors, so that we can refer to both in the same file.
  • Fix some random typos in error strings.

See individual commit messages for details.

I’d be happy to split this further as suggested, or drop/defer this now and return to that effort some time later, to coordinate with other work (notably because this would almost certainly require a rebase of any other outstanding work — and one more rebase after we eliminate pkg/errors.Wrap*).

mtrmac added 15 commits June 30, 2022 21:42
It's not a part of the API promise, it's fairly costly,
and we want to move away from pkg/errors.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This means we won't save the stack, which is cheaper
(and possibly might break callers' format strings that
want to print the stack, but we never promised the stack
to be available).

Use either fmt.Errorf, or errors.New (usually as a local
edit, not carring about errors.new vs. pkg/errors.New;
that's going to be cleaned up later).

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will allow imports of errors and pkg/errors to
coexist in a single package, avoid adding accidental
new uses via errors.New(), make any potential new
additions more visible in reviews.

Files that can import errors only (typically
because they only use errors.New) have been updated to
that import instead of renaming.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... now that the other package is referred to as perrors.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This removes the stack trace, potentially visible
to callers.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
For these marker errors, presence at any location
in the unwrap stack was most likely intentional, so this
should be safe.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
fs.ErrNotExist anywhere on the unwrap stack
should be sufficient, so use the recommended
modern replacement for os.IsNotExist, making errors.Cause
unnecessary.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Presence of storage.ErrDuplicateID anywhere on the unwrap
stack seems appropriate here.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Accepting ErrDuplicateID or ErrLayerUnknown anywhere
on the unwrap stack seems appropriate.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
By construction of pkg/errors.WithStack
 / pkg/errors.WithMessage / pkg/errors.Wrap*,
pkg/errors.Cause() can only return nil if the
error is nil.

Move that away from the error handling code.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Presence of exec.ErrNotFound anywhere on the unwrap
stack seems appropriate.

Also replace the switch with an if.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
In the exec.ErrNotFound case, set creds to nil
so that we have the ordinary handling on the main
line.

Then eliminate two consecutive if statements with the same
condition.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Also replace switch with an if

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

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

mtrmac commented Jun 30, 2022

Note to self: There’s probably a more reliable argument as to whether replacing errors.Cause is safe (if we know whether the type we are looking for implements Cause() or Unwrap()).

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
Nice improvements!

@rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Jul 1, 2022

LGTM

@rhatdan rhatdan merged commit 44e127c into containers:main Jul 1, 2022
@mtrmac mtrmac deleted the pkg_errors branch July 1, 2022 15:35
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