From a003fdb21bba05c5448abb2da1aa117ecc0b139e Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Tue, 30 Mar 2021 16:23:30 +0000 Subject: [PATCH 1/8] Run substitute prior to include processing Signed-off-by: JP Dhabolt john.p.dhabolt@gmail.com Signed-off-by: JP Dhabolt --- .../src/lib/transform/include.test.ts | 34 ++++++++++++++++++- .../src/lib/transform/include.ts | 7 +++- .../config-loader/src/lib/transform/types.ts | 15 +++----- packages/config-loader/src/loader.ts | 5 +-- 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/packages/config-loader/src/lib/transform/include.test.ts b/packages/config-loader/src/lib/transform/include.test.ts index 1724159e56..81c6cfb63b 100644 --- a/packages/config-loader/src/lib/transform/include.test.ts +++ b/packages/config-loader/src/lib/transform/include.test.ts @@ -14,11 +14,14 @@ * limitations under the License. */ +import { JsonValue } from '@backstage/config'; import * as os from 'os'; import { resolve as resolvePath } from 'path'; import { createIncludeTransform } from './include'; const root = os.platform() === 'win32' ? 'C:\\' : '/'; +const substituteMe = '${MY_SUBSTITUTION}'; +const mySubstitution = 'fooSubstitution'; const env = jest.fn(async (name: string) => { return ({ @@ -26,6 +29,24 @@ const env = jest.fn(async (name: string) => { } as { [name: string]: string })[name]; }); +const substitute = jest.fn( + async ( + value: JsonValue, + ): Promise<{ applied: boolean; value?: JsonValue }> => { + if (typeof value !== 'string') { + return { applied: false }; + } + if (value.includes(substituteMe)) { + return { + applied: true, + value: value.replace(substituteMe, mySubstitution), + }; + } + + return { applied: false }; + }, +); + const readFile = jest.fn(async (path: string) => { const content = ({ [resolvePath(root, 'my-secret')]: 'secret', @@ -33,6 +54,7 @@ const readFile = jest.fn(async (path: string) => { [resolvePath(root, 'my-data.yaml')]: 'some:\n yaml:\n key: 7', [resolvePath(root, 'my-data.yml')]: 'different: { key: hello }', [resolvePath(root, 'invalid.yaml')]: 'foo: [}', + [resolvePath(root, `${mySubstitution}/my-data.json`)]: '{"foo":"bar"}', } as { [key: string]: string })[path]; if (!content) { @@ -41,7 +63,7 @@ const readFile = jest.fn(async (path: string) => { return content; }); -const includeTransform = createIncludeTransform(env, readFile); +const includeTransform = createIncludeTransform(env, readFile, substitute); describe('includeTransform', () => { it('should not transform unknown values', async () => { @@ -136,4 +158,14 @@ describe('includeTransform', () => { 'failed to parse included file invalid.yaml, YAMLSyntaxError: Flow sequence contains an unexpected }', ); }); + + it('should call substitute prior to handling includes directive', async () => { + await expect( + includeTransform({ $include: `${substituteMe}/my-data.json` }, root), + ).resolves.toEqual({ + applied: true, + value: { foo: 'bar' }, + newBaseDir: `/${mySubstitution}`, + }); + }); }); diff --git a/packages/config-loader/src/lib/transform/include.ts b/packages/config-loader/src/lib/transform/include.ts index 1a6963672a..bffc2e0184 100644 --- a/packages/config-loader/src/lib/transform/include.ts +++ b/packages/config-loader/src/lib/transform/include.ts @@ -35,6 +35,7 @@ const includeFileParser: { export function createIncludeTransform( env: EnvFunc, readFile: ReadFileFunc, + substitute: TransformFunc, ): TransformFunc { return async (input: JsonValue, baseDir: string) => { if (!isObject(input)) { @@ -53,7 +54,11 @@ export function createIncludeTransform( return { applied: false }; } - const includeValue = input[includeKey]; + const rawIncludedValue = input[includeKey]; + const substituteResults = await substitute(rawIncludedValue!, baseDir); + const includeValue = substituteResults.applied + ? substituteResults.value + : rawIncludedValue; if (typeof includeValue !== 'string') { throw new Error(`${includeKey} include value is not a string`); } diff --git a/packages/config-loader/src/lib/transform/types.ts b/packages/config-loader/src/lib/transform/types.ts index c13f01d016..e7ebb0fddc 100644 --- a/packages/config-loader/src/lib/transform/types.ts +++ b/packages/config-loader/src/lib/transform/types.ts @@ -23,13 +23,8 @@ export type ReadFileFunc = (path: string) => Promise; export type TransformFunc = ( value: JsonValue, baseDir: string, -) => Promise< - | { - applied: false; - } - | { - applied: true; - value: JsonValue | undefined; - newBaseDir?: string | undefined; - } ->; +) => Promise<{ + applied: boolean; + value?: JsonValue; + newBaseDir?: string; +}>; diff --git a/packages/config-loader/src/loader.ts b/packages/config-loader/src/loader.ts index 54e4ab5208..95e5b570ca 100644 --- a/packages/config-loader/src/loader.ts +++ b/packages/config-loader/src/loader.ts @@ -75,9 +75,10 @@ export async function loadConfig( fs.readFile(resolvePath(dir, path), 'utf8'); const input = yaml.parse(await readFile(configPath)); + const substitutionTransform = createSubstitutionTransform(env); const data = await applyConfigTransforms(dir, input, [ - createIncludeTransform(env, readFile), - createSubstitutionTransform(env), + createIncludeTransform(env, readFile, substitutionTransform), + substitutionTransform, ]); configs.push({ data, context: basename(configPath) }); From 82c66b8cd5bfdcd34aa544e434330a4317a70dc5 Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Tue, 30 Mar 2021 17:32:02 +0000 Subject: [PATCH 2/8] Add changeset. Signed-off-by: JP Dhabolt john.p.dhabolt@gmail.com Signed-off-by: JP Dhabolt --- .changeset/fair-geese-yell.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/fair-geese-yell.md diff --git a/.changeset/fair-geese-yell.md b/.changeset/fair-geese-yell.md new file mode 100644 index 0000000000..d4d27c4885 --- /dev/null +++ b/.changeset/fair-geese-yell.md @@ -0,0 +1,9 @@ +--- +'@backstage/config-loader': minor +--- + +Add support for environment variable substitution in `$include` transform values. + +This change allows for including dynamic paths, such as environment specific secrets by using the same environment variable substitution already supported outside of the `$include` transform (`${..}`). + +If you are currently using the syntax `${...}` in your `$include` values, you will need to escape the substitution by using `$${...}` instead. From 97beeecb61c4c39a5b3e51a06a9e6dc4fcf8950c Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Tue, 30 Mar 2021 17:50:14 +0000 Subject: [PATCH 3/8] Update docs and changeset Signed-off-by: JP Dhabolt john.p.dhabolt@gmail.com Signed-off-by: JP Dhabolt --- .changeset/fair-geese-yell.md | 11 ++++++++--- docs/conf/writing.md | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/.changeset/fair-geese-yell.md b/.changeset/fair-geese-yell.md index d4d27c4885..5ca86b8913 100644 --- a/.changeset/fair-geese-yell.md +++ b/.changeset/fair-geese-yell.md @@ -2,8 +2,13 @@ '@backstage/config-loader': minor --- -Add support for environment variable substitution in `$include` transform values. +Add support for environment variable substitution in `$include`, `$file` and +`$env` transform values. -This change allows for including dynamic paths, such as environment specific secrets by using the same environment variable substitution already supported outside of the `$include` transform (`${..}`). +This change allows for including dynamic paths, such as environment specific +secrets by using the same environment variable substitution (`${..}`) already +supported outside of the various include transforms. -If you are currently using the syntax `${...}` in your `$include` values, you will need to escape the substitution by using `$${...}` instead. +If you are currently using the syntax `${...}` in your include transform values, +you will need to escape the substitution by using `$${...}` instead to maintain +the same behavior. diff --git a/docs/conf/writing.md b/docs/conf/writing.md index 057675c98e..ac8509db0a 100644 --- a/docs/conf/writing.md +++ b/docs/conf/writing.md @@ -177,3 +177,31 @@ configuration value will evaluate to `undefined`. The substitution syntax can be escaped using `$${...}`, which will be resolved as `${...}`. + +## Combining Includes and Environment Variable Substitution + +The Includes and Environment Variable Substitutions can be combined to do +something like read a secrets configuration for a specific environment. For +example: + +```yaml +integrations: + github: + - host: github.com + apps: + - $include: secrets.${BACKSTAGE_ENVIRONMENT}.yaml +``` + +Example `secrets.prod.yaml`: + +```yaml +appId: 1 +webhookUrl: https://smee.io/foo +clientId: someGithubAppClientId +clientSecret: someGithubAppClientSecret +webhookSecret: someWebhookSecret +privateKey: | + -----BEGIN RSA PRIVATE KEY----- + SomeRsaPrivateKey + -----END RSA PRIVATE KEY----- +``` From b593061da3446684cad0d11986b55f0747beff31 Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Wed, 31 Mar 2021 15:05:53 +0000 Subject: [PATCH 4/8] Fix bug in substitution transform Signed-off-by: JP Dhabolt john.p.dhabolt@gmail.com Signed-off-by: JP Dhabolt --- .changeset/fair-geese-yell.md | 15 +++---- .../src/lib/transform/substitution.test.ts | 5 ++- .../src/lib/transform/substitution.ts | 9 ++++- packages/config-loader/src/loader.test.ts | 40 +++++++++++++++++++ 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/.changeset/fair-geese-yell.md b/.changeset/fair-geese-yell.md index 5ca86b8913..00c5b16854 100644 --- a/.changeset/fair-geese-yell.md +++ b/.changeset/fair-geese-yell.md @@ -2,13 +2,14 @@ '@backstage/config-loader': minor --- +Fix bug where `$${...}` was not being escaped to `${...}` + Add support for environment variable substitution in `$include`, `$file` and `$env` transform values. -This change allows for including dynamic paths, such as environment specific -secrets by using the same environment variable substitution (`${..}`) already -supported outside of the various include transforms. - -If you are currently using the syntax `${...}` in your include transform values, -you will need to escape the substitution by using `$${...}` instead to maintain -the same behavior. +- This change allows for including dynamic paths, such as environment specific + secrets by using the same environment variable substitution (`${..}`) already + supported outside of the various include transforms. +- If you are currently using the syntax `${...}` in your include transform values, + you will need to escape the substitution by using `$${...}` instead to maintain + the same behavior. diff --git a/packages/config-loader/src/lib/transform/substitution.test.ts b/packages/config-loader/src/lib/transform/substitution.test.ts index e2c0200bfd..ef557431dd 100644 --- a/packages/config-loader/src/lib/transform/substitution.test.ts +++ b/packages/config-loader/src/lib/transform/substitution.test.ts @@ -51,7 +51,7 @@ describe('substituteTransform', () => { }); await expect( substituteTransform('${SECRET } $${} ${TOKEN }', '/'), - ).resolves.toEqual({ applied: true, value: 'my-secret $${} my-token' }); + ).resolves.toEqual({ applied: true, value: 'my-secret ${} my-token' }); await expect(substituteTransform('foo ${MISSING}', '/')).resolves.toEqual({ applied: true, value: undefined, @@ -62,5 +62,8 @@ describe('substituteTransform', () => { await expect( substituteTransform('foo ${SECRET} ${SECRET}', '/'), ).resolves.toEqual({ applied: true, value: 'foo my-secret my-secret' }); + await expect( + substituteTransform('foo ${SECRET} $$${ESCAPE_ME}', '/'), + ).resolves.toEqual({ applied: true, value: 'foo my-secret $${ESCAPE_ME}' }); }); }); diff --git a/packages/config-loader/src/lib/transform/substitution.ts b/packages/config-loader/src/lib/transform/substitution.ts index 821cc8bc67..482f1221a9 100644 --- a/packages/config-loader/src/lib/transform/substitution.ts +++ b/packages/config-loader/src/lib/transform/substitution.ts @@ -29,8 +29,13 @@ export function createSubstitutionTransform(env: EnvFunc): TransformFunc { } const parts: (string | undefined)[] = input.split(/(? part === undefined)) { return { applied: true, value: undefined }; diff --git a/packages/config-loader/src/loader.test.ts b/packages/config-loader/src/loader.test.ts index 8c7de557e3..27b1e20789 100644 --- a/packages/config-loader/src/loader.test.ts +++ b/packages/config-loader/src/loader.test.ts @@ -20,6 +20,7 @@ import mockFs from 'mock-fs'; describe('loadConfig', () => { beforeAll(() => { process.env.MY_SECRET = 'is-secret'; + process.env.SUBSTITUTE_ME = 'substituted'; mockFs({ '/root/app-config.yaml': ` @@ -27,6 +28,7 @@ describe('loadConfig', () => { title: Example App sessionKey: $file: secrets/session-key.txt + escaped: \$\${Escaped} `, '/root/app-config.development.yaml': ` app: @@ -45,6 +47,19 @@ describe('loadConfig', () => { foo: bar: token \${MY_SECRET} `, + '/root/app-config.substitute.yaml': ` + app: + someConfig: + $include: \${SUBSTITUTE_ME}.yaml + noSubstitute: + $file: \$\${ESCAPE_ME}.txt + `, + '/root/substituted.yaml': ` + secret: + $file: secrets/\${SUBSTITUTE_ME}.txt + `, + '/root/secrets/substituted.txt': '123abc', + '/root/${ESCAPE_ME}.txt': 'notSubstituted', }); }); @@ -66,6 +81,7 @@ describe('loadConfig', () => { app: { title: 'Example App', sessionKey: 'abc123', + escaped: '${Escaped}', }, }, }, @@ -86,6 +102,7 @@ describe('loadConfig', () => { app: { title: 'Example App', sessionKey: 'abc123', + escaped: '${Escaped}', }, }, }, @@ -109,6 +126,7 @@ describe('loadConfig', () => { app: { title: 'Example App', sessionKey: 'abc123', + escaped: '${Escaped}', }, }, }, @@ -130,4 +148,26 @@ describe('loadConfig', () => { }, ]); }); + + it('loads deep substituted config', async () => { + await expect( + loadConfig({ + configRoot: '/root', + configPaths: ['/root/app-config.substitute.yaml'], + env: 'development', + }), + ).resolves.toEqual([ + { + context: 'app-config.substitute.yaml', + data: { + app: { + someConfig: { + secret: '123abc', + }, + noSubstitute: 'notSubstituted', + }, + }, + }, + ]); + }); }); From 0efa7ae6a7d6335f2668c7ea3a6a2418f6bce672 Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Thu, 1 Apr 2021 12:00:17 +0000 Subject: [PATCH 5/8] Update per PR comments and add two test assertions Signed-off-by: JP Dhabolt --- .../src/lib/transform/include.test.ts | 30 +++++++++---------- .../src/lib/transform/include.ts | 12 ++++++-- .../src/lib/transform/substitution.test.ts | 12 ++++++++ .../src/lib/transform/substitution.ts | 15 +++++----- 4 files changed, 43 insertions(+), 26 deletions(-) diff --git a/packages/config-loader/src/lib/transform/include.test.ts b/packages/config-loader/src/lib/transform/include.test.ts index 81c6cfb63b..12f59feb84 100644 --- a/packages/config-loader/src/lib/transform/include.test.ts +++ b/packages/config-loader/src/lib/transform/include.test.ts @@ -29,23 +29,21 @@ const env = jest.fn(async (name: string) => { } as { [name: string]: string })[name]; }); -const substitute = jest.fn( - async ( - value: JsonValue, - ): Promise<{ applied: boolean; value?: JsonValue }> => { - if (typeof value !== 'string') { - return { applied: false }; - } - if (value.includes(substituteMe)) { - return { - applied: true, - value: value.replace(substituteMe, mySubstitution), - }; - } - +const substitute = async ( + value: JsonValue, +): Promise<{ applied: boolean; value?: JsonValue }> => { + if (typeof value !== 'string') { return { applied: false }; - }, -); + } + if (value.includes(substituteMe)) { + return { + applied: true, + value: value.replace(substituteMe, mySubstitution), + }; + } + + return { applied: false }; +}; const readFile = jest.fn(async (path: string) => { const content = ({ diff --git a/packages/config-loader/src/lib/transform/include.ts b/packages/config-loader/src/lib/transform/include.ts index bffc2e0184..2b5ccf23b6 100644 --- a/packages/config-loader/src/lib/transform/include.ts +++ b/packages/config-loader/src/lib/transform/include.ts @@ -55,12 +55,18 @@ export function createIncludeTransform( } const rawIncludedValue = input[includeKey]; - const substituteResults = await substitute(rawIncludedValue!, baseDir); + if (typeof rawIncludedValue !== 'string') { + throw new Error(`${includeKey} include value is not a string`); + } + + const substituteResults = await substitute(rawIncludedValue, baseDir); const includeValue = substituteResults.applied ? substituteResults.value : rawIncludedValue; - if (typeof includeValue !== 'string') { - throw new Error(`${includeKey} include value is not a string`); + + // The second string check is needed for Typescript to know this is a string. + if (includeValue === undefined || typeof includeValue !== 'string') { + throw new Error(`${includeKey} substitution value was undefined`); } switch (includeKey) { diff --git a/packages/config-loader/src/lib/transform/substitution.test.ts b/packages/config-loader/src/lib/transform/substitution.test.ts index ef557431dd..d07e88cfdd 100644 --- a/packages/config-loader/src/lib/transform/substitution.test.ts +++ b/packages/config-loader/src/lib/transform/substitution.test.ts @@ -56,6 +56,12 @@ describe('substituteTransform', () => { applied: true, value: undefined, }); + await expect( + substituteTransform('empty substitute ${}', '/'), + ).resolves.toEqual({ + applied: true, + value: undefined, + }); await expect( substituteTransform('foo ${MISSING} ${SECRET}', '/'), ).resolves.toEqual({ applied: true, value: undefined }); @@ -65,5 +71,11 @@ describe('substituteTransform', () => { await expect( substituteTransform('foo ${SECRET} $$${ESCAPE_ME}', '/'), ).resolves.toEqual({ applied: true, value: 'foo my-secret $${ESCAPE_ME}' }); + await expect( + substituteTransform('foo $${ESCAPE_ME} $$${ESCAPE_ME_TOO} $${}', '/'), + ).resolves.toEqual({ + applied: true, + value: 'foo ${ESCAPE_ME} $${ESCAPE_ME_TOO} ${}', + }); }); }); diff --git a/packages/config-loader/src/lib/transform/substitution.ts b/packages/config-loader/src/lib/transform/substitution.ts index 482f1221a9..56695fa431 100644 --- a/packages/config-loader/src/lib/transform/substitution.ts +++ b/packages/config-loader/src/lib/transform/substitution.ts @@ -28,15 +28,16 @@ export function createSubstitutionTransform(env: EnvFunc): TransformFunc { return { applied: false }; } - const parts: (string | undefined)[] = input.split(/(? part === undefined)) { return { applied: true, value: undefined }; } From dbbb8f7b37e496f2a00ad28526db3b9e27e1e240 Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Thu, 1 Apr 2021 15:19:08 +0000 Subject: [PATCH 6/8] Revert changes to types Signed-off-by: JP Dhabolt --- .../src/lib/transform/include.test.ts | 11 ++++++++++- packages/config-loader/src/lib/transform/types.ts | 15 ++++++++++----- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/config-loader/src/lib/transform/include.test.ts b/packages/config-loader/src/lib/transform/include.test.ts index 12f59feb84..750fa0c10b 100644 --- a/packages/config-loader/src/lib/transform/include.test.ts +++ b/packages/config-loader/src/lib/transform/include.test.ts @@ -31,7 +31,16 @@ const env = jest.fn(async (name: string) => { const substitute = async ( value: JsonValue, -): Promise<{ applied: boolean; value?: JsonValue }> => { +): Promise< + | { + applied: false; + } + | { + applied: true; + value: JsonValue | undefined; + newBaseDir?: string | undefined; + } +> => { if (typeof value !== 'string') { return { applied: false }; } diff --git a/packages/config-loader/src/lib/transform/types.ts b/packages/config-loader/src/lib/transform/types.ts index e7ebb0fddc..c13f01d016 100644 --- a/packages/config-loader/src/lib/transform/types.ts +++ b/packages/config-loader/src/lib/transform/types.ts @@ -23,8 +23,13 @@ export type ReadFileFunc = (path: string) => Promise; export type TransformFunc = ( value: JsonValue, baseDir: string, -) => Promise<{ - applied: boolean; - value?: JsonValue; - newBaseDir?: string; -}>; +) => Promise< + | { + applied: false; + } + | { + applied: true; + value: JsonValue | undefined; + newBaseDir?: string | undefined; + } +>; From c3b6fe4ffd42a4bf77f2c7589f11187c560b2606 Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Thu, 1 Apr 2021 15:38:10 +0000 Subject: [PATCH 7/8] Reduce type noise Signed-off-by: JP Dhabolt --- .../src/lib/transform/include.test.ts | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/packages/config-loader/src/lib/transform/include.test.ts b/packages/config-loader/src/lib/transform/include.test.ts index 750fa0c10b..1746bbcde9 100644 --- a/packages/config-loader/src/lib/transform/include.test.ts +++ b/packages/config-loader/src/lib/transform/include.test.ts @@ -18,6 +18,7 @@ import { JsonValue } from '@backstage/config'; import * as os from 'os'; import { resolve as resolvePath } from 'path'; import { createIncludeTransform } from './include'; +import { TransformFunc } from './types'; const root = os.platform() === 'win32' ? 'C:\\' : '/'; const substituteMe = '${MY_SUBSTITUTION}'; @@ -29,18 +30,7 @@ const env = jest.fn(async (name: string) => { } as { [name: string]: string })[name]; }); -const substitute = async ( - value: JsonValue, -): Promise< - | { - applied: false; - } - | { - applied: true; - value: JsonValue | undefined; - newBaseDir?: string | undefined; - } -> => { +const substitute: TransformFunc = async value => { if (typeof value !== 'string') { return { applied: false }; } From 3c9584801bf56c94805e5043dd732a2ea1b73432 Mon Sep 17 00:00:00 2001 From: JP Dhabolt Date: Thu, 1 Apr 2021 15:43:22 +0000 Subject: [PATCH 8/8] Remove unused JsonValue Signed-off-by: JP Dhabolt --- packages/config-loader/src/lib/transform/include.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/config-loader/src/lib/transform/include.test.ts b/packages/config-loader/src/lib/transform/include.test.ts index 1746bbcde9..200bbe3815 100644 --- a/packages/config-loader/src/lib/transform/include.test.ts +++ b/packages/config-loader/src/lib/transform/include.test.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import { JsonValue } from '@backstage/config'; import * as os from 'os'; import { resolve as resolvePath } from 'path'; import { createIncludeTransform } from './include';