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

Port manager #42

Merged
merged 27 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bcd10b7
start setting up port manager
camden11 Sep 26, 2023
d028563
detectPort first pass
camden11 Sep 26, 2023
82cd169
Fix port conflict bug
camden11 Sep 26, 2023
ecc19c8
Port manager get/set functionality
camden11 Sep 26, 2023
9b660ea
add delete endpoint + clean some things up
camden11 Sep 26, 2023
e151907
port manager module first pass
camden11 Sep 27, 2023
75726b0
export port manager
camden11 Sep 27, 2023
5766ea7
rename some things and add server index route
camden11 Sep 27, 2023
c2e3013
Fix some portmanager bugs
camden11 Sep 28, 2023
060b9f6
Fix some port manager bugs
camden11 Sep 28, 2023
d5ce5cd
use cors
camden11 Oct 3, 2023
61d81d4
Add logging/errors to PortManagerServer
camden11 Oct 4, 2023
b83750f
Merge branch 'main' into port-manager
camden11 Oct 18, 2023
73d05b1
Wait for PortManagerServer to start
camden11 Oct 18, 2023
90ae978
rename assignPortToServerInstance
camden11 Oct 18, 2023
e79df2a
Set up port manager for requesting multiple ports
camden11 Oct 18, 2023
1d30147
fix bug in requestPort
camden11 Oct 19, 2023
91a3228
Merge branch 'main' into port-manager
camden11 Oct 31, 2023
0bb5532
Update request ports to allow for specifying ports
camden11 Oct 31, 2023
d191ff3
port manager api tweaks
camden11 Oct 31, 2023
053dba6
Merge branch 'main' into port-manager
camden11 Nov 7, 2023
872304f
remove todo
camden11 Nov 7, 2023
2726eec
restore portmanager types file
camden11 Nov 7, 2023
de09bad
Merge branch 'main' into port-manager
camden11 Nov 8, 2023
25eb5ea
Add detectPort copy to lang file
camden11 Nov 8, 2023
825d86b
Add detect port license
camden11 Nov 8, 2023
bd1e9cb
update invalid port copy
camden11 Nov 8, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions constants/ports.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const MIN_PORT_NUMBER = 1024;
export const MAX_PORT_NUMBER = 65535;

// TODO: What's a good port for the port manager?
Copy link
Contributor

Choose a reason for hiding this comment

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

lingering todo (unless you still plan to revisit this)

export const PORT_MANAGER_SERVER_PORT = 8080;
12 changes: 12 additions & 0 deletions lang/en.lyaml
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,12 @@ en:
processFieldsJs:
converting: 'Converting "{{ src }}" to "{{ dest }}".'
converted: 'Finished converting "{{ src }}" to "{{ dest }}".'
utils:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm realizing we're going to hit some merge conflicts with all of the WIP PR's and my translations spike PR

PortManagerServer:
started: "PortManagerServer running on port {{ port }}"
setPort: "Server with instanceId {{ instanceId }} assigned to port {{ port }}"
deletedPort: "Server with instanceId {{ instanceId }} unassigned from port {{ port }}"
close: "PortManagerServer shutting down."
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should refer to this differently in logs. End users probably don't need to know about portManagerServer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, though these are all debug logs so theoretically end users wouldn't be seeing them. Seemed helpful if anyone was ever debugging something related to this or the inner workings became relevant to their workflow

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I was thinking that they were standard logs. That makes sense 👌

errors:
hubdb:
invalidJsonPath: "The HubDB table file must be a '.json' file"
Expand Down Expand Up @@ -201,6 +207,12 @@ en:
configIgnore: "Unable to determine if config file is properly ignored by git."
notify:
filePath: "Unable to notify file '{{ filePath }}'"
PortManagerServer:
portInUse: "Failed to start PortManagerServer. Port {{ port }} is already in use."
duplicateInstance: "Failed to start PortManagerServer. An instance of PortManagerServer is already running."
404: "Could not find a server with instanceId {{ instanceId }}"
409: "Failed to assign port. Server with instanceId {{ instanceId }} is already running on port {{ port }}"
400: "Invalid port requested. Port must be between 1024 and 65535."
config:
cliConfiguration:
noConfigLoaded: "No config loaded."
Expand Down
58 changes: 58 additions & 0 deletions lib/portManager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import axios from 'axios';

import PortManagerServer from '../utils/PortManagerServer';
import { detectPort } from '../utils/detectPort';
import { PORT_MANAGER_SERVER_PORT } from '../constants/ports';
import { RequestPortsData } from '../types/PortManager';

const BASE_URL = `http://localhost:${PORT_MANAGER_SERVER_PORT}`;

async function isPortManagerServerRunning(): Promise<boolean> {
const port = await detectPort(PORT_MANAGER_SERVER_PORT);
return port !== PORT_MANAGER_SERVER_PORT;
}

