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.

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

Refactor filter configuration to make the different test server filters independent #855

Merged
merged 33 commits into from
Jun 7, 2022

Conversation

dubious90
Copy link
Contributor

@dubious90 dubious90 commented May 19, 2022

This is a followup to #852. In that change, we duplicated ResponseOptions onto separate DynamicDelayConfiguration and TimeTrackingConfiguration protos. However, this resulted in many fields on those protos that were not relevant to each filter. In this PR, we make it so that only the relevant fields exist on each proto.

Resulting changes:

  1. Moved DynamicDelayConfiguration and TimeTrackingConfiguration into their own files, and moved their fields into those protos directly.
  2. In order to make request headers still work, there is now manual logic for taking in a ResponseOptions proto via json and cherrypicking the fields relevant to each filter.
  3. The generic method computeEffectiveConfiguration was moved to each filter's anonymous namespace.
  4. All configs, tests, and markdown files updated to use the new protos.
  5. Some tests have been removed that are no longer necessary, or never provided additional coverage, including some that are less feasible now.
    • HeaderMerge was removed, but everything it tested for was moved into new unit tests of MergeJsonConfig.

dubious90 and others added 22 commits February 5, 2021 12:36
Pull latest into my fork
Signed-off-by: Nathan Perry <[email protected]>
Signed-off-by: Nathan Perry <[email protected]>
dynamic-delay filter

Signed-off-by: Nathan Perry <[email protected]>
Signed-off-by: Nathan Perry <[email protected]>
Nathan Perry added 2 commits May 31, 2022 18:13
@dubious90 dubious90 changed the title Refactor filter configuration Refactor filter configuration to make the different test server filters independent May 31, 2022
@dubious90 dubious90 marked this pull request as ready for review May 31, 2022 23:31
@mum4k mum4k requested a review from qqustc June 1, 2022 00:16
@mum4k mum4k added the waiting-for-review A PR waiting for a review. label Jun 1, 2022
@mum4k
Copy link
Collaborator

mum4k commented Jun 1, 2022

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

Nathan Perry added 2 commits June 1, 2022 11:07
Signed-off-by: Nathan Perry <[email protected]>
Signed-off-by: Nathan Perry <[email protected]>
qqustc
qqustc previously approved these changes Jun 1, 2022
source/server/http_dynamic_delay_filter.cc Show resolved Hide resolved
Copy link
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Can we as part of this PR add a CHANGELOG.md file and document the breaking change? Specifically we should call out (with an example) what broke and how users are expected to modify their configs to make it work again.

Looks like we introduced two breaks, so we should document both:

  1. A break when we moved onto the experimental_response_options fields.
  2. A break where we removed the experimental_response_options fields.

api/server/dynamic_delay.proto Show resolved Hide resolved
api/server/response_options.proto Show resolved Hide resolved
api/server/time_tracking.proto Outdated Show resolved Hide resolved
source/server/http_dynamic_delay_filter.h Outdated Show resolved Hide resolved
source/server/http_test_server_filter.h Outdated Show resolved Hide resolved
@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jun 3, 2022
Signed-off-by: Nathan Perry <[email protected]>
@dubious90 dubious90 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jun 6, 2022
@dubious90
Copy link
Contributor Author

@mum4k Believe I have addressed all your feedback now.

Copy link
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Thank you for adding the documentation and tags. Sending a round of review on the docs.

source/server/http_test_server_filter.h Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jun 6, 2022
Signed-off-by: Nathan Perry <[email protected]>
Copy link
Contributor Author

@dubious90 dubious90 left a comment

Choose a reason for hiding this comment

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

Addressed feedback.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
source/server/http_test_server_filter.h Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dubious90 dubious90 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jun 6, 2022
Copy link
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Thank you.

@mum4k mum4k merged commit 7290824 into envoyproxy:main Jun 7, 2022
@dubious90 dubious90 deleted the envoy-update-051622 branch June 7, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-review A PR waiting for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants