rbac: add matched policy name in response code detail for RBAC filter#12912
rbac: add matched policy name in response code detail for RBAC filter#12912alyssawilk merged 18 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
a235346 to
d60599c
Compare
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
|
@dio @alyssawilk would you mind to take a look at this PR? thanks. |
alyssawilk
left a comment
There was a problem hiding this comment.
This looks great! Just a few requests and you should be good to go
test/extensions/filters/http/rbac/rbac_filter_integration_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
Looks good once you do that one rename. @dio can you do a pass as well?
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
dio
left a comment
There was a problem hiding this comment.
Sorry, was drowned by internal stuff. Looks good. A couple of comments.
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
5c58795 to
545f8da
Compare
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
1041db8 to
4c95a84
Compare
|
sorry I force pushed the last 2 commits to fix the DCO issue, @dio Could you take a look again, I reverted the API change, thanks. |
dio
left a comment
There was a problem hiding this comment.
LGTM. Just one small nit. Thanks!
| if (engine_result_ == Allow) { | ||
| return Network::FilterStatus::Continue; | ||
| } else if (engine_result_ == Deny) { | ||
| callbacks_->connection().streamInfo().setResponseCodeDetails( |
There was a problem hiding this comment.
Sorry I missed this earlier (I was focused on the filters/http part) but I don't think setting responseCodeDetails on the network filter makes sense per se. Response code details is an HTTP / L7 construct. Unfortunately I don't think there is a network (L4) equivalent for downstream. It might make more sense to add one than reuse the HTTP field.
There was a problem hiding this comment.
okay I reverted the change in the network filter, I feel we really should have a network equivalent, or maybe have a general RESPONSE_DETAILS so that it can be used in both L4 and L7?
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
|
LGTM! @dio any final thoughts? |
Commit Message: This patch adds the matched policy name in the response code detail for request denied by the RBAC filter.
Risk Level: Low
Testing: Added e2e tests and also tested manually
Docs Changes: Updated
Release Notes: Updated
Fixes #12873.
Signed-off-by: Yangmin Zhu ymzhu@google.com