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

Add http2 and grpc support to ingress gateways #8458

Merged

Conversation

jackloughran
Copy link
Contributor

While evaluating a Consul + Envoy request routing solution, I noticed ingress gateways don't support grpc (#8454). We were hoping to route grpc traffic though an ingress gateway during the first phase of our migration.

Out of curiosity, I tried adding grpc to the ingress gateway supported protocols map, and Consul was able to correctly configure the Envoy ingress gateway. From what I can tell, ingress gateways share a lot of code with sidecar proxies which already support grpc.

Could this work? Is there another reason why ingress gateways don't support grpc?

I'm happy to expand on our use case if that would help.

@jackloughran
Copy link
Contributor Author

Any updates on this? Is there any more information I can give you?

@blake

@freddygv
Copy link
Contributor

freddygv commented Aug 17, 2020

Hi @jackloughran, you are correct that grpc/http2 is supported for sidecar proxies.

The only reason why gRPC wasn't supported for ingress gateways initially is that it hasn't been tested. Have you tested sending gRPC traffic through the gateway with the changes in this PR?

@jackloughran
Copy link
Contributor Author

@freddygv Yes, I tried this out, and I was able to successfully proxy grpc traffic on my computer with the changes in this PR.

@freddygv
Copy link
Contributor

Great, so then I think the one thing missing here would be a new integration test for an Ingress gateways routing to a grpc service.

It would be a mix of:
https://github.com/hashicorp/consul/tree/master/test/integration/connect/envoy/case-grpc
and
https://github.com/hashicorp/consul/blob/master/test/integration/connect/envoy/case-ingress-gateway-simple

Here's some additional info on that:

  • To run specific test cases you can do: env GOFLAGS="-run=TestEnvoy/case-grpc" make test-envoy-integ where case-grpc would be replaced with the case you want to run.
  • When the test case directory is created, then it needs to be added to this function in order to run: https://github.com/hashicorp/consul/blob/master/test/integration/connect/envoy/main_test.go
  • Relevant files to update after copying case-ingress-gateway-simple:
    • config_entries.hcl defines configuration entries to use during the test. You will need to update the gateway's config entry so that the protocol for s1 is grpc (rather than tcp)
    • verify.bats defines the assertions for the test. You will need to update the assertion for connectivity through the gateway to use grpcping like here rather than curl as it [currently does]

@jackloughran jackloughran force-pushed the add-grpc-support-to-ingress-gateway branch from a6a6933 to c2b7ee8 Compare August 24, 2020 15:33
@jackloughran
Copy link
Contributor Author

@freddygv Thanks so much for writing that out! It was really helpful. I added the integration test, and I added http2 to the list of supported protocols since grpc always uses http2.

https://github.com/hashicorp/consul/blob/master/test/integration/connect/envoy/case-grpc/verify.bats#L31

@freddygv
Copy link
Contributor

Great, thanks @jackloughran! This looks good.

One last request: We recently moved to automating changelog entries, can you add a file like this one to ./changelog where the filename is the PR number?

https://github.com/hashicorp/consul/blob/master/.changelog/7628.txt

This would also be considered as an improvement, like 7628.

@jackloughran
Copy link
Contributor Author

@freddygv Sure! Thanks again for all the help!

Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Thanks you!

@freddygv freddygv changed the title Add grpc support to ingress gateway Add http2 and grpc support to ingress gateways Aug 27, 2020
@freddygv freddygv merged commit 9e1c672 into hashicorp:master Aug 27, 2020
@hashicorp-ci
Copy link
Contributor

🍒❌ Cherry pick of commit 9e1c672 onto release/1.8.x failed! Build Log

@freddygv
Copy link
Contributor

I'll take care of fixing up this cherry-pick @jackloughran. You're all set here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants