From a79f5a8dea952ac12dd61488cab4ffd6364ed6e9 Mon Sep 17 00:00:00 2001 From: mtgoncalves1 Date: Wed, 3 May 2023 15:27:17 -0400 Subject: [PATCH 1/4] Validate interpolation in register --- .../spec/utils/spec-validator.ts | 51 ++++++++++++++++++- .../interpolation-validation.test.ts | 39 ++++++++++++++ test/mocks/superset/architect.yml | 2 +- 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/src/dependency-manager/spec/utils/spec-validator.ts b/src/dependency-manager/spec/utils/spec-validator.ts index 9f31e4c25..5d273f578 100644 --- a/src/dependency-manager/spec/utils/spec-validator.ts +++ b/src/dependency-manager/spec/utils/spec-validator.ts @@ -354,6 +354,50 @@ export const validateOrRejectSpec = (parsed_yml: ParsedYaml, metadata?: Componen return component_spec; }; +const getDependencyName = (interpolation: string): string => { + const regex = /dependencies\.([^.[\]]+)/; + const match = interpolation.match(regex); + return match ? match[1] : ''; +}; + +const findInterpolationErrors = (errors: ValidationError[], component_spec: ComponentSpec) => { + const context_map = buildContextMap(component_spec); + const service_interface_config = new Set(['host', 'port', 'protocol', 'username', 'password', 'url', 'path']); + + const interpolation_errors: ValidationError[] = []; + for (const error of errors) { + let interpolation_str = error.value; + const is_dependency_url = interpolation_str.startsWith('dependencies'); + if (is_dependency_url) { + const dependency_name = getDependencyName(interpolation_str); + if (!(dependency_name in context_map.dependencies)) { + interpolation_errors.push(error); + } + } else { + const parts = interpolation_str.split('.'); + const last_element = parts[parts.length - 1]; + const is_service_url = interpolation_str.startsWith('services') && service_interface_config.has(last_element); + const is_database_url = interpolation_str.startsWith('databases') && interpolation_str.endsWith('url'); + if (is_service_url || is_database_url) { + interpolation_str = interpolation_str.substring(0, interpolation_str.lastIndexOf('.')); + } + if (!context_map[interpolation_str]) { + interpolation_errors.push(error); + } + } + } + return interpolation_errors; +}; + +const extractInterpolationErrors = (errors: ValidationError[]) => { + // Regular expression for architect variables such as architect.environment and architect.build.tag + const architect_regex = /architect\./; + + // Regular expression for environment variables such as environment.ingresses.hello-world.hello.url + const architect_environment_regex = /environment\./; + return errors.filter(error => error.message.startsWith(RequiredInterpolationRule.PREFIX) && !architect_regex.test(error.value) && !architect_environment_regex.test(error.value)); +}; + export const validateInterpolation = (component_spec: ComponentSpec): void => { const { errors } = interpolateObject(component_spec, {}, { keys: true, @@ -362,8 +406,11 @@ export const validateInterpolation = (component_spec: ComponentSpec): void => { }); const filtered_errors = errors.filter(error => !error.message.startsWith(RequiredInterpolationRule.PREFIX)); + let interpolation_errors = extractInterpolationErrors(errors); + interpolation_errors = findInterpolationErrors(interpolation_errors, component_spec); + const all_errors = [...filtered_errors, ...interpolation_errors]; - if (filtered_errors.length > 0) { - throw new ValidationErrors(filtered_errors, component_spec.metadata.file); + if (all_errors.length > 0) { + throw new ValidationErrors(all_errors, component_spec.metadata.file); } }; diff --git a/test/dependency-manager/interpolation-validation.test.ts b/test/dependency-manager/interpolation-validation.test.ts index f61a249da..3f43fc6b8 100644 --- a/test/dependency-manager/interpolation-validation.test.ts +++ b/test/dependency-manager/interpolation-validation.test.ts @@ -188,5 +188,44 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) validateInterpolation(component_spec) }); + + it('fail when using interpolation where path does not exist', () => { + const component_config = ` + name: hello-world + services: + api: + build: + context: . + environment: + DB_ADDR: \${{ services.database.interfaces.main.url }} + `; + + const component_spec = buildSpecFromYml(component_config); + + expect(() => { + validateInterpolation(component_spec); + }).to.be.throws(ValidationErrors); + }); + + it('fail when secret does not exist', () => { + const component_config = ` + name: hello-world + secrets: + world_text: + default: World + services: + api: + build: + context: . + environment: + WORLD_TEXT: \${{ secrets.notfound }} + `; + + const component_spec = buildSpecFromYml(component_config); + + expect(() => { + validateInterpolation(component_spec); + }).to.be.throws(ValidationErrors); + }); }); }); diff --git a/test/mocks/superset/architect.yml b/test/mocks/superset/architect.yml index 4edc51b38..7ec9bbf7c 100644 --- a/test/mocks/superset/architect.yml +++ b/test/mocks/superset/architect.yml @@ -161,7 +161,7 @@ services: interfaces: http: 8080 environment: - BACKUP_DB_ADDR: ${{ databases.api-db.connection_string }} + BACKUP_DB_ADDR: ${{ databases.api-db2.connection_string }} DB_ADDR: ${{ services.api-db.interfaces.postgres.url }}/${{ secrets.db_name }} DB_USER: ${{ secrets.db_user }} DB_PASS: ${{ secrets.db_pass }} From a69a0557fdf6ef16ea2aa9ceca695fdc13177f86 Mon Sep 17 00:00:00 2001 From: mtgoncalves1 Date: Thu, 4 May 2023 14:03:01 -0400 Subject: [PATCH 2/4] Handle the case where there are no dependencies block --- src/dependency-manager/spec/utils/spec-validator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dependency-manager/spec/utils/spec-validator.ts b/src/dependency-manager/spec/utils/spec-validator.ts index 5d273f578..5e3a7df62 100644 --- a/src/dependency-manager/spec/utils/spec-validator.ts +++ b/src/dependency-manager/spec/utils/spec-validator.ts @@ -370,7 +370,7 @@ const findInterpolationErrors = (errors: ValidationError[], component_spec: Comp const is_dependency_url = interpolation_str.startsWith('dependencies'); if (is_dependency_url) { const dependency_name = getDependencyName(interpolation_str); - if (!(dependency_name in context_map.dependencies)) { + if (!context_map.dependencies || !(dependency_name in context_map.dependencies)) { interpolation_errors.push(error); } } else { From 9db42dee07f7d8d7088b06cac34e1acde541134a Mon Sep 17 00:00:00 2001 From: mtgoncalves1 Date: Wed, 10 May 2023 13:33:48 -0400 Subject: [PATCH 3/4] Use graph to validate interpolation --- src/commands/register.ts | 6 +- src/dependency-manager/graph/type.ts | 1 + src/dependency-manager/manager.ts | 49 ++++++++ .../spec/utils/spec-validator.ts | 107 +++++++++++++++--- src/dependency-manager/utils/interpolation.ts | 49 +++++++- .../interpolation-validation.test.ts | 101 ++++++++++++----- 6 files changed, 261 insertions(+), 52 deletions(-) diff --git a/src/commands/register.ts b/src/commands/register.ts index 17cfc9c44..daadc868e 100644 --- a/src/commands/register.ts +++ b/src/commands/register.ts @@ -174,8 +174,6 @@ export default class ComponentRegister extends BaseCommand { throw new Error('Component Config must have a name'); } - validateInterpolation(component_spec); - const { component_name } = ComponentSlugUtils.parse(component_spec.name); let account_name; @@ -197,7 +195,9 @@ 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 graph = await dependency_manager.getGraph([instanceToInstance(component_spec)], undefined, { interpolate: true, validate: false, build_dependency_nodes: true }); + validateInterpolation(component_spec, graph); + // 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/dependency-manager/graph/type.ts b/src/dependency-manager/graph/type.ts index 930f5698b..99082dbfa 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; + build_dependency_nodes?: boolean, } diff --git a/src/dependency-manager/manager.ts b/src/dependency-manager/manager.ts index e5e476dc5..ad0cd7cc8 100644 --- a/src/dependency-manager/manager.ts +++ b/src/dependency-manager/manager.ts @@ -21,6 +21,7 @@ import { validateOrRejectSpec } from './spec/utils/spec-validator'; import { Dictionary, transformDictionary } from './utils/dictionary'; import { ArchitectError } from './utils/errors'; import { interpolateObjectLoose, interpolateObjectOrReject, replaceInterpolationBrackets } from './utils/interpolation'; +import { IngressConfig, ServiceConfig, ServiceInterfaceConfig } from './config/service-config'; export default abstract class DependencyManager { account?: string; @@ -395,6 +396,7 @@ export default abstract class DependencyManager { options = { interpolate: true, validate: true, + build_dependency_nodes: false, ...options, }; @@ -530,6 +532,53 @@ export default abstract class DependencyManager { graph.validated = true; } + if (options.build_dependency_nodes) { + for (const component_spec of evaluated_component_specs) { + for (const dependency_name of Object.keys(component_spec.dependencies || {})) { + // create mock dependency node for dependencies validation + const mock_service_ingress_config: IngressConfig = { + enabled: false, + subdomain: '', + path: '', + ip_whitelist: [], + sticky: '', + private: false, + consumers: [], + dns_zone: '', + host: '', + port: '', + protocol: '', + username: '', + password: '', + url: '', + }; + const mock_service_interface_config: ServiceInterfaceConfig = { + host: '', + port: '', + protocol: '', + username: '', + password: '', + url: '', + sticky: '', + path: '', + ingress: mock_service_ingress_config, + }; + const mock_dependency_node = { + __type: '', + config: {} as ServiceConfig, + ref: `${dependency_name}--*`, + component_ref: dependency_name, + service_name: '*', + interfaces: { '*': mock_service_interface_config }, + ingresses: { '*': mock_service_ingress_config }, + ports: [], + is_external: false, + instance_id: '', + }; + graph.addNode(mock_dependency_node); + } + } + } return Object.freeze(graph); } } diff --git a/src/dependency-manager/spec/utils/spec-validator.ts b/src/dependency-manager/spec/utils/spec-validator.ts index 5e3a7df62..5ff305c3e 100644 --- a/src/dependency-manager/spec/utils/spec-validator.ts +++ b/src/dependency-manager/spec/utils/spec-validator.ts @@ -16,6 +16,8 @@ import { ComponentInstanceMetadata, ComponentSpec } from '../component-spec'; import { ServiceInterfaceSpec } from '../service-spec'; import { findDefinition, getArchitectJSONSchema } from './json-schema'; import { Slugs } from './slugs'; +import { DependencyGraphMutable } from '../../graph'; +import { ServiceNode } from '../../graph/node/service'; export type AjvError = ErrorObject[] | null | undefined; @@ -354,34 +356,103 @@ export const validateOrRejectSpec = (parsed_yml: ParsedYaml, metadata?: Componen return component_spec; }; -const getDependencyName = (interpolation: string): string => { - const regex = /dependencies\.([^.[\]]+)/; +const extractServiceName = (interpolation: string, type: string): string => { + const regex = new RegExp(`${type}\\.([^.[\\]]+)`); const match = interpolation.match(regex); return match ? match[1] : ''; }; -const findInterpolationErrors = (errors: ValidationError[], component_spec: ComponentSpec) => { - const context_map = buildContextMap(component_spec); +const isDependencyInterpolation = (interpolation_str: string): boolean => { + const regex = new RegExp(`^dependencies.[^.]+.(?:services.[^.]+.){0,1}(?:interfaces|ingresses).[^.]+.(?:host|port|protocol|username|password|url|sticky|path|ingress.(?:private|url))$`); + return regex.test(interpolation_str); +}; + +const isDatabasesInterpolation = (interpolation_str: string): boolean => { + const regex = new RegExp(`^databases.[^.]+.(?:connection_string|url)$`); + return regex.test(interpolation_str); +}; + +const checkInterpolationPath = (queue: any, child: any): boolean => { + let key = queue.shift(); + const keys = Object.keys(child); + const star = keys.includes('*'); + if (star || child[key]) { + key = star ? '*' : key; + if (queue.length > 0 && typeof child[key] === 'object') { + return checkInterpolationPath(queue, child[key]); + } else { + return true; + } + } else { + return keys.includes(key); + } +}; + +const findInterpolationErrors = (errors: ValidationError[], component_spec: ComponentSpec, graph: Readonly) => { const service_interface_config = new Set(['host', 'port', 'protocol', 'username', 'password', 'url', 'path']); + const context_map = buildContextMap(component_spec); const interpolation_errors: ValidationError[] = []; for (const error of errors) { - let interpolation_str = error.value; - const is_dependency_url = interpolation_str.startsWith('dependencies'); - if (is_dependency_url) { - const dependency_name = getDependencyName(interpolation_str); - if (!context_map.dependencies || !(dependency_name in context_map.dependencies)) { + const interpolation_str = error.value; + const is_secret_interpolation = interpolation_str.startsWith('secrets'); + + if (is_secret_interpolation) { + if (!context_map[interpolation_str]) { + interpolation_errors.push(error); + } + } else if (isDependencyInterpolation(interpolation_str)) { + const dependency_name = extractServiceName(interpolation_str, 'dependencies'); + const node = graph.nodes_map.get(`${dependency_name}--*`); + + let path_exist; + if (node) { + const splitted_interpolation_path = interpolation_str.split('.'); + let splitted_path; + const type = splitted_interpolation_path[2]; + if (type === 'services') { + splitted_path = splitted_interpolation_path.slice(4); + } else if (type === 'ingresses' || type === 'interfaces') { + splitted_path = splitted_interpolation_path.slice(2); + } else { + splitted_path = splitted_interpolation_path; + } + + path_exist = checkInterpolationPath(splitted_path, node); + } + if (!node || !path_exist) { interpolation_errors.push(error); } } else { - const parts = interpolation_str.split('.'); - const last_element = parts[parts.length - 1]; + const splitted_interpolation_path = interpolation_str.split('.'); + const last_element = splitted_interpolation_path[splitted_interpolation_path.length - 1]; const is_service_url = interpolation_str.startsWith('services') && service_interface_config.has(last_element); - const is_database_url = interpolation_str.startsWith('databases') && interpolation_str.endsWith('url'); - if (is_service_url || is_database_url) { - interpolation_str = interpolation_str.substring(0, interpolation_str.lastIndexOf('.')); - } - if (!context_map[interpolation_str]) { + const is_database_url = isDatabasesInterpolation(interpolation_str); + + let service_name = ''; + if (is_database_url) { + service_name = extractServiceName(interpolation_str, 'databases'); + const node = graph.nodes.find(node => node instanceof ServiceNode && node.service_name === `${service_name}-db`); + if (!node) { + interpolation_errors.push(error); + } + continue; + } else if (is_service_url) { + service_name = extractServiceName(interpolation_str, 'services'); + if (!service_name) { + interpolation_errors.push(error); + continue; + } + const node = graph.nodes.find(node => node instanceof ServiceNode && node.service_name === service_name); + let path_exist; + if (node) { + splitted_interpolation_path.pop(); + path_exist = checkInterpolationPath(splitted_interpolation_path.slice(2), node); + } + if (!node || !path_exist) { + interpolation_errors.push(error); + } + } else { interpolation_errors.push(error); } } @@ -398,7 +469,7 @@ const extractInterpolationErrors = (errors: ValidationError[]) => { return errors.filter(error => error.message.startsWith(RequiredInterpolationRule.PREFIX) && !architect_regex.test(error.value) && !architect_environment_regex.test(error.value)); }; -export const validateInterpolation = (component_spec: ComponentSpec): void => { +export const validateInterpolation = (component_spec: ComponentSpec, graph: Readonly): void => { const { errors } = interpolateObject(component_spec, {}, { keys: true, values: true, @@ -407,7 +478,7 @@ export const validateInterpolation = (component_spec: ComponentSpec): void => { const filtered_errors = errors.filter(error => !error.message.startsWith(RequiredInterpolationRule.PREFIX)); let interpolation_errors = extractInterpolationErrors(errors); - interpolation_errors = findInterpolationErrors(interpolation_errors, component_spec); + interpolation_errors = findInterpolationErrors(interpolation_errors, component_spec, graph); const all_errors = [...filtered_errors, ...interpolation_errors]; if (all_errors.length > 0) { diff --git a/src/dependency-manager/utils/interpolation.ts b/src/dependency-manager/utils/interpolation.ts index 3dea52a7d..31752546b 100644 --- a/src/dependency-manager/utils/interpolation.ts +++ b/src/dependency-manager/utils/interpolation.ts @@ -6,6 +6,9 @@ import { ValidationError, ValidationErrors } from './errors'; import { ArchitectParser } from './parser'; import { matches } from './regex'; import { CONTEXT_KEY_DELIMITER } from './rules'; +import { ComponentSpec } from '../spec/component-spec'; +import { ArchitectContext, DependencyContext, ServiceContext } from '../config/component-context'; +import { IngressConfig, ServiceInterfaceConfig } from '../config/service-config'; export const replaceBrackets = (value: string): string => { return value.replace(/\[/g, '.').replace(/["'\\\]|]/g, ''); @@ -23,8 +26,52 @@ export const replaceInterpolationBrackets = (value: string): string => { return res; }; -export const buildContextMap = (context: any): any => { +/* + Create mock dependencies for dependencies..services.*.interfaces.*. +*/ +const createMockDependencies = (component_spec: ComponentSpec) => { + const dependencies: Dictionary = {}; + for (const dep_name of Object.keys(component_spec.dependencies || {})) { + const mock_service_interface_config: ServiceInterfaceConfig = { + host: '', + port: '', + protocol: '', + username: '', + password: '', + url: '', + sticky: '', + path: '', + ingress: { private: false } as IngressConfig, + }; + const mock_service_context: ServiceContext = { + interfaces: { '*': mock_service_interface_config }, + environment: {}, + }; + + const dependency_context = { + name: dep_name, + dependencies: {}, + secrets: {}, + outputs: {}, + databases: {}, + services: { '*': mock_service_context }, + tasks: {}, + architect: {} as ArchitectContext, + }; + dependencies[dep_name] = { + services: dependency_context.services || {}, + outputs: dependency_context.outputs || {}, + }; + } + return dependencies; +}; + +export const buildContextMap = (context: any, use_mock_dependencies?: boolean): any => { const context_map: Dictionary = {}; + if (use_mock_dependencies) { + context.dependencies = createMockDependencies(context); + } + const queue = [['', context]]; while (queue.length > 0) { const [prefix, c] = queue.shift()!; diff --git a/test/dependency-manager/interpolation-validation.test.ts b/test/dependency-manager/interpolation-validation.test.ts index 3f43fc6b8..9f1f1d18a 100644 --- a/test/dependency-manager/interpolation-validation.test.ts +++ b/test/dependency-manager/interpolation-validation.test.ts @@ -1,9 +1,50 @@ import { expect } from 'chai'; -import { buildSpecFromYml, validateInterpolation, ValidationErrors } from '../../src'; +import { buildSpecFromYml, DependencyEdge, DependencyGraphMutable, DependencyNode, ServiceNode, TaskNode, validateInterpolation, ValidationErrors } from '../../src'; describe('interpolation-validation', () => { - - const context = {} + const mock_graph: Readonly = { + nodes: [ + { + instance_id: '', + __type: 'service', + ref: 'hello-world--api', + component_ref: 'hello-world', + service_name: 'api', + artifact_image: undefined, + } as ServiceNode, + ], + edges: [], + validated: false, + nodes_map: {} as Map, + edges_map: {} as Map, + addNode: function (node: DependencyNode): DependencyNode { + throw new Error('Function not implemented.'); + }, + removeNodeByRef: function (ref: string): void { + throw new Error('Function not implemented.'); + }, + removeEdgeByRef: function (edge_ref: string): void { + throw new Error('Function not implemented.'); + }, + addEdge: function (edge: DependencyEdge): DependencyEdge { + throw new Error('Function not implemented.'); + }, + getNodeByRef: function (ref: string): DependencyNode { + throw new Error('Function not implemented.'); + }, + getDownstreamNodes: function (node: DependencyNode): DependencyNode[] { + throw new Error('Function not implemented.'); + }, + removeNode: function (node_ref: string, cleanup_dangling: boolean): void { + throw new Error('Function not implemented.'); + }, + getUpstreamNodes: function (node: DependencyNode): DependencyNode[] { + throw new Error('Function not implemented.'); + }, + getDependsOn: function (current_node: ServiceNode | TaskNode): (ServiceNode | TaskNode)[] { + throw new Error('Function not implemented.'); + }, + }; describe('validate build block', () => { it('cannot use secret in build block', () => { @@ -21,7 +62,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec, {} as Readonly); }).to.be.throws(ValidationErrors); }); @@ -39,7 +80,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec, {} as Readonly); }).to.be.throws(ValidationErrors); }); @@ -57,7 +98,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec, {} as Readonly); }).to.be.throws(ValidationErrors); }); @@ -75,7 +116,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec) + validateInterpolation(component_spec, {} as Readonly); }).to.be.throws(ValidationErrors); }); @@ -89,10 +130,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, mock_graph); }); it('can use conditional around build block if local', () => { @@ -104,10 +145,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, mock_graph); }); it('can use conditional around service block with build block if local', () => { @@ -119,10 +160,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, mock_graph); }); }); @@ -135,11 +176,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, {} as Readonly); }).to.be.throws(ValidationErrors); }); @@ -151,11 +192,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, {} as Readonly); }).to.be.throws(ValidationErrors); }); @@ -169,10 +210,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, {} as Readonly); }); it('can still use conditional without build block', () => { @@ -183,10 +224,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, {} as Readonly); }); it('fail when using interpolation where path does not exist', () => { @@ -203,7 +244,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec); + validateInterpolation(component_spec, mock_graph); }).to.be.throws(ValidationErrors); }); @@ -224,7 +265,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec); + validateInterpolation(component_spec, {} as Readonly); }).to.be.throws(ValidationErrors); }); }); From 87513ce92ad1ea7382977e798566f13f9f737426 Mon Sep 17 00:00:00 2001 From: mtgoncalves1 Date: Mon, 15 May 2023 12:46:33 -0400 Subject: [PATCH 4/4] Use current interpolation validation --- src/commands/register.ts | 5 +- src/dependency-manager/graph/type.ts | 2 +- src/dependency-manager/manager.ts | 128 ++++++++++-------- .../spec/utils/interpolation.ts | 4 + .../spec/utils/spec-validator.ts | 108 +-------------- src/dependency-manager/utils/interpolation.ts | 18 ++- .../interpolation-validation.test.ts | 107 ++------------- 7 files changed, 111 insertions(+), 261 deletions(-) diff --git a/src/commands/register.ts b/src/commands/register.ts index 8a8a36a7b..3bc488fef 100644 --- a/src/commands/register.ts +++ b/src/commands/register.ts @@ -174,6 +174,8 @@ export default class ComponentRegister extends BaseCommand { throw new Error('Component Config must have a name'); } + validateInterpolation(component_spec, true); + const { component_name } = ComponentSlugUtils.parse(component_spec.name); let account_name; @@ -195,8 +197,7 @@ 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: true, validate: false, build_dependency_nodes: true }); - validateInterpolation(component_spec, graph); + const graph = await dependency_manager.getGraph([instanceToInstance(component_spec)], undefined, { interpolate: true, validate: true, relax_validation: true }); // Tmp fix to register host overrides for (const node of graph.nodes.filter(n => n instanceof ServiceNode) as ServiceNode[]) { diff --git a/src/dependency-manager/graph/type.ts b/src/dependency-manager/graph/type.ts index 99082dbfa..72009088d 100644 --- a/src/dependency-manager/graph/type.ts +++ b/src/dependency-manager/graph/type.ts @@ -1,5 +1,5 @@ export interface GraphOptions { interpolate?: boolean; validate?: boolean; - build_dependency_nodes?: boolean, + relax_validation?: boolean, } diff --git a/src/dependency-manager/manager.ts b/src/dependency-manager/manager.ts index ad0cd7cc8..0f5c0f4a8 100644 --- a/src/dependency-manager/manager.ts +++ b/src/dependency-manager/manager.ts @@ -1,6 +1,6 @@ import { instanceToPlain, plainToInstance, serialize } from 'class-transformer'; import { buildNodeRef, ComponentConfig } from './config/component-config'; -import { ArchitectContext, ComponentContext, DatabaseContext } from './config/component-context'; +import { ArchitectContext, ComponentContext, DatabaseContext, ServiceContext } from './config/component-context'; import { DeprecatedInterfacesSpec } from './deprecated-spec/interfaces'; import { DependencyGraph, DependencyGraphMutable } from './graph'; import { IngressEdge } from './graph/edge/ingress'; @@ -22,6 +22,7 @@ import { Dictionary, transformDictionary } from './utils/dictionary'; import { ArchitectError } from './utils/errors'; import { interpolateObjectLoose, interpolateObjectOrReject, replaceInterpolationBrackets } from './utils/interpolation'; import { IngressConfig, ServiceConfig, ServiceInterfaceConfig } from './config/service-config'; +import { SecretDefinitionSpec, SecretSpecValue } from './spec/secret-spec'; export default abstract class DependencyManager { account?: string; @@ -396,7 +397,7 @@ export default abstract class DependencyManager { options = { interpolate: true, validate: true, - build_dependency_nodes: false, + relax_validation: false, ...options, }; @@ -414,6 +415,14 @@ export default abstract class DependencyManager { const evaluated_component_specs: ComponentSpec[] = []; for (const raw_component_spec of component_specs) { + if (options.relax_validation && raw_component_spec.secrets) { + // Assign dummy value to unset secrets + for (const [key, secret] of Object.entries(raw_component_spec.secrets as Dictionary)) { + if (!secret) { + raw_component_spec.secrets[key] = { default: '-999' }; + } + } + } const { component_spec, context } = await this.getComponentSpecContext(graph, raw_component_spec, secrets, options); if (options.interpolate) { @@ -488,12 +497,67 @@ export default abstract class DependencyManager { context.dependencies = {}; for (const dep_name of Object.keys(component_spec.dependencies || {})) { const dependency_context = context_map[dep_name]; - if (!dependency_context) continue; + if (!dependency_context && !options.relax_validation) { + continue; + } - context.dependencies[dep_name] = { - services: dependency_context.services || {}, - outputs: dependency_context.outputs || {}, - }; + if (options.relax_validation) { + // Mock dependency node for validation + const mock_service_ingress_config: IngressConfig = { + enabled: false, + subdomain: '', + path: '', + ip_whitelist: [], + sticky: '', + private: false, + consumers: [], + dns_zone: '', + host: '', + port: '', + protocol: '', + username: '', + password: '', + url: '', + }; + const mock_service_interface_config: ServiceInterfaceConfig = { + host: '', + port: '', + protocol: '', + username: '', + password: '', + url: '', + sticky: '', + path: '', + ingress: mock_service_ingress_config, + }; + const mock_dependency_node = { + __type: '', + config: {} as ServiceConfig, + ref: `${dep_name}--ø`, + component_ref: dep_name, + service_name: 'ø', + interfaces: { 'ø': mock_service_interface_config }, + ingresses: { 'ø': mock_service_ingress_config }, + ports: [], + is_external: false, + instance_id: '', + }; + graph.addNode(mock_dependency_node); + + const service_context: ServiceContext = { + environment: {}, + interfaces: { 'ø': mock_service_interface_config }, + }; + context.dependencies[dep_name] = { + services: { 'ø': service_context }, + outputs: {}, + }; + } else { + context.dependencies[dep_name] = { + services: dependency_context.services || {}, + outputs: dependency_context.outputs || {}, + }; + } } } @@ -508,7 +572,7 @@ export default abstract class DependencyManager { const context = context_map[component_spec.metadata.ref]; if (options.interpolate) { - component_spec = interpolateObject(component_spec, context, { keys: false, values: true, file: component_spec.metadata.file }); + component_spec = interpolateObject(component_spec, context, { keys: false, values: true, file: component_spec.metadata.file, relax_validation: options.relax_validation }); component_spec.metadata.interpolated = true; } @@ -531,54 +595,6 @@ export default abstract class DependencyManager { this.validateGraph(graph); graph.validated = true; } - - if (options.build_dependency_nodes) { - for (const component_spec of evaluated_component_specs) { - for (const dependency_name of Object.keys(component_spec.dependencies || {})) { - // create mock dependency node for dependencies validation - const mock_service_ingress_config: IngressConfig = { - enabled: false, - subdomain: '', - path: '', - ip_whitelist: [], - sticky: '', - private: false, - consumers: [], - dns_zone: '', - host: '', - port: '', - protocol: '', - username: '', - password: '', - url: '', - }; - const mock_service_interface_config: ServiceInterfaceConfig = { - host: '', - port: '', - protocol: '', - username: '', - password: '', - url: '', - sticky: '', - path: '', - ingress: mock_service_ingress_config, - }; - const mock_dependency_node = { - __type: '', - config: {} as ServiceConfig, - ref: `${dependency_name}--*`, - component_ref: dependency_name, - service_name: '*', - interfaces: { '*': mock_service_interface_config }, - ingresses: { '*': mock_service_ingress_config }, - ports: [], - is_external: false, - instance_id: '', - }; - graph.addNode(mock_dependency_node); - } - } - } return Object.freeze(graph); } } diff --git a/src/dependency-manager/spec/utils/interpolation.ts b/src/dependency-manager/spec/utils/interpolation.ts index cf4e0bfdb..255225829 100644 --- a/src/dependency-manager/spec/utils/interpolation.ts +++ b/src/dependency-manager/spec/utils/interpolation.ts @@ -1,2 +1,6 @@ export const EXPRESSION_REGEX = new RegExp(`\\\${{\\s*(.*?)\\s*}}`, 'g'); export const IF_EXPRESSION_REGEX = new RegExp(`^\\\${{\\s*if(.*?)\\s*}}$`); +export const DEPENDENCY_EXPRESSION_REGEX = new RegExp('^\\${{\\s*dependencies\\.[^}]*}}$'); +export const DEPRECATED_DEPENDENCY_EXPRESSION_REGEX = new RegExp('\\$\\{\\{ dependencies\\.[^.]*\\.(ingresses|interfaces)\\.[^.]*\\.url }}'); +export const ARCHITECT_EXPRESSION_REGEX = new RegExp('^\\${{\\s*architect\\.[^}]*}}'); +export const ENVIRONMENT_EXPRESSION_REGEX = new RegExp('^\\${{\\s*environment\\.[^}]*}}'); diff --git a/src/dependency-manager/spec/utils/spec-validator.ts b/src/dependency-manager/spec/utils/spec-validator.ts index 5ff305c3e..dd95bdae7 100644 --- a/src/dependency-manager/spec/utils/spec-validator.ts +++ b/src/dependency-manager/spec/utils/spec-validator.ts @@ -356,22 +356,6 @@ export const validateOrRejectSpec = (parsed_yml: ParsedYaml, metadata?: Componen return component_spec; }; -const extractServiceName = (interpolation: string, type: string): string => { - const regex = new RegExp(`${type}\\.([^.[\\]]+)`); - const match = interpolation.match(regex); - return match ? match[1] : ''; -}; - -const isDependencyInterpolation = (interpolation_str: string): boolean => { - const regex = new RegExp(`^dependencies.[^.]+.(?:services.[^.]+.){0,1}(?:interfaces|ingresses).[^.]+.(?:host|port|protocol|username|password|url|sticky|path|ingress.(?:private|url))$`); - return regex.test(interpolation_str); -}; - -const isDatabasesInterpolation = (interpolation_str: string): boolean => { - const regex = new RegExp(`^databases.[^.]+.(?:connection_string|url)$`); - return regex.test(interpolation_str); -}; - const checkInterpolationPath = (queue: any, child: any): boolean => { let key = queue.shift(); const keys = Object.keys(child); @@ -388,100 +372,16 @@ const checkInterpolationPath = (queue: any, child: any): boolean => { } }; -const findInterpolationErrors = (errors: ValidationError[], component_spec: ComponentSpec, graph: Readonly) => { - const service_interface_config = new Set(['host', 'port', 'protocol', 'username', 'password', 'url', 'path']); - const context_map = buildContextMap(component_spec); - - const interpolation_errors: ValidationError[] = []; - for (const error of errors) { - const interpolation_str = error.value; - const is_secret_interpolation = interpolation_str.startsWith('secrets'); - - if (is_secret_interpolation) { - if (!context_map[interpolation_str]) { - interpolation_errors.push(error); - } - } else if (isDependencyInterpolation(interpolation_str)) { - const dependency_name = extractServiceName(interpolation_str, 'dependencies'); - const node = graph.nodes_map.get(`${dependency_name}--*`); - - let path_exist; - if (node) { - const splitted_interpolation_path = interpolation_str.split('.'); - let splitted_path; - const type = splitted_interpolation_path[2]; - if (type === 'services') { - splitted_path = splitted_interpolation_path.slice(4); - } else if (type === 'ingresses' || type === 'interfaces') { - splitted_path = splitted_interpolation_path.slice(2); - } else { - splitted_path = splitted_interpolation_path; - } - - path_exist = checkInterpolationPath(splitted_path, node); - } - if (!node || !path_exist) { - interpolation_errors.push(error); - } - } else { - const splitted_interpolation_path = interpolation_str.split('.'); - const last_element = splitted_interpolation_path[splitted_interpolation_path.length - 1]; - const is_service_url = interpolation_str.startsWith('services') && service_interface_config.has(last_element); - const is_database_url = isDatabasesInterpolation(interpolation_str); - - let service_name = ''; - if (is_database_url) { - service_name = extractServiceName(interpolation_str, 'databases'); - const node = graph.nodes.find(node => node instanceof ServiceNode && node.service_name === `${service_name}-db`); - if (!node) { - interpolation_errors.push(error); - } - continue; - } else if (is_service_url) { - service_name = extractServiceName(interpolation_str, 'services'); - if (!service_name) { - interpolation_errors.push(error); - continue; - } - const node = graph.nodes.find(node => node instanceof ServiceNode && node.service_name === service_name); - let path_exist; - if (node) { - splitted_interpolation_path.pop(); - path_exist = checkInterpolationPath(splitted_interpolation_path.slice(2), node); - } - if (!node || !path_exist) { - interpolation_errors.push(error); - } - } else { - interpolation_errors.push(error); - } - } - } - return interpolation_errors; -}; - -const extractInterpolationErrors = (errors: ValidationError[]) => { - // Regular expression for architect variables such as architect.environment and architect.build.tag - const architect_regex = /architect\./; - - // Regular expression for environment variables such as environment.ingresses.hello-world.hello.url - const architect_environment_regex = /environment\./; - return errors.filter(error => error.message.startsWith(RequiredInterpolationRule.PREFIX) && !architect_regex.test(error.value) && !architect_environment_regex.test(error.value)); -}; - -export const validateInterpolation = (component_spec: ComponentSpec, graph: Readonly): void => { +export const validateInterpolation = (component_spec: ComponentSpec, relax_validation?: boolean): void => { const { errors } = interpolateObject(component_spec, {}, { keys: true, values: true, file: component_spec.metadata.file, + relax_validation: relax_validation, }); const filtered_errors = errors.filter(error => !error.message.startsWith(RequiredInterpolationRule.PREFIX)); - let interpolation_errors = extractInterpolationErrors(errors); - interpolation_errors = findInterpolationErrors(interpolation_errors, component_spec, graph); - const all_errors = [...filtered_errors, ...interpolation_errors]; - - if (all_errors.length > 0) { - throw new ValidationErrors(all_errors, component_spec.metadata.file); + if (filtered_errors.length > 0) { + throw new ValidationErrors(filtered_errors, component_spec.metadata.file); } }; diff --git a/src/dependency-manager/utils/interpolation.ts b/src/dependency-manager/utils/interpolation.ts index 31752546b..3b264a41d 100644 --- a/src/dependency-manager/utils/interpolation.ts +++ b/src/dependency-manager/utils/interpolation.ts @@ -1,6 +1,6 @@ import { instanceToInstance } from 'class-transformer'; import deepmerge from 'deepmerge'; -import { EXPRESSION_REGEX, IF_EXPRESSION_REGEX } from '../spec/utils/interpolation'; +import { ARCHITECT_EXPRESSION_REGEX, DEPENDENCY_EXPRESSION_REGEX, DEPRECATED_DEPENDENCY_EXPRESSION_REGEX, ENVIRONMENT_EXPRESSION_REGEX, EXPRESSION_REGEX, IF_EXPRESSION_REGEX } from '../spec/utils/interpolation'; import { Dictionary } from './dictionary'; import { ValidationError, ValidationErrors } from './errors'; import { ArchitectParser } from './parser'; @@ -94,6 +94,7 @@ export interface InterpolateObjectOptions { keys?: boolean; values?: boolean; file?: { path: string, contents: string }; + relax_validation?: boolean; } const overwriteMerge = (destinationArray: any[], sourceArray: any[], options: deepmerge.Options) => sourceArray; @@ -110,6 +111,7 @@ export const interpolateObject = (obj: T, context: any, _options?: Interpolat const options = { keys: false, values: true, + relax_validation: false, ..._options, }; @@ -146,8 +148,18 @@ export const interpolateObject = (obj: T, context: any, _options?: Interpolat error.invalid_key = true; } } else if (options.values && typeof value === 'string') { - const parsed_value = parser.parseString(value, context_map); - el[key] = parsed_value; + let updated_value = value; + const special_case = DEPRECATED_DEPENDENCY_EXPRESSION_REGEX.test(value) || ENVIRONMENT_EXPRESSION_REGEX.test(value) || ARCHITECT_EXPRESSION_REGEX.test(value); + if (options.relax_validation && special_case) { + el[key] = updated_value; + } else if (options.relax_validation && DEPENDENCY_EXPRESSION_REGEX.test(value)) { + updated_value = value.replace(/(services\.)[^.]+(\.interfaces\.)([^.]+)/g, '$1ø$2ø'); + const parsed_value = parser.parseString(updated_value, context_map); + el[key] = parsed_value; + } else { + const parsed_value = parser.parseString(value, context_map); + el[key] = parsed_value; + } } else { el[key] = value; if (value instanceof Object) { diff --git a/test/dependency-manager/interpolation-validation.test.ts b/test/dependency-manager/interpolation-validation.test.ts index 9f1f1d18a..003fe4ced 100644 --- a/test/dependency-manager/interpolation-validation.test.ts +++ b/test/dependency-manager/interpolation-validation.test.ts @@ -1,51 +1,7 @@ import { expect } from 'chai'; -import { buildSpecFromYml, DependencyEdge, DependencyGraphMutable, DependencyNode, ServiceNode, TaskNode, validateInterpolation, ValidationErrors } from '../../src'; +import { buildSpecFromYml, validateInterpolation, ValidationErrors } from '../../src'; describe('interpolation-validation', () => { - const mock_graph: Readonly = { - nodes: [ - { - instance_id: '', - __type: 'service', - ref: 'hello-world--api', - component_ref: 'hello-world', - service_name: 'api', - artifact_image: undefined, - } as ServiceNode, - ], - edges: [], - validated: false, - nodes_map: {} as Map, - edges_map: {} as Map, - addNode: function (node: DependencyNode): DependencyNode { - throw new Error('Function not implemented.'); - }, - removeNodeByRef: function (ref: string): void { - throw new Error('Function not implemented.'); - }, - removeEdgeByRef: function (edge_ref: string): void { - throw new Error('Function not implemented.'); - }, - addEdge: function (edge: DependencyEdge): DependencyEdge { - throw new Error('Function not implemented.'); - }, - getNodeByRef: function (ref: string): DependencyNode { - throw new Error('Function not implemented.'); - }, - getDownstreamNodes: function (node: DependencyNode): DependencyNode[] { - throw new Error('Function not implemented.'); - }, - removeNode: function (node_ref: string, cleanup_dangling: boolean): void { - throw new Error('Function not implemented.'); - }, - getUpstreamNodes: function (node: DependencyNode): DependencyNode[] { - throw new Error('Function not implemented.'); - }, - getDependsOn: function (current_node: ServiceNode | TaskNode): (ServiceNode | TaskNode)[] { - throw new Error('Function not implemented.'); - }, - }; - describe('validate build block', () => { it('cannot use secret in build block', () => { const component_config = ` @@ -62,7 +18,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -80,7 +36,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -98,7 +54,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -116,7 +72,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config) expect(() => { - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -133,7 +89,7 @@ describe('interpolation-validation', () => { `; const component_spec = buildSpecFromYml(component_config); - validateInterpolation(component_spec, mock_graph); + validateInterpolation(component_spec); }); it('can use conditional around build block if local', () => { @@ -148,7 +104,7 @@ describe('interpolation-validation', () => { `; const component_spec = buildSpecFromYml(component_config); - validateInterpolation(component_spec, mock_graph); + validateInterpolation(component_spec); }); it('can use conditional around service block with build block if local', () => { @@ -163,7 +119,7 @@ describe('interpolation-validation', () => { `; const component_spec = buildSpecFromYml(component_config); - validateInterpolation(component_spec, mock_graph); + validateInterpolation(component_spec); }); }); @@ -180,7 +136,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -196,7 +152,7 @@ describe('interpolation-validation', () => { const component_spec = buildSpecFromYml(component_config); expect(() => { - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }).to.be.throws(ValidationErrors); }); @@ -213,7 +169,7 @@ describe('interpolation-validation', () => { `; const component_spec = buildSpecFromYml(component_config); - validateInterpolation(component_spec, {} as Readonly); + validateInterpolation(component_spec); }); it('can still use conditional without build block', () => { @@ -227,46 +183,7 @@ describe('interpolation-validation', () => { `; const component_spec = buildSpecFromYml(component_config); - validateInterpolation(component_spec, {} as Readonly); - }); - - it('fail when using interpolation where path does not exist', () => { - const component_config = ` - name: hello-world - services: - api: - build: - context: . - environment: - DB_ADDR: \${{ services.database.interfaces.main.url }} - `; - - const component_spec = buildSpecFromYml(component_config); - - expect(() => { - validateInterpolation(component_spec, mock_graph); - }).to.be.throws(ValidationErrors); - }); - - it('fail when secret does not exist', () => { - const component_config = ` - name: hello-world - secrets: - world_text: - default: World - services: - api: - build: - context: . - environment: - WORLD_TEXT: \${{ secrets.notfound }} - `; - - const component_spec = buildSpecFromYml(component_config); - - expect(() => { - validateInterpolation(component_spec, {} as Readonly); - }).to.be.throws(ValidationErrors); + validateInterpolation(component_spec); }); }); });