Skip to content

Commit

Permalink
fix(pipelines): can't use exports from very long stack names (aws#18039)
Browse files Browse the repository at this point in the history
The variable namespace identifier in CodePipeline allows a maximum of
100 characters. If we ever come up with an identifier that would be
too long, trim it down.

While writing tests for this, discovered that actions exhibited the same
length issues, and did the same there too.

Fixes aws#17436.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored and TikiTDO committed Feb 21, 2022
1 parent 1c2b752 commit c8381f5
Show file tree
Hide file tree
Showing 6 changed files with 130 additions and 26 deletions.
14 changes: 2 additions & 12 deletions packages/@aws-cdk/pipelines/lib/codepipeline/_codebuild-factory.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as crypto from 'crypto';
import * as fs from 'fs';
import * as path from 'path';
import * as codebuild from '@aws-cdk/aws-codebuild';
Expand All @@ -8,9 +7,10 @@ import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import { IDependable, Stack } from '@aws-cdk/core';
import { Construct, Node } from 'constructs';
import { FileSetLocation, ShellStep, StackDeployment, StackOutputReference } from '../blueprint';
import { FileSetLocation, ShellStep, StackOutputReference } from '../blueprint';
import { PipelineQueries } from '../helpers-internal/pipeline-queries';
import { cloudAssemblyBuildSpecDir, obtainScope } from '../private/construct-internals';
import { hash, stackVariableNamespace } from '../private/identifiers';
import { mapValues, mkdict, noEmptyObject, noUndefined, partition } from '../private/javascript';
import { ArtifactMap } from './artifact-map';
import { CodeBuildStep } from './codebuild-step';
Expand Down Expand Up @@ -426,12 +426,6 @@ function isDefined<A>(x: A | undefined): x is NonNullable<A> {
return x !== undefined;
}

function hash<A>(obj: A) {
const d = crypto.createHash('sha256');
d.update(JSON.stringify(obj));
return d.digest('hex');
}

/**
* Serialize a build environment to data (get rid of constructs & objects), so we can JSON.stringify it
*/
Expand All @@ -447,10 +441,6 @@ function serializeBuildEnvironment(env: codebuild.BuildEnvironment) {
};
}

export function stackVariableNamespace(stack: StackDeployment) {
return stack.stackArtifactId;
}

/**
* Whether the given string contains a reference to a CodePipeline variable
*/
Expand Down
14 changes: 3 additions & 11 deletions packages/@aws-cdk/pipelines/lib/codepipeline/codepipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ import * as cxapi from '@aws-cdk/cx-api';
import { Construct, Node } from 'constructs';
import { AssetType, FileSet, IFileSetProducer, ManualApprovalStep, ShellStep, StackAsset, StackDeployment, Step } from '../blueprint';
import { DockerCredential, dockerCredentialsInstallCommands, DockerCredentialUsage } from '../docker-credentials';
import { GraphNode, GraphNodeCollection, isGraph, AGraphNode, PipelineGraph } from '../helpers-internal';
import { GraphNodeCollection, isGraph, AGraphNode, PipelineGraph } from '../helpers-internal';
import { PipelineBase } from '../main';
import { AssetSingletonRole } from '../private/asset-singleton-role';
import { appOf, assemblyBuilderOf, embeddedAsmPath, obtainScope } from '../private/construct-internals';
import { toPosixPath } from '../private/fs';
import { actionName, stackVariableNamespace } from '../private/identifiers';
import { enumerate, flatten, maybeSuffix, noUndefined } from '../private/javascript';
import { writeTemplateConfiguration } from '../private/template-configuration';
import { CodeBuildFactory, mergeCodeBuildOptions, stackVariableNamespace } from './_codebuild-factory';
import { CodeBuildFactory, mergeCodeBuildOptions } from './_codebuild-factory';
import { ArtifactMap } from './artifact-map';
import { CodeBuildStep } from './codebuild-step';
import { CodePipelineActionFactoryResult, ICodePipelineActionFactory } from './codepipeline-action-factory';
Expand Down Expand Up @@ -839,15 +840,6 @@ enum CodeBuildProjectType {
STEP = 'STEP',
}

function actionName<A>(node: GraphNode<A>, parent: GraphNode<A>) {
const names = node.ancestorPath(parent).map(n => n.id);
return names.map(sanitizeName).join('.').substr(0, 100); // Cannot exceed 100 chars
}

function sanitizeName(x: string): string {
return x.replace(/[^A-Za-z0-9.@\-_]/g, '_');
}

/**
* Take a set of tranches and split them up into groups so
* that no set of tranches has more than n items total
Expand Down
61 changes: 61 additions & 0 deletions packages/@aws-cdk/pipelines/lib/private/identifiers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import * as crypto from 'crypto';
import { StackDeployment } from '../blueprint/stack-deployment';
import { GraphNode } from '../helpers-internal/graph';

export function hash<A>(obj: A) {
const d = crypto.createHash('sha256');
d.update(JSON.stringify(obj));
return d.digest('hex');
}

export function actionName<A>(node: GraphNode<A>, parent: GraphNode<A>) {
const names = node.ancestorPath(parent).map(n => n.id).map(sanitizeName);

// Something slightly complicated here:
//
// Because we'll have structured action names like:
//
// 'VeryLongStackName.Prepare', 'VeryLongStackName.Deploy'
//
// it would be shitty if cut and hashed them differently:
//
// 'VeryLongSAAAAA.Prepare', 'VeryLonBBBBBme.Deploy'
//
// wouldn't sort and comprehend nicely. We will therefore trim each component individually.
const totalMax = 100; // Max length of everything

// No need to do anything
if (names.join('.').length <= totalMax) {
return names.join('.');
}

const componentMin = 15; // Can't really go shorter than this, becomes unreadable
const dots = names.length - 1;
const maxLength = Math.max(componentMin, Math.floor((totalMax - dots) / names.length));
const trimmedNames = names.map(name => limitIdentifierLength(name, maxLength));

return limitIdentifierLength(trimmedNames.join('.'), totalMax); // Final trim in case we couldn't make it
}

export function stackVariableNamespace(stack: StackDeployment) {
return limitIdentifierLength(stack.stackArtifactId, 100);
}

function sanitizeName(x: string): string {
return x.replace(/[^A-Za-z0-9.@\-_]/g, '_');
}


/**
* Makes sure the given identifier length does not exceed N characters
*
* Replaces characters in the middle (to leave the start and end identifiable) and replaces
* them with a hash to prevent collissions.
*/
export function limitIdentifierLength(s: string, n: number): string {
if (s.length <= n) { return s; }
const h = hash(s).substr(0, 8);
const mid = Math.floor((n - h.length) / 2);

return s.substr(0, mid) + h + s.substr(s.length - mid);
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Template, Match } from '@aws-cdk/assertions';
import { Stack } from '@aws-cdk/core';
import * as cdkp from '../../lib';
import { PIPELINE_ENV, TestApp } from '../testhelpers';
import { PIPELINE_ENV, TestApp, ModernTestGitHubNpmPipeline, AppWithOutput } from '../testhelpers';

let app: TestApp;
let pipelineStack: Stack;
Expand Down Expand Up @@ -41,4 +41,25 @@ test('additionalinputs creates the right commands', () => {
})),
},
});
});

test('envFromOutputs works even with very long stage and stack names', () => {
const pipeline = new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk');

const myApp = new AppWithOutput(app, 'Alpha'.repeat(20), {
stackId: 'Stack'.repeat(20),
});

pipeline.addStage(myApp, {
post: [
new cdkp.ShellStep('Approve', {
commands: ['/bin/true'],
envFromCfnOutputs: {
THE_OUTPUT: myApp.theOutput,
},
}),
],
});

// THEN - did not throw an error about identifier lengths
});
36 changes: 36 additions & 0 deletions packages/@aws-cdk/pipelines/test/private/identifiers.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { Graph } from '../../lib/helpers-internal';
import { actionName } from '../../lib/private/identifiers';
import { mkGraph } from '../blueprint/helpers-internal/util';

test('actionName trims subcomponents the same way', () => {
const long1 = 'ExtremelyLong'.repeat(10);
const long2 = 'AlsoLong'.repeat(10);

const g = mkGraph('MyGraph', G => {
G.graph(long1, [], G1 => {
G1.graph(long2, [], G2 => {
G2.node('Prepare');
G2.node('Deploy');
});
});
});

const G2 = ((g.tryGetChild(long1) as Graph<any>)?.tryGetChild(long2) as Graph<any>);
expect(G2).toBeDefined();

const prep = G2.tryGetChild('Prepare');
const deploy = G2.tryGetChild('Deploy');

expect(prep).toBeDefined();
expect(deploy).toBeDefined();

// ActionNames have the same prefix
const prepParts = actionName(prep!, g).split('.');
const deployParts = actionName(deploy!, g).split('.');

// Final parts are unchanged
expect(prepParts.pop()).toEqual('Prepare');
expect(deployParts.pop()).toEqual('Deploy');
// Prefixes are the same
expect(prepParts).toEqual(deployParts);
});
8 changes: 6 additions & 2 deletions packages/@aws-cdk/pipelines/test/testhelpers/test-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,17 @@ export class OneStackApp extends Stage {
}
}

export interface AppWithOutputProps extends StageProps {
readonly stackId?: string;
}

export class AppWithOutput extends Stage {
public readonly theOutput: CfnOutput;

constructor(scope: Construct, id: string, props?: StageProps) {
constructor(scope: Construct, id: string, props: AppWithOutputProps = {}) {
super(scope, id, props);

const stack = new BucketStack(this, 'Stack');
const stack = new BucketStack(this, props.stackId ?? 'Stack');
this.theOutput = new CfnOutput(stack, 'MyOutput', { value: stack.bucket.bucketName });
}
}
Expand Down

0 comments on commit c8381f5

Please sign in to comment.