V1: Set request info into error details for error responses #676

smaye81 · 2023-11-18T01:38:09Z

This updates a testing rule that server implementations should append a RequestInfo message into error details for the following conditions:

for unary and client-streaming tests that return an error
for server- and bidi-streaming tests that return no responses and only an error (i.e. trailers-only)

If the client specified any error details to be returned, the server should append the RequestInfo to those details.

smaye81 · 2023-11-18T01:39:35Z

internal/app/connectconformance/results.go

 		// TODO: Should this be more lenient? Are we okay with details getting re-ordered?
 		//       An alternative might be to create a map keyed by type, and for each type
 		//       remove expected messages as they are matched against actual ones.


I'm actually wondering whether we care about anything at all other than ConformancePayload? From a testing standpoint, we really only care that the server can support error details and the client can read what the server sent. Setting the ConformancePayload into error details for unary errors should actually cover that case maybe?

We also care about how unprocessable messages in an Any are handled, like if the server gets an Any it doesn't understand from a remote server and then must include it in the error details. This mainly impacts the Connect protocol, since it tries to parse the Any to produce the debug info.

Also, I'm not sure it's ConformancePayload we care about. I think it's only RequestInfo. The ConformancePayload also includes a data field, that is only used with real responses (when the response definition indicates the data it should return).

Yeah I actually started out with just RequestInfo, but it was easier to do the ConformancePayload because the checkPayloads function already existed. But, I split that out into a checkRequestInfo function and went with RequestInfo instead.

smaye81 · 2023-11-18T01:41:11Z

internal/app/connectconformance/testsuites/basic.yaml

@@ -103,11 +103,6 @@ testCases:
        error:
          code: 13
          message: "server stream failed"
-          details:


Removed from all but grpc-only.yaml because that file throws the following when details are removed:

could not write request: client closed stdin

Will fix in follow-up PR.

Figured out this issue for the follow-up PR, but probably needs a discussion. I'll post it once this one is hashed out.

I'm pretty sure that error means that the client program crashed.

See metadata.go. The issue was trying to write grpc-status-details-bin to stdout.

internal/app/connectconformance/results.go

internal/app/connectconformance/test_case_library.go

jhump · 2023-11-20T14:22:32Z

internal/app/grpcserver/impl.go

@@ -303,6 +303,18 @@ func parseUnaryResponseDefinition(
 	if def != nil {
 		switch respType := def.Response.(type) {
 		case *v1.UnaryResponseDefinition_Error:
+			// If error details were not provided to be returned, then the server
+			// should set the conformance payload as the error details for unary responses
+			if respType.Error.Details == nil {


It's often an anti-pattern to compare slices to nil. Rarely do you really intend to distinguish between nil and an empty slice. So it's safest to use len(respType.Error.Details) == 0.

Also, why do this only when error details is nil? I think we want to add this to the details, even if non-nil.

proto/connectrpc/conformance/v1/service.proto

smaye81 · 2023-11-20T19:57:37Z

internal/grpcutil/metadata.go

@@ -28,6 +28,11 @@ func ConvertMetadataToProtoHeader(
 ) []*v1.Header {
 	headerInfo := make([]*v1.Header, 0, len(src))
 	for key, value := range src {
+		// Omit grpc-status-details-bin from the response trailers reported


This fixes the client closed stdin problem because the problem results from trying to write grpc-status-details-bin base64 encoded data to the output (not entirely sure why -- bc of utf-8? buffer length?). But is this the right solution? Doing this removes anything grpc specific from the headers returned to the test. But maybe that's fine bc connect has already tried to parse the grpc stuff by now, so as long as we know the error that grpc returned by now we can discard anything we used to glean that information.

We sort of have to do this here bc it needs to be omitted before we send out the response, but thats probably
ok as long as we know the status and details by now.

This is very confusing. I don't think we should have to do this, and it would be even more confusing if we suggested that other clients-under-test or server-under-test should also do this.

I think we need to identify the real root cause of why this was causing the problem and (hopefully) resolve in a different way.

Having said that, I don't think we need to block this PR. Let's add a TODO to investigate further and remove this when the issue is more clearly understood.

smaye81 · 2023-11-20T21:13:45Z

@jhump I was actually going to do a follow-up PR that fixed the client closed stdin error and also update the server-stream responses to set error detail also, but I just moved it all to this PR since it's easier to get a holistic view of the changes. Lmk what you think.

jhump

Let's add a TODO about that grpc-details-bin thing. Otherwise, LGTM

jhump · 2023-11-20T22:20:25Z

internal/app/connectconformance/results.go

-				errs = append(errs, fmt.Errorf("request #%d: did not survive round-trip: - wanted, + got\n%s", reqNum, diff))
-			}
-		}
+		errs = append(errs, checkRequestInfo(expectedPayload.GetRequestInfo(), actualPayload.GetRequestInfo(), i)...)


Instead of passing i, I think you can check it here:

if i == 0 { // Validate headers, timeout, and query params. We only need to do this once, for first response. errs = append(errs, checkRequestInfo(expectedPayload.GetRequestInfo(), actualPayload.GetRequestInfo())...) }

That way you can omit the hard-coded zero in the other call site.

The only problem is that value is also used on line 347 if request lengths don't match:

errs = append(errs, fmt.Errorf("response #%d: expecting %d request messages to be described but instead got %d", respNum+1, len(expected.GetRequests()), len(actual.GetRequests())))

I just removed the number from that fmt.Errorf statement. It probably doesn't provide a ton of info when weighed against having to pass the number in as a param.

jhump · 2023-11-20T22:25:45Z

internal/grpcutil/metadata.go

@@ -28,6 +28,11 @@ func ConvertMetadataToProtoHeader(
 ) []*v1.Header {
 	headerInfo := make([]*v1.Header, 0, len(src))
 	for key, value := range src {
+		// Omit grpc-status-details-bin from the response trailers reported


Having said that, I don't think we need to block this PR. Let's add a TODO to investigate further and remove this when the issue is more clearly understood.

jhump · 2023-11-20T23:22:20Z

internal/app/connectconformance/results.go

@@ -379,7 +377,9 @@ func checkPayloads(expected, actual []*conformancev1.ConformancePayload) multiEr
 			errs = append(errs, fmt.Errorf("response #%d: expecting data %x, got %x", i+1, expectedPayload.Data, actualPayload.Data))
 		}

-		errs = append(errs, checkRequestInfo(expectedPayload.GetRequestInfo(), actualPayload.GetRequestInfo(), i)...)
+		if i == 0 { //nolint:nestif


I just realized that my suggestion changed the functionality here. It's only part of checkRequestInfo that we skip when i != 0 -- the headers, timeout, and query params.

For that, I think checkRequestInfo should have a checkHeaders bool param. This call site can pass i == 0 for that param, and the other call site can always provide true.

smaye81 added 4 commits November 17, 2023 15:13

Details

1a2c9e1

Details

dd7294d

Error Details

92075c6

Revert

4d6fc1e

smaye81 commented Nov 18, 2023

View reviewed changes

smaye81 requested a review from jhump November 20, 2023 00:25

jhump reviewed Nov 20, 2023

View reviewed changes

jhump changed the title ~~V2 (V1?): Set conformance payload into error details for unary responses~~ V1: Set conformance payload into error details for unary responses Nov 20, 2023

smaye81 added 3 commits November 20, 2023 14:45

Feedback

a60db90

Feedback

bfcb0ad

Metadata

bb7cc3a

smaye81 commented Nov 20, 2023

View reviewed changes

smaye81 requested a review from jhump November 20, 2023 21:13

smaye81 changed the title ~~V1: Set conformance payload into error details for unary responses~~ V1: Set request info into error details for error responses Nov 20, 2023

jhump approved these changes Nov 20, 2023

View reviewed changes

smaye81 added 2 commits November 20, 2023 17:54

TODO comment

583af83

Remove number from printout

9c9735d

jhump reviewed Nov 20, 2023

View reviewed changes

smaye81 and others added 2 commits November 20, 2023 21:05

Verify headers boolean

9709518

Merge branch 'v1' into sayers/error_details

1a1d6ec

smaye81 merged commit b9a2afd into v1 Nov 21, 2023

smaye81 deleted the sayers/error_details branch November 21, 2023 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V1: Set request info into error details for error responses #676

V1: Set request info into error details for error responses #676

smaye81 commented Nov 18, 2023 •

edited

Loading

smaye81 Nov 18, 2023

jhump Nov 20, 2023

smaye81 Nov 20, 2023

smaye81 Nov 18, 2023

smaye81 Nov 20, 2023

jhump Nov 20, 2023

smaye81 Nov 20, 2023

jhump Nov 20, 2023

smaye81 Nov 20, 2023 •

edited

Loading

jhump Nov 20, 2023

jhump Nov 20, 2023

smaye81 commented Nov 20, 2023

jhump left a comment

jhump Nov 20, 2023

smaye81 Nov 20, 2023

smaye81 Nov 20, 2023

jhump Nov 20, 2023

jhump Nov 20, 2023

V1: Set request info into error details for error responses #676

V1: Set request info into error details for error responses #676

Conversation

smaye81 commented Nov 18, 2023 • edited Loading

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

smaye81 Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaye81 commented Nov 20, 2023

jhump left a 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

smaye81 commented Nov 18, 2023 •

edited

Loading

smaye81 Nov 20, 2023 •

edited

Loading