conformance: add test to check for proper cors allow-credentials behvior #3990

EyalPazz · 2025-08-13T08:07:42Z

/kind test
/area conformance-test

What this PR does / why we need it:
Test for proper cors allow-credentials behavior, and add the CORS Feature
Which issue(s) this PR fixes:

Fixes #3946

Does this PR introduce a user-facing change?:

NONE

…vior

EyalPazz · 2025-08-13T11:28:35Z

/retest

EyalPazz · 2025-08-13T11:31:49Z

/ok-to-test

EyalPazz · 2025-08-13T11:32:06Z

/retest

EyalPazz · 2025-08-13T11:32:47Z

/test CRD Validation / CEL (v1.33.0, experimental) (pull_request)

k8s-ci-robot · 2025-08-13T11:32:49Z

@EyalPazz: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-gateway-api-test

/test pull-gateway-api-verify

Use /test all to run all jobs.

In response to this:

/test CRD Validation / CEL (v1.33.0, experimental) (pull_request)

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.

EyalPazz · 2025-08-13T11:33:07Z

/test all

shaneutt · 2025-08-19T19:32:46Z

/cc @lianglli

shaneutt

Looks good

/approve

A few minor comments. Ideally I would like to hold until we have some feedback on running the tests on at least one implementation:

/hold

/cc @robscott @lianglli

conformance/tests/cors-allow-credentials-behavior.go

shaneutt · 2025-08-25T19:13:50Z

conformance/tests/cors-allow-credentials-behavior.go

