diff --git a/CHANGELOG.md b/CHANGELOG.md index f5bc871e1af20..3589a7a3eb449 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix Netty deprecation warnings in transport-netty4 module ([#20233](https://github.com/opensearch-project/OpenSearch/pull/20233)) - Fix snapshot restore when an index sort is present ([#20284](https://github.com/opensearch-project/OpenSearch/pull/20284)) - Fix SearchPhaseExecutionException to properly initCause ([#20320](https://github.com/opensearch-project/OpenSearch/pull/20320)) +- [repository-s3] remove endpointOverride and let AWS SDK V2 S3 determine the s3 url based on bucket name or arn provided ([#20345](https://github.com/opensearch-project/OpenSearch/pull/20345)) ### Dependencies - Bump `com.google.auth:google-auth-library-oauth2-http` from 1.38.0 to 1.41.0 ([#20183](https://github.com/opensearch-project/OpenSearch/pull/20183)) diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java index 9161e5cce7ed6..124c92b84cfc0 100644 --- a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java +++ b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java @@ -95,6 +95,7 @@ import java.security.SecureRandom; import java.time.Duration; import java.util.Map; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ScheduledExecutorService; @@ -221,23 +222,10 @@ AmazonS3WithCredentials buildClient(final S3ClientSettings clientSettings) { builder.httpClientBuilder(buildHttpClient(clientSettings)); builder.overrideConfiguration(buildOverrideConfiguration(clientSettings, clientExecutorService)); - String endpoint = Strings.hasLength(clientSettings.endpoint) ? clientSettings.endpoint : DEFAULT_S3_ENDPOINT; - if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { - // Manually add the schema to the endpoint to work around https://github.com/aws/aws-sdk-java/issues/2274 - // TODO: Remove this once fixed in the AWS SDK - endpoint = clientSettings.protocol.toString() + "://" + endpoint; - } - logger.debug("using endpoint [{}] and region [{}]", endpoint, clientSettings.region); - - // If the endpoint configuration isn't set on the builder then the default behaviour is to try - // and work out what region we are in and use an appropriate endpoint - see AwsClientBuilder#setRegion. - // In contrast, directly-constructed clients use s3.amazonaws.com unless otherwise instructed. We currently - // use a directly-constructed client, and need to keep the existing behaviour to avoid a breaking change, - // so to move to using the builder we must set it explicitly to keep the existing behaviour. - // - // We do this because directly constructing the client is deprecated (was already deprecated in 1.1.223 too) - // so this change removes that usage of a deprecated API. - builder.endpointOverride(URI.create(endpoint)); + // Only apply endpointOverride when the user explicitly configures "endpoint". + // If "endpoint" is absent, DO NOT override; allow the AWS SDK to resolve endpoints dynamically. + // This is required for ARN buckets (access points / outposts / MRAP), and is also correct for normal buckets. + resolveEndpointOverride(clientSettings).ifPresent(builder::endpointOverride); if (Strings.hasText(clientSettings.region)) { builder.region(Region.of(clientSettings.region)); } @@ -256,6 +244,28 @@ AmazonS3WithCredentials buildClient(final S3ClientSettings clientSettings) { return AmazonS3WithCredentials.create(client, credentials); } + /** + * Returns an endpoint override ONLY when the user explicitly provided one. + * Otherwise returns Optional.empty(). + * + * Package-private to allow unit testing. + */ + Optional resolveEndpointOverride(final S3ClientSettings clientSettings) { + if (Strings.hasLength(clientSettings.endpoint) == false) { + return Optional.empty(); + } + + String endpoint = clientSettings.endpoint; + if ((endpoint.startsWith("http://") || endpoint.startsWith("https://")) == false) { + // Manually add the schema to the endpoint to work around https://github.com/aws/aws-sdk-java/issues/2274 + // TODO: Remove this once fixed in the AWS SDK + endpoint = clientSettings.protocol.toString() + "://" + endpoint; + } + + // Use URI.create for simplicity; if your codebase prefers checked handling, swap to new URI(endpoint). + return Optional.of(URI.create(endpoint)); + } + // Aws v2 sdk tries to load a default profile from home path which is restricted. Hence, setting these to random // valid paths. @SuppressForbidden(reason = "Need to provide this override to v2 SDK so that path does not default to home path") diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.java new file mode 100644 index 0000000000000..80d25c24d3375 --- /dev/null +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ArnEndpointResolutionTests.java @@ -0,0 +1,196 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.repositories.s3; + +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration; +import software.amazon.awssdk.core.interceptor.Context; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; +import software.amazon.awssdk.http.ExecutableHttpRequest; +import software.amazon.awssdk.http.HttpExecuteRequest; +import software.amazon.awssdk.http.HttpExecuteResponse; +import software.amazon.awssdk.http.SdkHttpClient; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; +import software.amazon.awssdk.services.s3.S3Configuration; +import software.amazon.awssdk.services.s3.model.HeadBucketRequest; + +import java.net.URI; +import java.util.concurrent.atomic.AtomicReference; + +/** + * Tests for AWS SDK endpoint resolution behavior for S3 bucket ARNs (Outposts / Access Points), + * and OpenSearch repository-s3 behavior around endpoint overrides. + * + * These tests are intentionally "no-network": they capture the resolved request URI using an + * ExecutionInterceptor and abort the request via a failing HTTP client. + */ +public class S3ArnEndpointResolutionTests extends AbstractS3RepositoryTestCase { + + private static final StaticCredentialsProvider DUMMY_CREDENTIALS = StaticCredentialsProvider.create( + AwsBasicCredentials.create("dummy-access-key", "dummy-secret-key") + ); + + // ---- Helpers ---- + + private static final class CapturingInterceptor implements ExecutionInterceptor { + private final AtomicReference captured = new AtomicReference<>(); + private final AtomicReference capturedSigningName = new AtomicReference<>(); + + @Override + public void beforeTransmission(Context.BeforeTransmission context, ExecutionAttributes executionAttributes) { + captured.set(context.httpRequest().getUri()); + // Capture the signing name from the Authorization header + // Format: AWS4-HMAC-SHA256 Credential=AKID/20250101/us-east-1/s3-outposts/aws4_request, ... + context.httpRequest().firstMatchingHeader("Authorization").ifPresent(auth -> { + int credIdx = auth.indexOf("Credential="); + if (credIdx >= 0) { + String[] parts = auth.substring(credIdx).split("/"); + if (parts.length >= 5) { + capturedSigningName.set(parts[3]); // service name is 4th part (index 3) + } + } + }); + } + + URI capturedUri() { + return captured.get(); + } + + String capturedSigningName() { + return capturedSigningName.get(); + } + } + + private static SdkHttpClient failingHttpClient() { + return new SdkHttpClient() { + @Override + public ExecutableHttpRequest prepareRequest(HttpExecuteRequest request) { + return new ExecutableHttpRequest() { + @Override + public HttpExecuteResponse call() { + throw new RuntimeException("stop after endpoint resolution"); + } + + @Override + public void abort() { + // no-op + } + }; + } + + @Override + public void close() { + // no-op + } + }; + } + + private static void issueNoNetworkRequest(S3Client client, String bucket) { + try { + client.headBucket(HeadBucketRequest.builder().bucket(bucket).build()); + fail("Expected to fail due to no-network http client"); + } catch (RuntimeException e) { + // expected + } + } + + // ---- SDK contract tests ---- + + public void testSdkResolvesOutpostsEndpointFromBucketArn() { + // Shape matters more than reality for endpoint logic; account/outpost IDs can be dummy. + final String bucketArn = "arn:aws:s3-outposts:us-east-1:111122223333:outpost/op-0123456789abcdef/accesspoint/my-ap"; + + final CapturingInterceptor interceptor = new CapturingInterceptor(); + + try ( + S3Client client = S3Client.builder() + .region(Region.US_EAST_1) + .credentialsProvider(DUMMY_CREDENTIALS) + .serviceConfiguration( + S3Configuration.builder() + // Ensure ARN region behavior is enabled; harmless for this test if already defaulted. + .useArnRegionEnabled(true) + .build() + ) + .overrideConfiguration(ClientOverrideConfiguration.builder().addExecutionInterceptor(interceptor).build()) + .httpClient(failingHttpClient()) + // IMPORTANT: do NOT set endpointOverride(...) in these SDK tests. + .build() + ) { + + issueNoNetworkRequest(client, bucketArn); + } + + final URI uri = interceptor.capturedUri(); + assertNotNull("SDK did not reach beforeTransmission; endpoint not captured", uri); + assertNotNull(uri.getHost()); + assertTrue("Expected outposts endpoint but was: " + uri, uri.getHost().contains("s3-outposts")); + + // Verify SDK uses correct signing service for Outposts + assertEquals("s3-outposts", interceptor.capturedSigningName()); + } + + public void testSdkResolvesAccessPointEndpointFromBucketArn() { + final String bucketArn = "arn:aws:s3:us-west-2:111122223333:accesspoint/my-ap"; + + final CapturingInterceptor interceptor = new CapturingInterceptor(); + + try ( + S3Client client = S3Client.builder() + .region(Region.US_WEST_2) + .credentialsProvider(DUMMY_CREDENTIALS) + .serviceConfiguration(S3Configuration.builder().useArnRegionEnabled(true).build()) + .overrideConfiguration(ClientOverrideConfiguration.builder().addExecutionInterceptor(interceptor).build()) + .httpClient(failingHttpClient()) + .build() + ) { + + issueNoNetworkRequest(client, bucketArn); + } + + final URI uri = interceptor.capturedUri(); + assertNotNull("SDK did not reach beforeTransmission; endpoint not captured", uri); + assertNotNull(uri.getHost()); + assertTrue("Expected accesspoint endpoint but was: " + uri, uri.getHost().contains("s3-accesspoint")); + + // Verify SDK uses correct signing service for access points + assertEquals("s3", interceptor.capturedSigningName()); + } + + public void testSdkResolvesRegularBucketToRegularS3Endpoint() { + final String bucket = "my-standard-bucket"; + + final CapturingInterceptor interceptor = new CapturingInterceptor(); + + try ( + S3Client client = S3Client.builder() + .region(Region.US_EAST_1) + .credentialsProvider(DUMMY_CREDENTIALS) + .overrideConfiguration(ClientOverrideConfiguration.builder().addExecutionInterceptor(interceptor).build()) + .httpClient(failingHttpClient()) + .build() + ) { + + issueNoNetworkRequest(client, bucket); + } + + final URI uri = interceptor.capturedUri(); + assertNotNull("SDK did not reach beforeTransmission; endpoint not captured", uri); + assertNotNull(uri.getHost()); + + final String host = uri.getHost(); + // Be tolerant across SDK versions/partitions: just assert it isn't ARN-specific routing. + assertFalse("Unexpected accesspoint host for non-ARN bucket: " + uri, host.contains("s3-accesspoint")); + assertFalse("Unexpected outposts host for non-ARN bucket: " + uri, host.contains("s3-outposts")); + assertTrue("Expected some form of S3 host but was: " + uri, host.contains("s3")); + } +} diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java index 21ae11a96e82e..e61ce543a912f 100644 --- a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java +++ b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/S3ServiceTests.java @@ -36,7 +36,9 @@ import org.opensearch.common.settings.Settings; import org.opensearch.secure_sm.AccessController; +import java.net.URI; import java.util.Map; +import java.util.Optional; public class S3ServiceTests extends AbstractS3RepositoryTestCase { public void testCachedClientsAreReleased() { @@ -83,4 +85,47 @@ public void testCachedClientsWithCredentialsAreReleased() { final S3ClientSettings clientSettingsReloaded = s3Service.settings(metadata1); assertNotSame(clientSettings, clientSettingsReloaded); } + + public void testResolveEndpointOverrideAbsentWhenEndpointNotProvided() { + final S3Service s3Service = new S3Service(configPath()); + final Settings repoSettings = Settings.builder() + // region is required by S3Service.settings(...) in this test suite + .put("region", "us-east-1") + // intentionally omit "endpoint" + .build(); + final RepositoryMetadata metadata = new RepositoryMetadata("no-endpoint", "s3", repoSettings); + + final S3ClientSettings clientSettings = s3Service.settings(metadata); + final Optional override = s3Service.resolveEndpointOverride(clientSettings); + assertTrue("Expected no endpoint override when endpoint setting is absent", override.isEmpty()); + } + + public void testResolveEndpointOverrideAddsSchemeWhenMissing() { + final S3Service s3Service = new S3Service(configPath()); + final Settings repoSettings = Settings.builder() + .put("region", "us-east-1") + // no scheme on purpose + .put("endpoint", "s3.us-east-1.amazonaws.com") + .build(); + final RepositoryMetadata metadata = new RepositoryMetadata("endpoint-no-scheme", "s3", repoSettings); + + final S3ClientSettings clientSettings = s3Service.settings(metadata); + final Optional override = s3Service.resolveEndpointOverride(clientSettings); + assertTrue("Expected endpoint override to be present when endpoint setting is provided", override.isPresent()); + + // S3 client settings default protocol in tests should be https unless explicitly configured otherwise. + // If this ever changes in the future, update this assertion accordingly. + assertEquals("https://s3.us-east-1.amazonaws.com", override.get().toString()); + } + + public void testResolveEndpointOverridePreservesExplicitScheme() { + final S3Service s3Service = new S3Service(configPath()); + final Settings repoSettings = Settings.builder().put("region", "us-east-1").put("endpoint", "http://localhost:9000").build(); + final RepositoryMetadata metadata = new RepositoryMetadata("endpoint-with-scheme", "s3", repoSettings); + + final S3ClientSettings clientSettings = s3Service.settings(metadata); + final Optional override = s3Service.resolveEndpointOverride(clientSettings); + assertTrue("Expected endpoint override to be present when endpoint has explicit scheme", override.isPresent()); + assertEquals("http://localhost:9000", override.get().toString()); + } }