status: fix/improve status handling #6662

dfawley · 2023-09-25T20:54:12Z

cc @ejona86 @markdroth FYI / to review the behavior to confirm it conforms to the grpc spec or the intentions of the spec, since there is unfortunately no written spec for grpc-status-details-bin.

API-layer:

If the user sets a status code outside the bounds defined in the codes package 0-16 (inclusive), set the status code to codes.Unknown. This impacts statuses created locally as well as statuses received in RPC response trailers. See grpc/grpc-java#10568 for evidence this may be happening in the wild.

Previous behavior: allowed any value to be used and set on the wire (server) or read from the wire (client).

Client-side:

When receiving a grpc-status-details-bin trailer:

If there is 1 value and it deserializes into a google.rpc.Status, ensure the code field matches the grpc-status header's code. If it does not match, convert the code to codes.Internal and set a message indicating the mismatch. If it does, the status will contain the full details of the grpc-status-details-bin proto. (Note that grpc-message will not be checked against the proto's message field, and will be silently discarded if there is a mismatch.)
Otherwise, the status returned to the application will use the grpc-status and grpc-message values only.
In all cases, the raw grpc-status-details-bin trailer will be visible to the application via trailer["grpc-status-details-bin"] (where trailer is either obtained via the grpc.Trailer() CallOption or grpc.ClientStream.Trailer()).

Previous behavior: use the grpc-status-details-bin, if present, directly, ignoring grpc-status and grpc-message, and returning a different error to the application if it could not be parsed as a google.rpc.Status.

Server-side:

If the user manually sets grpc-status-details-bin in the trailers:

If the status returned by the method handler does not include details (see status.(*Status).WithDetails), the transport will send the user's grpc-status-details-bin trailer(s) directly.
If the status returned by the method handler does include details, the transport will disregard the user's trailer(s) and replace them with a serialized google.rpc.Status proto version of the returned status.

Previous behavior: ignore any grpc-status-details-bin set by the user and set it automatically as a proto if details were present in the returned status.

Internal

This change also adds a way to use the stub server with the handler transport.

RELEASE NOTES:

status: enforce that all status codes must be a valid code.Code; other values will be converted to codes.Unknown AND server: allow applications to send arbitrary data in the grpc-status-details-bin trailer AND client: validate grpc-status-details-bin trailer and pass through the trailer to the application directly

API-layer: 1. If the user sets a status code outside the bounds defined in the `codes` package 0-16 (inclusive), set the status code to `codes.Unknown`. This impacts statuses created locally as well as statuses received in RPC response trailers. See grpc/grpc-java#10568 for evidence this may be happening in the wild. Client-side: 1. When receiving a `grpc-status-details-bin` trailer: - If there is 1 value and it deserializes into a `google.rpc.Status`, ensure the code field matches the `grpc-status` header's code. If it does not match, convert the code to `codes.Internal` and set a message indicating the mismatch. If it does, the status will contain the full details of the `grpc-status-details-bin` proto. (Note that `grpc-message` will not be checked against the proto's message field, and will be silently discarded if there is a mismatch.) - Otherwise, the status returned to the application will use the `grpc-status` and `grpc-message` values only. - In all cases, the raw `grpc-status-details-bin` trailer will be visible to the application via `metadata.FromIncomingContext(ctx)["grpc-status-details-bin"]`. Server-side: 1. If the user manually sets `grpc-status-details-bin` in the trailers: - If the status returned by the method handler _does not_ include details (see `status.(*Status).WithDetails`), the transport will send the user's `grpc-status-details-bin` trailer(s) directly. - If the status returned by the method handler _does_ include details, the transport will disregard the user's trailer(s) and replace them with a `google.rpc.Status` proto version of the returned status.

internal/transport/http2_server.go

internal/status/status.go

internal/stubserver/stubserver.go

status/status_ext_test.go

easwars · 2023-09-29T18:24:21Z

status/status_ext_test.go

