You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/06/30 10:32:35 UTC
[GitHub] [zeppelin] zlosim opened a new pull request, #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
zlosim opened a new pull request, #4390:
URL: https://github.com/apache/zeppelin/pull/4390
### What is this PR for?
Currently NotebookRepo S3 is not able to assume role from env variables due to missing aws sts sdk
- added missing dependency
- fixed documentation
### What type of PR is it?
Bug Fix
Improvement
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN-5757
### How should this be tested?
* Strongly recommended: add automated unit tests for any new or changed behavior
* Outline any manual steps to test the PR here.
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? just minor fix, docs updated with this pull request
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
Posted by GitBox <gi...@apache.org>.
Reamer commented on PR #4390:
URL: https://github.com/apache/zeppelin/pull/4390#issuecomment-1173539118
> Not really sure, one thing comes to my mind is to try creating S3 client with WebIdentityTokenCredentialsProvider and check if given provider fails with class not found exception - you think this could be enough?
Yes such a small unit test should be sufficient.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zlosim commented on pull request #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
Posted by GitBox <gi...@apache.org>.
zlosim commented on PR #4390:
URL: https://github.com/apache/zeppelin/pull/4390#issuecomment-1176156704
> Yes such a small unit test should be sufficient.
Thanks @Reamer
I found checking directly for the class from STS lib less error prone, if that is good enough :)
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zlosim commented on a diff in pull request #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
Posted by GitBox <gi...@apache.org>.
zlosim commented on code in PR #4390:
URL: https://github.com/apache/zeppelin/pull/4390#discussion_r911720526
##########
zeppelin-plugins/notebookrepo/s3/pom.xml:
##########
@@ -34,15 +34,28 @@
<description>NotebookRepo implementation based on S3</description>
<properties>
- <aws.sdk.s3.version>1.11.736</aws.sdk.s3.version>
+ <aws.sdk.version>1.11.736</aws.sdk.version>
<plugin.name>NotebookRepo/S3NotebookRepo</plugin.name>
</properties>
<dependencies>
<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
- <version>${aws.sdk.s3.version}</version>
+ <version>${aws.sdk.version}</version>
+ <exclusions>
+ <!-- jcl-over-slf4j is provided by zeppelin-interprerter -->
+ <exclusion>
+ <groupId>commons-logging</groupId>
+ <artifactId>commons-logging</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+
Review Comment:
sure, explanation added
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] zlosim commented on pull request #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
Posted by GitBox <gi...@apache.org>.
zlosim commented on PR #4390:
URL: https://github.com/apache/zeppelin/pull/4390#issuecomment-1172076336
@Reamer thanks for the suggestions, as for the questions:
- Yes, without the lib WebIdentityTokenCredentialsProvider [will fail](https://github.com/aws/aws-sdk-java/blob/9cb8d4b7d7d65e42f8a8a1f1b277ad9d6744ef76/aws-java-sdk-core/src/main/java/com/amazonaws/auth/WebIdentityTokenCredentialsProvider.java#L33) so DefaultAWSCredentialsProviderChain will skip it and move to the next provider
- Not really sure, one thing comes to my mind is to try creating S3 client with WebIdentityTokenCredentialsProvider and check if given provider fails with class not found exception - you think this could be enough?
- license file already contains info for [AWS SDKs ](https://github.com/apache/zeppelin/blob/bd951ee53d696a4d21a69f09f5cd7ba76f173a28/zeppelin-distribution/src/bin_license/LICENSE#L5)
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
Posted by GitBox <gi...@apache.org>.
Reamer commented on code in PR #4390:
URL: https://github.com/apache/zeppelin/pull/4390#discussion_r911031302
##########
zeppelin-plugins/notebookrepo/s3/pom.xml:
##########
@@ -34,15 +34,28 @@
<description>NotebookRepo implementation based on S3</description>
<properties>
- <aws.sdk.s3.version>1.11.736</aws.sdk.s3.version>
+ <aws.sdk.version>1.11.736</aws.sdk.version>
<plugin.name>NotebookRepo/S3NotebookRepo</plugin.name>
</properties>
<dependencies>
<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
- <version>${aws.sdk.s3.version}</version>
+ <version>${aws.sdk.version}</version>
+ <exclusions>
+ <!-- jcl-over-slf4j is provided by zeppelin-interprerter -->
+ <exclusion>
+ <groupId>commons-logging</groupId>
+ <artifactId>commons-logging</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+
Review Comment:
Since you are not using the library in your code, you should leave a comment why it is needed.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer merged pull request #4390: [ZEPPELIN-5757] Assume role in NotebookRepo S3
Posted by GitBox <gi...@apache.org>.
Reamer merged PR #4390:
URL: https://github.com/apache/zeppelin/pull/4390
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org