-
Notifications
You must be signed in to change notification settings - Fork 319
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
use wildcard to match alloworigins #2389
Conversation
fbdea90
to
27431e3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2389 +/- ##
==========================================
- Coverage 64.68% 64.65% -0.04%
==========================================
Files 114 114
Lines 16618 16605 -13
==========================================
- Hits 10750 10736 -14
- Misses 5190 5193 +3
+ Partials 678 676 -2 ☔ View full report in Codecov by Sentry. |
5814287
to
9a7d854
Compare
c3648da
to
16a6497
Compare
16a6497
to
f9d238e
Compare
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
f9d238e
to
07a1580
Compare
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.
LGTM thanks to simplifying this API field
adding a release-note tag so we highlight this breaking change in the release notes
Could it be that the validation pattern which has been added to the CRD in this MR prevents the allowance of requests from any origin, e.g. |
hey @jaynis, thanks for root causing the issue :) can you raise a separate GH issue for this, outlining your use case |
Hi @arkodg. I can surely create a separate issue but I was wondering whether it might make sense to revert this PR altogether. I appreciate the effort which @zhaohuabing put into this PR and generally into this project but the implementation before featured regexes and therefore was much more powerful and closer to envoys implementation. This has been changed to a glob notation with a quite strict validation regex which only allows wildcarded subdomains. And internally this is converted back to a regex again. I do not consider this much of an improvement. Also, it is debatable whether it makes sense to align the hostname validation with the CORS validation (which was one of the goals of this PR) because CORS is not a mere hostname but additionally contains the URI scheme and an optional port. |
@jaynis Thanks for the feedback, and sorry for the breaking API changes. We are afraid that if we keep the regex, we will see more users reporting similar issues and we need to continue to debug and explain how to use regex to them. We think it is better to use wildcard match to avoid these issues. Could you please help to provide more details about your use case? Could it be solved by wildcard match? |
Thank you for this clarification @zhaohuabing 👍 . If this was a design decision based on community feedback I am totally fine with this implementation. I personally find regexes more intuitive, but this might be just my personal taste. I will come up with a PR which extends the current wildcard approach with the functionality which I need and then we can discuss there whether it makes sense to merge it or not 😃 . |
thanks @jaynis, I think relaxing CEL validation to support |
The current CORS api uses regex to match the allowed origins. This PR changes this behavior to use wildcard matching instead. Using wildcard matching is more intuitive and easier to use, and also aligns with the gateway api hostname matching.
before:
after:
related:
#2350
#2312