search

tagomoris / presto-client-node

Distributed query engine Presto client library for node.js
MIT License
126 stars 57 forks source link

nextUri can return 301 redirect due to https/http mismatch #80

Closed MasterOdin closed 12 months ago

MasterOdin commented 12 months ago

When trino/presto is behind a load-balancer, it can be setup such that one connects to the load balancer via HTTPS to connect to trino/presto, but that the cluster itself was not setup with https, nor was the http-server.process-forward setting enabled. In this case, the HTTPS call to /v1/statement via HTTPS will end up returning a nextUri that is http, which the load balancer will respond with a 301 to the appropriate https endpoint. However, the client cannot handle this, and will end up in an error state. I believe that the client should follow the redirects, as it seems like other clients (e.g. dbeaver) do.

To handle this, I would propose using the follow-redirects package which is a drop-in replacement for the builtin http and https modules that are already used, just that it'll silently handle redirects. This would potentially break usage for anyone using this within a web context, but not sure that's an issue. Could also instead use node-fetch which similarly supports this, but would require a larger rewrite of how requests are done.

Let me know if such a change would be accepted and which library you'd prefer @tagomoris.

tagomoris commented 12 months ago

It sounds acceptable. The current implementation does not take any care of the 3xx status code, so we can expect that it doesn't break anything. I don't have any preference for the library around it.