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

router: Allow CORS to be configured using typed_per_filter_config #9324

Closed
wants to merge 14 commits into from
Closed

router: Allow CORS to be configured using typed_per_filter_config #9324

wants to merge 14 commits into from

Conversation

derekargueta
Copy link
Member

@derekargueta derekargueta commented Dec 12, 2019 •
edited
Loading

Description: Enable the CORS filter to be configured using per-route filter configs. This is the first half of deprecating the CORS configuration in the router. While the router's cors field is working, we need to keep the CorsPolicy logic where the router can access it. Once we can delete the cors field, we should consolidate all the CORS code in the CORS extension (left TODOs for myself).
Risk Level: Low - new functionality, no change to existing functionality. The CORS filter will continue to use ProtobufWkt::Empty for it's main filter configuration to avoid breaking wire compatibility (which is being worked on in #8933) while the per-route filter configs have a new protobuf message that excludes deprecated fields.
Testing: included (TODO update CORS integration tests)
Docs Changes: added
Release Notes: added
Fixes #8097
Deprecated: envoy.api.v2.route.VirtualHost.cors, envoy.api.v2.route.RouteAction.cors

Signed-off-by: Derek Argueta [email protected]

@derekargueta
move cors policy to per-route filter config
9a0c494 
Signed-off-by: Derek Argueta <[email protected]>
@derekargueta
cleanup
1eca04c 
Signed-off-by: Derek Argueta <[email protected]>
@derekargueta
add release note
53843ec 
Signed-off-by: Derek Argueta <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9324 was opened by derekargueta.

see: more, trace.

@derekargueta
fix format
f3a2ef8 
Signed-off-by: Derek Argueta <[email protected]>
@mattklein123
Copy link
Member

Makes sense to me at a high level. Will wait for CI to pass before taking a look.

/wait

@derekargueta
fix integration test
65db214 
Signed-off-by: Derek Argueta <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9324 was synchronize by derekargueta.

see: more, trace.

@derekargueta
add documentation
8829cc0 
Signed-off-by: Derek Argueta <[email protected]>
@derekargueta
fix integration test
6f4e369 
Signed-off-by: Derek Argueta <[email protected]>
@derekargueta
Merge branch 'master' into dereka/cors
3f8ade3 
Signed-off-by: Derek Argueta <[email protected]>
@derekargueta
fix proto_format
7759da1 
Signed-off-by: Derek Argueta <[email protected]>
@derekargueta derekargueta marked this pull request as ready for review December 19, 2019 19:02
mattklein123
mattklein123 requested changes Dec 20, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome work. A few small comments.

/wait

api/envoy/api/v2/route/route.proto
//
// This option has been deprecated. Please use `typed_per_filter_config` to configure
// the CORS HTTP filter.
CorsPolicy cors = 8 [deprecated = true];
Copy link
Member

Choose a reason for hiding this comment

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

Needs a note in deprecated.rst, same below

source/common/http/utility.cc Outdated
std::vector<const Router::RouteSpecificFilterConfig*>
Utility::resolveAllPerFilterConfigGeneric(const std::string& filter_name,
const Router::RouteConstSharedPtr& route) {
std::vector<const Router::RouteSpecificFilterConfig*> configs{};
Copy link
Member

Choose a reason for hiding this comment

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

nit: {} not needed

source/common/http/utility.cc Outdated
@@ -657,6 +657,16 @@ Utility::resolveMostSpecificPerFilterConfigGeneric(const std::string& filter_nam
return maybe_filter_config;
}

std::vector<const Router::RouteSpecificFilterConfig*>
Copy link
Member

Choose a reason for hiding this comment

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

perf nit: you might consider either an std::array of size 3 here, given that I think we know there can be a max of 3 entries? Or potentially for easier programming an absl::InlineVector of size 3 default storage?

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh, absl::InlineVector looks pretty cool. Although since we're holding pointers, callers should ideally be verifying non-nullptr anyways (as the cors filter does) so I think std::array would suffice

Copy link
Member Author

Choose a reason for hiding this comment

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

still need to finish addressing this before final review

@stale
Copy link

stale bot commented Dec 30, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 30, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 2, 2020
@stale
Copy link

stale bot commented Jan 9, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 9, 2020
@stale
Copy link

stale bot commented Jan 16, 2020

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Jan 16, 2020
@derekargueta derekargueta mentioned this pull request Feb 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 requested changes

Assignees

@mattklein123 mattklein123

Labels
api stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v3 API: move CorsPolicy to per_filter_configs
2 participants
@derekargueta @mattklein123