Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cors: add PNA support #21581

Merged
merged 24 commits into from
Aug 1, 2022
Merged

Cors: add PNA support #21581

merged 24 commits into from
Aug 1, 2022

Conversation

daixiang0
Copy link
Member

@daixiang0 daixiang0 commented Jun 6, 2022 •
edited
Loading

Add a PNA header check refer to https://developer.chrome.com/blog/private-network-access-preflight/.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA] #21553
[Optional Deprecated:]
[Optional API Considerations:]

@daixiang0
cors: add PNA support
ed2d689 
Signed-off-by: Loong Dai <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #21581 was opened by daixiang0.

see: more, trace.

@daixiang0
fix field
d96dea3 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
Copy link
Member Author

Since changes are related to v2 API, switch to runtime guard for now.

daixiang0 added 4 commits June 6, 2022 10:42
@daixiang0
use runtime guard rather than api change
0989a05 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
fix CI
0c820af 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
update changelog
08e2e91 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
revert unrelated changes
a881ac0 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
Copy link
Member Author

kindly ping @adisuissa

wbpcode
wbpcode requested changes Jun 8, 2022
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you contributions. Some comments are added. And I think may be we should add a new API in the envoy.config.route.v3.CorsPolicy, right?

source/extensions/filters/http/cors/cors_filter.h Show resolved Hide resolved
source/extensions/filters/http/cors/cors_filter.cc Outdated Show resolved Hide resolved
@wbpcode
Copy link
Member

wbpcode commented Jun 8, 2022

/assign

adisuissa
adisuissa reviewed Jun 8, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I grok the intention behind this feature, but it seems to address what's described in: https://developer.chrome.com/blog/private-network-access-preflight/

Probably no need for guarding it behind a runtime-flag.

source/extensions/filters/http/cors/cors_filter.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/cors/cors_filter.cc Outdated Show resolved Hide resolved
changelogs/current.yaml Outdated Show resolved Hide resolved
@daixiang0
Copy link
Member Author

Also please remove v2-freeze label, now it is unrelated.

@daixiang0
Copy link
Member Author

/retest for CI timeout.

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21581 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
Copy link
Member Author

@adisuissa friendly ping

@wrowe
Copy link
Contributor

wrowe commented Jul 7, 2022

@adisuissa and @wbpcode, ping for when you are able (with all consideration for US holiday and pto, final approval may still be a bit delayed).

wbpcode
wbpcode previously approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit comment. Thanks.

daixiang0 added 2 commits July 22, 2022 21:25
@daixiang0
add tests
1528816 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
feedback
87852fe 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21581 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
Copy link
Member Author

@phlax can we enable debug in verify step? The log is not helpful enough, I am not sure it is a network issue.

@daixiang0
remove useless test
f5a9857 
Signed-off-by: Loong Dai <[email protected]>
@wbpcode
Copy link
Member

wbpcode commented Jul 26, 2022

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21581 (comment) was created by @wbpcode.

see: more, trace.

@daixiang0
Copy link
Member Author

@envoyproxy/api-shepherd for anther look. Thank you!

@wbpcode
Copy link
Member

wbpcode commented Jul 28, 2022 •
edited
Loading

/assign-from @envoyproxy/api-shepherds

adisuissa
adisuissa reviewed Jul 28, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for dropping the ball on this.
/lgtm api
Will defer to @wbpcode for feature usage correctness.

@daixiang0
Copy link
Member Author

@wbpcode @adisuissa please review again, and remove "v2-freeze" label which is misleading.

@wbpcode wbpcode removed the v2-freeze label Jul 29, 2022
@wbpcode
Copy link
Member

wbpcode commented Jul 29, 2022

/wait

daixiang0 added 2 commits July 29, 2022 17:49
@daixiang0
feedback
52b2aff 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
fix CI
996e2a1 
Signed-off-by: Loong Dai <[email protected]>
@daixiang0
Copy link
Member Author

kindly ping @wbpcode

wbpcode
wbpcode approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@wbpcode wbpcode merged commit bf61b07 into envoyproxy:main Aug 1, 2022
@daixiang0 daixiang0 deleted the cors branch August 1, 2022 02:58
@gaby gaby mentioned this pull request Mar 14, 2024
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisuissa adisuissa adisuissa left review comments

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

@wbpcode wbpcode wbpcode approved these changes

@mattklein123 mattklein123 Awaiting requested review from mattklein123

Assignees

@adisuissa adisuissa

@wbpcode wbpcode

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@daixiang0 @wbpcode @wrowe @phlax @adisuissa @mattklein123 @RyanTheOptimist