apply path-style-access-enabled config to CRT client #872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisrhut
requested review from
maciejwalkowiak,
MatejNedic and
tomazfernandes
as code owners
chrisrhut
commented
Aug 23, 2023
| contextRunner.withPropertyValues().run(context -> { | ||
| S3AsyncClient client = context.getBean(S3AsyncClient.class); | ||
| S3AsyncClient delegate = (S3AsyncClient) ReflectionTestUtils.getField(client, "delegate"); | ||
| SdkClientConfiguration clientConfiguration = (SdkClientConfiguration) ReflectionTestUtils.getField(delegate, "clientConfiguration"); |
There was a problem hiding this comment.
If anybody can think of a way to accomplish this without the double reflection field access, I would love to do this more eloquently!
chrisrhut
force-pushed
the
crt-path-style-access
branch
from
7820662
to
baec512
Compare
| @@ -66,6 +66,7 @@ S3AsyncClient s3AsyncClient(AwsCredentialsProvider credentialsProvider) { | |||
| .region(this.awsClientBuilderConfigurer.resolveRegion(this.properties)); | |||
| Optional.ofNullable(this.awsProperties.getEndpoint()).ifPresent(builder::endpointOverride); | |||
| Optional.ofNullable(this.properties.getEndpoint()).ifPresent(builder::endpointOverride); | |||
| Optional.ofNullable(this.properties.getPathStyleAccessEnabled()).ifPresent(builder::forcePathStyle); | |||
There was a problem hiding this comment.
Is is actually the same? "forcePathStyle" and "pathStyleAccessEnabled"?
There was a problem hiding this comment.
There is a lot of code like this in the SDK codebase that heavily implies they are the same. I am definitely willing to be proven wrong but I couldn't find any other mapping for the option that does the same thing across the the SDK.
private AttributeMap createClientContextParams() {
AttributeMap.Builder params = AttributeMap.builder();
params.put(S3ClientContextParams.USE_ARN_REGION, s3Configuration.useArnRegionEnabled());
params.put(S3ClientContextParams.DISABLE_MULTI_REGION_ACCESS_POINTS,
!s3Configuration.multiRegionEnabled());
params.put(S3ClientContextParams.FORCE_PATH_STYLE, s3Configuration.pathStyleAccessEnabled());
params.put(S3ClientContextParams.ACCELERATE, s3Configuration.accelerateModeEnabled());
return params.build();
}
maciejwalkowiak
added
the
status: waiting-for-feedback
maciejwalkowiak
removed
the
status: waiting-for-feedback
maciejwalkowiak
added a commit
that referenced
this pull request
Dec 10, 2023
Co-authored-by: Maciej Walkowiak <walkowiak.maciej@yahoo.com>
maciejwalkowiak
added a commit
that referenced
this pull request
Dec 10, 2023
Co-authored-by: Maciej Walkowiak <walkowiak.maciej@yahoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Apply the
path-style-access-enabledS3 config property in theS3CrtAsyncClientAutoConfiguration💡 Motivation and Context
The standard S3 configuration allows you to configure "path style access" via property config. AWS has added this to the CRT client configuration as of version
2.20.42per this thread. It would be great to have this option available in the auto configuration for the CRT client.💚 How did you test it?
Reflection-driven unit test and empirical localhost testing.
📝 Checklist
🔮 Next steps