From e9b7db7b29b1e8fa4fba471f3f6de75fce842634 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 1 Mar 2021 17:33:48 +0100 Subject: [PATCH 01/12] backend-common: Add runDockerContainer shared by Scaffolder and TechDocs Signed-off-by: Himanshu Mishra --- packages/backend-common/package.json | 2 + packages/backend-common/src/index.ts | 7 +- .../backend-common/src/util/docker.test.ts | 200 ++++++++++++++++++ packages/backend-common/src/util/docker.ts | 119 +++++++++++ packages/backend-common/src/util/index.ts | 17 ++ .../src/stages/generate/helpers.test.ts | 133 ------------ .../src/stages/generate/helpers.ts | 88 -------- .../src/stages/generate/techdocs.ts | 6 +- .../stages/templater/cookiecutter.ts | 15 +- .../scaffolder/stages/templater/cra/index.ts | 8 +- .../stages/templater/helpers.test.ts | 184 ---------------- .../scaffolder/stages/templater/helpers.ts | 97 +-------- 12 files changed, 359 insertions(+), 517 deletions(-) create mode 100644 packages/backend-common/src/util/docker.test.ts create mode 100644 packages/backend-common/src/util/docker.ts create mode 100644 packages/backend-common/src/util/index.ts delete mode 100644 plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.test.ts diff --git a/packages/backend-common/package.json b/packages/backend-common/package.json index 3cd817427f..4205e12715 100644 --- a/packages/backend-common/package.json +++ b/packages/backend-common/package.json @@ -35,12 +35,14 @@ "@backstage/integration": "^0.5.0", "@octokit/rest": "^18.0.12", "@types/cors": "^2.8.6", + "@types/dockerode": "^3.2.1", "@types/express": "^4.17.6", "archiver": "^5.0.2", "compression": "^1.7.4", "concat-stream": "^2.0.0", "cors": "^2.8.5", "cross-fetch": "^3.0.6", + "dockerode": "^3.2.1", "express": "^4.17.1", "express-promise-router": "^3.0.3", "fs-extra": "^9.0.1", diff --git a/packages/backend-common/src/index.ts b/packages/backend-common/src/index.ts index 3f04f32b34..f3653f2627 100644 --- a/packages/backend-common/src/index.ts +++ b/packages/backend-common/src/index.ts @@ -18,10 +18,11 @@ export * from './config'; export * from './database'; export * from './discovery'; export * from './errors'; +export * from './hot'; export * from './logging'; export * from './middleware'; -export * from './reading'; -export * from './service'; export * from './paths'; -export * from './hot'; +export * from './reading'; export * from './scm'; +export * from './service'; +export * from './util'; diff --git a/packages/backend-common/src/util/docker.test.ts b/packages/backend-common/src/util/docker.test.ts new file mode 100644 index 0000000000..7f9bb2da33 --- /dev/null +++ b/packages/backend-common/src/util/docker.test.ts @@ -0,0 +1,200 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import Docker from 'dockerode'; +import fs from 'fs'; +import os from 'os'; +import Stream, { PassThrough } from 'stream'; +import { runDockerContainer, UserOptions } from './docker'; + +const mockDocker = new Docker() as jest.Mocked; + +describe('runDockerContainer', () => { + beforeEach(() => { + jest.spyOn(mockDocker, 'pull').mockImplementation((async ( + _image: string, + _something: any, + handler: (err: Error | undefined, stream: PassThrough) => void, + ) => { + const mockStream = new PassThrough(); + handler(undefined, mockStream); + mockStream.end(); + }) as any); + + jest + .spyOn(mockDocker, 'run') + .mockResolvedValue([{ Error: null, StatusCode: 0 }]); + + jest + .spyOn(mockDocker, 'ping') + .mockResolvedValue(Buffer.from('OK', 'utf-8')); + }); + + const imageName = 'dockerOrg/image'; + const args = ['bash', '-c', 'echo test']; + const inputDir = os.tmpdir(); + const outputDir = os.tmpdir(); + + it('should pull the docker container', async () => { + await runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + dockerClient: mockDocker, + }); + + expect(mockDocker.pull).toHaveBeenCalledWith( + imageName, + {}, + expect.any(Function), + ); + }); + + it('should call the dockerClient run command with the correct arguments passed through', async () => { + await runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + dockerClient: mockDocker, + }); + + expect(mockDocker.run).toHaveBeenCalledWith( + imageName, + args, + expect.any(Stream), + expect.objectContaining({ + HostConfig: { + Binds: expect.arrayContaining([ + `${await fs.promises.realpath(inputDir)}:/input`, + `${await fs.promises.realpath(outputDir)}:/output`, + ]), + }, + Volumes: { + '/input': {}, + '/output': {}, + }, + }), + ); + }); + + it('should ping docker to test availability', async () => { + await runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + dockerClient: mockDocker, + }); + + expect(mockDocker.ping).toHaveBeenCalled(); + }); + + it('should pass through the user and group id from the host machine and set the home dir', async () => { + await runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + dockerClient: mockDocker, + }); + + const userOptions: UserOptions = {}; + if (process.getuid && process.getgid) { + userOptions.User = `${process.getuid()}:${process.getgid()}`; + } + + expect(mockDocker.run).toHaveBeenCalledWith( + imageName, + args, + expect.any(Stream), + expect.objectContaining({ + ...userOptions, + }), + ); + }); + + it('throws a correct error if the command fails in docker', async () => { + mockDocker.run.mockResolvedValueOnce([ + { + Error: new Error('Something went wrong with docker'), + StatusCode: 0, + }, + ]); + + await expect( + runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + dockerClient: mockDocker, + }), + ).rejects.toThrow(/Something went wrong with docker/); + }); + + describe('where docker is unavailable', () => { + const dockerError = 'a docker error'; + + beforeEach(() => { + jest.spyOn(mockDocker, 'ping').mockImplementationOnce(() => { + throw new Error(dockerError); + }); + }); + + it('should throw with a descriptive error message including the docker error message', async () => { + await expect( + runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + dockerClient: mockDocker, + }), + ).rejects.toThrow(new RegExp(`.+: ${dockerError}`)); + }); + }); + + it('should pass through the log stream to the docker client', async () => { + const logStream = new PassThrough(); + await runDockerContainer({ + imageName, + args, + inputDir, + outputDir, + logStream, + dockerClient: mockDocker, + }); + + expect(mockDocker.run).toHaveBeenCalledWith( + imageName, + args, + logStream, + expect.objectContaining({ + HostConfig: { + Binds: expect.arrayContaining([ + `${await fs.promises.realpath(inputDir)}:/input`, + `${await fs.promises.realpath(outputDir)}:/output`, + ]), + }, + Volumes: { + '/input': {}, + '/output': {}, + }, + }), + ); + }); +}); diff --git a/packages/backend-common/src/util/docker.ts b/packages/backend-common/src/util/docker.ts new file mode 100644 index 0000000000..2b73915b0c --- /dev/null +++ b/packages/backend-common/src/util/docker.ts @@ -0,0 +1,119 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import Docker from 'dockerode'; +import fs from 'fs'; +import { PassThrough, Writable } from 'stream'; + +export type UserOptions = { + User?: string; +}; + +export type RunDockerContainerOptions = { + imageName: string; + args: string[]; + logStream?: Writable; + inputDir: string; + outputDir: string; + dockerClient: Docker; + createOptions?: Docker.ContainerCreateOptions; +}; + +/** + * + * @param options the options object + * @param options.imageName the image to run + * @param options.args the arguments to pass the container + * @param options.logStream the log streamer to capture log messages + * @param options.inputDir the /input path inside the container + * @param options.outputDir the /output path inside the container + * @param options.dockerClient the dockerClient to use + */ +export const runDockerContainer = async ({ + imageName, + args, + logStream = new PassThrough(), + inputDir, + outputDir, + dockerClient, + createOptions = {}, +}: RunDockerContainerOptions) => { + // Show a better error message when Docker is unavailable. + try { + await dockerClient.ping(); + } catch (e) { + throw new Error( + `This operation requires Docker. Docker does not appear to be available. Docker.ping() failed with: ${e.message}`, + ); + } + + await new Promise((resolve, reject) => { + dockerClient.pull(imageName, {}, (err, stream) => { + if (err) return reject(err); + stream.pipe(logStream, { end: false }); + stream.on('end', () => resolve()); + stream.on('error', (error: Error) => reject(error)); + return undefined; + }); + }); + + const userOptions: UserOptions = {}; + // @ts-ignore + if (process.getuid && process.getgid) { + // Files that are created inside the Docker container will be owned by + // root on the host system on non Mac systems, because of reasons. Mainly the fact that + // volume sharing is done using NFS on Mac and actual mounts in Linux world. + // So we set the user in the container as the same user and group id as the host. + // On Windows we don't have process.getuid nor process.getgid + userOptions.User = `${process.getuid()}:${process.getgid()}`; + } + + const [{ Error: error, StatusCode: statusCode }] = await dockerClient.run( + imageName, + args, + logStream, + { + Volumes: { '/output': {}, '/input': {} }, + HostConfig: { + Binds: [ + // Need to use realpath here as Docker mounting does not like + // symlinks for binding volumes + `${await fs.promises.realpath(outputDir)}:/output`, + `${await fs.promises.realpath(inputDir)}:/input`, + ], + }, + ...userOptions, + // Set the home directory inside the container as something that applications can + // write to, otherwise they will just flop and fail trying to write to / + Env: ['HOME=/tmp'], + ...createOptions, + }, + ); + + if (error) { + throw new Error( + `Docker failed to run with the following error message: ${error}`, + ); + } + + if (statusCode !== 0) { + throw new Error( + `Docker container returned a non-zero exit code (${statusCode})`, + ); + } + + return { error, statusCode }; +}; diff --git a/packages/backend-common/src/util/index.ts b/packages/backend-common/src/util/index.ts new file mode 100644 index 0000000000..04715824b4 --- /dev/null +++ b/packages/backend-common/src/util/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { runDockerContainer } from './docker'; diff --git a/packages/techdocs-common/src/stages/generate/helpers.test.ts b/packages/techdocs-common/src/stages/generate/helpers.test.ts index fbca6d1ed2..05402d0cf5 100644 --- a/packages/techdocs-common/src/stages/generate/helpers.test.ts +++ b/packages/techdocs-common/src/stages/generate/helpers.test.ts @@ -14,12 +14,10 @@ * limitations under the License. */ import { getVoidLogger } from '@backstage/backend-common'; -import Docker from 'dockerode'; import fs from 'fs-extra'; import mockFs from 'mock-fs'; import os from 'os'; import path, { resolve as resolvePath } from 'path'; -import Stream, { PassThrough } from 'stream'; import { ParsedLocationAnnotation } from '../../helpers'; import { RemoteProtocol } from '../prepare/types'; import { @@ -28,9 +26,7 @@ import { getRepoUrlFromLocationAnnotation, isValidRepoUrlForMkdocs, patchMkdocsYmlPreBuild, - runDockerContainer, storeEtagMetadata, - UserOptions, } from './helpers'; const mockEntity = { @@ -41,8 +37,6 @@ const mockEntity = { }, }; -const mockDocker = new Docker() as jest.Mocked; - const mkdocsYml = fs.readFileSync( resolvePath(__filename, '../__fixtures__/mkdocs.yml'), ); @@ -60,133 +54,6 @@ describe('helpers', () => { }); }); - describe('runDockerContainer', () => { - beforeEach(() => { - jest.spyOn(mockDocker, 'pull').mockImplementation((async ( - _image: string, - _something: any, - handler: (err: Error | undefined, stream: PassThrough) => void, - ) => { - const mockStream = new PassThrough(); - handler(undefined, mockStream); - mockStream.end(); - }) as any); - - jest - .spyOn(mockDocker, 'run') - .mockResolvedValue([{ Error: null, StatusCode: 0 }]); - - jest - .spyOn(mockDocker, 'ping') - .mockResolvedValue(Buffer.from('OK', 'utf-8')); - }); - - const imageName = 'spotify/techdocs'; - const args = ['build', '-d', '/result']; - const docsDir = os.tmpdir(); - const outputDir = os.tmpdir(); - - it('should pull the techdocs docker container', async () => { - await runDockerContainer({ - imageName, - args, - docsDir, - outputDir, - dockerClient: mockDocker, - }); - - expect(mockDocker.pull).toHaveBeenCalledWith( - imageName, - {}, - expect.any(Function), - ); - }); - - it('should run the techdocs docker container', async () => { - await runDockerContainer({ - imageName, - args, - docsDir, - outputDir, - dockerClient: mockDocker, - }); - - expect(mockDocker.run).toHaveBeenCalledWith( - imageName, - args, - expect.any(Stream), - expect.objectContaining({ - Volumes: { - '/content': {}, - '/result': {}, - }, - WorkingDir: '/content', - HostConfig: { - Binds: [`${docsDir}:/content`, `${outputDir}:/result`], - }, - }), - ); - }); - - it('should ping docker to test availability', async () => { - await runDockerContainer({ - imageName, - args, - docsDir, - outputDir, - dockerClient: mockDocker, - }); - - expect(mockDocker.ping).toHaveBeenCalled(); - }); - - it('should pass through the user and group id from the host machine and set the home dir', async () => { - await runDockerContainer({ - imageName, - args, - docsDir, - outputDir, - dockerClient: mockDocker, - }); - - const userOptions: UserOptions = {}; - if (process.getuid && process.getgid) { - userOptions.User = `${process.getuid()}:${process.getgid()}`; - } - - expect(mockDocker.run).toHaveBeenCalledWith( - imageName, - args, - expect.any(Stream), - expect.objectContaining({ - ...userOptions, - }), - ); - }); - - describe('where docker is unavailable', () => { - const dockerError = 'a docker error'; - - beforeEach(() => { - jest.spyOn(mockDocker, 'ping').mockImplementationOnce(() => { - throw new Error(dockerError); - }); - }); - - it('should throw with a descriptive error message including the docker error message', async () => { - await expect( - runDockerContainer({ - imageName, - args, - docsDir, - outputDir, - dockerClient: mockDocker, - }), - ).rejects.toThrow(new RegExp(`.+: ${dockerError}`)); - }); - }); - }); - describe('isValidRepoUrlForMkdocs', () => { it('should return true for valid repo_url values for mkdocs', () => { const validRepoUrls = [ diff --git a/packages/techdocs-common/src/stages/generate/helpers.ts b/packages/techdocs-common/src/stages/generate/helpers.ts index a88ed13231..a99544a1f8 100644 --- a/packages/techdocs-common/src/stages/generate/helpers.ts +++ b/packages/techdocs-common/src/stages/generate/helpers.ts @@ -16,7 +16,6 @@ import { Entity } from '@backstage/catalog-model'; import { spawn } from 'child_process'; -import Docker from 'dockerode'; import fs from 'fs-extra'; import yaml from 'js-yaml'; import { PassThrough, Writable } from 'stream'; @@ -34,16 +33,6 @@ export function getGeneratorKey(entity: Entity): SupportedGeneratorKey { return 'techdocs'; } -type RunDockerContainerOptions = { - imageName: string; - args: string[]; - logStream?: Writable; - docsDir: string; - outputDir: string; - dockerClient: Docker; - createOptions?: Docker.ContainerCreateOptions; -}; - export type RunCommandOptions = { command: string; args: string[]; @@ -51,83 +40,6 @@ export type RunCommandOptions = { logStream?: Writable; }; -export type UserOptions = { - User?: string; -}; - -// To be replaced by a runDockerContainer from backend-common -// shared between Scaffolder and TechDocs and any other plugin. -export async function runDockerContainer({ - imageName, - args, - logStream = new PassThrough(), - docsDir, - outputDir, - dockerClient, - createOptions, -}: RunDockerContainerOptions) { - try { - await dockerClient.ping(); - } catch (e) { - throw new Error( - `This operation requires Docker. Docker does not appear to be available. Docker.ping() failed with: ${e.message}`, - ); - } - - await new Promise((resolve, reject) => { - dockerClient.pull(imageName, {}, (err, stream) => { - if (err) return reject(err); - stream.pipe(logStream, { end: false }); - stream.on('end', () => resolve()); - stream.on('error', (error: Error) => reject(error)); - return undefined; - }); - }); - - const userOptions: UserOptions = {}; - // @ts-ignore - if (process.getuid && process.getgid) { - // Files that are created inside the Docker container will be owned by - // root on the host system on non Mac systems, because of reasons. Mainly the fact that - // volume sharing is done using NFS on Mac and actual mounts in Linux world. - // So we set the user in the container as the same user and group id as the host. - // On Windows we don't have process.getuid nor process.getgid - userOptions.User = `${process.getuid()}:${process.getgid()}`; - } - - const [{ Error: error, StatusCode: statusCode }] = await dockerClient.run( - imageName, - args, - logStream, - { - Volumes: { - '/content': {}, - '/result': {}, - }, - WorkingDir: '/content', - HostConfig: { - Binds: [`${docsDir}:/content`, `${outputDir}:/result`], - }, - ...userOptions, - ...createOptions, - }, - ); - - if (error) { - throw new Error( - `Docker failed to run with the following error message: ${error}`, - ); - } - - if (statusCode !== 0) { - throw new Error( - `Docker container returned a non-zero exit code (${statusCode})`, - ); - } - - return { error, statusCode }; -} - /** * * @param options the options object diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index 133755ecd2..e25ce696f6 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { runDockerContainer } from '@backstage/backend-common'; import { Config } from '@backstage/config'; import path from 'path'; import { PassThrough } from 'stream'; @@ -22,7 +23,6 @@ import { addBuildTimestampMetadata, patchMkdocsYmlPreBuild, runCommand, - runDockerContainer, storeEtagMetadata, } from './helpers'; import { GeneratorBase, GeneratorRunOptions } from './types'; @@ -96,9 +96,9 @@ export class TechdocsGenerator implements GeneratorBase { case 'docker': await runDockerContainer({ imageName: 'spotify/techdocs', - args: ['build', '-d', '/result'], + args: ['build', '-d', '/output'], logStream, - docsDir: inputDir, + inputDir, outputDir, dockerClient, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts index 9568af2a15..a71af1d3bf 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts @@ -13,11 +13,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import fs from 'fs-extra'; +import { runDockerContainer } from '@backstage/backend-common'; import { JsonValue } from '@backstage/config'; -import { runDockerContainer, runCommand } from './helpers'; -import { TemplaterBase, TemplaterRunOptions } from '.'; +import fs from 'fs-extra'; import path from 'path'; +import { TemplaterBase, TemplaterRunOptions } from '.'; +import { runCommand } from './helpers'; const commandExists = require('command-exists-promise'); @@ -71,12 +72,12 @@ export class CookieCutter implements TemplaterBase { 'cookiecutter', '--no-input', '-o', - '/result', - '/template', + '/output', + '/input', '--verbose', ], - templateDir, - resultDir: intermediateDir, + inputDir: templateDir, + outputDir: intermediateDir, logStream, dockerClient, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts index 5e957e0504..4f871516e9 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts @@ -13,11 +13,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import { runDockerContainer } from '@backstage/backend-common'; import fs from 'fs-extra'; -import { runDockerContainer } from '../helpers'; -import { TemplaterBase, TemplaterRunOptions } from '..'; import path from 'path'; import * as yaml from 'yaml'; +import { TemplaterBase, TemplaterRunOptions } from '..'; // TODO(blam): Replace with the universal import from github-actions after a release // As it will break the E2E without it @@ -49,8 +49,8 @@ export class CreateReactAppTemplater implements TemplaterBase { componentName as string, withTypescript ? ' --template typescript' : '', ], - templateDir: intermediateDir, - resultDir: intermediateDir, + inputDir: intermediateDir, + outputDir: intermediateDir, logStream: logStream, dockerClient: dockerClient, createOptions: { diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.test.ts deleted file mode 100644 index 57e47f4c27..0000000000 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.test.ts +++ /dev/null @@ -1,184 +0,0 @@ -/* - * Copyright 2020 Spotify AB - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -import Stream, { PassThrough } from 'stream'; -import os from 'os'; -import fs from 'fs'; -import Docker from 'dockerode'; -import { UserOptions, runDockerContainer } from './helpers'; - -describe('helpers', () => { - const mockDocker = new Docker() as jest.Mocked; - - beforeEach(() => { - jest - .spyOn(mockDocker, 'run') - .mockResolvedValue([{ Error: null, StatusCode: 0 }]); - jest.spyOn(mockDocker, 'pull').mockImplementation((async ( - _image: string, - _something: any, - handler: (err: Error | undefined, stream: PassThrough) => void, - ) => { - const mockStream = new PassThrough(); - handler(undefined, mockStream); - mockStream.end(); - }) as any); - }); - - describe('runDockerContainer', () => { - const imageName = 'blam/github:ben'; - const args = ['bash', '-c', 'echo lol']; - const templateDir = os.tmpdir(); - const resultDir = os.tmpdir(); - - it('will pull the docker container before running', async () => { - await runDockerContainer({ - imageName, - args, - templateDir, - resultDir, - dockerClient: mockDocker, - }); - - expect(mockDocker.pull).toHaveBeenCalledWith( - imageName, - {}, - expect.any(Function), - ); - }); - it('should call the dockerClient run command with the correct arguments passed through', async () => { - await runDockerContainer({ - imageName, - args, - templateDir, - resultDir, - dockerClient: mockDocker, - }); - - expect(mockDocker.run).toHaveBeenCalledWith( - imageName, - args, - expect.any(Stream), - expect.objectContaining({ - HostConfig: { - Binds: expect.arrayContaining([ - `${await fs.promises.realpath(templateDir)}:/template`, - `${await fs.promises.realpath(resultDir)}:/result`, - ]), - }, - Volumes: { - '/template': {}, - '/result': {}, - }, - }), - ); - }); - - it('throws a correct error if the templating fails in docker', async () => { - mockDocker.run.mockResolvedValueOnce([ - { - Error: new Error('Something went wrong with docker'), - StatusCode: 0, - }, - ]); - - await expect( - runDockerContainer({ - imageName, - args, - templateDir, - resultDir, - dockerClient: mockDocker, - }), - ).rejects.toThrow(/Something went wrong with docker/); - }); - - it('throws a correct error when the response code of the container is non-zero', async () => { - mockDocker.run.mockResolvedValueOnce([ - { - Error: null, - StatusCode: 123, - }, - ]); - - await expect( - runDockerContainer({ - imageName, - args, - templateDir, - resultDir, - dockerClient: mockDocker, - }), - ).rejects.toThrow( - /Docker container returned a non-zero exit code \(123\)/, - ); - }); - - it('should pass through the user and group id from the host machine and set the home dir', async () => { - await runDockerContainer({ - imageName, - args, - templateDir, - resultDir, - dockerClient: mockDocker, - }); - - const userOptions: UserOptions = {}; - if (process.getuid && process.getgid) { - userOptions.User = `${process.getuid()}:${process.getgid()}`; - } - - expect(mockDocker.run).toHaveBeenCalledWith( - imageName, - args, - expect.any(Stream), - expect.objectContaining({ - ...userOptions, - Env: ['HOME=/tmp'], - }), - ); - }); - - it('should pass through the log stream to the docker client', async () => { - const logStream = new PassThrough(); - await runDockerContainer({ - imageName, - args, - templateDir, - resultDir, - logStream, - dockerClient: mockDocker, - }); - - expect(mockDocker.run).toHaveBeenCalledWith( - imageName, - args, - logStream, - expect.objectContaining({ - HostConfig: { - Binds: expect.arrayContaining([ - `${await fs.promises.realpath(templateDir)}:/template`, - `${await fs.promises.realpath(resultDir)}:/result`, - ]), - }, - Volumes: { - '/template': {}, - '/result': {}, - }, - }), - ); - }); - }); -}); diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.ts index 645133a4ae..cb1e8532f2 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/helpers.ts @@ -13,22 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { Writable, PassThrough } from 'stream'; -import Docker from 'dockerode'; -import fs from 'fs'; -import { TemplateEntityV1alpha1 } from '@backstage/catalog-model'; import { InputError } from '@backstage/backend-common'; +import { TemplateEntityV1alpha1 } from '@backstage/catalog-model'; import { spawn } from 'child_process'; - -export type RunDockerContainerOptions = { - imageName: string; - args: string[]; - logStream?: Writable; - resultDir: string; - templateDir: string; - dockerClient: Docker; - createOptions?: Docker.ContainerCreateOptions; -}; +import { PassThrough, Writable } from 'stream'; export type RunCommandOptions = { command: string; @@ -36,10 +24,6 @@ export type RunCommandOptions = { logStream?: Writable; }; -export type UserOptions = { - User?: string; -}; - /** * Gets the templater key to use for templating from the entity * @param entity Template entity @@ -89,80 +73,3 @@ export const runCommand = async ({ }); }); }; - -/** - * - * @param options the options object - * @param options.imageName the image to run - * @param options.args the arguments to pass the container - * @param options.logStream the log streamer to capture log messages - * @param options.resultDir the /result path inside the container - * @param options.templateDir the /template path inside the container - * @param options.dockerClient the dockerClient to use - */ -export const runDockerContainer = async ({ - imageName, - args, - logStream = new PassThrough(), - resultDir, - templateDir, - dockerClient, - createOptions = {}, -}: RunDockerContainerOptions) => { - await new Promise((resolve, reject) => { - dockerClient.pull(imageName, {}, (err, stream) => { - if (err) return reject(err); - stream.pipe(logStream, { end: false }); - stream.on('end', () => resolve()); - stream.on('error', (error: Error) => reject(error)); - return undefined; - }); - }); - - const userOptions: UserOptions = {}; - // @ts-ignore - if (process.getuid && process.getgid) { - // Files that are created inside the Docker container will be owned by - // root on the host system on non Mac systems, because of reasons. Mainly the fact that - // volume sharing is done using NFS on Mac and actual mounts in Linux world. - // So we set the user in the container as the same user and group id as the host. - // On Windows we don't have process.getuid nor process.getgid - userOptions.User = `${process.getuid()}:${process.getgid()}`; - } - - const [{ Error: error, StatusCode: statusCode }] = await dockerClient.run( - imageName, - args, - logStream, - { - Volumes: { '/result': {}, '/template': {} }, - HostConfig: { - Binds: [ - // Need to use realpath here as Docker mounting does not like - // symlinks for binding volumes - `${await fs.promises.realpath(resultDir)}:/result`, - `${await fs.promises.realpath(templateDir)}:/template`, - ], - }, - ...userOptions, - // Set the home directory inside the container as something that applications can - // write to, otherwise they will just flop and fail trying to write to / - Env: ['HOME=/tmp'], - ...createOptions, - }, - ); - - if (error) { - throw new Error( - `Docker failed to run with the following error message: ${error}`, - ); - } - - if (statusCode !== 0) { - throw new Error( - `Docker container returned a non-zero exit code (${statusCode})`, - ); - } - - return { error, statusCode }; -}; From d7245b733252f58d9420423f12d48a414b550db3 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 1 Mar 2021 17:40:10 +0100 Subject: [PATCH 02/12] Add changesets for runDockerContainer Signed-off-by: Himanshu Mishra --- .changeset/funny-drinks-boil.md | 5 +++++ .changeset/great-bees-press.md | 6 ++++++ 2 files changed, 11 insertions(+) create mode 100644 .changeset/funny-drinks-boil.md create mode 100644 .changeset/great-bees-press.md diff --git a/.changeset/funny-drinks-boil.md b/.changeset/funny-drinks-boil.md new file mode 100644 index 0000000000..f7b84a32a1 --- /dev/null +++ b/.changeset/funny-drinks-boil.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Add a utility function runDockerContainer used to run a docker container (currently used by Scaffolder and TechDocs for their 'generate' processes) diff --git a/.changeset/great-bees-press.md b/.changeset/great-bees-press.md new file mode 100644 index 0000000000..1582e54519 --- /dev/null +++ b/.changeset/great-bees-press.md @@ -0,0 +1,6 @@ +--- +'@backstage/techdocs-common': patch +'@backstage/plugin-scaffolder-backend': patch +--- + +Remove runDockerContainer, and start using the utility function provided by @backstage/backend-common From d78769fa266315a5e36e49538ca234c9fa08ddde Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 1 Mar 2021 17:59:05 +0100 Subject: [PATCH 03/12] TechDocs: Show error from Docker container when build fails Signed-off-by: Himanshu Mishra --- packages/techdocs-common/src/stages/generate/techdocs.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index e25ce696f6..ff7b866a56 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -115,7 +115,7 @@ export class TechdocsGenerator implements GeneratorBase { this.logger.debug( `Failed to generate docs from ${inputDir} into ${outputDir}`, ); - this.logger.debug(`Build failed with error: ${log}`); + this.logger.error(`Build failed with error: ${log}`); throw new Error( `Failed to generate docs from ${inputDir} into ${outputDir} with error ${error.message}`, ); From d79540d0b8367f954a915575c3bf72894cd824e1 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 1 Mar 2021 17:59:47 +0100 Subject: [PATCH 04/12] backend-common: Set workingdir to /input by default Signed-off-by: Himanshu Mishra --- packages/backend-common/src/util/docker.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/backend-common/src/util/docker.ts b/packages/backend-common/src/util/docker.ts index 2b73915b0c..8ab2e99824 100644 --- a/packages/backend-common/src/util/docker.ts +++ b/packages/backend-common/src/util/docker.ts @@ -87,6 +87,7 @@ export const runDockerContainer = async ({ logStream, { Volumes: { '/output': {}, '/input': {} }, + WorkingDir: '/input', HostConfig: { Binds: [ // Need to use realpath here as Docker mounting does not like From a37a4d1a4c252e480ecd4c076c0b0a1bb10ab8c1 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 1 Mar 2021 18:40:43 +0100 Subject: [PATCH 05/12] Scaffolder/Tests: Fix by mocking @backstage/backend-common Signed-off-by: Himanshu Mishra --- .../stages/templater/cookiecutter.test.ts | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts index 77368fc1ab..eb73081568 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts @@ -18,16 +18,17 @@ const runDockerContainer = jest.fn(); const runCommand = jest.fn(); const commandExists = jest.fn(); -jest.mock('./helpers', () => ({ runDockerContainer, runCommand })); +jest.mock('./helpers', () => ({ runCommand })); +jest.mock('@backstage/backend-common', () => ({ runDockerContainer })); jest.mock('command-exists-promise', () => commandExists); jest.mock('fs-extra'); -import { CookieCutter } from './cookiecutter'; +import Docker from 'dockerode'; import fs from 'fs-extra'; +import parseGitUrl from 'git-url-parse'; import path from 'path'; import { PassThrough } from 'stream'; -import Docker from 'dockerode'; -import parseGitUrl from 'git-url-parse'; +import { CookieCutter } from './cookiecutter'; describe('CookieCutter Templater', () => { const mockDocker = {} as Docker; @@ -149,12 +150,12 @@ describe('CookieCutter Templater', () => { 'cookiecutter', '--no-input', '-o', - '/result', - '/template', + '/output', + '/input', '--verbose', ], - templateDir: path.join('tempdir', 'template'), - resultDir: path.join('tempdir', 'intermediate'), + inputDir: path.join('tempdir', 'template'), + outputDir: path.join('tempdir', 'intermediate'), logStream: undefined, dockerClient: mockDocker, }); @@ -188,12 +189,12 @@ describe('CookieCutter Templater', () => { 'cookiecutter', '--no-input', '-o', - '/result', - '/template', + '/output', + '/input', '--verbose', ], - templateDir: path.join('tempdir', 'template'), - resultDir: path.join('tempdir', 'intermediate'), + inputDir: path.join('tempdir', 'template'), + outputDir: path.join('tempdir', 'intermediate'), logStream: stream, dockerClient: mockDocker, }); From e67d055dc2357b0e6f7d6adc2cf05afbf3c5b973 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Fri, 5 Mar 2021 10:20:38 +0100 Subject: [PATCH 06/12] backend-common: remove useless ts ignore Signed-off-by: Himanshu Mishra --- packages/backend-common/src/util/docker.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/backend-common/src/util/docker.ts b/packages/backend-common/src/util/docker.ts index 8ab2e99824..9d2bccd549 100644 --- a/packages/backend-common/src/util/docker.ts +++ b/packages/backend-common/src/util/docker.ts @@ -71,7 +71,6 @@ export const runDockerContainer = async ({ }); const userOptions: UserOptions = {}; - // @ts-ignore if (process.getuid && process.getgid) { // Files that are created inside the Docker container will be owned by // root on the host system on non Mac systems, because of reasons. Mainly the fact that From 63b8914b82f901af312b5466a9abb62e994ba2b9 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Sat, 6 Mar 2021 12:42:02 +0100 Subject: [PATCH 07/12] backend-common: Make runDockerContainer more configurable 1. Add optional directories to mount 2. Add optional working directory to use 3. Add optional environment variables to set Signed-off-by: Himanshu Mishra --- .../backend-common/src/util/docker.test.ts | 58 ++++++++++--------- packages/backend-common/src/util/docker.ts | 44 ++++++++------ 2 files changed, 57 insertions(+), 45 deletions(-) diff --git a/packages/backend-common/src/util/docker.test.ts b/packages/backend-common/src/util/docker.test.ts index 7f9bb2da33..8af6a7366f 100644 --- a/packages/backend-common/src/util/docker.test.ts +++ b/packages/backend-common/src/util/docker.test.ts @@ -14,15 +14,24 @@ * limitations under the License. */ import Docker from 'dockerode'; -import fs from 'fs'; +import mockFs from 'mock-fs'; import os from 'os'; +import path from 'path'; import Stream, { PassThrough } from 'stream'; import { runDockerContainer, UserOptions } from './docker'; const mockDocker = new Docker() as jest.Mocked; +const rootDir = os.platform() === 'win32' ? 'C:\\rootDir' : '/rootDir'; describe('runDockerContainer', () => { beforeEach(() => { + mockFs({ + [rootDir]: { + input: mockFs.directory(), + output: mockFs.directory(), + }, + }); + jest.spyOn(mockDocker, 'pull').mockImplementation((async ( _image: string, _something: any, @@ -42,17 +51,23 @@ describe('runDockerContainer', () => { .mockResolvedValue(Buffer.from('OK', 'utf-8')); }); + afterEach(() => { + mockFs.restore(); + }); + const imageName = 'dockerOrg/image'; const args = ['bash', '-c', 'echo test']; - const inputDir = os.tmpdir(); - const outputDir = os.tmpdir(); + const mountDirs = new Map([ + [path.join(rootDir, 'input'), '/input'], + [path.join(rootDir, 'output'), '/output'], + ]); + const workingDir = path.join(rootDir, 'input'); + const envVars = ['HOME=/tmp', 'LOG_LEVEL=debug']; it('should pull the docker container', async () => { await runDockerContainer({ imageName, args, - inputDir, - outputDir, dockerClient: mockDocker, }); @@ -61,14 +76,17 @@ describe('runDockerContainer', () => { {}, expect.any(Function), ); + + expect(mockDocker.run).toHaveBeenCalled(); }); it('should call the dockerClient run command with the correct arguments passed through', async () => { await runDockerContainer({ imageName, args, - inputDir, - outputDir, + mountDirs, + envVars, + workingDir, dockerClient: mockDocker, }); @@ -77,10 +95,12 @@ describe('runDockerContainer', () => { args, expect.any(Stream), expect.objectContaining({ + Env: envVars, + WorkingDir: workingDir, HostConfig: { Binds: expect.arrayContaining([ - `${await fs.promises.realpath(inputDir)}:/input`, - `${await fs.promises.realpath(outputDir)}:/output`, + `${path.join(rootDir, 'input')}:/input`, + `${path.join(rootDir, 'output')}:/output`, ]), }, Volumes: { @@ -95,8 +115,6 @@ describe('runDockerContainer', () => { await runDockerContainer({ imageName, args, - inputDir, - outputDir, dockerClient: mockDocker, }); @@ -107,8 +125,6 @@ describe('runDockerContainer', () => { await runDockerContainer({ imageName, args, - inputDir, - outputDir, dockerClient: mockDocker, }); @@ -139,8 +155,6 @@ describe('runDockerContainer', () => { runDockerContainer({ imageName, args, - inputDir, - outputDir, dockerClient: mockDocker, }), ).rejects.toThrow(/Something went wrong with docker/); @@ -160,8 +174,6 @@ describe('runDockerContainer', () => { runDockerContainer({ imageName, args, - inputDir, - outputDir, dockerClient: mockDocker, }), ).rejects.toThrow(new RegExp(`.+: ${dockerError}`)); @@ -173,8 +185,6 @@ describe('runDockerContainer', () => { await runDockerContainer({ imageName, args, - inputDir, - outputDir, logStream, dockerClient: mockDocker, }); @@ -185,15 +195,9 @@ describe('runDockerContainer', () => { logStream, expect.objectContaining({ HostConfig: { - Binds: expect.arrayContaining([ - `${await fs.promises.realpath(inputDir)}:/input`, - `${await fs.promises.realpath(outputDir)}:/output`, - ]), - }, - Volumes: { - '/input': {}, - '/output': {}, + Binds: [], }, + Volumes: {}, }), ); }); diff --git a/packages/backend-common/src/util/docker.ts b/packages/backend-common/src/util/docker.ts index 9d2bccd549..f2d1ff6993 100644 --- a/packages/backend-common/src/util/docker.ts +++ b/packages/backend-common/src/util/docker.ts @@ -15,7 +15,6 @@ */ import Docker from 'dockerode'; -import fs from 'fs'; import { PassThrough, Writable } from 'stream'; export type UserOptions = { @@ -26,9 +25,10 @@ export type RunDockerContainerOptions = { imageName: string; args: string[]; logStream?: Writable; - inputDir: string; - outputDir: string; dockerClient: Docker; + mountDirs?: Map; + workingDir?: string; + envVars?: string[]; createOptions?: Docker.ContainerCreateOptions; }; @@ -38,17 +38,20 @@ export type RunDockerContainerOptions = { * @param options.imageName the image to run * @param options.args the arguments to pass the container * @param options.logStream the log streamer to capture log messages - * @param options.inputDir the /input path inside the container - * @param options.outputDir the /output path inside the container * @param options.dockerClient the dockerClient to use + * @param options.mountDirs A map of host directories to mount on the container. + * Map Key: Path on host machine, Value: Path on Docker container + * @param options.workingDir Working dir in the container + * @param options.envVars Environment variables to set in the container. e.g. ['HOME=/tmp'] */ export const runDockerContainer = async ({ imageName, args, logStream = new PassThrough(), - inputDir, - outputDir, dockerClient, + mountDirs = new Map(), + workingDir, + envVars = [], createOptions = {}, }: RunDockerContainerOptions) => { // Show a better error message when Docker is unavailable. @@ -80,25 +83,30 @@ export const runDockerContainer = async ({ userOptions.User = `${process.getuid()}:${process.getgid()}`; } + // Initialize volumes to mount based on mountDirs map + const Volumes: { [T: string]: object } = {}; + for (const containerDir of mountDirs.values()) { + Volumes[containerDir] = {}; + } + + // Create bind volumes + const Binds: string[] = []; + for (const [hostDir, containerDir] of mountDirs.entries()) { + Binds.push(`${hostDir}:${containerDir}`); + } + const [{ Error: error, StatusCode: statusCode }] = await dockerClient.run( imageName, args, logStream, { - Volumes: { '/output': {}, '/input': {} }, - WorkingDir: '/input', + Volumes, HostConfig: { - Binds: [ - // Need to use realpath here as Docker mounting does not like - // symlinks for binding volumes - `${await fs.promises.realpath(outputDir)}:/output`, - `${await fs.promises.realpath(inputDir)}:/input`, - ], + Binds, }, + ...(workingDir ? { WorkingDir: workingDir } : {}), + Env: envVars, ...userOptions, - // Set the home directory inside the container as something that applications can - // write to, otherwise they will just flop and fail trying to write to / - Env: ['HOME=/tmp'], ...createOptions, }, ); From 68575f368908480d9890935692b40590d7c0d44b Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Sat, 6 Mar 2021 13:11:36 +0100 Subject: [PATCH 08/12] Update techdocs/scaffolder with new function parameters of runDockerContainer Signed-off-by: Himanshu Mishra --- .../src/stages/generate/techdocs.ts | 16 ++++++++++++-- .../stages/templater/cookiecutter.test.ts | 21 +++++++++++++++---- .../stages/templater/cookiecutter.ts | 15 +++++++++++-- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index ff7b866a56..ab7b730106 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -16,6 +16,7 @@ import { runDockerContainer } from '@backstage/backend-common'; import { Config } from '@backstage/config'; +import fs from 'fs-extra'; import path from 'path'; import { PassThrough } from 'stream'; import { Logger } from 'winston'; @@ -78,6 +79,14 @@ export class TechdocsGenerator implements GeneratorBase { ); } + // Directories to bind on container + const mountDirs = new Map([ + // Need to use realpath here as Docker mounting does not like + // symlinks for binding volumes + [await fs.realpath(inputDir), '/input'], + [await fs.realpath(outputDir), '/output'], + ]); + try { switch (this.options.runGeneratorIn) { case 'local': @@ -98,8 +107,11 @@ export class TechdocsGenerator implements GeneratorBase { imageName: 'spotify/techdocs', args: ['build', '-d', '/output'], logStream, - inputDir, - outputDir, + mountDirs, + workingDir: '/input', + // Set the home directory inside the container as something that applications can + // write to, otherwise they will just fail trying to write to / + envVars: ['HOME=/tmp'], dockerClient, }); this.logger.info( diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts index eb73081568..a48a117639 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts @@ -136,6 +136,11 @@ describe('CookieCutter Templater', () => { }; jest.spyOn(fs, 'readdir').mockResolvedValueOnce(['newthing']); + jest + .spyOn(fs, 'realpath') + .mockImplementation((filePath: string | Buffer) => + Promise.resolve(filePath as string), + ); const templater = new CookieCutter(); await templater.run({ @@ -154,8 +159,12 @@ describe('CookieCutter Templater', () => { '/input', '--verbose', ], - inputDir: path.join('tempdir', 'template'), - outputDir: path.join('tempdir', 'intermediate'), + envVars: ['HOME=/tmp'], + mountDirs: new Map([ + [path.join('tempdir', 'template'), '/input'], + [path.join('tempdir', 'intermediate'), '/output'], + ]), + workingDir: '/input', logStream: undefined, dockerClient: mockDocker, }); @@ -193,8 +202,12 @@ describe('CookieCutter Templater', () => { '/input', '--verbose', ], - inputDir: path.join('tempdir', 'template'), - outputDir: path.join('tempdir', 'intermediate'), + envVars: ['HOME=/tmp'], + mountDirs: new Map([ + [path.join('tempdir', 'template'), '/input'], + [path.join('tempdir', 'intermediate'), '/output'], + ]), + workingDir: '/input', logStream: stream, dockerClient: mockDocker, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts index a71af1d3bf..39e6d7be42 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts @@ -58,6 +58,14 @@ export class CookieCutter implements TemplaterBase { await fs.writeJSON(path.join(templateDir, 'cookiecutter.json'), cookieInfo); + // Directories to bind on container + const mountDirs = new Map([ + // Need to use realpath here as Docker mounting does not like + // symlinks for binding volumes + [await fs.realpath(templateDir), '/input'], + [await fs.realpath(intermediateDir), '/output'], + ]); + const cookieCutterInstalled = await commandExists('cookiecutter'); if (cookieCutterInstalled) { await runCommand({ @@ -76,8 +84,11 @@ export class CookieCutter implements TemplaterBase { '/input', '--verbose', ], - inputDir: templateDir, - outputDir: intermediateDir, + mountDirs, + workingDir: '/input', + // Set the home directory inside the container as something that applications can + // write to, otherwise they will just fail trying to write to / + envVars: ['HOME=/tmp'], logStream, dockerClient, }); From 29b64038e1be5eda1e186ad551faa570e7b64311 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Sat, 6 Mar 2021 21:01:02 +0100 Subject: [PATCH 09/12] Scaffolder: Update CRA templater with new runDockerContainer format Signed-off-by: Himanshu Mishra --- .../src/scaffolder/stages/templater/cra/index.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts index 4f871516e9..371586d1da 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts @@ -40,7 +40,10 @@ export class CreateReactAppTemplater implements TemplaterBase { const intermediateDir = path.join(workspacePath, 'template'); await fs.ensureDir(intermediateDir); - const resultDir = path.join(workspacePath, 'result'); + const mountDirs = new Map([ + [await fs.realpath(intermediateDir), '/template'], + [await fs.realpath(intermediateDir), '/result'], + ]); await runDockerContainer({ imageName: 'node:lts-alpine', @@ -49,10 +52,12 @@ export class CreateReactAppTemplater implements TemplaterBase { componentName as string, withTypescript ? ' --template typescript' : '', ], - inputDir: intermediateDir, - outputDir: intermediateDir, + mountDirs, logStream: logStream, dockerClient: dockerClient, + // Set the home directory inside the container as something that applications can + // write to, otherwise they will just fail trying to write to / + envVars: ['HOME=/tmp'], createOptions: { Entrypoint: ['npx'], WorkingDir: '/result', @@ -67,6 +72,7 @@ export class CreateReactAppTemplater implements TemplaterBase { throw new Error('No data generated by cookiecutter'); } + const resultDir = path.join(workspacePath, 'result'); await fs.move(path.join(intermediateDir, generated), resultDir); const extraAnnotations: Record = {}; From c0c26244db538dc8751e4e23b72529295445b3c0 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 8 Mar 2021 12:44:11 +0100 Subject: [PATCH 10/12] 1. Use plain Record instead of Map 2. Call fs.realpath inside the utility function Signed-off-by: Himanshu Mishra --- packages/backend-common/src/util/docker.test.ts | 8 ++++---- packages/backend-common/src/util/docker.ts | 16 ++++++++++------ .../src/stages/generate/techdocs.ts | 11 ++++------- .../stages/templater/cookiecutter.test.ts | 16 ++++++++-------- .../scaffolder/stages/templater/cookiecutter.ts | 10 ++++------ .../src/scaffolder/stages/templater/cra/index.ts | 8 ++++---- 6 files changed, 34 insertions(+), 35 deletions(-) diff --git a/packages/backend-common/src/util/docker.test.ts b/packages/backend-common/src/util/docker.test.ts index 8af6a7366f..a6cc9889fe 100644 --- a/packages/backend-common/src/util/docker.test.ts +++ b/packages/backend-common/src/util/docker.test.ts @@ -57,10 +57,10 @@ describe('runDockerContainer', () => { const imageName = 'dockerOrg/image'; const args = ['bash', '-c', 'echo test']; - const mountDirs = new Map([ - [path.join(rootDir, 'input'), '/input'], - [path.join(rootDir, 'output'), '/output'], - ]); + const mountDirs = { + [path.join(rootDir, 'input')]: '/input', + [path.join(rootDir, 'output')]: '/output', + }; const workingDir = path.join(rootDir, 'input'); const envVars = ['HOME=/tmp', 'LOG_LEVEL=debug']; diff --git a/packages/backend-common/src/util/docker.ts b/packages/backend-common/src/util/docker.ts index f2d1ff6993..4e143a6d1b 100644 --- a/packages/backend-common/src/util/docker.ts +++ b/packages/backend-common/src/util/docker.ts @@ -15,6 +15,7 @@ */ import Docker from 'dockerode'; +import fs from 'fs-extra'; import { PassThrough, Writable } from 'stream'; export type UserOptions = { @@ -26,7 +27,7 @@ export type RunDockerContainerOptions = { args: string[]; logStream?: Writable; dockerClient: Docker; - mountDirs?: Map; + mountDirs?: Record; workingDir?: string; envVars?: string[]; createOptions?: Docker.ContainerCreateOptions; @@ -40,7 +41,7 @@ export type RunDockerContainerOptions = { * @param options.logStream the log streamer to capture log messages * @param options.dockerClient the dockerClient to use * @param options.mountDirs A map of host directories to mount on the container. - * Map Key: Path on host machine, Value: Path on Docker container + * Object Key: Path on host machine, Value: Path on Docker container * @param options.workingDir Working dir in the container * @param options.envVars Environment variables to set in the container. e.g. ['HOME=/tmp'] */ @@ -49,7 +50,7 @@ export const runDockerContainer = async ({ args, logStream = new PassThrough(), dockerClient, - mountDirs = new Map(), + mountDirs = {}, workingDir, envVars = [], createOptions = {}, @@ -85,14 +86,17 @@ export const runDockerContainer = async ({ // Initialize volumes to mount based on mountDirs map const Volumes: { [T: string]: object } = {}; - for (const containerDir of mountDirs.values()) { + for (const containerDir of Object.values(mountDirs)) { Volumes[containerDir] = {}; } // Create bind volumes const Binds: string[] = []; - for (const [hostDir, containerDir] of mountDirs.entries()) { - Binds.push(`${hostDir}:${containerDir}`); + for (const [hostDir, containerDir] of Object.entries(mountDirs)) { + // Need to use realpath here as Docker mounting does not like + // symlinks for binding volumes + const realHostDir = await fs.realpath(hostDir); + Binds.push(`${realHostDir}:${containerDir}`); } const [{ Error: error, StatusCode: statusCode }] = await dockerClient.run( diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index ab7b730106..a17ab94a2a 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -16,7 +16,6 @@ import { runDockerContainer } from '@backstage/backend-common'; import { Config } from '@backstage/config'; -import fs from 'fs-extra'; import path from 'path'; import { PassThrough } from 'stream'; import { Logger } from 'winston'; @@ -80,12 +79,10 @@ export class TechdocsGenerator implements GeneratorBase { } // Directories to bind on container - const mountDirs = new Map([ - // Need to use realpath here as Docker mounting does not like - // symlinks for binding volumes - [await fs.realpath(inputDir), '/input'], - [await fs.realpath(outputDir), '/output'], - ]); + const mountDirs = { + [inputDir]: '/input', + [outputDir]: '/output', + }; try { switch (this.options.runGeneratorIn) { diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts index a48a117639..cadff9e016 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts @@ -160,10 +160,10 @@ describe('CookieCutter Templater', () => { '--verbose', ], envVars: ['HOME=/tmp'], - mountDirs: new Map([ - [path.join('tempdir', 'template'), '/input'], - [path.join('tempdir', 'intermediate'), '/output'], - ]), + mountDirs: { + [path.join('tempdir', 'template')]: '/input', + [path.join('tempdir', 'intermediate')]: '/output', + }, workingDir: '/input', logStream: undefined, dockerClient: mockDocker, @@ -203,10 +203,10 @@ describe('CookieCutter Templater', () => { '--verbose', ], envVars: ['HOME=/tmp'], - mountDirs: new Map([ - [path.join('tempdir', 'template'), '/input'], - [path.join('tempdir', 'intermediate'), '/output'], - ]), + mountDirs: { + [path.join('tempdir', 'template')]: '/input', + [path.join('tempdir', 'intermediate')]: '/output', + }, workingDir: '/input', logStream: stream, dockerClient: mockDocker, diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts index 39e6d7be42..73822b150e 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts @@ -59,12 +59,10 @@ export class CookieCutter implements TemplaterBase { await fs.writeJSON(path.join(templateDir, 'cookiecutter.json'), cookieInfo); // Directories to bind on container - const mountDirs = new Map([ - // Need to use realpath here as Docker mounting does not like - // symlinks for binding volumes - [await fs.realpath(templateDir), '/input'], - [await fs.realpath(intermediateDir), '/output'], - ]); + const mountDirs = { + [templateDir]: '/input', + [intermediateDir]: '/output', + }; const cookieCutterInstalled = await commandExists('cookiecutter'); if (cookieCutterInstalled) { diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts index 371586d1da..3006b924d5 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts @@ -40,10 +40,10 @@ export class CreateReactAppTemplater implements TemplaterBase { const intermediateDir = path.join(workspacePath, 'template'); await fs.ensureDir(intermediateDir); - const mountDirs = new Map([ - [await fs.realpath(intermediateDir), '/template'], - [await fs.realpath(intermediateDir), '/result'], - ]); + const mountDirs = { + [intermediateDir]: '/template', + [intermediateDir]: '/result', + }; await runDockerContainer({ imageName: 'node:lts-alpine', From 3f2a0efe01b792eed310d0089fc1739acde14b20 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 8 Mar 2021 14:00:17 +0100 Subject: [PATCH 11/12] backend-common: Use Record for envVars in runDockerContainer Signed-off-by: Himanshu Mishra --- packages/backend-common/src/util/docker.test.ts | 5 +++-- packages/backend-common/src/util/docker.ts | 14 ++++++++++---- .../src/stages/generate/techdocs.ts | 2 +- .../scaffolder/stages/templater/cookiecutter.ts | 2 +- .../src/scaffolder/stages/templater/cra/index.ts | 2 +- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/packages/backend-common/src/util/docker.test.ts b/packages/backend-common/src/util/docker.test.ts index a6cc9889fe..2ec14494da 100644 --- a/packages/backend-common/src/util/docker.test.ts +++ b/packages/backend-common/src/util/docker.test.ts @@ -62,7 +62,8 @@ describe('runDockerContainer', () => { [path.join(rootDir, 'output')]: '/output', }; const workingDir = path.join(rootDir, 'input'); - const envVars = ['HOME=/tmp', 'LOG_LEVEL=debug']; + const envVars = { HOME: '/tmp', LOG_LEVEL: 'debug' }; + const envVarsArray = ['HOME=/tmp', 'LOG_LEVEL=debug']; it('should pull the docker container', async () => { await runDockerContainer({ @@ -95,7 +96,7 @@ describe('runDockerContainer', () => { args, expect.any(Stream), expect.objectContaining({ - Env: envVars, + Env: envVarsArray, WorkingDir: workingDir, HostConfig: { Binds: expect.arrayContaining([ diff --git a/packages/backend-common/src/util/docker.ts b/packages/backend-common/src/util/docker.ts index 4e143a6d1b..356d645465 100644 --- a/packages/backend-common/src/util/docker.ts +++ b/packages/backend-common/src/util/docker.ts @@ -29,7 +29,7 @@ export type RunDockerContainerOptions = { dockerClient: Docker; mountDirs?: Record; workingDir?: string; - envVars?: string[]; + envVars?: Record; createOptions?: Docker.ContainerCreateOptions; }; @@ -43,7 +43,7 @@ export type RunDockerContainerOptions = { * @param options.mountDirs A map of host directories to mount on the container. * Object Key: Path on host machine, Value: Path on Docker container * @param options.workingDir Working dir in the container - * @param options.envVars Environment variables to set in the container. e.g. ['HOME=/tmp'] + * @param options.envVars Environment variables to set in the container. e.g. {'HOME': '/tmp'} */ export const runDockerContainer = async ({ imageName, @@ -52,7 +52,7 @@ export const runDockerContainer = async ({ dockerClient, mountDirs = {}, workingDir, - envVars = [], + envVars = {}, createOptions = {}, }: RunDockerContainerOptions) => { // Show a better error message when Docker is unavailable. @@ -99,6 +99,12 @@ export const runDockerContainer = async ({ Binds.push(`${realHostDir}:${containerDir}`); } + // Create docker environment variables array + const Env = []; + for (const [key, value] of Object.entries(envVars)) { + Env.push(`${key}=${value}`); + } + const [{ Error: error, StatusCode: statusCode }] = await dockerClient.run( imageName, args, @@ -109,7 +115,7 @@ export const runDockerContainer = async ({ Binds, }, ...(workingDir ? { WorkingDir: workingDir } : {}), - Env: envVars, + Env, ...userOptions, ...createOptions, }, diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index a17ab94a2a..0dfa952a17 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -108,7 +108,7 @@ export class TechdocsGenerator implements GeneratorBase { workingDir: '/input', // Set the home directory inside the container as something that applications can // write to, otherwise they will just fail trying to write to / - envVars: ['HOME=/tmp'], + envVars: { HOME: '/tmp' }, dockerClient, }); this.logger.info( diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts index 73822b150e..b534610a39 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.ts @@ -86,7 +86,7 @@ export class CookieCutter implements TemplaterBase { workingDir: '/input', // Set the home directory inside the container as something that applications can // write to, otherwise they will just fail trying to write to / - envVars: ['HOME=/tmp'], + envVars: { HOME: '/tmp' }, logStream, dockerClient, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts index 3006b924d5..96050a14f9 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cra/index.ts @@ -57,7 +57,7 @@ export class CreateReactAppTemplater implements TemplaterBase { dockerClient: dockerClient, // Set the home directory inside the container as something that applications can // write to, otherwise they will just fail trying to write to / - envVars: ['HOME=/tmp'], + envVars: { HOME: '/tmp' }, createOptions: { Entrypoint: ['npx'], WorkingDir: '/result', From 4d9d38d2ded4692a1047f16749c97b0e106ccedb Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Mon, 8 Mar 2021 16:49:29 +0100 Subject: [PATCH 12/12] backend-common: Update tests with new envVars format Signed-off-by: Himanshu Mishra --- .../src/scaffolder/stages/templater/cookiecutter.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts index cadff9e016..537aac3088 100644 --- a/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/stages/templater/cookiecutter.test.ts @@ -159,7 +159,7 @@ describe('CookieCutter Templater', () => { '/input', '--verbose', ], - envVars: ['HOME=/tmp'], + envVars: { HOME: '/tmp' }, mountDirs: { [path.join('tempdir', 'template')]: '/input', [path.join('tempdir', 'intermediate')]: '/output', @@ -202,7 +202,7 @@ describe('CookieCutter Templater', () => { '/input', '--verbose', ], - envVars: ['HOME=/tmp'], + envVars: { HOME: '/tmp' }, mountDirs: { [path.join('tempdir', 'template')]: '/input', [path.join('tempdir', 'intermediate')]: '/output',