Use DefaultAwsRegionProviderChain to obtain default region

Author: DaveCTurner

modules/repository-s3/src/javaRestTest/java/org/elasticsearch/repositories/s3/RepositoryS3ImdsV2CredentialsRestIT.java

@@ -41,7 +41,7 @@ public class RepositoryS3ImdsV2CredentialsRestIT extends AbstractRepositoryS3Res
 
     private static final Ec2ImdsHttpFixture ec2ImdsHttpFixture = new Ec2ImdsHttpFixture(
         new Ec2ImdsServiceBuilder(Ec2ImdsVersion.V2).newCredentialsConsumer(dynamicCredentials::addValidCredentials)
-            .instanceIdentityDocument((b, p) -> b.field("region", regionSupplier.get())) // TODO NOMERGE this region is not used
+            .instanceIdentityDocument((b, p) -> b.field("region", regionSupplier.get()))
     );
 
     private static final S3HttpFixture s3Fixture = new S3HttpFixture(true, BUCKET, BASE_PATH, dynamicCredentials::isAuthorized);
@@ -50,7 +50,6 @@ public class RepositoryS3ImdsV2CredentialsRestIT extends AbstractRepositoryS3Res
         .module("repository-s3")
         .setting("s3.client." + CLIENT + ".endpoint", s3Fixture::getAddress)
         .systemProperty(Ec2ImdsHttpFixture.ENDPOINT_OVERRIDE_SYSPROP_NAME_SDK2, ec2ImdsHttpFixture::getAddress)
-        .systemProperty("aws.region", regionSupplier)
         .build();
 
     @ClassRule

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java

@@ -10,6 +10,7 @@
 package org.elasticsearch.repositories.s3;
 
 import software.amazon.awssdk.regions.Region;
+import software.amazon.awssdk.regions.providers.DefaultAwsRegionProviderChain;
 
 import org.apache.lucene.util.SetOnce;
 import org.elasticsearch.SpecialPermission;
@@ -20,6 +21,8 @@
 import org.elasticsearch.common.util.BigArrays;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.indices.recovery.RecoverySettings;
+import org.elasticsearch.logging.LogManager;
+import org.elasticsearch.logging.Logger;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.plugins.ReloadablePlugin;
 import org.elasticsearch.plugins.RepositoryPlugin;
@@ -43,6 +46,8 @@
  */
 public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin, ReloadablePlugin {
 
+    private static final Logger logger = LogManager.getLogger(S3RepositoryPlugin.class);
+
     static {
         SpecialPermission.check();
         AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
@@ -87,11 +92,22 @@ protected S3Repository createRepository(
     public Collection<?> createComponents(PluginServices services) {
         service.set(s3Service(services.environment(), services.clusterService().getSettings(), services.resourceWatcherService()));
         this.service.get().refreshAndClearCache(S3ClientSettings.load(settings));
-        return List.of(service);
+        return List.of(service.get());
     }
 
     S3Service s3Service(Environment environment, Settings nodeSettings, ResourceWatcherService resourceWatcherService) {
-        return new S3Service(environment, nodeSettings, resourceWatcherService);
+        return new S3Service(environment, nodeSettings, resourceWatcherService, S3RepositoryPlugin::getDefaultRegion);
+    }
+
+    private static Region getDefaultRegion() {
+        return AccessController.doPrivileged((PrivilegedAction<Region>) () -> {
+            try {
+                return DefaultAwsRegionProviderChain.builder().build().getRegion();
+            } catch (Exception e) {
+                logger.debug("failed to obtain region from default provider chain", e);
+                return null;
+            }
+        });
     }
 
     @Override

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

@@ -42,9 +42,11 @@
 import org.elasticsearch.cluster.metadata.RepositoryMetadata;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.component.AbstractLifecycleComponent;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.Maps;
+import org.elasticsearch.common.util.concurrent.RunOnce;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.Releasable;
 import org.elasticsearch.core.Releasables;
@@ -55,7 +57,6 @@
 import org.elasticsearch.watcher.FileWatcher;
 import org.elasticsearch.watcher.ResourceWatcherService;
 
-import java.io.Closeable;
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -69,14 +70,14 @@
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 import java.util.function.Consumer;
+import java.util.function.Supplier;
 
 import static java.util.Collections.emptyMap;
-import static software.amazon.awssdk.core.SdkSystemSetting.AWS_REGION;
 import static software.amazon.awssdk.core.SdkSystemSetting.AWS_ROLE_ARN;
 import static software.amazon.awssdk.core.SdkSystemSetting.AWS_ROLE_SESSION_NAME;
 import static software.amazon.awssdk.core.SdkSystemSetting.AWS_WEB_IDENTITY_TOKEN_FILE;
 
-class S3Service implements Closeable {
+class S3Service extends AbstractLifecycleComponent {
     private static final Logger LOGGER = LogManager.getLogger(S3Service.class);
 
     static final Setting<TimeValue> REPOSITORY_S3_CAS_TTL_SETTING = Setting.timeSetting(
@@ -108,13 +109,21 @@ class S3Service implements Closeable {
      */
     private volatile Map<Settings, S3ClientSettings> derivedClientSettings = emptyMap();
 
+    private final Runnable defaultRegionSetter;
+    private volatile Region defaultRegion;
+
     final CustomWebIdentityTokenCredentialsProvider webIdentityTokenCredentialsProvider;
 
     final TimeValue compareAndExchangeTimeToLive;
     final TimeValue compareAndExchangeAntiContentionDelay;
     final boolean isStateless;
 
-    S3Service(Environment environment, Settings nodeSettings, ResourceWatcherService resourceWatcherService) {
+    S3Service(
+        Environment environment,
+        Settings nodeSettings,
+        ResourceWatcherService resourceWatcherService,
+        Supplier<Region> defaultRegionSupplier
+    ) {
         webIdentityTokenCredentialsProvider = new CustomWebIdentityTokenCredentialsProvider(
             environment,
             System::getenv,
@@ -125,6 +134,7 @@ class S3Service implements Closeable {
         compareAndExchangeTimeToLive = REPOSITORY_S3_CAS_TTL_SETTING.get(nodeSettings);
         compareAndExchangeAntiContentionDelay = REPOSITORY_S3_CAS_ANTI_CONTENTION_DELAY_SETTING.get(nodeSettings);
         isStateless = DiscoveryNode.isStateless(nodeSettings);
+        defaultRegionSetter = new RunOnce(() -> defaultRegion = defaultRegionSupplier.get());
     }
 
     /**
@@ -230,20 +240,53 @@ protected S3ClientBuilder buildClientBuilder(S3ClientSettings clientSettings, Sd
         if (clientSettings.pathStyleAccess) {
             s3clientBuilder.forcePathStyle(true);
         }
-        if (Strings.hasLength(clientSettings.region)) {
-            s3clientBuilder.region(Region.of(clientSettings.region));
-        } else if (AWS_REGION.getStringValue().isPresent() == false) {
-            // TODO NOMERGE: how we handle regions TBD, this allows testing to pass
-            // TODO NOMERGE: specifically we don't pick up the region from IMDS
-            s3clientBuilder.region(Region.of("us-east-1"));
-        }
+
+        s3clientBuilder.region(getClientRegion(clientSettings));
+
         if (Strings.hasLength(clientSettings.endpoint)) {
             s3clientBuilder.endpointOverride(URI.create(clientSettings.endpoint)); // TODO NOMERGE what if URI.create fails?
         }
 
         return s3clientBuilder;
     }
 
+    // TODO NOMERGE test this logic
+    private Region getClientRegion(S3ClientSettings clientSettings) {
+        assert lifecycle.initialized() == false : lifecycle;
+
+        if (Strings.hasLength(clientSettings.region)) {
+            return Region.of(clientSettings.region);
+        }
+        if (Strings.hasLength(clientSettings.endpoint)) {
+            final var guessedRegion = RegionFromEndpointGuesser.guessRegion(clientSettings.endpoint);
+            if (guessedRegion != null) {
+                LOGGER.warn(
+                    """
+                        found S3 client with endpoint [{}] but no configured region, guessing it should use [{}]; \
+                        to suppress this warning, configure the [{}] setting on this node""",
+                    clientSettings.endpoint,
+                    guessedRegion,
+                    S3ClientSettings.REGION.getConcreteSettingForNamespace("CLIENT_NAME").getKey()
+                );
+                return Region.of(guessedRegion);
+            }
+        }
+        final var defaultRegion = this.defaultRegion;
+        if (defaultRegion != null) {
+            LOGGER.debug("""
+                found S3 client with no configured region, using region [{}] from SDK""", defaultRegion);
+            return defaultRegion;
+        }
+        LOGGER.warn(
+            """
+                found S3 client with no configured region, falling back to [{}]; \
+                to suppress this warning, configure the [{}] setting on this node""",
+            Region.US_EAST_1,
+            S3ClientSettings.REGION.getConcreteSettingForNamespace("CLIENT_NAME").getKey()
+        );
+        return Region.US_EAST_1;
+    }
+
     @Nullable // in production, but exposed for tests to override
     DnsResolver getCustomDnsResolver() {
         return null;
@@ -373,7 +416,15 @@ public void onBlobStoreClose() {
     }
 
     @Override
-    public void close() throws IOException {
+    protected void doStart() {
+        defaultRegionSetter.run();
+    }
+
+    @Override
+    protected void doStop() {}
+
+    @Override
+    public void doClose() throws IOException {
         releaseCachedClients();
         webIdentityTokenCredentialsProvider.close();
     }

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/RepositoryCredentialsTests.java

@@ -299,7 +299,7 @@ public static final class ProxyS3Service extends S3Service {
             private static final Logger logger = LogManager.getLogger(ProxyS3Service.class);
 
             ProxyS3Service(Environment environment, Settings nodeSettings, ResourceWatcherService resourceWatcherService) {
-                super(environment, nodeSettings, resourceWatcherService);
+                super(environment, nodeSettings, resourceWatcherService, () -> null);
             }
 
             @Override

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3BlobContainerRetriesTests.java

@@ -119,7 +119,7 @@ public class S3BlobContainerRetriesTests extends AbstractBlobContainerRetriesTes
     @Before
     public void setUp() throws Exception {
         shouldErrorOnDns = false;
-        service = new S3Service(Mockito.mock(Environment.class), Settings.EMPTY, Mockito.mock(ResourceWatcherService.class)) {
+        service = new S3Service(Mockito.mock(Environment.class), Settings.EMPTY, Mockito.mock(ResourceWatcherService.class), () -> null) {
             private InetAddress[] resolveHost(String host) throws UnknownHostException {
                 assertEquals("127.0.0.1", host);
                 if (shouldErrorOnDns && randomBoolean() && randomBoolean()) {
@@ -133,6 +133,7 @@ DnsResolver getCustomDnsResolver() {
                 return this::resolveHost;
             }
         };
+        service.start();
         recordingMeterRegistry = new RecordingMeterRegistry();
         super.setUp();
     }

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java

@@ -184,7 +184,14 @@ public void testRegionCanBeSet() throws IOException {
         assertThat(settings.get("default").region, is(""));
         assertThat(settings.get("other").region, is(region));
 
-        try (var s3Service = new S3Service(Mockito.mock(Environment.class), Settings.EMPTY, Mockito.mock(ResourceWatcherService.class))) {
+        try (
+            var s3Service = new S3Service(
+                Mockito.mock(Environment.class),
+                Settings.EMPTY,
+                Mockito.mock(ResourceWatcherService.class),
+                () -> null
+            )
+        ) {
             // TODO NOMERGE: S3Client#getBucketLocation is supposed to be a work around to access the region: the response object includes
             // the region. However, we can't build a S3Client in this file, so we need to migrate it elsewhere.
             // "Unable to load credentials from any of the providers in the chain AwsCredentialsProviderChain"

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java

@@ -55,7 +55,7 @@ public String serviceName() {
     private static class DummyS3Service extends S3Service {
 
         DummyS3Service(Environment environment, ResourceWatcherService resourceWatcherService) {
-            super(environment, Settings.EMPTY, resourceWatcherService);
+            super(environment, Settings.EMPTY, resourceWatcherService, () -> null);
         }
 
         @Override
@@ -67,7 +67,9 @@ public AmazonS3Reference client(RepositoryMetadata repositoryMetadata) {
         public void refreshAndClearCache(Map<String, S3ClientSettings> clientsSettings) {}
 
         @Override
-        public void close() {}
+        public void doClose() {
+            // nothing to clean up
+        }
     }
 
     public void testInvalidChunkBufferSizeSettings() {

modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java

@@ -12,6 +12,7 @@
 import software.amazon.awssdk.awscore.exception.AwsServiceException;
 import software.amazon.awssdk.core.retry.RetryPolicyContext;
 import software.amazon.awssdk.core.retry.conditions.RetryCondition;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.model.S3Exception;
 
 import org.elasticsearch.cluster.metadata.RepositoryMetadata;
@@ -29,7 +30,13 @@
 public class S3ServiceTests extends ESTestCase {
 
     public void testCachedClientsAreReleased() throws IOException {
-        final S3Service s3Service = new S3Service(mock(Environment.class), Settings.EMPTY, mock(ResourceWatcherService.class));
+        final S3Service s3Service = new S3Service(
+            mock(Environment.class),
+            Settings.EMPTY,
+            mock(ResourceWatcherService.class),
+            () -> Region.of("es-test-region")
+        );
+        s3Service.start();
         final String endpointOverride = "http://first";
         final Settings settings = Settings.builder().put("endpoint", endpointOverride).build();
         final RepositoryMetadata metadata1 = new RepositoryMetadata("first", "s3", settings);
@@ -44,13 +51,14 @@ public void testCachedClientsAreReleased() throws IOException {
         assertEquals("es-test-region", reference.client().serviceClientConfiguration().region().toString());
 
         reference.close();
-        s3Service.close();
+        s3Service.doClose();
         final AmazonS3Reference referenceReloaded = s3Service.client(metadata1);
         assertNotSame(referenceReloaded, reference);
         referenceReloaded.close();
-        s3Service.close();
+        s3Service.doClose();
         final S3ClientSettings clientSettingsReloaded = s3Service.settings(metadata1);
         assertNotSame(clientSettings, clientSettingsReloaded);
+        s3Service.close();
     }
 
     public void testRetryOn403RetryPolicy() {