You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "JeffBolle (via GitHub)" <gi...@apache.org> on 2023/11/29 14:41:51 UTC
[PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
JeffBolle opened a new pull request, #12068:
URL: https://github.com/apache/pinot/pull/12068
Adds support for StreamNative OAuth2 authentication, allowing Pinot to connect to StreamNative hosted Pulsar cluster.
fixes #12067
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12068:
URL: https://github.com/apache/pinot/pull/12068#issuecomment-1839804174
Thanks for the contribution! @KKcorps Can you help take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1415642044
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarConfig.java:
##########
@@ -52,45 +56,50 @@ public class PulsarConfig {
private final SubscriptionInitialPosition _subscriptionInitialPosition;
private final String _authenticationToken;
private final String _tlsTrustCertsFilePath;
+
+ private final String _issuerUrl;
+ private final String _credentialsFilePath; // Absolute path of your downloaded key file
Review Comment:
Is there a way we can mention encrypted version of this in table config or mention something like S3 path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "JeffBolle (via GitHub)" <gi...@apache.org>.
JeffBolle commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1417534518
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarConfig.java:
##########
@@ -52,45 +56,50 @@ public class PulsarConfig {
private final SubscriptionInitialPosition _subscriptionInitialPosition;
private final String _authenticationToken;
private final String _tlsTrustCertsFilePath;
+
+ private final String _issuerUrl;
+ private final String _credentialsFilePath; // Absolute path of your downloaded key file
Review Comment:
That is pretty far beyond the scope of this PR. That really comes down to how you share secrets into your Pinot deployment. The `_tlsTrustCertsFilePath` has the same issue of how the certs file is supposed to make it into the configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12068:
URL: https://github.com/apache/pinot/pull/12068#issuecomment-1832145059
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12068?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `12 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`3bd74b3`)](https://app.codecov.io/gh/apache/pinot/commit/3bd74b36e4fad0ba35579854339a07d86f5bebd5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.61% compared to head [(`ee084eb`)](https://app.codecov.io/gh/apache/pinot/pull/12068?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.51%.
> Report is 1 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pulsar/PulsarPartitionLevelConnectionHandler.java](https://app.codecov.io/gh/apache/pinot/pull/12068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1zdHJlYW0taW5nZXN0aW9uL3Bpbm90LXB1bHNhci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL3N0cmVhbS9wdWxzYXIvUHVsc2FyUGFydGl0aW9uTGV2ZWxDb25uZWN0aW9uSGFuZGxlci5qYXZh) | 14.28% | [11 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12068?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12068 +/- ##
============================================
- Coverage 61.61% 61.51% -0.11%
- Complexity 1152 1153 +1
============================================
Files 2390 2390
Lines 129823 129837 +14
Branches 20082 20084 +2
============================================
- Hits 79991 79863 -128
- Misses 44006 44153 +147
+ Partials 5826 5821 -5
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.57%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.51% <60.00%> (+0.01%)` | :arrow_up: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.48% <60.00%> (-0.12%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.45% <60.00%> (-0.02%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.51% <60.00%> (-0.11%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.50% <60.00%> (-0.11%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.74% <ø> (-0.15%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12068/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.61% <60.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12068?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #12068:
URL: https://github.com/apache/pinot/pull/12068
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1415640982
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarConfig.java:
##########
@@ -52,45 +56,50 @@ public class PulsarConfig {
private final SubscriptionInitialPosition _subscriptionInitialPosition;
private final String _authenticationToken;
private final String _tlsTrustCertsFilePath;
+
+ private final String _issuerUrl;
+ private final String _credentialsFilePath; // Absolute path of your downloaded key file
Review Comment:
How will someone provide this file in pinot deployments? I think if it's running in docker, expecting folks to copy the credentials file to docker volumn is wrong choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1424097480
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarConfig.java:
##########
@@ -52,45 +56,50 @@ public class PulsarConfig {
private final SubscriptionInitialPosition _subscriptionInitialPosition;
private final String _authenticationToken;
private final String _tlsTrustCertsFilePath;
+
+ private final String _issuerUrl;
+ private final String _credentialsFilePath; // Absolute path of your downloaded key file
Review Comment:
Ok,in that case mention a small TODO on top of it that mentions to find a good way to push this file to all servers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1415642668
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarConfig.java:
##########
@@ -52,45 +56,50 @@ public class PulsarConfig {
private final SubscriptionInitialPosition _subscriptionInitialPosition;
private final String _authenticationToken;
private final String _tlsTrustCertsFilePath;
+
+ private final String _issuerUrl;
Review Comment:
Add comment on what ech of these new configs mean along with sample values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "JeffBolle (via GitHub)" <gi...@apache.org>.
JeffBolle commented on PR #12068:
URL: https://github.com/apache/pinot/pull/12068#issuecomment-1843274948
I've added comments to the config params, which are also documented in https://github.com/pinot-contrib/pinot-docs/pull/263. I'm unclear what additional exceptions I should be concerned about catching, so I added some validation to the config to ensure that the oauth2 params are valid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "JeffBolle (via GitHub)" <gi...@apache.org>.
JeffBolle commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1417526705
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarPartitionLevelConnectionHandler.java:
##########
@@ -60,13 +65,34 @@ public PulsarPartitionLevelConnectionHandler(String clientId, StreamConfig strea
pulsarClientBuilder.authentication(authentication);
}
+ getAuthenticationFactory(_config).ifPresent(pulsarClientBuilder::authentication);
_pulsarClient = pulsarClientBuilder.build();
LOGGER.info("Created pulsar client {}", _pulsarClient);
} catch (Exception e) {
LOGGER.error("Could not create pulsar consumer", e);
}
}
+ private Optional<Authentication> getAuthenticationFactory(PulsarConfig pulsarConfig) {
+ if (StringUtils.isNotBlank(pulsarConfig.getIssuerUrl())
+ && StringUtils.isBlank(pulsarConfig.getAudience())
+ && StringUtils.isBlank(pulsarConfig.getCredentialsFilePath())) {
+ try {
+ return Optional.of(AuthenticationFactoryOAuth2.clientCredentials(
+ new URL(pulsarConfig.getIssuerUrl()),
+ new URL(pulsarConfig.getCredentialsFilePath()),
+ pulsarConfig.getAudience()));
+ } catch (MalformedURLException mue) {
Review Comment:
That is the only declared Exception thrown. What else are you worried about?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [feature] add support for StreamNative OAuth2 authentication for pulsar. [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #12068:
URL: https://github.com/apache/pinot/pull/12068#discussion_r1415638532
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarPartitionLevelConnectionHandler.java:
##########
@@ -60,13 +65,34 @@ public PulsarPartitionLevelConnectionHandler(String clientId, StreamConfig strea
pulsarClientBuilder.authentication(authentication);
}
+ getAuthenticationFactory(_config).ifPresent(pulsarClientBuilder::authentication);
_pulsarClient = pulsarClientBuilder.build();
LOGGER.info("Created pulsar client {}", _pulsarClient);
} catch (Exception e) {
LOGGER.error("Could not create pulsar consumer", e);
}
}
+ private Optional<Authentication> getAuthenticationFactory(PulsarConfig pulsarConfig) {
+ if (StringUtils.isNotBlank(pulsarConfig.getIssuerUrl())
+ && StringUtils.isBlank(pulsarConfig.getAudience())
+ && StringUtils.isBlank(pulsarConfig.getCredentialsFilePath())) {
+ try {
+ return Optional.of(AuthenticationFactoryOAuth2.clientCredentials(
+ new URL(pulsarConfig.getIssuerUrl()),
+ new URL(pulsarConfig.getCredentialsFilePath()),
+ pulsarConfig.getAudience()));
+ } catch (MalformedURLException mue) {
Review Comment:
Do we need to handle any other exceptions in this block?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org