http: add HCM functionality required for rate limiting#6242
http: add HCM functionality required for rate limiting#6242mattklein123 merged 3 commits intomasterfrom
Conversation
This PR adds new decode/encodeData() callbacks which allow allow filters direct control over sending data to subsequent filters, circumventing any HCM buffering. This is the simplest and lease invasive change I could come up with to support this functionality (or others like it). Fixes #6140 Part of #5942 Signed-off-by: Matt Klein <mklein@lyft.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
More questions than comments on this one - the code is pretty simple and thank you for already having an order of magnitude more comments than code on this one :-)
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::decodeData(Buffer::Instance& data, | ||
| bool end_stream) { | ||
| parent_.decodeData(this, data, end_stream); |
There was a problem hiding this comment.
Will this require some extra work on the StopAllIteration PR? If so, anything we can do here to reduce merge complexity?
There was a problem hiding this comment.
I don't think this should cause any issues/conflicts, other than maybe wanting some more asserts on when this shouldn't be called. @soya3129 WDYT?
There was a problem hiding this comment.
I agree some conditions/checks may be needed for StopAllIteration case. For example, checking if the current filter returns StopAllIteration for headers. Maybe add a more detailed comment about when injectDecodedDataToFilterChain() should be called? For example, injectDecodedDataToFilterChain() can be called only when iteration for data on this filter is not stopped(?).
What will happen if one subsequent filter stops all iteration for headers? Do we need to buffer the injected data somewhere in case buffer is required?
There was a problem hiding this comment.
For example, injectDecodedDataToFilterChain() can be called only when iteration for data on this filter is not stopped(?).
It should still work if headers was stopped and even if the data was previously buffered. Do you still think the comments need updating with the new version?
What will happen if one subsequent filter stops all iteration for headers? Do we need to buffer the injected data somewhere in case buffer is required?
This should still work. If the next filter in the filter chain decides to buffer data it should work fine.
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk @soya3129 PTAL updated per comments. |
* master: (59 commits) http fault: add response rate limit injection (envoyproxy#6267) xds: introduce initial_fetch_timeout option to limit initialization time (envoyproxy#6048) test: fix cpuset-threads tests (envoyproxy#6278) server: add an API for registering for notifications for server instance life… (envoyproxy#6254) remove remains of TestBase (envoyproxy#6286) dubbo_proxy: Implement the routing of Dubbo requests (envoyproxy#5973) Revert "stats: add new BoolIndicator stat type (envoyproxy#5813)" (envoyproxy#6280) runtime: codifying runtime guarded features (envoyproxy#6134) mysql_filter: fix integration test flakes (envoyproxy#6272) tls: update BoringSSL to debed9a4 (3683). (envoyproxy#6273) rewrite buffer implementation to eliminate evbuffer dependency (envoyproxy#5441) Remove the dependency from TimeSystem to libevent by using the Event::Scheduler abstraction as a delegate. (envoyproxy#6240) fuzz: fix use of literal in default initialization. (envoyproxy#6268) http: add HCM functionality required for rate limiting (envoyproxy#6242) Disable mysql_integration_test until it is deflaked. (envoyproxy#6250) test: use ipv6_only IPv6 addresses in custom cluster integration tests. (envoyproxy#6260) tracing: If parent span is propagated with empty string, it causes th… (envoyproxy#6263) upstream: fix oss-fuzz issue envoyproxy#11095. (envoyproxy#6220) Wire up panic mode subset to receive updates (envoyproxy#6221) docs: clarify xds docs with warming information (envoyproxy#6236) ...
3 participants
This PR adds new decode/encodeData() callbacks which allow
allow filters direct control over sending data to subsequent
filters, circumventing any HCM buffering. This is the simplest
and lease invasive change I could come up with to support this
functionality (or others like it).
Fixes #6140
Part of #5942
Risk Level: Low, does not actually modify any existing HCM logic.
Testing: New UTs. I have an integration test that covers this in a follow up change.
Docs Changes: N/A
Release Notes: N/A