Add S3 security mapping for native S3 by electrum · Pull Request #22559 · trinodb/trino

electrum · 2024-07-02T08:15:03Z

Description

Release notes

(x) Release notes are required, with the following suggested text:

# Object storage
* Add S3 security mapping for native S3. ({issue}`22559`)

electrum · 2024-07-02T08:17:52Z

@mosabua This is compatible with the legacy S3 feature and uses the same config properties (minus the hive. prefix). How do you want to handle the docs? Should we just copy/paste?

nineinchnick

Are the tests based on some existing ones in the legacy FS? I haven't reviewed them for coverage.

nineinchnick · 2024-07-02T08:50:55Z

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingsFileSource.java

Why not add @NotNull to getConfigFilePath()?

It's not needed because the code directly returns a non-null Optional. The validation annotations are for the Bean Validation framework to validate user-supplied values.

nineinchnick · 2024-07-02T08:51:36Z

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingsFileSource.java

Can this be checked in the config class?

We can't use @FileExists because the file path can also be a URL. But we could use @AssertTrue. I'll add that.

Actually, I'm going to make two configs.

nineinchnick · 2024-07-02T09:31:30Z

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemFactory.java

Would be easier to review if moving of this code to the loader was extracted to a separate commit.

I completely agree, and tried to do that originally, but the code is restructured significantly. I didn't see a good way to do a clean refactoring.

mosabua · 2024-07-02T15:16:51Z

@mosabua This is compatible with the legacy S3 feature and uses the same config properties (minus the hive. prefix). How do you want to handle the docs? Should we just copy/paste?

Yeah .. we can just have it in the new docs as copy.. then cleaning up later will be easier. You want to do that or should I send a separate PR @electrum ?

electrum · 2024-07-02T19:06:34Z

I copied over the documentation and cleaned it up a bit.

mosabua

Just a couple of doc suggestions

docs/src/main/sphinx/object-storage/file-system-s3.md

mosabua · 2024-07-02T19:41:32Z

docs/src/main/sphinx/object-storage/file-system-s3.md

do we need to say what type of regex?

And we should explain that the examples uses an OR and therefore matches for alice and bob

Do we go into this level of detail in the other documentation? Note that this documentation is copied over from the original, so we could do a more major cleanup as a separate task.

We should .. just have not gotten around to fixing that up.

Let's follow-up with the docs improvement after this PR is merged.

docs/src/main/sphinx/object-storage/file-system-s3.md

mosabua · 2024-07-02T19:54:22Z

docs/src/main/sphinx/object-storage/file-system-s3.md

Path to the JSON

relative to ? or absolute? or both

It can be relative or absolute. I don't think we discuss "relative to what" in every place that uses additional files, but it's relative to the server configuration directory. Do we have a place that discusses that to link to?

Not sure we have a generic place .. maybe just go with Absolute or relative path to the JSON configuration file.

docs/src/main/sphinx/object-storage/file-system-s3.md

electrum · 2024-07-02T21:19:06Z

@mosabua Thanks for the great review of the documentation. I believe I've addressed all of your comments.

electrum · 2024-07-02T21:36:58Z

@nineinchnick The tests are based on the legacy S3 support.

TestS3SecurityMappinghas extensive tests to verify that we produce the correctS3SecurityMappingResult` record, which is used to configure the S3 client.

What is not tested, either here or in the legacy version, is that the client configuration is correct. We would like to verify that the client is making the correct requests, i.e. using the correct access key, IAM role, KMS key ID, etc. This is what I investigated:

Tracing: Doesn't work because AWS SDK tracing support for STS doesn't include the access key, role, etc.
LocalStack: Create users and roles, then setup S3 policies such that different objects are only accessible to certain users or roles. Test via side effect that the correct values are sent -- the requests should fail if the wrong values are sent. I see that LocalStack supports IAM and STS, but I don't know if their security implementation is complete enough for this.
Mocking: Use MockWebServer to validate requests and send mock responses for STS and S3. I have an initial version of this working, but didn't have time to complete it.

anusudarsan · 2024-07-15T13:27:11Z

@electrum can we merge this PR, and the integration tests can be a followup?

electrum · 2024-07-16T02:39:29Z

@anusudarsan yes, I don’t plan to work on integration tests

wendigo · 2024-07-16T12:13:46Z

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemLoader.java

+import static java.util.concurrent.Executors.newCachedThreadPool;
+
+final class S3FileSystemLoader
+        implements Function<Location, TrinoFileSystemFactory>


Nit: this could be a dedicated interface

We can introduce one later if it's need

wendigo · 2024-07-16T12:15:08Z

...o-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingFileSystemFactory.java

+            Location location,
+            Executor uploadExecutor)
+    {
+        this.mappingProvider = mappingProvider;


nit: rnn mappingProvider

Thanks, I missed that

wendigo · 2024-07-16T12:15:29Z

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3SecurityMappingProvider.java

+    private static Supplier<S3SecurityMappings> mappingsProvider(Supplier<S3SecurityMappings> supplier, Optional<Duration> refreshPeriod)
+    {
+        return refreshPeriod
+                .map(refresh -> Suppliers.memoizeWithExpiration(supplier::get, refresh.toMillis(), MILLISECONDS))


nit: static import: memoize*

wendigo · 2024-07-16T12:16:53Z

@electrum let's merge it and follow-up with code and test improvements

dain · 2024-07-16T22:20:20Z

docs/src/main/sphinx/object-storage/file-system-s3.md

  - External ID for the IAM role trust policy when connecting to S3.
 :::
+
+## Security mapping


Maybe move this "copy from legacy docs" stuff to a separate commit

cla-bot bot added the cla-signed label Jul 2, 2024

electrum requested review from anusudarsan and dain July 2, 2024 08:18

nineinchnick approved these changes Jul 2, 2024

View reviewed changes

electrum force-pushed the s3security branch from 4cf48dd to be16f7b Compare July 2, 2024 19:05

github-actions bot added the docs label Jul 2, 2024

anusudarsan approved these changes Jul 2, 2024

View reviewed changes

mosabua reviewed Jul 2, 2024

View reviewed changes

electrum force-pushed the s3security branch 2 times, most recently from 0e0743d to 18b0e6f Compare July 2, 2024 21:18

electrum force-pushed the s3security branch from 18b0e6f to bb6da2f Compare July 2, 2024 21:27

electrum force-pushed the s3security branch from bb6da2f to 25ca0c6 Compare July 2, 2024 21:41

electrum requested a review from wendigo July 15, 2024 22:44

wendigo reviewed Jul 16, 2024

View reviewed changes

wendigo approved these changes Jul 16, 2024

View reviewed changes

dain reviewed Jul 16, 2024

View reviewed changes

electrum added 4 commits July 30, 2024 12:42

Remove unused lifecycle for HdfsFileSystem

9e6f4f4

Bind AzureFileSystemFactory in singleton scope

693e5aa

Make SwitchingFileSystemFactory reusable

c818b53

Simplify S3 upload executor type

87e8921

electrum added 2 commits July 30, 2024 12:56

Copy legacy S3 security mapping documentation

5d604cd

Add S3 security mapping for native S3

2d31a6c

electrum force-pushed the s3security branch from 25ca0c6 to 2d31a6c Compare July 30, 2024 19:58

electrum merged commit 90ea793 into trinodb:master Jul 30, 2024

electrum deleted the s3security branch July 30, 2024 21:04

github-actions bot added this to the 454 milestone Jul 30, 2024

colebow mentioned this pull request Aug 7, 2024

Add Trino 454 release notes #22977

Merged

Conversation

electrum commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Uh oh!

electrum commented Jul 2, 2024

Uh oh!

nineinchnick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosabua commented Jul 2, 2024

Uh oh!

electrum commented Jul 2, 2024

Uh oh!

mosabua left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

electrum commented Jul 2, 2024

Uh oh!

electrum commented Jul 2, 2024

Uh oh!

anusudarsan commented Jul 15, 2024

Uh oh!

electrum commented Jul 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

electrum Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wendigo Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wendigo commented Jul 16, 2024

Uh oh!

electrum commented Jul 2, 2024 •

edited

Loading

electrum Jul 30, 2024 •

edited

Loading

wendigo Jul 16, 2024 •

edited

Loading