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

Fix following 301 redirects for https upgrades #84

Merged
merged 2 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 1 addition & 10 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,13 @@ on:

jobs:
test:
services:
presto:
image: ghcr.io/popsql/prestodb-sandbox:0.284
ports:
- "18080:8080"
trino:
image: trinodb/trino:418
ports:
- "18081:8080"

runs-on: ubuntu-latest
strategy:
matrix:
node-version: [ 16.x, 18.x, 20.x ]
steps:
- uses: actions/checkout@v3
- run: docker compose up --detach --wait
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does the same thing as using the services key, where --wait gets the command to wait until the containers are marked as healthy before continuing, similar to what services was doing.

This simplifies the test setup to better match local development, and that there was no way to mount a volume from the local repository (as do nginx configuration) via the services setup.

- uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}
Expand Down
11 changes: 11 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,14 @@ services:
ports:
- "18081:8080"

nginx:
image: nginx:1.25-alpine
depends_on:
- presto
ports:
- "80:80"
- "443:443"
volumes:
- ./nginx/nginx.conf:/etc/nginx/nginx.conf:ro
- ./nginx/ssl/nginx-selfsigned.crt:/etc/nginx/ssl/nginx-selfsigned.crt:ro
- ./nginx/ssl/nginx-selfsigned.key:/etc/nginx/ssl/nginx-selfsigned.key:ro
32 changes: 32 additions & 0 deletions index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,4 +418,36 @@ describe('redirect tests', function(){
},
});
});

describe('when using reverse proxy that does not forward protocol', function(){
// The first request will go to https://localhost:443/v1/statement, but the
// nextUri will respond with http://localhost/v1/statement/... where then
// the nginx proxy will 301 redirect us to https again.
const client = new Client({
host: 'localhost',
port: 443,
catalog: 'tpch',
schema: 'tiny',
ssl: {
rejectUnauthorized: false,
}
});

test('that querying works', function(done){
expect.assertions(5);
client.execute({
query: 'SELECT 1 AS col',
data: function(error, data, columns){
expect(error).toBeNull();
expect(data).toEqual([[1]]);
expect(columns).toHaveLength(1);
expect(columns[0]).toEqual(expect.objectContaining({ name: 'col', type: 'integer' }));
},
callback: function(error){
expect(error).toBeNull();
done();
},
});
}, 10000);
});
});
33 changes: 18 additions & 15 deletions lib/presto-client/index.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
const { URL } = require('url') ;
const http = require('follow-redirects/http');
const https = require('follow-redirects/https');

var adapterFor = (function() {
var adapters = {
'http:': require('follow-redirects/http'),
'https:': require('follow-redirects/https'),
};

return function(protocol) {
return adapters[protocol];
};
}());
var adapters = {
'http:': http,
'https:': https,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed the the "raw" http and https modules to do the agent assignment below, and so then doing:

const http = require('follow-redirects/http');
const https = require('follow-redirects/https');

var adapterFor = function(protocol) {
  return adapters[protocol];
}

and it felt like it'd be easier to just not have the adapterFor function at all and just index the adapters object directly. Let me know if you'd like me to bring back the helper function.


var PrestoHeaders = require('./headers').Headers;
var TrinoHeaders = require('./headers').TrinoHeaders;
Expand Down Expand Up @@ -70,12 +66,12 @@ var Client = exports.Client = function(args){

Client.prototype.request = function(opts, callback) {
var client = this;
var adapter = adapterFor(client.protocol);
var contentBody = null;

if (opts instanceof Object) {
opts.host = client.host;
opts.port = client.port;
opts.protocol = client.protocol;
if (! opts.user)
opts.user = client.user
if (! opts.headers)
Expand All @@ -90,14 +86,13 @@ Client.prototype.request = function(opts, callback) {
port: href.port || (href.protocol === 'https:' ? '443' : '80'),
path: href.pathname + href.search,
headers: {},
protocol: href.protocol,
};
} catch (error) {
return callback(error);
}
}
opts.protocol = client.protocol;

opts = Object.assign({}, opts, client.ssl);
var adapter = adapters[opts.protocol];

if (opts.user)
opts.headers[client.headers.USER] = opts.user;
Expand All @@ -113,7 +108,15 @@ Client.prototype.request = function(opts, callback) {
if (opts.body)
contentBody = opts.body;

opts.agent = new adapter.Agent(opts); // Otherwise SSL params are ignored.
// `agents` is a custom property that follow-redirects supports, where
// it's similar to setting `agent`, but allows for setting it per protocol,
// so that if we redirect from http -> https (or vice versa) we use the right
// agent, instead of trying to reuse http.Agent for https (or vice versa) which
// will error.
opts.agents = {
http: new http.Agent({ keepAlive: false }),
https: new https.Agent({ ...client.ssl, keepAlive: false }),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to not pass in the opts object as is into the agent, as I don't think most users would need to have host, port, etc. set explicitly on the agent, vs letting that get picked up automatically from the request object. If wanted to do this, then would need to override the port value of opts if opts.protocol === 'http:', which just seemed overly complicated for little benefit. Additionally, passing the client.ssl object into the http.Agent was probably a no-op in the common case as the values there were ignored, so I think better to pass it only into the https.Agent.

Note, when passing no agent, the default behavior was changed in Node 20 such that keepAlive: true is used, so I still pass in an agent to explicitly turn that off to maintain BC, as well as since these are all one off requests, there's no point to having keepAlive: true.

Finally, as a note, doing a spread of undefined returns empty object essentially, so if client.ssl === undefined, then doing https.Agent({ ...client.ssl, keepAlive: false }) === https.Agent({ keepAlive: false }) essentially.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This opts.agents attribute is obviously different from other opts attributes because others are to specify http/https request parameters, but this one is to control the follow-redirects-specific behaviors.
So I want a comment here about what this controls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment in bcd7789, let me know if you'd like it tweaked.

};

var parser = this.jsonParser;

Expand Down
31 changes: 31 additions & 0 deletions nginx/nginx.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
events {
worker_connections 1024;
}

http {
server {
listen 80;
server_name _;
return 301 https://localhost:443$request_uri;
}

server {
listen 443 ssl;
server_name _;

ssl_certificate /etc/nginx/ssl/nginx-selfsigned.crt;
ssl_certificate_key /etc/nginx/ssl/nginx-selfsigned.key;

location / {
proxy_pass http://presto:8080/;
proxy_connect_timeout 3;

proxy_set_header Host $http_host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
# don't forward the protocol, so that presto will default to responding with
# http:// URLs, to test the 301 redirect logic.
# proxy_set_header X-Forwarded-Proto $scheme;
}
}
}
21 changes: 21 additions & 0 deletions nginx/ssl/nginx-selfsigned.crt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
-----BEGIN CERTIFICATE-----
MIIDbTCCAlWgAwIBAgIUeYV6hie38aYPRLqSqkgUdB1efQwwDQYJKoZIhvcNAQEL
BQAwRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM
GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAgFw0yMzExMjExNzM2MDlaGA8zMDIz
MDMyNDE3MzYwOVowRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUx
ITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBAJbNHsmo/jSyNKdSaXlIn9ZCgjTN2wp8yH5Kcwx4
RHwx8ZVPQIVz9aSnmmjwKZfpLtvNU7jrgBH8h4qxTw/bbETluTeH9asA71cliZyz
qZUVEXeQqXRlzDDafElWRyEQT+X9vTp7ApKfFRC6p64sZFeXgQtNkhfxWLrVuxTo
WoebROjHXo78M27i76InaorNDDgYoHByEeWqPcOrATT9v3e1UbbD8zrL28Qh23+R
hddefeB7CbwyIeGOEh6jde0lHEVkrv1u6jbFuvpalrtETyPKcT5wi056YjKgbA4m
2EULLwKhwki3/QMYEjLO/h9vnAiiWvemC+8U6aZGyGvS/00CAwEAAaNTMFEwHQYD
VR0OBBYEFOUr3sfsSTx293Ab72TMajKL1+xPMB8GA1UdIwQYMBaAFOUr3sfsSTx2
93Ab72TMajKL1+xPMA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEB
AA8we9GzV6j6jdvAyfnkRjdVB//L5VQzIbpCHQC8sZnLnJt0mzhdIcl+KnIq8v9x
AlUHJxMme2Xb22z15SBJWm2blg4M7izzKfsI27DjmnEFAaRSBFoLyqlyW9MJhS42
pWT8ltVzTnPwN/VMphrBOCZ7i5tNdzy4NcExvPuMVx3qrB5H0+Hoofjqo+pAe01+
yG6pdQyBOf4/MiePl5QR3E6vriMU0ddRaVnGot3wPw9sIWxjSVTGNTXSvo/PVyO/
XV1IaXaw8srYgH78pYkAa27QUIsx6m9EIdyE8yK3nXQHswYdNxP720p5GINZYKM9
37hfkpczTTBdK6jdHpFexYE=
-----END CERTIFICATE-----
28 changes: 28 additions & 0 deletions nginx/ssl/nginx-selfsigned.key
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCWzR7JqP40sjSn
Uml5SJ/WQoI0zdsKfMh+SnMMeER8MfGVT0CFc/Wkp5po8CmX6S7bzVO464AR/IeK
sU8P22xE5bk3h/WrAO9XJYmcs6mVFRF3kKl0Zcww2nxJVkchEE/l/b06ewKSnxUQ
uqeuLGRXl4ELTZIX8Vi61bsU6FqHm0Tox16O/DNu4u+iJ2qKzQw4GKBwchHlqj3D
qwE0/b93tVG2w/M6y9vEIdt/kYXXXn3gewm8MiHhjhIeo3XtJRxFZK79buo2xbr6
Wpa7RE8jynE+cItOemIyoGwOJthFCy8CocJIt/0DGBIyzv4fb5wIolr3pgvvFOmm
Rshr0v9NAgMBAAECggEAEaO4wDoGUj+uLQxUzh0SpUtuU6LoxldGVI982a6PxD9L
VP3KPFIOH86DH1dIZj6efpOMAYt6laAGctC+wMoX5g9BFR/QOsqHNJhtemkBozCt
tGC1kan+spA8DZAMDfAMiIifw+FzsZbuLeDHkHYc3qoYLCxMtIRErsYldhKf1FDF
S1eQ2Oam3deHDRQy/NTwwTrC322CLgB5XO63Ustdc30rfulpX1WvxUz9udmHxZZV
32gbUv54hr6/AzN6BLPUUvzO5VZeizssM9RR97oBH/fALbiOnCCf7UyEB19wX4ev
dmQLXr/KpRvYAzKYg7UAmt2Ut4Cm0V+TqQgvNivxQQKBgQDIHbObobdNyuEi4aie
tSJX5pEG30+zHdtTA0XhkXG+7l223xVM+PTfPSRxMgGIef14hfA1yS1JslNf5vpb
l/HJ7Bz65I5ENwi9LjxILSjDpJ2R95eoBFa0AqzdpkUkDHWb3qWhvsE624JXXHlU
6t2Lk3Rq/b8cny8DMEFwBJHFxQKBgQDA6eebo8B2ny9i8FdBWQrCiepPSrn+OIZ5
B9shUy1r51wL+rdBctO+YZAdd91Gy3FV16M/gjhatxnpQLo6DSQPRBICb2SgVdaG
AyJ0WxUJulIRhO+VubObjk8DFsBikr8mm1osVX8G6qhTjC3dW2QJYfAsDLDTU7Ea
VdWERXzz6QKBgQCT/js85pzcQCS9misMrCJo5U/tyCp16aewvaPpjJmVPU07F8H1
+cuGaP5RZEzz9Fu6zTr3W/9NGD9Gllgicr1SunY4Kdz4n8aruczFB/i0r0IEmBml
HQhN+giDpxpM7ZXwnvjZJGxcrce3+eCVJ1iOh486LMwwS50F+6L5R7fSjQKBgBj/
9vjPfsiglnZ+6P/Z9zAAyXGfIH1We+7BWt91tQQvxljzE76Sj+gzIob/GpjrEnPq
bwhy6rSu19fHgJq3Sz4DN0ZDg0nX9eYGD/f+Obq5/5qvnJDNsP3uskSXNTVRCR6K
sWPfbIfL7yZsmyD9j/g7TfGXb27jgKcGCpaKnsfJAoGAATLeEAD8rHMyxmBmYdcO
GUxLaqOtODEje/8zOn5hUFbISUEk4MKw0kT3XHtIFqhxIFJrV0Au4urMNuauneur
OEyVN7Y9RreGgzt0rPvpFvyv2kTBbhihd0lJ86UrhUDsllNphpb43UPurZM/kp0h
djFSBC+GgWwuWFqsRntRhFg=
-----END PRIVATE KEY-----