Skip to content

Enhance S3TransferManager S3(Native)ClientConfiguration to include additional CRT S3ClientOptions #2945

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

Open
1 task done
ron1 opened this issue Jan 3, 2022 · 7 comments
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue transfer-manager

Comments

@ron1
Copy link
Contributor

ron1 commented Jan 3, 2022

Describe the feature

The S3TransferManager components S3ClientConfiguration, S3NativeClientConfiguration, and others are enhanced to allow customization of the following software.amazon.awssdk.crt.s3.S3ClientOptions properties:

String endpoint;
TlsContext tlsContext;

Is your Feature Request related to a problem?

I am unable to upgrade the aws sdk used in my application from sdk v1 to sdk v2 because the sdk v2 S3TransferManager does not allow the S3 endpoint nor the SSLSocketFactory to be customized.

Proposed Solution

Enhance several S3TransferManager components to allow customization of the CRT S3ClientOptions specified above.

Describe alternatives you've considered

No response

Acknowledge

  • I may be able to implement this feature request

Would you accept a PR that implements this feature?

AWS Java SDK version used

2.17.102

JDK version used

OpenJDK Runtime Environment (build 1.8.0_302-b08)

Operating System and version

CentOS 7.9

@ron1 ron1 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jan 3, 2022
@debora-ito
Copy link
Member

debora-ito commented Jan 13, 2022

As @zoewangg mentioned in #2946, the V2 TransferManager is being refactored and the way to configure the S3 client will probably change - the plan is to allow to pass a S3Client to TransferManager, with all the client customizations you need.

Let's keep this open to track if the feature request is fulfilled after the refactor.

@debora-ito debora-ito added transfer-manager and removed needs-triage This issue or PR still needs to be triaged. labels Jan 13, 2022
@ron1
Copy link
Contributor Author

ron1 commented Mar 1, 2022

@debora-ito @zoewangg Now that v0.15.20 of the AWS CRT for Java has been released with the ability to Perform S3 get and put operations over HTTP as well as HTTPS, and with custom ports, would you reconsider accepting a S3TransferManager PR that adds just the endpoint (and not the TLSContext) as an additional S3ClientOption? The latest set of S3TransferManager changes made by @zoewangg should simplify the PR. Removal of the TLSContext from the PR should also reduce the scope of the PR.

We have an immediate need to use the S3TransferManager to perform get and put operations over HTTP with custom ports. Without this PR, we must copy, paste, tweak, and maintain more than six S3TransferManger classes since most of these classes are internal and therefore not reusable. Thanks for your consideration.

@zoewangg
Copy link
Contributor

zoewangg commented Mar 1, 2022

Hi @ron1, I think just exposing endpointOverride(URI endpointOverride); on the S3TransferManagerConfiguration is fine, but we still intend to get rid of the S3TransferManagerConfiguration in favor of S3AsyncClient in the near future, which means part of the code would be removed eventually before GA. If you are fine with it, we can merge the change.

@ron1
Copy link
Contributor Author

ron1 commented Mar 2, 2022

Hi @zoewangg. I am fine with an interim solution that includes throw-away code. I will go to work on a PR that exposes endpointOverride(URI endpointOverride); Thanks much.

@ron1
Copy link
Contributor Author

ron1 commented Mar 2, 2022

@zoewangg BTW, when you mentioned S3TransferManagerConfiguration in your comment, were you referring specifically to class S3ClientConfiguration, classS3TransferManagerOverrideConfiguration, or some other class?

@zoewangg
Copy link
Contributor

zoewangg commented Mar 3, 2022

Apologies for the delayed response, I meant to say S3ClientConfiguration .

@dvisigalli
Copy link

Hi @ron1, I think just exposing endpointOverride(URI endpointOverride); on the S3TransferManagerConfiguration is fine, but we still intend to get rid of the S3TransferManagerConfiguration in favor of S3AsyncClient in the near future, which means part of the code would be removed eventually before GA. If you are fine with it, we can merge the change.

@zoewangg with this approach will also be possible to set a specific proxy configuration to S3 Transfer Manager? Is there already a way to do this?
At the moment i'm using an AwsCrtAsyncHttpClient instance configured on a S3AsyncClient and it seems to work behind a proxy (i can connect to buckets and list the objects).
Unfortunately i still have issues with S3 Transfer Manager, which fails in downloading objects (AWS_ERROR_INVALID_ARGUMENT(34) when i override the endpoint, CRT error code: 1069 not overriding endpoint).
Could this be a caused by Transfer Manager that is simply not using the correct proxy?

@yasminetalby yasminetalby added the p2 This is a standard priority issue label Nov 28, 2022
aws-sdk-java-automation added a commit that referenced this issue Apr 5, 2024
…6cd466fca

Pull request: release <- staging/b7de7094-d69b-46d0-9321-6326cd466fca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue transfer-manager
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants