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.

Sign up for GitHub

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

Jump to bottom

feat: add supported features to gateway class #2491

Merged
merged 2 commits into from
May 15, 2024
Merged

feat: add supported features to gateway class #2491

merged 2 commits into from
May 15, 2024

Conversation

levikobi
Copy link
Contributor

What type of PR is this?

feat: add supported features to gateway class.

What this PR does / why we need it:
This PR implements GEP-2162, which is about adding a list of supported features to the status of the gateway class.

@levikobi levikobi requested a review from a team as a code owner January 24, 2024 06:12
@levikobi levikobi force-pushed the gatewayapi-supported-features branch 2 times, most recently from 62df7c9 to 1c581eb Compare January 24, 2024 06:15
@arkodg
Copy link
Contributor

arkodg commented Jan 24, 2024

hey @levikobi suggest running make testdata, this change probably ends up updates a bunch of golden yamldata

@levikobi levikobi force-pushed the gatewayapi-supported-features branch from 93e7c03 to 6be8154 Compare January 25, 2024 14:46
@levikobi
Copy link
Contributor Author

hey @levikobi suggest running make testdata, this change probably ends up updates a bunch of golden yamldata

Thank you for your review @arkodg!
I addressed your comments, make testdata also seems to work now locally.

@levikobi levikobi force-pushed the gatewayapi-supported-features branch from 6be8154 to 8716c9a Compare January 26, 2024 06:37
@levikobi levikobi force-pushed the gatewayapi-supported-features branch from 8716c9a to 00439a5 Compare January 28, 2024 14:20
@arkodg
Copy link
Contributor

arkodg commented Jan 30, 2024

@levikobi your changes might have affected e2e runs

@levikobi
Copy link
Contributor Author

levikobi commented Jan 31, 2024 •
edited
Loading

@levikobi your changes might have affected e2e runs

Apparently the SupportedFeatures list has a kubebuilder validation that checks for a hard-coded list of features (see here.
The supported features I took from the conformance suite contain features that aren't part of this list, which makes the update fail. That's why only e2e fails and not unit tests.

I saw that Cillium hard-coded their list of supported features. I'll dig around to see what others have done to develop a proper solution.
@arkodg If you have any preferences, please let me know.

@LiorLieberman LiorLieberman mentioned this pull request Feb 6, 2024
Open
@levikobi
Copy link
Contributor Author

levikobi commented Feb 6, 2024 •
edited
Loading

The issue around the list of supported features is solved upstream via kubernetes-sigs/gateway-api#2754 so I believe the tests should pass now. @arkodg can you please trigger another run?

@arkodg
Copy link
Contributor

arkodg commented Feb 6, 2024

@levikobi dont we need to wait for a gateway-api release that has the change in CRDs ?
currently pinned to v1.0.0

gateway/go.mod

Line 46 in f21f9ff

sigs.k8s.io/gateway-api v1.0.0

@levikobi
Copy link
Contributor Author

levikobi commented Feb 7, 2024

@levikobi dont we need to wait for a gateway-api release that has the change in CRDs ? currently pinned to v1.0.0

gateway/go.mod

Line 46 in f21f9ff

sigs.k8s.io/gateway-api v1.0.0

Definitely, my bad

@LiorLieberman LiorLieberman mentioned this pull request Feb 20, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Mar 8, 2024
@LiorLieberman
Copy link
Contributor

I think a release should be out relatively soon (1-2 weeks from now). I suggest you keep an eye on it and merge after the release is out

@github-actions github-actions bot removed the stale label Mar 27, 2024
@levikobi
Copy link
Contributor Author

I think a release should be out relatively soon (1-2 weeks from now). I suggest you keep an eye on it and merge after the release is out

Waiting for this release. Once it's out, I'll update this PR (rebase + the change you suggested @LiorLieberman)

@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Apr 28, 2024
@LiorLieberman
Copy link
Contributor

@levikobi i think rc1 is out. 1.1 should be out soon after we get feedback on the rc

@github-actions github-actions bot removed the stale label Apr 28, 2024
@levikobi levikobi force-pushed the gatewayapi-supported-features branch from 00439a5 to 5d7a94c Compare April 30, 2024 05:29
@arkodg
Copy link
Contributor

arkodg commented May 7, 2024

hey @levikobi v1.1.0-rc2 is in, you should be unblocked now

@levikobi levikobi force-pushed the gatewayapi-supported-features branch from 5d7a94c to d7593bd Compare May 9, 2024 06:00
@levikobi
Copy link
Contributor Author

levikobi commented May 9, 2024

Thanks @arkodg
I just updated the PR

@levikobi levikobi force-pushed the gatewayapi-supported-features branch 3 times, most recently from 3d11c02 to 83bbbcb Compare May 9, 2024 07:07
arkodg
arkodg reviewed May 9, 2024
View reviewed changes
internal/cmd/egctl/testdata/translate/out/invalid-envoyproxy.all.yaml Outdated
supportedFeatures:
- GRPCRoute
- Gateway
- GatewayHTTPListenerIsolation
Copy link
Contributor

Choose a reason for hiding this comment

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

the skip test values still showed up here, they shouldnt

Copy link
Contributor Author

@levikobi levikobi May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just to make sure I understand:
We have a list of SkipTests. Should we delete the Features of every skipped test from the final supported features list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also noticed that the list of MeshExtendedFeatures appears in the supported features. This happens since we only specify MeshCoreFeatures in the ExemptFeatures list. It doesn't make sense (correct me if I'm wrong) to have the extended without the core.

Should I add the MeshExtendedFeatures to the ExemptFeatures list?

Copy link
Contributor

@arkodg arkodg May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes we should be deleting them from this list because we don't support it yet :)

Sure let's also add MeshExtendedFeatures to ExemptFeatures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification @arkodg. I've added the requested change.
Please notice though that features such as Gateway and HTTPRoute have been removed (as a dependency of skipped test named GatewayHTTPListenerIsolation).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for highlighting that, that is something worth discussing in upstream since GatewayHTTPListenerIsolation is an extended test

@levikobi levikobi force-pushed the gatewayapi-supported-features branch 5 times, most recently from b0b5e20 to 96f69b0 Compare May 13, 2024 04:26
arkodg
arkodg previously approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

we probably need a follow up to decide whether this is opt in / opt out or always on once we gather more feedback from users

@arkodg arkodg requested review from a team May 13, 2024 22:32
@levikobi
Copy link
Contributor Author

LGTM thanks !

we probably need a follow up to decide whether this is opt in / opt out or always on once we gather more feedback from users

Thanks for the multiple reviews @arkodg!
I'm in for the follow up

@levikobi levikobi force-pushed the gatewayapi-supported-features branch from 96f69b0 to a84d146 Compare May 15, 2024 05:39
@levikobi levikobi force-pushed the gatewayapi-supported-features branch from a84d146 to 09a600c Compare May 15, 2024 05:39
shawnh2
shawnh2 approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@shawnh2
Copy link
Contributor

shawnh2 commented May 15, 2024

/retest

@arkodg arkodg merged commit 02e9367 into envoyproxy:main May 15, 2024
24 checks passed
@levikobi levikobi mentioned this pull request May 19, 2024
Closed
@levikobi levikobi deleted the gatewayapi-supported-features branch June 1, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiorLieberman LiorLieberman LiorLieberman left review comments

@shawnh2 shawnh2 shawnh2 approved these changes

@cnvergence cnvergence cnvergence approved these changes

@zirain zirain zirain approved these changes

@arkodg arkodg arkodg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@levikobi @arkodg @LiorLieberman @shawnh2 @zirain @cnvergence