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

Deprecate Envoy API v2 primitives in configuration for our Envoy filters. #588

Merged
merged 6 commits into from
Dec 8, 2020
Merged

Deprecate Envoy API v2 primitives in configuration for our Envoy filters. #588

merged 6 commits into from
Dec 8, 2020

Conversation

mum4k
Copy link
Collaborator

@mum4k mum4k commented Dec 4, 2020 •
edited
Loading

Adds a new field v3_response_headers alongside the old v2 response_headers in api/server/response_options.proto and marks the old field as deprecated. The old field retains its functionality for backward compatibility.

Since these are repeated fields, we cannot use a oneof. Instead validation is added to ensure the filters report an error on configuration that has both the v2 and the v3 fields set. Envoy::EnvoyException is thrown on validation errors to comply with the Envoy::Server::Configuration::NamedHttpFilterConfigFactory interface.

Also:

  • sorting imports in response_options.proto alphabetically.
  • adding missing anonymous namespace in http_dynamic_delay_filter_integration_test.cc.

Works on #580.

Signed-off-by: Jakub Sobon mumak@google.com

mum4k added 6 commits December 3, 2020 22:45
@mum4k
Adding Envoy v3 version of HeaderValueOption to response_options.proto.
e6da15e 
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k
Utility that upgrades HeaderValueOption from v2 to v3.
a0cf4ec 
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k
Code that upgrades config from Envoy API v2 to v3.
1a7ae77 
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k
Validation for the case when both v2 and v3 primitives are set.
f6f6b7a 
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k
Our Envoy filters now fail on invalid configuration.
d71924c 
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k
Amending after self-review.
8e094a0 
Signed-off-by: Jakub Sobon <mumak@google.com>
@mum4k mum4k requested review from oschaaf and jiajunye December 4, 2020 07:56
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 4, 2020

@jiajunye please review and assign back to me once done.

@mum4k mum4k added the waiting-for-review A PR waiting for a review. label Dec 4, 2020
oschaaf
oschaaf approved these changes Dec 5, 2020
View reviewed changes
Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

Similar thought as in #589 that I'm bring up for consideration, other then that looks good.

api/server/response_options.proto Show resolved Hide resolved
test/server/http_time_tracking_filter_integration_test.cc Show resolved Hide resolved
jiajunye
jiajunye reviewed Dec 7, 2020
View reviewed changes
Copy link
Contributor

@jiajunye jiajunye left a comment

Choose a reason for hiding this comment

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

LGTM

@mum4k mum4k merged commit facd66b into envoyproxy:master Dec 8, 2020
@mum4k mum4k deleted the filter-api-v2-to-v3 branch December 8, 2020 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiajunye jiajunye jiajunye left review comments

@oschaaf oschaaf oschaaf approved these changes

Assignees
No one assigned
Labels
waiting-for-review A PR waiting for a review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mum4k @oschaaf @jiajunye