From e0cde7ef07601772935d2897d944f8731db183f9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 11:14:49 +0800 Subject: [PATCH 01/16] fix lint warnings by reordering code --- .../@uppy/companion/src/standalone/helper.js | 76 +++++++++---------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/packages/@uppy/companion/src/standalone/helper.js b/packages/@uppy/companion/src/standalone/helper.js index 5d8df6b524..6f2a27744c 100644 --- a/packages/@uppy/companion/src/standalone/helper.js +++ b/packages/@uppy/companion/src/standalone/helper.js @@ -9,13 +9,28 @@ const logger = require('../server/logger') const { version } = require('../../package.json') /** - * Reads all companion configuration set via environment variables - * and via the config file path + * Tries to read the secret from a file if the according environment variable is set. + * Otherwise it falls back to the standard secret environment variable. * - * @returns {object} + * @param {string} baseEnvVar + * + * @returns {string} */ -exports.getCompanionOptions = (options = {}) => { - return merge({}, getConfigFromEnv(), getConfigFromFile(), options) +const getSecret = (baseEnvVar) => { + const secretFile = process.env[`${baseEnvVar}_FILE`] + return secretFile + ? fs.readFileSync(secretFile).toString() + : process.env[baseEnvVar] +} + +/** + * Auto-generates server secret + * + * @returns {string} + */ +const generateSecret = () => { + logger.warn('auto-generating server secret because none was specified', 'startup.secret') + return crypto.randomBytes(64).toString('hex') } /** @@ -119,28 +134,23 @@ const getConfigFromEnv = () => { } /** - * Tries to read the secret from a file if the according environment variable is set. - * Otherwise it falls back to the standard secret environment variable. - * - * @param {string} baseEnvVar + * Returns the config path specified via cli arguments * * @returns {string} */ -const getSecret = (baseEnvVar) => { - const secretFile = process.env[`${baseEnvVar}_FILE`] - return secretFile - ? fs.readFileSync(secretFile).toString() - : process.env[baseEnvVar] -} +const getConfigPath = () => { + let configPath -/** - * Auto-generates server secret - * - * @returns {string} - */ -const generateSecret = () => { - logger.warn('auto-generating server secret because none was specified', 'startup.secret') - return crypto.randomBytes(64).toString('hex') + for (let i = process.argv.length - 1; i >= 0; i--) { + const isConfigFlag = process.argv[i] === '-c' || process.argv[i] === '--config' + const flagHasValue = i + 1 <= process.argv.length + if (isConfigFlag && flagHasValue) { + configPath = process.argv[i + 1] + break + } + } + + return configPath } /** @@ -158,23 +168,13 @@ const getConfigFromFile = () => { } /** - * Returns the config path specified via cli arguments + * Reads all companion configuration set via environment variables + * and via the config file path * - * @returns {string} + * @returns {object} */ -const getConfigPath = () => { - let configPath - - for (let i = process.argv.length - 1; i >= 0; i--) { - const isConfigFlag = process.argv[i] === '-c' || process.argv[i] === '--config' - const flagHasValue = i + 1 <= process.argv.length - if (isConfigFlag && flagHasValue) { - configPath = process.argv[i + 1] - break - } - } - - return configPath +exports.getCompanionOptions = (options = {}) => { + return merge({}, getConfigFromEnv(), getConfigFromFile(), options) } /** From 68ae43ed4db010aba77a53d807eba490bc714f86 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 11:19:45 +0800 Subject: [PATCH 02/16] fix lint and refactor --- packages/@uppy/companion/src/standalone/index.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index 649826d243..e2848027bd 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -11,7 +11,7 @@ const connectRedis = require('connect-redis') const logger = require('../server/logger') const redis = require('../server/redis') const companion = require('../companion') -const helper = require('./helper') +const { hasProtocol, getCompanionOptions, buildHelpfulStartupMessage } = require('./helper') /** * Configures an Express app for running Companion standalone @@ -23,13 +23,13 @@ module.exports = function server (inputCompanionOptions = {}) { if (process.env.COMPANION_CLIENT_ORIGINS) { corsOrigins = process.env.COMPANION_CLIENT_ORIGINS .split(',') - .map((url) => (helper.hasProtocol(url) ? url : `${process.env.COMPANION_PROTOCOL || 'http'}://${url}`)) + .map((url) => (hasProtocol(url) ? url : `${process.env.COMPANION_PROTOCOL || 'http'}://${url}`)) } else if (process.env.COMPANION_CLIENT_ORIGINS_REGEX) { corsOrigins = new RegExp(process.env.COMPANION_CLIENT_ORIGINS_REGEX) } const moreCompanionOptions = { ...inputCompanionOptions, corsOrigins } - const companionOptions = helper.getCompanionOptions(moreCompanionOptions) + const companionOptions = getCompanionOptions(moreCompanionOptions) const app = express() @@ -98,6 +98,7 @@ module.exports = function server (inputCompanionOptions = {}) { const { query, censored } = censorQuery(rawQuery) return censored ? `${parsed.href.split('?')[0]}?${qs.stringify(query)}` : parsed.href } + return undefined }) router.use(bodyParser.json()) @@ -140,7 +141,7 @@ module.exports = function server (inputCompanionOptions = {}) { if (process.env.COMPANION_HIDE_WELCOME !== 'true') { router.get('/', (req, res) => { res.setHeader('Content-Type', 'text/plain') - res.send(helper.buildHelpfulStartupMessage(companionOptions)) + res.send(buildHelpfulStartupMessage(companionOptions)) }) } From 75710d058358a150c0ee6d5d1787317b8a647ada Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 11:31:27 +0800 Subject: [PATCH 03/16] move corsOrigins config into helper --- .../@uppy/companion/src/standalone/helper.js | 29 ++++++++++++++----- .../@uppy/companion/src/standalone/index.js | 16 ++-------- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/packages/@uppy/companion/src/standalone/helper.js b/packages/@uppy/companion/src/standalone/helper.js index 6f2a27744c..2ba0407d03 100644 --- a/packages/@uppy/companion/src/standalone/helper.js +++ b/packages/@uppy/companion/src/standalone/helper.js @@ -33,6 +33,26 @@ const generateSecret = () => { return crypto.randomBytes(64).toString('hex') } +/** + * + * @param {string} url + */ +const hasProtocol = (url) => { + return url.startsWith('http://') || url.startsWith('https://') +} + +function getCorsOrigins () { + if (process.env.COMPANION_CLIENT_ORIGINS) { + return process.env.COMPANION_CLIENT_ORIGINS + .split(',') + .map((url) => (hasProtocol(url) ? url : `${process.env.COMPANION_PROTOCOL || 'http'}://${url}`)) + } + if (process.env.COMPANION_CLIENT_ORIGINS_REGEX) { + return new RegExp(process.env.COMPANION_CLIENT_ORIGINS_REGEX) + } + return undefined +} + /** * Loads the config from environment variables * @@ -130,6 +150,7 @@ const getConfigFromEnv = () => { clientSocketConnectTimeout: process.env.COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT ? parseInt(process.env.COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT, 10) : undefined, metrics: process.env.COMPANION_HIDE_METRICS !== 'true', + corsOrigins: getCorsOrigins(), } } @@ -177,14 +198,6 @@ exports.getCompanionOptions = (options = {}) => { return merge({}, getConfigFromEnv(), getConfigFromFile(), options) } -/** - * - * @param {string} url - */ -exports.hasProtocol = (url) => { - return url.startsWith('http://') || url.startsWith('https://') -} - exports.buildHelpfulStartupMessage = (companionOptions) => { const buildURL = utils.getURLBuilder(companionOptions) const callbackURLs = [] diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index e2848027bd..f7b50c8600 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -11,25 +11,15 @@ const connectRedis = require('connect-redis') const logger = require('../server/logger') const redis = require('../server/redis') const companion = require('../companion') -const { hasProtocol, getCompanionOptions, buildHelpfulStartupMessage } = require('./helper') +const { getCompanionOptions, buildHelpfulStartupMessage } = require('./helper') /** * Configures an Express app for running Companion standalone * * @returns {object} */ -module.exports = function server (inputCompanionOptions = {}) { - let corsOrigins - if (process.env.COMPANION_CLIENT_ORIGINS) { - corsOrigins = process.env.COMPANION_CLIENT_ORIGINS - .split(',') - .map((url) => (hasProtocol(url) ? url : `${process.env.COMPANION_PROTOCOL || 'http'}://${url}`)) - } else if (process.env.COMPANION_CLIENT_ORIGINS_REGEX) { - corsOrigins = new RegExp(process.env.COMPANION_CLIENT_ORIGINS_REGEX) - } - - const moreCompanionOptions = { ...inputCompanionOptions, corsOrigins } - const companionOptions = getCompanionOptions(moreCompanionOptions) +module.exports = function server (inputCompanionOptions) { + const companionOptions = getCompanionOptions(inputCompanionOptions) const app = express() From 9c2b6ddc1fa7c2fc2e5a1a74ff7c8d1b9af15765 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 11:33:24 +0800 Subject: [PATCH 04/16] allow customising logger name "companion" --- packages/@uppy/companion/src/companion.js | 8 +++++ packages/@uppy/companion/src/server/logger.js | 34 +++++++++++-------- .../@uppy/companion/src/standalone/helper.js | 7 ++-- .../@uppy/companion/src/standalone/index.js | 7 +++- website/src/docs/companion.md | 2 ++ 5 files changed, 39 insertions(+), 19 deletions(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index 03ca93e7e1..04a7d03a46 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -21,6 +21,10 @@ const { getCredentialsOverrideMiddleware } = require('./server/provider/credenti // @ts-ignore const { version } = require('../package.json') +function setLoggerProcessName ({ loggerProcessName }) { + if (loggerProcessName != null) logger.setProcessName(loggerProcessName) +} + // intercepts grantJS' default response error when something goes // wrong during oauth process. const interceptGrantErrorResponse = interceptor((req, res) => { @@ -51,6 +55,8 @@ const interceptGrantErrorResponse = interceptor((req, res) => { module.exports.errors = { ProviderApiError, ProviderAuthError } module.exports.socket = require('./server/socket') +module.exports.setLoggerProcessName = setLoggerProcessName + /** * Entry point into initializing the Companion app. * @@ -58,6 +64,8 @@ module.exports.socket = require('./server/socket') * @returns {{ app: import('express').Express, emitter: any }}} */ module.exports.app = (optionsArg = {}) => { + setLoggerProcessName(optionsArg) + validateConfig(optionsArg) const options = merge({}, defaultOptions, optionsArg) diff --git a/packages/@uppy/companion/src/server/logger.js b/packages/@uppy/companion/src/server/logger.js index c10b753894..112a35555b 100644 --- a/packages/@uppy/companion/src/server/logger.js +++ b/packages/@uppy/companion/src/server/logger.js @@ -33,18 +33,25 @@ function maskMessage (msg) { return out } +let processName = 'companion' + +exports.setProcessName = (newProcessName) => { + processName = newProcessName +} + /** * message log * - * @param {string | Error} arg the message or error to log - * @param {string} tag a unique tag to easily search for this message - * @param {string} level error | info | debug - * @param {string} [id] a unique id to easily trace logs tied to a request - * @param {Function} [color] function to display the log in appropriate color + * @param {object} params + * @param {string | Error} params.arg the message or error to log + * @param {string} params.tag a unique tag to easily search for this message + * @param {string} params.level error | info | debug + * @param {string} [params.traceId] a unique id to easily trace logs tied to a request + * @param {Function} [params.color] function to display the log in appropriate color */ -const log = (arg, tag = '', level, id = '', color = (message) => message) => { +const log = ({ arg, tag = '', level, traceId = '', color = (message) => message }) => { const time = new Date().toISOString() - const whitespace = tag && id ? ' ' : '' + const whitespace = tag && traceId ? ' ' : '' function msgToString () { // We don't need to log stack trace on special errors that we ourselves have produced @@ -59,7 +66,7 @@ const log = (arg, tag = '', level, id = '', color = (message) => message) => { const msgString = msgToString() const masked = maskMessage(msgString) // eslint-disable-next-line no-console - console.log(color(`companion: ${time} [${level}] ${id}${whitespace}${tag}`), color(masked)) + console.log(color(`${processName}: ${time} [${level}] ${traceId}${whitespace}${tag}`), color(masked)) } /** @@ -70,7 +77,7 @@ const log = (arg, tag = '', level, id = '', color = (message) => message) => { * @param {string} [traceId] a unique id to easily trace logs tied to a request */ exports.info = (msg, tag, traceId) => { - log(msg, tag, 'info', traceId) + log({ arg: msg, tag, level: 'info', traceId }) } /** @@ -81,8 +88,7 @@ exports.info = (msg, tag, traceId) => { * @param {string} [traceId] a unique id to easily trace logs tied to a request */ exports.warn = (msg, tag, traceId) => { - // @ts-ignore - log(msg, tag, 'warn', traceId, chalk.bold.yellow) + log({ arg: msg, tag, level: 'warn', traceId, color: chalk.bold.yellow }) } /** @@ -93,8 +99,7 @@ exports.warn = (msg, tag, traceId) => { * @param {string} [traceId] a unique id to easily trace logs tied to a request */ exports.error = (msg, tag, traceId) => { - // @ts-ignore - log(msg, tag, 'error', traceId, chalk.bold.red) + log({ arg: msg, tag, level: 'error', traceId, color: chalk.bold.red }) } /** @@ -106,7 +111,6 @@ exports.error = (msg, tag, traceId) => { */ exports.debug = (msg, tag, traceId) => { if (process.env.NODE_ENV !== 'production') { - // @ts-ignore - log(msg, tag, 'debug', traceId, chalk.bold.blue) + log({ arg: msg, tag, level: 'debug', traceId, color: chalk.bold.blue }) } } diff --git a/packages/@uppy/companion/src/standalone/helper.js b/packages/@uppy/companion/src/standalone/helper.js index 2ba0407d03..cf9a862c92 100644 --- a/packages/@uppy/companion/src/standalone/helper.js +++ b/packages/@uppy/companion/src/standalone/helper.js @@ -28,7 +28,7 @@ const getSecret = (baseEnvVar) => { * * @returns {string} */ -const generateSecret = () => { +exports.generateSecret = () => { logger.warn('auto-generating server secret because none was specified', 'startup.secret') return crypto.randomBytes(64).toString('hex') } @@ -139,8 +139,8 @@ const getConfigFromEnv = () => { redisOptions: {}, sendSelfEndpoint: process.env.COMPANION_SELF_ENDPOINT, uploadUrls: uploadUrls ? uploadUrls.split(',') : null, - secret: getSecret('COMPANION_SECRET') || generateSecret(), - preAuthSecret: getSecret('COMPANION_PREAUTH_SECRET') || generateSecret(), + secret: getSecret('COMPANION_SECRET'), + preAuthSecret: getSecret('COMPANION_PREAUTH_SECRET'), allowLocalUrls: process.env.COMPANION_ALLOW_LOCAL_URLS === 'true', // cookieDomain is kind of a hack to support distributed systems. This should be improved but we never got so far. cookieDomain: process.env.COMPANION_COOKIE_DOMAIN, @@ -150,6 +150,7 @@ const getConfigFromEnv = () => { clientSocketConnectTimeout: process.env.COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT ? parseInt(process.env.COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT, 10) : undefined, metrics: process.env.COMPANION_HIDE_METRICS !== 'true', + loggerProcessName: process.env.COMPANION_LOGGER_PROCESS_NAME, corsOrigins: getCorsOrigins(), } } diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index f7b50c8600..e4bdd7ee96 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -11,7 +11,7 @@ const connectRedis = require('connect-redis') const logger = require('../server/logger') const redis = require('../server/redis') const companion = require('../companion') -const { getCompanionOptions, buildHelpfulStartupMessage } = require('./helper') +const { getCompanionOptions, generateSecret, buildHelpfulStartupMessage } = require('./helper') /** * Configures an Express app for running Companion standalone @@ -21,6 +21,11 @@ const { getCompanionOptions, buildHelpfulStartupMessage } = require('./helper') module.exports = function server (inputCompanionOptions) { const companionOptions = getCompanionOptions(inputCompanionOptions) + companion.setLoggerProcessName(companionOptions) + + if (!companionOptions.secret) companionOptions.secret = generateSecret() + if (!companionOptions.preAuthSecret) companionOptions.preAuthSecret = generateSecret() + const app = express() const router = express.Router() diff --git a/website/src/docs/companion.md b/website/src/docs/companion.md index 9d6624b68c..2fe90f374a 100644 --- a/website/src/docs/companion.md +++ b/website/src/docs/companion.md @@ -191,6 +191,8 @@ export COMPANION_PATH="/SERVER/PATH/TO/WHERE/COMPANION/LIVES" export COMPANION_HIDE_WELCOME="true" # disables the metrics page, defaults to false export COMPANION_HIDE_METRICS="true" +# prefix all log entries with this value - useful for multiple instances +export COMPANION_LOGGER_PROCESS_NAME="companion" # use this in place of COMPANION_PATH if the server path should not be # handled by the express.js app, but maybe by an external server configuration From 5b7cbe9c2f4f7166ce49f7df3dc8bb673382e0fa Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 11:34:03 +0800 Subject: [PATCH 05/16] fix lint config --- .eslintrc.js | 2 +- e2e/cypress.config.mjs | 5 +---- e2e/generate-test.mjs | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 9d65711980..fe884c96a2 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -518,7 +518,7 @@ module.exports = { extends: ['plugin:cypress/recommended'], }, { - files: ['e2e/**/*.ts', 'e2e/**/*.js', 'e2e/**/*.jsx'], + files: ['e2e/**/*.ts', 'e2e/**/*.js', 'e2e/**/*.jsx', 'e2e/**/*.mjs'], rules: { 'import/no-extraneous-dependencies': 'off', 'no-unused-expressions': 'off' }, }, ], diff --git a/e2e/cypress.config.mjs b/e2e/cypress.config.mjs index 4873b7b451..9932263602 100644 --- a/e2e/cypress.config.mjs +++ b/e2e/cypress.config.mjs @@ -1,6 +1,4 @@ -// eslint-disable-next-line import/no-extraneous-dependencies import { defineConfig } from 'cypress' -// eslint-disable-next-line import/no-extraneous-dependencies import installLogsPrinter from 'cypress-terminal-report/src/installLogsPrinter.js' export default defineConfig({ @@ -10,8 +8,7 @@ export default defineConfig({ baseUrl: 'http://localhost:1234', specPattern: 'cypress/integration/*.spec.ts', - // eslint-disable-next-line no-unused-vars - setupNodeEvents (on, config) { + setupNodeEvents (on) { // implement node event listeners here installLogsPrinter(on) }, diff --git a/e2e/generate-test.mjs b/e2e/generate-test.mjs index 99b06dea6f..ba3a0ffa68 100755 --- a/e2e/generate-test.mjs +++ b/e2e/generate-test.mjs @@ -1,5 +1,4 @@ #!/usr/bin/env node -/* eslint-disable no-console, import/no-extraneous-dependencies */ import prompts from 'prompts' import fs from 'node:fs/promises' From 2a95405bd28f7c5b6b32b74b5e9841ff79d338bf Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 12:36:32 +0800 Subject: [PATCH 06/16] disable console lint warn for e2e --- .eslintrc.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index fe884c96a2..7419c75a3b 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -519,7 +519,7 @@ module.exports = { }, { files: ['e2e/**/*.ts', 'e2e/**/*.js', 'e2e/**/*.jsx', 'e2e/**/*.mjs'], - rules: { 'import/no-extraneous-dependencies': 'off', 'no-unused-expressions': 'off' }, + rules: { 'import/no-extraneous-dependencies': 'off', 'no-unused-expressions': 'off', 'no-console': 'off' }, }, ], } From ba3a527d91ec278182eb5c5cc82f156cda6ba258 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 12:37:22 +0800 Subject: [PATCH 07/16] remove unused env var --- .github/workflows/e2e.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index f9c67410e5..099d389a90 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -80,7 +80,6 @@ jobs: COMPANION_AWS_SECRET: ${{secrets.COMPANION_AWS_SECRET}} COMPANION_AWS_BUCKET: ${{secrets.COMPANION_AWS_BUCKET}} COMPANION_AWS_REGION: ${{secrets.COMPANION_AWS_REGION}} - COMPANION_AWS_DISABLE_ACL: 'true' # https://docs.cypress.io/guides/references/advanced-installation#Binary-cache CYPRESS_CACHE_FOLDER: ${{ steps.cypress-cache-dir-path.outputs.dir }} - name: Upload videos in case of failure From 7e718315c1cae940fd56d248ba08de39a8855055 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 12:38:02 +0800 Subject: [PATCH 08/16] log startup message using logger too --- packages/@uppy/companion/src/standalone/start-server.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/companion/src/standalone/start-server.js b/packages/@uppy/companion/src/standalone/start-server.js index 18b17a6db8..6fc3659c68 100755 --- a/packages/@uppy/companion/src/standalone/start-server.js +++ b/packages/@uppy/companion/src/standalone/start-server.js @@ -3,6 +3,7 @@ const companion = require('../companion') // @ts-ignore const { version } = require('../../package.json') const standalone = require('.') +const logger = require('../server/logger') const { getURLBuilder } = require('../server/helpers/utils') const port = process.env.COMPANION_PORT || process.env.PORT || 3020 @@ -13,6 +14,5 @@ companion.socket(app.listen(port)) const buildURL = getURLBuilder(companionOptions) -/* eslint-disable no-console */ -console.log(`Welcome to Companion! v${version}`) -console.log(`Listening on ${buildURL('/', false)}`) +logger.info(`Welcome to Companion! v${version}`) +logger.info(`Listening on ${buildURL('/', false)}`) From ccbad8e492a51502f3173a50d7f1979ce7d291cf Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 12:38:43 +0800 Subject: [PATCH 09/16] use load balancer for companion in e2e tests --- .github/workflows/e2e.yml | 7 +++ e2e/package.json | 1 + e2e/start-companion-with-load-balancer.mjs | 70 ++++++++++++++++++++++ package.json | 5 +- yarn.lock | 1 + 5 files changed, 82 insertions(+), 2 deletions(-) create mode 100755 e2e/start-companion-with-load-balancer.mjs diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 099d389a90..e1d62b0249 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -59,6 +59,12 @@ jobs: uses: actions/setup-node@v3 with: node-version: lts/* + + - name: Start Redis + uses: supercharge/redis-github-action@1.4.0 + with: + redis-version: 7 + - name: Install dependencies run: corepack yarn install --immutable env: @@ -76,6 +82,7 @@ jobs: VITE_TRANSLOADIT_SECRET: ${{secrets.TRANSLOADIT_SECRET}} VITE_TRANSLOADIT_TEMPLATE: ${{secrets.TRANSLOADIT_TEMPLATE}} VITE_TRANSLOADIT_SERVICE_URL: ${{secrets.TRANSLOADIT_SERVICE_URL}} + COMPANION_REDIS_URL: redis://localhost:6379 COMPANION_AWS_KEY: ${{secrets.COMPANION_AWS_KEY}} COMPANION_AWS_SECRET: ${{secrets.COMPANION_AWS_SECRET}} COMPANION_AWS_BUCKET: ${{secrets.COMPANION_AWS_BUCKET}} diff --git a/e2e/package.json b/e2e/package.json index 92df911f03..2fe1ef2582 100644 --- a/e2e/package.json +++ b/e2e/package.json @@ -48,6 +48,7 @@ "cypress": "^10.0.0", "cypress-terminal-report": "^4.1.2", "deep-freeze": "^0.0.1", + "execa": "^6.1.0", "parcel": "^2.0.1", "prompts": "^2.4.2", "react": "^18.1.0", diff --git a/e2e/start-companion-with-load-balancer.mjs b/e2e/start-companion-with-load-balancer.mjs new file mode 100755 index 0000000000..936e07cee2 --- /dev/null +++ b/e2e/start-companion-with-load-balancer.mjs @@ -0,0 +1,70 @@ +#!/usr/bin/env node + +import { execa } from 'execa' +import http from 'node:http' +import httpProxy from 'http-proxy' + +const numInstances = 3 +const lbPort = 3020 +const companionStartPort = 3021 + +// simple load balancer that will direct requests round robin between companion instances +function createLoadBalancer (baseUrls) { + const proxy = httpProxy.createProxyServer({ ws: true }) + + let i = 0 + + function getTarget () { + return baseUrls[i % baseUrls.length] + } + + const server = http.createServer((req, res) => { + const target = getTarget() + // console.log('req', req.method, target, req.url) + proxy.web(req, res, { target }) + i++ + }) + + server.on('upgrade', (req, socket, head) => { + const target = getTarget() + // console.log('upgrade', target, req.url) + proxy.ws(req, socket, head, { target }) + i++ + }) + + server.listen(lbPort) + console.log('Load balancer listening', lbPort) + return server +} + +const startCompanion = ({ name, port }) => execa('nodemon', [ + '--watch', 'packages/@uppy/companion/src', '--exec', 'node', '-r', 'dotenv/config', './packages/@uppy/companion/src/standalone/start-server.js', +], { + stdio: 'inherit', + env: { + // Note: these env variables will override anything set in .env + COMPANION_PORT: port, + COMPANION_SECRET: 'development', // multi instance will not work without secret set + COMPANION_PREAUTH_SECRET: 'development', // multi instance will not work without secret set + COMPANION_ALLOW_LOCAL_URLS: 'true', + COMPANION_LOGGER_PROCESS_NAME: name, + }, +}) + +const hosts = Array(numInstances).fill().map((unused, index) => { + const port = companionStartPort + index + return { index, port } +}) + +console.log('Starting companion instances on ports', hosts.map(({ port }) => port)) + +const companions = hosts.map(({ index, port }) => startCompanion({ name: `companion${index}`, port })) + +let loadBalancer +try { + loadBalancer = createLoadBalancer(hosts.map(({ port }) => `http://localhost:${port}`)) + await Promise.all(companions) +} finally { + if (loadBalancer) loadBalancer.close() + companions.forEach((companion) => companion.kill()) +} diff --git a/package.json b/package.json index 3e7e69879e..ac8aa55890 100644 --- a/package.json +++ b/package.json @@ -130,10 +130,11 @@ "release": "PACKAGES=$(yarn workspaces list --json) yarn workspace @uppy-dev/release interactive", "size": "echo 'JS Bundle mingz:' && cat ./packages/uppy/dist/uppy.min.js | gzip | wc -c && echo 'CSS Bundle mingz:' && cat ./packages/uppy/dist/uppy.min.css | gzip | wc -c", "start:companion": "bash bin/companion.sh", + "start:companion:with-loadbalancer": "e2e/start-companion-with-load-balancer.mjs", "start": "npm-run-all --parallel watch start:companion web:start", "e2e": "yarn build && yarn e2e:skip-build", - "e2e:skip-build": "npm-run-all --parallel watch:js:lib e2e:client start:companion e2e:cypress", - "e2e:ci": "start-server-and-test 'npm-run-all --parallel e2e:client start:companion' '1234|3020' e2e:headless", + "e2e:skip-build": "npm-run-all --parallel watch:js:lib e2e:client start:companion:with-loadbalancer e2e:cypress", + "e2e:ci": "start-server-and-test 'npm-run-all --parallel e2e:client start:companion:with-loadbalancer' '1234|3020' e2e:headless", "e2e:client": "yarn workspace e2e client:start", "e2e:cypress": "yarn workspace e2e cypress:open", "e2e:headless": "yarn workspace e2e cypress:headless", diff --git a/yarn.lock b/yarn.lock index 5bab180a0f..e514b25d80 100644 --- a/yarn.lock +++ b/yarn.lock @@ -15144,6 +15144,7 @@ __metadata: cypress: ^10.0.0 cypress-terminal-report: ^4.1.2 deep-freeze: ^0.0.1 + execa: ^6.1.0 parcel: ^2.0.1 prompts: ^2.4.2 react: ^18.1.0 From 9468a5aa75fde97cf65b6d4ad30a0cda5358cd28 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 12:49:50 +0800 Subject: [PATCH 10/16] add missing env vars also reorder/group vars --- .github/workflows/e2e.yml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index e1d62b0249..3782b8b476 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -75,18 +75,21 @@ jobs: - name: Run end-to-end browser tests run: corepack yarn run e2e:ci env: + COMPANION_DATADIR: ./output + COMPANION_DOMAIN: localhost:3020 + COMPANION_PROTOCOL: http + COMPANION_REDIS_URL: redis://localhost:6379 COMPANION_UNSPLASH_KEY: ${{secrets.COMPANION_UNSPLASH_KEY}} COMPANION_UNSPLASH_SECRET: ${{secrets.COMPANION_UNSPLASH_SECRET}} + COMPANION_AWS_KEY: ${{secrets.COMPANION_AWS_KEY}} + COMPANION_AWS_SECRET: ${{secrets.COMPANION_AWS_SECRET}} + COMPANION_AWS_BUCKET: ${{secrets.COMPANION_AWS_BUCKET}} + COMPANION_AWS_REGION: ${{secrets.COMPANION_AWS_REGION}} VITE_COMPANION_URL: http://localhost:3020 VITE_TRANSLOADIT_KEY: ${{secrets.TRANSLOADIT_KEY}} VITE_TRANSLOADIT_SECRET: ${{secrets.TRANSLOADIT_SECRET}} VITE_TRANSLOADIT_TEMPLATE: ${{secrets.TRANSLOADIT_TEMPLATE}} VITE_TRANSLOADIT_SERVICE_URL: ${{secrets.TRANSLOADIT_SERVICE_URL}} - COMPANION_REDIS_URL: redis://localhost:6379 - COMPANION_AWS_KEY: ${{secrets.COMPANION_AWS_KEY}} - COMPANION_AWS_SECRET: ${{secrets.COMPANION_AWS_SECRET}} - COMPANION_AWS_BUCKET: ${{secrets.COMPANION_AWS_BUCKET}} - COMPANION_AWS_REGION: ${{secrets.COMPANION_AWS_REGION}} # https://docs.cypress.io/guides/references/advanced-installation#Binary-cache CYPRESS_CACHE_FOLDER: ${{ steps.cypress-cache-dir-path.outputs.dir }} - name: Upload videos in case of failure From 3b32e0e82bc2762e247ef820fb84fd783c92bf4a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 13:18:43 +0800 Subject: [PATCH 11/16] fix incorrect log --- packages/@uppy/companion/src/standalone/start-server.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/@uppy/companion/src/standalone/start-server.js b/packages/@uppy/companion/src/standalone/start-server.js index 6fc3659c68..2bc25214b1 100755 --- a/packages/@uppy/companion/src/standalone/start-server.js +++ b/packages/@uppy/companion/src/standalone/start-server.js @@ -4,15 +4,12 @@ const companion = require('../companion') const { version } = require('../../package.json') const standalone = require('.') const logger = require('../server/logger') -const { getURLBuilder } = require('../server/helpers/utils') const port = process.env.COMPANION_PORT || process.env.PORT || 3020 -const { app, companionOptions } = standalone() +const { app } = standalone() companion.socket(app.listen(port)) -const buildURL = getURLBuilder(companionOptions) - logger.info(`Welcome to Companion! v${version}`) -logger.info(`Listening on ${buildURL('/', false)}`) +logger.info(`Listening on http://localhost:${port}`) From eb713a23cefbcef1bb70319628a1f128efb1d1dd Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 13:24:54 +0800 Subject: [PATCH 12/16] make load balancer robust to errors --- e2e/start-companion-with-load-balancer.mjs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/e2e/start-companion-with-load-balancer.mjs b/e2e/start-companion-with-load-balancer.mjs index 936e07cee2..f34b6117d2 100755 --- a/e2e/start-companion-with-load-balancer.mjs +++ b/e2e/start-companion-with-load-balancer.mjs @@ -21,14 +21,22 @@ function createLoadBalancer (baseUrls) { const server = http.createServer((req, res) => { const target = getTarget() // console.log('req', req.method, target, req.url) - proxy.web(req, res, { target }) + proxy.web(req, res, { target }, (err) => { + console.error('Load balancer faield to proxy request', err.message) + res.statusCode = 500 + res.end() + }) i++ }) server.on('upgrade', (req, socket, head) => { const target = getTarget() // console.log('upgrade', target, req.url) - proxy.ws(req, socket, head, { target }) + proxy.ws(req, socket, head, { target }, (err) => { + console.error('Load balancer faield to proxy websocket', err.message) + console.error(err) + socket.destroy() + }) i++ }) From 5c0967fcdea7036517d0c480a6ff7b005e67d5b6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 22 Nov 2022 13:34:53 +0800 Subject: [PATCH 13/16] fix typo --- e2e/start-companion-with-load-balancer.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/start-companion-with-load-balancer.mjs b/e2e/start-companion-with-load-balancer.mjs index f34b6117d2..6503372138 100755 --- a/e2e/start-companion-with-load-balancer.mjs +++ b/e2e/start-companion-with-load-balancer.mjs @@ -22,7 +22,7 @@ function createLoadBalancer (baseUrls) { const target = getTarget() // console.log('req', req.method, target, req.url) proxy.web(req, res, { target }, (err) => { - console.error('Load balancer faield to proxy request', err.message) + console.error('Load balancer failed to proxy request', err.message) res.statusCode = 500 res.end() }) @@ -33,7 +33,7 @@ function createLoadBalancer (baseUrls) { const target = getTarget() // console.log('upgrade', target, req.url) proxy.ws(req, socket, head, { target }, (err) => { - console.error('Load balancer faield to proxy websocket', err.message) + console.error('Load balancer failed to proxy websocket', err.message) console.error(err) socket.destroy() }) From aee3d757b05983b95b57f0bd85e2e6f5d7b9bc7b Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 5 Dec 2022 17:23:08 +0800 Subject: [PATCH 14/16] Apply suggestions from code review Co-authored-by: Antoine du Hamel --- e2e/start-companion-with-load-balancer.mjs | 4 ++-- packages/@uppy/companion/src/standalone/helper.js | 2 +- packages/@uppy/companion/src/standalone/index.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/e2e/start-companion-with-load-balancer.mjs b/e2e/start-companion-with-load-balancer.mjs index 6503372138..53777340a8 100755 --- a/e2e/start-companion-with-load-balancer.mjs +++ b/e2e/start-companion-with-load-balancer.mjs @@ -59,7 +59,7 @@ const startCompanion = ({ name, port }) => execa('nodemon', [ }, }) -const hosts = Array(numInstances).fill().map((unused, index) => { +const hosts = Array.from({ length: numInstances }, (_, index) => { const port = companionStartPort + index return { index, port } }) @@ -73,6 +73,6 @@ try { loadBalancer = createLoadBalancer(hosts.map(({ port }) => `http://localhost:${port}`)) await Promise.all(companions) } finally { - if (loadBalancer) loadBalancer.close() + loadBalancer?.close() companions.forEach((companion) => companion.kill()) } diff --git a/packages/@uppy/companion/src/standalone/helper.js b/packages/@uppy/companion/src/standalone/helper.js index cf9a862c92..4b90eda6a0 100644 --- a/packages/@uppy/companion/src/standalone/helper.js +++ b/packages/@uppy/companion/src/standalone/helper.js @@ -38,7 +38,7 @@ exports.generateSecret = () => { * @param {string} url */ const hasProtocol = (url) => { - return url.startsWith('http://') || url.startsWith('https://') + return url.startsWith('https://') || url.startsWith('http://') } function getCorsOrigins () { diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index e4bdd7ee96..92222310b3 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -23,8 +23,8 @@ module.exports = function server (inputCompanionOptions) { companion.setLoggerProcessName(companionOptions) - if (!companionOptions.secret) companionOptions.secret = generateSecret() - if (!companionOptions.preAuthSecret) companionOptions.preAuthSecret = generateSecret() + companionOptions.secret ||= generateSecret() + companionOptions.preAuthSecret ||= generateSecret() const app = express() From 2c0ef593b9fa44788dd8fbc91892798cc586ebc1 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 5 Dec 2022 17:26:46 +0800 Subject: [PATCH 15/16] revert modern syntax doesn't seem to work on node 14 --- packages/@uppy/companion/src/standalone/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index 92222310b3..e4bdd7ee96 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -23,8 +23,8 @@ module.exports = function server (inputCompanionOptions) { companion.setLoggerProcessName(companionOptions) - companionOptions.secret ||= generateSecret() - companionOptions.preAuthSecret ||= generateSecret() + if (!companionOptions.secret) companionOptions.secret = generateSecret() + if (!companionOptions.preAuthSecret) companionOptions.preAuthSecret = generateSecret() const app = express() From d36bdd7f6c25be9f94ada031f313933e4f851c05 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 5 Jan 2023 21:34:01 +0800 Subject: [PATCH 16/16] Update e2e/start-companion-with-load-balancer.mjs Co-authored-by: Antoine du Hamel --- e2e/start-companion-with-load-balancer.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/e2e/start-companion-with-load-balancer.mjs b/e2e/start-companion-with-load-balancer.mjs index 53777340a8..28de8fcc71 100755 --- a/e2e/start-companion-with-load-balancer.mjs +++ b/e2e/start-companion-with-load-balancer.mjs @@ -48,6 +48,7 @@ function createLoadBalancer (baseUrls) { const startCompanion = ({ name, port }) => execa('nodemon', [ '--watch', 'packages/@uppy/companion/src', '--exec', 'node', '-r', 'dotenv/config', './packages/@uppy/companion/src/standalone/start-server.js', ], { + cwd: new URL('../', import.meta.url), stdio: 'inherit', env: { // Note: these env variables will override anything set in .env