Skip to content
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

rbac: add delay_deny implementation in RBAC network filter #33875

Merged
merged 13 commits into from
Aug 8, 2024

Conversation

yangminzhu
Copy link
Contributor

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:

[2024-04-30 11:03:14.080][22663692][debug][rbac] [source/extensions/filters/network/rbac/rbac_filter.cc:95] checking connection: requestedServerName: , sourceIP: 127.0.0.1:54006, directRemoteIP: 127.0.0.1:54006,remoteIP: 127.0.0.1:54006, localAddress: 127.0.0.1:10000, ssl: none, dynamicMetadata:
[2024-04-30 11:03:14.080][22663692][debug][rbac] [source/extensions/filters/network/rbac/rbac_filter.cc:201] enforced denied, matched policy none
[2024-04-30 11:03:14.080][22663692][debug][rbac] [source/extensions/filters/network/rbac/rbac_filter.cc:129] connection will be delay denied in 1000ms

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:]

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33875 was opened by yangminzhu.

see: more, trace.

@yangminzhu
Copy link
Contributor Author

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 <[email protected]>
Signed-off-by: Yangmin Zhu <[email protected]>
@yangminzhu
Copy link
Contributor Author

@wbpcode , @lambdai could you take another look if the api looks good? also wondering who should be reviewing the actual code changes? thanks.

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

(PS: sorry for the delay, I was on vacation last days.

@repokitteh-read-only repokitteh-read-only bot removed the api label May 7, 2024
@wbpcode wbpcode assigned yanavlasov and unassigned wbpcode May 7, 2024
@wbpcode
Copy link
Member

wbpcode commented May 7, 2024

@yangminzhu could you merge the main and resolve the conflict? Thanks.

@wbpcode
Copy link
Member

wbpcode commented May 7, 2024

assign this to @yanavlasov for final review and because @yanavlasov is the code owner (and senior maintainer)

Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/wait

Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 13, 2024
@KBaichoo
Copy link
Contributor

/wait

Seems like an RBAC test relevant to this change is timing out in CI @yangminzhu

@yangminzhu
Copy link
Contributor Author

@KBaichoo yeah, but I think per @yanavlasov 's comment (#33875 (comment)), it is working on local workstataion but not in CI.

@yangminzhu
Copy link
Contributor Author

yangminzhu commented Jul 19, 2024

This work for me on my workstation. I've opened #35128 to see what may be different.

@yanavlasov oh I'm a bit surprised it worked on your workstation (the test just always timeout in my local test). Also do you think if we can get this PR in without using the simulated timer (or we can get back to this once we found the root cause in #35128 as this PR has been out for very long time)? the new partialWrite test should still provide pretty good coverage in the test. thanks.

@yanavlasov
Copy link
Contributor

It worked both on workstation and CI. See #35128 Can you copy the test from my PR into yours and see if it will work? I would also remove the test with partialWrite It assumes a lot of things, such that Envoy can absorb such large write quickly. This looks to me like a recipe for a flaky test, especially in sanitized builds. And the test does not seem to validate that disconnect happens after 1 second, only that it happens at some point. Do you know for sure that it happened because of delayed disconnect, or it happened immediately and just it happens that loopback socket had large enough buffer to absorb 500K write? Or disconnected after 5 seconds because of inactivity timer?

/wait-any

@yangminzhu
Copy link
Contributor Author

@yanavlasov ah thanks, updated the test and it works now, also removed the partialWrite in the test since now the simulated testing is working and should be good to cover the use cases now.

@yanavlasov
Copy link
Contributor

@yangminzhu it looks like main merge is needed to resolve the coverage error.

@yanavlasov yanavlasov enabled auto-merge (squash) August 8, 2024 00:50
@yangminzhu
Copy link
Contributor Author

/retest

@yanavlasov yanavlasov merged commit bf65ad3 into envoyproxy:main Aug 8, 2024
49 of 55 checks passed
martinduke pushed a commit to martinduke/envoy that referenced this pull request Aug 8, 2024
…y#33875)

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 <[email protected]>
Signed-off-by: Martin Duke <[email protected]>
yanavlasov pushed a commit that referenced this pull request Aug 15, 2024
#35693)

…" (#35655)

This reverts commit 7ae5ca5.

The flakiness in #35653 is not
caused by this PR. If you look at [the
failure](https://btx.cloud.google.com/invocations/08f19e64-5934-4ed9-9f4c-e0d14e113360/targets/%2F%2Ftest%2Fextensions%2Ffilters%2Fnetwork%2Frbac:integration_test;config=a48d3689347bfb6aaa2bb9b4c87edaab1bb2d2c4710bec9b6690537c48f15140/log)
(you can also find this by going to the issue and clicking the link),
the failing test is
`RoleBasedAccessControlNetworkFilterIntegrationTest.DelayDenied`. That
test was added in #33875 and is
still flaky at head.

Signed-off-by: antoniovleonti <[email protected]>
asingh-g pushed a commit to asingh-g/envoy that referenced this pull request Aug 20, 2024
…y#33875)

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 <[email protected]>
Signed-off-by: asingh-g <[email protected]>
phlax added a commit to phlax/envoy that referenced this pull request Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants