Skip to content
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

CI: Updates Makefile and GH Workflow to Support Multiple Gateway API Versions #10390

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

danehans
Copy link

@danehans danehans commented Nov 21, 2024

Description

Updates the Makefile and conformance GH workflow to support multiple Gateway API versions.

Fixes #10359

API changes

None

Code changes

  • Makefile- Refactors conformance testing targets to support multiple Gateway API versions.

CI changes

  • .github/workflows/composite-actions/kube-gateway-api-conformance-tests/action.yaml: Adds the k8sgateway-api-version as input to set the CONFORMANCE_VERSION environment variable used by the conformance target.
  • .github/workflows/conformance-tests.yaml: Creates a Gateway API version matrix for conformance testing in CI.
  • .github/workflows/nightly-tests.yaml: Creates a Gateway API version matrix for nightly testing in CI.
  • .github/workflows/regression-tests.yaml: Creates a Gateway API version matrix for regression testing in CI.
    -.github/workflows/.env/pr-tests/{min,max}_versions.env: Adds Dotenv files to define the source min/max GW API versions used for conformance testing.

Docs changes

N/A
BOT NOTES:
resolves kgateway-dev#10359

@solo-changelog-bot
Copy link

Issues linked to changelog:
kgateway-dev#10359

@@ -53,6 +57,8 @@ runs:
fi
- name: Run the kubernetes gateway API conformance tests
shell: bash
env:
GATEWAY_API_VERSION: ${{ inputs.gateway-api-version }}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-heilbron @nfuden this env var is not getting set causing the CI failure. PTAL and let me know if you see the issue or if you suggest anyone else with more familiarity with CI that can review.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that stands out to me is that we mixing "matrix" and "input" in this composite-action. I prefer your approach, since it makes the action standalone, and unaware that it's called within a matrix. I'm not sure if it causes issues combingin both, but from a mainteance perspective, I think it would be better if we adjusted the syntax to be entirely input based, as you have it

Copy link

github-actions bot commented Nov 26, 2024

Visit the preview URL for this PR (updated for commit 240b7ad):

https://gloo-edge--pr10390-issue-10359-9hzllf0a.web.app

(expires Thu, 23 Jan 2025 16:27:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

- name: Run Conformance Tests
uses: ./.github/workflows/composite-actions/kube-gateway-api-conformance-tests
with:
gateway-api-version: ${{ matrix.gateway-api-version }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/solo-io/gloo/blob/main/.github/workflows/nightly-tests.yaml#L86 we rely on an env file to define inputs for some of our other tests. I think it could be great to do something similar for this job. it can be a separate file, but it would standardize our way of defining these variables and make it easier to keep in sync

@danehans danehans marked this pull request as draft December 3, 2024 19:35
@danehans danehans force-pushed the issue_10359 branch 3 times, most recently from ab977eb to 7bb39ce Compare December 5, 2024 18:00
@danehans danehans marked this pull request as ready for review December 5, 2024 18:00
@danehans danehans force-pushed the issue_10359 branch 2 times, most recently from 1982e91 to 1131566 Compare December 5, 2024 20:10
@sam-heilbron
Copy link

@danehans this PR is out of date, but I can approve once you update it

@danehans
Copy link
Author

@sam-heilbron I am still working through the implementation. I have tried implementing your feedback but no luck thus far. I should have time later today to continue troubleshooting.

@danehans danehans force-pushed the issue_10359 branch 5 times, most recently from 4d1200f to aac5da1 Compare December 11, 2024 01:09
@@ -0,0 +1 @@
k8sgateway_api_version='v1.1.0'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially set this value to v1.0.0 to match .github/workflows/.env/nightly-tests/min_versions.env but the following conformance test consistently failed:

=== RUN   TestConformance/GatewayWithAttachedRoutes/Gateway_listener_should_have_AttachedRoutes_set_even_when_Gateway_has_unresolved_refs
    helpers.go:650: Gateway expected observedGeneration to be updated to 1 for all conditions, only 0/2 were updated. stale conditions are: Accepted (generation 0), Programmed (generation 0)
    helpers.go:655: Conditions matched expectations
    helpers.go:655: Gateway status listeners matched expectations
    helpers.go:681: Accepted condition set to Status True with Reason Accepted, expected Status False
    helpers.go:681: Accepted was not in conditions list [[{ResolvedRefs False 1 2024-12-11 00:21:38 +0000 UTC BackendNotFound Service "does-not-exist" not found} {Accepted True 1 2024-12-11 00:21:38 +0000 UTC Accepted }]]

^ is a test bug that was fixed in kubernetes-sigs/gateway-api@8bd5491 (included in v1.1.0). @sam-heilbron should .github/workflows/.env/nightly-tests/min_versions.env be updated accordingly?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weren't we previously passing conformance on 1.0.0? (before we upgraded to 1.1 / 1.2)

@danehans
Copy link
Author

danehans commented Jan 6, 2025

@sam-heilbron @jenshu PTAL

cc: @josh-pritchard

@jenshu jenshu requested a review from sheidkamp January 7, 2025 16:22
Copy link

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question about what tests run on PRs, but generally the changes look good to me

@@ -33,12 +33,27 @@ jobs:
- ${{ inputs.image-variant }}
version:
- ${{ inputs.version }}
version-files:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a shift in behavior. Previously, we ran min/max versions on an async schedule (ie not on PRs) and it seems we are shifting that to run this on PRs. This isn't bad per se, I just wanted to call it out. What are we trying to achieve by running min/max on PRs, that running these nightly did not achieve? Or, if we didn't run the min/max for conformance tests nightly, perhaps we should do that instead?

Copy link

@sheidkamp sheidkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall makes sense, a few comments.

Can you also kick off a nightly run for this branch and link the results in the PR description?

uses: falti/[email protected]
id: dotenv
with:
path: ${{ matrix.version-files.file }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the matrix (line 484) does not have version-files defined and is getting its versions from the inline kube-version array. I think k8sgateway_api_version can get added to the nightly env files and then those files can be used here.

uses: falti/[email protected]
id: dotenv
with:
path: ${{ matrix.version-files.file }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above (version-files not defined in matrix).

Also, this won't work until the env files are backported to 1.18.x

- label: 'min'
file: './.github/workflows/.env/pr-tests/min_versions.env'
- label: 'max'
file: './.github/workflows/.env/pr-tests/max_versions.env'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the kube-version versions into the env file as well so we don't mix ways of tracking versions? I wouldn't be opposed to re-using the nightly env files, though renaming the files to be more accurate would be tricky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Multiple Gateway API Versions in CI
4 participants