feat: add CORS to SecurityPolicy #2065
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: huabing zhao <[email protected]>
zhaohuabing
force-pushed
the
cors-gateway-api
branch
from
ec1822c to
8497f86
Compare
zhaohuabing
force-pushed
the
cors-gateway-api
branch
from
ec11803 to
0e21c1d
Compare
zhaohuabing
requested review from
a team,
Alice-Lilith,
chauhanshubham and
Xunzhuo
and removed request for
a team
zhaohuabing
force-pushed
the
cors-gateway-api
branch
2 times, most recently
from
ade088a to
84add96
Compare
Signed-off-by: huabing zhao <[email protected]>
zhaohuabing
force-pushed
the
cors-gateway-api
branch
from
84add96 to
fc2b8ca
Compare
Signed-off-by: huabing zhao <[email protected]>
zhaohuabing
force-pushed
the
cors-gateway-api
branch
from
faa63b5 to
244afe8
Compare
Signed-off-by: huabing zhao <[email protected]>
zhaohuabing
force-pushed
the
cors-gateway-api
branch
from
46d164b to
74f5b8d
Compare
Codecov Report
@@ Coverage Diff @@
## main #2065 +/- ##
==========================================
+ Coverage 65.01% 65.13% +0.12%
==========================================
Files 99 99
Lines 14536 14598 +62
==========================================
+ Hits 9450 9509 +59
- Misses 4498 4502 +4
+ Partials 588 587 -1
|
arkodg
reviewed
Oct 25, 2023
arkodg
reviewed
Oct 25, 2023
| // CORS defines the configuration for Cross-Origin Resource Sharing (CORS). | ||
| type CORS struct { | ||
| // AllowOrigins defines the origins that are allowed to make requests. | ||
| AllowOrigins []StringMatch `json:"allowOrigins,omitempty" yaml:"allowOrigins,omitempty"` |
There was a problem hiding this comment.
min length for AllowOrigins should be 1
There was a problem hiding this comment.
I think an empty AllowOrigins could be useful if the backend service allows CORS but we want to explicitly disable it at the Gateway.
I remember we discussed this at the ir PR.
There was a problem hiding this comment.
not setting CORS disables CORS :)
There was a problem hiding this comment.
Yes, it's a bit tricky. Maybe we should add some comments on the API.
arkodg
reviewed
Oct 25, 2023
| // AllowOrigins defines the origins that are allowed to make requests. | ||
| AllowOrigins []StringMatch `json:"allowOrigins,omitempty" yaml:"allowOrigins,omitempty"` | ||
| // AllowMethods defines the methods that are allowed to make requests. | ||
| AllowMethods []string `json:"allowMethods,omitempty" yaml:"allowMethods,omitempty"` |
There was a problem hiding this comment.
what happens when AllowMethods is empty ?
are all requests types allowed or none ?
There was a problem hiding this comment.
None, just like empty AllowOrigins.
There was a problem hiding this comment.
I did some experiments, the CORS filter won't set "Access-Control-Request-Headers" in the preflight response if AllowMethods is empty. So it means none Method Types are allowed.
curl -H "Origin: http://example.com" \
-H "Access-Control-Request-Method: GET" \
-H "Access-Control-Request-Headers: X-Requested-With" \
-X OPTIONS --verbose \
http://localhost:8002/cors/open
* Trying 127.0.0.1:8002...
* Connected to localhost (127.0.0.1) port 8002 (#0)
> OPTIONS /cors/open HTTP/1.1
> Host: localhost:8002
> User-Agent: curl/8.1.2
> Accept: */*
> Origin: http://example.com
> Access-Control-Request-Method: GET
> Access-Control-Request-Headers: X-Requested-With
>
< HTTP/1.1 200 OK
< access-control-allow-origin: http://example.com
< date: Wed, 25 Oct 2023 17:32:39 GMT
< server: envoy
< content-length: 0
<
* Connection #0 to host localhost left intact
There was a problem hiding this comment.
cool, so its opt in, thanks for running the test
arkodg
reviewed
Oct 25, 2023
|
looking good @zhaohuabing ! added some comments |
arkodg
reviewed
Oct 25, 2023
arkodg
reviewed
Oct 25, 2023
Signed-off-by: huabing zhao <[email protected]>
zhaohuabing
force-pushed
the
cors-gateway-api
branch
from
a657bfc to
983c794
Compare
Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: huabing zhao <[email protected]>
Signed-off-by: huabing zhao <[email protected]>
|
Need to retrigger the coverage-test. |
|
/retest |
zirain
reviewed
Oct 25, 2023
| @@ -60,7 +60,7 @@ type ProxyPrometheusProvider struct { | |||
| } | |||
|
|
|||
| // Match defines the stats match configuration. | |||
| type Match struct { | |||
| type Match struct { // TODO: zhaohuabing this type should be renamed to StatsMatch | |||
There was a problem hiding this comment.
can you open an issue to track this?
zirain
approved these changes
Oct 25, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
Relate issues:
Related PRs:
BTW, the string match types are a bit messy. They're defined in multiple APIs but look similar. For most of EG APIs(Ratelimit and CORS for now, more in the future), the match types are just used to represent the StringMatcher XDS API. I think we can have one common string match structure used by all the EG APIs. I would like to raise a PR to refactor it if it makes sense.