-
Notifications
You must be signed in to change notification settings - Fork 117
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
Support for EventPolicy filters #4086
Support for EventPolicy filters #4086
Conversation
/cc @pierDipi |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4086 +/- ##
==========================================
- Coverage 48.44% 48.43% -0.01%
==========================================
Files 244 244
Lines 14765 14767 +2
==========================================
Hits 7153 7153
- Misses 6900 6901 +1
- Partials 712 713 +1 ☔ View full report in Codecov by Sentry. |
…request when policies claims matched
3a79845
to
e264796
Compare
/test channel-reconciler-tests-sasl-plain |
@@ -160,6 +153,8 @@ public void start(final Promise<Void> startPromise) { | |||
} | |||
} | |||
|
|||
authVerifier.start(vertx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting that this is somehow returning an error / Future to report when discovery fails, how are we handling that?
} | ||
} | ||
|
||
return Future.failedFuture(new AuthorizationException("Not authorized by any EventPolicy")); | ||
if (claimMatchingPolicies.isEmpty()) { | ||
return Future.failedFuture(new AuthorizationException("Not authorized by any EventPolicy")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could have a static final filed with this generic AuthorizationException and re-use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Left comments mostly as food for thoughts / improvements
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: creydr, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
Fixes #4079
Proposed Changes
Hint:
e2e tests are added only for Broker yet, as KafkaChannel and KafkaSink don't support it yet (#4038 #4039)). But will be enabled for those resources in #4068
Release Note