Add conformance tests for ReferencePolicy #1081

mikemorris · 2022-03-29T20:49:38Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds conformance tests for ReferencePolicy. Given that ReferencePolicy is not among the resources graduating to v1beta1, we may need to add some ability fro implementations to opt-in to conformance tests for specific experimental or alpha features, similar to the functionality proposed in #482

~~The HTTPRouteMissingReferencePolicy test could likely be consolidated into the primary valid HTTPRouteReferencePolicy test~~

I plan to handle lifecycle behavior such as validating a mutation like deleting a ReferencePolicy causing an HTTPRoute (and associated Listener?) to switch their ResolvedRefs status to RefNotPermitted and stop sending traffic in a follow-up PR once we get alignment on expected behavior and if we want to mutate Kubernetes objects like that inside these tests before implementing.

Which issue(s) this PR fixes:

Refs #20

Does this PR introduce a user-facing change?:

Add conformance tests for ReferencePolicy

k8s-ci-robot · 2022-03-29T20:49:47Z

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

robscott

Thanks for the work on these tests @mikemorris! This is a great addition and matches what I was thinking for testing ReferencePolicy. In the future we may also want to add additional tests for cross-namespace Secret references from Gateways, but this is a great start.

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

conformance/tests/httproute-missing-reference-policy.go

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

robscott · 2022-03-30T23:15:26Z

/ok-to-test

conformance/tests/httproute-reference-policy.go

This reverts commit 72ee68b.

…"" This reverts commit b59c8f0.

…rcement (#142) * update rbac * checkpoint, wip * Take a first pass at implementing HTTPRoute validation * Regenerate mock client * Stub additional function in client to conform to interface * Early out if no reference policies in namespace * Working test verifying failed validation if no reference policies * Implement test coverage for reconciler w/ valid reference policy * Reduce scope of RBAC permissions for reference policies * Add test coverage for HTTPRoute validator * Use rebased version of kubernetes-sigs/gateway-api#1081 * Add TODO noting performance edge case * Extend route + backend validation to TCPRoutes * Fix unit test coverage for utils.go * working get reference policy by namespace * added soe logging * Add test coverage for util func w/ TCPRoute * tmate session * Log route + backend validation instead of returning error * fix workflow * Add test coverage for unspecified backendRef namespace * Use in-namespace opt for reference policy list instead of label selector * Use consul-k8s ref w/ updated RBAC for ReferencePolicy * Revert "Use rebased version of kubernetes-sigs/gateway-api#1081" This reverts commit 72ee68b. * use updated conforance/reference-policy PR branch * Revert "use updated conforance/reference-policy PR branch" This reverts commit 41c6b0d. * Revert "Revert "Use rebased version of kubernetes-sigs/gateway-api#1081"" This reverts commit b59c8f0. * Default group + kind for backend ref when checking ReferencePolicy Co-Authored-By: Mike Morris <mikemorris@users.noreply.github.com> * Clean up implementation, add comments for followup work * Remove tmate from conformance workflow * Restore non-experimental conformance tests, add TODO for consul-k8s * Remove stray debug log * Remove unused function added earlier * Only log and fall through for now if ReferencePolicy would be enforced * Remove consul-k8s PR ref for conformance * Remove unnecessary check for IsNotFound on List operation Co-authored-by: Sarah Alsmiller <sarah.alsmiller@hashicorp.com> Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>

nathancoleman

Our ReferencePolicy implementation is passing these tests except for the namespace issue. Not sure which path is the "correct" one for resolving yet.

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

conformance/tests/httproute-missing-reference-policy.go

conformance/conformance_test.go

conformance/tests/httproute-reference-policy.go

conformance/utils/suite/suite.go

skriss · 2022-04-11T18:53:49Z

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

+		t.Run("Gateway should have 0 Routes attached", func(t *testing.T) {
+			gw := &v1alpha2.Gateway{}
+			err := s.Client.Get(context.TODO(), gwNN, gw)
+			require.NoError(t, err, "error fetching Gateway")
+			// There are two valid ways to represent this:
+			// 1. No listeners in status
+			// 2. One listener in status with 0 attached routes
+			if len(gw.Status.Listeners) == 0 {
+				// No listeners in status.
+			} else if len(gw.Status.Listeners) == 1 {
+				require.Equal(t, int32(0), gw.Status.Listeners[0].AttachedRoutes)
+			} else {
+				t.Errorf("Expected no more than 1 listener in status, got %d", len(gw.Status.Listeners))
+			}


Is there a canonical definition for "Attached"? In this case, if I'm not mistaken, the route should still result in app-backend-v1 being programmed with 90% weight, with a 10% weight being applied to a 503 response for the app-backend-v2 backend that's not covered by a ReferencePolicy, based on https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/httproute_types.go#L191-L196. We (contour) had been considering the route to be "Attached" in this case since it's being at least partially programmed, but I'm not seeing a clear definition one way or another.

Agreed that the spec could be more clear on how these situations are intended to be handled - it's potentially more complicated when either resource changes after having been iniitally accepted too (and so is potentially actively routing traffic). I added a discussion topic for the weekly meeting later today to hopefully hear perspectives from various implementations.

Opened #1112 for further discussion. TL;DR I think your implementation makes sense @skriss, I'll be updating this test to check for ResolvedRefs { status: False, reason: RefNotPermitted } instead.

@skriss I updated this test to instead check for ResolvedRefs { status: False, reason: RefNotPermitted } conditions on both the Route and ListenerStatus, and added a check that HTTP requests eventually start returning 503 status codes - does this match your expected behavior?

Definitely makes sense for the HTTPRoute - our implementation is currently using a different reason, but it's wrong & should be switched to RefNotPermitted.

We're not currently setting that condition on the Listener status, though - I do see in the API docs that the RefNotPermitted listener condition reason says "This reason is used with the “ResolvedRefs” condition when one of the Listener’s Routes has a BackendRef to an object in another namespace, where the object in the other namespace does not have a ReferencePolicy explicitly allowing the reference.", so I guess we should be, though I also wonder why this condition is needed on the Listener (where it could be relevant to any one of the listener's routes) rather than just on the HTTPRoute itself. Looks like this has already been raised in #1112 (specifically in #1112 (comment)).

Thanks for digging into all this!

Thanks for the detail @skriss! I skipped the listener check for now to hopefully get this merge-able and can follow up on that later once we can reach consensus across implementations on expected behavior.

Opened #1143 to track updating this test to explicitly check for partial acceptance.

conformance/utils/suite/suite.go

mikemorris · 2022-04-14T21:09:14Z

Pushed up some relatively significant logical changes in response to reviews and discussion, also extracted #1114 for adding RouteConditionReason if that would be easier to review separately. This should be ready for another review pass now, and hopefully reaching some consensus around minimum conformance expectations for Routes with invalid BackendRefs.

…iated handling

…rossNamespaceBackendRef test

…f.go Co-authored-by: Nathan Coleman <nathandanielcoleman@gmail.com>

Co-authored-by: Steve Kriss <stephen.kriss@gmail.com>

robscott

Thanks @mikemorris! One more tiny nit otherwise LGTM.

/approve

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

k8s-ci-robot · 2022-05-04T16:55:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikemorris, robscott

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:

~~OWNERS~~ [robscott]

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

conformance/tests/httproute-invalid-cross-namespace-backend-ref.go

…f.go

Co-authored-by: Rob Scott <rob.scott87@gmail.com>

skriss

I re-ran this against Contour with the latest revisions and it's still passing so 👍 from me.

robscott · 2022-05-04T21:47:42Z

Thanks @mikemorris!

/lgtm

* conformance: add test for reference policy * conformance: add test for missing ReferencePolicy preventing route attachment * conformance: add tests for improperly configured ReferencePolicy * conformance: update ReferencePolicy tests to expect eventually consistent response conformance: removed unused imports from httproute-reference-policy test * conformance: update ReferencePolicy tests to expect an Accepted: False route parent status condition * conformance: add ExtendedSupport field to ConformanceTest struct, bump to Go 1.18 for slice.Contains * conformance: opt in to ReferencePolicy tests in conformance_test.go * conformance: fixup slices module dep usage * conformance: reimplement opt-in as SupportedFeature slice on ConformanceTestSuite and ConformanceTest * fixup: remove unusued slices dep import * fixup: fix routeNN for HTTPRouteInvalidReferencePolicy * conformance(reference-policy): don't require namespace for HTTPRouteMustHaveParents check for routes in same namesapce as gateway * Update conformance/utils/suite/suite.go Co-authored-by: Steve Kriss <stephen.kriss@gmail.com> * conformance: add conditionsMatch helper func * conformance: add GatewayStatusMustHaveListeners and listenersMatch helper funcs * conformance: update InvalidReferencePolicy test to check for ResolvedRefs RefNotPermitted status * fixup: remove unused imports for HTTPRouteInvalidReferencePolicy * conformance: rename HTTPRouteMissingReferencePolicy test to HTTPRouteInvalidCrossNamespaceBackendRef update test to check for ReslovedRefs RefNotPermitted condition on Route * conformance: rename HTTPRouteInvalidCrossNamespace test to HTTPRouteInvalidCrossNamespaceParentRef * conformance: add Exemptions field to ConformanceTest struct and associated handling * conformance: add ExemptReferencePolicy exemption to HTTPRouteInvalidCrossNamespaceBackendRef test * fixup: HTTPRouteInvalidCrossNamespaceBackendRef boilerplate * conformance: skip Listener ResolvedRefs check, add TODO notes * Update conformance/tests/httproute-invalid-cross-namespace-backend-ref.go Co-authored-by: Nathan Coleman <nathandanielcoleman@gmail.com> * Update conformance/utils/kubernetes/helpers.go Co-authored-by: Steve Kriss <stephen.kriss@gmail.com> * Update conformance/utils/kubernetes/helpers.go * conformance: fixup ExemptReferencePolicy description * Update conformance/tests/httproute-invalid-cross-namespace-backend-ref.go * Update conformance/tests/httproute-invalid-reference-policy.go Co-authored-by: Rob Scott <rob.scott87@gmail.com> Co-authored-by: Steve Kriss <stephen.kriss@gmail.com> Co-authored-by: Nathan Coleman <nathandanielcoleman@gmail.com> Co-authored-by: Rob Scott <rob.scott87@gmail.com>

k8s-ci-robot requested review from bowei and thockin March 29, 2022 20:49

robscott reviewed Mar 30, 2022

View reviewed changes

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

conformance/tests/httproute-missing-reference-policy.go Outdated Show resolved Hide resolved

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

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 Mar 30, 2022

mikemorris mentioned this pull request Apr 4, 2022

Add conformance testing workflow for GitHub Actions hashicorp/consul-api-gateway#139

Merged

2 tasks

nathancoleman reviewed Apr 5, 2022

View reviewed changes

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

nathancoleman mentioned this pull request Apr 5, 2022

Validate ReferencePolicy for cross-namespace routing, log future enforcement hashicorp/consul-api-gateway#142

Merged

2 tasks

nathancoleman added a commit to hashicorp/consul-api-gateway that referenced this pull request Apr 5, 2022

Use rebased version of kubernetes-sigs/gateway-api#1081

72ee68b

mikemorris force-pushed the conformance/reference-policy branch from 4f5dc45 to c0379bc Compare April 6, 2022 18:41

mikemorris added a commit to hashicorp/consul-api-gateway that referenced this pull request Apr 6, 2022

Revert "Use rebased version of kubernetes-sigs/gateway-api#1081"

b59c8f0

This reverts commit 72ee68b.

mikemorris added a commit to hashicorp/consul-api-gateway that referenced this pull request Apr 6, 2022

Revert "Revert "Use rebased version of kubernetes-sigs/gateway-api#1081…

a3b302e

…"" This reverts commit b59c8f0.

mikemorris force-pushed the conformance/reference-policy branch from 7dea9bc to ac69c07 Compare April 7, 2022 00:45

nathancoleman reviewed Apr 8, 2022

View reviewed changes

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

conformance/tests/httproute-missing-reference-policy.go Outdated Show resolved Hide resolved

sarahalsmiller mentioned this pull request Apr 8, 2022

Set status error message hashicorp/consul-api-gateway#143

Merged

2 tasks

robscott reviewed Apr 8, 2022

View reviewed changes

conformance/conformance_test.go Outdated Show resolved Hide resolved

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

conformance/utils/suite/suite.go Outdated Show resolved Hide resolved

robscott mentioned this pull request Apr 8, 2022

Conformance Tests: ReferencePolicy #1105

Closed

8 tasks

skriss reviewed Apr 11, 2022

View reviewed changes

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

mikemorris changed the title ~~[WIP] Add conformance tests for ReferencePolicy~~ Add conformance tests for ReferencePolicy Apr 18, 2022

mikemorris and others added 8 commits April 21, 2022 17:09

conformance: add Exemptions field to ConformanceTest struct and assoc…

54f59d3

…iated handling

conformance: add ExemptReferencePolicy exemption to HTTPRouteInvalidC…

b632029

…rossNamespaceBackendRef test

fixup: HTTPRouteInvalidCrossNamespaceBackendRef boilerplate

c282172

conformance: skip Listener ResolvedRefs check, add TODO notes

afd5808

Update conformance/tests/httproute-invalid-cross-namespace-backend-re…

f220aa7

…f.go Co-authored-by: Nathan Coleman <nathandanielcoleman@gmail.com>

Update conformance/utils/kubernetes/helpers.go

11fb014

Co-authored-by: Steve Kriss <stephen.kriss@gmail.com>

Update conformance/utils/kubernetes/helpers.go

b28f207

conformance: fixup ExemptReferencePolicy description

d3b6e1b

mikemorris force-pushed the conformance/reference-policy branch from d8f1946 to d3b6e1b Compare April 21, 2022 21:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2022

This was referenced May 4, 2022

conformance: refactor listenersMatch and parentsMatch helpers into a single generic function #1142

Closed

conformance: add test for partial acceptance of a route with one allowed and one unpermitted backend reference #1143

Closed

robscott reviewed May 4, 2022

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2022

mikemorris commented May 4, 2022

View reviewed changes

conformance/tests/httproute-invalid-cross-namespace-backend-ref.go Outdated Show resolved Hide resolved

mikemorris and others added 2 commits May 4, 2022 13:42

Update conformance/tests/httproute-invalid-cross-namespace-backend-re…

2a3c756

…f.go

Update conformance/tests/httproute-invalid-reference-policy.go

a68f85c

Co-authored-by: Rob Scott <rob.scott87@gmail.com>

skriss reviewed May 4, 2022

View reviewed changes

k8s-ci-robot assigned robscott May 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2022

k8s-ci-robot merged commit b5f5cd3 into kubernetes-sigs:master May 4, 2022

mikemorris deleted the conformance/reference-policy branch May 4, 2022 21:54

nathancoleman mentioned this pull request May 10, 2022

conformance: add test cases for Gateway->Secret ReferenceGrant #1158

Merged

mikemorris mentioned this pull request Jun 2, 2022

REQUEST: New membership for mikemorris kubernetes/org#3470

Closed

9 tasks

mikemorris mentioned this pull request Jun 14, 2022

Conflicting guidance when a HTTPRoute refers to an invalid backend #1211

Closed

mikemorris mentioned this pull request Sep 16, 2022

Publish policy on using generics in gateway-api #1388

Open

mikemorris mentioned this pull request Oct 12, 2022

Route handling of invalid BackendRefs needs clarification #1112

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conformance tests for ReferencePolicy #1081

Add conformance tests for ReferencePolicy #1081

mikemorris commented Mar 29, 2022 •

edited

Loading

k8s-ci-robot commented Mar 29, 2022

robscott left a comment

robscott commented Mar 30, 2022

nathancoleman left a comment

skriss Apr 11, 2022

mikemorris Apr 11, 2022 •

edited

Loading

mikemorris Apr 13, 2022 •

edited

Loading

mikemorris Apr 14, 2022

skriss Apr 15, 2022 •

edited

Loading

mikemorris Apr 18, 2022

mikemorris May 4, 2022

mikemorris commented Apr 14, 2022 •

edited

Loading

robscott left a comment

k8s-ci-robot commented May 4, 2022

skriss left a comment

robscott commented May 4, 2022

Add conformance tests for ReferencePolicy #1081

Add conformance tests for ReferencePolicy #1081

Conversation

mikemorris commented Mar 29, 2022 • edited Loading

k8s-ci-robot commented Mar 29, 2022

robscott left a comment

Choose a reason for hiding this comment

robscott commented Mar 30, 2022

nathancoleman left a comment

Choose a reason for hiding this comment

skriss Apr 11, 2022

Choose a reason for hiding this comment

mikemorris Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

mikemorris Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

mikemorris Apr 14, 2022

Choose a reason for hiding this comment

skriss Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

mikemorris Apr 18, 2022

Choose a reason for hiding this comment

mikemorris May 4, 2022

Choose a reason for hiding this comment

mikemorris commented Apr 14, 2022 • edited Loading

robscott left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 4, 2022

skriss left a comment

Choose a reason for hiding this comment

robscott commented May 4, 2022

mikemorris commented Mar 29, 2022 •

edited

Loading

mikemorris Apr 11, 2022 •

edited

Loading

mikemorris Apr 13, 2022 •

edited

Loading

skriss Apr 15, 2022 •

edited

Loading

mikemorris commented Apr 14, 2022 •

edited

Loading