[WIP, POC] Add PolicyEnforcementMode to RBAC policy definition#3548
[WIP, POC] Add PolicyEnforcementMode to RBAC policy definition#3548quanjielin wants to merge 2 commits intoenvoyproxy:masterfrom
Conversation
The field is optional by default
4f34517 to
7995570
Compare
|
Nice, I like this idea. @rodaine can you take a first pass? |
d5e7ab4 to
3404a67
Compare
There was a problem hiding this comment.
coding wise, it is more readable to return a struct with two boolean.
There was a problem hiding this comment.
Or return an enum.
Alternatively, keep this function the same, but handle the result in filter only.
There was a problem hiding this comment.
Or return an enum.
Alternatively, keep this function the same, but handle the result in filter only.
There was a problem hiding this comment.
I'm not follow this logic. Can we keep this function as is, and in the filter, simply check if poicy.isDarklaunch() (or engine.isDarklaunch()) to decide if it should reject request or simply log result?
rodaine
left a comment
There was a problem hiding this comment.
Oh this is great! Some comments around intent in the PolicyMatcher, particularly around changing the current behavior which short-circuits on the first match.
Based off this, I'd recommend we treat policies different than just matchers and elevate them. FWICT, we want to execute all dark policies for evaluation, and emit tagged stats for any matched policy (be it dark or ... erm light?)
Also, darklaunch seems a bit strange to me naming wise. Is there an existing convention in Envoy's nomenclature (like shadow?) we can use instead?
| enum PolicyEnforcementMode { | ||
| POLICY_ENFORCEMENT_ENFORCED = 0; | ||
| POLICY_ENFORCEMENT_DARKLAUNCH = 1; | ||
| } |
There was a problem hiding this comment.
Can we namespace this under the Policy message? Then we don't need to prefix these.
| for (const auto& policy : policies_) { | ||
| if (policy.matches(connection, headers)) { | ||
| matched = true; | ||
| break; |
There was a problem hiding this comment.
This (erm) breaks the existing short circuiting behavior of the engine. I don't think we want to always evaluate every policy.
There was a problem hiding this comment.
separated rules(enforce/darklaunch) approach in 2nd POC solves this issue
| matched = true; | ||
| } | ||
|
|
||
| darklaunch_matched_ = true; |
There was a problem hiding this comment.
What's the intent here? We want to tell the filter that we matched a particular dark policy (or policies) correct? If so, this currently will always be true if we match any filter, and we end up hiding which policy (or policies) actually matched.
| if (darklaunch_allowed) { | ||
| config_->stats().darklaunch_allowed_.inc(); | ||
| } else { | ||
| config_->stats().darklaunch_denied_.inc(); |
There was a problem hiding this comment.
For both the existing stats as well as these dark ones, I feel like we should apply tags with the matching policy name so we can identify the source of the allow/deny.
There was a problem hiding this comment.
agree, will put this in separated PR about what attributes need to be collected and how to pass data to metrics storage(pass data cross-filters, etc)
|
Close this PR since it's replaced by #3554 |
#3552
Updated POC PR: #3554
Use case:
rbac policy dark launch - test new rbac policies work as expected before it's rolled out to production, policy in dark_launch mode won't effect real users, rbac filter just log the results by applying policies and sending metrics to some metrics store/then build dashboard.