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

cloud,grpcutil: avoid pretty-printing nil gRPC status #103856

Merged
merged 1 commit into from
May 30, 2023

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented May 24, 2023

In some cases, errors from the GCP library would be printed as

grpc:  [code 0/OK]

This was the result of a few interacting features and changes:

  • The GCP library added an Unwrap() method to the error type they returned.

  • Our errors library has code to manually unwrap errors.

  • The underlying GCP error attempts to abstract over gRPC and HTTP errors. It implements a GRPCStatus() method that returns nil if the underlying issue was an HTTP issue.

  • Our custom error printer in grpcutil would specially print anything that implements GRPCStatus(), regardless of the return value.

Here, I've updated the custom printer to ignore cases where GRPCStatus() returns nil.

Epic: CRDB-27642

Release note (bug fix): Fix a bug in which some GCP-related errors would be returned with an uninformative error.

@stevendanna stevendanna requested a review from a team as a code owner May 24, 2023 18:18
@stevendanna stevendanna requested review from lidorcarmel and removed request for a team May 24, 2023 18:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna stevendanna requested review from dt and knz and removed request for lidorcarmel May 24, 2023 18:19
@stevendanna stevendanna force-pushed the error-wrapping-oddness branch 2 times, most recently from a497904 to 16d82a1 Compare May 25, 2023 09:23
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

:lgtm: with nit.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt and @stevendanna)


pkg/util/grpcutil/grpc_err_redaction.go line 26 at r1 (raw file):

	// return true, but the retuned error will have no message and
	// a code of OK.
	if se := (interface{ GRPCStatus() *grpcStatus.Status })(nil); errors.As(err, &se) {

Since the errors library is going to call grpcSpecialCasePrintFn on every level of the error chain, don't use errors.As here, which also recurses.
You can use the same simple cast as is performed inside status.FromError already.

@stevendanna stevendanna force-pushed the error-wrapping-oddness branch from 16d82a1 to cffa3f8 Compare May 25, 2023 12:58
@stevendanna stevendanna added the backport-23.1.x Flags PRs that need to be backported to 23.1 label May 25, 2023
In some cases, errors from the GCP library would be printed as

    grpc:  [code 0/OK]

This was the result of a few interacting features and changes:

- The GCP library added an Unwrap() method to the error type they
  returned.

- Our errors library has code to manually unwrap errors.

- The underlying GCP error attempts to abstract over gRPC and HTTP
  errors. It implements a GRPCStatus() method that returns nil if the
  underlying issue was an HTTP issue.

- Our custom error printer in grpcutil would specially print anything
  that implements GRPCStatus(), regardless of the return value.

Here, I've updated the custom printer to ignore cases where
GRPCStatus() returns nil.

Epic: none

Release note (bug fix): Fix a bug in which some GCP-related errors
would be returned with an uninformative error.
@stevendanna stevendanna force-pushed the error-wrapping-oddness branch from cffa3f8 to 5b4aa44 Compare May 25, 2023 14:08
@stevendanna
Copy link
Collaborator Author

bors r=knz

@craig
Copy link
Contributor

craig bot commented May 30, 2023

Build failed:

@stevendanna
Copy link
Collaborator Author

bors retry

@craig
Copy link
Contributor

craig bot commented May 30, 2023

Build succeeded:

@shermanCRL
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants