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.

Sign up for GitHub

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

Jump to bottom

Enable instance and custom credential provider for glue #1363

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Enable instance and custom credential provider for glue #1363

merged 1 commit into from
Sep 11, 2019

Conversation

51yu
Copy link
Contributor

@51yu 51yu commented Aug 26, 2019

Add two configs for glue credential provider

Use instance credential(glue configuration property)
hive.metastore.glue.use-instance-credentials=true

Use custom credential class(hdfs configuration)
presto.glue.credentials-provider

where value is fully qualified class name of
custom aws credential provider implementation

@cla-bot
Copy link

cla-bot bot commented Aug 26, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

@51yu
Copy link
Contributor Author

51yu commented Aug 26, 2019

I just submitted CLA to [email protected]

sopel39
sopel39 reviewed Aug 26, 2019
View reviewed changes
presto-hive/src/main/java/io/prestosql/plugin/hive/HdfsEnvironment.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveHdfsConfiguration.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java Outdated
@@ -131,6 +135,7 @@
private static final String PUBLIC_ROLE_NAME = "public";
private static final String DEFAULT_METASTORE_USER = "presto";
private static final String WILDCARD_EXPRESSION = "";
private static final String GLUE_CREDENTIALS_PROVIDER = "presto.glue.credentials-provider";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GLUE_CREDENTIALS_PROVIDER config property is not set anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's defined in hdfs config xml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd that we put Glue specific config in hdfs-config.xml.

I would rather pass the class name similar as:

    public String getS3EncryptionMaterialsProvider()
    {
        return s3EncryptionMaterialsProvider;
    }

    @Config("hive.s3.encryption-materials-provider")
    @ConfigDescription("Use a custom encryption materials provider for S3 data encryption")
    public HiveS3Config setS3EncryptionMaterialsProvider(String s3EncryptionMaterialsProvider)
    {
        this.s3EncryptionMaterialsProvider = s3EncryptionMaterialsProvider;
        return this;
    }

from HiveS3Config

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java
this(hdfsEnvironment, glueConfig, createAsyncGlueClient(glueConfig));
}

public GlueHiveMetastore(HdfsEnvironment hdfsEnvironment, GlueHiveMetastoreConfig glueConfig, AWSGlueAsync glueClient)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can set credential provider class name directly in GlueHiveMetastoreConfig. No need for passthrough via Configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to follow the convention in document

This class must implement the AWSCredentialsProvider interface and provide a two-argument constructor that takes a java.net.URI and a Hadoop org.apache.hadoop.conf.Configuration as arguments.

https://prestosql.io/docs/current/connector/hive.html#amazon-s3-configuration
^^ see custom s3 credential provider.
shall we use the same pattern for glue ?

@cla-bot
Copy link

cla-bot bot commented Aug 26, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

@cla-bot cla-bot bot added the cla-signed label Aug 27, 2019
@51yu
Copy link
Contributor Author

51yu commented Aug 28, 2019

@Praveen2112 @sopel39 I resolved comments except hdfs configuration one
ready for review.

sopel39
sopel39 reviewed Aug 29, 2019
View reviewed changes
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java Outdated
@@ -131,6 +135,7 @@
private static final String PUBLIC_ROLE_NAME = "public";
private static final String DEFAULT_METASTORE_USER = "presto";
private static final String WILDCARD_EXPRESSION = "";
private static final String GLUE_CREDENTIALS_PROVIDER = "presto.glue.credentials-provider";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd that we put Glue specific config in hdfs-config.xml.

I would rather pass the class name similar as:

    public String getS3EncryptionMaterialsProvider()
    {
        return s3EncryptionMaterialsProvider;
    }

    @Config("hive.s3.encryption-materials-provider")
    @ConfigDescription("Use a custom encryption materials provider for S3 data encryption")
    public HiveS3Config setS3EncryptionMaterialsProvider(String s3EncryptionMaterialsProvider)
    {
        this.s3EncryptionMaterialsProvider = s3EncryptionMaterialsProvider;
        return this;
    }

from HiveS3Config

@51yu
Copy link
Contributor Author

51yu commented Aug 29, 2019

@sopel39 ready for review.

sopel39
sopel39 approved these changes Sep 3, 2019
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm + minor comment

@51yu
Copy link
Contributor Author

51yu commented Sep 6, 2019

@sopel39 ready for review.

@51yu
Enable instance and custom credentials provider for glue
d25fd51 
Add two configs for glue credentials

1. Use instance credential
hive.metastore.glue.use-instance-credentials=true

2. Use custom credentials provider
hive.metastore.glue.aws-credentials-provider
where value is fully qualified class name
@sopel39 sopel39 merged commit e7b0363 into trinodb:master Sep 11, 2019
@sopel39
Copy link
Member

sopel39 commented Sep 11, 2019 •
edited
Loading

For future:

  1. We should think about adding similar property for S3.
  2. We might want to change useInstanceCredentials default to true as for S3
  3. We might want to check if custom AWS credentials provider is Configurable (see io.prestosql.plugin.hive.s3.PrestoS3FileSystem#createEncryptionMaterialsProvider)

@sopel39 sopel39 mentioned this pull request Sep 11, 2019
Closed
6 tasks
@martint martint added this to the 319 milestone Sep 20, 2019
@51yu 51yu deleted the enable-custom-aws-glue-credential-provider branch December 8, 2019 03:28
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Jun 2, 2021
@v-jizhang @51yu @hashhar
Enable instance and custom credential provider for glue
9186909 
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]>
@v-jizhang v-jizhang mentioned this pull request Jun 2, 2021
Closed
@v-jizhang v-jizhang mentioned this pull request Jul 20, 2021
Closed
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Jul 23, 2021
@v-jizhang @51yu
Enable instance and custom credentials provider for glue
3aa0e22 
Cherry-pick of trinodb/trino#1363

Add a config for glue credential hive.metastore.glue.aws-credentials-provider
where value is fully qualified class name.

Co-authored-by: Li Yu <[email protected]>
@findinpath findinpath mentioned this pull request Sep 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praveen2112 Praveen2112 Praveen2112 left review comments

@sopel39 sopel39 sopel39 approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
319
Development

Successfully merging this pull request may close these issues.
4 participants
@51yu @sopel39 @Praveen2112 @martint