rbac: add delay_deny implementation in RBAC network filter #33875
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
yangminzhu
force-pushed
the
rbac-delay-deny
branch
from
abfa3e4
to
240f07b
Compare
|
cc @kyessenov , @lambdai |
If specified, the RBAC network filter will delay the specified duration before actually closing the connection. This implements envoyproxy#33771. Signed-off-by: Yangmin Zhu <ymzhu@uber.com>
yangminzhu
force-pushed
the
rbac-delay-deny
branch
from
240f07b
to
6f3aabf
Compare
Signed-off-by: Yangmin Zhu <ymzhu@uber.com>
| if (is_delay_denied_) { | ||
| return Network::FilterStatus::StopIteration; | ||
| } |
There was a problem hiding this comment.
should we drain the data to avoid buffering the data when we wait the timer?
There was a problem hiding this comment.
Would it be good enough if we just disable the read after line 123 with callbacks_->connection().readDisable(true);? That way we stop reading any data if we determined the authz result is deny regardless if it is immediate or delay deny? cc @lambdai
|
@yangminzhu could you merge the main and resolve the conflict? Thanks. |
|
assign this to @yanavlasov for final review and because @yanavlasov is the code owner (and senior maintainer) |
| // result is DENY. If this is not present, the connection will be closed immediately. | ||
| // This is useful to provide a better protection for Envoy against clients that retries | ||
| // aggressively when the connection is rejected by the RBAC filter. | ||
| google.protobuf.Duration delay_deny = 8; |
There was a problem hiding this comment.
An optional suggestion for a better name. Feel free to ignore.
| google.protobuf.Duration delay_deny = 8; | |
| google.protobuf.Duration close_delay = 8; |
| )EOF"); | ||
| IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); | ||
| ASSERT_TRUE(tcp_client->write("hello", false, false)); | ||
| tcp_client->waitForDisconnect(); |
There was a problem hiding this comment.
This does not really test that disconnect occurred after configured delay. You can use simulated time for that. Check out MultiplexedIntegrationTestWithSimulatedTime
There was a problem hiding this comment.
@yanavlasov I checked MultiplexedIntegrationTestWithSimulatedTime and found it uses the Event::TestUsingSimulatedTime but not quite understand how it works.
The MultiplexedIntegrationTestWithSimulatedTime uses timeSystem().advanceTimeWait in its test case but the same statement doesn't seem working in the RBAC integration:
I subclassed TestUsingSimulatedTime and tried the same but it just never triggers the timer in the RBAC filter as the tcp_client->waitForDisconnect() is blocked forever, also tried simTime().advanceTimeWait but it is still not working.
would you mind expand a little bit how to use the simulated time in the integration test? thanks.
Commit Message: If specified, the RBAC network filter will delay the specified duration before actually closing the connection.
Additional Description: This implements #33771.
Risk Level: Low
Testing: Added Unit Test and Integration Test
Manual Local Test Log:
Docs Changes: N/A
Release Notes: Updated version history
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]