-
Notifications
You must be signed in to change notification settings - Fork 596
Support running e2e tests across different Gw API versions #12794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support running e2e tests across different Gw API versions #12794
Conversation
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
24c11fa to
168c9a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for running E2E tests against different versions and channels (standard/experimental) of the Gateway API. It introduces a versioning framework that allows tests to declare minimum/maximum version requirements and automatically skip tests when requirements are not met.
Key changes:
- Adds Gateway API version detection and caching mechanism based on CRD annotations
- Implements test-level and suite-level version constraints via
MinGwApiVersionandMaxGwApiVersionfields - Introduces
SetupByVersionto allow different suite setups based on installed Gateway API version - Updates multiple test suites to specify appropriate version requirements for experimental features
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/tests/base/base_suite.go | Core implementation of version detection, constraint checking, and versioned setup selection |
| test/e2e/tests/kgateway_tests.go | Comments out Lambda test (temporarily for local/fork testing) |
| test/e2e/tests/listenerset_test.go | Removes manual CRD existence check (replaced by version requirements) |
| test/e2e/features/*/types.go, suite.go | Updates test suites to declare Gateway API version requirements for experimental features |
| .github/workflows/nightly-tests.yaml | Adds workflow to run E2E tests across different Gateway API versions |
| go.mod | Promotes semver package from indirect to direct dependency |
| design/12721.md | Design document explaining the versioning approach |
Comments suppressed due to low confidence (1)
test/e2e/features/metrics/testdata/setup.yaml:1
- The removed 'allowedListeners' configuration appears to be a necessary change for backwards compatibility with older Gateway API versions, but the comment explaining this removal is missing. Consider adding a comment explaining why this was removed from the base setup and moved to the versioned setup.
kind: Gateway
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: Seth Heidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]> Add TLSRoute/TCPRoute and Session persistence types Signed-off-by: sheidkamp <[email protected]>
This reverts commit f5dc30f. Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
This reverts commit c1efda1. Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: Seth Heidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]> Update local.go Signed-off-by: sheidkamp <[email protected]> Update nightly-tests.yaml Signed-off-by: sheidkamp <[email protected]> default namespace Signed-off-by: sheidkamp <[email protected]> Update kgateway_tests.go Signed-off-by: sheidkamp <[email protected]> fix up ci Signed-off-by: sheidkamp <[email protected]> Update action.yaml Signed-off-by: sheidkamp <[email protected]> more ci cleanup Signed-off-by: sheidkamp <[email protected]> more ci cleanup Signed-off-by: sheidkamp <[email protected]> verify Signed-off-by: sheidkamp <[email protected]>
c6519ec to
ea89536
Compare
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
pkg/utils/threadsafe/writer.go
Outdated
| ) | ||
|
|
||
| // threadSafeWriter wraps an io.Writer with mutex protection for concurrent writes | ||
| type ThreadSafeWriter struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| type ThreadSafeWriter struct { | |
| type Writer struct { |
since this is in the threadsafe package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to "WriterWrapper"
| } | ||
| } | ||
|
|
||
| // TODO (sheidkamp) updating ordering for debugging, revert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned up.
| // BackendTLSPolicy moved to standard/v1 in 1.4.0 and experimental (alpha1v3 version is not supported), HTTPRoutes.spec.rules[].name was added to standard in 1.4.0 | ||
| GwApiV1_4_0 = GwApiVersionMustParse("1.4.0") | ||
|
|
||
| GwApiRequireRouteNames = map[GwApiChannel]*GwApiVersion{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand why this is a map ? Why can't this be a string / enum of channel_version ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest reason was to make it impossible to enter bad/ambiguous configuration.
With the type you can only use known channels and valid semvars, and are limited to one version per channel by the map. It also stores the data in a way that makes it easy to access when checking.
Maybe I'm misunderstanding, but are you thinking the string would look something like "experimental:1.2.0;standard:1.40"?
Signed-off-by: sheidkamp <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, approach sounds reasonable to me. I think we could investigate the race issues around the buffer a bit more but that can be tackled over time imo.
Thanks, once this merges I will create an issue with links to the relevant spots in code. |
…rsion-tests Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Description
Passing nightly tests in fork: https://github.com/sheidkamp/kgateway/actions/runs/19080780416/job/54508476958 (minus the Lambda suite which I did not have the environment setup for)
Example running locally:
You should see
Resolves #12721
Change Type
Changelog
Additional Notes
The first commit is base test suite changes and CI jobs
The second commit is the test updates.
Further commits are tweaks/updates/fixes/cleanup