Add classifiers to flowcontrolv1.CheckResponse #310
Conversation
Codecov ReportBase: 48.79% // Head: 48.81% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
+ Coverage 48.79% 48.81% +0.02%
==========================================
Files 246 246
Lines 19161 19176 +15
==========================================
+ Hits 9349 9361 +12
- Misses 9194 9197 +3
Partials 618 618
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Reviewed 3 of 15 files at r1, 2 of 3 files at r2, 16 of 16 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DariaKunoichi)
|
@kwapik, you would need this change for flow analytics dashboard on Classifiers resource page, please prioritize before Sept 12th |
IridiumOxide
force-pushed
the
GH-268
branch
3 times, most recently
from
0874711 to
2a91ca0
Compare
There was a problem hiding this comment.
Reviewed 22 of 22 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DariaKunoichi)
IridiumOxide
force-pushed
the
GH-268
branch
2 times, most recently
from
5fb2438 to
1df2419
Compare
There was a problem hiding this comment.
Reviewed 1 of 24 files at r6, 2 of 20 files at r7, all commit messages.
Reviewable status: 6 of 29 files reviewed, 1 unresolved discussion (waiting on @DariaKunoichi and @kwapik)
* Add classifiers to flowcontrolv1.CheckResponse * Add classifier index to classifier ID Closes: GH-268 Co-authored-by: Filip Chmielewski <[email protected]>
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Register metric with PCA | ||
| err = engineAPI.RegisterClassifier(classifier) |
There was a problem hiding this comment.
This implementation is incorrect. Classifier runs in the Authz stage. And it may manipulate the labels. By the time engine runs, the labels are already mutated so it might not give the same results. On top of that we are unnecessarily running multi matcher for Classifiers twice.
The matched Classifiers should be encoded in flowcontrol.AuthzResult (I'll be renaming it from AuthzResponse)
cc: @krdln
| activeRulesets map[rulesetID]CompiledRuleset // protected by mu | ||
| nextRulesetID rulesetID // protected by mu | ||
| classifierProto *classificationv1.Classifier | ||
| policyName string |
There was a problem hiding this comment.
Classifier is a singleton, it is not being created per Policy.Classifiers[i]
cc: @krdln
| rs := wrapperMessage.Classifier | ||
| classifier.classifierProto = rs | ||
| classifier.policyName = wrapperMessage.PolicyName | ||
| classifier.policyHash = wrapperMessage.PolicyHash | ||
| classifier.classifierIndex = wrapperMessage.ClassifierIndex |
There was a problem hiding this comment.
This will just overwrite the values in the singleton Classifier!
This change is