Standardise Process Execution in tests/utils/exec.ts

[CMCDragonkai]

Specification

With the resolution of #410, there is some messiness left in the tests/utils/exec.ts.

We want to standardise the implementation of:

• pkExec
• pkSpawn
• pkExpect

Clarify the behaviour of pk and pkStdio in the comments.

And also incorporate the requirement of pkExecNs and pkSpawnNs into the main 3 functions above.

Due to the difference in command and argument quoting behaviour when shell: true for child_process.execFile (#391 (comment)), we should no longer using child_process.execFile (and nor are we supposed to use child_process.exec).

This means all 3 functions will instead be using child_process.spawn.

These functions will need to add an additional option for the command string override. Currently the functions all take existing required parameters, this could be rolled into an options object instead for easier extensibility. The below examples assumes this has been done.

Right now we have a PK_TEST_COMMAND which overrides the default ts-node command string running against ${globalThis.projectDir}/src/bin/polykey.ts.

However we want to have the standard override idiom: PARAMETERS > ENVIRONMENT > DEFAULT.

This allows us to incorporate pkExecNs and pkSpawnNS into the same system, as they will be explicitly setting network namespaced command string.

To make this nicer to use, the exec.ts should also be exporting:

const polykeyPath = `${globalThis.projectDir}/src/bin/polykey`

export {
  polykeyPath
};

So any usage of pkExecNs and pkSpawnNs should look like:

import * as testExec from '../utils/exec';

await pkExec(
  ['agent', 'start'], 
  { command: `${nsenter(usernsPid, netnsPid)} ts-node ${testExec.polykeyPath}}`}
)

Now the 3 commands are going to be the entrypoints for executing Polykey. We can still export the target variants (which we've renamed to be explicitly about WithShell and WithoutShell) in case there are edge cases.

If we need to, it should be like this:

const polykeyPath = `${globalThis.projectDir}/src/bin/polykey`

// By default `globalThis.testCommand` should be `undefined` because `PK_TEST_COMMAND` will not be set
// This is strictly checking for existence, `PK_TEST_COMMAND=''` is legitimate but undefined behaviour
async function pkExec(args, opts: { command: globalThis.testCommand }) {
  if (opts.command == null) {
    await pkExecWithoutShell(args);
  } else {
    await pkExecWithShell(args, { command });
  }
}

// This is the DEFAULT
async function pkExecWithOutShell(args) {
  await childProcess.spawn(`ts-node ${polykeyPath}`, args, { shell: false });
}

// This is the PARAMETER > ENVIRONMENT override
async function pkExecWithShell(args, opts) {
  args = args.map(escapeShellArg);
  await childProcess.spawn(opts.command, args, { shell: true });
}

function escapeShellArg(arg: string): string {
  return arg.replace(/(["\s'$`\\])/g,'\\$1');
}

export {
  polykeyPath,
  pkExec,
  pkExecWithoutShell,
  pkExecWithShell,
  escapeShellArg,
};

This requires that the from the user's (tester's) perspective, the execution behaviour should stay the same. This requires us to sanity check a few things:

1. All signals are propagated from the intermediate shell process to the PK agent process
2. All child process events are received as if the shell: false was set

Note the argument escaping function escapeShellArg, this is necessary to ensure that any shell metacharacters in the argument strings are interpeted literally.

To make the above work for docker integration tests, we also need to change our release.nix to use Entrypoint instead of Cmd:

      config = {
        Entrypoint = "/bin/polykey";
      };

This will require updates to Polykey-Infrastructure as well, as there's no more /bin/polykey needed. You can just pass arguments directly after, e.g. docker run image:tag agent start.

Finally we are left with:

• pk - this just calls the main function
• pkStdio - this just wraps pk with mocking
• exec - this is intended to call any other program arbitrarily

The pk and pkStdio should be reverted to back to how it was before. Except with potentially some signature change to abide by changes to pkExec and friends. They are intended to be only for function execution, and not real process testing. If any test needs to do integration testing, they can migrate to using pkExec instead. Or a new test can be created that does that specifically.

The exec should taking input signature similar to spawn, this way it is possible for the end user to also specify shell: true and shell: false and use escapeShellArg for their special cases.

Additional context

• Because spawn returns a process reference, any function expecting simple promise results should wrap the process with any relevant promise handlers.
• Packaged executable integration tests - initial integration tests with docker #391 (comment) - research on shell behaviour differences
• MatrixAI/Polykey-Simulation#1 - NAT tests is currently incompatible with docker integration tests, they are going to be running independently, this is because NAT tests use a different command string, using namespaces, if NAT tests need to be run in integrated fashion, they can only be applied on the Linux platform with a Linux binary in the PK_TEST_COMMAND, if NAT tests need to be run during integration for other platforms, separate tests would have to be written for Docker, MacOS, Windows as they cannot simulate network namespaces (except for docker, but I don't think the CI/CD would be capable of this), so instead they would do this as part of Integration Tests for testnet.polykey.com Polykey-CLI#71 which means going for testnet.polykey.io instead of local NAT simulation testing

Tasks

1. Move test/utils.ts into tests/utils/utils.ts and create tests/utils/index.ts:

   export * from './utils.ts';
   export * as exec from './exec.ts';

2. Change the pkExec, pkSpawn, pkExpect signatures to taken an options object at the very end, required parameters can be specified as part of object, or as initial parameters
3. Refactor pkExec, pkSpawn, pkExpect according to spec.
4. Remove pkExecNs and pkSpawnNS and replace with pkExec and pkSpawn with command override options.
5. Command override options should produce a command string to be used in spawn(cmd).
6. Any usage of global should be using globalThis instead, this is the correct form way of accessing globals.
   • Incorporate https://eslint.org/docs/latest/rules/no-restricted-globals and ensure that window and global are not allowed to be used.
7. Export functions in order of precedence (always the functions the users are supposed to be using, and then utilities for edgecases).
8. Change release.nix to use Entrypoint.
9. Deploy container in CI/CD but also update task definition in Polykey-Infrastructure
10. Sanity check signalling and process events when using shell: true with the intermediate process, post results of these sanity checks along with code doing the check in this issue for reference.
11. No usages of execFile nor exec should be used at all.
12. Due to linting rules, child_process should actually be childProcess, make the necessary changes.

[emmacasolin]

pkExpect is included here as a method to be converted over to using the with shell/without shell variants, however nexpect.spawn doesn't have a shell option so I'm not sure if it can be converted to something equivalent.

[emmacasolin]

PR for Polykey-Infrastructure is here: https://gitlab.com/MatrixAI/Engineering/Polykey/Polykey-Infrastructure/-/merge_requests/1