Skip to content
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

Adding pinot server grpc metadata acl #8030

Merged

Conversation

xiangfu0
Copy link
Contributor

Description

Support presto side to auth pinot server using grpc metadata.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2022

Codecov Report

Merging #8030 (d367ef3) into master (8ca4026) will increase coverage by 0.07%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8030      +/-   ##
============================================
+ Coverage     71.17%   71.25%   +0.07%     
- Complexity     4260     4262       +2     
============================================
  Files          1600     1605       +5     
  Lines         83335    83394      +59     
  Branches      12451    12455       +4     
============================================
+ Hits          59316    59422     +106     
+ Misses        19975    19929      -46     
+ Partials       4044     4043       -1     
Flag Coverage Δ
integration1 29.06% <12.12%> (+0.08%) ⬆️
integration2 27.61% <33.33%> (+0.01%) ⬆️
unittests1 67.87% <56.66%> (+0.02%) ⬆️
unittests2 14.25% <0.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...org/apache/pinot/core/auth/BasicAuthPrincipal.java 72.72% <0.00%> (+2.72%) ⬆️
...che/pinot/server/access/AllowAllAccessFactory.java 80.00% <ø> (ø)
...va/org/apache/pinot/spi/utils/CommonConstants.java 23.63% <ø> (ø)
...che/pinot/core/transport/grpc/GrpcQueryServer.java 42.62% <23.07%> (-6.36%) ⬇️
...che/pinot/server/access/HttpRequesterIdentity.java 50.00% <50.00%> (ø)
...che/pinot/server/access/GrpcRequesterIdentity.java 66.66% <66.66%> (ø)
...he/pinot/server/access/BasicAuthAccessFactory.java 83.33% <83.33%> (ø)
...a/org/apache/pinot/common/metrics/ServerMeter.java 100.00% <100.00%> (ø)
...a/org/apache/pinot/core/transport/QueryServer.java 74.46% <100.00%> (ø)
...ache/pinot/server/access/AccessControlFactory.java 100.00% <100.00%> (ø)
... and 32 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 8ca4026...d367ef3. Read the comment docs.

@richardstartin
Copy link
Member

Is there a way to test this? Given that it's a security feature I would want to know if breaks or is broken.

@xiangfu0
Copy link
Contributor Author

Is there a way to test this? Given that it's a security feature I would want to know if breaks or is broken.

I'm testing this e2e with presto now. Will add unit test and e2e test for this.

@xiangfu0 xiangfu0 force-pushed the adding-pinot-server-grpc-metadata-acl branch 2 times, most recently from 638ce63 to ccc33c0 Compare January 18, 2022 01:23
@xiangfu0 xiangfu0 force-pushed the adding-pinot-server-grpc-metadata-acl branch from ccc33c0 to 2ea0af3 Compare January 19, 2022 06:58
Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@xiangfu0 xiangfu0 force-pushed the adding-pinot-server-grpc-metadata-acl branch 3 times, most recently from ca77989 to a4ffdbc Compare January 21, 2022 00:42
@xiangfu0 xiangfu0 force-pushed the adding-pinot-server-grpc-metadata-acl branch 2 times, most recently from 7549194 to d367ef3 Compare January 21, 2022 01:47
@xiangfu0 xiangfu0 merged commit ca629d6 into apache:master Jan 21, 2022
@xiangfu0 xiangfu0 deleted the adding-pinot-server-grpc-metadata-acl branch January 21, 2022 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants