Controller AccessControl interface accepts Request parameter #14414
Conversation
|
@Jackie-Jiang , @mcvsubbu Adding in previous reviewers of the similar draft PR. |
ilamhs
force-pushed
the
access_control_request
branch
from
e956fbb
to
a4544f0
Compare
ilamhs
force-pushed
the
access_control_request
branch
from
a4544f0
to
3839bac
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14414 +/- ##
============================================
+ Coverage 61.75% 63.83% +2.08%
- Complexity 207 1555 +1348
============================================
Files 2436 2660 +224
Lines 133233 146017 +12784
Branches 20636 22359 +1723
============================================
+ Hits 82274 93206 +10932
- Misses 44911 45909 +998
- Partials 6048 6902 +854
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
The failing Integration Tests seem to be unrelated to the code in the PR above. |
Jackie-Jiang
added
feature
release-notes
| * @param accessControl AccessControl object which does the actual validation | ||
| */ | ||
| public static void validatePermission(@Nullable String tableName, AccessType accessType, | ||
| @Nullable HttpHeaders httpHeaders, String endpointUrl, AccessControl accessControl) { | ||
| @Nullable HttpHeaders httpHeaders, Request request, String endpointUrl, AccessControl accessControl) { |
There was a problem hiding this comment.
Can you create a new method with the request param and deprecate the existing one, instead of changing the existing method?
There was a problem hiding this comment.
I see, just for my understanding, is the concern that validatePermission function is used outside this codebase and a deprecation path will be better?
| @@ -101,10 +102,16 @@ public class PinotQueryResource { | |||
| @Inject | |||
| ControllerConf _controllerConf; | |||
|
|
|||
| @Context | |||
There was a problem hiding this comment.
Just for my info, how is the injection handled across different requests on various endpoints?
Labels:
featurerelease-notes(backward compatible, note the deprecation, later on major semver remove the deprecation)Context
This allows for using properties of the
Requestobject in theAccessControlto achieve things like using the peer TLS certificates to assign roles ( tested and running in Production with this PR on our cluster usingRequest)Notes
AccessControlinterface who only define the deprecated methods have the exact same behavior as well (ignoring theRequestessentially)AccessControlacross Pinot source with the new methods only ( the deprecated methods are not used in the Pinot source anymore - comes in very handy at dropping the deprecation)