os: add CIDR support by zeusdeux · Pull Request #14307 · nodejs/node · GitHub

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

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

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

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)

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));

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

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

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

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

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.

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.

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

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.

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

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)}`)
  });

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

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

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

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

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

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

os: add CIDR support by zeusdeux · Pull Request #14307 · nodejs/node · GitHub

Uh oh!

Uh oh!

os: add CIDR support - #14307

os: add CIDR support#14307refack merged 1 commit intonodejs:masternodejs/node:masterfrom zeusdeux:feature/cidr-support-issue-14006zeusdeux/node:feature/cidr-support-issue-14006Copy head branch name to clipboard

Conversation

zeusdeux commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeusdeux commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 16, 2017

Uh oh!

refack commented Jul 16, 2017

Uh oh!

silverwind Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jul 16, 2017

Uh oh!

zeusdeux commented Jul 16, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Aug 7, 2017

Uh oh!

zeusdeux commented Aug 8, 2017

Uh oh!

zeusdeux commented Aug 11, 2017

Uh oh!

refack commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

os: add CIDR support#14307
refack merged 1 commit into
nodejs:masterfrom
zeusdeux:feature/cidr-support-issue-14006

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