Skip to content

Conversation

@apucher
Copy link
Contributor

@apucher apucher commented May 10, 2022

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.token values, if configured.

Potentially backwards-incompatible changes:

AddTableCommand.setAuthToken() removed (same for other commands)
FileUploadDownloadClient removes several methods specific to auth tokens in favor of header-based method signatures

Release Notes:

Add pluggable auth providers to pinot to enable dynamic client token rotation:

  • StaticTokenAuthProvider legacy behavior, job specs
  • UrlTokenAuthProvider dynamic file- or url-based token retrieval
  • AuthProvider interface for generic third-party implementations

Add support for pinot admin command to inject tokens from a URL using a -authTokenUrl param

New configuration options:
...auth.provider.class provider class name for dynamic loading
...auth.token StaticTokenAuthProvider token, legacy behavior
...auth.url UrlTokenAuthProvider source URL
...auth.prefix StaticTokenAuthProvider and UrlTokenAuthProvider token prefix (typically Basic or Bearer)
...auth.header StaticTokenAuthProvider and UrlTokenAuthProvider http header name (typically Authorization)

@apucher apucher added feature release-notes Referenced by PRs that need attention when compiling the next release notes backward-incompat Referenced by PRs that introduce or fix backward compat issues labels May 10, 2022
@apucher apucher requested review from walterddr and xiangfu0 May 10, 2022 08:34
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Merging #8670 (800a11b) into master (b8af790) will decrease coverage by 10.33%.
The diff coverage is 56.37%.

❗ Current head 800a11b differs from pull request most recent head 1b0a2da. Consider uploading reports for the commit 1b0a2da to get more accurate results

@@              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     
Flag Coverage Δ
integration1 ?
integration2 25.26% <55.03%> (+0.12%) ⬆️
unittests1 66.14% <17.69%> (-0.05%) ⬇️
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...r/api/resources/PinotIngestionRestletResource.java 0.00% <0.00%> (-56.42%) ⬇️
...che/pinot/controller/util/FileIngestionHelper.java 0.00% <0.00%> (-91.03%) ⬇️
...ion/tasks/BaseSingleSegmentConversionExecutor.java 0.00% <0.00%> (-74.61%) ⬇️
...ot/plugin/minion/tasks/SegmentConversionUtils.java 38.33% <0.00%> (-38.34%) ⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java 0.00% <0.00%> (-87.10%) ⬇️
...ent/local/data/manager/TableDataManagerConfig.java 0.00% <0.00%> (ø)
...ache/pinot/segment/local/utils/IngestionUtils.java 25.15% <0.00%> (+0.15%) ⬆️
...he/pinot/segment/local/utils/SegmentPushUtils.java 12.70% <0.00%> (-0.22%) ⬇️
.../starter/helix/HelixInstanceDataManagerConfig.java 79.54% <ø> (-0.46%) ⬇️
...ingestion/batch/spec/SegmentGenerationJobSpec.java 53.33% <ø> (ø)
... and 366 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8af790...1b0a2da. Read the comment docs.

@apucher apucher changed the title Support pluggable auth provider Add pluggable client auth provider May 10, 2022
Copy link
Contributor

@walterddr walterddr left a 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!

Comment on lines +123 to +127
Copy link
Contributor

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?

Copy link
Contributor Author

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))

Copy link
Contributor

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

Copy link
Contributor

@walterddr walterddr left a 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?

@apucher apucher force-pushed the pinot-flexible-auth-provider branch 3 times, most recently from 0fdc0b0 to 9689b64 Compare May 13, 2022 21:40
@apucher apucher force-pushed the pinot-flexible-auth-provider branch from e700acc to 1b0a2da Compare May 17, 2022 05:30
@apucher apucher requested a review from Jackie-Jiang May 17, 2022 05:30
@apucher apucher merged commit 2c3813b into master May 17, 2022
@apucher apucher deleted the pinot-flexible-auth-provider branch May 17, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backward-incompat Referenced by PRs that introduce or fix backward compat issues feature release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants