conformance test: CORS #3739

zhaohuabing · 2025-04-09T02:25:09Z

What type of PR is this?
/kind test
/area conformance-test

What this PR does / why we need it:
This PR adds conformance tests for HTTPCORSFilter.

Which issue(s) this PR fixes:

Related #3649

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2025-04-09T02:25:18Z

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

zhaohuabing · 2025-05-06T04:01:47Z

Hi @mikemorris @sunjayBhatia @youngnick Is there anything I can do to help the review? This is the first time creating a conformance test PR, so there is a good chance I may have missed something that's blocking the review. Please feel free to let me know if there's anything I can address. Thanks!

cc @arkodg

pkg/features/httproute.go

conformance/tests/httproute-cors.yaml

conformance/tests/httproute-cors.go

snorwin · 2025-05-15T08:02:20Z

@zhaohuabing I verified the new CORS tests against our implementation and identified a difference in how the access-control-request-headers is mapped. See the comment here: #3739 (comment).

Just a heads-up: depending on the outcome of #3648, the tests might need to be adapted or extended accordingly.

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

k8s-ci-robot · 2025-08-28T12:17:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: snorwin, zhaohuabing
Once this PR has been reviewed and has the lgtm label, please ask for approval from shaneutt. For more information see the 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:

conformance/OWNERS

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

zhaohuabing · 2025-08-28T12:24:56Z

@rikatz I rebased and might have broken conformance/utils/http/http.go - I will wrap this up this weekend.

Thanks!

shaneutt · 2025-08-28T12:33:04Z

Thank you @zhaohuabing! Sorry that this lagged for a while. We're trying to code freeze next wednesday for v1.4.0 and we appreciate you jumping back in 🖖

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

snorwin

I haven’t had the time to review the recent changes in detail. However, I can confirm that the Airlock Microgateway implementation successfully passes all HTTPRouteCORS conformance tests.

rikatz · 2025-09-02T12:27:42Z

/release-note-none

rikatz · 2025-09-02T12:47:30Z

conformance/utils/http/http.go

-		if expected.ExpectedRequest == nil {
-			expected.ExpectedRequest = &ExpectedRequest{Request: expected.Request}
-		}
+	if cRes.StatusCode == 200 || cRes.StatusCode == 204 {


it is a bit confusing here to understand why the code "204" was added here, just then to be checked on line 391.

What if you split the cases here as:

just code 200 specifics (from line 328 to 391)

code 200 or 204 specifics (from 391 onwards)

Also, please add a comment why we care now about code 204, that is used by CORS preflight tests (should this be an option instead of implicit on 200 returns?)

rikatz · 2025-09-02T12:58:40Z

/hold
/assign
/cc

I am taking a look into it. I think the test is fine, but I have some concerns on changing the http library, so I will take a deeper look into the changes :)

rikatz · 2025-09-02T13:55:50Z

conformance/tests/httproute-cors.go

+					HeadersWithMultipleValues: map[string][]string{
+						"access-control-allow-origin": {"https://www.bar.com"},
+						"access-control-allow-methods": {
+							"GET, OPTIONS",


is the order relevant here or is it because the assertion is not able to check if the array contains an item?

If this is the second, I would prefer that we have single entries here, and that we fix the http library to check if the array contains both items

rikatz · 2025-09-02T15:37:23Z

conformance/utils/http/http.go

 			if cRes.Headers == nil {
-				return fmt.Errorf("no headers captured, expected %v", len(expected.ExpectedRequest.Headers))
+				return fmt.Errorf("no headers captured, expected %v", len(expected.Response.Headers))


shouldn't this error contain the len of HeadersWithMultipleValues as well?

rikatz · 2025-09-02T17:13:20Z

conformance/utils/http/http.go

+						return fmt.Errorf("expected %s header to be set to one of %v, got %s", name, expectedVals, actualValStr)
+					}
+				}
+			} else {


I think this else is unnecessary, right?

Like, you can check the headerswithMultipleValues, and then if Headers is also set you check it.

I think the whole if expected.Response.HeadersWithMultipleValues != nil { can be removed, you simply iterate over the HeadersWithMultipleValues first, and then on the Response.Headers, wdyt?

rikatz · 2025-09-02T17:14:35Z

conformance/utils/http/http.go

+						}
+					}
+					matched := false
+					for _, expectedVal := range expectedVals {


can be replaced by https://pkg.go.dev/slices#Contains

zhaohuabing marked this pull request as draft April 9, 2025 02:25

k8s-ci-robot requested review from mikemorris and sunjayBhatia April 9, 2025 02:25

zhaohuabing force-pushed the cors-test branch from 51d1e15 to 1772aa1 Compare April 9, 2025 02:27

zhaohuabing changed the title ~~test: cors~~ conformance test: CORS Apr 9, 2025

zhaohuabing force-pushed the cors-test branch from 1772aa1 to 1cb6fac Compare April 9, 2025 02:51

zhaohuabing force-pushed the cors-test branch from 1cb6fac to 1d639f0 Compare April 24, 2025 11:57

zhaohuabing marked this pull request as ready for review April 24, 2025 12:00

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 Apr 24, 2025

k8s-ci-robot requested a review from youngnick April 24, 2025 12:00

zhaohuabing force-pushed the cors-test branch from d041501 to aa41160 Compare April 25, 2025 07:35

snorwin reviewed May 9, 2025

View reviewed changes

pkg/features/httproute.go Outdated Show resolved Hide resolved

snorwin mentioned this pull request May 9, 2025

conformance: initial draft for CORS filter tests #3740

Closed

2 tasks

snorwin reviewed May 9, 2025

View reviewed changes

conformance/tests/httproute-cors.yaml Show resolved Hide resolved

zhaohuabing requested a review from snorwin May 14, 2025 08:00

snorwin reviewed May 15, 2025

View reviewed changes

conformance/tests/httproute-cors.go Outdated Show resolved Hide resolved

snorwin reviewed May 15, 2025

View reviewed changes

conformance/tests/httproute-cors.go Outdated Show resolved Hide resolved

snorwin reviewed May 15, 2025

View reviewed changes

conformance/tests/httproute-cors.go Show resolved Hide resolved

zhaohuabing force-pushed the cors-test branch 2 times, most recently from e6c17a3 to d0e2f9a Compare May 15, 2025 11:01

zhaohuabing added 3 commits August 28, 2025 12:14

address comment

712a7a7

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

address comment

4a3b284

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

fix lint

c3624c8

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

zhaohuabing force-pushed the cors-test branch from 164c5a0 to c3624c8 Compare August 28, 2025 12:17

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 28, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2025

fix test

4150618

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2025

address comment

a224d52

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

snorwin reviewed Sep 1, 2025

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 2, 2025

rikatz reviewed Sep 2, 2025

View reviewed changes

k8s-ci-robot assigned rikatz Sep 2, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2025

k8s-ci-robot requested a review from rikatz September 2, 2025 12:58

rikatz reviewed Sep 2, 2025

View reviewed changes

shaneutt removed this from Release v1.4.0 Sep 3, 2025

shaneutt modified the milestones: v1.4.0, v1.5.0 Sep 3, 2025

conformance test: CORS #3739

Are you sure you want to change the base?

conformance test: CORS #3739

Conversation

zhaohuabing commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 9, 2025

Uh oh!

zhaohuabing commented May 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snorwin commented May 15, 2025

Uh oh!

k8s-ci-robot commented Aug 28, 2025

Uh oh!

zhaohuabing commented Aug 28, 2025

Uh oh!

shaneutt commented Aug 28, 2025

Uh oh!

snorwin left a comment

Choose a reason for hiding this comment

Uh oh!

rikatz commented Sep 2, 2025

Uh oh!

rikatz Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

rikatz commented Sep 2, 2025

Uh oh!

rikatz Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

rikatz Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

rikatz Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

rikatz Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhaohuabing commented Apr 9, 2025 •

edited

Loading