[Proposal] sails.config.http.createServerFn (originally re: Add Proxy Protocol support)

[fidian]

In order to pass websocket traffic through Amazon's elastic load balancers (ELBs), you must force the ELB to proxy TCP traffic instead of HTTP traffic. These proxies are application-level and do not forward network packets verbatim. In order to know the source IP, Amazon allows Proxy Protocol (as defined in HAProxy's spec) to be enabled on the ELBs, so the source and destination IP + port can be preserved.

As mentioned in #3505, this requires a change to the http hook (code mentioned in the issue) and the use of another module to unwrap the TCP traffic and pull off the first line of the request, which contains the Proxy Protocol information. Excerpts from that issue are included here.

I would suggest using a flag to the configuration object, such as proxyProtocol = true or http.proxyProtocol = true. When truthy, Proxy Protocol would be enabled and required. When falsy, Proxy Protocol support would be disabled.

Comments from the original issue:

It looks like one could easily add support with code in lib/hooks/http/initialize.js, perhaps roughly at line 50. I don't know if this follows the same coding method as you, so I present it here for your review instead of in a pull request.

if (sails.config.proxyProtocol == true) {
    var httpConnectionElement = usingSSL ? require('https') : require('http');
    var proxiedHttp = require('findhit-proxywrap').proxy(httpConnectionElement);
    createServer = proxiedHttp.createServer;
}

For this code to work, I was told that connectSailsClient() in lib/hooks/sockets/lib/initialize.js (about line 130) needed to be commented out, though I don't know the details and can not elaborate on why this is the case.

[sgress454]

Interesting, thanks for posting @fidian. There are a few good reasons not to add code that is this specific to the http hook, foremost of which is that it means we have another dependency (findhit-proxywrap) to keep on top of. But it seems like there is a case here for a configuration option (e.g. sails.config.http.createServerFn) which allows you to specify a function that creates the HTTP server for you.

[fidian]

That approach would work wonderfully for us. I agree that it is better than adding another dependency.

[mikermcneil]

@fidian Awesome! (and thanks for detailing your use case)

So to expand on @sgress454's suggestion, and take this a step closer to a full proposal, what about something like this:

sails.config.http.createServerFn

Usage

In config/http.js:

  // ...
  createServerFn: function (sails) {
    // This function receives the Sails app instance (`sails`) as the first argument.

    // Should return an HTTP server instance.
    // (note that this means this function must be synchronous)
    return require('http').createServer();
  },
  // ...

Property	Type	Default	Details
createServerFn	((function))	By default, Sails will obtain an HTTP server instance by calling the createServer method from either the [http](link to appropriate section in node docs here) or [https](link to appropriate section in node docs here) modules in Node core (depending on your app's [sails.config.ssl](link to appropriate section in sails docs) configuration), and calling that function with [sails.config.http.serverOptions](link to appropriate section in sails docs).	A custom createServer() function that Sails will call in order to obtain an HTTP server. Receives the app instance (sails) as an argument, and should return an HTTP server (e.g. like you get from calling require('http').createServer()

To implement:

Tests

• add a test that ensures that, if this config is provided as anything other than a function (!_.isFunction()), the http hook fails to load in the http hook throws an error
• add a test that ensures that, if this config is provided as a valid function, the http hook calls the provided function with the expected arguments. Also test that the provided function is called by the http hook in Sails core here, that it passes in the expected arguments, and that the value returned from the provided function is used as the http server when Sails lifts
• add a test that ensures that, if this config is provided as a valid function AND sails.config.http.serverOptions is provided, then an error is thrown explaining that either one or the other config should be used (but not both).

Docs

• update sails.config.http reference docs to include the new option on the v0.12 branch
• on the same reference page on the 0.12 branch, clarify that the serverOptions configuration is completely ignored if a custom createServerFn is provided.
• on the bottom of the same reference page, a small section called "Example createServerFn" that describes the use case for this configuration and shows a quick code example of how to use it.
• on the reference page that covers sails.config.ssl in sails-docs, clarify that while the ssl configuration normally impacts which createServer function is used, that does not happen if a custom createServerFn is provided.
• Probably more here, I couldn't take the time to do a thorough search right now. If anyone has time to help here, that'd be great.

http hook (core hook)

• in the configure function in the http hook, throw an error with a reasonable error message if the provided createServerFn config is !_.isUndefined(..) && !_.isFunction(). . Also throw an error w/ a reasonable message if a valid createServerFn config is provided, but serverOptions are also provided (see above). For reference when implementing both checks, here's an example of how the views hook does it: https://github.com/balderdashy/sails/blob/master/lib/hooks/views/configure.js#L42
• if sails.config.http.createServerFn is !_.isUndefined(), then call that custom function instead of require('http').createServer here.

Generators

• N/A (I think we can leave this out of the default generated configuration files in new Sails apps, and therefore also out of the anatomy docs)

[sgress454]

@mikermcneil this looks good! I don't think createServerFn, sails.config.http.ssl and sails.config.http.serverOptions need to be mutually exclusive, though. In @fidian's example above, the server could still be either http or https depending on the config settings. The ssl config wouldn't necessarily have any effect on the server that gets created if you use createServerFn; it just depends how you implement the function. For instance, I could see someone creating sails-hook-proxyServer where all it does is add a sails.config.http.createServerFn that looks like @fidian 's example. It would be wise for something like that to use the documented config properties!

[mikermcneil]

Also, to be clear: I'm not suggesting we make ssl and http.createServerFn mutually exclusive -- just that we specifically document that the ssl will not have the usual effect if you are using the http.createServerFn config. I am suggesting that http.serverOptions and http.createServerFn be mutually exclusive though, and that we enforce that programmatically.

It would be wise for something like that to use the documented config properties!

Perhaps, but as we've seen with generators, there's no way to enforce that it would-- and it usually doesn't :/ I'd rather be opinionated and specific about what the other config does rather than leaving room for interpretation (i.e. and consequently issues & incompatibilities). I think our main priority here needs to be "How do we provide a clean, flexibility abstraction to allow complete customizability in this area without adding any more weight to core in this area over the long term?"

[mfowlewebs]

I attempted to use Proxyquire today to install a Proxywrap'ed set of modules in place of HTTP & HTTPS, but ran into a Proxyquire issue. It should hypothetically be pretty simple:

module.exports = proxyquire("sails",  {
    "http": require("./proxywraphttp"),
    "https": require("./proxywraphttps")
})

https://github.com/mfowlewebs/sails-proxywrap/blob/master/sails-proxywrap.js#L4

I do have moderate belief that attacking this problem at a Node level (rather than via creating more API surface to expose) has merit, but I also would affirm that this is probably not an expected use for Proxyquire or something most people would think to go do.

I was going to try to compare the memory impact & load time of a vanilla sails & this proxyquired proxywrapped Sails as a first pass evaluation, but can't seem to get this proxyquire approach going: thought I'd post the attempt anyways (with source, for your reproducability).

[mfowlewebs]

Update: was doing really boneheaded things when trying to use proxyquire above. So good news! Using Proxyquire lets one monkeypatch in proxywrap-ed, Proxy Protocol friendly versions of HTTP and HTTPS, giving Sails proxy protocol support.

And I have code up on git and also posted on npm too: sails-proxyquire. Replace require("sails") with require("sails-proxywrap"), that should be all.