Initial Conformance Tests for Gateway API #969

robscott · 2021-12-20T22:52:18Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds a very basic initial framework for conformance tests. Although there is still plenty of room for both improvement and expansion, I want to keep this initial starting point relatively small and straightforward. This PR is ready for full review, I'm especially interested in feedback on the general structure and concepts in this PR though.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2021-12-20T22:52:20Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

robscott · 2021-12-31T00:40:59Z

At long last, I think these tests are ready for full review. They line up with the earlier conformance related GEPs and cover the following:

GatewayClass
- Status:
  - Accepted
Gateway:
- Provisioning (provisioning in advance is also supported)
- Status:
  - Ready
HTTPRoute:
- Multiple routes
- Multiple rules
- Matching:
  - Path
  - Hostname
  - Header
- Status:
  - Parents

robscott · 2021-12-31T00:41:39Z

Adding a hold because this a big PR and want to have at least a couple LGTMs before merging:

/hold

conformance/manifests/httproute-cross-namespace.yaml

conformance/utils/roundtripper/roundtripper.go

hbagdi · 2022-01-03T18:57:13Z

conformance/utils/roundtripper/roundtripper.go

+			return nil, nil, dumpErr
+		}
+
+		fmt.Printf("Received response:\n%s\n\n", formatDump(dump, "< "))


nit: I'd be a bit happier if we could use a different prefix for requests and responses. Easier on the eyes.

I think that's already the case? One is "Sending Request: ...", and this is "Received Response: ...", any changes you'd recommend?

conformance/utils/suite/suite.go

hbagdi · 2022-01-03T19:13:20Z

conformance/manifests/base.yaml

+apiVersion: gateway.networking.k8s.io/v1alpha2
+kind: Gateway
+metadata:
+  name: gateway-same-namespace


Having more specific names with some semantic meaning like "shared-infra-gateway" would be better, especially as more and more tests use this.

I've added some comments at the top of this file describing the key resources, but unfortunately don't have great ideas for how to make the names of the Gateways more descriptive right now. At this point, the only real variation between them is where they accept routes to bind from:

gateway-same-namespace (only supports route in same ns)

gateway-all-namespaces (supports routes in all ns)

gateway-backend-namespaces (supports routes in ns with backend label)

In the future when we add resources with different protocols, multiple listeners, etc, naming may become a bit more clear. Open to ideas though.

I know it's more verbose, but does it make sense to repeat the YAML across test cases (i.e. make each case hermetic) rather than trying to deduplicate? I know it's kind of a pain, but sharing the setup can result in coupling between test cases and the creation of a complex "base" case that makes it harder to understand the relationship later on.

Yeah it's going to be tough to find the right balance here. For any route to work, it needs the following:

Namespace

Gateway

Service

Pods

