Support host (IP address) in parseSeedNodes

[joshuakarp]

Specification

parseSeedNodes currently assumes that the provided address of the seed node is a hostname, and does not provide support for a seed node with an IP address as a host address:

// src/bin/utils/parsers.ts - parseSeedNodes()
if (!networkUtils.isHostname(idHostPort[1])) {
  throw new commander.InvalidOptionArgumentError(
    `${idHostPort[1]} is not a valid hostname`,
  );
}

Furthermore, it should also provide support for IPv6 addresses - as such, we can no longer rely on a colon/semi-colon split usage. It's been suggested to use new URL to construct a URL from the nodeId@host:port format (will require a prepended pk:// 'protocol') such that we can acquire the different components of the seed nodes.

Additional context

• discussed in sprint meeting https://vimeo.com/674273240 approx 23:00
• original comment here General Data Validation - Boundary IO locations should be doing data validation and marshalling (and the decommissioning of GenericIdTypes for all IDs) #321 (comment) and General Data Validation - Boundary IO locations should be doing data validation and marshalling (and the decommissioning of GenericIdTypes for all IDs) #321 (comment)

Tasks

1. Utilise new URL to generate the nodeId@host:port
2. Include the appropriate exception handling for invalid formats
   • can we specify the particular component of the nodeId@host:port that's invalid if an exception is raised (it's currently done this way through the use of split)? Look into this.
3. Move function to validation utils
4. Add test for IP address in seed node

[joshuakarp]

Experiencing some very weird behaviour when expecting a TypeError exception with jest.

Consider the following:

try {
  new URL('bad url');
} catch (e) {
  if (e instanceof TypeError) {
    console.log('This is a TypeError')
  }
  console.log(e);
}

We get the following output:

This is a TypeError
TypeError [ERR_INVALID_URL]: Invalid URL: bad url
    at onParseError (internal/url.js:279:9)
    at new URL (internal/url.js:355:5)
    at Object.<anonymous> (/home/josh/Documents/MatrixAI/js-polykey/test.ts:6:3)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Module.m._compile (/home/josh/Documents/MatrixAI/js-polykey/node_modules/ts-node/src/index.ts:1371:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Object.require.extensions.<computed> [as .ts] (/home/josh/Documents/MatrixAI/js-polykey/node_modules/ts-node/src/index.ts:1374:12)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12) {
  input: 'bad url',
  code: 'ERR_INVALID_URL'
}

Running the same code within a jest test:

    test.only('invalid', () => {
      try {
        new URL('bad url');
      } catch (e) {
        if (e instanceof TypeError) {
          console.log('This is a TypeError')
        }
        console.log(e);
      }
    });

We instead only get the error output: we never enter the e instanceof TypeError conditional:

