-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
config: subscription factory dependency injection. #7200
config: subscription factory dependency injection. #7200
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>
@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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! just one nit.
"duplicate scoped route configuration foo_scope found"); | ||
} | ||
|
||
// Tests that defining an invalid cluster in the SRDS config results in an error. | ||
TEST_F(ScopedRdsTest, UnknownCluster) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is no longer the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
@lizan looks like |
…/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>
…/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>
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