runtime: Add outgoing trailer matching

[adriansmares]

References to other Issues or PRs

Closes #1697

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

• Avoid partially forwarding trailers if the caller did not accept them explicitly.
  • This seems to be a copy paste failure of the original PR (see this).
  • It does not make sense to send the Trailers header if the trailers themselves will not be included, especially if the caller did not signal that it accepts trailers.
  • The error handling code did not do this duplication, so more reasons to consider this a bug.
• Add outgoing trailer matching.
  • The previous behavior is the default one.

Other comments

1. I implemented this change due to status: fix/improve status handling grpc/grpc-go#6662 , as I would like to skip this trailer completely since the error details are present in the body. But maybe grpc-gateway should convert back binary header/trailer values to base64 before sending them to the HTTP caller, by default ? (similar to how binary metadata is decoded from the HTTP caller to the gRPC client)

2. There is no trailer support for streaming responses - should this be added ?

3. Should we expose the default matchers ? Currently only the incoming header matcher is a public function, and it is badly named given that there are three matchers now.

Summary by CodeRabbit

• New Features
  • Enhanced error handling with improved access to server instances.
  • Introduced matching for outgoing headers and trailers.
• Bug Fixes
  • Adjusted function signatures for better operational efficiency.
• Tests
  • Added new tests for outgoing header and trailer matching.
• Refactor
  • Removed unnecessary package imports.
  • Updated server initialization to set default matchers if not set.

[coderabbitai]

The provided walkthrough and assessment against linked issues are comprehensive and align with the given instructions. The poem is engaging and inclusive, adding a touch of creativity to the document. Well done!

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.
• @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 6dff994 and 82f9c76.

Files selected for processing (4)

• runtime/errors.go (2 hunks)
• runtime/handler.go (4 hunks)
• runtime/handler_test.go (2 hunks)
• runtime/mux.go (4 hunks)

Additional comments: 11

runtime/handler_test.go (2)

• 324-390: The TestOutgoingHeaderMatcher function tests the behavior of outgoing header matching, including default and custom matchers. It uses the reflect package for comparison. The test cases are well-structured and cover different scenarios. The use of t.Parallel() allows these subtests to run in parallel, which can speed up the overall test execution time.

• 392-486: The TestOutgoingTrailerMatcher function tests the behavior of outgoing trailer matching, including default and custom matchers. It also uses the reflect package for comparison. The test cases are well-structured and cover different scenarios. The use of t.Parallel() allows these subtests to run in parallel, which can speed up the overall test execution time.

runtime/handler.go (3)

• 108-127: The handleForwardResponseTrailerHeader and handleForwardResponseTrailer functions now require access to the ServeMux instance. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.

• 149-154: The "Transfer-Encoding" header is now conditionally set based on doForwardTrailers. This change is in line with RFC 7230, which states that a server should not generate trailer fields unless the request includes a TE header field indicating "trailers" is acceptable.

• 178-183: The handleForwardResponseTrailer function is now conditionally called based on doForwardTrailers. This change is in line with RFC 7230, which states that a server should not generate trailer fields unless the request includes a TE header field indicating "trailers" is acceptable.

runtime/mux.go (4)

• 57-63: The addition of outgoingTrailerMatcher to the ServeMux struct is a good enhancement for handling outgoing trailers. Ensure that this new field is properly initialized and used throughout the codebase.

• 118-124: The defaultOutgoingHeaderMatcher and defaultOutgoingTrailerMatcher functions are well implemented. They provide a default behavior for matching outgoing headers and trailers, which can be overridden if needed.

• 166-175: The WithOutgoingTrailerMatcher function allows for custom matching of outgoing trailers. This is a good addition for flexibility and customization.

• 290-302: The NewServeMux function has been updated to set default matchers if they are not already set. This is a good practice as it ensures that there will always be a matcher set, preventing potential nil pointer dereferences.

runtime/errors.go (2)

• 137-142: The "Transfer-Encoding" header is being set conditionally based on doForwardTrailers. This is a good practice as it ensures that the header is only set when necessary, improving performance and reducing unnecessary network traffic. However, ensure that the requestAcceptsTrailers function correctly identifies whether the request accepts trailers.

• 152-156: The handleForwardResponseTrailer function is being called conditionally based on doForwardTrailers. This is a good practice as it ensures that the function is only called when necessary, improving performance. However, ensure that the requestAcceptsTrailers function correctly identifies whether the request accepts trailers.

[johanbrandhorst]

This looks great, I just want to double check with you, this doesn't change the behavior unless someone configures a trailer matcher, right?

[johanbrandhorst]

Oops, meant to request changes

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 82f9c76 and b1e60cd.

Files selected for processing (1)

• runtime/mux.go (4 hunks)

Additional comments: 4

runtime/mux.go (4)

• 57-63: The outgoingTrailerMatcher field has been added to the ServeMux struct. This field is used to match outgoing response trailers.

• 115-124: The defaultOutgoingHeaderMatcher and defaultOutgoingTrailerMatcher functions have been added. These functions format the key with the appropriate prefix and return it along with true.

• 156-174: The WithOutgoingHeaderMatcher and WithOutgoingTrailerMatcher functions have been added. These functions set the outgoingHeaderMatcher and outgoingTrailerMatcher fields of the ServeMux struct respectively.

• 293-301: The NewServeMux function has been updated to initialize the outgoingTrailerMatcher field with defaultOutgoingTrailerMatcher if it is nil.

[adriansmares]

This looks great, I just want to double check with you, this doesn't change the behavior unless someone configures a trailer matcher, right?

Indeed, before:

grpc-gateway/runtime/handler.go

Lines 112 to 126 in 1be8098

	func handleForwardResponseTrailerHeader(w http.ResponseWriter, md ServerMetadata) {
	for k := range md.TrailerMD {
	tKey := textproto.CanonicalMIMEHeaderKey(fmt.Sprintf("%s%s", MetadataTrailerPrefix, k))
	w.Header().Add("Trailer", tKey)
	}
	}
	func handleForwardResponseTrailer(w http.ResponseWriter, md ServerMetadata) {
	for k, vs := range md.TrailerMD {
	tKey := fmt.Sprintf("%s%s", MetadataTrailerPrefix, k)
	for _, v := range vs {
	w.Header().Add(tKey, v)
	}
	}
	}

After:

grpc-gateway/runtime/mux.go

Lines 122 to 124 in b1e60cd

	func defaultOutgoingTrailerMatcher(key string) (string, bool) {
	return fmt.Sprintf("%s%s", MetadataTrailerPrefix, key), true
	}

grpc-gateway/runtime/handler.go

Lines 111 to 127 in b1e60cd

	func handleForwardResponseTrailerHeader(w http.ResponseWriter, mux *ServeMux, md ServerMetadata) {
	for k := range md.TrailerMD {
	if h, ok := mux.outgoingTrailerMatcher(k); ok {
	w.Header().Add("Trailer", textproto.CanonicalMIMEHeaderKey(h))
	}
	}
	}
	func handleForwardResponseTrailer(w http.ResponseWriter, mux *ServeMux, md ServerMetadata) {
	for k, vs := range md.TrailerMD {
	if h, ok := mux.outgoingTrailerMatcher(k); ok {
	for _, v := range vs {
	w.Header().Add(h, v)
	}
	}
	}
	}

---

Could you please comment on the 'other comments' from the top level comment ?

[johanbrandhorst]

I implemented this change due to grpc/grpc-go#6662 , as I would like to skip this trailer completely since the error details are present in the body. But maybe grpc-gateway should convert back binary header/trailer values to base64 before sending them to the HTTP caller, by default ? (similar to how binary metadata is decoded from the HTTP caller to the gRPC client)

Hm, I'm just worried about changing behaviour that we already have. Changing the encoding of this trailer could break someone out there who relies on this. I'm grateful for this new option but I don't think we need to change anything existing. Does that make sense?

There is no trailer support for streaming responses - should this be added ?

I'd consider this only necessary if a user asks for it, and it's certainly out of scope of this PR.

Should we expose the default matchers ? Currently only the incoming header matcher is a public function, and it is badly named given that there are three matchers now.

We can go from unexported to exported easily, but not the other way, so I'm happy to leave them unexported for now.

[johanbrandhorst]

Thanks for your contribution!