-
Notifications
You must be signed in to change notification settings - Fork 585
conformance: add test to check for proper cors allow-credentials behvior #3990
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,100 @@ | ||||||||
/* | ||||||||
Copyright 2025 The Kubernetes Authors. | ||||||||
|
||||||||
Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||
you may not use this file except in compliance with the License. | ||||||||
You may obtain a copy of the License at | ||||||||
|
||||||||
http://www.apache.org/licenses/LICENSE-2.0 | ||||||||
|
||||||||
Unless required by applicable law or agreed to in writing, software | ||||||||
distributed under the License is distributed on an "AS IS" BASIS, | ||||||||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||
See the License for the specific language governing permissions and | ||||||||
limitations under the License. | ||||||||
*/ | ||||||||
|
||||||||
package tests | ||||||||
|
||||||||
import ( | ||||||||
"testing" | ||||||||
|
||||||||
"k8s.io/apimachinery/pkg/types" | ||||||||
|
||||||||
"sigs.k8s.io/gateway-api/conformance/utils/http" | ||||||||
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes" | ||||||||
"sigs.k8s.io/gateway-api/conformance/utils/suite" | ||||||||
"sigs.k8s.io/gateway-api/pkg/features" | ||||||||
) | ||||||||
|
||||||||
func init() { | ||||||||
ConformanceTests = append(ConformanceTests, CORSAllowCredentialsBehavior) | ||||||||
} | ||||||||
|
||||||||
var CORSAllowCredentialsBehavior = suite.ConformanceTest{ | ||||||||
ShortName: "CORSAllowCredentialsBehavior", | ||||||||
Description: "Validate ACA-Credentials responses", | ||||||||
Manifests: []string{"tests/cors-allow-credentials-behavior.yaml"}, | ||||||||
Features: []features.FeatureName{ | ||||||||
features.SupportGateway, | ||||||||
features.SupportHTTPRoute, | ||||||||
features.SupportHTTPRouteCORS, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okie, let's wait for Shane to chime in! thanks! |
||||||||
}, | ||||||||
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { | ||||||||
ns := "gateway-conformance-infra" | ||||||||
shaneutt marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
routeNN := types.NamespacedName{Name: "cors-allow-credentials", Namespace: ns} | ||||||||
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} | ||||||||
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN) | ||||||||
kubernetes.HTTPRouteMustHaveResolvedRefsConditionsTrue(t, suite.Client, suite.TimeoutConfig, routeNN, gwNN) | ||||||||
|
||||||||
origin := "https://app.example" | ||||||||
|
||||||||
testCases := []http.ExpectedResponse{ | ||||||||
{ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it's good to use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pascal or Camel case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually several of them are basically just descriptions with spaces and everything. We may even want to rename it |
||||||||
Request: http.Request{ | ||||||||
Method: "GET", | ||||||||
Path: "/cors-behavior-creds-false", | ||||||||
Headers: map[string]string{ | ||||||||
"Origin": origin, | ||||||||
"Cookie": "sid=abc123", | ||||||||
"Authorization": "Bearer test", | ||||||||
}, | ||||||||
}, | ||||||||
Response: http.Response{ | ||||||||
StatusCode: 200, | ||||||||
AbsentHeaders: []string{"Access-Control-Allow-Credentials"}, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you wanna follow the same test as bellow here and check if the Access-Control-Allow-Origin reflects what you added? |
||||||||
}, | ||||||||
Namespace: ns, | ||||||||
}, | ||||||||
{ | ||||||||
Request: http.Request{ | ||||||||
Method: "GET", | ||||||||
Path: "/cors-behavior-creds-true", | ||||||||
Headers: map[string]string{ | ||||||||
"Origin": origin, | ||||||||
"Cookie": "sid=abc123", | ||||||||
"Authorization": "Bearer test", | ||||||||
}, | ||||||||
}, | ||||||||
Response: http.Response{ | ||||||||
StatusCode: 200, | ||||||||
Headers: map[string]string{ | ||||||||
"Access-Control-Allow-Credentials": "true", | ||||||||
"Access-Control-Allow-Origin": origin, | ||||||||
}, | ||||||||
}, | ||||||||
Namespace: ns, | ||||||||
}, | ||||||||
} | ||||||||
|
||||||||
for i := range testCases { | ||||||||
// Declare tc here to avoid loop variable | ||||||||
// reuse issues across parallel tests. | ||||||||
Comment on lines
+91
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a linter checking for this, right? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so? are you talking about copyloopvar maybe? |
||||||||
tc := testCases[i] | ||||||||
t.Run(tc.GetTestCaseName(i), func(t *testing.T) { | ||||||||
t.Parallel() | ||||||||
http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, tc) | ||||||||
}) | ||||||||
} | ||||||||
}, | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
apiVersion: gateway.networking.k8s.io/v1 | ||
kind: HTTPRoute | ||
metadata: | ||
name: cors-allow-credentials | ||
namespace: gateway-conformance-infra | ||
spec: | ||
parentRefs: | ||
- name: same-namespace | ||
rules: | ||
- matches: | ||
- path: | ||
type: PathPrefix | ||
value: /cors-behavior-creds-false | ||
backendRefs: | ||
- name: infra-backend-v1 | ||
port: 8080 | ||
filters: | ||
- cors: | ||
allowCredentials: false | ||
EyalPazz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
type: CORS | ||
- matches: | ||
- path: | ||
type: PathPrefix | ||
value: /cors-behavior-creds-true | ||
backendRefs: | ||
- name: infra-backend-v1 | ||
port: 8080 | ||
filters: | ||
- cors: | ||
allowCredentials: true | ||
type: CORS | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a better description, what is it doing, what is expected, what kind of validation will happen here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much better! thank you!