Skip to content

Add support for forcePathStyle to S3CrtAsyncClientBuilder #3817

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

Closed
almogtavor opened this issue Mar 7, 2023 · 22 comments
Closed

Add support for forcePathStyle to S3CrtAsyncClientBuilder #3817

almogtavor opened this issue Mar 7, 2023 · 22 comments
Labels
crt-client feature-request A feature should be added or improved.

Comments

@almogtavor
Copy link

Describe the bug

As said in #3813, I also agree that the S3CrtAsyncClientBuilder is missing a lot of functions available for S3AsyncClientBuilder. But for me, the case causes a pretty large bug, since I cannot connect to the implementation of the S3 I use (Ceph), while I could do that with the S3AsyncClientBuilder using forcePathStyle.

Expected Behavior

I'd expect the AWS CRT SDK to be able to connect to the S3 implementation I use as usual, but in practice, it fails to do so and I get the error of: software.amazon.awssdk.core.exception.SdkClientException: software.amazon.awssdk.crt.s3.CrtS3RuntimeException: Host name was invalid for dns resolution. Enabling the forcePathStyle option would probably make this work.

Current Behavior

I cannot migrate to the CRT implementation even though I really want to since I need the multipart uploading logic and I don't want to implement this all by myself just because I'm not able to connect to S3 using the crt client.

Reproduction Steps

Cannot be provided since this error getting thrown only when I connect to the S3 implementation of Ceph.

Possible Solution

Adding the forcePathStyle

Additional Information/Context

No response

AWS Java SDK version used

2.20.17

JDK version used

17.0.2

Operating System and version

Windows 10

@almogtavor almogtavor added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2023
@debora-ito
Copy link
Member

@almogtavor thank you for reaching out, this is in our backlog.

Changing to a feature request, because it's a feature that is missing.

@debora-ito debora-ito added feature-request A feature should be added or improved. crt-client and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 7, 2023
@almogtavor
Copy link
Author

@debora-ito Thanks. Very appreciated. Can I track the backlog somewhere? I haven't seen this task on the GitHub Projects tab

@debora-ito
Copy link
Member

We don't have a public backlog unfortunately.

@almogtavor
Copy link
Author

@debora-ito Thanks. Isn't there a workaround for now, until the implementation be ready? We cannot use the crt client at all right now...

@akolacca
Copy link

@zoewangg Any progress here? I really need this

@debora-ito
Copy link
Member

@akolacca can you tell us more about your use case and why do you need forcePathStyle? Are you also using Ceph?

@akolacca
Copy link

@debora-ito yes. and I cannot at all while using the crt client. Its not like a feature request, the whole crt client is not usable to me. For me to use the crt client I have to wait for this feature. No workarounds. Right now I implement the range fetching and the multipart upload myself and I got bugs, so I would really love to see this one gets implemented

@akolacca
Copy link

@debora-ito So this is by far the most important feature I ever needed and that I will ever need from this SDK

@akolacca
Copy link

@debora-ito @zoewangg I think this is pretty related to the PR of #3824. Do you think it will be hard to implement or will this probably can get released at the next couple of weeks?

@fatih-celonis
Copy link

It's also important for us to use the SDK for local development as we're using Minio for both local development and integration test with testcontainers.

@akolacca
Copy link

@debora-ito when will this get supported approximately? We really wait for this...

@akolacca
Copy link

@zoewangg @debora-ito Sorry for all of the interruptions, but we cannot really progress at using the aws-sdk-java-v2 since I cannot connect at all to my S3.

@zoewangg
Copy link
Contributor

zoewangg commented Mar 28, 2023

Hi @akolacca, apologies for the delayed response. While we can't share a date, we have prioritized this task and are planning to work on this soon, in the next couple of weeks, if not earlier. Thank you for your patience. As a workaround, can you try with the standard Java s3 client that has forcePathStyle support? S3AsyncClient.builder().build();

@akolacca
Copy link

@akolacca Hi. I'm already using it, but I need to implement the fetch ranges logic and the multipart upload myself (pretty complicated. hard error handling and edge cases) because the S3AsyncClient.builder().build(); won't do it automatically.

@okrische
Copy link

Same here. pathStyleAccess needs to be implemented. Otherwise we can not test anything against our s3 test container.

Beside that, the TransferManager wants us to use the crtBuilder, as it otherwise lacks important features, like the automatic multipart handling, which saves us a lot of code:

The provided DefaultS3AsyncClient is not an instance of S3CrtAsyncClient, and thus multipart upload/download feature is not enabled and resumable file upload is not supported. To benefit from maximum throughput, consider using S3AsyncClient.crtBuilder().build() instead

@akolacca
Copy link

akolacca commented Apr 4, 2023

@okrische Couldn't agree more. @debora-ito @zoewangg @davidh44 what do you think? I would really love to see this happen

@zoewangg
Copy link
Contributor

zoewangg commented Apr 4, 2023

This is the PR to support this: #3880

@fatih-celonis
Copy link

Hey @zoewangg It is released as of 2.20.42 according to the changelog, right?

@zoewangg
Copy link
Contributor

Yes, correct, please try with the latest version (2.20.42 as of now).

Closing the issue, feel free to open a new issue if you have further questions.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@alicianewhagen
Copy link

Yes, correct, please try with the latest version (2.20.42 as of now).

Closing the issue, feel free to open a new issue if you have further questions.

I pulled the new jar 2.20.42 on Monday April 10 and the .forcePathStyle(true) now exists and it works. I can read, but I can't write with the S3TransferManager which uses the crt.Builder as the client even though it writes fine just using the client. The trace log says access denied, but the client itself has no problems, while would the s3transferManager have problems

@akolacca
Copy link

@zoewangg Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crt-client feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

7 participants