+		origin := "https://app.example"
+
+		testCases := []http.ExpectedResponse{
+			{


nit: it's good to use the TestCaseName here when you can. We kind of (ab)use it in various places to provide a description explaining the tests intent.

Pascal or Camel case?

Actually several of them are basically just descriptions with spaces and everything. We may even want to rename it Description (or I suppose add Description) but in the meantime, if you wanna just look how the bulk of them are done and follow that that's fine. We need to revisit these things (but that doesn't need to be done as part of this specific PR).

shaneutt · 2025-08-25T19:19:16Z

conformance/tests/cors-allow-credentials-behavior.go

+			// Declare tc here to avoid loop variable
+			// reuse issues across parallel tests.


We have a linter checking for this, right? 🤔

I don't think so? are you talking about copyloopvar maybe?

pkg/features/httproute.go

rikatz · 2025-08-25T23:00:20Z

conformance/tests/cors-allow-credentials-behavior.go

+
+var CORSAllowCredentialsBehavior = suite.ConformanceTest{
+	ShortName:   "CORSAllowCredentialsBehavior",
+	Description: "Validate ACA-Credentials responses",


This needs a better description, what is it doing, what is expected, what kind of validation will happen here

Does this seem ok?
Validate ACA-Credentials responses, the gateway needs to return Access-Control-Allow-Credentials: true for credentialed CORS requests when enabled on the gateway, and omit it when disabled

much better! thank you!

rikatz · 2025-08-25T23:06:24Z

conformance/tests/cors-allow-credentials-behavior.go

+	Features: []features.FeatureName{
+		features.SupportGateway,
+		features.SupportHTTPRoute,
+		features.SupportHTTPRouteCORS,


this reminds me: we don't have any other CORS test. Wouldn't be better to either add those tests as well (support Cors, then test authorization)? Maybe this test should cover the whole extension, eg.:

different allowed origins - CORS doesn't support a list of origins, but we say we do on our API, so this should be tested, see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Access-Control-Allow-Origin and

gateway-api/apis/v1/httproute_types.go

Line 1377 in 110bcaf

// When the `AllowOrigins` field is configured with multiple origins, it

Does it support different methods? See

gateway-api/apis/v1/httproute_types.go

Line 1425 in 110bcaf

// AllowMethods indicates which HTTP methods are supported for accessing the

Does it respect the Header max age? See

gateway-api/apis/v1/httproute_types.go

Line 1563 in 110bcaf

MaxAge int32 `json:"maxAge,omitempty"`

My point is that while the authorization test is desired, we lack some basic CORS conformance test that I think could be part of this change

(but also, if we feel comfortable with merging this and following up, just be sure to open an issue to track the lack of basic cors tests)

don't mind either way — I just don't know if I'll have time in the next few days, so it depends on whether we want to get it merged now. @shaneutt

okie, let's wait for Shane to chime in! thanks!

rikatz · 2025-08-25T23:09:41Z

conformance/tests/cors-allow-credentials-behavior.go

+				},
+				Response: http.Response{
+					StatusCode:    200,
+					AbsentHeaders: []string{"Access-Control-Allow-Credentials"},


do you wanna follow the same test as bellow here and check if the Access-Control-Allow-Origin reflects what you added?

conformance/tests/cors-allow-credentials-behavior.yaml

rikatz · 2025-08-25T23:26:21Z

/hold

The test is will not work as of today, because the manifest sets a nullable field as false and confuses CEL and other parts.

We may want to fix the CORS field to either be a string, or not be a bool pointer and accept it can be true or false, but not "nullable"

rikatz · 2025-08-25T23:44:45Z

/hold cancel
@shaneutt leaving the final approval for you, I think it would be good to test other CORS cases here (eg.: different origins, different methods, etc), as we are here and it means just creating some extra path/match entries :)

shaneutt

/approve
/lgtm

@EyalPazz mentioned not necessarily having any extra time in the above comments, so let's merge for now and we can follow-up 👍

k8s-ci-robot · 2025-08-26T19:15:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EyalPazz, 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:

~~OWNERS~~ [shaneutt]

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

conformance: add test to check for proper cors allow-credentials beha…

f832644

…vior

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance-test Issues or PRs related to Conformance tests. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 13, 2025

k8s-ci-robot requested review from kflynn and mlavacca August 13, 2025 08:07

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

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 13, 2025

chore: rephrase description

c062aaf

shaneutt added this to Release v1.4.0 Aug 19, 2025

shaneutt moved this to Review in Release v1.4.0 Aug 19, 2025

shaneutt added this to the v1.4.0 milestone Aug 19, 2025

shaneutt self-assigned this Aug 19, 2025

shaneutt requested a review from robscott August 19, 2025 19:32

kubernetes-sigs deleted a comment from k8s-ci-robot Aug 19, 2025

shaneutt approved these changes Aug 25, 2025

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 25, 2025

kubernetes-sigs deleted a comment from k8s-ci-robot Aug 25, 2025

rikatz reviewed Aug 25, 2025

View reviewed changes

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

fix: SupportHTTPRouteCORS FeatureName

47db9c8

rikatz reviewed Aug 25, 2025

View reviewed changes

conformance/tests/cors-allow-credentials-behavior.yaml Show resolved Hide resolved

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

shaneutt approved these changes Aug 26, 2025

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 26, 2025

shaneutt added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 26, 2025

k8s-ci-robot merged commit d28cd59 into kubernetes-sigs:main Aug 26, 2025
19 checks passed

github-project-automation bot moved this from Review to Done in Release v1.4.0 Aug 26, 2025

		// Declare tc here to avoid loop variable
		// reuse issues across parallel tests.

conformance: add test to check for proper cors allow-credentials behvior #3990

conformance: add test to check for proper cors allow-credentials behvior #3990

Conversation

EyalPazz commented Aug 13, 2025

Uh oh!

EyalPazz commented Aug 13, 2025

Uh oh!

EyalPazz commented Aug 13, 2025

Uh oh!

EyalPazz commented Aug 13, 2025

Uh oh!

EyalPazz commented Aug 13, 2025

Uh oh!

k8s-ci-robot commented Aug 13, 2025

Uh oh!

EyalPazz commented Aug 13, 2025

Uh oh!

shaneutt commented Aug 19, 2025

Uh oh!

shaneutt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rikatz commented Aug 25, 2025

Uh oh!

rikatz commented Aug 25, 2025

Uh oh!

shaneutt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 26, 2025

Uh oh!

Uh oh!

Uh oh!

shaneutt left a comment •

edited

Loading