Skip to content
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

Endpoint override type is not strict enough #626

Open
KineticCookie opened this issue Dec 8, 2021 · 3 comments
Open

Endpoint override type is not strict enough #626

KineticCookie opened this issue Dec 8, 2021 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@KineticCookie
Copy link

Hi!
I got a strange exception trying to connect s3 client to local minio.

software.amazon.awssdk.core.exception.SdkClientException: Unable to marshall request to JSON: baseUri must not be null.
        at software.amazon.awssdk.core.exception.SdkClientException$BuilderImpl.build(SdkClientException.java:98) ~[software.amazon.awssdk.sdk-core-2.17.61.jar:?]
        at software.amazon.awssdk.services.s3.transform.HeadObjectRequestMarshaller.marshall(HeadObjectRequestMarshaller.java:53) ~[software.amazon.awssdk.s3-2.17.61.jar:?]
        ...
        at io.github.vigoo.zioaws.s3.package$S3Impl.$anonfun$headObject$1(package.scala:2392) ~[io.github.vigoo.zio-aws-s3_2.13-3.17.61.1.jar:3.17.61.1]

Caused by: java.lang.NullPointerException: baseUri must not be null.
        at software.amazon.awssdk.utils.Validate.paramNotNull(Validate.java:156) ~[software.amazon.awssdk.utils-2.17.61.jar:?]
        ...

After some experiments, I found that the type of endpointOverride defined here: https://github.com/vigoo/zio-aws/blob/master/zio-aws-core/src/main/scala/io/github/vigoo/zioaws/core/config/package.scala#L237-L239 allows for incorrect URIs to be passed to underlying aws sdk.

In my case it was minio:9000 instead of correct http://minio:9000

In my semi-educated guess, it would be better to change endpointOverride descriptor to url to enforce type safety.

@vigoo vigoo added enhancement New feature or request good first issue Good for newcomers labels Dec 30, 2021
@vigoo
Copy link
Collaborator

vigoo commented Dec 30, 2021

Thanks, sounds like a good idea!

@etherline
Copy link

etherline commented May 2, 2022

Are there other use cases?

What if you are accessing another service deployed on your Kubernetes cluster that acts as a proxy to an AWS service? Would you not want to retain the ability to use a URI?

The Java SdkClientBuilder interface expects a URI as well: B endpointOverride(URI endpointOverride);. Easy enough to widen it again in AwsConfig.configured() before invoking, but it does make you wonder about the intent.

[https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/](k8s dns-pod-service)

@KineticCookie
Copy link
Author

@etherline
I'm not sure if I can recall the details exactly 😅

I was under impression that java SDK indeed expects URI. But there is a code that parses the URI, and if it can't convert it to URL, the it throws an exception as displayed above.

After filling out this issue I did a little more digging and found out that java URL is badly designed (because of DNS queries) and that's why people prefer to use URIs instead.
But the problem here (as I see it) is that this specific java design choice led to URI usage, where developer expects it to be URL.

Maybe the issues is deeper that I anticipated 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants