[Pinot connector] Add support for basic authentication by ddcprg · Pull Request #9541 · trinodb/trino

ddcprg · 2021-10-07T06:29:00Z

cla-bot · 2021-10-07T06:29:02Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

ebyhr

Could you try updating TestingPinotCluster class to enable authentication in tests?

ddcprg · 2021-10-07T07:05:06Z

Thanks, I will do

Praveen2112

Just skimmed it

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

Praveen2112 · 2021-10-07T07:09:17Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

I guess we can have a separate config for BasicAuthentation where the userName and password should be non-null and the config should be binded if the authentation method is BASIC.

No at the moment, I've added username and password to PinotConfig. I'll see if I can extract that into a separate config class and bind it

In case of JDBC authentication - we split the user name and password and capture them in a different config object - this also allows to implement custom authentication technique in the future.

@ddcprg Reminder.

thanks, I'll take a look at this one later today

I've now refactored the code to follow a similar pattern to JDBC authentication

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/auth/AuthenticationFactory.java

docs/src/main/sphinx/connector/pinot.rst

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/auth/AuthenticationMethod.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/auth/AuthenticationMethod.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

docs/src/main/sphinx/connector/pinot.rst

cla-bot · 2021-10-07T12:08:59Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

ddcprg · 2021-10-07T17:30:00Z

For the integration test I'll have to wait till #9098 is merged since authentication features were introduced in Pinot 0.7.1 and we are at 0.6.0 in master

ebyhr · 2021-11-05T23:16:03Z

@ddcprg Could you rebase on upstream since #9098 was already merged? Also, please submit CLA if you haven't yet sent it.

cla-bot · 2021-11-08T19:44:28Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

ddcprg · 2021-11-08T19:47:47Z

@ebyhr I'll submit the CLA in the next few days, I still need confirmation from my employer - this shouldn't be a problem. In the meantime I'll add ITs for this functionality

cla-bot · 2021-11-08T21:13:45Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotIntegrationSmokeTest.java

cla-bot · 2021-11-08T21:39:45Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-11-09T08:08:07Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-11-09T08:35:44Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-11-09T08:38:40Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-11-10T08:52:30Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-11-10T08:54:42Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

cla-bot · 2021-11-10T09:16:59Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

ddcprg · 2021-11-12T11:22:46Z

@ebyhr I've submitted my CLA now, apologies for the delays

ddcprg · 2021-12-13T20:34:22Z

@ebyhr I can't find an easy way to do the authentication permutations in a single class because of the way tests are structured and I think changing that could be a big refactoring. I can easily add 2 more test classes to test the missing combinations although running the same of test suit for different authentication methods may not be the best thing to do? Otherwise I can leave one test in the abstract class and move the others to one of the other classes

ddcprg · 2021-12-14T23:01:55Z

docs/src/main/sphinx/connector/pinot.rst

I've now called this basic-inline because there could be more than one way to provide basic auth credentials, e.g. via password file. Let me know if you prefer a different name for this, in my opinion the mapping between the type enum and the property name is a bit awkward and the name not great

...t/java/io/trino/plugin/pinot/auth/basic/inline/TestPinotBrokerBasicAuthenticationConfig.java

...va/io/trino/plugin/pinot/auth/basic/inline/TestPinotControllerBasicAuthenticationConfig.java

.../trino-pinot/src/test/java/io/trino/plugin/pinot/auth/TestPinotAuthenticationTypeConfig.java

ebyhr · 2022-01-01T01:25:10Z

...o-pinot/src/main/java/io/trino/plugin/pinot/auth/basic/PinotBasicAuthenticationProvider.java

Please use library to build credential. For instance, Okhttp has Credentials.basic.

...n/java/io/trino/plugin/pinot/auth/basic/inline/PinotControllerBasicAuthenticationConfig.java

ebyhr · 2022-01-01T01:40:11Z

...o-pinot/src/main/java/io/trino/plugin/pinot/auth/basic/PinotBasicAuthenticationProvider.java

Rename to PinotPasswordAuthenticationProvider.

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestingPinotCluster.java

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java

ebyhr · 2022-01-13T08:44:59Z

@ddcprg By the way, there is #dev channel in Trino community Slack. Please feel free to join the workspace.
https://trino.io/slack.html

ddcprg · 2022-01-17T09:14:48Z

@ebyhr sure. it's probably easier to discuss over slack... I'll send you a message later today in case there are more changes you'd like me to make

ebyhr

We can use airlift's BasicAuthRequestFilter, but follow-up PR is fine.

Please squash commits into two commits. #9541 (comment) meant:

