From 6dad04afa7bfcd43ee062331dd903ea70cec079a Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 12 Jun 2024 11:47:29 +0200 Subject: [PATCH 1/4] chore: fixing redactor formatting Signed-off-by: blam --- .../tasks/NunjucksWorkflowRunner.test.ts | 92 +++++++++++++++++++ .../tasks/NunjucksWorkflowRunner.ts | 14 +-- .../src/scaffolder/tasks/logger.ts | 40 ++++++-- 3 files changed, 123 insertions(+), 23 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 5d70e5bfba..daa5e80124 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -644,6 +644,98 @@ describe('NunjucksWorkflowRunner', () => { }); }); + describe('redactions', () => { + it('should redact secrets that are passed with the task', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.info(ctx.input.secret); + }, + schema: { + input: { + type: 'object', + required: ['secret'], + properties: { + secret: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + apiVersion: 'scaffolder.backstage.io/v1beta3', + parameters: {}, + output: {}, + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + secret: '${{ secrets.secret }}', + }, + }, + ], + }, + { secret: 'my-secret-value' }, + ); + + await runner.execute(task); + + expectTaskLog('info: ***'); + }); + + it('should redact meta fields properly', async () => { + actionRegistry.register({ + id: 'log-secret', + description: 'Mock action for testing', + supportsDryRun: true, + handler: async ctx => { + ctx.logger.child({ thing: ctx.input.secret }).info(ctx.input.secret); + }, + schema: { + input: { + type: 'object', + required: ['secret'], + properties: { + secret: { + type: 'string', + }, + }, + }, + }, + }); + + const task = createMockTaskWithSpec( + { + apiVersion: 'scaffolder.backstage.io/v1beta3', + parameters: {}, + output: {}, + steps: [ + { + id: 'test', + name: 'name', + action: 'log-secret', + input: { + secret: '${{ secrets.secret }}', + }, + }, + ], + }, + { secret: 'my-secret-value' }, + ); + + await runner.execute(task); + + expectTaskLog('info: *** {"thing":"***"}'); + }); + }); + describe('each', () => { it('should run a step repeatedly - flat values', async () => { const colors = ['blue', 'green', 'red']; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index 85664e1a6c..a80fdb68ec 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -97,31 +97,19 @@ const isValidTaskSpec = (taskSpec: TaskSpec): taskSpec is TaskSpecV1beta3 => { const createStepLogger = ({ task, - step, rootLogger, }: { task: TaskContext; step: TaskStep; rootLogger: winston.Logger; }) => { - const stepLogStream = new PassThrough(); - stepLogStream.on('data', async data => { - const message = data.toString().trim(); - if (message?.length > 1) { - await task.emitLog(message, { stepId: step.id }); - } - }); - const taskLogger = WinstonLogger.create({ level: process.env.LOG_LEVEL || 'info', format: winston.format.combine( winston.format.colorize(), winston.format.simple(), ), - transports: [ - new winston.transports.Stream({ stream: stepLogStream }), - new BackstageLoggerTransport(rootLogger), - ], + transports: [new BackstageLoggerTransport(rootLogger, task)], }); taskLogger.addRedactions(Object.values(task.secrets ?? {})); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts index e85c61d489..c668c4a589 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts @@ -21,7 +21,9 @@ import { JsonObject } from '@backstage/types'; import { Format, TransformableInfo } from 'logform'; import Transport, { TransportStreamOptions } from 'winston-transport'; import { Logger, format, createLogger, transports } from 'winston'; -import { MESSAGE } from 'triple-beam'; +import { LEVEL, MESSAGE, SPLAT } from 'triple-beam'; +import { TaskContext } from '@backstage/plugin-scaffolder-node'; +import _ from 'lodash'; /** * Escapes a given string to be used inside a RegExp. @@ -44,33 +46,41 @@ interface WinstonLoggerOptions { export class BackstageLoggerTransport extends Transport { constructor( private readonly backstageLogger: LoggerService, + private readonly taskContext: TaskContext, opts?: TransportStreamOptions, ) { super(opts); } - log(info: unknown, callback: VoidFunction) { + log(info: TransformableInfo, callback: VoidFunction) { if (typeof info !== 'object' || info === null) { callback(); return; } - const { level, message, ...meta } = info as JsonObject; + + const message = info[MESSAGE]; + const level = info[LEVEL]; + const splat = info[SPLAT]; + switch (level) { case 'error': - this.backstageLogger.error(String(message), meta); + this.backstageLogger.error(String(message), ...splat); break; case 'warn': - this.backstageLogger.warn(String(message), meta); + this.backstageLogger.warn(String(message), ...splat); break; case 'info': - this.backstageLogger.info(String(message), meta); + this.backstageLogger.info(String(message), ...splat); break; case 'debug': - this.backstageLogger.debug(String(message), meta); + this.backstageLogger.debug(String(message), ...splat); break; default: - this.backstageLogger.info(String(message), meta); + this.backstageLogger.info(String(message), ...splat); } + + this.taskContext.emitLog(message, ...splat); + callback(); } } @@ -87,9 +97,10 @@ export class WinstonLogger implements RootLoggerService { let logger = createLogger({ level: options.level, - format: format.combine(redacter.format, options.format), + format: format.combine(options.format, redacter.format), transports: options.transports ?? new transports.Console(), }); + if (options.meta) { logger = logger.child(options.meta); } @@ -116,6 +127,12 @@ export class WinstonLogger implements RootLoggerService { obj[MESSAGE] = obj[MESSAGE]?.replace?.(redactionPattern, '***'); + if (obj[SPLAT]) { + obj[SPLAT] = JSON.parse( + JSON.stringify(obj[SPLAT]).replace(redactionPattern, '***'), + ); + } + return obj; })(), add(newRedactions) { @@ -163,7 +180,10 @@ export class WinstonLogger implements RootLoggerService { }, }), format.printf((info: TransformableInfo) => { - const { timestamp, level, message, plugin, service, ...fields } = info; + const { timestamp, plugin, service } = info; + const message = info[MESSAGE]; + const level = info[LEVEL]; + const fields = info[SPLAT]; const prefix = plugin || service; const timestampColor = colorizer.colorize('timestamp', timestamp); const prefixColor = colorizer.colorize('prefix', prefix); From 510d0f2a8d5fc719f881bb8f821a677361aae3f8 Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 12 Jun 2024 12:35:27 +0200 Subject: [PATCH 2/4] chore: refactor a little bit Signed-off-by: blam --- .../src/scaffolder/tasks/NunjucksWorkflowRunner.ts | 3 ++- plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index a80fdb68ec..e9b1d78e36 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -97,6 +97,7 @@ const isValidTaskSpec = (taskSpec: TaskSpec): taskSpec is TaskSpecV1beta3 => { const createStepLogger = ({ task, + step, rootLogger, }: { task: TaskContext; @@ -109,7 +110,7 @@ const createStepLogger = ({ winston.format.colorize(), winston.format.simple(), ), - transports: [new BackstageLoggerTransport(rootLogger, task)], + transports: [new BackstageLoggerTransport(rootLogger, task, step.id)], }); taskLogger.addRedactions(Object.values(task.secrets ?? {})); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts index c668c4a589..1c413547cb 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts @@ -24,6 +24,7 @@ import { Logger, format, createLogger, transports } from 'winston'; import { LEVEL, MESSAGE, SPLAT } from 'triple-beam'; import { TaskContext } from '@backstage/plugin-scaffolder-node'; import _ from 'lodash'; +import { TaskStep } from '@backstage/plugin-scaffolder-common'; /** * Escapes a given string to be used inside a RegExp. @@ -47,6 +48,7 @@ export class BackstageLoggerTransport extends Transport { constructor( private readonly backstageLogger: LoggerService, private readonly taskContext: TaskContext, + private readonly stepId: string, opts?: TransportStreamOptions, ) { super(opts); @@ -79,8 +81,7 @@ export class BackstageLoggerTransport extends Transport { this.backstageLogger.info(String(message), ...splat); } - this.taskContext.emitLog(message, ...splat); - + this.taskContext.emitLog(message, { stepId: this.stepId }); callback(); } } From 8e60a63364491e0d0c5d8db19f6dedaaa0e750be Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 12 Jun 2024 12:37:05 +0200 Subject: [PATCH 3/4] chore: refactor a little more Signed-off-by: blam --- plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts index 1c413547cb..a75f31a07f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts @@ -128,12 +128,6 @@ export class WinstonLogger implements RootLoggerService { obj[MESSAGE] = obj[MESSAGE]?.replace?.(redactionPattern, '***'); - if (obj[SPLAT]) { - obj[SPLAT] = JSON.parse( - JSON.stringify(obj[SPLAT]).replace(redactionPattern, '***'), - ); - } - return obj; })(), add(newRedactions) { From 5c65785ae34ac56991e705bb0b7602fedbe9c8fa Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 12 Jun 2024 12:37:47 +0200 Subject: [PATCH 4/4] chore: add changeset Signed-off-by: blam --- .changeset/neat-chairs-complain.md | 5 +++++ .../src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts | 2 ++ plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts | 1 - 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 .changeset/neat-chairs-complain.md diff --git a/.changeset/neat-chairs-complain.md b/.changeset/neat-chairs-complain.md new file mode 100644 index 0000000000..96c0271d81 --- /dev/null +++ b/.changeset/neat-chairs-complain.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Fixing issues with log redaction in the scaffolder logs diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index daa5e80124..aece424f5f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -645,6 +645,7 @@ describe('NunjucksWorkflowRunner', () => { }); describe('redactions', () => { + // eslint-disable-next-line jest/expect-expect it('should redact secrets that are passed with the task', async () => { actionRegistry.register({ id: 'log-secret', @@ -690,6 +691,7 @@ describe('NunjucksWorkflowRunner', () => { expectTaskLog('info: ***'); }); + // eslint-disable-next-line jest/expect-expect it('should redact meta fields properly', async () => { actionRegistry.register({ id: 'log-secret', diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts index a75f31a07f..38263b73ca 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/logger.ts @@ -24,7 +24,6 @@ import { Logger, format, createLogger, transports } from 'winston'; import { LEVEL, MESSAGE, SPLAT } from 'triple-beam'; import { TaskContext } from '@backstage/plugin-scaffolder-node'; import _ from 'lodash'; -import { TaskStep } from '@backstage/plugin-scaffolder-common'; /** * Escapes a given string to be used inside a RegExp.