Host and port binding environment variables for `pk agent start`

[CMCDragonkai]

Block 2 from #194 (comment)

Specification

The pk agent start will be used inside a docker container as the initial command. This means it has to bind to the container host and relevant ports:

• the PK agent runs with ingressHost and ingressPort used by reverse proxy, this must specified ahead of time in the TD
• the PK agent also requires a specification for the GRPCServer host and port, as this is used directly by the CLI that doesn't go through the reverse proxy

Therefore, we need environment variables for:

• PK_INGRESS_HOST + PK_INGRESS_PORT: the public-facing host and port of the ReverseProxy
• PK_CLIENT_HOST + PK_CLIENT_PORT: the public-facing host and port of the GRPC server (to remotely interact with the client service on the AWS instance of Polykey from our own devices)

---

Variable specification

We expect the variables to have the following properties:

Variable	pk agent start flag	.env variable	Default value (src/config.ts)
ReverseProxy ingress host	--ingress-host	PK_INGRESS_HOST	0.0.0.0
ReverseProxy ingress port	--ingress-port	PK_INGRESS_PORT	0
GRPCServer public-facing host	--client-host	PK_CLIENT_HOST	localhost (127.0.0.1)
GRPCServer public-facing port	--client-port	PK_CLIENT_PORT	0

(Note that the default host values are only with reference to IPv4 addresses - we don't currently support IPv6. The default IPv6 host would be expected to be ::1 though.)

Also note that defaulting the ports to 0 eventually randomises the port allocation in the GRPCServer and ReverseProxy when the port is bound to the GRPC server and UTP socket respectively.

To future-proof for supporting IPv6, we should also support being able to specify --ingress-host multiple times. This would be used such that we can bind to both an IPv4 and IPv6 host at the same time.

Similarly to the password and recovery code, the precedence for these variables would be: flag > environment variable > default value.

Parsing utility functions

We'll also require some parsing utility functions for these hosts and ports. For example, for a port:

function parsePort(value: string | undefined): number {
  let port = parseInt(value ?? '0');
  if (isNaN(port)) {
    // port = 0;
    // perhaps instead throw an exception here? See below
    throw new clientErrors.ErrorMalformedPort();
  }
  return port;
}

• (Originally discussed here: Testnet Node Deployment (testnet.polykey.io) #194 (comment))
• I'm suggesting that with these parsing functions, instead of returning some default value though (e.g. with parsePort here, we return 0), we'd want to instead throw an exception. The idea with that is, if the passed host/port is wrong (from the parameters, environment variable, wherever), we open the PK agent up to performing unexpected behaviour.
• For example, if we had a malformed port in our environment variable, no exception would be thrown, and it would be defaulted to 0. Ideally, we should instead inform the user that the host/port is malformed, and it should be fixed. However, if the host/port is simply missing, then we can use the default value.

These can be integrated with the getFrom...() functions that will be created in #202. For example:

try {
  ingressPort = parsePort(getFromParams() ?? getFromEnv('PK_INGRESS_PORT') ?? getFromConfig());
catch (e) {
  if (e instanceof clientErrors.ErrorMalformedPort) {
    // retry? Ask from STDIN? Handle in some way. 
    // Perhaps just let it bubble up to the CLI display without handling
  }
  throw e;
}

Additional context

• https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/issues/237 - old issue with screenshot about the ECS task definition and problematic environment variables
• Testnet Node Deployment (testnet.polykey.io) #194 (comment) - comment regarding the propagation of the start() method parameters to the async creation, this is required since the networking domain objects are likely to be using the StartStop decorator
• Testnet Node Deployment (testnet.polykey.io) #194 (comment) - comment regarding why we need separate port bindings for ingress and client port, as in the difference between using PK CLI/GUI client and NodeManager client.

Tasks

1. Variable specification
   • add pk agent start flags for all hosts and ports
   • support multiple --ingress-host CLI parameters being provided
2. Parsing utility functions
   • parseHost
   • parsePort
3. Incorporate all changes into pk agent start
   • retrieve variables in precedence: flag > environment variable > default value
   • exceptions thrown for malformed host/port
4. Ensure that environment variables are recorded in .env.example

• Create port and host options defined in src/bin/options.ts
• add host and port options to pk agent start
• support multiple passing multiple hosts to --ingress-host

[CMCDragonkai]

Due to refactoring the CLI in https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213#note_721718865

I'm seeing the usecase of defining options in src/bin/options.ts.

Right now I'm creating:

function parseNumber(v: string): number {
  const num = parseInt(v);
  if (isNaN(num)) {
    throw new commander.InvalidOptionArgumentError(`${v} is not a number`);
  }
  return num;
}

const clientHost = new commander.Option(
  '-ch, --client-host <address>',
  'Client Host Address'
).env('PK_CLIENT_HOST').default('127.0.0.1');

const clientPort = new commander.Option(
  '-cp, --client-port <port>',
  'Client Port'
).argParser(parseNumber).env('PK_CLIENT_PORT').default(0);

Note that I upgraded to commander 8.3.0. See the MR for details.

[tegefaulkes]

Having a look at the AgentServer and RevProxy

    await this.agentGrpcServer.start({
      services: [[AgentService, this.agentService]],
      host: this.agentGrpcHost as Host,
      port: this.agentGrpcPort as Port,
    });

  // ...

    await this.revProxy.start({
      serverHost: this.agentGrpcHost as Host,
      serverPort: this.agentGrpcServer.getPort() as Port,
      tlsConfig: {
        keyPrivatePem: keyPrivatePem,
        certChainPem: certChainPem,
      },
    });

I think how i had the host and port set here is wrong. we don't care what the agent server's host and port are so long as we pass it to the revProxy, However we want to set the ingress host and port for the revProxy here right? I will need to update this.

[tegefaulkes]

Ok, fixed that part.

[tegefaulkes]

I made --ingress-port variadic but right now only takes the 1st provided host.

[tegefaulkes]

Unless I'm missing anything this should be done pending the merge of #283

[CMCDragonkai]

This is done, but requires further testing under tests/bin/agent/ subcommands. Specifically an additional test that tests the binding to a different host like 127.0.0.2 and specific port. We just need to check that the status is writing the correct host and port, or use the status command subsequently to check.

[CMCDragonkai]

Testing has been done with specified network configuration.