cloudflare / node-cloudflare

Node.js API for Client API
https://cloudflare.github.io/node-cloudflare/
Other
335 stars 92 forks source link

Specifying an HTTPS proxy for the API client #36

Closed buschtoens closed 6 years ago

buschtoens commented 6 years ago

I originally opened #33 to enable users to specify an HTTPS proxy that should be used by the API client, but the PR was closed down in https://github.com/cloudflare/node-cloudflare/pull/33#issuecomment-354005530:

I'm probably going to implement this in a different way, since got is an implementation detail (and worse: likely to be removed). Can you open an issue instead?

I personally don't care about any of the other options offered by got while using the Cloudflare API and don't think any other user would.

@terinjokes I'd be happy to open another PR that implements proxy support, like you would want it, since I'm interested in moving this forward quickly. :zap: :smile:

I think allowing users to pass an HTTPS agent offers the greatest flexibility to users, since some might need to tunnel through proprietary proxy protocols. The agent option is understood universally by all underlying HTTP libs, like got or request.

Usage example

Using caw
import Cloudflare from 'cloudflare':
import caw from 'caw';

const cf = new Cloudflare({
  email: 'you@example.com',
  key: '<your API key here>',
  agent: caw({ protocol: 'https' }) // Cloudflare API is HTTPS only
});
Using tunnel-agent directly
import Cloudflare from 'cloudflare':
import TunnelAgent from 'tunnel-agent';

const cf = new Cloudflare({
  email: 'you@example.com',
  key: '<your API key here>',
  agent: TunnelAgent.httpsOverHttp({ port: 80, host: 'proxy.example.com' })
});
buschtoens commented 6 years ago

We could even go a step further and read the HTTPS_PROXY and NO_PROXY environment variables ourselves and set up the proxy accordingly. This is what request does and I think it makes for a great user experience, since it just works. ™

In that case, we should also add a proxy option that behaves just like requests's one and would only take effect, if the agent option is not defined.

Usage example

import Cloudflare from 'cloudflare':

const cf = new Cloudflare({
  email: 'you@example.com',
  key: '<your API key here>',
  proxy: 'http://proxy.example.com'
});
buschtoens commented 6 years ago

@terinjokes Happy new year. :fireworks: Were you able to make up your mind yet?

terinjokes commented 6 years ago

Thanks for the great write up. My apologies in the delay in getting back to you.

Like you, I like software that just works, and I plan on supporting the HTTP_PROXY environment variable. I actually thought I had already supported it, but clearly I was mistaken.

I suspect most users won't be using them, but for reasons other than proxying, I do plan on accepting an option for agents as well. I'm working this weekend on refactoring the how I deal with global and call-specific options, so I'll likely get the environment variable support in first.

buschtoens commented 6 years ago

Awesome, thank you!

btw, I just discovered env-proxy-agent through https://github.com/nodejs/node/issues/15620#issuecomment-356163889. If you don't want to implement all the env logic yourself, this looks like a great utility.

terinjokes commented 6 years ago

Thanks for the links. While, I'm not yet sure which approach I'll take towards proxies yet, I'll try to have a branch up tomorrow that takes a stab.

Quick question, how does your proxy deal with HTTP/2? This module previously preferred HTTP/2, but I removed it ahead of native support landing in Node.js 8. I'd like to integrate it back in, but dunno how it plays with proxies like yours.

On Jan 8, 2018 8:39 PM, "Jan Buschtöns" notifications@github.com wrote:

Awesome, thank you!

btw, I just discovered env-proxy-agent https://github.com/crosscompile/env-proxy-agent through nodejs/node#15620 (comment) https://github.com/nodejs/node/issues/15620#issuecomment-356163889. If you don't want to implement all the env logic yourself, this looks like a great utility.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cloudflare/node-cloudflare/issues/36#issuecomment-356178180, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQsZS5DC54rb2DcGOgEOV7lQqPjC9cwks5tIu2bgaJpZM4ROQJD .

buschtoens commented 6 years ago

Hmmm, good question. The Node 8 HTTP/2 module has no notion of agents. This would require some more trickery.

It also depends on whether the proxy itself runs on HTTP/2 or /1.

A minimal and probably not really useful example for an HTTP/2 proxy taken from the Node docs.

const http2 = require('http2');

const client = http2.connect('http://localhost:8001');

// Must not specify the ':path' and ':scheme' headers
// for CONNECT requests or an error will be thrown.
const req = client.request({
  ':method': 'CONNECT',
  ':authority': `localhost:${port}`
});

req.on('response', (headers) => {
  console.log(headers[http2.constants.HTTP2_HEADER_STATUS]);
});
let data = '';
req.setEncoding('utf8');
req.on('data', (chunk) => data += chunk);
req.on('end', () => {
  console.log(`The server says: ${data}`);
  client.destroy();
});
req.end('Jane');

Since most users probably won't use an HTTP/2 proxy, but a regular HTTP/1 one, we need to work with a custom createConnection callback. The following code isn't tested, but something along these lines should work:


import http2 from 'http2';
import net from 'net';

const proxy = 'localhost:3128';

const client = http2.connect('https://api.cloudflare.com', {
  createConnection(url, options) {
    const socket = net.connect(proxy);
    socket.write(`CONNECT ${url.hostname}:${url.port} HTTP/1.1\r\n`); // maybe HTTP/2 ?
    // do some buffering here
    return socket;
  }
});
terinjokes commented 6 years ago

@buschtoens I mean what does Chrome or Firefox do? Do they avoid connecting over HTTP2 when the system has an HTTP/S proxy?

terinjokes commented 6 years ago

@buschtoens Can you try the patches/http-proxy branch? This should automatically configure an agent based on the HTTPS_PROXY and NO_PROXY environment variables.

terinjokes commented 6 years ago

@buschtoens Any luck?

buschtoens commented 6 years ago

I was caught up in meetings at work today. Sorry. I'll check this our first thing in the morning, when I'm behind the corporate proxy again.

I left two comments on your commit, but other than that, LGTM.

buschtoens commented 6 years ago

Just gave your branch a go. Works perfectly. :rainbow:

terinjokes commented 6 years ago

Great. I'll get it tagged tomorrow.

On Jan 10, 2018 11:22 PM, "Jan Buschtöns" notifications@github.com wrote:

Just gave your branch a go. Works perfectly. 🌈

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cloudflare/node-cloudflare/issues/36#issuecomment-356847952, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQsZflLVdyX2zOkIOExIXYCsrpT32i1ks5tJbargaJpZM4ROQJD .

terinjokes commented 6 years ago

Released in v.2.4.0