-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add pluggable client auth provider #8670
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8670 +/- ##
=============================================
- Coverage 69.66% 59.32% -10.34%
+ Complexity 4571 4433 -138
=============================================
Files 1725 1717 -8
Lines 90101 89823 -278
Branches 13412 13371 -41
=============================================
- Hits 62765 53287 -9478
- Misses 22989 32608 +9619
+ Partials 4347 3928 -419
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
walterddr
left a comment
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.
looks good to me. thanks for putting this together @apucher . I left a few comments regarding the backward compatibilities. please kindly take a look and see if those make sense. thank you!
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/spec/SegmentGenerationJobSpec.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/auth/StaticTokenAuthProvider.java
Outdated
Show resolved
Hide resolved
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/AbstractBaseAdminCommand.java
Outdated
Show resolved
Hide resolved
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/auth/AuthProviderUtils.java
Outdated
Show resolved
Hide resolved
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.
checking null values, stream it, filter and reduce to a list on a per request basis?
this is for backward compatibility yes? because previously we don't know what's the content of the headers.
If we only allow the API below (e.g. header extracted from AuthProvider and requires the AuthProvider plugin to return non null header objects), can we avoid this check?
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.
This is mainly defensive programming. The check existed previously in various places as if ()StringUtils.isNotBlank(authToken))
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.
agree. if only there's a way to require AuthProvider to return a non-null, never null-value map we can avoid this, but again this can be an optimization in general
walterddr
left a comment
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.
looking good to me. @xiangfu0 any additional comments?
0fdc0b0 to
9689b64
Compare
e700acc to
1b0a2da
Compare
This PR adds support for pluggable client auth providers, which enables pinot components (controller, server, minion) to use dynamically changing tokens, such as kubernetes service account JWTs. The implementation is generic and enables the addition of third-party auth providers to support virtually any environment.
Previously, pinot components were stuck with statically pre-configured client auth tokens. While authentication for the server-side (e.g. restlets responses) was pluggable already, the client-side (e.g. segment fetcher http requests) was configured statically. This would require a full restart of pinot components to address token changes (e.g. rotation). This PR removes this limitation while preserving legacy behavior for static
auth.tokenvalues, if configured.Potentially backwards-incompatible changes:
AddTableCommand.setAuthToken()removed (same for other commands)FileUploadDownloadClientremoves several methods specific to auth tokens in favor of header-based method signaturesRelease Notes:
Add pluggable auth providers to pinot to enable dynamic client token rotation:
StaticTokenAuthProviderlegacy behavior, job specsUrlTokenAuthProviderdynamic file- or url-based token retrievalAuthProviderinterface for generic third-party implementationsAdd support for pinot admin command to inject tokens from a URL using a
-authTokenUrlparamNew configuration options:
...auth.provider.classprovider class name for dynamic loading...auth.tokenStaticTokenAuthProvider token, legacy behavior...auth.urlUrlTokenAuthProvider source URL...auth.prefixStaticTokenAuthProvider and UrlTokenAuthProvider token prefix (typicallyBasicorBearer)...auth.headerStaticTokenAuthProvider and UrlTokenAuthProvider http header name (typicallyAuthorization)