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
+116 −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 rauanmayemir force-pushed the feature/gwapi-alpn branch 2 times, most recently from be516f6 to b293cd5 Compare May 13, 2024 13:44
@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?

@youngnick
Copy link
Contributor

/test

youngnick
youngnick requested changes May 27, 2024
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment •
edited

Choose a reason for hiding this comment

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

I think the code is great, but we need to clarify the requirements and behavior a bit more. Any chance you'd be up to write a small tutorial demonstrating how the feature would work? If not, I think we at least need a similar change to what I've asked for below.

install/kubernetes/cilium/values.yaml.tmpl Outdated
@@ -812,6 +812,8 @@ gatewayAPI:
enableProxyProtocol: false
# -- Enable Backend Protocol selection support (GEP-1911) for Gateway API via appProtocol.
enableAppProtocol: false
# -- Enable ALPN for Gateway API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# -- Enable ALPN for Gateway API.
# -- Enable ALPN for all listeners configured with Gateway API. ALPN will attempt HTTP/2, then HTTP 1.1. Note that this will also enable `appProtocol` support, and services that wish to use HTTP/2 will need to indicate that via their `appProtocol`.

Maybe just to make this one hundred percent clear? Ideally, we would add a tutorial to Documentation/network/servicemesh/gateway-api/ as well walking through what's required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we leave the documentation part for a different PR? I would like to actually run this in my env before writing any tutorials. 😅

Will add the detailed description for the helm values.

@rauanmayemir
gateway-api: ALPN support
8ea0fc8 
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 <rauan@mayemir.io>
@rauanmayemir
helm: add .Values.gatewayAPI.enableAlpn
449bb6b 
`enable-gateway-api-alpn` flag is false by default, setting it explicitly will enable ALPN support alongside appProtocol.

Signed-off-by: Rauan Mayemir <rauan@mayemir.io>
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 youngnick requested changes

Requested changes must be addressed 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