Spinning all of those up and down for every test would quickly become quite time consuming. Alternatively if we were to leave them running indefinitely, we'd need each test case to be sure that it wasn't overlapping with resources defined by other test cases. This would also result in rather verbose yaml definitions for each test case. A final argument in favor of this shared set of base resources is that it enables conformance tests to be meaningful for implementations that are not currently able to support dynamically provisioning Gateways (this generally means there's a 1:1 relationship between Gateway and controller).

With all that said, more isolated tests would enable us to run tests in parallel which could significantly speed up a test run when there were a large number of tests in play. It would also be easier to understand each individual test case than our current approach that requires an understanding of the base resources.

I think we'll inevitably want to move towards a test suite that has more isolated test definitions, but fortunately that should be a fairly straightforward transition from this current state. I think that the current proposal is an acceptable starting point given the constraints above, but that we should try to avoid expanding the base set of resources, and in the future add test cases that are not entirely dependent on them.

conformance/tests/httproute.go

conformance/utils/http/http.go

conformance/tests/httproute-cross-namespace.go

conformance/tests/httproute-cross-namespace.yaml

shaneutt

Once other people's comments are resolved this looks like a great place to start for me. We'll be adding these to our test suite soon, the only thing I was really concerned about going into this was making sure there wasn't any friction for testing "unmanaged mode" (existing gateways) but I don't think what's here will cause any problem with that.

k8s-ci-robot · 2022-01-12T18:28:55Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

jpeach · 2022-01-14T06:48:43Z

conformance/utils/suite/suite.go

+// Setup ensures the base resources required for conformance tests are installed
+// in the cluster. It also ensures that all relevant resources are ready.
+func (suite *ConformanceTestSuite) Setup(t *testing.T) {
+	suite.ControllerName = kubernetes.GWCMustBeAccepted(t, suite.Client, suite.GatewayClassName, 180)


This can fail and doesn't give any feedback on what it is checking for. Suggest that every function that takes a testing.T should internally do a t.Run to give contextual feedback.

I tried t.Run first, but that didn't feel right because these items are not really tests as much as they are setup steps. (The GatewayClass status == accepted is relatively close to a test, but the rest are not). Instead I've added logging before each step, which does seem to make the output much clearer here.

hbagdi

I didn't look at all the code but I think we can iterate here.
I'm comfortable merging.

conformance/utils/roundtripper/roundtripper.go

hbagdi · 2022-01-14T18:49:55Z

conformance/utils/roundtripper/roundtripper.go

@@ -0,0 +1,154 @@
+/*
+Copyright 2022 The Kubernetes Authors.


I'd recommend starting with a single conformance/utils/http package which includes this file.
Any reason to have so manage packages upfront?

Since this is an interface that implementations can override, I've wanted to keep it as generic and possible and leave the door open for at least TCP. That's a ways out though, and I'm not really sure how we'd structure that. The idea was to separate the interface + default implementation from HTTP specific helper funcs.

conformance/utils/http/http.go

conformance/utils/suite/suite.go

conformance/utils/kubernetes/apply.go

hbagdi · 2022-01-14T18:56:50Z

conformance/utils/kubernetes/apply.go

+			if !apierrors.IsNotFound(err) {
+				require.NoErrorf(t, err, "error getting resource")
+			}
+			t.Logf("Creating %s %s", uObj.GetName(), uObj.GetKind())


Add a similar log line before c.Get() on line 62.

I'm a little hesitant to do that because our test output is already fairly verbose here. Here's the output I'm currently getting:

suite.go:56: Test Setup: Ensuring GatewayClass has been accepted suite.go:59: Test Setup: Applying base manifests apply.go:81: Updating gateway-conformance-infra Namespace apply.go:81: Updating same-namespace Gateway apply.go:81: Updating all-namespaces Gateway apply.go:81: Updating backend-namespaces Gateway apply.go:81: Updating infra-backend-v1 Service apply.go:81: Updating infra-backend-v1 Deployment apply.go:81: Updating infra-backend-v2 Service apply.go:81: Updating infra-backend-v2 Deployment apply.go:81: Updating gateway-conformance-app-backend Namespace apply.go:81: Updating app-backend-v1 Service apply.go:81: Updating app-backend-v1 Deployment apply.go:81: Updating app-backend-v2 Service apply.go:81: Updating app-backend-v2 Deployment apply.go:81: Updating gateway-conformance-web-backend Namespace apply.go:81: Updating web-backend Service apply.go:81: Updating web-backend Deployment suite.go:62: Test Setup: Ensuring Gateways and Pods from base manifests are ready

Sometimes there's a mix of Updating and Creating depending on if I've enabled cleanup or added new resources to the base manifests. In either case, adding a Getting before each Updating or Creating line may add more confusion or noise than it's worth. I think it could be helpful to log before the first API call we're doing in case of a timeout or other connection issue, but that's actually happening earlier in the test flow. Do you think there's a better way to log this output as a whole?

robscott · 2022-01-14T21:34:31Z

Thanks for all the great feedback on this @hbagdi, @howardjohn, @jpeach, and anyone else who's commented. I think I've resolved everything and this should be good to go now, PTAL.

conformance/utils/suite/suite.go

hbagdi · 2022-01-19T00:38:12Z

/lgtm

hbagdi · 2022-01-19T05:31:16Z

@robscott Linter unhappy.

robscott · 2022-01-19T17:32:55Z

Woops, fixed the failing test, thanks for catching that @hbagdi! Removing the hold so next LGTM will result in a merge.

/hold cancel

hbagdi · 2022-01-19T17:58:56Z

/lgtm
Thank you for all your hard work on this, Rob! This one took a lot of effort over the past year.

k8s-ci-robot requested review from jpeach and youngnick December 20, 2021 22:52

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2021

robscott force-pushed the conformance-init branch 2 times, most recently from c7f499c to 3653c0e Compare December 22, 2021 20:31

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 22, 2021

robscott force-pushed the conformance-init branch 4 times, most recently from f757859 to 98d4822 Compare December 29, 2021 02:09

robscott force-pushed the conformance-init branch 5 times, most recently from 563e31c to 529a279 Compare December 31, 2021 00:28

robscott marked this pull request as ready for review December 31, 2021 00:29

robscott changed the title ~~[WIP] Initial Conformance Tests for Gateway API~~ Initial Conformance Tests for Gateway API Dec 31, 2021

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 Dec 31, 2021

robscott force-pushed the conformance-init branch from 529a279 to 92e5e07 Compare December 31, 2021 00:32

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 31, 2021

hbagdi reviewed Jan 3, 2022

View reviewed changes

robscott force-pushed the conformance-init branch from 92e5e07 to e6f52dc Compare January 5, 2022 01:27

robscott force-pushed the conformance-init branch from bfe48f8 to acd9597 Compare January 10, 2022 22:57

hbagdi reviewed Jan 11, 2022

View reviewed changes

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

hbagdi reviewed Jan 11, 2022

View reviewed changes

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

hbagdi reviewed Jan 11, 2022

View reviewed changes

conformance/tests/httproute-cross-namespace.yaml Outdated Show resolved Hide resolved

shaneutt approved these changes Jan 12, 2022

View reviewed changes

robscott force-pushed the conformance-init branch 4 times, most recently from 847fbc5 to bf64c67 Compare January 13, 2022 19:38

robscott mentioned this pull request Jan 13, 2022

Additional Conformance Tests covering HTTPRoute and Gateway #994

Merged

jpeach reviewed Jan 14, 2022

View reviewed changes

hbagdi reviewed Jan 14, 2022

View reviewed changes

robscott force-pushed the conformance-init branch from bf64c67 to 4efae19 Compare January 14, 2022 20:51

hbagdi reviewed Jan 19, 2022

View reviewed changes

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

k8s-ci-robot assigned hbagdi Jan 19, 2022

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

robscott force-pushed the conformance-init branch from 4efae19 to 38e5f1d Compare January 19, 2022 01:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2022

Initial Conformance Tests for Gateway API

1c6c178

robscott force-pushed the conformance-init branch from 38e5f1d to 1c6c178 Compare January 19, 2022 17:18

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

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

k8s-ci-robot merged commit 398f7e1 into kubernetes-sigs:master Jan 19, 2022

markmc mentioned this pull request May 31, 2022

Add Initial Conformance Test Framework #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Conformance Tests for Gateway API #969

Initial Conformance Tests for Gateway API #969

robscott commented Dec 20, 2021 •

edited

Loading

k8s-ci-robot commented Dec 20, 2021

robscott commented Dec 31, 2021 •

edited

Loading

robscott commented Dec 31, 2021

hbagdi Jan 3, 2022

robscott Jan 5, 2022

hbagdi Jan 3, 2022

robscott Jan 5, 2022

bowei Jan 5, 2022

robscott Jan 6, 2022

shaneutt left a comment •

edited

Loading

k8s-ci-robot commented Jan 12, 2022

jpeach Jan 14, 2022

robscott Jan 14, 2022

hbagdi left a comment

hbagdi Jan 14, 2022

robscott Jan 14, 2022

hbagdi Jan 14, 2022

robscott Jan 14, 2022

robscott commented Jan 14, 2022

hbagdi commented Jan 19, 2022

hbagdi commented Jan 19, 2022

robscott commented Jan 19, 2022

hbagdi commented Jan 19, 2022

Initial Conformance Tests for Gateway API #969

Initial Conformance Tests for Gateway API #969

Conversation

robscott commented Dec 20, 2021 • edited Loading

k8s-ci-robot commented Dec 20, 2021

robscott commented Dec 31, 2021 • edited Loading

robscott commented Dec 31, 2021

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

Choose a reason for hiding this comment

shaneutt left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi 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

robscott commented Jan 14, 2022

hbagdi commented Jan 19, 2022

hbagdi commented Jan 19, 2022

robscott commented Jan 19, 2022

hbagdi commented Jan 19, 2022

robscott commented Dec 20, 2021 •

edited

Loading

robscott commented Dec 31, 2021 •

edited

Loading

shaneutt left a comment •

edited

Loading