Delay S3 repo warning if default region absent (elastic#133848) (elastic#133917)

Today we emit a log message at startup if we fail to look up the default
region for use by the S3 repository. However, this is only a potential
issue if the default region is needed. If the user doesn't use
`repository-s3` at all, or specifies the `region` for each S3
repository, then the absence of the default region is not an issue.

With this commit we delay logging anything until we encounter a
situation where we actually need the default region for something.

Author: DaveCTurner

docs/changelog/133848.yaml

@@ -0,0 +1,5 @@
+pr: 133848
+summary: Delay S3 repo warning if default region absent
+area: Snapshot/Restore
+type: bug
+issues: []

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

@@ -12,6 +12,7 @@
 import software.amazon.awssdk.auth.credentials.AwsCredentials;
 import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
 import software.amazon.awssdk.http.SdkHttpClient;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.S3Client;
 
 import org.apache.logging.log4j.LogManager;
@@ -306,7 +307,7 @@ public static final class ProxyS3Service extends S3Service {
                 ProjectResolver projectResolver,
                 ResourceWatcherService resourceWatcherService
             ) {
-                super(environment, clusterService, projectResolver, resourceWatcherService, () -> null);
+                super(environment, clusterService, projectResolver, resourceWatcherService, () -> Region.of(randomIdentifier()));
             }
 
             @Override

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

@@ -0,0 +1,61 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.repositories.s3;
+
+import software.amazon.awssdk.regions.Region;
+
+import org.elasticsearch.common.util.concurrent.RunOnce;
+import org.elasticsearch.logging.LogManager;
+import org.elasticsearch.logging.Logger;
+
+import java.util.function.Supplier;
+
+/**
+ * Holds onto the {@link Region} provided by the given supplier (think: the AWS SDK's default provider chain) in case it's needed for a S3
+ * repository. If the supplier fails with an exception, the first call to {@link #getDefaultRegion} will log a warning message recording
+ * the exception.
+ */
+class S3DefaultRegionHolder {
+
+    private static final Logger logger = LogManager.getLogger(S3DefaultRegionHolder.class);
+
+    // no synchronization required, assignments happen in start() which happens-before all reads
+    private Region defaultRegion;
+    private Runnable defaultRegionFailureLogger = () -> {};
+
+    private final Runnable initializer;
+
+    /**
+     * @param delegateRegionSupplier Supplies a non-null {@link Region} or throws a {@link RuntimeException}.
+     *                               <p>
+     *                               Retained until its first-and-only invocation when {@link #start()} is called, and then released.
+     */
+    S3DefaultRegionHolder(Supplier<Region> delegateRegionSupplier) {
+        initializer = new RunOnce(() -> {
+            try {
+                defaultRegion = delegateRegionSupplier.get();
+                assert defaultRegion != null;
+            } catch (Exception e) {
+                defaultRegion = null;
+                defaultRegionFailureLogger = new RunOnce(() -> logger.warn("failed to obtain region from default provider chain", e));
+            }
+        });
+    }
+
+    void start() {
+        initializer.run();
+    }
+
+    Region getDefaultRegion() {
+        assert defaultRegion != null || defaultRegionFailureLogger instanceof RunOnce : "not initialized";
+        defaultRegionFailureLogger.run();
+        return defaultRegion;
+    }
+}

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

@@ -98,12 +98,7 @@ S3Service s3Service(
     }
 
     private static Region getDefaultRegion() {
-        try {
-            return DefaultAwsRegionProviderChain.builder().build().getRegion();
-        } catch (Exception e) {
-            logger.info("failed to obtain region from default provider chain", e);
-            return null;
-        }
+        return DefaultAwsRegionProviderChain.builder().build().getRegion();
     }
 
     @Override

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

@@ -48,7 +48,6 @@
 import org.elasticsearch.common.component.AbstractLifecycleComponent;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.util.concurrent.RunOnce;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.Releasable;
 import org.elasticsearch.core.Releasables;
@@ -94,8 +93,7 @@ class S3Service extends AbstractLifecycleComponent {
         Setting.Property.NodeScope
     );
 
-    private final Runnable defaultRegionSetter;
-    private volatile Region defaultRegion;
+    private final S3DefaultRegionHolder defaultRegionHolder;
 
     /**
      * Use a signer that does not require to pre-read (and checksum) the body of PutObject and UploadPart requests since we can rely on
@@ -129,7 +127,7 @@ class S3Service extends AbstractLifecycleComponent {
         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());
+        defaultRegionHolder = new S3DefaultRegionHolder(defaultRegionSupplier);
         s3ClientsManager = new S3ClientsManager(
             nodeSettings,
             this::buildClientReference,
@@ -266,7 +264,7 @@ Region getClientRegion(S3ClientSettings clientSettings) {
         } else {
             endpointDescription = "no configured endpoint";
         }
-        final var defaultRegion = this.defaultRegion;
+        final var defaultRegion = defaultRegionHolder.getDefaultRegion();
         if (defaultRegion != null) {
             LOGGER.debug("""
                 found S3 client with no configured region and {}, using region [{}] from SDK""", endpointDescription, defaultRegion);
@@ -415,7 +413,7 @@ public void onBlobStoreClose(@Nullable ProjectId projectId) {
 
     @Override
     protected void doStart() {
-        defaultRegionSetter.run();
+        defaultRegionHolder.start();
     }
 
     @Override

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

@@ -101,6 +101,7 @@
 import static org.elasticsearch.repositories.s3.S3ClientSettings.MAX_CONNECTIONS_SETTING;
 import static org.elasticsearch.repositories.s3.S3ClientSettings.MAX_RETRIES_SETTING;
 import static org.elasticsearch.repositories.s3.S3ClientSettings.READ_TIMEOUT_SETTING;
+import static org.elasticsearch.repositories.s3.S3ClientSettingsTests.DEFAULT_REGION_UNAVAILABLE;
 import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.contains;
@@ -134,7 +135,7 @@ public void setUp() throws Exception {
             ClusterServiceUtils.createClusterService(new DeterministicTaskQueue().getThreadPool()),
             TestProjectResolvers.DEFAULT_PROJECT_ONLY,
             Mockito.mock(ResourceWatcherService.class),
-            () -> null
+            DEFAULT_REGION_UNAVAILABLE
         ) {
             private InetAddress[] resolveHost(String host) throws UnknownHostException {
                 assertEquals("127.0.0.1", host);
@@ -1323,7 +1324,7 @@ public void testRetryOn403InStateless() {
             ),
             TestProjectResolvers.DEFAULT_PROJECT_ONLY,
             Mockito.mock(ResourceWatcherService.class),
-            () -> null
+            DEFAULT_REGION_UNAVAILABLE
         );
         service.start();
         recordingMeterRegistry = new RecordingMeterRegistry();

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

@@ -13,6 +13,7 @@
 import software.amazon.awssdk.auth.credentials.AwsSessionCredentials;
 import software.amazon.awssdk.regions.Region;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.cluster.project.TestProjectResolvers;
 import org.elasticsearch.common.settings.MockSecureSettings;
 import org.elasticsearch.common.settings.Settings;
@@ -24,6 +25,7 @@
 import org.mockito.Mockito;
 
 import java.util.Map;
+import java.util.function.Supplier;
 
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.emptyString;
@@ -190,9 +192,11 @@ public void testRegionCanBeSet() {
                 ClusterServiceUtils.createClusterService(new DeterministicTaskQueue().getThreadPool()),
                 TestProjectResolvers.DEFAULT_PROJECT_ONLY,
                 Mockito.mock(ResourceWatcherService.class),
-                () -> null
+                DEFAULT_REGION_UNAVAILABLE
             )
         ) {
+            s3Service.start();
+
             var otherSettings = settings.get("other");
             Region otherRegion = s3Service.getClientRegion(otherSettings);
             assertEquals(randomRegion, otherRegion.toString());
@@ -213,4 +217,8 @@ public void testMaxConnectionsCanBeSet() {
         // the default appears in the docs so let's make sure it doesn't change:
         assertEquals(50, S3ClientSettings.Defaults.MAX_CONNECTIONS);
     }
+
+    public static final Supplier<Region> DEFAULT_REGION_UNAVAILABLE = () -> {
+        throw new ElasticsearchException("default region unavailable in this test");
+    };
 }

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

@@ -0,0 +1,85 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.repositories.s3;
+
+import software.amazon.awssdk.regions.Region;
+
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.core.LogEvent;
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.MockLog;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public class S3DefaultRegionHolderTests extends ESTestCase {
+    public void testSuccess() {
+        try (var mockLog = MockLog.capture(S3DefaultRegionHolder.class)) {
+            mockLog.addExpectation(new NoLogEventsExpectation());
+
+            final var region = Region.of(randomIdentifier());
+            final var regionSupplied = new AtomicBoolean();
+            final var regionHolder = new S3DefaultRegionHolder(() -> {
+                assertTrue(regionSupplied.compareAndSet(false, true)); // only called once
+                return region;
+            });
+
+            regionHolder.start();
+            assertTrue(regionSupplied.get());
+            assertSame(region, regionHolder.getDefaultRegion());
+            assertSame(region, regionHolder.getDefaultRegion());
+
+            mockLog.assertAllExpectationsMatched();
+        }
+    }
+
+    public void testFailure() {
+        try (var mockLog = MockLog.capture(S3DefaultRegionHolder.class)) {
+            final var warningSeenExpectation = new MockLog.EventuallySeenEventExpectation(
+                "warning",
+                S3DefaultRegionHolder.class.getCanonicalName(),
+                Level.WARN,
+                "failed to obtain region from default provider chain"
+            );
+            mockLog.addExpectation(warningSeenExpectation);
+
+            final var regionSupplied = new AtomicBoolean();
+            final var regionHolder = new S3DefaultRegionHolder(() -> {
+                assertTrue(regionSupplied.compareAndSet(false, true)); // only called once
+                throw new ElasticsearchException("simulated");
+            });
+
+            regionHolder.start();
+            assertTrue(regionSupplied.get());
+
+            warningSeenExpectation.setExpectSeen(); // not seen yet, but will be seen now
+            regionHolder.getDefaultRegion();
+
+            mockLog.addExpectation(new NoLogEventsExpectation()); // log message not duplicated
+            regionHolder.getDefaultRegion();
+
+            mockLog.assertAllExpectationsMatched();
+        }
+    }
+
+    private static class NoLogEventsExpectation implements MockLog.LoggingExpectation {
+        private boolean seenLogEvent;
+
+        @Override
+        public void match(LogEvent event) {
+            seenLogEvent = true;
+        }
+
+        @Override
+        public void assertMatched() {
+            assertFalse(seenLogEvent);
+        }
+    }
+}

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

@@ -10,6 +10,7 @@
 package org.elasticsearch.repositories.s3;
 
 import software.amazon.awssdk.http.SdkHttpClient;
+import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.S3Client;
 
 import org.elasticsearch.cluster.metadata.ProjectId;
@@ -68,7 +69,7 @@ private static class DummyS3Service extends S3Service {
             ProjectResolver projectResolver,
             ResourceWatcherService resourceWatcherService
         ) {
-            super(environment, clusterService, projectResolver, resourceWatcherService, () -> null);
+            super(environment, clusterService, projectResolver, resourceWatcherService, () -> Region.of(randomIdentifier()));
         }
 
         @Override