ratelimit: add support for data-plane-api proto#3675
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
| client_->cancel(); | ||
| } | ||
|
|
||
| TEST(RateLimitGrpcFactoryTest, Create) { |
There was a problem hiding this comment.
I was thinking about explicitly testing the factory to make sure the correct Impl is being created (rather than the implicit testing done above), but I don't have a good sense on how to accomplish that other than type checking. Thoughts?
There was a problem hiding this comment.
I think the implicit testing is probably fine as long as we cover both cases for now.
| // :repo:`api/envoy/service/ratelimit/v2/rls.proto` or the legacy | ||
| // client :repo:`source/common/ratelimit/ratelimit.proto` when | ||
| // making requests to the rate limit service. | ||
| bool use_data_plane_proto = 3; |
There was a problem hiding this comment.
I think we should invert this so that data_plane_api is default? use_legacy_proto? I would also deprecate this right away and explain what the user is supposed to do.
There was a problem hiding this comment.
I was struggling to decide the correct direction here. My main objection to having use_legacy_proto is that current configuration (absence of the field) would turn on using the data-plane-api proto by default; meaning that this commit would be breaking.
There was a problem hiding this comment.
Hmm. OK I guess that's OK as long as we deprecate it right away and are clear what the default will become.
There was a problem hiding this comment.
Exactly. Ok, I will deprecate the field and add more documentation. Also after this lands, I am going to write and email on the mailing lists that has all of the context from here, and from ratelimit as well.
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
nice. thanks for fixing this. a few comments.
|
|
||
| ClientPtr GrpcFactoryImpl::create(const absl::optional<std::chrono::milliseconds>& timeout) { | ||
| return std::make_unique<GrpcClientImpl>(async_client_factory_->create(), timeout); | ||
| // TODO(junr03): remove support for the lyft proto after 1.7.0. |
There was a problem hiding this comment.
nit: add deprecated in comment for easy searching
| }; | ||
|
|
||
| class RateLimitGrpcClientTest : public testing::Test { | ||
| // TODO(junr03): remove the boolean parameter after 1.7.0. |
| client_->cancel(); | ||
| } | ||
|
|
||
| TEST(RateLimitGrpcFactoryTest, Create) { |
There was a problem hiding this comment.
I think the implicit testing is probably fine as long as we cover both cases for now.
| namespace Envoy { | ||
| namespace { | ||
|
|
||
| // TODO(junr03): go back to having only one GrpcClientIntegrationParamTest after 1.7.0. |
There was a problem hiding this comment.
deprecated comment (same elsewhere)
|
@mattklein123 updated with your comments |
| async_client_factory_->create(), timeout, | ||
| "envoy.service.ratelimit.v2.RateLimitService.ShouldRateLimit"); | ||
| } | ||
| ENVOY_LOG_MISC(warn, "legacy rate limit client is deprecated, update your service to support " |
There was a problem hiding this comment.
When does this run? Only on startup? Or on every request?
There was a problem hiding this comment.
good point. I moved to the GrpcFactoryImpl constructor which is called only once in the server code
envoy/source/server/configuration_impl.cc
Line 83 in 08c1399
| async_client_factory_ = async_client_manager.factoryForGrpcService(grpc_service, scope, false); | ||
|
|
||
| // TODO(junr03): legacy rate limit is deprecated. Remove this warning after 1.7.0. | ||
| ENVOY_LOG_MISC(warn, "legacy rate limit client is deprecated, update your service to support " |
There was a problem hiding this comment.
This should only print in the legacy case. You have it printing in all cases?
There was a problem hiding this comment.
@mattklein123 yeah, total brain fart. Will fix when I merge master after the release and fix all the deprecation stuff.
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
@mattklein123 ok fixed my brain fart with the logging, and the release cycle numbers. I think I tagged everything appropriately for the release cycle. The use of the legacy client is deprecated now, and when we tag 1.8.0 we can remove the support. After this merges I will send an email to the mailing lists. |
2 participants
ratelimit: add support for data-plane-api proto
Description:
added support for
api/envoy/service/ratelimit/v2/rls.proto. Envoy can use either proto to send client requests to a ratelimit server with the use of theuse_data_plane_protoboolean flag in the ratelimit configuration. Support for the legacy proto is deprecated and will be removed at the start of the 1.8.0 release cycle.Risk Level: Medium
This PR modifies the Ratelimit client code to internally use the data-plane-api proto. The data-plane-api proto is binary compatible with the proto defined in
source/common/ratelimit/ratelimit.proto(legacy proto). In order to support servers using the legacy proto, Envoy selectively uses the legacy method string or the data-plane-api method string based on the user configurableuse_data_plane_protoflag.Testing:
Update unit and integration tests, parameterized to test both protos. Additionally, I performed a smoke test at Lyft against Lyft's ratelimit service v1.1.1.
Docs Changes:
added documentation.
Release Notes:
added the release notes.
Deprecated:
The use of the legacy proto is deprecated, and using the legacy proto will log a warning log entry.
Fixes #1034