From 032e4997878f26584e276b60506a9032cade55b1 Mon Sep 17 00:00:00 2001 From: Tyler Aldrich Date: Mon, 1 May 2023 14:55:00 -0400 Subject: [PATCH 1/2] Allow secrets to be used in build args while registering --- src/commands/deploy.ts | 19 ------------ src/commands/register.ts | 36 ++++++++++++++++++++++- src/common/utils/deploy.utils.ts | 4 +++ src/dependency-manager/graph/type.ts | 1 + src/dependency-manager/manager.ts | 9 +++++- src/dependency-manager/secrets/secrets.ts | 4 +++ src/dependency-manager/utils/rules.ts | 21 ++++++++----- 7 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/commands/deploy.ts b/src/commands/deploy.ts index 36900cb84..ab6de805a 100644 --- a/src/commands/deploy.ts +++ b/src/commands/deploy.ts @@ -141,25 +141,6 @@ export default class Deploy extends DeployCommand { deprecated: true, hidden: true, }), - 'secret-file': Flags.string({ - description: 'Path of secrets file', - multiple: true, - default: [], - }), - secrets: Flags.string({ - description: `Please use --secret-file.`, - multiple: true, - hidden: true, - deprecated: { - to: 'secret-file', - }, - }), - secret: Flags.string({ - char: 's', - description: 'An individual secret key and value in the form SECRET_KEY=SECRET_VALUE', - multiple: true, - default: [], - }), values: Flags.string({ char: 'v', hidden: true, diff --git a/src/commands/register.ts b/src/commands/register.ts index 73d6fb89d..66b18e26c 100644 --- a/src/commands/register.ts +++ b/src/commands/register.ts @@ -25,6 +25,7 @@ import PluginManager from '../common/plugins/plugin-manager'; import BuildPackUtils from '../common/utils/buildpack'; import { transformVolumeSpec } from '../dependency-manager/spec/transform/common-transform'; import { IF_EXPRESSION_REGEX } from '../dependency-manager/spec/utils/interpolation'; +import DeployUtils from '../common/utils/deploy.utils'; tmp.setGracefulCleanup(); @@ -46,6 +47,25 @@ export const SHARED_REGISTER_FLAGS = { description: 'Directory to write build cache to. Do not use in Github Actions: https://docs.architect.io/deployments/automated-previews/#caching-between-workflow-runs', sensitive: false, }), + 'secret-file': Flags.string({ + description: 'Path of secrets file', + multiple: true, + default: [], + }), + secrets: Flags.string({ + description: `Please use --secret-file.`, + multiple: true, + hidden: true, + deprecated: { + to: 'secret-file', + }, + }), + secret: Flags.string({ + char: 's', + description: 'An individual secret key and value in the form SECRET_KEY=SECRET_VALUE', + multiple: true, + default: [], + }), }; interface ImageRefOutput { @@ -197,7 +217,21 @@ export default class ComponentRegister extends BaseCommand { const dependency_manager = new LocalDependencyManager(this.app.api, selected_account.name); dependency_manager.environment = 'production'; - const graph = await dependency_manager.getGraph([instanceToInstance(component_spec)], undefined, { interpolate: false, validate: false }); + const all_secret_file_values = [...(flags['secret-file'] || []), ...(flags.secrets || [])]; + const component_secrets = DeployUtils.getComponentSecrets(flags.secret, all_secret_file_values); + + // Default graph options + const graph_options = { interpolate: false, validate: false, interpolate_build_args: false }; + // If the spec contains a service that has build args, set validate/interpolate_build_args + // so that secrets are validated and enforce any required secrets are provided via --secret-file. + for (const service of Object.values(component_spec.services || {})) { + if (service.build && service.build.args) { + graph_options.validate = true; + graph_options.interpolate_build_args = true; + } + } + + const graph = await dependency_manager.getGraph([instanceToInstance(component_spec)], component_secrets, graph_options); // Tmp fix to register host overrides for (const node of graph.nodes.filter(n => n instanceof ServiceNode) as ServiceNode[]) { for (const interface_config of Object.values(node.interfaces)) { diff --git a/src/common/utils/deploy.utils.ts b/src/common/utils/deploy.utils.ts index 1c283ff07..2c60fdf2b 100644 --- a/src/common/utils/deploy.utils.ts +++ b/src/common/utils/deploy.utils.ts @@ -64,6 +64,10 @@ export default class DeployUtils { return flags; } + /** + * Combine secrets from the secret flag, a secret file, and optionally an existing SecretsDict + * into a single SecretsDict. + */ static getComponentSecrets(individual_secrets: string[], secrets_file: string[], env_secrets?: SecretsDict): SecretsDict { let component_secrets: SecretsDict = env_secrets ? env_secrets : {}; diff --git a/src/dependency-manager/graph/type.ts b/src/dependency-manager/graph/type.ts index 930f5698b..d44152c0a 100644 --- a/src/dependency-manager/graph/type.ts +++ b/src/dependency-manager/graph/type.ts @@ -1,4 +1,5 @@ export interface GraphOptions { interpolate?: boolean; validate?: boolean; + interpolate_build_args?: boolean; } diff --git a/src/dependency-manager/manager.ts b/src/dependency-manager/manager.ts index e5e476dc5..c952fbe2a 100644 --- a/src/dependency-manager/manager.ts +++ b/src/dependency-manager/manager.ts @@ -289,7 +289,7 @@ export default abstract class DependencyManager { ...secrets_dict, }; - if (options.interpolate && options.validate) { + if ((options.interpolate || options.interpolate_build_args) && options.validate) { secrets.validateComponentSpec(component_spec); } @@ -299,6 +299,13 @@ export default abstract class DependencyManager { // Replace conditionals component_spec = interpolateObject(component_spec, context, { keys: true, values: false, file: component_spec.metadata.file }); + } else if (options.interpolate_build_args) { + // Only interpolate the build args block - used when registering a component to interpolate secret values. + for (const service of Object.values(component_spec.services || {})) { + if (service.build && service.build.args) { + service.build.args = interpolateObject(service.build.args, context, { keys: false, values: true, file: component_spec.metadata.file }); + } + } } const component_config = transformComponentSpec(component_spec); diff --git a/src/dependency-manager/secrets/secrets.ts b/src/dependency-manager/secrets/secrets.ts index fa89e1fd6..b0d1937ea 100644 --- a/src/dependency-manager/secrets/secrets.ts +++ b/src/dependency-manager/secrets/secrets.ts @@ -111,6 +111,10 @@ export class Secrets { } } + /** + * Validates that all required secrets are provided. If any required secrets are missing, + * raises a ValidationErrors error. + */ validateComponentSpec(component_spec: ComponentSpec): void { const secrets_dict = this.getSecretsForComponentSpec(component_spec, true); diff --git a/src/dependency-manager/utils/rules.ts b/src/dependency-manager/utils/rules.ts index 0d3d5b58e..a83b9c06a 100644 --- a/src/dependency-manager/utils/rules.ts +++ b/src/dependency-manager/utils/rules.ts @@ -30,9 +30,17 @@ abstract class InterpolationRule { } class BuildInterpolationRule extends InterpolationRule { - protected checkKey(key: string) { - const split = key.split('.').filter(key => !key.startsWith('${{')); - return (split[0] === 'services' || split[0] === 'tasks') && split[2] === 'build'; + /** + * Returns true if the interpolation is inside of a build block + * and the interpolation is not a secret value. + */ + protected checkPathAndKey(path: string, key: string) { + const split = path.split('.').filter(path => !path.startsWith('${{')); + const is_build_block = (split[0] === 'services' || split[0] === 'tasks') && split[2] === 'build'; + if (is_build_block) { + return !key.startsWith('secrets'); + } + return false; } check(context_map: ContextMap, context_key: string): string | undefined { @@ -42,15 +50,14 @@ class BuildInterpolationRule extends InterpolationRule { return; } - // Check if the interpolation is inside of a build block - if (this.checkKey(context_map._path)) { - return `Cannot use \${{ ${context_key} }} inside a build block. The build block is immutable. Use architect dev --arg KEY=VALUE for build args.`; + if (this.checkPathAndKey(context_map._path, context_key)) { + return `Cannot use \${{ ${context_key} }} inside a build block. The build block is immutable. Use architect dev --arg KEY=VALUE or use secrets for build args.`; } // Check if the interpolation is around a build block const maybe_child_key = Object.keys(context_map._obj_map).find(key => key.startsWith(`${context_map._path}.`) && this.checkKey(key)); if (maybe_child_key) { - return `Cannot use \${{ ${context_key} }} around a build block. The build block is immutable. Use architect dev --arg KEY=VALUE for build args.`; + return `Cannot use \${{ ${context_key} }} around a build block. The build block is immutable. Use architect dev --arg KEY=VALUE or use secrets for build args.`; } } } From 3cf6b48b6de9c71efacf594aaec6c35a1bcc397e Mon Sep 17 00:00:00 2001 From: Tyler Aldrich Date: Tue, 2 May 2023 14:01:23 -0400 Subject: [PATCH 2/2] Updated tests to fix the broken and add tests for secrets in build arg in the superset --- src/dependency-manager/utils/rules.ts | 8 +- test/commands/register.test.ts | 17 ++-- .../interpolation-validation.test.ts | 89 +++++++++++-------- test/mocks/superset/architect.yml | 3 + 4 files changed, 67 insertions(+), 50 deletions(-) diff --git a/src/dependency-manager/utils/rules.ts b/src/dependency-manager/utils/rules.ts index a83b9c06a..955d3b7a1 100644 --- a/src/dependency-manager/utils/rules.ts +++ b/src/dependency-manager/utils/rules.ts @@ -34,13 +34,13 @@ class BuildInterpolationRule extends InterpolationRule { * Returns true if the interpolation is inside of a build block * and the interpolation is not a secret value. */ - protected checkPathAndKey(path: string, key: string) { + protected checkPathAndKey(path: string, key?: string) { const split = path.split('.').filter(path => !path.startsWith('${{')); const is_build_block = (split[0] === 'services' || split[0] === 'tasks') && split[2] === 'build'; - if (is_build_block) { + if (is_build_block && key) { return !key.startsWith('secrets'); } - return false; + return is_build_block; } check(context_map: ContextMap, context_key: string): string | undefined { @@ -55,7 +55,7 @@ class BuildInterpolationRule extends InterpolationRule { } // Check if the interpolation is around a build block - const maybe_child_key = Object.keys(context_map._obj_map).find(key => key.startsWith(`${context_map._path}.`) && this.checkKey(key)); + const maybe_child_key = Object.keys(context_map._obj_map).find(key => key.startsWith(`${context_map._path}.`) && this.checkPathAndKey(key)); if (maybe_child_key) { return `Cannot use \${{ ${context_key} }} around a build block. The build block is immutable. Use architect dev --arg KEY=VALUE or use secrets for build args.`; } diff --git a/test/commands/register.test.ts b/test/commands/register.test.ts index 74f5f94c8..bda32edf3 100644 --- a/test/commands/register.test.ts +++ b/test/commands/register.test.ts @@ -15,7 +15,6 @@ import PluginManager from '../../src/common/plugins/plugin-manager'; import BuildPackUtils from '../../src/common/utils/buildpack'; import { IF_EXPRESSION_REGEX } from '../../src/dependency-manager/spec/utils/interpolation'; import { getMockComponentContextPath, getMockComponentFilePath, MockArchitectApi, ReplyCallback } from '../utils/mocks'; -import { Body, ReplyBody } from 'nock/types'; describe('register', function () { const mock_account_response = { @@ -60,11 +59,11 @@ describe('register', function () { const text_file = fs.readFileSync('test/mocks/superset/filedata.txt'); expect(body.config.services['stateful-api'].environment.FILE_DATA).to.eq(text_file.toString().trim()); return body; - } + }, }) .getTests() .stub(ComponentRegister.prototype, 'uploadVolume', sinon.stub().returns({})) - .command(['register', 'test/mocks/superset/architect.yml', '-a', 'examples']) + .command(['register', 'test/mocks/superset/architect.yml', '-a', 'examples', '-s', 'param_unset=foo']) .it('test file: replacement', ctx => { expect(ctx.stdout).to.contain('Successfully registered component'); }); @@ -120,7 +119,7 @@ describe('register', function () { .stub(DockerComposeUtils, 'writeCompose', sinon.stub()) .stub(fs, 'move', sinon.stub()) .stub(ComponentRegister.prototype, 'uploadVolume', sinon.stub().returns({})) - .command(['register', 'test/mocks/superset/architect.yml', '-t', '1.0.0', '-a', 'examples']) + .command(['register', 'test/mocks/superset/architect.yml', '-t', '1.0.0', '-a', 'examples', '-s', 'param_unset=foo']) .it('register superset', async ctx => { expect(ctx.stdout).to.contain('Successfully registered component'); /* @@ -160,7 +159,7 @@ describe('register', function () { .architectRegistryHeadRequest('/v2/examples/superset.services.stateful-frontend/manifests/1.0.0') .architectRegistryHeadRequest('/v2/examples/superset.tasks.curler-build/manifests/1.0.0') .getTests() - .command(['register', 'test/mocks/superset/architect.yml', '-t', '1.0.0', '-a', 'examples']) + .command(['register', 'test/mocks/superset/architect.yml', '-t', '1.0.0', '-a', 'examples', '-s', 'param_unset=foo']) .it('it reports to the user that the superset was registered successfully', ctx => { expect(ctx.stdout).to.contain('Successfully registered component'); }); @@ -168,7 +167,7 @@ describe('register', function () { new MockArchitectApi() .getAccount(mock_account_response) .architectRegistryHeadRequest() - .getEnvironment(mock_account_response, { name: 'test-env'}) + .getEnvironment(mock_account_response, { name: 'test-env' }) .registerComponentDigest(mock_account_response, { body: (body) => { expect(body.tag).to.eq('architect.environment.test-env'); @@ -447,7 +446,7 @@ describe('register', function () { build: () => { }, })) .stub(DockerBuildXUtils, 'dockerBuildX', sinon.stub()) - .command(['register', 'test/mocks/buildpack/buildpack-architect.yml', '-t', '1.0.0', '-a', 'examples']) + .command(['register', 'test/mocks/buildpack/buildpack-architect.yml', '-t', '1.0.0', '-a', 'examples', '-s', 'param_unset=foo']) .it('register with buildpack set to true override Dockerfile', ctx => { expect(ctx.stderr).to.contain('Registering component hello-world:1.0.0 with Architect Cloud...... done\n'); expect(ctx.stdout).to.contain('Successfully registered component'); @@ -470,7 +469,7 @@ describe('register', function () { })) .stub(DockerHelper, 'composeVersion', sinon.stub().returns(true)) .stub(DockerHelper, 'buildXVersion', sinon.stub().returns(true)) - .command(['register', 'test/mocks/buildpack/buildpack-dockerfile-architect.yml', '-t', '1.0.0', '-a', 'examples']) + .command(['register', 'test/mocks/buildpack/buildpack-dockerfile-architect.yml', '-t', '1.0.0', '-a', 'examples', '-s', 'param_unset=foo']) .it('register with buildpack and dockerfile services', ctx => { const buildpack = BuildPackUtils.build as sinon.SinonStub; expect(buildpack.args.toString()).to.equal(`${path.normalize('test/plugins')},hello-world--buildpack-api,,${path.join(path.resolve('test/integration'), './hello-world/')}`); @@ -487,7 +486,7 @@ describe('register', function () { .getTests() .stub(DockerBuildXUtils, 'dockerBuildX', sinon.stub()) .stub(DockerUtils, 'doesDockerfileExist', sinon.stub().callsFake(DockerUtils.doesDockerfileExist)) // override global stub - .command(['register', 'test/mocks/register/nonexistence-dockerfile-architect.yml', '-t', '1.0.0', '-a', 'examples']) + .command(['register', 'test/mocks/register/nonexistence-dockerfile-architect.yml', '-t', '1.0.0', '-a', 'examples', '-s', 'param_unset=foo']) .catch(e => { expect(e.message).contains(`${path.resolve('./test/integration/hello-world/nonexistent-dockerfile')} does not exist. Please verify the correct context and/or dockerfile were given.`); }) diff --git a/test/dependency-manager/interpolation-validation.test.ts b/test/dependency-manager/interpolation-validation.test.ts index f61a249da..79da1dc88 100644 --- a/test/dependency-manager/interpolation-validation.test.ts +++ b/test/dependency-manager/interpolation-validation.test.ts @@ -1,12 +1,11 @@ -import { expect } from 'chai'; +import { expect } from '@oclif/test'; import { buildSpecFromYml, validateInterpolation, ValidationErrors } from '../../src'; describe('interpolation-validation', () => { - - const context = {} + const context = {}; describe('validate build block', () => { - it('cannot use secret in build block', () => { + it('can use secret in build block', () => { const component_config = ` name: hello-world secrets: @@ -16,12 +15,28 @@ describe('interpolation-validation', () => { build: args: ENV: \${{ secrets.environment }} - ` + `; + + const component_spec = buildSpecFromYml(component_config); + validateInterpolation(component_spec); + }); + + it('cannot use other interpolation in build block', () => { + const component_config = ` + name: hello-world + services: + api: + build: + args: + PORT: \${{ services.api.interfaces.main.port }} + interfaces: + main: 3000 + `; - const component_spec = buildSpecFromYml(component_config) + const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -34,12 +49,12 @@ describe('interpolation-validation', () => { args: \${{ if architect.environment == 'prod' }}: ENV: prod - ` + `; - const component_spec = buildSpecFromYml(component_config) + const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -52,12 +67,12 @@ describe('interpolation-validation', () => { build: args: ENV: prod - ` + `; - const component_spec = buildSpecFromYml(component_config) + const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -70,12 +85,12 @@ describe('interpolation-validation', () => { build: args: ENV: prod - ` + `; - const component_spec = buildSpecFromYml(component_config) + const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -89,10 +104,10 @@ describe('interpolation-validation', () => { args: \${{ if architect.environment == 'local' }}: ENV: local - ` + `; - const component_spec = buildSpecFromYml(component_config) - validateInterpolation(component_spec) + const component_spec = buildSpecFromYml(component_config); + validateInterpolation(component_spec); }); it('can use conditional around build block if local', () => { @@ -104,10 +119,10 @@ describe('interpolation-validation', () => { build: args: ENV: local - ` + `; - const component_spec = buildSpecFromYml(component_config) - validateInterpolation(component_spec) + const component_spec = buildSpecFromYml(component_config); + validateInterpolation(component_spec); }); it('can use conditional around service block with build block if local', () => { @@ -119,10 +134,10 @@ describe('interpolation-validation', () => { build: args: ENV: local - ` + `; - const component_spec = buildSpecFromYml(component_config) - validateInterpolation(component_spec) + const component_spec = buildSpecFromYml(component_config); + validateInterpolation(component_spec); }); }); @@ -135,11 +150,11 @@ describe('interpolation-validation', () => { args: \${{ if architect.build.tag == 'latest' }}: ENV: prod - ` + `; - const component_spec = buildSpecFromYml(component_config) + const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -151,11 +166,11 @@ describe('interpolation-validation', () => { build: args: TAG: \${{ architect.build.tag }} - ` + `; - const component_spec = buildSpecFromYml(component_config) + const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -169,10 +184,10 @@ describe('interpolation-validation', () => { api: environment: TEST: \${{ secrets.test }} - ` + `; - const component_spec = buildSpecFromYml(component_config) - validateInterpolation(component_spec) + const component_spec = buildSpecFromYml(component_config); + validateInterpolation(component_spec); }); it('can still use conditional without build block', () => { @@ -183,10 +198,10 @@ describe('interpolation-validation', () => { \${{ if architect.environment == 'local' }}: environment: TEST: test - ` + `; - const component_spec = buildSpecFromYml(component_config) - validateInterpolation(component_spec) + const component_spec = buildSpecFromYml(component_config); + validateInterpolation(component_spec); }); }); }); diff --git a/test/mocks/superset/architect.yml b/test/mocks/superset/architect.yml index 4edc51b38..9b236932e 100644 --- a/test/mocks/superset/architect.yml +++ b/test/mocks/superset/architect.yml @@ -40,6 +40,8 @@ secrets: param_unset: frontend_private: default: false + build_arg_secret: + default: secret-build-arg dependencies: hello-world: latest @@ -153,6 +155,7 @@ services: args: build_arg_string: arg_value build_arg_unset: + build_arg_secret: ${{ secrets.build_arg_secret }} # target: production # TODO: re-enable when we move the example app to the test directory # ${{ if architect.environment == 'local' }}: # target: dev