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.

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

Do not expose gRPC trailers in the gRPC bridge #6690

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Nov 8, 2023

Summary

References grpc/grpc-go#6662
References grpc-ecosystem/grpc-gateway#3725

This PR stops our RPCs from propagating trailers from the gRPC calls to the HTTP clients. It does so because we don't use trailers in general, and it would be better to do it on a whitelist basis anyway.

Second, it fixes a bug caused by the above linked grpc-go change, which adds a binary-valued trailer to RPCs which have errored out. The binary value makes some callers (specifically, Firefox) to timeout the request. Generally this a grpc-gateway issue (probably the header should be re-encoded as base64, which is the inbound behavior anyway), but we don't have any reason to propagate the error header anyway, since the error details are provided in the response body.

Changes

  • Use a fork of grpc-gateway until the changes are accepted upstream.
  • Avoid forwarding all trailers from the gRPC call to the HTTP caller.

Testing

Local testing. Basically any API call done from Firefox which fails will cause this issue.
There are no special steps required to test this issue - if you can login into the Console, and go to the gateways page and see that a gateway is disconnected, you're good to go.

Regressions

This new filtering policy will stop any trailers from being propagated. Fundamentally we don't use trailers in our RPC server implementations, so I am optimistic that we're not regressing anything.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • The steps/process to test this feature are clearly explained including testing for regressions.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@adriansmares adriansmares added the bug Something isn't working label Nov 8, 2023
@adriansmares adriansmares added this to the v3.28.1 milestone Nov 8, 2023
@adriansmares adriansmares self-assigned this Nov 8, 2023
@github-actions github-actions bot added dependencies Pull requests that update a dependency file tooling Development tooling labels Nov 8, 2023
@adriansmares adriansmares force-pushed the feature/use-grpc-gateway-fork branch from c3bb8ba to 79ced9c Compare November 8, 2023 11:21
@adriansmares adriansmares marked this pull request as ready for review November 8, 2023 11:43
@adriansmares adriansmares merged commit 57eafd1 into v3.28 Nov 9, 2023
16 checks passed
@adriansmares adriansmares deleted the feature/use-grpc-gateway-fork branch November 9, 2023 09:12
@johanstokking
Copy link
Member

grpc-ecosystem/grpc-gateway#3725 has been merged, we can remove the replace directive from our go.mod right?

@adriansmares adriansmares mentioned this pull request Nov 20, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file tooling Development tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants