lint: forbid gRPC Status.WithDetails() due to gogoproto issues #61617
Conversation
This file contains 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
gRPC's `Status.WithDetails()` allows callers to attach Protobuf-structured details to gRPC errors. Unfortunately, this does not work with gogoproto types, since they're stored in an `Any` field internally and gogo types are not registered in the standard Protobuf type registry. Calling `Details()` with such a type will return an unmarshalling error in place of the detail. To avoid this problem, this patch adds a linter that disallows any use of `Status.WithDetails()`. Note that there is a separate package `github.com/gogo/status` that emulates the standard gRPC `status` package using gogoproto Protobufs instead. However, due to the uncertain future of the gogoproto project we are hesitant to introduce additional gogo dependencies. Release justification: non-production code changes Release note: None
|
This change is |
tbg
approved these changes
Mar 8, 2021
|
bors r=tbg |
|
Build succeeded: |
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 8, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 9, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 10, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 14, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. We lock the `gogo/googleapis` package to 1.2.0 to use gogoproto 1.2-compatible Protobufs required by CRDB, these have been verified to not be vulnerable to the "skippy pb" bug. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 14, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. We lock the `gogo/googleapis` package to 1.2.0 to use gogoproto 1.2-compatible Protobufs required by CRDB, these have been verified to not be vulnerable to the "skippy pb" bug. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 15, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. We lock the `gogo/googleapis` package to 1.2.0 to use gogoproto 1.2-compatible Protobufs required by CRDB, these have been verified to not be vulnerable to the "skippy pb" bug. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to erikgrinaker/errors
that referenced
this pull request
Mar 15, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. We lock the `gogo/googleapis` package to 1.2.0 to use gogoproto 1.2-compatible Protobufs required by CRDB, these have been verified to not be vulnerable to the "skippy pb" bug. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
erikgrinaker
added a commit
to cockroachdb/errors
that referenced
this pull request
Mar 17, 2021
This patch adds support for automatic en/decoding of gRPC `Status` errors, enabled by importing the `extgrpc` package. It supports statuses generated both by the standard `google.golang.org/grpc/status` and the gogoproto-based `github.com/gogo/status` packages. Encoded statuses are always represented as `gogo/status`-based `Status` Protobufs, since the `EncodedError` type is a gogoproto type using an `Any` field for structured errors, and only gogoproto types can be placed here since standard Protobufs are not registered in the gogoproto type registry. We lock the `gogo/googleapis` package to 1.2.0 to use gogoproto 1.2-compatible Protobufs required by CRDB, these have been verified to not be vulnerable to the "skippy pb" bug. Note that to preserve error details, gogo-based `Status` objects with gogo-based details must be used, otherwise details may not be preserved. This is for similar reasons as outlined above, and is a limitation in the gRPC package -- see code comments for further details. A CRDB linter rule forbidding use of `Status.WithDetails()` was submitted in cockroachdb/cockroach#61617.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
gRPC's
Status.WithDetails()allows callers to attachProtobuf-structured details to gRPC errors. Unfortunately, this does not
work with gogoproto types, since they're stored in an
Anyfieldinternally and gogo types are not registered in the standard Protobuf
type registry. Calling
Details()with such a type will return anunmarshalling error in place of the detail.
To avoid this problem, this patch adds a linter that disallows any use of
Status.WithDetails().Note that there is a separate package
github.com/gogo/statusthatemulates the standard gRPC
statuspackage using gogoproto Protobufsinstead. However, due to the uncertain future of the gogoproto project
we are hesitant to introduce additional gogo dependencies.
Related to #56208 and cockroachdb/errors#63.
Release justification: non-production code changes
Release note: None