os: add CIDR support #14307

zeusdeux · 2017-07-16T16:30:14Z

This patch adds support for CIDR notation to the output of the
networkInterfaces() method

Fixes: #14006

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

os

mscdex · 2017-07-16T16:47:47Z

If it is agreed that we should add this, I think it would best be done in C++ land for performance reasons.

zeusdeux · 2017-07-16T18:30:57Z

I think a benchmark would be useful before deciding to put this in C++ land.
How is benchmarking done in our codebase? (I haven't written one yet for node)

refack · 2017-07-16T18:13:11Z

test/parallel/test-os.js

+  'ffff:ffff:ffff:ffff::': 64,
+  'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff': 128
+}));
+flatten(Object.values(interfaces)).forEach((v) => {


why not do a map:

flatten(Object.values(interfaces)) .map((v) => ({v, mask: netmaskToCIDRSuffixMap.get(v.netmask)})) .forEach(({v, mask}) => {

Only because not all netmasks returned by networkInterfaces() might be in the map above. That's also the cause for the if condition.

What do you think of the following?

flatten(Object.values(interfaces)) .map((v) => { assert.ok('cidr' in v, `"cidr" prop not found in ${inspect(v)}`); return v; }) .filter((v) => netmaskToCIDRSuffixMap.has(v.netmask)) .map((v) => [v.cidr, `${v.address}/${netmaskToCIDRSuffixMap.get(v.netmask)}`]) .forEach(([actual, expected]) => assert.strictEqual(actual, expected));

refack · 2017-07-16T18:48:22Z

test/parallel/test-os.js

+
+  return arr.reduce(_flatten, []);
+};
+const netmaskToCIDRSuffixMap = new Map(Object.entries({


I like the use of entries but it will exclude this from backporting to v6.x, and it's a nice feature.
So be ready to backport.

So, should I make a more backport friendly version or leave this be till we decide to backport it?

Ping @gibfahn

refack · 2017-07-16T18:49:40Z

lib/os.js

+    acc[key] = val.map((v) => {
+      const protocol = v.family.toLowerCase();
+
+      return Object.assign({}, v, {


style nit: assign getCIDRSuffix(v.netmask, protocol) to a const, and do this in one line.

refack · 2017-07-16T18:52:25Z

Don't know why but Functional Programing makes me happy.

refack · 2017-07-16T18:54:41Z

If it is agreed that we should add this, I think it would best be done in C++ land for performance reasons.

AFAICT It's a very cold code-path. So maybe for reusability.

I think a benchmark would be useful before deciding to put this in C++ land.

https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md

Needs doc update

silverwind · 2017-07-16T20:36:29Z

test/parallel/test-os.js

+  '255.0.0.0': 8,
+  '255.255.255.0': 24,
+  'ffff:ffff:ffff:ffff::': 64,
+  'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff': 128


Also add some tests for non-byte-aligned subnet masks, e.g. 255.255.224.0 -> 19 and ffff:ffff:ffff:ff80:: -> 57.

This is hard as I rely on the output of networkInterfaces method and that gets data from the os binding. So, I don't really know where I can inject some mock data with a custom non-aligned netmask and have the js land networkInterfaces method consume that. Any ideas?

Hmm, you could potential define the parsing method in a internal os module and test that separately.

@silverwind ~~So I moved the code out but requiring internal/os fails with a module not found. Do I need to register it somewhere?~~ Found this registry node.gyp.

Secondly, where do tests for internal modules go? I see thetest folder in lib/internal only has tests for unicode.

@zeusdeux In test, just like everything else. You would need to specify a // Flags: --expose-internals to make the testing harness expose internal modules for testing though.

Moved the CIDR logic to internal/os and added tests for it @silverwind @TimothyGu.

silverwind · 2017-07-16T20:42:44Z

lib/os.js

+    .map((v) => parseInt(v, 10))
+    .reduce((acc, v) => acc += v, 0);
+}
+


I think you can remove temp altogether and return .join('').length after then maps above.

This won't work as temp.join('') could be a string of the form 11111111000 for 255.0.0.0 and called .length would return 11 which is wrong. Hence the parse and reduce.

Right, here's a slight modification I came up with that includes validation for invalid masks:

function getCIDRSuffix(subnetMask, proto) { const v6 = proto === 'ipv6'; const parts = subnetMask .split(v6 ? ':' : '.') .map((part) => parseInt(part || 0, v6 ? 16 : 10)); const isInvalidSubnetMask = parts.some(function(_, i) { if (i > 1 && parts[i - 1] < parts[i]) { return true; } }); if (isInvalidSubnetMask) { return null; } return parts .map((dec) => dec.toString(2)) .join('') .split('') .map((bin) => parseInt(bin, 10)) .reduce((acc, v) => acc += v, 0); }

There's probably a mathematical way to count the number of binary 1s in a number that eludes me right now.

silverwind · 2017-07-16T20:51:47Z

We also got to consider invalid subnet masks, for example 255.0.0.255. I think returning null in that case might be reasonable (should be documented).

zeusdeux · 2017-07-16T22:16:26Z

We also got to consider invalid subnet masks, for example 255.0.0.255. I think returning null in that case might be reasonable (should be documented).

I thought of adding this validation in, but I decided not to as that I believe should lie in C++ land. I just wanted this to be a js land "transform" on data coming in from the os binding. Nevertheless, I am open to a discussion on this.

refack

You'll need to write a few words in the documentation

refack · 2017-07-17T02:10:01Z

test/parallel/test-os.js

-});
+// assert cidr prop present all interface info
+flatten(Object.values(interfaces))
+  .map((v) => {


You don't have to care that the map.get returns undefined. It's not pure FP, but it's susinct

flatten(Object.values(interfaces)) .map((v) => ({v, mask: netmaskToCIDRSuffixMap.get(v.netmask)})) .forEach(({v, mask}) => { assert.ok('cidr' in v, `"cidr" prop not found in ${inspect(v)}`); if (mask) assert.strictEqual(v.cidr, `${v.address}/${mask)}`) });

zeusdeux · 2017-08-07T00:14:54Z

Sorry, was away for a bit. Pulled in suggested changes :)

zeusdeux · 2017-08-08T12:06:15Z

Would you mind taking another look folks? @refack @silverwind @TimothyGu
Thank you!

zeusdeux · 2017-08-11T12:06:15Z

One more approval and I'll squash my commits.
/cc @refack @TimothyGu

refack · 2017-08-14T00:16:21Z

@zeusdeux no need to squash, the person landing the PR will squash.
~~CI: https://ci.nodejs.org/job/node-test-pull-request/9644/~~
CI: https://ci.nodejs.org/job/node-test-commit/11746/

This patch adds support for CIDR notation to the output of the `networkInterfaces()` method PR-URL: nodejs#14307 Fixes: nodejs#14006 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

refack · 2017-08-14T19:46:18Z

Extra sanity on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7965/
Full spectrum JIC: https://ci.nodejs.org/job/node-test-commit/11764/

This patch adds support for CIDR notation to the output of the `networkInterfaces()` method PR-URL: #14307 Fixes: #14006 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

gibfahn · 2018-01-15T21:45:49Z

Release team decided not to land on v6.x, if you disagree let us know.

os.networkInterfaces() does support a new .cidr property since version 8.5.0 which is below the minimum 10.0.0 required by this module so use it. Ref: nodejs/node#14307

nodejs-github-bot added the os Issues and PRs related to the os subsystem. label Jul 16, 2017

zeusdeux mentioned this pull request Jul 16, 2017

CIDR support for os.networkInterfaces #14006

Closed

refack added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 16, 2017

refack previously approved these changes Jul 16, 2017

View reviewed changes

silverwind reviewed Jul 16, 2017

View reviewed changes

refack reviewed Jul 17, 2017

View reviewed changes

zeusdeux force-pushed the feature/cidr-support-issue-14006 branch from 92a4eec to 9604dfb Compare August 7, 2017 00:14

silverwind approved these changes Aug 8, 2017

View reviewed changes

jasnell approved these changes Aug 13, 2017

View reviewed changes

refack approved these changes Aug 14, 2017

View reviewed changes

refack self-assigned this Aug 14, 2017

TimothyGu approved these changes Aug 14, 2017

View reviewed changes

refack force-pushed the feature/cidr-support-issue-14006 branch from 9604dfb to 4a6b678 Compare August 14, 2017 19:43

refack merged commit 4a6b678 into nodejs:master Aug 14, 2017

refack added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 14, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

gibfahn added dont-land-on-v6.x labels Jan 15, 2018

refack removed their assignment Oct 12, 2018

zeusdeux deleted the feature/cidr-support-issue-14006 branch October 15, 2018 23:23

silverwind mentioned this pull request Aug 3, 2020

Simplify CIDR retrieval sindresorhus/internal-ip#33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: add CIDR support #14307

os: add CIDR support #14307

zeusdeux commented Jul 16, 2017 •

edited

Loading

mscdex commented Jul 16, 2017 •

edited

Loading

zeusdeux commented Jul 16, 2017 •

edited

Loading

refack Jul 16, 2017

zeusdeux Jul 16, 2017 •

edited

Loading

refack Jul 16, 2017

zeusdeux Jul 16, 2017

refack Jul 17, 2017

refack Jul 16, 2017

refack commented Jul 16, 2017

refack commented Jul 16, 2017

silverwind Jul 16, 2017 •

edited

Loading

zeusdeux Jul 16, 2017

silverwind Jul 16, 2017

zeusdeux Jul 16, 2017 •

edited

Loading

TimothyGu Jul 16, 2017

zeusdeux Aug 7, 2017 •

edited

Loading

silverwind Jul 16, 2017

zeusdeux Jul 16, 2017

silverwind Jul 17, 2017 •

edited

Loading

silverwind commented Jul 16, 2017

zeusdeux commented Jul 16, 2017

refack left a comment

refack Jul 17, 2017

zeusdeux commented Aug 7, 2017

zeusdeux commented Aug 8, 2017

zeusdeux commented Aug 11, 2017

refack commented Aug 14, 2017 •

edited

Loading

refack commented Aug 14, 2017 •

edited

Loading

gibfahn commented Jan 15, 2018

os: add CIDR support #14307

os: add CIDR support #14307

Conversation

zeusdeux commented Jul 16, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented Jul 16, 2017 • edited Loading

zeusdeux commented Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

zeusdeux Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 16, 2017

refack commented Jul 16, 2017

silverwind Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeusdeux Jul 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeusdeux Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

silverwind commented Jul 16, 2017

zeusdeux commented Jul 16, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeusdeux commented Aug 7, 2017

zeusdeux commented Aug 8, 2017

zeusdeux commented Aug 11, 2017

refack commented Aug 14, 2017 • edited Loading

refack commented Aug 14, 2017 • edited Loading

gibfahn commented Jan 15, 2018

zeusdeux commented Jul 16, 2017 •

edited

Loading

mscdex commented Jul 16, 2017 •

edited

Loading

zeusdeux commented Jul 16, 2017 •

edited

Loading

zeusdeux Jul 16, 2017 •

edited

Loading

silverwind Jul 16, 2017 •

edited

Loading

zeusdeux Jul 16, 2017 •

edited

Loading

zeusdeux Aug 7, 2017 •

edited

Loading

silverwind Jul 17, 2017 •

edited

Loading

refack commented Aug 14, 2017 •

edited

Loading

refack commented Aug 14, 2017 •

edited

Loading