GCS OAuth token #124

luohao · 2019-01-31T22:39:08Z

This PR adds connector token support in Presto. and GCS module in Hive Connectors.

ea2df20..1b7d68c: allow client to pass connector tokens to connector
fedc801: add dynamic configuration provider
c661834: add HiveGcsModule to support OAuth token authentication with Google Cloud Storage

supersedes #94

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GcsAccessTokenUpdater.java

electrum

I am still reviewing the GCS commit, but adding initial comments now, mostly around naming. The overall design and code looks good.

presto-spi/src/main/java/io/prestosql/spi/security/ConnectorIdentity.java

presto-spi/src/main/java/io/prestosql/spi/security/Identity.java

electrum · 2019-02-03T08:26:57Z

presto-client/src/main/java/io/prestosql/client/PrestoHeaders.java

@@ -50,5 +50,7 @@
    public static final String PRESTO_PAGE_NEXT_TOKEN = "X-Presto-Page-End-Sequence-Id";
    public static final String PRESTO_BUFFER_COMPLETE = "X-Presto-Buffer-Complete";

+    public static final String PRESTO_CONNECTOR_TOKEN = "X-Presto-Connector-Token";


We need to figure out a better name here. Maybe one of

X-Presto-Credential

X-Presto-Extra-Credential

X-Presto-User-Credential

presto-jdbc/src/test/java/io/prestosql/jdbc/TestPrestoDriverUri.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HdfsConfigurationStaticUpdater.java

presto-hive/src/main/java/io/prestosql/plugin/hive/DynamicConfigurationUpdater.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveClientModule.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveHdfsConfiguration.java

luohao · 2019-02-04T08:20:03Z

@electrum Thanks for the review. I updated the commit to address your comments.

I had a really hard time picking names (have used credential, connector-credential and connector-token during the development)... I think ExtraCredential is a better name.

I also updated the naming related to *ConfigurationInitializer and *ConfigurationProvider.

kokosing

Skimmed

presto-spi/src/main/java/io/prestosql/spi/security/ConnectorIdentity.java

presto-spi/src/main/java/io/prestosql/spi/security/Identity.java

kokosing · 2019-02-04T13:02:23Z

presto-client/src/main/java/io/prestosql/client/PrestoHeaders.java

@@ -50,5 +50,7 @@
    public static final String PRESTO_PAGE_NEXT_TOKEN = "X-Presto-Page-End-Sequence-Id";
    public static final String PRESTO_BUFFER_COMPLETE = "X-Presto-Buffer-Complete";

+    public static final String PRESTO_EXTRA_CREDENTIAL = "X-Presto-Extra-Credential";


what is going to happen when client does not support this header and header is not empty?

This is a header sent from clients. If they don't support it then they won't send it :)

I would name it to PRESTO_CLIENT_EXTRA_CREDENTIAL then.

We don't do that for any other headers. For example, X-Presto-User is how clients specify the username.

Please move the constant up to be after PRESTO_RESOURCE_ESTIMATE.

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveClientModule.java

presto-geospatial/src/test/java/io/prestosql/plugin/geospatial/TestSpatialJoins.java

pom.xml

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/HiveGcsConfig.java

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GasConfigurationInitializer.java

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java

presto-cli/src/main/java/io/prestosql/cli/ClientOptions.java

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoConnection.java

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GasConfigurationInitializer.java

presto-hive/src/main/java/io/prestosql/plugin/hive/HdfsConfigurationInitializer.java

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/PrestoGasConfigInitializer.java

electrum · 2019-02-04T18:15:53Z

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GcsAccessTokenProvider.java

+    }
+
+    @Override
+    public void refresh()


This can be

public void refresh() {}

Since the implementation doesn't throw

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GcsAccessTokenProvider.java

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GcsConfigurationProvider.java

kokosing · 2019-02-05T08:20:56Z

presto-client/src/main/java/io/prestosql/client/PrestoHeaders.java

@@ -50,5 +50,7 @@
    public static final String PRESTO_PAGE_NEXT_TOKEN = "X-Presto-Page-End-Sequence-Id";
    public static final String PRESTO_BUFFER_COMPLETE = "X-Presto-Buffer-Complete";

+    public static final String PRESTO_EXTRA_CREDENTIAL = "X-Presto-Extra-Credential";


I would name it to PRESTO_CLIENT_EXTRA_CREDENTIAL then.

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java

presto-main/src/main/java/io/prestosql/server/TaskUpdateRequest.java

kokosing · 2019-02-05T08:24:15Z

presto-cli/src/main/java/io/prestosql/cli/ClientOptions.java

+        for (ClientExtraCredentials credential : credentials) {
+            builder.put(credential.getName(), credential.getValue());
+        }
+        return builder.build();


I would also change other methods here (as separate commit or PR). Up to you.

kokosing · 2019-02-05T08:24:46Z

presto-cli/src/main/java/io/prestosql/cli/ClientOptions.java

+
+        public ClientExtraCredentials(String extraCredentials)
+        {
+            List<String> nameValue = NAME_VALUE_SPLITTER.splitToList(extraCredentials);


requireNonNull(extraCredentials,...);

@electrum is this guaranteed to be non-null? ClientSessionProperty and ClientResourceEstimate doesn't perform this check. I vaguely remember the constructor is called only we pass some non-null string via the options, but don't remember where the related code resides.

Yes, the Airline framework calls the constructor to convert the string argument into an object, so it wouldn't make sense for it to pass a null.

Also, the null check isn't required anyway, since we are immediately using the value and thus the constructor would throw NPE. The normal reason to check for nulls in a constructor is fail immediately (maintain invariants) rather than at some point in the future when you won't know where the null came from.

presto-hive/src/main/java/io/prestosql/plugin/hive/HdfsConfigurationInitializer.java

presto-hive/src/main/java/io/prestosql/plugin/hive/DynamicConfigurationProvider.java

kokosing · 2019-02-05T08:46:23Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HdfsConfigurationInitializer.java

    {
        this(config, ignored -> {});
    }

    @Inject
-    public HdfsConfigurationUpdater(HiveClientConfig config, S3ConfigurationUpdater s3ConfigurationUpdater)
+    public HdfsConfigurationInitializer(HiveClientConfig config, S3ConfigurationUpdater s3ConfigurationUpdater)


Should S3ConfigurationUpdater become a DynamicConfigurationProvider?

I thought about that as well, but it has complications, so let's defer that.

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveHdfsConfiguration.java

electrum

Nit: capitalize "CLI" in commit message

presto-cli/src/test/java/io/prestosql/cli/TestClientOptions.java

presto-cli/src/main/java/io/prestosql/cli/ClientOptions.java

electrum · 2019-02-06T05:34:01Z

presto-cli/src/main/java/io/prestosql/cli/ClientOptions.java

+
+        public ClientExtraCredentials(String extraCredentials)
+        {
+            List<String> nameValue = NAME_VALUE_SPLITTER.splitToList(extraCredentials);


Yes, the Airline framework calls the constructor to convert the string argument into an object, so it wouldn't make sense for it to pass a null.

Also, the null check isn't required anyway, since we are immediately using the value and thus the constructor would throw NPE. The normal reason to check for nulls in a constructor is fail immediately (maintain invariants) rather than at some point in the future when you won't know where the null came from.

electrum

A few minor comments, otherwise this looks ready to merge

presto-main/src/main/java/io/prestosql/SessionRepresentation.java

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java

presto-main/src/test/java/io/prestosql/server/TestHttpRequestSessionContext.java

presto-docs/src/main/sphinx/installation/jdbc.rst

electrum · 2019-02-06T06:19:26Z

presto-hive/src/main/java/io/prestosql/plugin/hive/DynamicConfigurationProvider.java

+
+public interface DynamicConfigurationProvider
+{
+    Configuration updateConfiguration(Configuration configuration, HdfsContext context, URI uri);


Please remove the return type. It isn't used and the semantics of it are not clear (since the method is supposed to update the passed in configuration object).

This was fixed, but the change is in the Rename commit by mistake. I moved it to the commit where I introduce DynamicConfigurationProvider.

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveHdfsConfiguration.java

pom.xml

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GcsConfigurationProvider.java

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GoogleGcsConfigurationInitializer.java

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java

presto-docs/src/main/sphinx/installation/jdbc.rst

presto-jdbc/src/main/java/io/prestosql/jdbc/ConnectionProperties.java

When trinodb/trino#124 is backported in prestodb#13196, the following artifacts are removed: - com.google.cloud.bigdataoss:util - com.google.cloud.bigdataoss:gcsio This is because at that time PrestoDB depends on Airbase 88 with maven-dependency-plugin 2.1. This maven-dependency-plugin is upgraded to 3.1.1 in Airbase 90, and will complain if these two artifacts are not added.

When trinodb/trino#124 is backported in #13196, the following artifacts are removed: - com.google.cloud.bigdataoss:util - com.google.cloud.bigdataoss:gcsio This is because at that time PrestoDB depends on Airbase 88 with maven-dependency-plugin 2.1. This maven-dependency-plugin is upgraded to 3.1.1 in Airbase 90, and will complain if these two artifacts are not added.

When trinodb/trino#124 is backported in prestodb#13196, the following artifacts are removed: - com.google.cloud.bigdataoss:util - com.google.cloud.bigdataoss:gcsio This is because at that time PrestoDB depends on Airbase 88 with maven-dependency-plugin 2.1. This maven-dependency-plugin is upgraded to 3.1.1 in Airbase 90, and will complain if these two artifacts are not added.

cla-bot bot added the cla-signed label Jan 31, 2019

dain reviewed Jan 31, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/gcs/GcsAccessTokenUpdater.java Outdated Show resolved Hide resolved

electrum reviewed Feb 3, 2019

View reviewed changes

kokosing reviewed Feb 4, 2019

View reviewed changes

electrum reviewed Feb 4, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java Show resolved Hide resolved

presto-cli/src/main/java/io/prestosql/cli/ClientOptions.java Outdated Show resolved Hide resolved

electrum reviewed Feb 4, 2019

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoConnection.java Outdated Show resolved Hide resolved

electrum reviewed Feb 4, 2019

View reviewed changes

kokosing reviewed Feb 5, 2019

View reviewed changes

Add credentials in Identity and ConnectorIdentity

b147b3a

electrum reviewed Feb 6, 2019

View reviewed changes

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java Outdated Show resolved Hide resolved

luohao added 2 commits February 6, 2019 09:42

Add header for extra credential

1492ca4

Add support for extra credential in Presto CLI

b3505dc

electrum reviewed Feb 6, 2019

View reviewed changes

presto-docs/src/main/sphinx/installation/jdbc.rst Outdated Show resolved Hide resolved

electrum reviewed Feb 6, 2019

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/ConnectionProperties.java Outdated Show resolved Hide resolved

luohao added 4 commits February 6, 2019 09:50

Add support for extra credential in JDBC driver

c843154

Introduce DynamicConfigurationProvider

886acc3

Rename HdfsConfigurationUpdater to HdfsConfigurationInitializer

7e2fbfe

Add HiveGcsModule

9d92ded

electrum merged commit 50224b3 into trinodb:master Feb 6, 2019

luohao mentioned this pull request Feb 20, 2019

Backport GCS related patches twitter-forks/presto#196

Merged

kewang1024 mentioned this pull request Jul 26, 2019

Add extra credentials prestodb/presto#13134

Merged

shawnzhu mentioned this pull request Jan 20, 2020

Supports the parameter extracredentials trinodb/trino-python-client#6

Closed

findepi mentioned this pull request Oct 24, 2022

Add Iceberg RESTSessionCatalog Implementation #13294

Merged

rice668 pushed a commit to rice668/trino that referenced this pull request Jan 31, 2023

[trinodb#124] IcebergSplitSource should generate splits asynchronously

e9deceb

rice668 pushed a commit to rice668/trino that referenced this pull request Jul 24, 2023

[trinodb#124] IcebergSplitSource should generate splits asynchronously

e744c70

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCS OAuth token #124

GCS OAuth token #124

luohao commented Jan 31, 2019 •

edited

Loading

electrum left a comment

electrum Feb 3, 2019

luohao commented Feb 4, 2019

kokosing left a comment

kokosing Feb 4, 2019

electrum Feb 4, 2019

kokosing Feb 5, 2019

electrum Feb 5, 2019

electrum Feb 4, 2019

kokosing Feb 5, 2019

kokosing Feb 5, 2019

kokosing Feb 5, 2019

luohao Feb 5, 2019

electrum Feb 6, 2019

kokosing Feb 5, 2019

electrum Feb 5, 2019

electrum left a comment

electrum Feb 6, 2019

electrum left a comment

electrum Feb 6, 2019

luohao Feb 6, 2019

GCS OAuth token #124

GCS OAuth token #124

Conversation

luohao commented Jan 31, 2019 • edited Loading

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luohao commented Feb 4, 2019

kokosing 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

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

electrum left a 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

luohao commented Jan 31, 2019 •

edited

Loading