cloudfoundry-community / node-cfenv

easy access to your Cloud Foundry application environment for node
Apache License 2.0
73 stars 20 forks source link

Invalid PORT value #13

Open ncruces opened 8 years ago

ncruces commented 8 years ago

Trying to keep a codebase compatible between CloudFoundry (Bluemix) and Azure.

I'm getting an error on getAppEnv, because under Azure, port is not a number but a named pipe (/\\.\pipe\xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx).

I was hoping to use cfenv with the vcap option filled some other way when running "locally" on Azure.

pmuellr commented 8 years ago

Trying to keep a codebase compatible between CloudFoundry (Bluemix) and Azure.

Nice! Worthy effort.

Without know anything about suitability for running in Azure, guessing a "cloud-env" sort of package would be shaped differently than node-cfenv. Meaning, prolly time to build a cloud-env sort of package. Happy to provide advice, though I don't use CF much ATM.

I think what I'd do is provide some kind of conversion wrapper module, that calls either the CF or Azure-based main module, but translates env vars and such to the other format before calling the main. Good way to figure out what needs to be dealt with, feasibility, etc.

pmuellr commented 7 years ago

Original request is a bit out-of-scope for this package, I think. Please re-open if you still want to pursue the Azure support - note that I'm assuming this isn't CF on Azure, but Azure itself.

sdsanders commented 7 years ago

@ncruces did you ever figure this out? I'm running into similar issues on Azure.

ncruces commented 7 years ago

@sdsanders no, I haven't. I ended up not needing Azure.

I'm pretty sure just changing cfenv here to port = portString; fixes it, though.

ncruces commented 7 years ago

@pmuellr, my intention was never to argue for Azure support in cfenv, that would be out of scope, sure.

But in keeping with the idea that cfenv "provides useful default values when you're running locally", does it make sense to mandate process.env.PORT to be an integer? Node.js, Express.js both seem perfectly happy with a named pipe for the port.

Also, even if you feel cfenv.port should be an integer, throwing on initialization prevents me from doing the obvious (I need to wrap require in a try-catch instead):

const cfenv = require('cfenv').getAppEnv();
if (cfenv.isLocal) // ...

Assuming VCAP_APPLICATION and VCAP_SERVICES are JSON, makes sense, and is not likely to cause much grief. Assuming process.env.PORT is always an integer (and throwing on initialization) goes a step too far, IMHO.

pmuellr commented 7 years ago

@ncruces Ah, I see.

If we were to start accepting non-numeric strings as ports, we'd need to change the values of the properties bind, urls, and url in the object that getAppEnv() returns. I guess these would all be nulls, or just not set at all?

I'm a little hesitant to just make this the default behavior, since I'd think that using port numbers would be the default strategy folks would be using, so that if they inadvertently provide a non-numeric port value, they'll get a fast fail when trying to use it.

What about some kind of opt-in on the getAppEnv() parameter options? Maybe something alike allowNonNumericPort: aBoolean (but shorter, somehow).

ncruces commented 7 years ago

Hadn't thought of that, sorry.

Creating http(s) URLs for these is indeed troublesome, and not very useful, I imagine. So I think nulls/undefined (whatever's easier) would be the better option.

An opt-in would be fine. In fact, anything that get's me through require('cfenv').getAppEnv() without crashing would do. If app/services are filled with something useful, even better.

Note that this isn't exclusive to Azure/Windows named pipes. Unix Domain Sockets work similarly. Though the folks at IISNode are the big ones doing this.

As I said, I'm not pursuing this anymore, and at the moment it'd be a big detour for me to ensure something works and is useful on Azure. So feel free to close this.

pmuellr commented 7 years ago

Thanks for chiming in on this.

For me the big thing w/cfenv was getting the VCAP_SERVICES parsed and then accessed in a somewhat sane manner. Getting the ports, urls, etc was really just gravy.

I would be surprised to find anyone would want to structure their non-CF service info in the existing VCAP_SERVICES format just to be able to use cfenv. That seems like a lot of work. I think as we see more service discovery structure become standardized across these cloud frameworks, it may make sense to create some higher level "env" libraries to make it easier to write portable code.

Having said all that, I think we've sketched out a path for handling non-numeric ports in cfenv, so I'll leave this issue open in case anyone else finds a need for it. We've started playing a bit with unix domain sockets as an alternative to port-based ones, just to avoid the nasty issue of "finding an open port" - but not for our http servers, yet :-), and not for our cf services, and mainly just for testing.