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

os: add CIDR support #14307

Merged
merged 1 commit into from
Aug 14, 2017
Merged

Conversation

zeusdeux
Copy link
Contributor

@zeusdeux zeusdeux commented Jul 16, 2017

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

@nodejs-github-bot nodejs-github-bot added the os Issues and PRs related to the os subsystem. label Jul 16, 2017
@mscdex
Copy link
Contributor

mscdex commented Jul 16, 2017

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

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

zeusdeux commented Jul 16, 2017

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
refack previously approved these changes Jul 16, 2017
'ffff:ffff:ffff:ffff::': 64,
'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff': 128
}));
flatten(Object.values(interfaces)).forEach((v) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not do a map:

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

Copy link
Contributor Author

@zeusdeux zeusdeux Jul 16, 2017

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping @gibfahn

lib/os.js Outdated
acc[key] = val.map((v) => {
const protocol = v.family.toLowerCase();

return Object.assign({}, v, {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@refack
Copy link
Contributor

refack commented Jul 16, 2017

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

@refack
Copy link
Contributor

refack commented Jul 16, 2017

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

@refack refack dismissed their stale review July 16, 2017 18:55

Needs doc update

'255.0.0.0': 8,
'255.255.255.0': 24,
'ffff:ffff:ffff:ffff::': 64,
'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff': 128
Copy link
Contributor

@silverwind silverwind Jul 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@zeusdeux zeusdeux Jul 16, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@zeusdeux zeusdeux Aug 7, 2017

Choose a reason for hiding this comment

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

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

lib/os.js Outdated
.map((v) => parseInt(v, 10))
.reduce((acc, v) => acc += v, 0);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@silverwind silverwind Jul 17, 2017

Choose a reason for hiding this comment

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

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

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

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.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 zeusdeux force-pushed the feature/cidr-support-issue-14006 branch from 92a4eec to 9604dfb Compare August 7, 2017 00:14
@zeusdeux
Copy link
Contributor Author

zeusdeux commented Aug 7, 2017

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

@zeusdeux
Copy link
Contributor Author

zeusdeux commented Aug 8, 2017

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

@zeusdeux
Copy link
Contributor Author

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

@refack refack self-assigned this Aug 14, 2017
@refack
Copy link
Contributor

refack commented Aug 14, 2017

@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 refack force-pushed the feature/cidr-support-issue-14006 branch from 9604dfb to 4a6b678 Compare August 14, 2017 19:43
@refack refack merged commit 4a6b678 into nodejs:master Aug 14, 2017
@refack
Copy link
Contributor

refack commented Aug 14, 2017

@refack refack added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 14, 2017
MylesBorins pushed a commit that referenced this pull request Sep 9, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins added a commit that referenced this pull request Sep 12, 2017
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
MylesBorins added a commit that referenced this pull request Sep 12, 2017
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
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
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
Copy link
Member

gibfahn commented Jan 15, 2018

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

@refack refack removed their assignment Oct 12, 2018
@zeusdeux zeusdeux deleted the feature/cidr-support-issue-14006 branch October 15, 2018 23:23
silverwind added a commit to sindresorhus/internal-ip that referenced this pull request Aug 3, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable-change PRs with changes that should be highlighted in changelogs. os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants