Get PrestoS3FileSystem to work with the AWS Default Credentials Provider #741

anoopj · 2019-05-10T17:26:14Z

DefaultAWSCredentialsProviderChain is frequently used by AWS customers
and it provides access from a documented list of sources. This
especially makes it easier to run Presto on non-EC2 hosts where you
don't have the instance profile. (e.g. Macs, during development). This
change also makes PrestoS3FileSystem to be consistent with the Glue connector
in Presto.

See:
https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.html

Fixes #625

findepi · 2019-05-10T19:42:26Z

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java

        }

-        throw new RuntimeException("S3 credentials not configured");
+        return Optional.empty();


Why not com.amazonaws.auth.DefaultAWSCredentialsProviderChain#getInstance?
Then you perhaps wouldn't need the other changes, like making it Optional.

I did this to be consistent with the Glue client, but can make this change.

Agreed. This is what the client does internally:

private AWSCredentialsProvider resolveCredentials() { return (credentials == null) ? DefaultAWSCredentialsProviderChain.getInstance() : credentials; }

Not setting credentials results in it being null.

Thanks, I will get rid of the Optional.

findepi · 2019-05-10T19:44:23Z

presto-hive/src/test/java/io/prestosql/plugin/hive/s3/TestPrestoS3FileSystem.java

@@ -155,7 +156,7 @@ public void testAssumeRoleCredentials()
        }
    }

-    @Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "S3 credentials not configured")
+    @Test
    public void testInstanceCredentialsDisabled()


Please rename the test method to eg testDefaultCredentials.

electrum

This generally looks good. We need to make the same change in PrestoS3ClientFactory so that it works for S3 select (unfortunately it's a different code path).

electrum · 2019-05-10T22:21:00Z

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java

@@ -243,8 +243,11 @@ public void close()
    {
        try (Closer closer = Closer.create()) {
            closer.register(super::close);
-            if (credentialsProvider instanceof Closeable) {
-                closer.register((Closeable) credentialsProvider);
+            if (credentialsProvider.isPresent()) {


@findepi has a good point about not needing Optional. But a general FYI, you can simplify code like this using ifPresent:

credentialsProvider.ifPresent(provider -> { if (provider instanceof Closeable) { ... } });

Yep. Thanks.

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java

electrum · 2019-05-10T22:26:16Z

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java

        }

-        throw new RuntimeException("S3 credentials not configured");
+        return Optional.empty();


Agreed. This is what the client does internally:

private AWSCredentialsProvider resolveCredentials() { return (credentials == null) ? DefaultAWSCredentialsProviderChain.getInstance() : credentials; }

Not setting credentials results in it being null.

sopel39 · 2019-05-13T14:04:40Z

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java

@@ -744,27 +748,27 @@ private AmazonS3 createAmazonS3Client(Configuration hadoopConfig, ClientConfigur
        }
    }

-    private AWSCredentialsProvider createAwsCredentialsProvider(URI uri, Configuration conf)
+    private Optional<AWSCredentialsProvider> createAwsCredentialsProvider(URI uri, Configuration conf)


What about access to Glue Metastore?

I think io.prestosql.plugin.hive.s3.PrestoS3FileSystem#createAwsCredentialsProvider should be extracted, properly parametrized and also used in io.prestosql.plugin.hive.metastore.glue.GlueHiveMetastore#createAsyncGlueClient

I will fix it too. I would rather avoid factoring them in because a customer should be able to use different AWS accounts for each of these.

customer should be able to use different AWS accounts for each of these.

That should be the case anyway. What I meant is that IAM feature toggles should be similar for both S3 and Glue to avoid confusion

anoopj · 2019-05-19T23:04:27Z

Updated the PR incorporating review feedback.

findepi · 2019-05-20T09:04:48Z

presto-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java

@@ -764,7 +765,8 @@ private AWSCredentialsProvider createAwsCredentialsProvider(URI uri, Configurati
            return getCustomAWSCredentialsProvider(uri, conf, providerClass);
        }

-        throw new RuntimeException("S3 credentials not configured");
+        // No credentials configured. Fall back to the default credentials provider.


In https://github.com/prestosql/presto/pull/741/files#diff-2902dc1ae71e318999f5a777a0e0176fR155 you didn't find the comment necessary. I think it's not necessary here as well.

Will remove.

findepi · 2019-05-20T09:05:55Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java

-
-        return asyncGlueClientBuilder.build();
+        else {
+            credentialsProvider = new DefaultAWSCredentialsProviderChain();


You should probably call DefaultAWSCredentialsProviderChain.getInstance()

(then you should rename the method to getAwsCredentialsProvider, the same way it was in s3 code)

findepi · 2019-05-20T09:06:58Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java

@@ -168,19 +169,28 @@ else if (config.getPinGlueClientToCurrentRegion()) {
            }
        }

+        AWSCredentialsProvider credentialsProvider = createAwsCredentialsProvider(config);
+        asyncGlueClientBuilder.setCredentials(credentialsProvider);


please inline credentialsProvider variable and (optionally) add a blank line before final return

findepi · 2019-05-20T09:07:36Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java

+
+    private static AWSCredentialsProvider createAwsCredentialsProvider(GlueHiveMetastoreConfig config)
+    {
+        AWSCredentialsProvider credentialsProvider;


There is no need for this variable. Just return in each if block. Then, don't use else (will become redundant).

In general, I like reducing the independent paths (cyclomatic complexity), but don't mind making the change.

…Default Credentials Provider DefaultAWSCredentialsProviderChain is frequently used by AWS customers and it provides access from a documented list of sources. This especially makes it easier to run Presto on non-EC2 hosts where you don't have the instance profile. (e.g. Macs, during development). This change also makes PrestoS3FileSystem to be consistent with the Glue connector in Presto. See: https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.html

anoopj · 2019-06-21T23:54:59Z

I think I've incorporated all comments. Let me know if there is more feedback.

sopel39 · 2019-06-27T09:22:09Z

Will merge tomorrow if nobody else has any comments, @electrum @findepi ?

findepi

As a follow-up, we could think about deprecating use-instance-credentials.

sopel39 · 2019-06-28T07:21:09Z

@anoopj Could you add a release note to #1000?

anoopj · 2019-06-29T16:05:10Z

@sopel39 Done. Not sure if it's in the right format, please feel free to edit.

Cherry-pick of trinodb/trino#1363, trinodb/trino#741 and trinodb/trino#3689 Add config hive.metastore.glue.aws-credentials-provider for glue credential provider. where value is fully qualified class name of custom aws credential provider implementation. Co-authored-by: Li Yu <[email protected]> Co-authored-by: Anoop Johnson <[email protected]> Co-authored-by: Ashhar Hasan <[email protected]>

Cherry-pick of trinodb/trino#741 DefaultAWSCredentialsProviderChain is frequently used by AWS customers and it provides access from a documented list of sources. This especially makes it easier to run Presto on non-EC2 hosts where you don't have the instance profile. (e.g. Macs, during development). This change also makes PrestoS3FileSystem to be consistent with the Glue connector in Presto. Co-authored-by: Anoop Johnson <[email protected]>

findepi reviewed May 10, 2019

View reviewed changes

cla-bot bot added the cla-signed label May 10, 2019

trinodb deleted a comment from martint May 10, 2019

trinodb deleted a comment from cla-bot bot May 10, 2019

electrum reviewed May 10, 2019

View reviewed changes

sopel39 reviewed May 13, 2019

View reviewed changes

ddrinka mentioned this pull request May 15, 2019

PrestoS3ClientFactory should use the AWS Java SDK default credential provider #625

Closed

findepi reviewed May 20, 2019

View reviewed changes

sopel39 approved these changes Jun 27, 2019

View reviewed changes

findepi approved these changes Jun 27, 2019

View reviewed changes

sopel39 merged commit b8e1c47 into trinodb:master Jun 28, 2019

anoopj mentioned this pull request Jun 29, 2019

Release notes for 316 #1000

Closed

6 tasks

chancez mentioned this pull request Oct 16, 2019

Support IAM Roles for Service Accounts kube-reporting/metering-operator#947

Open

v-jizhang mentioned this pull request Jun 2, 2021

Enable instance and custom credential provider for glue prestodb/presto#16195

Closed

v-jizhang mentioned this pull request Jul 20, 2021

Enable instance and custom credential provider for glue prestodb/presto#16452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get PrestoS3FileSystem to work with the AWS Default Credentials Provider #741

Get PrestoS3FileSystem to work with the AWS Default Credentials Provider #741

anoopj commented May 10, 2019 •

edited by findepi

Loading

findepi May 10, 2019

anoopj May 10, 2019

electrum May 10, 2019

anoopj May 19, 2019

findepi May 10, 2019

anoopj May 10, 2019

electrum left a comment

electrum May 10, 2019

anoopj May 19, 2019

electrum May 10, 2019

sopel39 May 13, 2019

anoopj May 19, 2019

sopel39 May 20, 2019 •

edited

Loading

anoopj commented May 19, 2019

findepi May 20, 2019

anoopj Jun 21, 2019 •

edited

Loading

findepi May 20, 2019

anoopj Jun 21, 2019

findepi May 20, 2019

anoopj Jun 21, 2019

findepi May 20, 2019

anoopj Jun 21, 2019

anoopj commented Jun 21, 2019

sopel39 commented Jun 27, 2019

findepi left a comment

sopel39 commented Jun 28, 2019

anoopj commented Jun 29, 2019

Get PrestoS3FileSystem to work with the AWS Default Credentials Provider #741

Get PrestoS3FileSystem to work with the AWS Default Credentials Provider #741

Conversation

anoopj commented May 10, 2019 • edited by findepi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 May 20, 2019 • edited Loading

Choose a reason for hiding this comment

anoopj commented May 19, 2019

Choose a reason for hiding this comment

anoopj Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoopj commented Jun 21, 2019

sopel39 commented Jun 27, 2019

findepi left a comment

Choose a reason for hiding this comment

sopel39 commented Jun 28, 2019

anoopj commented Jun 29, 2019

anoopj commented May 10, 2019 •

edited by findepi

Loading

sopel39 May 20, 2019 •

edited

Loading

anoopj Jun 21, 2019 •

edited

Loading