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

Allow injecting authentication components for ThriftMetastore #12937

Merged
merged 4 commits into from
Sep 5, 2022

Conversation

phd3
Copy link
Member

@phd3 phd3 commented Jun 22, 2022

Description

We've a requirement for using an external service to authenticate to HMS as Trino, and also fetch HMS delegation tokens for the end-user. So proposing this change to make these components to be provided by an external module. ( Using optional binder to avoid having to make ThriftMetastoreAuthenticationType enum a string)

The PR contains following major changes:

  • Rename the Thrift metastore client factories to distinguish them better given what the interface exposes
Interface old name Signature Return Type New name
ThriftMetastoreFactory (highest level) createMetastore(Optional<ConnectorIdentity> identity) ThriftMetastore ThriftMetastoreFactory (unchanged)
TokenDelegationThriftMetastoreFactory createMetastoreClient(Optional<ConnectorIdentity> identity) ThriftMetastoreClient IdentityAwareMetastoreClientFactory
MetastoreLocator createMetastoreClient(Optional<String> token) ThriftMetastoreClient TokenAwareMetastoreClientFactory
ThriftMetastoreClientFactory (lowest level) create(HostAndPort address, Optional<String> delegationToken) ThriftMetastoreClient ThriftMetastoreClientFactory (unchanged)
  • Narrow down usage of ThriftMetastoreAuthenticationType to make auth decision. (Also makes TokenDelegationThriftMetastoreFactory aka IdentityAwareMetastoreClientFactory an interface)

  • Allow binding your own thrift authentication module

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jun 22, 2022
@phd3 phd3 marked this pull request as draft June 22, 2022 15:21
@phd3 phd3 marked this pull request as ready for review June 24, 2022 16:28
@phd3 phd3 requested review from findepi and electrum June 24, 2022 17:32
@findepi findepi requested a review from dain June 25, 2022 21:13
@phd3
Copy link
Member Author

phd3 commented Jul 18, 2022

@dain @electrum can you please take a look?

@phd3 phd3 force-pushed the hms-auth branch 6 times, most recently from a2a28e2 to 4d564c9 Compare July 29, 2022 17:47
@phd3
Copy link
Member Author

phd3 commented Jul 29, 2022

Rebased with some modifications to align with the recent refactoring. some more details in PR description. @dain @losipiuk does this approach look reasonable?

@phd3
Copy link
Member Author

phd3 commented Aug 3, 2022

cc @electrum

Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Couple of minor comments, but otherwise looks good

@dain dain merged commit bb608ef into trinodb:master Sep 5, 2022
@github-actions github-actions bot added this to the 395 milestone Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants