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

gateway-api: ALPN support #32486

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

gateway-api: ALPN support #32486

wants to merge 2 commits into from
+114 −1

Conversation

rauanmayemir
Copy link
Contributor

This feature is hidden behind enable-gateway-api-alpn flag on the operator gwapi cell.
The implementation will change envoy listener configuration to expose ALPN suggesting both HTTP/2 and HTTP/1.1.

Fixes: #30794

@rauanmayemir rauanmayemir requested a review from a team as a code owner May 11, 2024 14:53
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 11, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 11, 2024
@rauanmayemir rauanmayemir requested review from a team as code owners May 11, 2024 15:29
@sayboras sayboras added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. feature/k8s-gateway-api labels May 13, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 13, 2024
@sayboras
Copy link
Member

/test

sayboras
sayboras reviewed May 13, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

One comment as per below, the rest seems reasonable to me. Thanks for your help.

operator/pkg/model/translation/cec_translator.go Outdated Show resolved Hide resolved
@rauanmayemir
gateway-api: ALPN support
e24e0de 
This feature is hidden behind `enable-gateway-api-alpn` flag on the operator gwapi cell. The implementation will change envoy listener configuration to expose ALPN suggesting both HTTP/2 and HTTP/1.1.

Fixes: cilium#30794

Signed-off-by: Rauan Mayemir <[email protected]>
@rauanmayemir
helm: add .Values.gatewayAPI.enableAlpn
b293cd5 
`enable-gateway-api-alpn` flag is false by default, setting it explicitly will enable ALPN support alongside appProtocol.

Signed-off-by: Rauan Mayemir <[email protected]>
@youngnick
Copy link
Contributor

The code itself looks good, but I think we should spend some time thinking about what this means for users.

Could you walk through some scenarios for me?

What happens when:

  • someone turns this on and is doing other TLS connections over HTTP2?
  • someone turns this on and is doing other TLS connections over HTTP1.1 (only)?

Bascially, what I'm trying to think of here are situations where we should call out in the docs how this could break people before they enable it.

@rauanmayemir
Copy link
Contributor Author

rauanmayemir commented May 16, 2024 •
edited

@youngnick enabling ALPN doesn't change anything for connections explicitly specifying their protocols, it only plays if client asks for available ALPN protocols (i.e initiates negotiation).

With ALPN exposed, you can exec curl commands with either of --http1.1, --http2 or --http2-prior-knowledge, commands will work fine.

It could have broken clients if I negotiated http1.1, but my backend only serves http2. (I think this would have been the case with cilium anyway)
But if you check the helm chart values, enabling ALPN will also enable appProtocol support which will opt to use HTTP1.1 by default for upstreams.

We can call out in the docs that if you enable ALPN, it will also enable appProtocol, so if one's upstream is serving HTTP2, they should specify appProtocol: kubernetes.io/h2c on their Service specs.

@rauanmayemir
Copy link
Contributor Author

someone turns this on and is doing other TLS connections over HTTP2?

If their backend is only serving HTTP2 and they don't specify appProtocol: kubernetes.io/h2c and they enable ALPN (enabling appProtocol alongside), their requests will fail with 502 protocol error.

(unless they're using GRPCRoute which will force HTTP2 on the upstream whether you have appProtocol or not)

someone turns this on and is doing other TLS connections over HTTP1.1 (only)?

Nothing will break.

@rauanmayemir
Copy link
Contributor Author

If their backend is only serving HTTP2 and they don't specify appProtocol: kubernetes.io/h2c and they enable ALPN (enabling appProtocol alongside), their requests will fail with 502 protocol error.

Here, I was talking about hypothetical protocol negotiating scenario. ALPN doesn't change much for this case, because what breaks the traffic flow is enabling appProtocol, not ALPN.

@rauanmayemir
Copy link
Contributor Author

@youngnick Anything else we need for this to go in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sayboras sayboras sayboras left review comments

@youngnick youngnick Awaiting requested review from youngnick youngnick is a code owner automatically assigned from cilium/sig-servicemesh

At least 0 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
feature/k8s-gateway-api kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Gateway API - add ALPN negotiation to cilium-envoy for gRPC support
3 participants
@rauanmayemir @sayboras @youngnick