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

conformance: initial UDPRoute test #2661

Merged
merged 8 commits into from
Mar 22, 2024

Conversation

cnvergence
Copy link
Contributor

@cnvergence cnvergence commented Dec 8, 2023

What type of PR is this?

/kind test
/area conformance

What this PR does / why we need it:

  • initial conformance tests for UDPRoute
  • dedup code for Route Handler

Which issue(s) this PR fixes:

Fixes #1792

Does this PR introduce a user-facing change?:

NONE

@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. release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 8, 2023
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 8, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @cnvergence. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 8, 2023
@cnvergence cnvergence marked this pull request as ready for review December 12, 2023 09:14
@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 Dec 12, 2023
@k8s-ci-robot k8s-ci-robot requested a review from kflynn December 12, 2023 09:14
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023
@shaneutt
Copy link
Member

Hey @cnvergence thanks for adding this! Out of curiosity, what implementation are you adding this on the behalf of?

Also: not a big problem in this instance, but just note that you did create a PR intended to resolve an issue that was already assigned. In the future please comment on the issue your interest in working on it prior to starting work as this helps us to organize better and reduce overlap where multiple people are working on the same thing.

@cnvergence
Copy link
Contributor Author

Hey @shaneutt
Ah, sorry, I did not see it was assigned.
I was going to create a new issue for this and propose UDPRoute conformance test suite, but found the older issue that I linked here.
It's coming from Envoy Gateway implementation: envoyproxy/gateway#2011

@shaneutt
Copy link
Member

Hey @shaneutt Ah, sorry, I did not see it was assigned. I was going to create a new issue for this and propose UDPRoute conformance test suite, but found the older issue that I linked here. It's coming from Envoy Gateway implementation: envoyproxy/gateway#2011

Yeah no worries! Just something to think about in the future. I have some active work on this within https://github.com/kubernetes-sigs/blixt, which you and I might need to coordinate on a little bit (after the holidays). But in any case, very much appreciate your interest and willingness to jump on this, and thanks for linking the relevant envoy gateway issue that's good context.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023
@cnvergence
Copy link
Contributor Author

@shaneutt sounds good to me

@arkodg
Copy link
Contributor

arkodg commented Dec 15, 2023

thanks @cnvergence, this looks good, can you confirm if this test passes on any implementation ?
@shaneutt is there anything specific in this PR you'd like changed ?

Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Thank you for your PR, @cnvergence!

conformance/utils/kubernetes/helpers.go Outdated Show resolved Hide resolved
conformance/base/manifests.yaml Outdated Show resolved Hide resolved
@cnvergence cnvergence requested a review from mlavacca December 21, 2023 10:30
@cnvergence
Copy link
Contributor Author

thanks @mlavacca, @arkodg, I have applied some improvements after the review
I will now check on passing those tests on different implementations

@cnvergence
Copy link
Contributor Author

So far I tested it on:

  • envoy-gateway
  • kong-ingress-controller