+					trailerGot := metadata.MD{}
+					_, errGot := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{}, grpc.Trailer(&trailerGot))
+					gsdb := trailerGot["grpc-status-details-bin"]
+					if !reflect.DeepEqual(gsdb, tc.trailerWant) {


Any reason for this to not use cmp.Equal?

I'll flip it around: why prefer cmp.Equal over reflect.DeepEqual if there are no special equality options needed?

why prefer cmp.Equal over reflect.DeepEqual?

Because it is the recommended way of doing comparisons in tests.

if there are no special equality options needed?

We shouldn't have to think about whether special comparison options are required before making the decision between the two, every time we have to make a comparison in our tests. reflect.DeepEqual is sensitive to changes in unexported fields. Agreed that we are only comparing a slice of strings here, but I feel more usages of reflect.DeepEqual will lead to more of them in our codebase (saying I'm using it to maintain consistency, or oh that test does it and why shouldn't this one do?)

Because it is the recommended way of doing comparisons in tests.

The rationale cites "reflect.DeepEqual compares unexported fields <which presumably shouldn't ever be important to your tests I guess?>"...but if unexported fields is what you want isn't reflect.DeepEqual much easier to use? In this case it's comparing []string so if it isn't good enough then I don't understand why.

If cmp is preferred why isn't it in the stdlib? 🤷

I switched it but don't really see the rationale of always reaching for something that's often more complicated and harder to use by default over something that for simple use cases is easier to use and understand.

If cmp is preferred why isn't it in the stdlib? 🤷

Don't know why it is not part of the stdlib. But they do mention that it is maintained by the Go team.

I switched it but don't really see the rationale of always reaching for something that's often more complicated and harder to use by default over something that for simple use cases is easier to use and understand.

I don't see it switched yet. But why is cmp.Equal(gsdb, tc.trailerWant) more difficult to use or understand here when compared to reflect.DeepEqual(gsdb, tc.trailerWant)? Am I missing something?

I see that commit where you switched, but for some reason github doesn't show it in the full diff.

But why is cmp.Equal(gsdb, tc.trailerWant) more difficult to use or understand here when compared to reflect.DeepEqual(gsdb, tc.trailerWant)?

Here it's equivalent. But there are other places where, e.g. if you want to compare the unexported fields, it can be more complicated. For diffs and complex comparisons, it's great, but for simple things, reflect.DeepEqual matches with its own simplicity.

I see that commit where you switched, but for some reason github doesn't show it in the full diff.

That's strange. It shows up in the "Files Changed" tab for me as expected.

status/status_ext_test.go

internal/stubserver/stubserver.go

internal/status/status.go

easwars · 2023-09-29T21:47:30Z

status/status_ext_test.go

+					trailerGot := metadata.MD{}
+					_, errGot := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{}, grpc.Trailer(&trailerGot))
+					gsdb := trailerGot["grpc-status-details-bin"]
+					if !reflect.DeepEqual(gsdb, tc.trailerWant) {


why prefer cmp.Equal over reflect.DeepEqual?

Because it is the recommended way of doing comparisons in tests.

if there are no special equality options needed?

We shouldn't have to think about whether special comparison options are required before making the decision between the two, every time we have to make a comparison in our tests. reflect.DeepEqual is sensitive to changes in unexported fields. Agreed that we are only comparing a slice of strings here, but I feel more usages of reflect.DeepEqual will lead to more of them in our codebase (saying I'm using it to maintain consistency, or oh that test does it and why shouldn't this one do?)

easwars · 2023-09-29T22:55:29Z

status/status_ext_test.go

+					trailerGot := metadata.MD{}
+					_, errGot := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{}, grpc.Trailer(&trailerGot))
+					gsdb := trailerGot["grpc-status-details-bin"]
+					if !reflect.DeepEqual(gsdb, tc.trailerWant) {


If cmp is preferred why isn't it in the stdlib? 🤷

Don't know why it is not part of the stdlib. But they do mention that it is maintained by the Go team.

easwars · 2023-09-29T22:59:53Z

status/status_ext_test.go

+					trailerGot := metadata.MD{}
+					_, errGot := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{}, grpc.Trailer(&trailerGot))
+					gsdb := trailerGot["grpc-status-details-bin"]
+					if !reflect.DeepEqual(gsdb, tc.trailerWant) {


I switched it but don't really see the rationale of always reaching for something that's often more complicated and harder to use by default over something that for simple use cases is easier to use and understand.

I don't see it switched yet. But why is cmp.Equal(gsdb, tc.trailerWant) more difficult to use or understand here when compared to reflect.DeepEqual(gsdb, tc.trailerWant)? Am I missing something?

easwars · 2023-09-29T23:01:12Z

status/status_ext_test.go

+					trailerGot := metadata.MD{}
+					_, errGot := ss.Client.UnaryCall(ctx, &testpb.SimpleRequest{}, grpc.Trailer(&trailerGot))
+					gsdb := trailerGot["grpc-status-details-bin"]
+					if !reflect.DeepEqual(gsdb, tc.trailerWant) {


I see that commit where you switched, but for some reason github doesn't show it in the full diff.

This reverts commit 0772ed7.

This reverts commit 9e1fc3e.

This reverts commit 9e1fc3e. It removes the part of the commit that forces status codes out of range to be converted to UNKNOWN, however.

dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Sep 25, 2023

dfawley added this to the 1.59 Release milestone Sep 25, 2023

dfawley requested a review from easwars September 25, 2023 20:54

dfawley assigned easwars Sep 25, 2023

ejona86 mentioned this pull request Sep 25, 2023

"The implementations of gRPC in Java and Golang differ in terms of the Status.Code." grpc/grpc-java#10568

Closed

dfawley force-pushed the status branch from d0a7967 to a16a83c Compare September 29, 2023 16:39

easwars reviewed Sep 29, 2023

View reviewed changes

internal/transport/http2_server.go Show resolved Hide resolved

internal/status/status.go Show resolved Hide resolved

internal/status/status.go Show resolved Hide resolved

internal/status/status.go Show resolved Hide resolved

internal/status/status.go Show resolved Hide resolved

easwars reviewed Sep 29, 2023

View reviewed changes

easwars assigned dfawley and unassigned easwars Sep 29, 2023

review comments

1804795

dfawley assigned easwars and unassigned dfawley Sep 29, 2023

easwars reviewed Sep 29, 2023

View reviewed changes

easwars assigned dfawley and unassigned easwars Sep 29, 2023

review comments

f3cad42

dfawley assigned easwars and unassigned dfawley Sep 29, 2023

oops

ac60aab

easwars approved these changes Sep 29, 2023

View reviewed changes

easwars assigned dfawley and unassigned easwars Sep 29, 2023

dfawley merged commit 0772ed7 into grpc:master Oct 2, 2023
10 checks passed

dfawley deleted the status branch October 2, 2023 16:54

dfawley added a commit to dfawley/grpc-go that referenced this pull request Oct 2, 2023

Revert "status: fix/improve status handling (grpc#6662)"

660603e

This reverts commit 0772ed7.

dfawley added a commit that referenced this pull request Oct 2, 2023

Revert "status: fix/improve status handling (#6662)" (#6673)

9e1fc3e

dfawley added a commit to dfawley/grpc-go that referenced this pull request Oct 2, 2023

Reapply "status: fix/improve status handling (grpc#6662)" (grpc#6673)

9774dcd

This reverts commit 9e1fc3e.

dfawley added a commit that referenced this pull request Oct 5, 2023

Reapply "status: fix/improve status handling (#6662)" (#6673) (#6688)

e3f1514

dfawley mentioned this pull request Oct 17, 2023

Revert "status: fix/improve status handling (#6662)" #6673

Merged

akshayjshah mentioned this pull request Oct 20, 2023

Document grpc-status-details-bin grpc/grpc#24007

Closed

This was referenced Nov 7, 2023

runtime: Add outgoing trailer matching grpc-ecosystem/grpc-gateway#3725

Merged

Do not expose gRPC trailers in the gRPC bridge TheThingsNetwork/lorawan-stack#6690

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

status: fix/improve status handling #6662

status: fix/improve status handling #6662

dfawley commented Sep 25, 2023 •

edited

Loading

easwars Sep 29, 2023

dfawley Sep 29, 2023

easwars Sep 29, 2023

dfawley Sep 29, 2023

easwars Sep 29, 2023

easwars Sep 29, 2023

easwars Sep 29, 2023

dfawley Oct 2, 2023

easwars Sep 29, 2023

easwars Sep 29, 2023

easwars Sep 29, 2023

easwars Sep 29, 2023

status: fix/improve status handling #6662

status: fix/improve status handling #6662

Conversation

dfawley commented Sep 25, 2023 • edited Loading

API-layer:

Client-side:

Server-side:

Internal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Sep 25, 2023 •

edited

Loading