added trailer match capability #1709

[ghost]

Related to #1697

Fixes #1697
(hopefully)

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[ghost]

@googlebot

I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[johanbrandhorst]

Thanks for this PR! It seems that you're missing the logic for actually using the new field to filter the trailers. We'll need to add this somewhere in the ServeMux logic, I suggest looking at how outgoingHeaderMatcher is used.

Also I'd like a test for this before we can merge it. Again I suggest looking at how outgoingHeaderMatcher is tested.

Let me know if you need anymore pointers!

[ghost]

I am unsure of how to test the handler. Should I modify the TestForwardResponseMessage func to include tests with trailers in handler_test.go? Or append a spec to the TestMuxServeHTTP func in mux_test.go? I'm assuming the latter, even though the changes were made in handler.go.

[johanbrandhorst]

I am unsure of how to test the handler. Should I modify the TestForwardResponseMessage func to include tests with trailers in handler_test.go? Or append a spec to the TestMuxServeHTTP func in mux_test.go? I'm assuming the latter, even though the changes were made in handler.go.

Sorry if I wasn't clear. I thought we had some tests for outgoingHeaderMatcher but it doesn't look like it 😂. I think these tests would probably live in handler_test, maybe in a new test that can test the functionality of the outgoing trailer matcher. You can copy one of the existing tests and play around with it until it does what you want, but at the very least we should have a test without the trailer matcher set, and another test with a custom trailer matcher and check that the behaviour changes as expected.

Does that sound okay?

[ghost]

Got it. No worries. I didn't add the logic at first because I mistakenly thought the implementation of the interface would be done by the client, but even back then I still felt like I was incorrect. I will continue to work on this until it is up to standard.

[ghost]

Still unsure of how to test in handler_test.go, the only test headers I see are in mux_test.go.I feel like both test files need to be modified in order to test this functionality. The modified handler was the non-exported func handleForwardResponseServerMetadata(w http.ResponseWriter, mux *ServeMux, md ServerMetadata)

[ghost]

The CI tests pass because if there is no specified outgoingTrailerMatcher, "TE" will be passed. The original issue wanted to be agnostic to whether or not "TE" was passed.
However, gRPC trailers will be forwarded by default, so I believe this is in conflict with #1709

[johanbrandhorst]

I'm not sure what you're trying to say. There are two different concerns here:

• Trailers should not be sent unless the "TE" header is sent in the request. This is Allow client to disable sending of trailers by omitting "TE" header. #1709.
• There should be a way for users to customize how to return headers. This is Support WithOutgoingTrailerMatcher #1697.

These are separate concerns, there is no conflict between the two. This PR should tackle #1697. Don't worry about the other issue for the purposes of this PR.

Re: testing, I think you should be able to copy one of the tests in handler_test.go and adjust it so that it can test the behaviour of the outgoingTrailerMatcher. Have you tried doing this?

[ghost]

I am not necessarily saying there is a conflict between the concerns. I'm saying that the way I resolved issue #1697 the TE" header is set just returns "Grpc-Trailer-TE" and true by default, with or without values, similar to outgoingHeaderMatcher. Not sure if what I did is the correct approach, but it is why it passes the CI tests. I apologize for the confusion, I am still working on it.

Ok. I'll spend more time attempting to modify the handler_test.go.

Also, If you have any tips for me to improve my communication, please let me know. I am gladly willing to explain my current understanding of the problem, no matter how incorrect it may be, so that we can solve the issue. I took this on thinking it was OK for first timers.
The reason why I brought up the fact that the modified function is non exported is because current tests check Forwarded Streams and Messages, which call it internally. Since it didn't break those tests, and outgoingHeaderMatcher was not tested prior to this, I thought the tests that cover this are written somewhere outside of handler_test.go

[johanbrandhorst]

Hi, could you please rebase this on master? We've merged v2 into master.

[johanbrandhorst]

Are you still intending to merge this? Otherwise I will close it soon.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.