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

test: Introduce knowledge of FreeBSD jails #1167

Closed
wants to merge 1 commit into from

Conversation

jbergstroem
Copy link
Member

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

indutny commented Mar 17, 2015

cc @bnoordhuis

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

var opensslCli = null;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put get: on a next line.

@jbergstroem
Copy link
Member Author

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

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

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

shigeki commented Mar 17, 2015

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

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

shigeki commented Mar 17, 2015

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably cache the result.

@bnoordhuis
Copy link
Member

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

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

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

@indutny
Copy link
Member

indutny commented Mar 18, 2015

Yay!

@bnoordhuis
Copy link
Member

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

@jbergstroem
Copy link
Member Author

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

jbergstroem added a commit that referenced this pull request Mar 18, 2015
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 <info@bnoordhuis.nl>
@jbergstroem
Copy link
Member Author

Fixed in c15e81a. Thanks for review and feedback.

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

var opensslCli = null;

Object.defineProperty(exports, 'inFreeBSDJail', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@shigeki
Copy link
Contributor

shigeki commented Mar 19, 2015

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

@rvagg rvagg mentioned this pull request Mar 19, 2015
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

Successfully merging this pull request may close these issues.

6 participants