Add Rbac Access Denied Policy Info in Access logs #3100
Conversation
gargnupur
requested review from
yangminzhu,
kyessenov,
bianpengyuan and
a team
google-cla
bot
added
the
cla: yes
istio-testing
added
the
size/L
Signed-off-by: gargnupur <[email protected]>
| // It is of the format: | ||
| // "rbac_access_denied_matched_policy[ns[NAMESPACE]-policy[POLICY]-rule[POLICY_INDEX]]" | ||
| const RE2 rbac_denied_match( | ||
| "rbac_access_denied_matched_policy\\[ns\\[(.*)\\]-policy\\[(.*)\\]-rule\\[(" |
There was a problem hiding this comment.
The suffix is istio specific. Should we just look for prefix to support non-Istio RBAC?
There was a problem hiding this comment.
/cc @yangminzhu
We wanted to extract policy name and rule, hence the suffix...
what is the exact scenario you thinking Kuat?
There was a problem hiding this comment.
Custom RBAC rules with EnvoyFilter. I guess we could have a fallback in case regex does not match, just log the whole thing.
There was a problem hiding this comment.
I'm fine either way, I think we normally don't do anything special to handle customized config from EnvoyFilter but I think it also doesn't harm anything with a fallback here.
There was a problem hiding this comment.
yeah we are actually logging the whole thing if regex doesn't match.
extensions/stackdriver/log/logger.cc
Outdated
| !request_info.connection_termination_details.empty()) { | ||
| const std::string response_details = | ||
| !request_info.response_code_details.empty() | ||
| ? request_info.response_code_details |
There was a problem hiding this comment.
I think you could re-use the same field for both.
| @@ -6,7 +6,7 @@ | |||
| rules: | |||
| action: DENY | |||
| policies: | |||
| "test": | |||
| ns[foo]-policy[httpbin-deny]-rule[0]: | |||
There was a problem hiding this comment.
Do we need an example for network RBAC?
There was a problem hiding this comment.
Should be similar to this one with response to extracting stuff? I can add if needed..
There was a problem hiding this comment.
Could be done in a follow-up. Just want to make sure we have coverage for network filter as well.
There was a problem hiding this comment.
Sure, will add one
Signed-off-by: gargnupur <[email protected]>
Signed-off-by: gargnupur <[email protected]>
| @@ -428,6 +428,11 @@ void populateExtendedHTTPRequestInfo(RequestInfo* request_info) { | |||
| getValue({"request", "url_path"}, &request_info->url_path); | |||
| getValue({"request", "host"}, &request_info->url_host); | |||
| getValue({"request", "scheme"}, &request_info->url_scheme); | |||
| std::string response_details; | |||
| getValue({"response", "code_details"}, &response_details); | |||
There was a problem hiding this comment.
@kyessenov : It's not possible for both connection_termination_details and response_code_details to have values right as one is filled for TCP and other for HTTP
There was a problem hiding this comment.
nit: no need to stack allocate a string I think.
There was a problem hiding this comment.
will fix in the follow-up pr
What this PR does / why we need it:
Add Rbac Access Denied Policy Info in Access logs
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: