Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 28, 2019

Conversation

4 participants
@anoopj
Copy link
Member

commented May 10, 2019

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

}

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

This comment has been minimized.

Copy link
@findepi

findepi May 10, 2019

Member

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

This comment has been minimized.

Copy link
@anoopj

anoopj May 10, 2019

Author Member

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

This comment has been minimized.

Copy link
@electrum

electrum May 10, 2019

Member

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.

This comment has been minimized.

Copy link
@anoopj

anoopj May 19, 2019

Author Member

Thanks, I will get rid of the Optional.

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

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

This comment has been minimized.

Copy link
@findepi

findepi May 10, 2019

Member

Please rename the test method to eg testDefaultCredentials.

This comment has been minimized.

Copy link
@anoopj

anoopj May 10, 2019

Author Member

Will do

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

@prestosql prestosql deleted a comment from martint May 10, 2019

@prestosql prestosql deleted a comment from cla-bot bot May 10, 2019

@prestosql prestosql deleted a comment from cla-bot bot May 10, 2019

@electrum
Copy link
Member

left a comment

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).

@@ -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()) {

This comment has been minimized.

Copy link
@electrum

electrum May 10, 2019

Member

@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) { ... }
});

This comment has been minimized.

Copy link
@anoopj

anoopj May 19, 2019

Author Member

Yep. Thanks.

Show resolved Hide resolved ...o-hive/src/main/java/io/prestosql/plugin/hive/s3/PrestoS3FileSystem.java Outdated
}

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

This comment has been minimized.

Copy link
@electrum

electrum May 10, 2019

Member

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.

@@ -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)

This comment has been minimized.

Copy link
@sopel39

sopel39 May 13, 2019

Member

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

This comment has been minimized.

Copy link
@anoopj

anoopj May 19, 2019

Author Member

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.

This comment has been minimized.

Copy link
@sopel39

sopel39 May 20, 2019

Member

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

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

Updated the PR incorporating review feedback.

@@ -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.

This comment has been minimized.

Copy link
@findepi

findepi May 20, 2019

Member

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.

This comment has been minimized.

Copy link
@anoopj

anoopj Jun 21, 2019

Author Member

Will remove.


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

This comment has been minimized.

Copy link
@findepi

findepi May 20, 2019

Member

You should probably call DefaultAWSCredentialsProviderChain.getInstance()

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

This comment has been minimized.

Copy link
@anoopj

anoopj Jun 21, 2019

Author Member

Will do

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

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

This comment has been minimized.

Copy link
@findepi

findepi May 20, 2019

Member

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

This comment has been minimized.

Copy link
@anoopj

anoopj Jun 21, 2019

Author Member

Sure


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

This comment has been minimized.

Copy link
@findepi

findepi May 20, 2019

Member

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

This comment has been minimized.

Copy link
@anoopj

anoopj Jun 21, 2019

Author Member

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

Get PrestoS3FileSystem, S3Select and Glue Metastore to work with AWS …
…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 anoopj force-pushed the anoopj:master branch from 50e38ce to 2f5590e Jun 21, 2019

@anoopj

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

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

@sopel39

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

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

@findepi
Copy link
Member

left a comment

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

@sopel39 sopel39 merged commit b8e1c47 into prestosql:master Jun 28, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details
@sopel39

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

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

@anoopj anoopj referenced this pull request Jun 29, 2019

Closed

Release notes for 316 #1000

5 of 6 tasks complete
@anoopj

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.