some tidying, questions, and marking todo work

Author: DiannaHohensee

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

@@ -254,7 +254,7 @@ public void close() {
 
     @Override
     public String serviceName() {
-        return null;
+        return "AmazonS3Wrapper";
     }
 
     @Override

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

@@ -259,9 +259,13 @@ S3Service s3Service(Environment environment, Settings nodeSettings, ResourceWatc
             return new ProxyS3Service(environment, nodeSettings, resourceWatcherService);
         }
 
+        /**
+         * This wrapper exposes a copy of the AWS credentials that the S3Client uses.
+         */
         public static final class ClientAndCredentials extends AmazonS3Wrapper {
             final AwsCredentialsProvider credentials;
-            final SdkHttpClient httpClient;
+            // The httpClient must be explicitly closed. Closure of the S3Client, which uses the httpClient, will not do so.
+            private final SdkHttpClient httpClient;
 
             ClientAndCredentials(S3Client delegate, SdkHttpClient httpClient, AwsCredentialsProvider credentials) {
                 super(delegate);

modules/repository-s3/src/internalClusterTest/java/org/elasticsearch/repositories/s3/S3BlobStoreRepositoryTests.java

@@ -658,12 +658,16 @@ protected static class S3ErroneousHttpHandler extends ErroneousHttpHandler {
         // S3 SDK stops retrying after TOKEN_BUCKET_SIZE/DEFAULT_EXCEPTION_TOKEN_COST == 500/5 == 100 failures in quick succession
         // see software.amazon.awssdk.retries.DefaultRetryStrategy.Legacy.TOKEN_BUCKET_SIZE
         // see software.amazon.awssdk.retries.DefaultRetryStrategy.Legacy.DEFAULT_EXCEPTION_TOKEN_COST
+        // TODO NOMERGE: do we need to use (100 - DEFAULT_EXCEPTION_TOKEN_COST) to avoid running out of tokens?
         private final Semaphore failurePermits = new Semaphore(100);
 
         S3ErroneousHttpHandler(final HttpHandler delegate, final int maxErrorsPerRequest) {
             super(delegate, maxErrorsPerRequest);
         }
 
+        /**
+         * Bypasses {@link ErroneousHttpHandler#handle} once we exhaust {@link #failurePermits} because S3 will start rate limiting.
+         */
         @Override
         public void handle(HttpExchange exchange) throws IOException {
             if (failurePermits.tryAcquire()) {

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

@@ -19,6 +19,7 @@
 import org.elasticsearch.common.settings.Setting.Property;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.core.UpdateForV10;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -76,7 +77,8 @@ final class S3ClientSettings {
         key -> new Setting<>(key, "", s -> s.toLowerCase(Locale.ROOT), Property.NodeScope)
     );
 
-    /** Formerly the protocol to use to connect to s3, now unused */
+    /** Formerly the protocol to use to connect to s3, now unused. V2 AWS SDK can infer the protocol from {@link #endpoint}. */
+    @UpdateForV10(owner = UpdateForV10.Owner.DISTRIBUTED_COORDINATION) // no longer used, should be removed in v10
     static final Setting.AffixSetting<HttpScheme> UNUSED_PROTOCOL_SETTING = Setting.affixKeySetting(
         PREFIX,
         "protocol",
@@ -139,7 +141,8 @@ final class S3ClientSettings {
         key -> Setting.intSetting(key, Defaults.RETRY_COUNT, 0, Property.NodeScope)
     );
 
-    /** Formerly whether retries should be throttled (ie use backoff), now unused. */
+    /** Formerly whether retries should be throttled (ie use backoff), now unused. V2 AWS SDK always uses throttling. */
+    @UpdateForV10(owner = UpdateForV10.Owner.DISTRIBUTED_COORDINATION) // no longer used, should be removed in v10
     static final Setting.AffixSetting<Boolean> UNUSED_USE_THROTTLE_RETRIES_SETTING = Setting.affixKeySetting(
         PREFIX,
         "use_throttle_retries",
@@ -167,7 +170,8 @@ final class S3ClientSettings {
         key -> Setting.simpleString(key, Property.NodeScope)
     );
 
-    /** Formerly an override for the signer to use, now unused. */
+    /** Formerly an override for the signer to use, now unused. V2 AWS SDK only supports AWS v4 signatures. */
+    @UpdateForV10(owner = UpdateForV10.Owner.DISTRIBUTED_COORDINATION) // no longer used, should be removed in v10
     static final Setting.AffixSetting<String> UNUSED_SIGNER_OVERRIDE = Setting.affixKeySetting(
         PREFIX,
         "signer_override",
@@ -354,6 +358,7 @@ static boolean checkDeprecatedCredentials(Settings repositorySettings) {
     }
 
     // backcompat for reading keys out of repository settings (clusterState)
+    // TODO NOMERGE: delete this comment? Doesn't give me any context to understand. Can we delete deprecated code?
     private static AwsCredentials loadDeprecatedCredentials(Settings repositorySettings) {
         assert checkDeprecatedCredentials(repositorySettings);
         try (
@@ -474,6 +479,6 @@ static final class Defaults {
         static final TimeValue READ_TIMEOUT = TimeValue.timeValueSeconds(50);
         static final int MAX_CONNECTIONS = 50;
         static final int RETRY_COUNT = 3;
-        static final boolean THROTTLE_RETRIES = true;
+        static final boolean THROTTLE_RETRIES = true; // NOMERGE: delete this and hardcode for one (deprecated) use?
     }
 }

test/fixtures/aws-fixture-utils/src/main/java/fixture/aws/AwsCredentialsUtils.java

@@ -26,6 +26,7 @@ public enum AwsCredentialsUtils {
     /**
      * Region supplier which matches any region.
      */
+    // TODO NOMERGE: replace with DynamicRegionSupplier.
     public static final Supplier<String> ANY_REGION = () -> "*";
 
     /**
@@ -72,7 +73,7 @@ public static boolean isValidAwsV4SignedAuthorizationHeader(
         }
 
         if (region.equals("*")) {
-            // skip region validation; TODO eliminate this when region is fixed in all tests
+            // skip region validation; TODO NOMERGE eliminate this when region is fixed in all tests
             return authorizationHeader.contains("/" + serviceName + "/aws4_request, ");
         }