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

Support vended s3 credentials in the Iceberg REST catalog #20186

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Support vended s3 credentials in the Iceberg REST catalog #20186

merged 1 commit into from
Feb 5, 2024

Conversation

alexjo2144
Copy link
Member

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

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

@cla-bot cla-bot bot added the cla-signed label Dec 20, 2023
@github-actions github-actions bot added tests:hive iceberg Iceberg connector labels Dec 20, 2023
@ebyhr
Copy link
Member

ebyhr commented Dec 21, 2023 •
edited
Loading

Pushed another commit to fix NPE and remove redundant annotation. Feel free to drop or edit the commit.

@findepi
Copy link
Member

findepi commented Dec 21, 2023

@danielcweeks can you please take a look?

danielcweeks
danielcweeks reviewed Dec 21, 2023
View reviewed changes
...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java
@@ -81,13 +83,18 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity)
warehouse.ifPresent(location -> properties.put(CatalogProperties.WAREHOUSE_LOCATION, location));
properties.put("trino-version", trinoVersion);
properties.putAll(securityProperties.get());

if (vendedCredentialsEnabled) {
properties.put("header.x-tabular-s3-access", "vended_credentials");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably standardize this to something like header.x-iceberg-s3-access so we don't reference Tabular specifically. We can add this to the REST Spec as a signal that the client is requesting credentials.

Copy link
Member

Choose a reason for hiding this comment

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

@danielcweeks was this ever standardized?

danielcweeks
danielcweeks reviewed Dec 21, 2023
View reviewed changes
pom.xml Outdated Show resolved Hide resolved
@danielcweeks
Copy link
Contributor

@alexjo2144 minor comments, but I was able to get this running with our smoke tests, and everything passed.

electrum
electrum reviewed Dec 25, 2023
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Looks good at a high level. This is the design we discussed. I haven’t had time to do a full review yet.

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3Context.java Outdated Show resolved Hide resolved
lib/trino-filesystem/src/main/java/io/trino/filesystem/s3/S3FileSystemConstants.java Outdated Show resolved Hide resolved
.../src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogFileSystemFactory.java Outdated
public TrinoFileSystem create(ConnectorIdentity identity, Map<String, String> fileIoProperties)
{
if ("true".equals(fileIoProperties.get(REMOTE_SIGNING_ENABLED))) {
throw new TrinoException(NOT_SUPPORTED, "Remote signing is not supported");
Copy link
Member

Choose a reason for hiding this comment

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

When would this error occur? If this is a bug/problem with the REST catalog service, we should add a new Iceberg specific error code. It's not something unsupported in Trino.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I think we should take this check out alltogether. If the rest catalog server requests remote signing ignore it and use any credentials the user may have provided in the Trino config file.

.../src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogFileSystemFactory.java Outdated Show resolved Hide resolved
.../src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogFileSystemFactory.java Outdated Show resolved Hide resolved
...ino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogModule.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergWritableTableHandle.java Outdated Show resolved Hide resolved
electrum
electrum approved these changes Feb 1, 2024
View reviewed changes
plugin/trino-iceberg/pom.xml Show resolved Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSourceProvider.java Outdated Show resolved Hide resolved
.../src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogFileSystemFactory.java Outdated Show resolved Hide resolved
.../src/main/java/io/trino/plugin/iceberg/catalog/rest/IcebergRestCatalogFileSystemFactory.java Show resolved Hide resolved
...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java
@@ -81,13 +83,18 @@ public synchronized TrinoCatalog create(ConnectorIdentity identity)
warehouse.ifPresent(location -> properties.put(CatalogProperties.WAREHOUSE_LOCATION, location));
properties.put("trino-version", trinoVersion);
properties.putAll(securityProperties.get());

if (vendedCredentialsEnabled) {
properties.put("header.x-tabular-s3-access", "vended_credentials");
Copy link
Member

Choose a reason for hiding this comment

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

@danielcweeks was this ever standardized?

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java Show resolved Hide resolved
...va/io/trino/plugin/iceberg/catalog/rest/TestIcebergVendingRestCatalogConnectorSmokeTest.java Show resolved Hide resolved
...containers/src/main/java/io/trino/testing/containers/IcebergRestCatalogBackendContainer.java Outdated Show resolved Hide resolved
@alexjo2144 alexjo2144 force-pushed the iceberg/v2-signer branch 2 times, most recently from 84c5612 to 497f7c1 Compare February 1, 2024 20:45
@alexjo2144
Copy link
Member Author

Thanks for the review @electrum, addressed those couple comments and resoled conflicts.

@electrum
Copy link
Member

electrum commented Feb 2, 2024

It looks like we'll need to add trino-filesystem-s3 as a runtime dependency, since it's used directly by the test code but is also used transitively. I can't see a good way around that. You shouldn't need to add the AWS dependencies, though.

@alexjo2144
Copy link
Member Author

alexjo2144 commented Feb 2, 2024 •
edited
Loading

You shouldn't need to add the AWS dependencies, though.

I did wind up having to put those back in. They're used directly in the test as well, to set up the temporary session auth.

@electrum electrum merged commit 9730845 into trinodb:master Feb 5, 2024
88 of 90 checks passed
@github-actions github-actions bot added this to the 439 milestone Feb 5, 2024
@danielcweeks danielcweeks mentioned this pull request Feb 6, 2024
Merged
@colebow colebow mentioned this pull request Feb 6, 2024
Merged
@colebow
Copy link
Member

colebow commented Feb 6, 2024

Does this need a release note? cc @alexjo2144 @electrum

@findinpath findinpath mentioned this pull request Mar 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum approved these changes

@danielcweeks danielcweeks danielcweeks left review comments

@findepi findepi Awaiting requested review from findepi

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed iceberg Iceberg connector
Milestone
439
Development

Successfully merging this pull request may close these issues.
6 participants
@alexjo2144 @ebyhr @findepi @danielcweeks @electrum @colebow