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

Adds ReferenceGrant Tests for TCPRoute #10419

Merged
merged 7 commits into from
Dec 5, 2024
Merged

Conversation

danehans
Copy link

@danehans danehans commented Dec 2, 2024

Adds ReferenceGrant tests for TCPRoute coverage.

Checklist:

@danehans
Copy link
Author

danehans commented Dec 2, 2024

Reviewer note: Only review commit 1a1430b since this PR requires #10418.

Copy link

github-actions bot commented Dec 2, 2024

Visit the preview URL for this PR (updated for commit 7425d74):

https://gloo-edge--pr10419-issue-10414-issue-73-vfi1nodh.web.app

(expires Thu, 12 Dec 2024 15:13:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

@danehans danehans force-pushed the issue_10414_issue_7309 branch from 1a1430b to 8d96b69 Compare December 2, 2024 03:43
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.

Loooking good!

Copy link

@lgadban lgadban left a comment

Choose a reason for hiding this comment

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

I think I'm missing how this actually changes the behavior given that the RefGrant support is baked into the query engine

@danehans danehans force-pushed the issue_10414_issue_7309 branch from 8d96b69 to 1718452 Compare December 2, 2024 20:28
@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/7309

@danehans danehans force-pushed the issue_10414_issue_7309 branch from 1718452 to 281ace0 Compare December 2, 2024 20:38
@danehans
Copy link
Author

danehans commented Dec 2, 2024

@lgadban thanks for the review. I resolved your feedback in commit 1718452, PTAL.

Copy link

@lgadban lgadban left a comment

Choose a reason for hiding this comment

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

I think I'm still missing what is the functional change?

It looks like the only difference was the status reporting -- was the previous behavior incorrectly reporting status only?

@danehans
Copy link
Author

danehans commented Dec 3, 2024

The Kubernetes Tests / End-to-End (cluster-six) CI job failed due to:

--- FAIL: TestK8sGatewayNoValidation (269.72s)
    --- PASS: TestK8sGatewayNoValidation/ListenerOptions (49.36s)
        --- PASS: TestK8sGatewayNoValidation/ListenerOptions/TestConfigureListenerOptions (0.66s)
    --- PASS: TestK8sGatewayNoValidation/RouteOptions (68.27s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestConfigureInvalidRouteOptionsWithTargetRef (2.29s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestConfigureRouteOptionsWithFilterExtension (2.58s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestConfigureRouteOptionsWithMultipleFilterExtensionManualSetup (0.54s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestConfigureRouteOptionsWithMultipleTargetRefManualSetup (2.56s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestConfigureRouteOptionsWithTargetRef (0.39s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestConfigureRouteOptionsWithTargetRefAndFilterExtension (2.67s)
        --- PASS: TestK8sGatewayNoValidation/RouteOptions/TestOptionsMerge (12.06s)
    --- FAIL: TestK8sGatewayNoValidation/VirtualHostOptions (69.00s)
        --- FAIL: TestK8sGatewayNoValidation/VirtualHostOptions/TestConfigureInvalidVirtualHostOptions (22.34s)

Note: The TCPRoute e2e tests passed:

--- PASS: TestK8sGateway (353.90s)
    --- PASS: TestK8sGateway/CRDCategories (0.32s)
        --- PASS: TestK8sGateway/CRDCategories/TestCommonCategory (0.15s)
    --- PASS: TestK8sGateway/TCPRouteServices (190.34s)
        --- PASS: TestK8sGateway/TCPRouteServices/TestConfigureTCPRouteBackingDestinations (190.34s)
            --- PASS: TestK8sGateway/TCPRouteServices/TestConfigureTCPRouteBackingDestinations/SingleServiceTCPRoute (45.80s)
            --- PASS: TestK8sGateway/TCPRouteServices/TestConfigureTCPRouteBackingDestinations/MultiServicesTCPRoute (41.81s)
            --- PASS: TestK8sGateway/TCPRouteServices/TestConfigureTCPRouteBackingDestinations/CrossNamespaceTCPRouteWithReferenceGrant (52.47s)
            --- PASS: TestK8sGateway/TCPRouteServices/TestConfigureTCPRouteBackingDestinations/CrossNamespaceTCPRouteWithoutReferenceGrant (50.26s)
    --- PASS: TestK8sGateway/HTTPRouteServices (89.36s)
        --- PASS: TestK8sGateway/HTTPRouteServices/TestConfigureHTTPRouteBackingDestinationsWithService (46.20s)
        --- PASS: TestK8sGateway/HTTPRouteServices/TestConfigureHTTPRouteBackingDestinationsWithServiceAndWithoutTCPRoute (43.16s)

@danehans danehans changed the title Adds ReferenceGrant Support for TCPRoute Adds ReferenceGrant Tests for TCPRoute Dec 3, 2024
@danehans
Copy link
Author

danehans commented Dec 3, 2024

I think I'm still missing what is the functional change?

@lgadban thanks for calling this out. After repro'ing https://github.com/solo-io/solo-projects/issues/7309, I confirmed that the issue was resolved by k8sgateway#10365. I have updated the title and description of this PR.

@sam-heilbron we may still want to consider this a release blocker since it fixes the root cause of the TCPRoute e2e flakes and bumps the GW API CRDs to match CI (v1.1 -> v1.2). I can pull out the commits into separate PRs if needed, LMK.

@danehans danehans force-pushed the issue_10414_issue_7309 branch from 89b6433 to 1f09c3e Compare December 4, 2024 16:20
@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/7309
#10434

@danehans danehans force-pushed the issue_10414_issue_7309 branch 2 times, most recently from 3f82175 to 2981cc0 Compare December 4, 2024 16:26
@danehans danehans requested a review from lgadban December 4, 2024 17:30
@danehans danehans force-pushed the issue_10414_issue_7309 branch from 2981cc0 to b86fb3f Compare December 4, 2024 20:18
@danehans
Copy link
Author

danehans commented Dec 4, 2024

Rebased since #10435 merged to fix the CI flake affecting this PR.

@danehans danehans requested a review from nfuden December 4, 2024 20:20
Copy link

@jenshu jenshu left a comment

Choose a reason for hiding this comment

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

how did you regenerate the crds in projects/gateway2/crds?

@danehans
Copy link
Author

danehans commented Dec 5, 2024

how did you regenerate the crds in projects/gateway2/crds

The old school way:

$ curl -sLo ./projects/gateway2/crds/gateway-crds.yaml https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.2.0/experimental-install.yaml

@soloio-bulldozer soloio-bulldozer bot merged commit d0fe477 into main Dec 5, 2024
19 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the issue_10414_issue_7309 branch December 5, 2024 16:15
@danehans
Copy link
Author

danehans commented Dec 5, 2024

how did you regenerate the crds in projects/gateway2/crds

@jenshu #10446 adds a target for automating this process, PTAL.

danehans added a commit that referenced this pull request Dec 5, 2024
Signed-off-by: Daneyon Hansen <[email protected]>
Co-authored-by: Sam Heilbron <[email protected]>
soloio-bulldozer bot pushed a commit that referenced this pull request Dec 6, 2024
Signed-off-by: Daneyon Hansen <[email protected]>
Co-authored-by: Sam Heilbron <[email protected]>
Co-authored-by: Jenny Shu <[email protected]>
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.

e2e: SingleServiceTCPRoute Test Case
5 participants