-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add forcePathStyle and enableAccelerate config properties back to the S3 client #1099
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"id": "256a9c00-f658-4ee8-9e02-2cf46b76a900", | ||
"type": "bugfix", | ||
"description": "Add `forcePathStyle` and `enableAccelerate` config properties back to the S3 client.", | ||
"issues": [ | ||
"awslabs/aws-sdk-kotlin#1098" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
package aws.sdk.kotlin.services.s3.internal | ||
|
||
import aws.smithy.kotlin.runtime.config.* | ||
|
||
/** | ||
* S3 specific system settings | ||
*/ | ||
internal object S3Setting { | ||
/** | ||
* Configure whether the S3 client should uses the access point ARN AWS region to construct the regional endpoint | ||
* for the request. See [Amazon S3 access points](https://docs.aws.amazon.com/sdkref/latest/guide/feature-s3-access-point.html) | ||
*/ | ||
public val UseArnRegion: EnvironmentSetting<Boolean> = boolEnvSetting("aws.s3UseArnRegion", "AWS_S3_USE_ARN_REGION") | ||
|
||
/** | ||
* Configure whether the S3 client potentially attempts cross-Region requests. | ||
* See [Amazon S3 Multi-Region Access Points](https://docs.aws.amazon.com/sdkref/latest/guide/feature-s3-mrap.html) | ||
*/ | ||
public val DisableMultiRegionAccessPoints: EnvironmentSetting<Boolean> = boolEnvSetting("aws.s3DisableMultiRegionAccessPoints", "AWS_S3_DISABLE_MULTIREGION_ACCESS_POINTS") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
/* | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
package aws.sdk.kotlin.services.s3.internal | ||
|
||
import aws.sdk.kotlin.runtime.config.profile.loadAwsSharedConfig | ||
import aws.sdk.kotlin.services.s3.S3Client | ||
import aws.smithy.kotlin.runtime.util.TestPlatformProvider | ||
import aws.smithy.kotlin.runtime.util.asyncLazy | ||
import kotlinx.coroutines.test.runTest | ||
import kotlin.test.Test | ||
import kotlin.test.assertEquals | ||
|
||
class FinalizeConfigTest { | ||
@Test | ||
fun testExplicitConfigTakesPrecedence() = runTest { | ||
val builder = S3Client.builder() | ||
builder.config.useArnRegion = false | ||
builder.config.disableMrap = true | ||
val platform = TestPlatformProvider( | ||
fs = mapOf( | ||
"/users/test/.aws/config" to "[default]\ns3_use_arn_region = true\ns3_disable_multiregion_access_points = false", | ||
), | ||
) | ||
val sharedConfig = asyncLazy { loadAwsSharedConfig(platform) } | ||
|
||
finalizeS3Config(builder, sharedConfig, platform) | ||
assertEquals(false, builder.config.useArnRegion) | ||
assertEquals(true, builder.config.disableMrap) | ||
} | ||
|
||
@Test | ||
fun testSystemProperties() = runTest { | ||
val builder = S3Client.builder() | ||
val platform = TestPlatformProvider( | ||
env = mapOf( | ||
S3Setting.UseArnRegion.envVar to "false", | ||
S3Setting.DisableMultiRegionAccessPoints.envVar to "true", | ||
), | ||
props = mapOf( | ||
S3Setting.UseArnRegion.sysProp to "true", | ||
S3Setting.DisableMultiRegionAccessPoints.sysProp to "false", | ||
), | ||
fs = mapOf( | ||
"/users/test/.aws/config" to "[default]\ns3_use_arn_region = false\ns3_disable_multiregion_access_points = true", | ||
), | ||
) | ||
val sharedConfig = asyncLazy { loadAwsSharedConfig(platform) } | ||
|
||
finalizeS3Config(builder, sharedConfig, platform) | ||
assertEquals(true, builder.config.useArnRegion) | ||
assertEquals(false, builder.config.disableMrap) | ||
} | ||
|
||
@Test | ||
fun testEnvironmentVariables() = runTest { | ||
val builder = S3Client.builder() | ||
val platform = TestPlatformProvider( | ||
env = mapOf( | ||
S3Setting.UseArnRegion.envVar to "false", | ||
S3Setting.DisableMultiRegionAccessPoints.envVar to "true", | ||
), | ||
fs = mapOf( | ||
"/users/test/.aws/config" to "[default]\ns3_use_arn_region = true\ns3_disable_multiregion_access_points = false", | ||
), | ||
) | ||
val sharedConfig = asyncLazy { loadAwsSharedConfig(platform) } | ||
|
||
finalizeS3Config(builder, sharedConfig, platform) | ||
assertEquals(false, builder.config.useArnRegion) | ||
assertEquals(true, builder.config.disableMrap) | ||
} | ||
|
||
@Test | ||
fun testProfile() = runTest { | ||
val builder = S3Client.builder() | ||
val platform = TestPlatformProvider( | ||
fs = mapOf( | ||
"/users/test/.aws/config" to "[default]\ns3_use_arn_region = true\ns3_disable_multiregion_access_points = false", | ||
), | ||
) | ||
val sharedConfig = asyncLazy { loadAwsSharedConfig(platform) } | ||
|
||
finalizeS3Config(builder, sharedConfig, platform) | ||
assertEquals(true, builder.config.useArnRegion) | ||
assertEquals(false, builder.config.disableMrap) | ||
} | ||
|
||
@Test | ||
fun testConfigPropertiesPresent() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this testing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That these properties don't accidentally disappear again from codegen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not immediately clear from the code. Can we leave a comment indicating why we have a test with no assertions? |
||
val builder = S3Client.builder() | ||
builder.config.disableMrap = true | ||
builder.config.useArnRegion = true | ||
builder.config.forcePathStyle = true | ||
builder.config.enableAccelerate = true | ||
builder.config.useDualStack = true | ||
builder.config.useFips = true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this still true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We select auth scheme based on the model not on endpoint rules (yet) and we don't support sigv4a yet no matter what. Whether it's enabled or not isn't going to matter I don't think so I just changed it to the "correct" default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still causes changes to how our endpoints are resolved. I think we should leave this disabled until we support MRAP in the default case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a hill worth dying on, reverting.