diff --git a/.changeset/silent-pillows-reflect.md b/.changeset/silent-pillows-reflect.md new file mode 100644 index 0000000000..fe755056cc --- /dev/null +++ b/.changeset/silent-pillows-reflect.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Refactoring the runner to generate minimally informative task log per iteration and properly validate iterated actions. diff --git a/plugins/scaffolder-backend/package.json b/plugins/scaffolder-backend/package.json index e0b9e5b993..df1298f93e 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 6907f8cc2c..3085bb8687 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', () => { +describe('NunjucksWorkflowRunner', () => { 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,81 @@ 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 () => { + 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); + + 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 () => { + 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(); }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 1f93ded94c..58ea689260 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -276,74 +276,71 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { return; } } + 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 + input: step.input + ? this.render( + step.input, + { ...context, ...i, ...task }, + renderTemplate, + ) + : {}, + })); + for (const iteration of iterations) { + const actionId = + action.id + (iteration.each ? `[${iteration.each.key}]` : ''); - // 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 (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, + )}`, ); } } - - 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(); - 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] }; - }), - ); - } 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, - )) ?? - {}; + 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, secrets: task.secrets ?? {}, logger: taskLogger, logStream: streamLogger, diff --git a/yarn.lock b/yarn.lock index 5b17c25579..22544627c2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8900,6 +8900,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 @@ -42809,12 +42810,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