Determining test suites to run...
GLOBAL SETUP
Global Data Dir: /run/user/1000/polykey-test-global-7djSRg
 PASS  tests/validation/utils.test.ts (5.872 s)
  Validation utils
    parseSeedNodes
      ✓ invalid (15 ms)
      ○ skipped valid seed nodes (using hostname, IPv4, IPv6)
      ○ skipped invalid node ID
      ○ skipped invalid hostname
      ○ skipped invalid port
      ○ skipped invalid structure

  console.log
    TypeError [ERR_INVALID_URL]: Invalid URL: bad url
        at onParseError (internal/url.js:279:9)
        at new URL (internal/url.js:355:5)
        at Object.<anonymous> (/home/josh/Documents/MatrixAI/js-polykey/tests/validation/utils.test.ts:60:9)
        at Object.asyncJestTest (/home/josh/Documents/MatrixAI/js-polykey/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:106:37)
        at /home/josh/Documents/MatrixAI/js-polykey/node_modules/jest-jasmine2/build/queueRunner.js:45:12
        at new Promise (<anonymous>)
        at mapper (/home/josh/Documents/MatrixAI/js-polykey/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
        at /home/josh/Documents/MatrixAI/js-polykey/node_modules/jest-jasmine2/build/queueRunner.js:75:41
        at processTicksAndRejections (internal/process/task_queues.js:95:5) {
      input: 'bad url',
      code: 'ERR_INVALID_URL'
    }

Similarly, when trying to expect the exception to be a TypeError, jest instead tells us that it received a NodeError:

    test.only('invalid', () => {
      expect(() => {
        new URL('bad URL');
      }).toThrowError(TypeError);
    });

Determining test suites to run...
GLOBAL SETUP
Global Data Dir: /run/user/1000/polykey-test-global-Mm52Jp
 FAIL  tests/validation/utils.test.ts (6.502 s)
  Validation utils
    parseSeedNodes
      ✕ invalid (16 ms)
      ○ skipped valid seed nodes (using hostname, IPv4, IPv6)
      ○ skipped invalid node ID
      ○ skipped invalid hostname
      ○ skipped invalid port
      ○ skipped invalid structure

  ● Validation utils › parseSeedNodes › invalid

    expect(received).toThrowError(expected)

    Expected constructor: TypeError
    Received constructor: NodeError

    Received message: "Invalid URL: bad URL"

          55 |     test.only('invalid', () => {
          56 |       expect(() => {
        > 57 |         new URL('bad URL');
             |         ^
          58 |       }).toThrowError(TypeError)
          59 |     });
          60 |   });

          at tests/validation/utils.test.ts:57:9
          at Object.<anonymous> (node_modules/expect/build/toThrowMatchers.js:83:11)
          at Object.throwingMatcher [as toThrowError] (node_modules/expect/build/index.js:338:21)
          at Object.<anonymous> (tests/validation/utils.test.ts:58:10)

      56 |       expect(() => {
      57 |         new URL('bad URL');
    > 58 |       }).toThrowError(TypeError)
         |          ^
      59 |     });
      60 |   });
      61 | });

      at Object.<anonymous> (tests/validation/utils.test.ts:58:10)

Have any of you come across this at all with jest? @CMCDragonkai @tegefaulkes @emmacasolin

[CMCDragonkai]

Is Jest messing up native exception classes?

[CMCDragonkai]

@tegefaulkes is this still need to be done, or was it solved by #378?

[CMCDragonkai]

We need a test or tests here that tests the usage of hostnames, ipv4 hosts (and optionally ipv6 hosts) directly from the command line from pk agent start.

We should have these addresses usable:

pk://eoirus89fus89f@192.168.1.1:4343
pk://eoirus89fus89f@[::1]:4343
eoirus89fus89f@[::1]:4343
eoirus89fus89f@testnet.polykey.io:4343

For now ipv6 will not work, so we can just have a test stub. Like test.todo.

Note that we would not force the usage of pk://. If the address doesn't have the protocol, just add one in front. This is necessary for new URL to work.

[tegefaulkes]

Looks like src/validation/utils.ts:211 parseSeedNodes exists and is using new URL('pk://${seedNodeString}'); to seperate the host and port. I think we justt need to check and expand the tests to see that it's working properly.

[tegefaulkes]

Tests for this already exist at tests/validation/utils.nodes.test.ts that cover all of the test cases above. I think this issue has been addressed. I think we can close this issue.

[CMCDragonkai]

Can you check #324 (comment), to see if that's a problem. We need to confirm if TypeError is in fact an issue inside jest.

[CMCDragonkai]

[CMCDragonkai]

So parseSeedNodes assumes you don't have the protocol. So it adds pk:// in front.

Can you change it up so it can also take in the case where a protocol does in fact exist, and if so, doesn't add a prefix. The adding of the prefix should only be there if a protocol doesn't exist.

[tegefaulkes]

I can replicate the TypeError jest problem. they TypeError thrown from the new URL() doesn't match the global TypeError but only in jest. Trying the same check in a test file with ts-node gives us the expected behaviour of them matching. Even explicitly importing URL and using that doesn't seem to solve the problem.

I'm not super sure how to fix this properly. It only really affects the tests and only in a specific way. Since it seems that URL can only throw a TypeError we can just assume any error out of it is the TypeError and re-throw as a ErrorParse error. that should allow us to write a test for it. But it is also just a problem that we can ignore with little issue. we already don't have a test where it causes an issue.

[CMCDragonkai]

Yea ok in that case, if you catch any error from new URL it should be overridden as ErrorParse. We should make that change so that tests don't fail in the future.

[tegefaulkes]

I just pushed up the changes to staging. I'm closing this issue.