go test  -v ./conformance -run TestConformance -args -supported-features=UDPRoute
=== RUN   TestConformance
    conformance_test.go:60: Running conformance tests with gateway-conformance GatewayClass
         cleanup: true
         debug: false
         enable all features: false 
         supported features: [UDPRoute]
         exempt features: []
    suite.go:168: Test Setup: Ensuring GatewayClass has been accepted
    suite.go:174: Test Setup: Applying base manifests
    apply.go:272: Creating gateway-conformance-infra Namespace
    apply.go:272: Creating same-namespace Gateway
    apply.go:272: Creating same-namespace-with-https-listener Gateway
    apply.go:272: Creating all-namespaces Gateway
    apply.go:272: Creating backend-namespaces Gateway
    apply.go:272: Creating infra-backend-v1 Service
    apply.go:272: Creating infra-backend-v1 Deployment
    apply.go:272: Creating infra-backend-v2 Service
    apply.go:272: Creating infra-backend-v2 Deployment
    apply.go:272: Creating infra-backend-v3 Service
    apply.go:272: Creating infra-backend-v3 Deployment
    apply.go:272: Creating tls-backend Service
    apply.go:272: Creating tls-backend Deployment
    apply.go:272: Creating gateway-conformance-app-backend Namespace
    apply.go:272: Creating tls-backend Service
    apply.go:272: Creating tls-backend Deployment
    apply.go:272: Creating app-backend-v1 Service
    apply.go:272: Creating app-backend-v1 Deployment
    apply.go:272: Creating app-backend-v2 Service
    apply.go:272: Creating app-backend-v2 Deployment
    apply.go:272: Creating gateway-conformance-web-backend Namespace
    apply.go:272: Creating web-backend Service
    apply.go:272: Creating web-backend Deployment
    apply.go:272: Creating gateway-conformance-udp Namespace
    apply.go:272: Creating coredns Service
    apply.go:272: Creating coredns Deployment
    apply.go:272: Creating coredns ConfigMap
    apply.go:272: Creating udp-gateway Gateway
    suite.go:177: Test Setup: Applying programmatic resources
    apply.go:223: Creating certificate 
    apply.go:223: Creating tls-validity-checks-certificate 
    apply.go:223: Creating tls-passthrough-checks-certificate 
    apply.go:223: Creating tls-passthrough-checks-certificate 
=== RUN   TestConformance/UDPRoute
    suite.go:250: Applying tests/udproute-simple.yaml
    apply.go:272: Creating udp-coredns UDPRoute
=== RUN   TestConformance/UDPRoute/Simple_UDP_request_matching_UDPRoute_should_reach_coredns_backend
    helpers.go:776: Route gateway-conformance-udp/udp-coredns expected 1 Parents got 0
    helpers.go:776: Conditions matched expectations
    helpers.go:776: Route gateway-conformance-udp/udp-coredns Parents matched expectations
    udproute-simple.go:56: performing DNS query foo.bar.com. on 172.18.255.200:5300
=== NAME  TestConformance/UDPRoute
    apply.go:280: Deleting udp-coredns UDPRoute
    --- PASS: TestConformance/UDPRoute (2.06s)
        --- PASS: TestConformance/UDPRoute/Simple_UDP_request_matching_UDPRoute_should_reach_coredns_backend (2.03s)
PASS

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.

/approve
thanks !

@cnvergence
Copy link
Contributor Author

hey @shaneutt, combing back to this, would like to coordinate with you :)

@shaneutt
Copy link
Member

shaneutt commented Feb 6, 2024

Sounds good, what did you have in mind? Where are things at currently?

@cnvergence
Copy link
Contributor Author

Refactored the code a bit as per the review and manually tested out this simple UDP scenario on two implementations that support UDPRoute: kong-ingress-gateway and envoy-gateway.

Would like to ask about what's left to move this PR.

Yeah no worries! Just something to think about in the future. I have some active work on this within https://github.com/kubernetes-sigs/blixt, which you and I might need to coordinate on a little bit (after the holidays). But in any case, very much appreciate your interest and willingness to jump on this, and thanks for linking the relevant envoy gateway issue that's good context.

@cnvergence cnvergence requested a review from aryan9600 February 6, 2024 20:58
@shaneutt shaneutt added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 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.

/approve

/cc @robscott @youngnick for lgtm

@k8s-ci-robot
Copy link
Contributor

@shaneutt: GitHub didn't allow me to request PR reviews from the following users: for, lgtm.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/approve

/cc @robscott @youngnick for lgtm

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2024
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2024
@cnvergence
Copy link
Contributor Author

rebased PR, PTAL again
@shaneutt @robscott

@shaneutt
Copy link
Member

Thanks for adding this, let's move on this!

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, aryan9600, cnvergence, shaneutt

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

The pull request process is described 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. 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.

Initial conformance tests for UDPRoute
6 participants