Check ResolvedRefs condition in Gateway conformance tests #2247

levikobi · 2023-07-28T06:29:20Z

What type of PR is this?

/kind test
/area conformance

What this PR does / why we need it:
Checks the ListenerConditionResolvedRefs value. This is needed to ensure that its value is always set, no matter if the ConditionStatus is true, or false.

Which issue(s) this PR fixes:

Fixes #1315

Does this PR introduce a user-facing change?:

None

k8s-ci-robot · 2023-07-28T06:29:29Z

Hi @levikobi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

michaelbeaumont

/lgtm !

levikobi · 2023-07-29T05:45:50Z

/ok-to-test

Xunzhuo

/lgtm thanks!

defer to others, make sure nowhere is missed.

LiorLieberman · 2023-07-31T11:55:17Z

/cc

LiorLieberman

I might be missing something, but if are adding these conditions here and not changing https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/utils/kubernetes/helpers.go#L708-L711

It must say that either our tests fail now or this change will cause them to fail.

What am I missing?

michaelbeaumont · 2023-07-31T12:01:49Z

I might be missing something, but if are adding these conditions here and not changing https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/utils/kubernetes/helpers.go#L708-L711

It must say that either our tests fail now or this change will cause them to fail.

What am I missing?

The check only asserts on conditions explicitly listed in the test, so whether existing or missing, the ResolvedRefs condition was ignored

LiorLieberman · 2023-07-31T13:24:55Z

The check only asserts on conditions explicitly listed in the test, so whether existing or missing, the ResolvedRefs condition was ignored

But it looks like it compares the number of actual conditions vs the expected conditions before.
So if we add another condition the length should be different

levikobi · 2023-07-31T14:16:16Z

The check only asserts on conditions explicitly listed in the test, so whether existing or missing, the ResolvedRefs condition was ignored

But it looks like it compares the number of actual conditions vs the expected conditions before. So if we add another condition the length should be different

Have a look at https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/utils/kubernetes/helpers.go#L721

Only if the number of expected conditions is larger than the actual the test will fail.
Up until now, the number of expected was lower, hence the tests passed.
Right now we are adding another condition, which should have already been in the actual conditions list (this is what the modification is verifying). So, len(actual) will equal len(expected) at most, and if not - the test should fail

LiorLieberman · 2023-07-31T14:27:23Z

Right, thats what I was missing, long day ...😅

Thanks!

/lgtm

LiorLieberman · 2023-08-02T13:27:47Z

/assign @sunjayBhatia

mlavacca

Thanks for this PR @levikobi!

I think we should check that all the gateways used in the tests have actually set such a condition to true (Even the gateways used in the HTTPRoute tests). To do so, we could change this helper to check that the resolvedRefs is enforced as well.

mlavacca · 2023-08-02T15:06:08Z

/cc

levikobi · 2023-08-03T10:39:01Z

Thanks for this PR @levikobi!

I think we should check that all the gateways used in the tests have actually set such a condition to true (Even the gateways used in the HTTPRoute tests). To do so, we could change this helper to check that the resolvedRefs is enforced as well.

That's a great suggestion, thanks @mlavacca!

conformance/utils/kubernetes/helpers.go

robscott

Thanks @levikobi!

robscott · 2023-09-18T21:45:35Z

conformance/utils/kubernetes/helpers.go

+// GatewayAndHTTPRoutesMustBeAccepted waits until:
+// 1. The specified Gateway has an IP address assigned to it.
+// 2. The route has a ParentRef referring to the Gateway.
+// 3. All the gateway's listeners have the ListenerConditionResolvedRefs set to true.


Do we have any tests that currently expect ListenerConditionResolvedRefs to be false? Since this is such a wide-reaching change, I'm concerned we may break some implementations with this change. Have you run this test against any implementations?

Yes, we do have a bunch of tests that expect ListenerConditionResolvedRefs to be false:

gateway-api/conformance/tests/gateway-invalid-route-kind.go

Line 67 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

gateway-api/conformance/tests/gateway-invalid-tls-certificateref.go

Line 49 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

gateway-api/conformance/tests/gateway-secret-invalid-reference-grant.go

Line 53 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

gateway-api/conformance/tests/gateway-secret-missing-reference-grant.go

Line 53 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

I tested the changes against Kong's implementation and it works 👍

@robscott I tried running it against Contour's implementation but got some errors. It didn't seem like a problem in the conformance though, but missing assignments of the ResolvedRefs.

Then I discovered @sunjayBhatia had already opened projectcontour/contour#5668 to address that 😅

Nevertheless I'm glad to hear that Kong and Kuma works.

mlavacca

Thanks @levikobi!

/approve

leaving the lgtm to @robscott to be sure his concern is addressed.

mlavacca · 2023-09-21T10:06:55Z

conformance/utils/kubernetes/helpers.go

+// GatewayAndHTTPRoutesMustBeAccepted waits until:
+// 1. The specified Gateway has an IP address assigned to it.
+// 2. The route has a ParentRef referring to the Gateway.
+// 3. All the gateway's listeners have the ListenerConditionResolvedRefs set to true.


Yes, we do have a bunch of tests that expect ListenerConditionResolvedRefs to be false:

gateway-api/conformance/tests/gateway-invalid-route-kind.go

Line 67 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

gateway-api/conformance/tests/gateway-invalid-tls-certificateref.go

Line 49 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

gateway-api/conformance/tests/gateway-secret-invalid-reference-grant.go

Line 53 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

gateway-api/conformance/tests/gateway-secret-missing-reference-grant.go

Line 53 in 8ab4e18

Type: string(v1beta1.ListenerConditionResolvedRefs),

I tested the changes against Kong's implementation and it works 👍

mlavacca · 2023-09-21T10:27:59Z

conformance/utils/kubernetes/helpers.go

+		Reason: "", // any reason
+	}
+
+	GatewayListenersMustHaveCondition(t, c, timeoutConfig, gw.NamespacedName, resolvedRefsCondition)


Since we are already adding the ResolvedRefs condition here, I think we should check all the positive-polarity conditions:

ResolvedRefs

Accepted

Programmed

Ideally, GatewayListenersMustHaveCondition should check the three above conditions to be set and true, before going on. I've created #2415 for this purpose, as it looks like a lack of consistency in our suite. We can address it in another PR, as it is a bit out of scope for this PR, but I wanted to highlight this aspect here as well.

mlavacca · 2023-09-21T10:31:46Z

/unhold

michaelbeaumont

LGTM, passes with Kuma

k8s-ci-robot · 2023-09-21T12:10:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: levikobi, michaelbeaumont, mlavacca, Xunzhuo

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:

~~conformance/OWNERS~~ [mlavacca]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

robscott · 2023-09-21T18:41:28Z

Thanks @levikobi!

/lgtm

k8s-ci-robot requested review from michaelbeaumont and sunjayBhatia July 28, 2023 06:29

skriss mentioned this pull request Jul 28, 2023

Gateway API: set Listener ResolvedRefs=true if not set to false projectcontour/contour#5648

Closed

michaelbeaumont approved these changes Jul 28, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 29, 2023

Xunzhuo approved these changes Jul 31, 2023

View reviewed changes

k8s-ci-robot requested a review from LiorLieberman July 31, 2023 11:55

LiorLieberman reviewed Jul 31, 2023

View reviewed changes

k8s-ci-robot assigned LiorLieberman Jul 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2023

k8s-ci-robot assigned sunjayBhatia Aug 2, 2023

mlavacca reviewed Aug 2, 2023

View reviewed changes

k8s-ci-robot requested a review from mlavacca August 2, 2023 15:06

shaneutt added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2023

mlavacca reviewed Aug 3, 2023

View reviewed changes

conformance/utils/kubernetes/helpers.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from youngnick August 4, 2023 21:15

sunjayBhatia mentioned this pull request Aug 10, 2023

fix [Gateway API]set Listener ResolvedRefs=true if not set to false projectcontour/contour#5668

Closed

shaneutt added the release-blocker MUST be completed to complete the milestone label Aug 22, 2023

shaneutt assigned robscott Sep 12, 2023

shaneutt added this to the v1.0.0 milestone Sep 15, 2023

levikobi added 3 commits September 17, 2023 19:39

Check ResolvedRefs condition in Gateway conformance tests

4fbe557

Check resolvedRefs is enforced at GatewayAndHTTPRoutesMustBeAccepted

ac5e4ea

Add GatewayListenersMustHaveCondition helper function

6ced935

levikobi force-pushed the conformance-1315 branch from d2adafb to 6ced935 Compare September 17, 2023 16:46

shaneutt requested review from mlavacca, robscott, shaneutt and LiorLieberman September 18, 2023 11:53

robscott reviewed Sep 18, 2023

View reviewed changes

mlavacca mentioned this pull request Sep 21, 2023

conformance: always check gateway positive-polarity conditions #2415

Closed

mlavacca approved these changes Sep 21, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2023

michaelbeaumont approved these changes Sep 21, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2023

k8s-ci-robot merged commit 2457533 into kubernetes-sigs:main Sep 21, 2023

levikobi deleted the conformance-1315 branch September 22, 2023 05:39

This was referenced Oct 11, 2023

Conformance: Set ResolvedRefs Condition by Default envoyproxy/gateway#1956

Closed

Sets Default ResolvedRefs Status Condition envoyproxy/gateway#1960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check ResolvedRefs condition in Gateway conformance tests #2247

Check ResolvedRefs condition in Gateway conformance tests #2247

levikobi commented Jul 28, 2023

k8s-ci-robot commented Jul 28, 2023

michaelbeaumont left a comment

levikobi commented Jul 29, 2023

Xunzhuo left a comment

LiorLieberman commented Jul 31, 2023

LiorLieberman left a comment

michaelbeaumont commented Jul 31, 2023 •

edited

Loading

LiorLieberman commented Jul 31, 2023

levikobi commented Jul 31, 2023

LiorLieberman commented Jul 31, 2023

LiorLieberman commented Aug 2, 2023

mlavacca left a comment •

edited

Loading

mlavacca commented Aug 2, 2023

levikobi commented Aug 3, 2023

robscott left a comment

robscott Sep 18, 2023

mlavacca Sep 21, 2023

levikobi Sep 21, 2023

mlavacca left a comment

mlavacca Sep 21, 2023

mlavacca Sep 21, 2023

mlavacca commented Sep 21, 2023

michaelbeaumont left a comment

k8s-ci-robot commented Sep 21, 2023

robscott commented Sep 21, 2023

Check ResolvedRefs condition in Gateway conformance tests #2247

Check ResolvedRefs condition in Gateway conformance tests #2247

Conversation

levikobi commented Jul 28, 2023

k8s-ci-robot commented Jul 28, 2023

michaelbeaumont left a comment

Choose a reason for hiding this comment

levikobi commented Jul 29, 2023

Xunzhuo left a comment

Choose a reason for hiding this comment

LiorLieberman commented Jul 31, 2023

LiorLieberman left a comment

Choose a reason for hiding this comment

michaelbeaumont commented Jul 31, 2023 • edited Loading

LiorLieberman commented Jul 31, 2023

levikobi commented Jul 31, 2023

LiorLieberman commented Jul 31, 2023

LiorLieberman commented Aug 2, 2023

mlavacca left a comment • edited Loading

Choose a reason for hiding this comment

mlavacca commented Aug 2, 2023

levikobi commented Aug 3, 2023

robscott left a comment

Choose a reason for hiding this comment

robscott Sep 18, 2023

Choose a reason for hiding this comment

mlavacca Sep 21, 2023

Choose a reason for hiding this comment

levikobi Sep 21, 2023

Choose a reason for hiding this comment

mlavacca left a comment

Choose a reason for hiding this comment

mlavacca Sep 21, 2023

Choose a reason for hiding this comment

mlavacca Sep 21, 2023

Choose a reason for hiding this comment

mlavacca commented Sep 21, 2023

michaelbeaumont left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 21, 2023

robscott commented Sep 21, 2023

michaelbeaumont commented Jul 31, 2023 •

edited

Loading

mlavacca left a comment •

edited

Loading