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.

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

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Oct 27, 2023

What type of PR is this?

What this PR does / why we need it:

  • Set status to False for notAccepted Route condition

  • Remove Accepted condition from status.listeners[0].conditions since it is not mandatory for the Accepted condition to be set. In this case the ResolvedRefs=False due to InvalidCertificateRef so an implementation may choose to not generate any listener config

Which issue(s) this PR fixes:

Also Fixes # #2526

Does this PR introduce a user-facing change?:

Fixed the status condition requirements in the `GatewayWithAttachedRoutes` test

* 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>
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 27, 2023
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 27, 2023
@arkodg
Copy link
Contributor Author

arkodg commented Oct 27, 2023

cc @danehans

@youngnick
Copy link
Contributor

Uh, where do we say that the Accepted condition is not mandatory? That is a mistake, it should be mandatory.

@arkodg
Copy link
Contributor Author

arkodg commented Oct 27, 2023

@youngnick based on the definition here

// This condition indicates that the listener is syntactically and

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

@youngnick
Copy link
Contributor

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.

@youngnick
Copy link
Contributor

@robscott and I talked about this, and agreed that this change is good for now, but I've also logged #2536 to come back and clear this up a bit.

@robscott
Copy link
Member

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,
Copy link
Contributor

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)
?

Copy link
Contributor

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.

Copy link
Contributor

@michaelbeaumont michaelbeaumont Nov 1, 2023

Choose a reason for hiding this comment

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

Addressed in #2548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants