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

Invalid PORT value #13

Open
ncruces opened this issue Jan 7, 2016 · 8 comments
Open

Invalid PORT value #13

ncruces opened this issue Jan 7, 2016 · 8 comments

Comments

@ncruces
Copy link

ncruces commented Jan 7, 2016

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
Copy link
Member

pmuellr commented Jan 7, 2016

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
Copy link
Member

pmuellr commented Jan 13, 2017

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.

@pmuellr pmuellr closed this as completed Jan 13, 2017
@sdsanders
Copy link

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

@ncruces
Copy link
Author

ncruces commented Feb 13, 2017

@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
Copy link
Author

ncruces commented Feb 13, 2017

@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
Copy link
Member

pmuellr commented Feb 13, 2017

@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).

@pmuellr pmuellr reopened this Feb 13, 2017
@ncruces
Copy link
Author

ncruces commented Feb 13, 2017

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
Copy link
Member

pmuellr commented Feb 14, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants