search

zio / zio-aws

Low level ZIO interface for the full AWS
https://zio.dev/zio-aws
Apache License 2.0
139 stars 31 forks source link

Endpoint override type is not strict enough #626

Open KineticCookie opened 2 years ago

KineticCookie commented 2 years ago

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 commented 2 years ago

Thanks, sounds like a good idea!

etherline commented 2 years ago

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 commented 2 years ago

@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 šŸ¤”