test: Introduce knowledge of FreeBSD jails #1167

jbergstroem · 2015-03-17T01:11:15Z

FreeBSD jails act differently than your average vm or similar application container. All routing passes through one ip address, which makes things like localhost or 0.0.0.0 resolve differently.

Introduce a helper that allows us to verify if we're in a jail and another one for returning an ip address for localhost.

Also, skip one test instead of trading additional complexity in common.js for one specific user scenario.

R=@indutny

indutny · 2015-03-17T01:12:03Z

cc @bnoordhuis

indutny · 2015-03-17T01:14:27Z

test/common.js

@@ -21,6 +21,32 @@ exports.tmpDir = path.join(exports.testDir, exports.tmpDirName);

 var opensslCli = null;

+Object.defineProperty(exports, 'inFbsdJail', {get: function() {


Please put get: on a next line.

jbergstroem · 2015-03-17T01:23:33Z

There's probably more places which could benefit from the localhost abstraction, but since these be the tests that specifically asserts against 127.0.0.1 I chose to keep the diff as small as possible. We should probably expand on this the next time some obscure network scenario causes more pain.

brendanashworth · 2015-03-17T01:33:39Z

+1! (can't review though since I don't have a jail) Does this mean that FreeBSD will be moved to the supported systems?

jbergstroem · 2015-03-17T01:38:04Z

@brendanashworth that list is (unfortunately, our bad) slightly outdated. What I at least can say (with a smile) is that tests should now pass both on Jenkins – which is run on jails – and in my non-jail environments. As for making it supported I don't think we should rush it just yet.

shigeki · 2015-03-17T05:49:15Z

+1 for I'm very happy CI become more healthy. I could not figure out why those tests were failed on FreeBSD.

Is process.env.LOCALHOST locally defined on only CI? In my jail host in FreeBSD, the following works fine to find out the bound address aliased lo0 in the jail.

require('os').networkInterfaces().lo0.filter(function(i) {return i.family==='IPv4'})[0].address;

jbergstroem · 2015-03-17T07:53:36Z

@shigeki thanks for the feedback! Me and @indutny talked about the most reliable approach to this. Since FreeBSD jails likely represents a small usage we opted to assume that these users could handle providing a "reliable" environment variable instead of trying to figure out what network interface to pass. If route default get was available from within jails, this would without a doubt be the best solution - but traversing os.networkInterfaces() is likely to fail.

Btw, would you be keen to join the @iojs/platform-freebsd team? Always good to have more people caring about improving io.js on FreeBSD. In practise, it means that you'd get cc'ed every time the group is referenced, but no responsibility to fix anything. For more info about platforms, see issue 1006.

shigeki · 2015-03-17T08:27:53Z

@jbergstroem I agree the jail environment is very rare and more generic approach is preferable. I'm happy to join freebsd team though I don't usually use FreeBSD mainly. For these days, I need to work on it to upgrade openssl as in shigeki@eaf44c8 because clang in FreeBSD is different from that in MacOS and it's hard to know the llvm version from outputs. I'd like to have a more specific information on FreeBSD.

jbergstroem · 2015-03-17T08:36:05Z

@shigeki; glad to have you part of the team -- increased visibility is always a good thing. FreeBSD isn't my main platform either, but FreeBSD is such a great platform to debug on so I often find myself using it! Anyway, I'll see to it you're added. Thanks for reviewing the patch. If you think its good to merge, a comment would be appreciated.

bnoordhuis · 2015-03-17T12:53:28Z

test/common.js

+  get: function() {
+    if (process.platform === 'freebsd' &&
+      child_process.execSync('sysctl -n security.jail.jailed').toString() ===
+      '1\n') {


You should probably cache the result.

bnoordhuis · 2015-03-17T13:02:19Z

A bunch of style nits but nothing substantial. I'm not really fond of magical properties but we're already doing that for other things.

If I can make one suggestion: maybe rename inFbsdJail to e.g. inFreeBSDJail? I suspect that not everyone will immediately recognize that Fbsd stand for FreeBSD.

FreeBSD jails act differently than your average vm or similar application container. All routing passes through one ip address, which makes things like localhost or 0.0.0.0 resolve differently. Introduce a helper that allows us to verify if we're in a jail and another one for returning an ip address for localhost. Also, skip one test instead of trading additional complexity in common.js for one specific user scenario. PR-URL: nodejs#1167

jbergstroem · 2015-03-18T04:19:55Z

Just fixed all style nits and renamed the function. Skipping caching for now. I'm starting to dislike what common.js is turning into..

jbergstroem · 2015-03-18T21:54:02Z

Here's two test runs on our ci -- one without LOCALHOST= and one with (focus on the freebsd machines):

indutny · 2015-03-18T21:56:15Z

Yay!

bnoordhuis · 2015-03-18T21:57:45Z

LGTM. I'm surprised you only had to update a handful of tests, we use 127.0.0.1 all over the place.

jbergstroem · 2015-03-18T21:59:05Z

@bnoordhuis Yeah, but only to assign - not compare. I can fix this in another PR.

FreeBSD jails act differently than your average vm or similar application container. All routing passes through one ip address, which makes things like localhost or 0.0.0.0 resolve differently. Introduce a helper that allows us to verify if we're in a jail and another one for returning an ip address for localhost. Also, skip one test instead of trading additional complexity in common.js for one specific user scenario. PR-URL: #1167 Reviewed-By: Ben Noordhuis <[email protected]>

jbergstroem · 2015-03-18T22:13:15Z

Fixed in c15e81a. Thanks for review and feedback.

rvagg · 2015-03-18T22:14:44Z

test/common.js

@@ -21,6 +21,35 @@ exports.tmpDir = path.join(exports.testDir, exports.tmpDirName);

 var opensslCli = null;

+Object.defineProperty(exports, 'inFreeBSDJail', {


Perhaps the return value of these should be cached? In test-net-local-address-port.js already you're using localhost_ipv4 twice and it's easy to forget that dynamic properties are lazy. See opensslCli for an already cached property.

brendanashworth · 2015-03-19T00:40:33Z

This was really a step forward. Thanks for your work here.

shigeki · 2015-03-19T00:44:57Z

Great job! I'm very happy to see a blue right of FreeBSDs on Jenkins.

jbergstroem force-pushed the fbsd_jail_networking branch from 0b53290 to d48dfe7 Compare March 17, 2015 01:11

indutny reviewed Mar 17, 2015
View reviewed changes

jbergstroem force-pushed the fbsd_jail_networking branch from d48dfe7 to fa29f2c Compare March 17, 2015 01:19

jbergstroem force-pushed the fbsd_jail_networking branch from fa29f2c to 2713538 Compare March 17, 2015 01:21

bnoordhuis reviewed Mar 17, 2015
View reviewed changes

jbergstroem force-pushed the fbsd_jail_networking branch from 2713538 to 5d3eefa Compare March 18, 2015 04:09

rvagg mentioned this pull request Mar 18, 2015

Evangelism: Weekly Update March 20th nodejs/evangelism#46

Closed

jbergstroem closed this Mar 18, 2015

rvagg reviewed Mar 18, 2015
View reviewed changes

rvagg mentioned this pull request Mar 18, 2015

test: cache lazy properties, fix style nits #1196

Merged

rvagg mentioned this pull request Mar 19, 2015

Release proposal: v1.6.0 #1157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Introduce knowledge of FreeBSD jails #1167

test: Introduce knowledge of FreeBSD jails #1167

jbergstroem commented Mar 17, 2015

indutny commented Mar 17, 2015

indutny Mar 17, 2015

jbergstroem commented Mar 17, 2015

brendanashworth commented Mar 17, 2015

jbergstroem commented Mar 17, 2015

shigeki commented Mar 17, 2015

jbergstroem commented Mar 17, 2015

shigeki commented Mar 17, 2015

jbergstroem commented Mar 17, 2015

bnoordhuis Mar 17, 2015

bnoordhuis commented Mar 17, 2015

jbergstroem commented Mar 18, 2015

jbergstroem commented Mar 18, 2015

indutny commented Mar 18, 2015

bnoordhuis commented Mar 18, 2015

jbergstroem commented Mar 18, 2015

jbergstroem commented Mar 18, 2015

rvagg Mar 18, 2015

brendanashworth commented Mar 19, 2015

shigeki commented Mar 19, 2015

		@@ -21,6 +21,32 @@ exports.tmpDir = path.join(exports.testDir, exports.tmpDirName);

		var opensslCli = null;

		Object.defineProperty(exports, 'inFbsdJail', {get: function() {

		@@ -21,6 +21,35 @@ exports.tmpDir = path.join(exports.testDir, exports.tmpDirName);

		var opensslCli = null;

		Object.defineProperty(exports, 'inFreeBSDJail', {

test: Introduce knowledge of FreeBSD jails #1167

test: Introduce knowledge of FreeBSD jails #1167

Conversation

jbergstroem commented Mar 17, 2015

indutny commented Mar 17, 2015

indutny Mar 17, 2015

Choose a reason for hiding this comment

jbergstroem commented Mar 17, 2015

brendanashworth commented Mar 17, 2015

jbergstroem commented Mar 17, 2015

shigeki commented Mar 17, 2015

jbergstroem commented Mar 17, 2015

shigeki commented Mar 17, 2015

jbergstroem commented Mar 17, 2015

bnoordhuis Mar 17, 2015

Choose a reason for hiding this comment

bnoordhuis commented Mar 17, 2015

jbergstroem commented Mar 18, 2015

jbergstroem commented Mar 18, 2015

indutny commented Mar 18, 2015

bnoordhuis commented Mar 18, 2015

jbergstroem commented Mar 18, 2015

jbergstroem commented Mar 18, 2015

rvagg Mar 18, 2015

Choose a reason for hiding this comment

brendanashworth commented Mar 19, 2015

shigeki commented Mar 19, 2015