-
Notifications
You must be signed in to change notification settings - Fork 665
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
internal/dag: Process HTTPRoute.Spec.Gateways #3618
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3618 +/- ##
==========================================
+ Coverage 76.71% 76.81% +0.09%
==========================================
Files 100 100
Lines 7142 7159 +17
==========================================
+ Hits 5479 5499 +20
+ Misses 1542 1539 -3
Partials 121 121
|
bah these integration tests, will look into why they are failing in CI. Works on my machine. ™️ |
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.
This is lookin great, thanks Steve.
Do we need to mention anything about the default value of spec.gateways
? I can't remember how it plays into these interactions.
The default value is |
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.
couple nits but otherwise looks good
_integration/testsuite/gatewayapi/007-httproute-gatewayallowtype.yaml
Outdated
Show resolved
Hide resolved
_integration/testsuite/gatewayapi/007-httproute-gatewayallowtype.yaml
Outdated
Show resolved
Hide resolved
|
||
--- | ||
|
||
apiVersion: apps/v1 |
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.
probably not needed since we're moving to the go framework, but maybe we shouldve made the fixture able to set a namespace as well as name of the resource
internal/dag/gatewayapi_processor.go
Outdated
if selMatches && nsMatches { | ||
// If all the match criteria for this HTTPRoute match the Gateway, then add | ||
// the route to the set of matchingRoutes. | ||
if selMatches && nsMatches && p.gatewayMatches(route) { |
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.
the RouteGateways
type mentions:
RouteGateways defines which Gateways will be able to use a route. If this field results in preventing the selection of a Route by a Gateway, an “Admitted” condition with a status of false must be set for the Gateway on that Route.
Seems like we should set status of "Admitted=false" on Routes that are selected by a Gateway but the Route does not "allow" the Gateway to select it
See https://gateway-api.sigs.k8s.io/references/spec/#networking.x-k8s.io/v1alpha1.RouteGateways
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.
Hmm good catch. We'll need to see if they are defined now to know if they matched by default vs matching the selectors. Let me write up some more tests.
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.
Ok just pushed a new commit @sunjayBhatia. =)
Implement support for HTTPRoutes.Spec.Gateways to allow an HTTPRoute to define how it binds to a Gateway via "All", "SameNamespace", or "FromList". Fixes projectcontour#3615 Signed-off-by: Steve Sloka <steve@stevesloka.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
't set status to Admitted: false Signed-off-by: Steve Sloka <slokas@vmware.com>
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.
LGTM, nice.
Implement support for HTTPRoutes.Spec.Gateways to allow an HTTPRoute to define how it
binds to a Gateway via "All", "SameNamespace", or "FromList".
Fixes #3615
Signed-off-by: Steve Sloka steve@stevesloka.com