Use local private credentials (json key file) to refresh GCS access token#14585
Use local private credentials (json key file) to refresh GCS access token#14585zhenxiao merged 6 commits intoprestodb:masterfrom
Conversation
There was a problem hiding this comment.
Do we need to hardcode the URL here? Maybe we can move it to a static variable.
There was a problem hiding this comment.
Good catch, will extract it as a constant val
There was a problem hiding this comment.
The GCP documentation here (https://developers.google.com/identity/protocols/oauth2) recommends creating a signed JWT with the client ID and a private key from the GCP credential. Is this step automatically covered in the Google OAuth libs?
There was a problem hiding this comment.
Good call, I guess not. I will post another commit or PR to support JWT with a given client ID
There was a problem hiding this comment.
Is the synchronized keyword necessary here? Will this function be invoked in a multi-threaded situation?
There was a problem hiding this comment.
Good call. This interceptor is used by presto-cli and presto-jdbc. I think presto-jdbc might have some queries running in parallel. Even for a single query, during the http conversation, there are still multiple http calls, some of which might be triggered in an asynchronous way.
There was a problem hiding this comment.
Gotcha. Thanks for your explanation.
There was a problem hiding this comment.
add one blank line between private static final and private final
There was a problem hiding this comment.
shall we keep hive here? kind of misleading. If the session property is already defined, could we import the string from other class?
There was a problem hiding this comment.
The "hive.gcs.credentials.path" is the key I newly added. The other 'hive.gcs.oauth' is an existing one, which can be also found in presto-hive.
presto-hive/src/main/java/com/facebook/presto/hive/gcs/GcsConfigurationProvider.java
28: private static final String GCS_OAUTH_KEY = "hive.gcs.oauth";
If we imported the constant "hive.gcs.oauth" from presto-hive, we might introduce a new dependency from presto-client to presto-hive. Shall we do that?
I'm just thinking the name of these two keys need to be consistent.
There was a problem hiding this comment.
get it. no need to have dependency from presto-hive
keep hive.gcs.credentials.path is fine
There was a problem hiding this comment.
make constant:
https://www.googleapis.com/auth/devstorage.read_only
pom.xml
Outdated
There was a problem hiding this comment.
oh, this PR is a dependency of:
prestodb/presto-hadoop-apache#43
There was a problem hiding this comment.
We have updated the version to 2.7.4-8 in another pr
There was a problem hiding this comment.
static import GCSOAuthInterceptor.GCS_CREDENTIALS_PATH_KEY
beinan
force-pushed
the
gcs_private_key_auth
branch
from
4e3b2d6 to
11bc554
Compare
beinan
force-pushed
the
gcs_private_key_auth
branch
2 times, most recently
from
47236c3 to
56c88e1
Compare
beinan
force-pushed
the
gcs_private_key_auth
branch
from
56c88e1 to
e852a2b
Compare
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
shall we keep the blank line?
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
keep the blank line for format
beinan
force-pushed
the
gcs_private_key_auth
branch
3 times, most recently
from
b17a3e0 to
09af78c
Compare
beinan
force-pushed
the
gcs_private_key_auth
branch
from
09af78c to
5bb0689
Compare
prestodb#14585 introduced new dependencies that entered presto-jdbc uber jar. presto-jdbc shades out all dependencies other than the core jdbc classes itself and this PR does the same.
#14585 introduced new dependencies that entered presto-jdbc uber jar. presto-jdbc shades out all dependencies other than the core jdbc classes itself and this PR does the same.
prestodb#14585 introduced new dependencies that entered presto-jdbc uber jar. presto-jdbc shades out all dependencies other than the core jdbc classes itself and this PR does the same.
3 participants
Generating and refresh GCS access token by the local private credentials (json key file)
Support the credentials of either "service_account" or "authorized_user" type
Added to both presto-cli and presto-jdbc