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

Add steps for running conformance tests on Envoy Gateway #3266

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

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Aug 13, 2024

/kind documentation
/kind test
/area conformance

What this PR does / why we need it:

  • Adds steps showing how to run conformance tests on Envoy Gateway
  • This should help
    • Increase velocity of conformance test contributions since tests can be easier to validate
    • Give more confidence to conformance test PR reviewers by knowing the outcome of the test result on an implementation
  • The current documentation on https://gateway-api.sigs.k8s.io/concepts/conformance/#3-conformance-tests does not seem adequate enough for conformance tests contributors to validate their changes on implementations

@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 13, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arkodg
Once this PR has been reviewed and has the lgtm label, please assign shaneutt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@arkodg arkodg marked this pull request as draft August 13, 2024 01:39
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 13, 2024
Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg marked this pull request as ready for review August 29, 2024 00:17
@k8s-ci-robot 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 Aug 29, 2024
Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg
Copy link
Contributor Author

arkodg commented Sep 10, 2024

ptal @robscott

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @arkodg!

@@ -0,0 +1,100 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

I'm torn on this script. It feels like it should either exist in Envoy Gateway or in a more generic place within GW API. If it were within GW API, it seems like it could just be a generic script for creating a kind cluster with metal LB for the sake of testing with ~any in-cluster implementation of Gateway API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah would you prefer a curl / wget that fetches this script and runs it ?

yah its a generic script which can be used for any implementation - should i move it under hack/implementations/common ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that a curl or wget that fetches this script from EG is probably better for now.

It might be good to have something similar in the Gateway API repo, but that's probably a larger discussion that we shouldn't block the rest of this PR for.

Copy link
Member

Choose a reason for hiding this comment

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

We already have a place where such a piece of documentation should be put: https://github.com/kubernetes-sigs/gateway-api/tree/main/conformance/reports/v1.1.0/envoy-gateway. I guess that running CONFORMANCE_REPORT_PATH=conformance-report-k8s.yaml make experimental-conformance reproduces the same result as here (and if not, I think we should update the reports README instead of putting this script along with README here). I lean toward leaving this kind of script out of the Gateway API repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

over the past years, Ive seen many contributors find it hard to test their PRs associated with adding/modifying conformance tests, This was an attempt to make it easier for a contributor to say Hey I tested this Conformance Test PR on these X platforms by following the steps in ..., thereby making it easier for new contributors to come in and pick up conformance issues, contributions seems to be on the decline rn.

The steps in the conformance tests are not sufficient.

Happy to close this PR if the maintainers prefer that

Copy link
Member

@shaneutt shaneutt Sep 18, 2024

Choose a reason for hiding this comment

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

README's for our reporting implementations seem entirely fine (and quite helpful for users), but I don't want to set a precedent for implementation-specific executable code as something that we host here in the upstream (and if that precedent has already been set somehow, we should re-look at that).

Copy link
Member

@robscott robscott Sep 18, 2024

Choose a reason for hiding this comment

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

but I don't want to set a precedent for implementation-specific executable code as something that we host here in the upstream

Mostly agree with this reasoning, but I think this specific script would be useful for ~any implementation that could be deployed inside a cluster. I think I'd be fine with either bumping this script up to a more generic place that could be used to test with any in-cluster implementation, or just updating the directions to describe how to run conformance tests with Envoy Gateway by pulling this script in from a remote location.

I'd also agree that we shouldn't open this broader idea to just any implementation, maybe we can limit it to implementations that have a solid track record of delivering passing conformance reports?

Copy link
Member

Choose a reason for hiding this comment

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

I can be persuaded, putting it in a general place and so forth would certainly be preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ive moved the create-cluster.sh to hack/implementations/common so any implementation can peruse it

@robscott
Copy link
Member

/retest

@shaneutt shaneutt self-requested a review September 18, 2024 13:16
@shaneutt shaneutt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 18, 2024
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Strong preference to not host the script here, but the README to explain how the tests are run for this are very appreciated, thank you for taking the time to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/documentation Categorizes issue or PR as related to documentation. kind/test priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants