-
Notifications
You must be signed in to change notification settings - Fork 480
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
Fix bugs in GatewayWithAttachedRoutes #2535
Conversation
* Set status to `False` for `notAccepted` Route condition * Remove `Accepted` condition from `status.listeners[0].conditions` since it is not mandatory for the `Accepted` to be set. In this case the `ResolvedRefs=False` due to `InvalidCertificateRef` so an implementation may choose to not generate any listener config Signed-off-by: Arko Dasgupta <arko@tetrate.io>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkodg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cc @danehans |
Uh, where do we say that the |
@youngnick based on the definition here gateway-api/apis/v1/gateway_types.go Line 918 in cb82fb5
an implementation should set the Listener Condition to Accepted , only if it generates any valid data plane config.For this case, since gateway-conformance-infra/does-not-exist secret does not exist, the implementation may choose to not generate any data plane config, so imo the conformance test should not enforce the Accepted=True condition
|
I think that we may need to update that definition - as part of clarifying AttachedRoutes, we've effectively relaxed that so that Accepted means more "Accepted for processing" rather than "accepted for config". @robscott, you need to see this one. |
Thanks @arkodg! Also thanks to @youngnick for all the thought put into this one! /lgtm |
@@ -125,7 +120,7 @@ var GatewayWithAttachedRoutes = suite.ConformanceTest{ | |||
hrouteNN := types.NamespacedName{Name: "http-route-4", Namespace: "gateway-conformance-infra"} | |||
notAccepted := metav1.Condition{ | |||
Type: string(v1.RouteConditionAccepted), | |||
Status: metav1.ConditionTrue, | |||
Status: metav1.ConditionFalse, |
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.
is it conflicting with
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN) |
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.
Yeah. I don't see how an implementation can pass both of these tests.
We have this resource that should be Accepted
as @sayboras mentioned that's essentially identical to the resource in the test from this PR that should NOT be Accepted
.
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.
Addressed in #2548
What type of PR is this?
What this PR does / why we need it:
Set status to
False
fornotAccepted
Route conditionRemove
Accepted
condition fromstatus.listeners[0].conditions
since it is not mandatory for theAccepted
condition to be set. In this case theResolvedRefs=False
due toInvalidCertificateRef
so an implementation may choose to not generate any listener configWhich issue(s) this PR fixes:
Also Fixes # #2526
Does this PR introduce a user-facing change?: