Skip to content

Commit

Permalink
Correctly cache S3 client in S3 native FS with security mapping
Browse files Browse the repository at this point in the history
S3 native FS with security mapping enabled is supposed to cache S3
clients for mappings resolved using both an identity and location.
This change avoids creating new caches when resolving locations.
  • Loading branch information
nineinchnick authored and wendigo committed Oct 9, 2024
1 parent 5646cc2 commit 8916dce
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 73 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@
import software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;

import java.net.URI;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executor;
import java.util.concurrent.ExecutorService;
import java.util.function.Function;
Expand All @@ -58,6 +60,7 @@ final class S3FileSystemLoader
private final S3Presigner preSigner;
private final S3Context context;
private final ExecutorService uploadExecutor = newCachedThreadPool(daemonThreadsNamed("s3-upload-%s"));
private final Map<Optional<S3SecurityMappingResult>, S3Client> clients = new ConcurrentHashMap<>();

@Inject
public S3FileSystemLoader(S3SecurityMappingProvider mappingProvider, OpenTelemetry openTelemetry, S3FileSystemConfig config, S3FileSystemStats stats)
Expand Down Expand Up @@ -95,7 +98,18 @@ private S3FileSystemLoader(Optional<S3SecurityMappingProvider> mappingProvider,
@Override
public TrinoFileSystemFactory apply(Location location)
{
return new S3SecurityMappingFileSystemFactory(mappingProvider.orElseThrow(), clientFactory, preSigner, context, location, uploadExecutor);
return identity -> {
Optional<S3SecurityMappingResult> mapping = mappingProvider.orElseThrow().getMapping(identity, location);

S3Client client = clients.computeIfAbsent(mapping, _ -> clientFactory.create(mapping));
S3Context context = this.context.withCredentials(identity);

if (mapping.isPresent() && mapping.get().kmsKeyId().isPresent()) {
context = context.withKmsKeyId(mapping.get().kmsKeyId().get());
}

return new S3FileSystem(uploadExecutor, client, preSigner, context);
};
}

@PreDestroy
Expand Down

This file was deleted.

0 comments on commit 8916dce

Please sign in to comment.