Conformance: Adds Test to Exercise Gateway AttachedRoutes #2477

danehans · 2023-10-12T23:21:36Z

What type of PR is this?
/kind test

Optionally add one or more of the following kinds if applicable:
/area conformance

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #2402

Does this PR introduce a user-facing change?:

NONE

robscott · 2023-10-13T16:27:02Z

Thanks @danehans!

/cc @arkodg @mlavacca @sunjayBhatia

arkodg

/approve
thanks @danehans, hope this has been tested with at least one implementation

danehans · 2023-10-13T18:15:12Z

thanks @danehans, hope this has been tested with at least one implementation

@arkodg I have not tested this with an implementation yet but I'll start doing that today. Let's /hold until I complete this task.

arkodg · 2023-10-13T18:23:59Z

thanks @danehans, hope this has been tested with at least one implementation

@arkodg I have not tested this with an implementation yet but I'll start doing that today. Let's /hold until I complete this task.

thanks, I hope in the future there are more automated ways of doing this in this project itself, but unfortunately for now this burden is falling on the contributor and drops the overall velocity of contributions

danehans · 2023-10-18T21:48:03Z

I successfully tested this PR against Envoy Gateway. I had to use this patch since Envoy Gateway does not pass this test until envoyproxy/gateway#1916 is fixed. I also had to use v1beta1 APIs since envoyproxy/gateway#1160 is required for Envoy Gateway to support v1 APIs.

arkodg

thanks !
/approve

mlavacca

Thanks @danehans!

k8s-ci-robot · 2023-10-20T16:15:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, danehans, mlavacca

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~~ [arkodg,mlavacca]

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

robscott

Thanks for adding this @danehans! One other nit - I think the tests in this file should also depend on the SupportHTTPRoute feature.

conformance/tests/gateway-with-attached-routes.go

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>

danehans · 2023-10-23T19:09:43Z

conformance/tests/gateway-with-attached-routes.go

@@ -36,6 +36,7 @@ var GatewayWithAttachedRoutes = suite.ConformanceTest{
 	Description: "A Gateway in the gateway-conformance-infra namespace should be attached to routes.",
 	Features: []suite.SupportedFeature{
 		suite.SupportGateway,
+		suite.SupportHTTPRoute,


@robscott I added L39 and L149 based on your comments in #2477 (review).

youngnick · 2023-10-25T22:27:52Z

/lgtm

Thanks @danehans!

Problem: - Rules for traffic attachment of HTTPRoutes to Gateways were clarified in kubernetes-sigs/gateway-api#2396 -- successful attachment should depend only on parentRefs in an HTTPRoute and AllowedRoutes of a Listener, even if either or both of them are invalid. - The corresponding conformance test GatewayWithAttachedRoutes was added in kubernetes-sigs/gateway-api#2477 , which fails for NGINX Gateway Fabric. - NGINX Gateway Fabric will not try to attach an HTTPRoute to a Listener if either or both of them are invalid. Solution: - Make NGF compliant with the Gateway API and make the corresponding test pass. - Introduce Attachable fields for Listener and HTTPRoute types of the Graph in the graph package. - Update the validation logic: - NGF considers a Listener attachable if (a) its hostname is valid, (b) protocol is supported by NGF and (c) AllowedRoutes are valid. - NGF considers an HTTPRoute attachable if (d) its hostnames are valid. - Attach an HTTPRoute to a Listener if both are attachable. Note: (a), (b) and (d) are not mentioned in kubernetes-sigs/gateway-api#2396 However, they are necessary: For (b), NGF doesn't know how to attach to non-supported protocols like TCP. For (a), Listener hostname needed for HTTPRoute attaching, because it affects if an HTTPRoute can attach or not (per Gateway API spec). For (c), HTTPRoute hostnames are also needed, because they affect if an HTTPRoute can attach or not per Gateway API spec). See https://github.com/kubernetes-sigs/gateway-api/blob/52c2994ed9de1c287a37465490b91cfcf01bf16e/apis/v1/httproute_types.go#L71-L73 Testing: - Unit tests are updated and extended - Failing conformance test GatewayWithAttachedRoutes now passes. CLOSES nginxinc#1148 Co-authored-by: Saylor Berman <s.berman@f5.com>

Problem: - Rules for traffic attachment of HTTPRoutes to Gateways were clarified in kubernetes-sigs/gateway-api#2396 -- successful attachment should depend only on parentRefs in an HTTPRoute and AllowedRoutes of a Listener, even if either or both of them are invalid. - The corresponding conformance test GatewayWithAttachedRoutes was added in kubernetes-sigs/gateway-api#2477 , which fails for NGINX Gateway Fabric. - NGINX Gateway Fabric will not try to attach an HTTPRoute to a Listener if either or both of them are invalid. Solution: - Make NGF compliant with the Gateway API and make the corresponding test pass. - Introduce Attachable fields for Listener and HTTPRoute types of the Graph in the graph package. - Update the validation logic: - NGF considers a Listener attachable if (a) its hostname is valid, (b) protocol is supported by NGF and (c) AllowedRoutes are valid. - NGF considers an HTTPRoute attachable if (d) its hostnames are valid. - Attach an HTTPRoute to a Listener if both are attachable. Note: (a), (b) and (d) are not mentioned in kubernetes-sigs/gateway-api#2396 However, they are necessary: For (b), NGF doesn't know how to attach to non-supported protocols like TCP. For (a), Listener hostname needed for HTTPRoute attaching, because it affects if an HTTPRoute can attach or not (per Gateway API spec). For (c), HTTPRoute hostnames are also needed, because they affect if an HTTPRoute can attach or not per Gateway API spec). See https://github.com/kubernetes-sigs/gateway-api/blob/52c2994ed9de1c287a37465490b91cfcf01bf16e/apis/v1/httproute_types.go#L71-L73 Testing: - Unit tests are updated and extended - Failing conformance test GatewayWithAttachedRoutes now passes. CLOSES #1148 Co-authored-by: Saylor Berman <s.berman@f5.com>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 12, 2023

k8s-ci-robot requested review from arkodg and keithmattix October 12, 2023 23:21

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 12, 2023

k8s-ci-robot requested review from mlavacca and sunjayBhatia October 13, 2023 16:27

arkodg approved these changes Oct 13, 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 Oct 13, 2023

danehans mentioned this pull request Oct 13, 2023

AttachedRoutes not necessarily on accepted routes and listeners envoyproxy/gateway#1916

Closed

arkodg approved these changes Oct 18, 2023

View reviewed changes

danehans force-pushed the issue_2402 branch from 5754c2f to fe8be7a Compare October 18, 2023 21:55

mlavacca approved these changes Oct 20, 2023

View reviewed changes

robscott reviewed Oct 20, 2023

View reviewed changes

conformance/tests/gateway-with-attached-routes.go Outdated Show resolved Hide resolved

Conformance: Adds Test to Exercise Gateway AttachedRoutes

8b76391

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>

danehans force-pushed the issue_2402 branch from fe8be7a to 8b76391 Compare October 23, 2023 19:06

danehans commented Oct 23, 2023

View reviewed changes

danehans requested a review from robscott October 25, 2023 21:09

k8s-ci-robot assigned youngnick Oct 25, 2023

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

robscott mentioned this pull request Oct 25, 2023

Adding changelog for v1.0.0-rc2 #2523

Merged

k8s-ci-robot merged commit 22b7f77 into kubernetes-sigs:main Oct 25, 2023
8 checks passed

skriss mentioned this pull request Oct 31, 2023

Gateway API: fix for failing GatewayWithAttachedRoutes conformance test projectcontour/contour#5920

Closed

pleshakov mentioned this pull request Nov 21, 2023

Fix attaching of HTTPRoutes to Gateway Listeners nginxinc/nginx-gateway-fabric#1275

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conformance: Adds Test to Exercise Gateway AttachedRoutes #2477

Conformance: Adds Test to Exercise Gateway AttachedRoutes #2477

danehans commented Oct 12, 2023

robscott commented Oct 13, 2023

arkodg left a comment

danehans commented Oct 13, 2023

arkodg commented Oct 13, 2023 •

edited

Loading

danehans commented Oct 18, 2023 •

edited

Loading

arkodg left a comment

mlavacca left a comment

k8s-ci-robot commented Oct 20, 2023

robscott left a comment

danehans Oct 23, 2023 •

edited

Loading

youngnick commented Oct 25, 2023

Conformance: Adds Test to Exercise Gateway AttachedRoutes #2477

Conformance: Adds Test to Exercise Gateway AttachedRoutes #2477

Conversation

danehans commented Oct 12, 2023

robscott commented Oct 13, 2023

arkodg left a comment

Choose a reason for hiding this comment

danehans commented Oct 13, 2023

arkodg commented Oct 13, 2023 • edited Loading

danehans commented Oct 18, 2023 • edited Loading

arkodg left a comment

Choose a reason for hiding this comment

mlavacca left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 20, 2023

robscott left a comment

Choose a reason for hiding this comment

danehans Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

youngnick commented Oct 25, 2023

arkodg commented Oct 13, 2023 •

edited

Loading

danehans commented Oct 18, 2023 •

edited

Loading

danehans Oct 23, 2023 •

edited

Loading