Change table width in docs (= don't add new properties)
Implement basic auth + add new config properties to docs

ddcprg · 2022-01-18T08:33:14Z

We can use airlift's BasicAuthRequestFilter, but follow-up PR is fine.

Please squash commits into two commits. #9541 (comment) meant:

Change table width in docs (= don't add new properties)

Implement basic auth + add new config properties to docs

done

ebyhr · 2022-01-18T08:59:02Z

Thanks. Please fix commit titles as below (no prefix, no commit body)

Make Pinot configuration table wider
Add basic authentication to Pinot connector

This is our commit message guideline.
https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

ddcprg · 2022-01-18T11:37:47Z

Thanks. Please fix commit titles as below (no prefix, no commit body)

Make Pinot configuration table wider

Add basic authentication to Pinot connector

This is our commit message guideline. https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

Thank you, done

ebyhr · 2022-01-19T00:03:22Z

Could you rebase on upstream to resolve conflicts? Sorry, another commit was merged before I merged.

ebyhr · 2022-01-19T14:19:10Z

Merged, thanks!

ebyhr requested review from ebyhr and elonazoulay October 7, 2021 06:30

ebyhr reviewed Oct 7, 2021

View reviewed changes

Praveen2112 reviewed Oct 7, 2021

View reviewed changes

ebyhr reviewed Oct 7, 2021

View reviewed changes

ddcprg force-pushed the issue_9531 branch from 9951ff7 to d735ce0 Compare November 8, 2021 19:44

ddcprg force-pushed the issue_9531 branch from d735ce0 to 20e5faa Compare November 8, 2021 21:13

ddcprg commented Nov 8, 2021

View reviewed changes

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotIntegrationSmokeTest.java Outdated Show resolved Hide resolved

ddcprg force-pushed the issue_9531 branch from 20e5faa to 480e8f6 Compare November 8, 2021 21:39

ddcprg force-pushed the issue_9531 branch from 480e8f6 to a1818a2 Compare November 9, 2021 08:08

ddcprg force-pushed the issue_9531 branch from c8ca04c to 138d819 Compare November 9, 2021 08:38

ddcprg force-pushed the issue_9531 branch from 138d819 to a1f3185 Compare November 10, 2021 08:52

ddcprg force-pushed the issue_9531 branch from a1f3185 to 94af341 Compare November 10, 2021 08:54

ddcprg force-pushed the issue_9531 branch from 94af341 to 37605fa Compare November 10, 2021 09:16

ebyhr self-requested a review December 14, 2021 00:29

ddcprg force-pushed the issue_9531 branch from 73bcfc1 to 4f818ad Compare December 14, 2021 22:56

ddcprg commented Dec 14, 2021

View reviewed changes

ddcprg force-pushed the issue_9531 branch from 4f818ad to d629ddf Compare December 17, 2021 10:11

ebyhr reviewed Jan 1, 2022

View reviewed changes

ddcprg force-pushed the issue_9531 branch from d629ddf to 6a2e4d7 Compare January 7, 2022 20:09

ddcprg force-pushed the issue_9531 branch from 6a2e4d7 to dfdda64 Compare January 17, 2022 09:12

ddcprg force-pushed the issue_9531 branch 3 times, most recently from 3499d91 to afdc21b Compare January 17, 2022 18:33

github-actions bot added the docs label Jan 17, 2022

ebyhr approved these changes Jan 18, 2022

View reviewed changes

ddcprg force-pushed the issue_9531 branch from afdc21b to 98f0be8 Compare January 18, 2022 08:32

ddcprg force-pushed the issue_9531 branch from 98f0be8 to 5faff10 Compare January 18, 2022 11:37

ddcprg force-pushed the issue_9531 branch from 5faff10 to 0f1894d Compare January 18, 2022 19:02

ddcprg added 2 commits January 19, 2022 08:48

Make Pinot configuration table wider

1ea6fa5

Add basic authentication to Pinot connector

2e7b6d2

ddcprg force-pushed the issue_9531 branch from 0f1894d to 2e7b6d2 Compare January 19, 2022 09:04

ebyhr merged commit 830ac66 into trinodb:master Jan 19, 2022

ebyhr mentioned this pull request Jan 19, 2022

Release notes for 369 #10552

Closed

github-actions bot added this to the 369 milestone Jan 19, 2022

mosabua mentioned this pull request Jan 19, 2022

Add Trino 369 release notes #10553

Merged

Conversation

ddcprg commented Oct 7, 2021

Uh oh!

cla-bot bot commented Oct 7, 2021

Uh oh!

ebyhr left a comment

Choose a reason for hiding this comment

Uh oh!

ddcprg commented Oct 7, 2021

Uh oh!

Praveen2112 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Praveen2112 Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Praveen2112 Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

ebyhr Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

ddcprg Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

ddcprg Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cla-bot bot commented Oct 7, 2021

Uh oh!

ddcprg commented Oct 7, 2021

Uh oh!

ebyhr commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-bot bot commented Nov 8, 2021

Uh oh!

ddcprg commented Nov 8, 2021

Uh oh!

cla-bot bot commented Nov 8, 2021

Uh oh!

Uh oh!

cla-bot bot commented Nov 8, 2021

Uh oh!

cla-bot bot commented Nov 9, 2021

Uh oh!

cla-bot bot commented Nov 9, 2021

Uh oh!

cla-bot bot commented Nov 9, 2021

Uh oh!

cla-bot bot commented Nov 10, 2021

Uh oh!

cla-bot bot commented Nov 10, 2021

Uh oh!

cla-bot bot commented Nov 10, 2021

Uh oh!

ddcprg commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddcprg commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddcprg Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Praveen2112 Oct 7, 2021 •

edited

Loading

ddcprg Oct 7, 2021 •

edited

Loading

ebyhr commented Nov 5, 2021 •

edited

Loading

ddcprg commented Nov 12, 2021 •

edited

Loading

ddcprg commented Dec 13, 2021 •

edited

Loading

ddcprg Dec 14, 2021 •

edited

Loading

ebyhr commented Jan 19, 2022 •

edited

Loading