conformance against invalid ReferenceGrants in HTTPRoute and TLSRoute #2076

meyskens · 2023-05-29T14:29:36Z

What type of PR is this?
/kind test
/area conformance

What this PR does / why we need it:

This PR adds two new conformance tests to test that invalid ReferenceGrants are not getting programmed as well as checking that they are indeed not. It does this check for both HTTPRoutes and TLSRoutes. The list of invalid ReferenceGrants is based on the test in https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/gateway-secret-invalid-reference-grant.go

Which issue(s) this PR fixes:

Fixes #2075

Does this PR introduce a user-facing change?:

Add conformance tests against accepting invalid ReferenceGrants in HTTPRoute and TLSRoute

k8s-ci-robot · 2023-05-29T14:29:44Z

Welcome @meyskens!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-05-29T14:29:45Z

Hi @meyskens. 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.

linux-foundation-easycla · 2023-05-29T14:29:45Z

The committers listed above are authorized under a signed CLA.

✅ login: meyskens / name: Maartje Eyskens (a13b497, e3c6ccc)

conformance/tests/httproute-invalid-reference-grant.yaml

sunjayBhatia

missing from here:

gateway-api/conformance/utils/config/timeout.go

Line 108 in c8b54a0

func SetupTimeoutConfig(timeoutConfig *TimeoutConfig) {

	if timeoutConfig.TLSRouteMustHaveCondition == 0 {
		timeoutConfig.TLSRouteMustHaveCondition = defaultTimeoutConfig.TLSRouteMustHaveCondition
	}

conformance/tests/httproute-invalid-reference-grant.go

conformance/tests/tlsroute-invalid-reference-grant.go

sunjayBhatia · 2023-06-01T18:09:32Z

conformance/base/manifests.yaml

@@ -300,6 +300,75 @@ spec:
 ---
 apiVersion: v1
 kind: Namespace
+metadata:
+  name: gateway-conformance-tls-backend


could be reuse the existing -app-backend instead of having to create a new namespace?

i was doubting to do to that but not entirely too sure about it so I chose to make a separate namespace to make clear boundaries. Happy to revert it and use the app namespace.

i think one of the maintainers may chime in here but I believe we've tried to limit the number of namespaces/resources the tests create to streamline things a bit 👍🏽

I agree with @sunjayBhatia, where possible I think we should reuse the same resources instead of creating new ones for specific use cases.

sunjayBhatia · 2023-06-01T21:32:03Z

(with the timeout change, these pass against Contour)

This adds a test comparable to the one for Gateway checking if ReferenceGrants are not incorrectly accepted. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

sunjayBhatia · 2023-06-05T15:17:16Z

/ok-to-test

mlavacca · 2023-06-07T09:54:57Z

conformance/base/manifests.yaml

@@ -300,6 +300,75 @@ spec:
 ---
 apiVersion: v1
 kind: Namespace
+metadata:
+  name: gateway-conformance-tls-backend


I agree with @sunjayBhatia, where possible I think we should reuse the same resources instead of creating new ones for specific use cases.

mlavacca · 2023-06-07T09:58:35Z

conformance/tests/httproute-invalid-reference-grant.yaml

:+1 I like that you created many ReferenceGrants, each with a possible misconfiguration.

Yeah this is really great, nicely done @meyskens!

conformance/utils/kubernetes/helpers.go

conformance/base/manifests.yaml

robscott

Thanks @meyskens! Other than @sunjayBhatia's comment, this mostly LGTM.

robscott · 2023-06-26T21:20:31Z

conformance/tests/httproute-invalid-reference-grant.yaml

Yeah this is really great, nicely done @meyskens!

conformance/tests/httproute-invalid-reference-grant.go

meyskens · 2023-06-28T10:35:06Z

/retest

sunjayBhatia · 2023-06-28T15:28:35Z

conformance/tests/tlsroute-invalid-reference-grant.go

+
+		kubernetes.GatewayAndTLSRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)
+
+		t.Run("TLSRoute with BackendRef in another namespace and no ReferenceGrant covering the Service has a ResolvedRefs Condition with status False and Reason RefNotPermitted", func(t *testing.T) {


realizing this test only specifies the conditions set on the route with no valid referencegrant to make the backendref in it valid, which is valuable

however, it seems the language on what to do with an invalid backendref needs some more specificity, particularly what "reject" means since this is a "must" statement:

gateway-api/apis/v1alpha2/tlsroute_types.go

Lines 104 to 108 in a5ede12

// sent. If unspecified or invalid (refers to a non-existent resource or

// a Service with no endpoints), the rule performs no forwarding; if no

// filters are specified that would result in a response being sent, the

// underlying implementation must actively reject request attempts to this

// backend, by rejecting the connection or returning a 500 status code.

I think we could use a follow up to this PR that clarifies that language (and possibly removes the HTTP 500 reference) and adds coverage to this test

follow up #2153

sunjayBhatia

LGTM, tests both pass with Contour

one comment here that needs a follow up issue: https://github.com/kubernetes-sigs/gateway-api/pull/2076/files#r1245405876

conformance/tests/tlsroute-invalid-reference-grant.yaml

This checks ReferenceGrant rules for TLSRoute against accepting an invalid ReferenceGrant for the backend service. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

robscott · 2023-06-29T19:45:09Z

Thanks @meyskens!

/lgtm
/approve

k8s-ci-robot · 2023-06-29T19:45:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: meyskens, robscott, sunjayBhatia

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~~ [robscott]

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/test area/conformance labels May 29, 2023

k8s-ci-robot requested review from sunjayBhatia and Xunzhuo May 29, 2023 14:29

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 29, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 29, 2023

meyskens changed the title ~~Meyskens/aditional referencegrant tests~~ conformance against invalid ReferenceGrants in HTTPRoute and TLSRoute May 30, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 30, 2023

meyskens marked this pull request as ready for review May 30, 2023 06:47

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2023

k8s-ci-robot requested review from michaelbeaumont and mlavacca May 30, 2023 06:47

sunjayBhatia reviewed May 31, 2023

View reviewed changes

conformance/tests/httproute-invalid-reference-grant.yaml Outdated Show resolved Hide resolved

sunjayBhatia reviewed May 31, 2023

View reviewed changes

conformance/tests/httproute-invalid-reference-grant.yaml Outdated Show resolved Hide resolved

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch from da1c97d to af206d2 Compare June 1, 2023 11:49

sunjayBhatia suggested changes Jun 1, 2023

View reviewed changes

sunjayBhatia reviewed Jun 1, 2023

View reviewed changes

conformance/tests/httproute-invalid-reference-grant.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Jun 1, 2023

View reviewed changes

conformance/tests/tlsroute-invalid-reference-grant.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Jun 1, 2023

View reviewed changes

Add tests for HTTPRoutes + invalid ReferenceGrants

a13b497

This adds a test comparable to the one for Gateway checking if ReferenceGrants are not incorrectly accepted. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch from af206d2 to 6c7fe97 Compare June 5, 2023 08:11

mlavacca reviewed Jun 7, 2023

View reviewed changes

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch 3 times, most recently from d404197 to b5c82e5 Compare June 26, 2023 12:14

sunjayBhatia reviewed Jun 26, 2023

View reviewed changes

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

sunjayBhatia reviewed Jun 26, 2023

View reviewed changes

conformance/base/manifests.yaml Outdated Show resolved Hide resolved

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch from b5c82e5 to fb72173 Compare June 26, 2023 15:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 26, 2023

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch from fb72173 to 75a582b Compare June 26, 2023 16:02

sunjayBhatia reviewed Jun 26, 2023

View reviewed changes

conformance/base/manifests.yaml Show resolved Hide resolved

robscott added this to the v0.8.0 milestone Jun 26, 2023

robscott reviewed Jun 26, 2023

View reviewed changes

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch from 75a582b to 3a12feb Compare June 27, 2023 12:49

sunjayBhatia reviewed Jun 28, 2023

View reviewed changes

sunjayBhatia approved these changes Jun 28, 2023

View reviewed changes

This was referenced Jun 28, 2023

conformance: Coverage for request routing to TLSRoute with no valid BackendRefs #2153

Open

Conformance Tests: TLSRoute Passthrough #1579

Open

sunjayBhatia reviewed Jun 28, 2023

View reviewed changes

conformance/tests/tlsroute-invalid-reference-grant.yaml Show resolved Hide resolved

Add TLSRoute + ReferenceGrant check

e3c6ccc

This checks ReferenceGrant rules for TLSRoute against accepting an invalid ReferenceGrant for the backend service. Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>

meyskens force-pushed the meyskens/aditional-referencegrant-tests branch from 3a12feb to e3c6ccc Compare June 29, 2023 08:54

k8s-ci-robot assigned robscott Jun 29, 2023

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

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

k8s-ci-robot merged commit 9b839f2 into kubernetes-sigs:main Jun 29, 2023

meyskens deleted the meyskens/aditional-referencegrant-tests branch June 30, 2023 09:07

mlavacca mentioned this pull request Aug 24, 2023

Gateway API TLSRouteInvalidReferenceGrant conformance test Kong/kubernetes-ingress-controller#4562

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conformance against invalid ReferenceGrants in HTTPRoute and TLSRoute #2076

conformance against invalid ReferenceGrants in HTTPRoute and TLSRoute #2076

meyskens commented May 29, 2023

k8s-ci-robot commented May 29, 2023

k8s-ci-robot commented May 29, 2023

linux-foundation-easycla bot commented May 29, 2023 •

edited

Loading

sunjayBhatia left a comment

sunjayBhatia Jun 1, 2023

meyskens Jun 5, 2023

sunjayBhatia Jun 5, 2023

mlavacca Jun 7, 2023

sunjayBhatia commented Jun 1, 2023

sunjayBhatia commented Jun 5, 2023

mlavacca Jun 7, 2023

mlavacca Jun 7, 2023

robscott Jun 26, 2023

robscott left a comment

robscott Jun 26, 2023

meyskens commented Jun 28, 2023

sunjayBhatia Jun 28, 2023

sunjayBhatia Jun 28, 2023

sunjayBhatia left a comment

robscott commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023


		kubernetes.GatewayAndTLSRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)

		t.Run("TLSRoute with BackendRef in another namespace and no ReferenceGrant covering the Service has a ResolvedRefs Condition with status False and Reason RefNotPermitted", func(t *testing.T) {

	// sent. If unspecified or invalid (refers to a non-existent resource or
	// a Service with no endpoints), the rule performs no forwarding; if no
	// filters are specified that would result in a response being sent, the
	// underlying implementation must actively reject request attempts to this
	// backend, by rejecting the connection or returning a 500 status code.

conformance against invalid ReferenceGrants in HTTPRoute and TLSRoute #2076

conformance against invalid ReferenceGrants in HTTPRoute and TLSRoute #2076

Conversation

meyskens commented May 29, 2023

k8s-ci-robot commented May 29, 2023

k8s-ci-robot commented May 29, 2023

linux-foundation-easycla bot commented May 29, 2023 • edited Loading

sunjayBhatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia commented Jun 1, 2023

sunjayBhatia commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meyskens commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

robscott commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

linux-foundation-easycla bot commented May 29, 2023 •

edited

Loading