export async function startPortManagerServer(): Promise<void> {
const isRunning = await isPortManagerServerRunning();

if (!isRunning) {
await PortManagerServer.init();
}
}

export async function stopPortManagerServer(): Promise<void> {
const isRunning = await isPortManagerServerRunning();

if (isRunning) {
await axios.post(`${BASE_URL}/close`);
}
}

export async function requestPorts(
portData: Array<RequestPortsData>
): Promise<Array<RequestPortsData>> {
const { data } = await axios.post(`${BASE_URL}/servers`, {
portData: portData,
});

return data.ports;
}

export async function deleteServerInstance(
serverInstanceId: string
): Promise<void> {
await axios.post(`${BASE_URL}/servers/${serverInstanceId}`);
}

export async function portManagerHasActiveServers() {
const { data } = await axios.get(`${BASE_URL}/servers`);
return data.count > 0;
}

function toId(str: string) {
return str.replace(/\s+/g, '-').toLowerCase();
}

export function getServerInstanceId(serverId: string, resourceId: string) {
return `${toId(serverId)}__${toId(resourceId)}`;
}
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
"license": "Apache-2.0",
"devDependencies": {
"@types/content-disposition": "^0.5.5",
"@types/cors": "^2.8.15",
"@types/debounce": "^1.2.1",
"@types/express": "^4.17.18",
"@types/findup-sync": "^4.0.2",
"@types/fs-extra": "^11.0.1",
"@types/jest": "^29.5.0",
Expand All @@ -56,9 +58,12 @@
"./logger": "./lib/logging/logger.js"
},
"dependencies": {
"address": "^2.0.1",
"axios": "^1.3.5",
"chokidar": "^3.5.3",
"content-disposition": "^0.5.4",
"cors": "^2.8.5",
"express": "^4.18.2",
"extract-zip": "^2.0.1",
"findup-sync": "^5.0.0",
"fs-extra": "^11.1.0",
Expand Down
4 changes: 4 additions & 0 deletions types/PortManager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export type RequestPortsData = {
instanceId: string;
port?: number;
};
184 changes: 184 additions & 0 deletions utils/PortManagerServer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
import express, { Express, Request, Response } from 'express';
import { Server } from 'http';
import cors from 'cors';

import { detectPort } from './detectPort';
import {
MIN_PORT_NUMBER,
MAX_PORT_NUMBER,
PORT_MANAGER_SERVER_PORT,
} from '../constants/ports';
import { throwErrorWithMessage } from '../errors/standardErrors';
import { debug } from './logger';
import { i18n } from './lang';
import { BaseError } from '../types/Error';
import { RequestPortsData } from '../types/PortManager';

type ServerPortMap = {
[instanceId: string]: number;
};

const i18nKey = 'utils.PortManagerServer';

class PortManagerServer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker - I wonder if we should even bother having a models/ folder in this repo. I think we have a few other classes in here that also aren't in that folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 🤷 . Was left over from cli-lib but definitely don't think we need to make the distinction. I'm down to remove in the future

app?: Express;
server?: Server;
serverPortMap: ServerPortMap;

constructor() {
this.serverPortMap = {};
}

async init(): Promise<void> {
if (this.app) {
throwErrorWithMessage(`${i18nKey}.duplicateInstance`);
}
this.app = express();
this.app.use(express.json());
this.app.use(cors());
this.setupRoutes();

try {
this.server = await this.listen();
} catch (e) {
const error = e as BaseError;
if (error.code === 'EADDRINUSE') {
throwErrorWithMessage(
`${i18n}.portInUse`,
{
port: PORT_MANAGER_SERVER_PORT,
},
error
);
}
throw error;
}
}

listen(): Promise<Server> {
return new Promise<Server>((resolve, reject) => {
const server = this.app!.listen(PORT_MANAGER_SERVER_PORT, () => {
debug(`${i18nKey}.started`, {
port: PORT_MANAGER_SERVER_PORT,
});
resolve(server);
}).on('error', err => {
reject(err);
});
});
}

setupRoutes(): void {
if (!this.app) {
return;
}

this.app.get('/servers', this.getServers);
this.app.get('/servers/:instanceId', this.getServerPortByInstanceId);
this.app.post('/servers', this.assignPortsToServers);
this.app.delete('/servers/:instanceId', this.deleteServerInstance);
this.app.post('/close', this.closeServer);
}

setPort(instanceId: string, port: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this protect against initializing duplicate instanceId's?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setPort is only called by assignPortToServer, which does protect against duplicates

debug(`${i18nKey}.setPort`, { instanceId, port });
this.serverPortMap[instanceId] = port;
}

deletePort(instanceId: string) {
debug(`${i18nKey}.deletePort`, {
instanceId,
port: this.serverPortMap[instanceId],
});
delete this.serverPortMap[instanceId];
}

send404(res: Response, instanceId: string) {
res
.status(404)
.send(i18n(`errors.${i18nKey}.404`, { instanceId: instanceId }));
}

getServers = async (req: Request, res: Response): Promise<void> => {
res.send({
servers: this.serverPortMap,
count: Object.keys(this.serverPortMap).length,
});
};

getServerPortByInstanceId = (req: Request, res: Response): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the return on this be Promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't thinks so, this isn't async. Express knows how to handle it as is

const { instanceId } = req.params;
const port = this.serverPortMap[instanceId];

if (port) {
res.send({ port });
} else {
this.send404(res, instanceId);
}
};

assignPortsToServers = async (
req: Request<never, never, { portData: Array<RequestPortsData> }>,
res: Response
): Promise<void> => {
const { portData } = req.body;

const portPromises: Array<Promise<Required<RequestPortsData>>> = [];

portData.forEach(data => {
const { port, instanceId } = data;
if (this.serverPortMap[instanceId]) {
res.status(409).send(
i18n(`errors.${i18nKey}.409`, {
instanceId,
port: this.serverPortMap[instanceId],
})
);
return;
} else if (port && (port < MIN_PORT_NUMBER || port > MAX_PORT_NUMBER)) {
res.status(400).send(i18n(`errors.${i18nKey}.400`));
return;
} else {
const promise = new Promise<Required<RequestPortsData>>(resolve => {
detectPort(port).then(resolvedPort => {
resolve({
instanceId,
port: resolvedPort,
});
});
});
portPromises.push(promise);
}
});

const ports = await Promise.all(portPromises);

ports.forEach(({ port, instanceId }) => {
this.setPort(instanceId, port);
});

res.send({ ports });
};

deleteServerInstance = (req: Request, res: Response): void => {
const { instanceId } = req.params;
const port = this.serverPortMap[instanceId];

if (port) {
this.deletePort(instanceId);
res.send(200);
} else {
this.send404(res, instanceId);
}
};

closeServer = (req: Request, res: Response): void => {
if (this.server) {
debug(`${i18nKey}.close`);
res.send(200);
this.server.close();
}
};
}

export default new PortManagerServer();
87 changes: 87 additions & 0 deletions utils/detectPort.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import net, { AddressInfo } from 'net';
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to include the copyright info in here per the license: https://github.com/node-modules/detect-port/blob/master/LICENSE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I could have sworn I had already done this

import { ip } from 'address';

import { MIN_PORT_NUMBER, MAX_PORT_NUMBER } from '../constants/ports';

type NetError = Error & {
code: string;
};

type ListenCallback = (error: NetError | null, port: number) => void;

export function detectPort(port?: number | null): Promise<number> {
if (port && (port < MIN_PORT_NUMBER || port > MAX_PORT_NUMBER)) {
throw new Error('Port must be between 1024 and 65535');
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put this copy in the lang file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good call, I guess I'll hold off on that until the lang stuff is merged

}

const portToUse = port || 0;
const maxPort = Math.min(portToUse + 10, MAX_PORT_NUMBER);

return new Promise(resolve => {
tryListen(portToUse, maxPort, (_, resolvedPort) => {
resolve(resolvedPort);
});
});
}

function tryListen(port: number, maxPort: number, callback: ListenCallback) {
const shouldGiveUp = port >= maxPort;
const nextPort = shouldGiveUp ? 0 : port + 1;
const nextMaxPort = shouldGiveUp ? 0 : maxPort;

listen(port, undefined, (err, realPort) => {
// ignore random listening
if (port === 0) {
return callback(err, realPort);
}

if (err) {
return tryListen(nextPort, nextMaxPort, callback);
}

// 2. check 0.0.0.0
listen(port, '0.0.0.0', err => {
if (err) {
return tryListen(nextPort, nextMaxPort, callback);
}

// 3. check localhost
listen(port, 'localhost', err => {
if (err && err.code !== 'EADDRNOTAVAIL') {
return tryListen(nextPort, nextMaxPort, callback);
}

// 4. check current ip
listen(port, ip(), (err, realPort) => {
if (err) {
return tryListen(nextPort, nextMaxPort, callback);
}

callback(null, realPort);
});
});
});
});
}

function listen(
port: number,
hostname: string | undefined,
callback: ListenCallback
): void {
const server = new net.Server();

server.on('error', (err: NetError) => {
server.close();
if (err.code === 'ENOTFOUND') {
return callback(null, port);
}
return callback(err, 0);
});

server.listen(port, hostname, () => {
const addressInfo = server.address() as AddressInfo;
server.close();
return callback(null, addressInfo.port);
});
}