From 1a6875274b8dc2dd9deb1eb08b96fd377bb97d6c Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 23 Oct 2023 17:47:44 -0500 Subject: [PATCH 1/9] add failing test for single template expression input values in each-iterated actions Signed-off-by: Matt Benson --- .../tasks/NunjucksWorkflowRunner.test.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 6907f8cc2c..30856482a1 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -645,6 +645,35 @@ describe('DefaultWorkflowRunner', () => { }), ); }); + + it('should run a step repeatedly with validation of single-expression value', async () => { + const numbers = [5, 7, 9]; + const task = createMockTaskWithSpec({ + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [ + { + id: 'test', + name: 'name', + each: '${{parameters.numbers}}', + action: 'jest-validated-action', + input: { foo: '${{each.value}}' }, + }, + ], + output: {}, + parameters: { + numbers, + }, + }); + await runner.execute(task); + + for (const foo of numbers) { + expect(fakeActionHandler).toHaveBeenCalledWith( + expect.objectContaining({ + input: { foo }, + }), + ); + } + }); }); describe('secrets', () => { From 8acc167dfc90174ac58669de0a584480c544a055 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 23 Oct 2023 17:50:11 -0500 Subject: [PATCH 2/9] refactor NunjucksWorkflowTaskRunner to: - generate minimally informative task log per iteration - properly validate iterated actions Signed-off-by: Matt Benson --- .../tasks/NunjucksWorkflowRunner.ts | 110 +++++++++--------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 1f93ded94c..9d0ab0ad06 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -277,73 +277,73 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { } } - // Secrets are only passed when templating the input to actions for security reasons - const input = - (step.input && - this.render( - step.input, - { ...context, secrets: task.secrets ?? {} }, - renderTemplate, - )) ?? - {}; - - if (action.schema?.input) { - const validateResult = validateJsonSchema(input, action.schema.input); - if (!validateResult.valid) { - const errors = validateResult.errors.join(', '); - throw new InputError( - `Invalid input passed to action ${action.id}, ${errors}`, - ); - } - } - - if (!isActionAuthorized(decision, { action: action.id, input })) { - throw new NotAllowedError( - `Unauthorized action: ${ - action.id - }. The action is not allowed. Input: ${JSON.stringify( - input, - null, - 2, - )}`, - ); - } - - const tmpDirs = new Array(); - const stepOutput: { [outputName: string]: JsonValue } = {}; - - const iterations = new Array(); + const iterations = new Array(); if (step.each) { const each = await this.render(step.each, context, renderTemplate); iterations.push( ...Object.keys(each).map((key: any) => { - return { key: key, value: each[key] }; + return { each: { key, value: each[key] } }; }), ); } else { iterations.push({}); } - - let actionInput = input; for (const iteration of iterations) { - if (step.each) { - taskLogger.info(`Running step each: ${iteration}`); - const iterationContext = { - ...context, - each: iteration, - }; - // re-render input with the modified context that includes each - actionInput = - (step.input && - this.render( - step.input, - { ...iterationContext, secrets: task.secrets ?? {} }, - renderTemplate, - )) ?? - {}; + // Secrets are only passed when templating the input to actions for security reasons + iteration.input = + (step.input && + this.render( + step.input, + { ...context, ...iteration, secrets: task.secrets ?? {} }, + renderTemplate, + )) ?? + {}; + let actionId = action.id; + if (Object.hasOwn(iteration, 'each')) { + actionId += `[${(iteration.each as JsonObject).key}]`; + } + if (action.schema?.input) { + const validateResult = validateJsonSchema( + iteration.input, + action.schema.input, + ); + if (!validateResult.valid) { + const errors = validateResult.errors.join(', '); + throw new InputError( + `Invalid input passed to action ${actionId}, ${errors}`, + ); + } + } + if ( + !isActionAuthorized(decision, { + action: action.id, + input: iteration.input, + }) + ) { + throw new NotAllowedError( + `Unauthorized action: ${actionId}. The action is not allowed. Input: ${JSON.stringify( + iteration.input, + null, + 2, + )}`, + ); + } + } + const tmpDirs = new Array(); + const stepOutput: { [outputName: string]: JsonValue } = {}; + + for (const iteration of iterations) { + if (iteration.each) { + taskLogger.info( + `Running step each: ${JSON.stringify( + iteration.each, + (k, v) => (k ? v.toString() : v), + 0, + )}`, + ); } await action.handler({ - input: actionInput, + input: iteration.input as JsonObject, secrets: task.secrets ?? {}, logger: taskLogger, logStream: streamLogger, From 5696c46d0b87ccb3fb88075f2057bb6b6d11cd46 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 23 Oct 2023 20:25:45 -0500 Subject: [PATCH 3/9] test that one invalid iteration blocks the entire iterated action and identifies the offending key Signed-off-by: Matt Benson --- .../tasks/NunjucksWorkflowRunner.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 30856482a1..076c9e1a11 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -674,6 +674,34 @@ describe('DefaultWorkflowRunner', () => { ); } }); + + it('should validate each action iteration', async () => { + const task = createMockTaskWithSpec({ + apiVersion: 'scaffolder.backstage.io/v1beta3', + steps: [ + { + id: 'test', + name: 'name', + each: '${{parameters.data}}', + action: 'jest-validated-action', + input: { foo: '${{each.value.foo}}' }, + }, + ], + output: {}, + parameters: { + data: [ + { + foo: 0, + }, + {}, + ], + }, + }); + await expect(runner.execute(task)).rejects.toThrow( + 'Invalid input passed to action jest-validated-action[1], instance requires property "foo"', + ); + expect(fakeActionHandler).not.toHaveBeenCalled(); + }); }); describe('secrets', () => { From 8673b8257efe02896c62412cfda2c305af61f60e Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 23 Oct 2023 21:51:59 -0500 Subject: [PATCH 4/9] test scaffolder iteration info messages Signed-off-by: Matt Benson --- plugins/scaffolder-backend/package.json | 1 + .../tasks/NunjucksWorkflowRunner.test.ts | 70 ++++++++++++------- yarn.lock | 9 +-- 3 files changed, 49 insertions(+), 31 deletions(-) diff --git a/plugins/scaffolder-backend/package.json b/plugins/scaffolder-backend/package.json index 20c56e9a8d..afa0d883cc 100644 --- a/plugins/scaffolder-backend/package.json +++ b/plugins/scaffolder-backend/package.json @@ -113,6 +113,7 @@ "esbuild": "^0.19.0", "jest-when": "^3.1.0", "msw": "^1.0.0", + "strip-ansi": "^7.1.0", "supertest": "^6.1.3", "wait-for-expect": "^3.0.2", "yaml": "^2.0.0" diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 076c9e1a11..a50baa9944 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -34,12 +34,14 @@ import { } from '@backstage/plugin-permission-common'; import { RESOURCE_TYPE_SCAFFOLDER_ACTION } from '@backstage/plugin-scaffolder-common/alpha'; import { createMockDirectory } from '@backstage/backend-test-utils'; +import stripAnsi from 'strip-ansi'; describe('DefaultWorkflowRunner', () => { const logger = getVoidLogger(); let actionRegistry = new TemplateActionRegistry(); let runner: NunjucksWorkflowRunner; let fakeActionHandler: jest.Mock; + let fakeTaskLog: jest.Mock; const mockDir = createMockDirectory(); @@ -65,17 +67,24 @@ describe('DefaultWorkflowRunner', () => { isDryRun, complete: async () => {}, done: false, - emitLog: async () => {}, + emitLog: fakeTaskLog, cancelSignal: new AbortController().signal, getWorkspaceName: () => Promise.resolve('test-workspace'), }); + function expectTaskLog(message: string) { + expect(fakeTaskLog.mock.calls.map(args => stripAnsi(args[0]))).toContain( + message, + ); + } + beforeEach(() => { mockDir.clear(); jest.resetAllMocks(); actionRegistry = new TemplateActionRegistry(); fakeActionHandler = jest.fn(); + fakeTaskLog = jest.fn(); actionRegistry.register({ id: 'jest-mock-action', @@ -554,6 +563,7 @@ describe('DefaultWorkflowRunner', () => { describe('each', () => { it('should run a step repeatedly - flat values', async () => { + const colors = ['blue', 'green', 'red']; const task = createMockTaskWithSpec({ apiVersion: 'scaffolder.backstage.io/v1beta3', steps: [ @@ -567,21 +577,19 @@ describe('DefaultWorkflowRunner', () => { ], output: {}, parameters: { - colors: ['blue', 'green', 'red'], + colors, }, }); - await runner.execute(task); - expect(fakeActionHandler).toHaveBeenCalledWith( - expect.objectContaining({ input: { color: 'blue' } }), - ); - expect(fakeActionHandler).toHaveBeenCalledWith( - expect.objectContaining({ input: { color: 'green' } }), - ); - expect(fakeActionHandler).toHaveBeenCalledWith( - expect.objectContaining({ input: { color: 'red' } }), - ); + colors.forEach((color, idx) => { + expectTaskLog( + `info: Running step each: {"key":"${idx}","value":"${color}"}`, + ); + expect(fakeActionHandler).toHaveBeenCalledWith( + expect.objectContaining({ input: { color } }), + ); + }); }); it('should run a step repeatedly - object list', async () => { @@ -604,9 +612,11 @@ describe('DefaultWorkflowRunner', () => { settings: [{ color: 'blue' }], }, }); - await runner.execute(task); + expectTaskLog( + 'info: Running step each: {"key":"0","value":"[object Object]"}', + ); expect(fakeActionHandler).toHaveBeenCalledWith( expect.objectContaining({ input: { key: '0', value: { color: 'blue' } }, @@ -615,6 +625,10 @@ describe('DefaultWorkflowRunner', () => { }); it('should run a step repeatedly - object', async () => { + const settings = { + color: 'blue', + transparent: 'yes', + }; const task = createMockTaskWithSpec({ apiVersion: 'scaffolder.backstage.io/v1beta3', steps: [ @@ -628,22 +642,21 @@ describe('DefaultWorkflowRunner', () => { ], output: {}, parameters: { - settings: { color: 'blue', transparent: 'yes' }, + settings, }, }); - await runner.execute(task); - expect(fakeActionHandler).toHaveBeenCalledWith( - expect.objectContaining({ - input: { key: 'color', value: 'blue' }, - }), - ); - expect(fakeActionHandler).toHaveBeenCalledWith( - expect.objectContaining({ - input: { key: 'transparent', value: 'yes' }, - }), - ); + for (const [key, value] of Object.entries(settings)) { + expectTaskLog( + `info: Running step each: {"key":"${key}","value":"${value}"}`, + ); + expect(fakeActionHandler).toHaveBeenCalledWith( + expect.objectContaining({ + input: { key, value }, + }), + ); + } }); it('should run a step repeatedly with validation of single-expression value', async () => { @@ -666,13 +679,16 @@ describe('DefaultWorkflowRunner', () => { }); await runner.execute(task); - for (const foo of numbers) { + numbers.forEach((foo, idx) => { + expectTaskLog( + `info: Running step each: {"key":"${idx}","value":"${foo}"}`, + ); expect(fakeActionHandler).toHaveBeenCalledWith( expect.objectContaining({ input: { foo }, }), ); - } + }); }); it('should validate each action iteration', async () => { diff --git a/yarn.lock b/yarn.lock index 70667644f7..115cfb9137 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8634,6 +8634,7 @@ __metadata: p-limit: ^3.1.0 p-queue: ^6.6.2 prom-client: ^14.0.1 + strip-ansi: ^7.1.0 supertest: ^6.1.3 uuid: ^8.2.0 wait-for-expect: ^3.0.2 @@ -40763,12 +40764,12 @@ __metadata: languageName: node linkType: hard -"strip-ansi@npm:^7.0.1": - version: 7.0.1 - resolution: "strip-ansi@npm:7.0.1" +"strip-ansi@npm:^7.0.1, strip-ansi@npm:^7.1.0": + version: 7.1.0 + resolution: "strip-ansi@npm:7.1.0" dependencies: ansi-regex: ^6.0.1 - checksum: 257f78fa433520e7f9897722731d78599cb3fce29ff26a20a5e12ba4957463b50a01136f37c43707f4951817a75e90820174853d6ccc240997adc5df8f966039 + checksum: 859c73fcf27869c22a4e4d8c6acfe690064659e84bef9458aa6d13719d09ca88dcfd40cbf31fd0be63518ea1a643fe070b4827d353e09533a5b0b9fd4553d64d languageName: node linkType: hard From 23f72b2cbaa0040ab08958689ac52af6ca8db054 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 23 Oct 2023 22:19:33 -0500 Subject: [PATCH 5/9] refactor NunjucksWorkflowTaskRunner to: - generate minimally informative task log per iteration - properly validate iterated actions Signed-off-by: Matt Benson --- .changeset/silent-pillows-reflect.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/silent-pillows-reflect.md diff --git a/.changeset/silent-pillows-reflect.md b/.changeset/silent-pillows-reflect.md new file mode 100644 index 0000000000..95735e64c5 --- /dev/null +++ b/.changeset/silent-pillows-reflect.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +refactor NunjucksWorkflowTaskRunner to: + +- generate minimally informative task log per iteration +- properly validate iterated actions From 6eb9446ccf84a0b41bdc084e2d13be9b67b0a3e7 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 6 Nov 2023 19:11:43 -0600 Subject: [PATCH 6/9] correct name of tested class Signed-off-by: Matt Benson --- .../src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index a50baa9944..3085bb8687 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -36,7 +36,7 @@ import { RESOURCE_TYPE_SCAFFOLDER_ACTION } from '@backstage/plugin-scaffolder-co import { createMockDirectory } from '@backstage/backend-test-utils'; import stripAnsi from 'strip-ansi'; -describe('DefaultWorkflowRunner', () => { +describe('NunjucksWorkflowRunner', () => { const logger = getVoidLogger(); let actionRegistry = new TemplateActionRegistry(); let runner: NunjucksWorkflowRunner; From 661c787b150d6b7882f593e1b95c7b1f870da4f6 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Mon, 6 Nov 2023 19:15:59 -0600 Subject: [PATCH 7/9] refactor: const + infer type for workflow runner iterations Signed-off-by: Matt Benson --- .../tasks/NunjucksWorkflowRunner.ts | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 9d0ab0ad06..4f0d6f7091 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -276,31 +276,29 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { return; } } - - const iterations = new Array(); - if (step.each) { - const each = await this.render(step.each, context, renderTemplate); - iterations.push( - ...Object.keys(each).map((key: any) => { - return { each: { key, value: each[key] } }; - }), - ); - } else { - iterations.push({}); - } - for (const iteration of iterations) { + const iterations = ( + step.each + ? Object.entries(this.render(step.each, context, renderTemplate)).map( + ([key, value]) => ({ + each: { key, value }, + }), + ) + : [{}] + ).map(i => ({ + ...i, // Secrets are only passed when templating the input to actions for security reasons - iteration.input = - (step.input && - this.render( + input: step.input + ? this.render( step.input, - { ...context, ...iteration, secrets: task.secrets ?? {} }, + { ...context, ...i, ...task }, renderTemplate, - )) ?? - {}; + ) + : {}, + })); + for (const iteration of iterations) { let actionId = action.id; - if (Object.hasOwn(iteration, 'each')) { - actionId += `[${(iteration.each as JsonObject).key}]`; + if (iteration.each) { + actionId += `[${iteration.each.key}]`; } if (action.schema?.input) { const validateResult = validateJsonSchema( @@ -343,7 +341,7 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { ); } await action.handler({ - input: iteration.input as JsonObject, + input: iteration.input, secrets: task.secrets ?? {}, logger: taskLogger, logStream: streamLogger, From cf5d275ad21015ee1fee6704b912f9231fb5deb8 Mon Sep 17 00:00:00 2001 From: Matt Benson Date: Tue, 7 Nov 2023 10:04:53 -0600 Subject: [PATCH 8/9] const actionId Signed-off-by: Matt Benson --- .../src/scaffolder/tasks/NunjucksWorkflowRunner.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 4f0d6f7091..58ea689260 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -296,10 +296,9 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { : {}, })); for (const iteration of iterations) { - let actionId = action.id; - if (iteration.each) { - actionId += `[${iteration.each.key}]`; - } + const actionId = + action.id + (iteration.each ? `[${iteration.each.key}]` : ''); + if (action.schema?.input) { const validateResult = validateJsonSchema( iteration.input, From cc6765ecc7559ed5afe47c47ed27fad064b6fc7a Mon Sep 17 00:00:00 2001 From: Ben Lambert Date: Wed, 8 Nov 2023 14:26:25 -0600 Subject: [PATCH 9/9] Update silent-pillows-reflect.md Signed-off-by: Ben Lambert --- .changeset/silent-pillows-reflect.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.changeset/silent-pillows-reflect.md b/.changeset/silent-pillows-reflect.md index 95735e64c5..fe755056cc 100644 --- a/.changeset/silent-pillows-reflect.md +++ b/.changeset/silent-pillows-reflect.md @@ -2,7 +2,4 @@ '@backstage/plugin-scaffolder-backend': patch --- -refactor NunjucksWorkflowTaskRunner to: - -- generate minimally informative task log per iteration -- properly validate iterated actions +Refactoring the runner to generate minimally informative task log per iteration and properly validate iterated actions.