kubernetes-sigs
conformance test: CORS
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
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.
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
@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: https://github.com/kubernetes-sigs/gateway-api/pull/3739#discussion_r2090538491.
Just a heads-up: depending on the outcome of #3648, the tests might need to be adapted or extended accordingly.
@zhaohuabing lgtm, however, since I’m not part of the approvers, I’m unable to formally approve it.
Gentle ping @youngnick @sunjayBhatia @mikemorris — whenever you get a chance, would love your thoughts on this.
/lgtm /assign @youngnick
@zhaohuabing your None in the PR description is not recognized by the bot, please replace it with:
```
NONE
```
@zhaohuabing just checking in :wave:
It seems that we're still able to move forward with this one, but there are several lingering comments that need addressing. How are things going? Do you need any support to move this forward?
@zhaohuabing just checking in 👋
It seems that we're still able to move forward with this one, but there are several lingering comments that need addressing. How are things going? Do you need any support to move this forward?
Hi @shaneutt Sorry for the delay - will continue working on this and address the comments.
Do we want to test (or specify!) the behavior of multiple CORs filters ? Like in the route and backendRef etc
I think we should clarify the behavior in the GEP first - should multiple CORS filters be allowed? If yes, what's the expected behavior (or, and)?
Hi @shaneutt I've addressed the lingering comments. There are still a few open issues related to the CORS GEP. I'm happy to wait until we reach consensus on those, or we can go ahead and merge this PR and handle the remaining items in follow-up PRs.
Sounds good, we also have https://github.com/kubernetes-sigs/gateway-api/pull/3895 incoming due to an issue with the current AllowCredentials to keep a lookout for.
We're doing a time extension for v1.4.0, I realize it would be a nice-to-have to get this one in, so I'm flagging it as v1.4.0 in case we can manage it.
/cc
hi @zhaohuabing
echoing what @snorwin requested + the fix added on https://github.com/kubernetes-sigs/gateway-api/pull/4032 can you please rebase your PR over main?
Thank you!
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@rikatz I rebased and might have broken conformance/utils/http/http.go - I will wrap this up this weekend.
Thanks!
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 :vulcan_salute:
/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 :)
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the PR is closed
You can:
- Mark this PR as fresh with
/remove-lifecycle stale - Close this PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
PR needs rebase.
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.