-
Notifications
You must be signed in to change notification settings - Fork 126
Add support for backendref service appProtocol #3511
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3511 +/- ##
==========================================
+ Coverage 86.78% 86.81% +0.03%
==========================================
Files 127 127
Lines 15079 15169 +90
Branches 62 62
==========================================
+ Hits 13086 13169 +83
- Misses 1841 1848 +7
Partials 152 152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3ef2f04
to
3043d95
Compare
Still debugging one conformance test |
@@ -12,7 +12,7 @@ GW_SERVICE_TYPE = NodePort## Service type to use for the gateway | |||
NGF_VERSION ?= edge## NGF version to be tested | |||
PULL_POLICY = Never## Pull policy for the images | |||
NGINX_CONF_DIR = internal/controller/nginx/conf | |||
SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors | |||
SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteBackendProtocolWebSocket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the H2C test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, just saw your comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming it's not test related, because there are multiple other implementations that support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea most likely not, but it also feels like the appProtocol tests aren't too fleshed out, and I'm looking into some stuff where the request the test makes doesn't actually have a host. And it kind of makes sense when the conformance test errors with status code 400, which is a "Bad request". But i'll keep looking into it.
Proposed changes
Add support for backendref service appProtocol
Problem: Users who want to specify the appProtocol on a Service port should correctly have checks in place so traffic does not flow to those services that do not have the matching appProtocol.
Solution: Mark BackendRefs on routes that cannot send traffic using the specified protocol as invalid and support new
NewRouteBackendRefUnsupportedProtocol
condition.Testing: Manually tested changes work and added unit tests.
Manual test cases verified:
HTTPRoute with h2c on works
HTTPRoute with h2c off correctly shows invalid protocol condition, generates nginx conf which points route to invalid backendRef, and correctly returns 500 status code when sending traffic
HTTPRoute with ws works
HTTPRoute with wss and a corresponding backendTLSPolicy correctly shows valid condition generates correct nginx conf, traffic flows
HTTPRoute with wss and no backendTLSPolicy correctly shows invalid protocol condition, generates nginx conf which points route to invalid backendRef, and correctly returns 500 status code when sending traffic
GRPCRoute with h2c on works
GRPC with h2c off correctly shows unsupported configuration condition, generates nginx conf which points route to invalid backendRef. (This is a different condition because the route cannot attach to the gateway).
GRPCRoute with ws correctly shows unsupported protocol condition, generates nginx conf which points route to invalid backendRef
GRPCRoute with wss correctly shows unsupported protocol condition, generates nginx conf which points route to invalid backendRef
TLSRoute with h2c correctly shows unsupported protocol condition, generates nginx conf whichh points to connection closed server
TLSRoute with ws correctly shows unsupported protocol condition, generates nginx conf which points to connection closed server
TLSRoute with wss works
Use this chart as reference for supported protocol -> route type. https://gateway-api.sigs.k8s.io/geps/gep-1911/#supporting-protocols
Closes #2945
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.