You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by chadnickbok <gi...@git.apache.org> on 2016/07/15 23:41:57 UTC
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
GitHub user chadnickbok opened a pull request:
https://github.com/apache/flink/pull/2260
[FLINK-4222] Allow Kinesis configuration to get credentials from AWS Metadata
When called without credentials, the AmazonKinesisClient tries to configure itself automatically, searching for credentials from environment variables, java system properties, and finally from instance profile credentials delivered through the Amazon EC2 metadata service.
Add the AWSConfigConstant "AUTO", which supports creating an AmazonKinesisClient without any AWSCredentials, which allows for this auto-discovery mechanism to take place and supports getting kinesis credentials from the AWS EC2 metadata service.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chadnickbok/flink aws-metadata-auth-kinesis
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2260.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2260
----
commit 8d43f05ce65b88067fd5a4808c773cafc693c0f2
Author: Nick Chadwick <ch...@gmail.com>
Date: 2016-07-15T23:24:19Z
Support automatic AWS Credentials discovery.
When called without credentials, the AmazonKinesisClient tries to configure itself automatically, searching for credentials from environment variables, java system properties, and finally from instance profile credentials delivered through the Amazon EC2 metadata service.
Add the AWSConfigConstant "AUTO", which supports creating an AmazonKinesisClient without any AWSCredentials, which allows for this auto-discovery mechanism to take place and supports getting kinesis credentials from the AWS EC2 metadata service.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2260
Merging ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2260
@chadnickbok Thank you very much for opening a PR for this, I think this is definitely a good add to the connector's config options.
Have you tested this new credential option in a AWS instance yet? I don't expect any problems, but it would be good to have tested it out also. I'll also give it a test, but perhaps later today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by chadnickbok <gi...@git.apache.org>.
Github user chadnickbok commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062357
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/util/AWSUtil.java ---
@@ -50,8 +50,14 @@ public static AmazonKinesisClient createKinesisClient(Properties configProps) {
awsClientConfig.setUserAgent("Apache Flink " + EnvironmentInformation.getVersion() +
" (" + EnvironmentInformation.getRevisionInformation().commitId + ") Kinesis Connector");
- AmazonKinesisClient client =
- new AmazonKinesisClient(AWSUtil.getCredentialsProvider(configProps).getCredentials(), awsClientConfig);
+ AWSCredentialsProvider credentialsProvider = AWSUtil.getCredentialsProvider(configProps);
+
+ if (credentialsProvider != null) {
+ AmazonKinesisClient client =
+ new AmazonKinesisClient(credentialsProvider.getCredentials(), awsClientConfig);
+ } else {
+ AmazonKinesisClient client = new AmazonKinesisClient(awsClientConfig);
+ }
--- End diff --
Well that's embarassing - I was so sure I compiled this first! That's what I get for pushing a pull request late on a Friday.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2260
Okay. Lets wait for your feedback then (I had some trouble finding time over the weekend, so I haven't tested it myself yet) :) Thank you for testing this!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by chadnickbok <gi...@git.apache.org>.
Github user chadnickbok commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062411
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/config/AWSConfigConstants.java ---
@@ -40,7 +40,10 @@
PROFILE,
/** Simply create AWS credentials by supplying the AWS access key ID and AWS secret key in the configuration properties */
- BASIC
+ BASIC,
+
+ /** Don't supply AWS credentials and rely on the AWS library auto-detecting, which supports ENV vars and AWS MetaData **/
--- End diff --
Makes sense to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/2260
Thanks a lot for the review @tzulitai.
@chadnickbok Thanks a lot for the contribution!
I can also offer to add the missing `break;` when merging the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062324
--- Diff: docs/apis/streaming/connectors/kinesis.md ---
@@ -26,7 +26,7 @@ specific language governing permissions and limitations
under the License.
-->
-The Kinesis connector provides access to [Amazon AWS Kinesis Streams](http://aws.amazon.com/kinesis/streams/).
--- End diff --
Just a small nit note: we should try to keep comment / doc reformatting to a minimal unless necessary, since it'll make the diff quite unreadable. You can also take a look at the code contribution / style guidelines here: https://flink.apache.org/contribute-code.html :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by chadnickbok <gi...@git.apache.org>.
Github user chadnickbok commented on the issue:
https://github.com/apache/flink/pull/2260
We haven't yet tested this - I was getting a head-start on opening the pull request because I didn't realize you'd be so responsive!
Plan is to migrate some code over to the latest master kinesis connector and test this out on Monday.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2260
Hi @chadnickbok,
I've gave the new config a test on AWS EC2 instances with appropriate IAM roles attached, and it works as expected and nicely. I think the changes are good to merge once the remaining comments are addressed :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062232
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/util/AWSUtil.java ---
@@ -50,8 +50,14 @@ public static AmazonKinesisClient createKinesisClient(Properties configProps) {
awsClientConfig.setUserAgent("Apache Flink " + EnvironmentInformation.getVersion() +
" (" + EnvironmentInformation.getRevisionInformation().commitId + ") Kinesis Connector");
- AmazonKinesisClient client =
- new AmazonKinesisClient(AWSUtil.getCredentialsProvider(configProps).getCredentials(), awsClientConfig);
+ AWSCredentialsProvider credentialsProvider = AWSUtil.getCredentialsProvider(configProps);
+
+ if (credentialsProvider != null) {
+ AmazonKinesisClient client =
+ new AmazonKinesisClient(credentialsProvider.getCredentials(), awsClientConfig);
+ } else {
+ AmazonKinesisClient client = new AmazonKinesisClient(awsClientConfig);
+ }
--- End diff --
The `client` variable still needs to be declared outside the if/else clause, because `client` will be used below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71095891
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/util/AWSUtil.java ---
@@ -50,8 +50,14 @@ public static AmazonKinesisClient createKinesisClient(Properties configProps) {
awsClientConfig.setUserAgent("Apache Flink " + EnvironmentInformation.getVersion() +
" (" + EnvironmentInformation.getRevisionInformation().commitId + ") Kinesis Connector");
- AmazonKinesisClient client =
- new AmazonKinesisClient(AWSUtil.getCredentialsProvider(configProps).getCredentials(), awsClientConfig);
+ AmazonKinesisClient client;
+ if (AWSUtil.getCredentialsProvider(configProps) != null) {
+ client = new AmazonKinesisClient(
+ AWSUtil.getCredentialsProvider(configProps).getCredentials(), awsClientConfig);
--- End diff --
Ah, sorry, I made a misjudgement on my last comment here. It'll actually be better to have `AWSUtil.getCredentialsProvider(configProps)` declared as a variable outside and reuse it in the `if` clause, otherwise we'll be calculating the credentials provider twice. Trivial thing, since it's only a one-time thing, but it'll be good to revert it back to what you did in the beginning :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062382
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/config/AWSConfigConstants.java ---
@@ -40,7 +40,10 @@
PROFILE,
/** Simply create AWS credentials by supplying the AWS access key ID and AWS secret key in the configuration properties */
- BASIC
+ BASIC,
+
+ /** Don't supply AWS credentials and rely on the AWS library auto-detecting, which supports ENV vars and AWS MetaData **/
--- End diff --
"which supports ENV vars and AWS MetaData" -> I think the auto-detecting checks more than env vars.
Can we change this to something like, "A credentials provider chain will be used that searches for credentials in this order: ENV_VARS, SYS_PROPS, PROFILE in the AWS instance metadata". According to the AWS API Javadocs, this is the actual behaviour.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by chadnickbok <gi...@git.apache.org>.
Github user chadnickbok commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062384
--- Diff: docs/apis/streaming/connectors/kinesis.md ---
@@ -26,7 +26,7 @@ specific language governing permissions and limitations
under the License.
-->
-The Kinesis connector provides access to [Amazon AWS Kinesis Streams](http://aws.amazon.com/kinesis/streams/).
--- End diff --
My editor did this automatically and I didn't catch it. Again, no more Friday afternoon pushes for me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71062495
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/util/AWSUtil.java ---
@@ -50,8 +50,14 @@ public static AmazonKinesisClient createKinesisClient(Properties configProps) {
awsClientConfig.setUserAgent("Apache Flink " + EnvironmentInformation.getVersion() +
" (" + EnvironmentInformation.getRevisionInformation().commitId + ") Kinesis Connector");
- AmazonKinesisClient client =
- new AmazonKinesisClient(AWSUtil.getCredentialsProvider(configProps).getCredentials(), awsClientConfig);
+ AWSCredentialsProvider credentialsProvider = AWSUtil.getCredentialsProvider(configProps);
+
+ if (credentialsProvider != null) {
+ AmazonKinesisClient client =
+ new AmazonKinesisClient(credentialsProvider.getCredentials(), awsClientConfig);
+ } else {
+ AmazonKinesisClient client = new AmazonKinesisClient(awsClientConfig);
+ }
--- End diff --
No problem at all :) Thank you for the quick fix.
Can you also revert back the declaration of `credentialsProvider`? I don't think it is required to declare this variable outside the scope.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2260
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by chadnickbok <gi...@git.apache.org>.
Github user chadnickbok commented on the issue:
https://github.com/apache/flink/pull/2260
Sorry I dropped off the map on this one - I actually got pulled onto a front-end project and ended up not having any time to follow this pull request :(
Glad to hear this PR is being considered for merging :D
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71557572
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/util/AWSUtil.java ---
@@ -87,6 +93,8 @@ public static AWSCredentialsProvider getCredentialsProvider(final Properties con
? new ProfileCredentialsProvider(profileName)
: new ProfileCredentialsProvider(profileConfigPath, profileName);
break;
+ case AUTO:
+ credentialsProvider = null;
--- End diff --
missing `break;` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2260: [FLINK-4222] Allow Kinesis configuration to get cr...
Posted by chadnickbok <gi...@git.apache.org>.
Github user chadnickbok commented on a diff in the pull request:
https://github.com/apache/flink/pull/2260#discussion_r71068329
--- Diff: flink-streaming-connectors/flink-connector-kinesis/src/main/java/org/apache/flink/streaming/connectors/kinesis/util/AWSUtil.java ---
@@ -50,8 +50,14 @@ public static AmazonKinesisClient createKinesisClient(Properties configProps) {
awsClientConfig.setUserAgent("Apache Flink " + EnvironmentInformation.getVersion() +
" (" + EnvironmentInformation.getRevisionInformation().commitId + ") Kinesis Connector");
- AmazonKinesisClient client =
- new AmazonKinesisClient(AWSUtil.getCredentialsProvider(configProps).getCredentials(), awsClientConfig);
+ AWSCredentialsProvider credentialsProvider = AWSUtil.getCredentialsProvider(configProps);
+
+ if (credentialsProvider != null) {
+ AmazonKinesisClient client =
+ new AmazonKinesisClient(credentialsProvider.getCredentials(), awsClientConfig);
+ } else {
+ AmazonKinesisClient client = new AmazonKinesisClient(awsClientConfig);
+ }
--- End diff --
Okay, figured out how to just compile the kinesis connector and tweaked this code. Let me know if this style of getting the credentialsProvider looks okay.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2260: [FLINK-4222] Allow Kinesis configuration to get credentia...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/2260
@rmetzger Sure, that would be great. Thanks for your help with merging this.
Thanks for your contribution @chadnickbok :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---