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.

Sign up for GitHub

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

Jump to bottom

forward trailers on error only if client send TE header #1499

Merged
merged 4 commits into from
Jul 6, 2020
Merged

forward trailers on error only if client send TE header #1499

merged 4 commits into from
Jul 6, 2020

Conversation

strobil
Copy link

@strobil strobil commented Jul 3, 2020 •
edited
Loading

This PR fixes linkerd/linkerd2#4714 and fixes hyperium/hyper#2171
According to RFC7230, server should send trailers only if client send TE header hyperium/hyper#2171 (comment)

@johanbrandhorst
Copy link
Collaborator

Hi Mikhail, thanks for your PR! This looks really great, and of course we should abide by the relevant standards. Could you add some comments to the code to the relevant sections of the RFC, just so we have it in the code as well? It also looks like we may need to add the TE: trailers header to some of our integration tests.

Thanks!

@strobil
Copy link
Author

strobil commented Jul 4, 2020 •
edited
Loading

Hello Johan!
I've added a comment to the code. Could you please check, Is it ok?

johanbrandhorst
johanbrandhorst requested changes Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It still looks like we need to change the integration tests to account for this change. Could you please add the TE header to the tests that are failing?

runtime/errors.go Outdated
Comment on lines 158 to 162
//RFC 7230 https://tools.ietf.org/html/rfc7230#section-4.1.2
//Unless the request includes a TE header field indicating "trailers"
//is acceptable, as described in Section 4.3, a server SHOULD NOT
//generate trailer fields that it believes are necessary for the user
//agent to receive.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: please add a space after the // since that is the convention.

Copy link
Author

Choose a reason for hiding this comment

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

Done ;)

@strobil
Copy link
Author

strobil commented Jul 6, 2020 •
edited
Loading

It still looks like we need to change the integration tests to account for this change. Could you please add the TE header to the tests that are failing?

Done! Maybe not the best solution, but it works as expected :)

@strobil strobil requested a review from johanbrandhorst July 6, 2020 19:44
@googlebot
Copy link

☹️ Sorry, but only Googlers may change the label cla: yes.

@johanbrandhorst
Copy link
Collaborator

Hm, looks like the CLA bot is stuck... could you try to prompt it by saying @googlebot I signed it!?

@johanbrandhorst
Copy link
Collaborator

NVM there we go!

@johanbrandhorst johanbrandhorst merged commit 80e6ce6 into grpc-ecosystem:master Jul 6, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution! Could you please cherry pick this against the v2 branch?

@strobil strobil mentioned this pull request Jul 6, 2020
Merged
@strobil
Copy link
Author

strobil commented Jul 6, 2020 •
edited
Loading

Thanks for your contribution! Could you please cherry pick this against the v2 branch?

Done! #1507

@zoido zoido mentioned this pull request May 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanbrandhorst johanbrandhorst johanbrandhorst approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
3 participants
@strobil @johanbrandhorst @googlebot