config: subscription factory dependency injection.#7200
Merged
htuch merged 13 commits intoenvoyproxy:masterfrom Jun 11, 2019
Merged
config: subscription factory dependency injection.#7200htuch merged 13 commits intoenvoyproxy:masterfrom
htuch merged 13 commits intoenvoyproxy:masterfrom
Conversation
While working on TDS, it became apparent that the xDS subscription tests are a bit conflated today, since they include both testing of the xDS resource specific subscription and also things like the REST fetcher, etc. This is because the subscription factory is a static, rather than a mockable dependency of subscriptions. This PR transforms subscription factory to an instance object. The PR is quite massive, but it has big pay-off in terms of reducing the boiler plate in xDS tests, separating concerns and making it easier to add new xDS types. Given the recent additions of SRDS, VHDS, TDS and FCDS, this seems worth it. A number of xDS tests have been modified to work with the new pattern. Some tests and properties that are orthogonally tested in subscription_factory_test or the gRPC/REST subscription tests are no longer tested in the resource specified tests. Some general absl::string_view goodness plumbing was also needed due to the new string_view-based interface. Risk level: Low Testing: Modified a number of resource specified xDS unit tests. Relates to envoyproxy#6708. Signed-off-by: Harvey Tuch <htuch@google.com>
Member
Author
|
@fredlas @dmitri-d @AndresGuedez @silentdai this is the cleanup PR that should make adding new xDS a bit easier. |
Signed-off-by: Harvey Tuch <htuch@google.com>
lambdai
reviewed
Jun 7, 2019
lambdai
reviewed
Jun 7, 2019
Contributor
lambdai
left a comment
There was a problem hiding this comment.
Thank you so much for simplifying the code!
I might be the first beneficiary.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
lizan
reviewed
Jun 7, 2019
| } | ||
|
|
||
| // Tests that defining an invalid cluster in the SRDS config results in an error. | ||
| TEST_F(ScopedRdsTest, UnknownCluster) { |
Member
Author
There was a problem hiding this comment.
This validation is done by the subscription factory and tested at https://github.com/envoyproxy/envoy/blob/master/test/common/config/subscription_factory_test.cc#L313. It's not really possible to test this with a mock subscription factory as a result.
fredlas
previously approved these changes
Jun 10, 2019
Contributor
fredlas
left a comment
There was a problem hiding this comment.
All looks good, and in particular the various calls of subscriptionFromConfigSource are much cleaner. Very nice!
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Member
Author
|
@lizan looks like |
lizan
approved these changes
Jun 11, 2019
htuch
added a commit
to htuch/envoy
that referenced
this pull request
Aug 7, 2019
…/dynamic config. As per envoyproxy#6651, this PR plumbs in CLI options to allow independent control over static/dynamic unknown field validation. The defaults are the same for static as today (strict) and for dynamic we are by default permissive. This permits easy rollout of new API minor versions, including those related to security fixes. Fixes a regression that occurred in envoyproxy#7200 where strict/permissive checking CLI options were inverted. Risk level: Low (strictly more permissive by default) Testing: additional unit and integration tests added, exercising both permissive/strict checking over various parts of the API (bootstrap, listeners, clusters, xDS, network filters, etc). Fixes envoyproxy#6651 Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
added a commit
that referenced
this pull request
Aug 19, 2019
…/dynamic config. (#7857) As per #6651, this PR plumbs in CLI options to allow independent control over static/dynamic unknown field validation. The defaults are the same for static as today (strict) and for dynamic we are by default permissive. This permits easy rollout of new API minor versions, including those related to security fixes. Fixes a regression that occurred in #7200 where strict/permissive checking CLI options were inverted. As per #6818, added stats/warning for any unknown fields encountered. Risk level: Low (strictly more permissive by default) Testing: additional unit and integration tests added, exercising both permissive/strict checking over various parts of the API (bootstrap, listeners, clusters, xDS, network filters, etc). Fixes #6651 Fixed #6818 Signed-off-by: Harvey Tuch <htuch@google.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
While working on TDS, it became apparent that the xDS subscription tests are a bit conflated today,
since they include both testing of the xDS resource specific subscription and also things like the
REST fetcher, etc. This is because the subscription factory is a static, rather than a mockable
dependency of subscriptions. This PR transforms subscription factory to an instance object.
The PR is quite massive, but it has big pay-off in terms of reducing the boiler plate in xDS tests,
separating concerns and making it easier to add new xDS types. Given the recent additions of SRDS,
VHDS, TDS and FCDS, this seems worth it.
A number of xDS tests have been modified to work with the new pattern. Some tests and properties
that are orthogonally tested in subscription_factory_test or the gRPC/REST subscription tests are no
longer tested in the resource specified tests.
Some general absl::string_view goodness plumbing was also needed due to the new string_view-based
interface.
Risk level: Medium
Testing: Modified a number of resource specified xDS unit tests.
Relates to #6708.
Signed-off-by: Harvey Tuch htuch@google.com