From a0ed0f8f69d31e65af36c9b81d53bb7499432b69 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 3 Aug 2018 14:21:52 +0200 Subject: [PATCH 1/7] Assets: upload to and grant permissions on prefix We need to give asset consumers permissions on all versions of an asset, not just the latest version. Otherwise, we will never be able to do rolling updates. Also add caching on AWS client instances, so with multiple asset uploads we don't have to construct a new S3 client for every asset (incurring credential lookups for each one). This fixes #484. --- packages/@aws-cdk/assets/lib/asset.ts | 30 ++- .../integ.assets.directory.lit.expected.json | 18 +- .../test/integ.assets.file.lit.expected.json | 18 +- ...integ.assets.permissions.lit.expected.json | 18 +- .../test/integ.assets.refs.lit.expected.json | 22 +- packages/@aws-cdk/assets/test/test.asset.ts | 244 +++--------------- packages/@aws-cdk/cx-api/lib/cxapi.ts | 25 ++ packages/aws-cdk/lib/api/util/sdk.ts | 152 +++++++---- packages/aws-cdk/lib/assets.ts | 7 +- 9 files changed, 246 insertions(+), 288 deletions(-) diff --git a/packages/@aws-cdk/assets/lib/asset.ts b/packages/@aws-cdk/assets/lib/asset.ts index a3fc81ed107e3..d22cc3444d008 100644 --- a/packages/@aws-cdk/assets/lib/asset.ts +++ b/packages/@aws-cdk/assets/lib/asset.ts @@ -67,6 +67,8 @@ export class Asset extends cdk.Construct { private readonly bucket: s3.BucketRef; + private readonly s3PrefixParam: cdk.Parameter; + constructor(parent: cdk.Construct, id: string, props: GenericAssetProps) { super(parent, id); @@ -84,16 +86,19 @@ export class Asset extends cdk.Construct { description: `S3 bucket for asset "${this.path}"`, }); - const keyParam = new cdk.Parameter(this, 'S3ObjectKey', { + this.s3PrefixParam = new cdk.Parameter(this, 'S3Prefix', { + type: 'String', + description: `S3 prefix for asset "${this.path}"` + }); + + const keyParam = new cdk.Parameter(this, 'S3VersionKey', { type: 'String', - description: `S3 object for asset "${this.path}"` + description: `S3 key for asset version "${this.path}"` }); this.s3BucketName = bucketParam.value; this.s3ObjectKey = keyParam.value; - // grant the lambda's role read permissions on the code s3 object - this.bucket = s3.BucketRef.import(parent, 'AssetBucket', { bucketName: this.s3BucketName }); @@ -101,16 +106,25 @@ export class Asset extends cdk.Construct { // form the s3 URL of the object key this.s3Url = this.bucket.urlForObject(this.s3ObjectKey); + // Get a unique identifier for this asset, which will be used + // as a folder to group different versions of the same asset together. + // + // Even though this code looks horrible, this is actually not a terrible thing + // to do. The generated ID will contain the *stack name* as well, which is a + // perfectly nice thing to do to disambiguate similarly named assets in different stacks. + const uniqueId = new cdk.HashedAddressingScheme().allocateAddress(this.path.split('/')); + // attach metadata to the lambda function which includes information // for tooling to be able to package and upload a directory to the // s3 bucket and plug in the bucket name and key in the correct // parameters. - const asset: cxapi.AssetMetadataEntry = { path: this.assetPath, + id: uniqueId, packaging: props.packaging, s3BucketParameter: bucketParam.logicalId, s3KeyParameter: keyParam.logicalId, + s3PrefixParameter: this.s3PrefixParam.logicalId }; this.addMetadata(cxapi.ASSET_METADATA, asset); @@ -124,7 +138,11 @@ export class Asset extends cdk.Construct { * Grants read permissions to the principal on the asset's S3 object. */ public grantRead(principal?: iam.IPrincipal) { - this.bucket.grantRead(principal, this.s3ObjectKey); + // We give permissions on all files with the same prefix. Presumably + // different versions of the same file will have the same prefix + // and we don't want to accidentally revoke permission on old versions + // when deploying a new version. + this.bucket.grantRead(principal, new cdk.FnConcat(this.s3PrefixParam.value, "*")); } } diff --git a/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json index e3ca3797ddd0c..937a3fe810b53 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json @@ -4,9 +4,13 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-test/SampleAsset\"" }, - "SampleAssetS3ObjectKey6F5D200B": { + "SampleAssetS3Prefix6E89595F": { "Type": "String", - "Description": "S3 object for asset \"aws-cdk-asset-test/SampleAsset\"" + "Description": "S3 prefix for asset \"aws-cdk-asset-test/SampleAsset\"" + }, + "SampleAssetS3VersionKey3E106D34": { + "Type": "String", + "Description": "S3 key for asset version \"aws-cdk-asset-test/SampleAsset\"" } }, "Resources": { @@ -76,7 +80,15 @@ }, "/", { - "Ref": "SampleAssetS3ObjectKey6F5D200B" + "Fn::Join": [ + "", + [ + { + "Ref": "SampleAssetS3Prefix6E89595F" + }, + "*" + ] + ] } ] ] diff --git a/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json index 99d5cee84363e..013773a690b8a 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json @@ -4,9 +4,13 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-file-test/SampleAsset\"" }, - "SampleAssetS3ObjectKey6F5D200B": { + "SampleAssetS3Prefix6E89595F": { "Type": "String", - "Description": "S3 object for asset \"aws-cdk-asset-file-test/SampleAsset\"" + "Description": "S3 prefix for asset \"aws-cdk-asset-file-test/SampleAsset\"" + }, + "SampleAssetS3VersionKey3E106D34": { + "Type": "String", + "Description": "S3 key for asset version \"aws-cdk-asset-file-test/SampleAsset\"" } }, "Resources": { @@ -76,7 +80,15 @@ }, "/", { - "Ref": "SampleAssetS3ObjectKey6F5D200B" + "Fn::Join": [ + "", + [ + { + "Ref": "SampleAssetS3Prefix6E89595F" + }, + "*" + ] + ] } ] ] diff --git a/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json index e7540be27bb73..a98abf17c408e 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json @@ -4,9 +4,13 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-refs/MyFile\"" }, - "MyFileS3ObjectKey4641930D": { + "MyFileS3PrefixF078A585": { "Type": "String", - "Description": "S3 object for asset \"aws-cdk-asset-refs/MyFile\"" + "Description": "S3 prefix for asset \"aws-cdk-asset-refs/MyFile\"" + }, + "MyFileS3VersionKey568C3C9F": { + "Type": "String", + "Description": "S3 key for asset version \"aws-cdk-asset-refs/MyFile\"" } }, "Resources": { @@ -76,7 +80,15 @@ }, "/", { - "Ref": "MyFileS3ObjectKey4641930D" + "Fn::Join": [ + "", + [ + { + "Ref": "MyFileS3PrefixF078A585" + }, + "*" + ] + ] } ] ] diff --git a/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json index 659b218855cc3..dc8769adb0fdc 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json @@ -4,9 +4,13 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-refs/SampleAsset\"" }, - "SampleAssetS3ObjectKey6F5D200B": { + "SampleAssetS3Prefix6E89595F": { "Type": "String", - "Description": "S3 object for asset \"aws-cdk-asset-refs/SampleAsset\"" + "Description": "S3 prefix for asset \"aws-cdk-asset-refs/SampleAsset\"" + }, + "SampleAssetS3VersionKey3E106D34": { + "Type": "String", + "Description": "S3 key for asset version \"aws-cdk-asset-refs/SampleAsset\"" } }, "Outputs": { @@ -20,7 +24,7 @@ }, "S3ObjectKey": { "Value": { - "Ref": "SampleAssetS3ObjectKey6F5D200B" + "Ref": "SampleAssetS3VersionKey3E106D34" }, "Export": { "Name": "aws-cdk-asset-refs:S3ObjectKey" @@ -46,7 +50,7 @@ }, "/", { - "Ref": "SampleAssetS3ObjectKey6F5D200B" + "Ref": "SampleAssetS3VersionKey3E106D34" } ] ] @@ -123,7 +127,15 @@ }, "/", { - "Ref": "SampleAssetS3ObjectKey6F5D200B" + "Fn::Join": [ + "", + [ + { + "Ref": "SampleAssetS3Prefix6E89595F" + }, + "*" + ] + ] } ] ] diff --git a/packages/@aws-cdk/assets/test/test.asset.ts b/packages/@aws-cdk/assets/test/test.asset.ts index eee3edd0c7686..6e8319287ad7c 100644 --- a/packages/@aws-cdk/assets/test/test.asset.ts +++ b/packages/@aws-cdk/assets/test/test.asset.ts @@ -1,4 +1,4 @@ -import { expect } from '@aws-cdk/assert'; +import { expect, haveResource } from '@aws-cdk/assert'; import iam = require('@aws-cdk/aws-iam'); import cdk = require('@aws-cdk/cdk'); import { Test } from 'nodeunit'; @@ -19,24 +19,18 @@ export = { test.ok(entry, 'found metadata entry'); test.deepEqual(entry!.data, { path: dirPath, + id: 'MyAsset', packaging: 'zip', s3BucketParameter: 'MyAssetS3Bucket68C9B344', - s3KeyParameter: 'MyAssetS3ObjectKeyC07605E4' + s3KeyParameter: 'MyAssetS3VersionKey68E1A45D', + s3PrefixParameter: 'MyAssetS3Prefix5E79A15D', }); - // verify that now the template contains two parameters for this asset - expect(stack).toMatch({ - Parameters: { - MyAssetS3Bucket68C9B344: { - Type: "String", - Description: 'S3 bucket for asset "MyAsset"' - }, - MyAssetS3ObjectKeyC07605E4: { - Type: "String", - Description: 'S3 object for asset "MyAsset"' - } - } - }); + // verify that now the template contains parameters for this asset + const template = stack.toCloudFormation(); + test.equal(template.Parameters.MyAssetS3Bucket68C9B344.Type, 'String'); + test.equal(template.Parameters.MyAssetS3VersionKey68E1A45D.Type, 'String'); + test.equal(template.Parameters.MyAssetS3Prefix5E79A15D.Type, 'String'); test.done(); }, @@ -50,22 +44,17 @@ export = { test.deepEqual(entry!.data, { path: filePath, packaging: 'file', + id: 'MyAsset', s3BucketParameter: 'MyAssetS3Bucket68C9B344', - s3KeyParameter: 'MyAssetS3ObjectKeyC07605E4' + s3KeyParameter: 'MyAssetS3VersionKey68E1A45D', + s3PrefixParameter: 'MyAssetS3Prefix5E79A15D', }); - expect(stack).toMatch({ - Parameters: { - MyAssetS3Bucket68C9B344: { - Type: "String", - Description: 'S3 bucket for asset "MyAsset"' - }, - MyAssetS3ObjectKeyC07605E4: { - Type: "String", - Description: 'S3 object for asset "MyAsset"' - } - } - }); + // verify that now the template contains parameters for this asset + const template = stack.toCloudFormation(); + test.equal(template.Parameters.MyAssetS3Bucket68C9B344.Type, 'String'); + test.equal(template.Parameters.MyAssetS3VersionKey68E1A45D.Type, 'String'); + test.equal(template.Parameters.MyAssetS3Prefix5E79A15D.Type, 'String'); test.done(); }, @@ -82,188 +71,25 @@ export = { asset.grantRead(group); - expect(stack).toMatch({ - Resources: { - MyUserDC45028B: { - Type: "AWS::IAM::User" - }, - MyUserDefaultPolicy7B897426: { - Type: "AWS::IAM::Policy", - Properties: { - PolicyDocument: { - Statement: [ - { - Action: [ - "s3:GetObject*", - "s3:GetBucket*", - "s3:List*" - ], - Effect: "Allow", - Resource: [ - { - "Fn::Join": [ - "", - [ - "arn", - ":", - { - Ref: "AWS::Partition" - }, - ":", - "s3", - ":", - "", - ":", - "", - ":", - { - Ref: "MyAssetS3Bucket68C9B344" - } - ] - ] - }, - { - "Fn::Join": [ - "", - [ - { - "Fn::Join": [ - "", - [ - "arn", - ":", - { - Ref: "AWS::Partition" - }, - ":", - "s3", - ":", - "", - ":", - "", - ":", - { - Ref: "MyAssetS3Bucket68C9B344" - } - ] - ] - }, - "/", - { - Ref: "MyAssetS3ObjectKeyC07605E4" - } - ] - ] - } - ] - } - ], - Version: "2012-10-17" - }, - PolicyName: "MyUserDefaultPolicy7B897426", - Users: [ - { - Ref: "MyUserDC45028B" - } + expect(stack).to(haveResource('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: ["s3:GetObject*", "s3:GetBucket*", "s3:List*"], + Resource: [ + {"Fn::Join": ["", ["arn", ":", {Ref: "AWS::Partition"}, ":", "s3", ":", "", ":", "", ":", {Ref: "MyAssetS3Bucket68C9B344"}]]}, + {"Fn::Join": [ "", [ + {"Fn::Join": ["", [ "arn", ":", {Ref: "AWS::Partition"}, ":", "s3", ":", "", ":", "", ":", {Ref: "MyAssetS3Bucket68C9B344"}]]}, + "/", + {"Fn::Join": ["", [ + {Ref: "MyAssetS3Prefix5E79A15D"}, + "*" + ]]} + ]]} ] - } - }, - MyGroupCBA54B1B: { - Type: "AWS::IAM::Group" - }, - MyGroupDefaultPolicy72C41231: { - Type: "AWS::IAM::Policy", - Properties: { - Groups: [ - { - Ref: "MyGroupCBA54B1B" - } - ], - PolicyDocument: { - Statement: [ - { - Action: [ - "s3:GetObject*", - "s3:GetBucket*", - "s3:List*" - ], - Effect: "Allow", - Resource: [ - { - "Fn::Join": [ - "", - [ - "arn", - ":", - { - Ref: "AWS::Partition" - }, - ":", - "s3", - ":", - "", - ":", - "", - ":", - { - Ref: "MyAssetS3Bucket68C9B344" - } - ] - ] - }, - { - "Fn::Join": [ - "", - [ - { - "Fn::Join": [ - "", - [ - "arn", - ":", - { - Ref: "AWS::Partition" - }, - ":", - "s3", - ":", - "", - ":", - "", - ":", - { - Ref: "MyAssetS3Bucket68C9B344" - } - ] - ] - }, - "/", - { - Ref: "MyAssetS3ObjectKeyC07605E4" - } - ] - ] - } - ] - } - ], - Version: "2012-10-17" - }, - PolicyName: "MyGroupDefaultPolicy72C41231" - } - } - }, - Parameters: { - MyAssetS3Bucket68C9B344: { - Type: "String", - Description: "S3 bucket for asset \"MyAsset\"" - }, - MyAssetS3ObjectKeyC07605E4: { - Type: "String", - Description: "S3 object for asset \"MyAsset\"" } - } - }); + ] + }})); test.done(); }, @@ -274,4 +100,4 @@ export = { })); test.done(); } -}; +}; \ No newline at end of file diff --git a/packages/@aws-cdk/cx-api/lib/cxapi.ts b/packages/@aws-cdk/cx-api/lib/cxapi.ts index 06b5e453caa4a..5d0df2d3168e6 100644 --- a/packages/@aws-cdk/cx-api/lib/cxapi.ts +++ b/packages/@aws-cdk/cx-api/lib/cxapi.ts @@ -109,10 +109,35 @@ export const DEFAULT_REGION_CONTEXT_KEY = 'aws:cdk:toolkit:default-region'; export const ASSET_METADATA = 'aws:cdk:asset'; export interface AssetMetadataEntry { + /** + * Path on disk to the asset + */ path: string; + + /** + * Logical identifier for the asset + */ + id: string; + + /** + * Requested packaging style + */ packaging: 'zip' | 'file'; + + /** + * Name of parameter where S3 bucket should be passed in + */ s3BucketParameter: string; + + /** + * Name of parameter where S3 key should be passed in + */ s3KeyParameter: string; + + /** + * Name of parameter where S3 folder should be passed in + */ + s3PrefixParameter: string; } /** diff --git a/packages/aws-cdk/lib/api/util/sdk.ts b/packages/aws-cdk/lib/api/util/sdk.ts index 2f12c5db467dc..8db8f270778a9 100644 --- a/packages/aws-cdk/lib/api/util/sdk.ts +++ b/packages/aws-cdk/lib/api/util/sdk.ts @@ -15,54 +15,113 @@ import { AccountAccessKeyCache } from './account-cache'; * to the requested account. */ export class SDK { - private defaultAccountFetched = false; - private defaultAccountId?: string = undefined; private readonly userAgent: string; - private readonly accountCache = new AccountAccessKeyCache(); + private readonly s3ClientCache: AWSClientCache; + private readonly cfnClientCache: AWSClientCache; + private readonly ec2ClientCache: AWSClientCache; + private readonly ssmClientCache: AWSClientCache; constructor() { // Find the package.json from the main toolkit const pkg = (require.main as any).require('../package.json'); this.userAgent = `${pkg.name}/${pkg.version}`; + + this.s3ClientCache = new AWSClientCache(S3, this.userAgent); + this.cfnClientCache = new AWSClientCache(CloudFormation, this.userAgent); + this.ec2ClientCache = new AWSClientCache(EC2, this.userAgent); } - public async cloudFormation(environment: Environment, mode: Mode): Promise { - return new CloudFormation({ - region: environment.region, - credentialProvider: await this.getCredentialProvider(environment.account, mode), - customUserAgent: this.userAgent - }); + public cloudFormation(environment: Environment, mode: Mode): Promise { + return this.cfnClientCache.get(environment.account, environment.region, mode); } - public async ec2(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { - return new EC2({ - region, - credentialProvider: await this.getCredentialProvider(awsAccountId, mode), - customUserAgent: this.userAgent - }); + public ec2(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { + return this.ec2ClientCache.get(awsAccountId, region, mode); } - public async ssm(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { - return new SSM({ - region, - credentialProvider: await this.getCredentialProvider(awsAccountId, mode), - customUserAgent: this.userAgent - }); + public ssm(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { + return this.ssmClientCache.get(awsAccountId, region, mode); } public async s3(environment: Environment, mode: Mode): Promise { - return new S3({ - region: environment.region, - credentialProvider: await this.getCredentialProvider(environment.account, mode), - customUserAgent: this.userAgent - }); + return this.s3ClientCache.get(environment.account, environment.region, mode); } public defaultRegion() { return config.region; } - public async defaultAccount() { + public defaultAccount(): Promise { + return DEFAULT_ACCOUNT.get(); + } +} + +/** + * Factory and cache for AWS clients + */ +class AWSClientCache { + private readonly cache: {[key: string]: T} = {}; + + public constructor(private readonly klass: new (u: any) => T, private readonly userAgent: string) { + } + + public async get(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { + const key = `${awsAccountId}-${region}-${mode}`; + if (!(key in this.cache)) { + this.cache[key] = new this.klass({ + region, + credentialProvider: await this.getCredentialProvider(awsAccountId, mode), + customUserAgent: this.userAgent + }); + } + return this.cache[key]; + } + + private async getCredentialProvider(awsAccountId: string | undefined, mode: Mode): Promise { + // If requested account is undefined or equal to default account, use default credentials provider. + const defaultAccount = await DEFAULT_ACCOUNT.get(); + if (!awsAccountId || awsAccountId === defaultAccount) { + debug(`Using default AWS SDK credentials for account ${awsAccountId}`); + return undefined; + } + + const triedSources: CredentialProviderSource[] = []; + + // Otherwise, inspect the various credential sources we have + for (const source of PluginHost.instance.credentialProviderSources) { + if (!(await source.isAvailable())) { + debug('Credentials source %s is not available, ignoring it.', source.name); + continue; + } + triedSources.push(source); + + if (!(await source.canProvideCredentials(awsAccountId))) { continue; } + debug(`Using ${source.name} credentials for account ${awsAccountId}`); + + return await source.getProvider(awsAccountId, mode); + } + + const sourceNames = ['default credentials'].concat(triedSources.map(s => s.name)).join(', '); + + throw new Error(`Need to perform AWS calls for account ${awsAccountId}, but no credentials found. Tried: ${sourceNames}.`); + } +} + +/** + * Class to retrieve the current default account + * + * This requires making an STS call using the credentials + * currently available to the AWS SDK, and is heavily cached. + */ +class DefaultAccount { + private defaultAccountFetched = false; + private defaultAccountId?: string = undefined; + private readonly accountCache = new AccountAccessKeyCache(); + + /** + * Return the default account + */ + public async get(): Promise { if (!this.defaultAccountFetched) { this.defaultAccountId = await this.lookupDefaultAccount(); this.defaultAccountFetched = true; @@ -70,7 +129,7 @@ export class SDK { return this.defaultAccountId; } - private async lookupDefaultAccount() { + private async lookupDefaultAccount(): Promise { try { debug('Resolving default credentials'); const chain = new CredentialProviderChain(); @@ -99,33 +158,12 @@ export class SDK { return undefined; } } - - private async getCredentialProvider(awsAccountId: string | undefined, mode: Mode): Promise { - // If requested account is undefined or equal to default account, use default credentials provider. - const defaultAccount = await this.defaultAccount(); - if (!awsAccountId || awsAccountId === defaultAccount) { - debug(`Using default AWS SDK credentials for account ${awsAccountId}`); - return undefined; - } - - const triedSources: CredentialProviderSource[] = []; - - // Otherwise, inspect the various credential sources we have - for (const source of PluginHost.instance.credentialProviderSources) { - if (!(await source.isAvailable())) { - debug('Credentials source %s is not available, ignoring it.', source.name); - continue; - } - triedSources.push(source); - - if (!(await source.canProvideCredentials(awsAccountId))) { continue; } - debug(`Using ${source.name} credentials for account ${awsAccountId}`); - - return await source.getProvider(awsAccountId, mode); - } - - const sourceNames = ['default credentials'].concat(triedSources.map(s => s.name)).join(', '); - - throw new Error(`Need to perform AWS calls for account ${awsAccountId}, but no credentials found. Tried: ${sourceNames}.`); - } } + +/** + * Singleton instance of DefaultAccount. + * + * Per execution there will only ever be one default account, so we might as well + * cache it process-wide. + */ +const DEFAULT_ACCOUNT = new DefaultAccount(); \ No newline at end of file diff --git a/packages/aws-cdk/lib/assets.ts b/packages/aws-cdk/lib/assets.ts index fcda72433f870..e3c3dbf9d9495 100644 --- a/packages/aws-cdk/lib/assets.ts +++ b/packages/aws-cdk/lib/assets.ts @@ -69,8 +69,10 @@ async function prepareFileAsset( const data = await fs.readFile(filePath); + const s3KeyPrefix = `assets/${asset.id}/`; + const { key, changed } = await toolkitInfo.uploadIfChanged(data, { - s3KeyPrefix: 'assets/', + s3KeyPrefix, s3KeySuffix: path.extname(filePath), contentType }); @@ -84,7 +86,8 @@ async function prepareFileAsset( return [ { ParameterKey: asset.s3BucketParameter, ParameterValue: toolkitInfo.bucketName }, - { ParameterKey: asset.s3KeyParameter, ParameterValue: key } + { ParameterKey: asset.s3KeyParameter, ParameterValue: key }, + { ParameterKey: asset.s3PrefixParameter, ParameterValue: s3KeyPrefix }, ]; } From b5c3f6fd9b193ad9ef6cbc7e3e26418361005179 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Aug 2018 16:08:37 +0200 Subject: [PATCH 2/7] Toolkit: add caching for AWS credential providers. --- packages/aws-cdk/lib/api/util/sdk.ts | 137 ++++++++++++++++++--------- 1 file changed, 94 insertions(+), 43 deletions(-) diff --git a/packages/aws-cdk/lib/api/util/sdk.ts b/packages/aws-cdk/lib/api/util/sdk.ts index 49e24cb84658a..b3e62dbacfd83 100644 --- a/packages/aws-cdk/lib/api/util/sdk.ts +++ b/packages/aws-cdk/lib/api/util/sdk.ts @@ -19,22 +19,24 @@ import { SharedIniFile } from './sdk_ini_file'; * to the requested account. */ export class SDK { - private defaultAccountFetched = false; - private defaultAccountId?: string = undefined; private readonly userAgent: string; - private readonly accountCache = new AccountAccessKeyCache(); - private defaultCredentialProvider?: AWS.CredentialProviderChain; + private readonly defaultAwsAccount: DefaultAWSAccount; + private readonly credentialProviderCache: CredentialProviderCache; constructor(private readonly profile: string | undefined) { // Find the package.json from the main toolkit const pkg = (require.main as any).require('../package.json'); this.userAgent = `${pkg.name}/${pkg.version}`; + + const defaultCredentialProvider = makeCLICompatibleCredentialProvider(profile); + + this.defaultAwsAccount = new DefaultAWSAccount(defaultCredentialProvider); } public async cloudFormation(environment: Environment, mode: Mode): Promise { return new AWS.CloudFormation({ region: environment.region, - credentialProvider: await this.getCredentialProvider(environment.account, mode), + credentialProvider: await this.credentialProviderCache.get(environment.account, mode), customUserAgent: this.userAgent }); } @@ -42,7 +44,7 @@ export class SDK { public async ec2(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { return new AWS.EC2({ region, - credentialProvider: await this.getCredentialProvider(awsAccountId, mode), + credentialProvider: await this.credentialProviderCache.get(awsAccountId, mode), customUserAgent: this.userAgent }); } @@ -50,7 +52,7 @@ export class SDK { public async ssm(awsAccountId: string | undefined, region: string | undefined, mode: Mode): Promise { return new AWS.SSM({ region, - credentialProvider: await this.getCredentialProvider(awsAccountId, mode), + credentialProvider: await this.credentialProviderCache.get(awsAccountId, mode), customUserAgent: this.userAgent }); } @@ -58,7 +60,7 @@ export class SDK { public async s3(environment: Environment, mode: Mode): Promise { return new AWS.S3({ region: environment.region, - credentialProvider: await this.getCredentialProvider(environment.account, mode), + credentialProvider: await this.credentialProviderCache.get(environment.account, mode), customUserAgent: this.userAgent }); } @@ -67,7 +69,86 @@ export class SDK { return await getCLICompatibleDefaultRegion(this.profile); } - public async defaultAccount(): Promise { + public defaultAccount(): Promise { + return this.defaultAwsAccount.get(); + } +} + +/** + * Cache for credential providers. + * + * Given an account and an operating mode (read or write) will return an + * appropriate credential provider for credentials for the given account. The + * credential provider will be cached so that multiple AWS clients for the same + * environment will not make multiple network calls to obtain credentials. + * + * Will use default credentials if they are for the right account; otherwise, + * all loaded credential provider plugins will be tried to obtain credentials + * for the given account. + */ +class CredentialProviderCache { + private readonly cache: {[key: string]: AWS.CredentialProviderChain} = {}; + + public constructor( + private readonly defaultAwsAccount: DefaultAWSAccount, + private readonly defaultCredentialProvider: Promise) { + } + + public async get(awsAccountId: string | undefined, mode: Mode): Promise { + const key = `${awsAccountId}-${mode}`; + if (!(key in this.cache)) { + this.cache[key] = await this.getCredentialProvider(awsAccountId, mode); + } + return this.cache[key]; + } + + private async getCredentialProvider(awsAccountId: string | undefined, mode: Mode): Promise { + // If requested account is undefined or equal to default account, use default credentials provider. + // (Note that we ignore the mode in this case, if you preloaded credentials they better be correct!) + const defaultAccount = await this.defaultAwsAccount.get(); + if (!awsAccountId || awsAccountId === defaultAccount) { + debug(`Using default AWS SDK credentials for account ${awsAccountId}`); + return this.defaultCredentialProvider; + } + + const triedSources: CredentialProviderSource[] = []; + // Otherwise, inspect the various credential sources we have + for (const source of PluginHost.instance.credentialProviderSources) { + if (!(await source.isAvailable())) { + debug('Credentials source %s is not available, ignoring it.', source.name); + continue; + } + triedSources.push(source); + if (!(await source.canProvideCredentials(awsAccountId))) { continue; } + debug(`Using ${source.name} credentials for account ${awsAccountId}`); + return await source.getProvider(awsAccountId, mode); + } + const sourceNames = ['default credentials'].concat(triedSources.map(s => s.name)).join(', '); + throw new Error(`Need to perform AWS calls for account ${awsAccountId}, but no credentials found. Tried: ${sourceNames}.`); + } +} + +/** + * Class to retrieve the account for default credentials and cache it. + * + * Uses the default credentials provider to obtain credentials (if available), + * and uses those credentials to call STS to request the current account ID. + * + * The credentials => accountId lookup is cached on disk, since it's + * guaranteed that igven access key will always remain for the same account. + */ +class DefaultAWSAccount { + private defaultAccountFetched = false; + private defaultAccountId?: string = undefined; + private readonly accountCache = new AccountAccessKeyCache(); + + constructor(private readonly defaultCredentialsProvider: Promise) { + } + + /** + * Return the default account + */ + public async get(): Promise { if (!this.defaultAccountFetched) { this.defaultAccountId = await this.lookupDefaultAccount(); this.defaultAccountFetched = true; @@ -75,13 +156,12 @@ export class SDK { return this.defaultAccountId; } - private async lookupDefaultAccount() { + private async lookupDefaultAccount(): Promise { try { debug('Resolving default credentials'); - if (!this.defaultCredentialProvider) { - this.defaultCredentialProvider = await makeCLICompatibleCredentialProvider(this.profile); - } - const creds = await this.defaultCredentialProvider.resolvePromise(); + const credentialProvider = await this.defaultCredentialsProvider; + const creds = await credentialProvider.resolvePromise(); + const accessKeyId = creds.accessKeyId; if (!accessKeyId) { throw new Error('Unable to resolve AWS credentials (setup with "aws configure")'); @@ -106,35 +186,6 @@ export class SDK { return undefined; } } - - private async getCredentialProvider(awsAccountId: string | undefined, mode: Mode): Promise { - // If requested account is undefined or equal to default account, use default credentials provider. - const defaultAccount = await this.defaultAccount(); - if (!awsAccountId || awsAccountId === defaultAccount) { - debug(`Using default AWS SDK credentials for account ${awsAccountId}`); - return undefined; - } - - const triedSources: CredentialProviderSource[] = []; - - // Otherwise, inspect the various credential sources we have - for (const source of PluginHost.instance.credentialProviderSources) { - if (!(await source.isAvailable())) { - debug('Credentials source %s is not available, ignoring it.', source.name); - continue; - } - triedSources.push(source); - - if (!(await source.canProvideCredentials(awsAccountId))) { continue; } - debug(`Using ${source.name} credentials for account ${awsAccountId}`); - - return await source.getProvider(awsAccountId, mode); - } - - const sourceNames = ['default credentials'].concat(triedSources.map(s => s.name)).join(', '); - - throw new Error(`Need to perform AWS calls for account ${awsAccountId}, but no credentials found. Tried: ${sourceNames}.`); - } } /** From 3a7dac922d7ace5133d93e5af317f61c9110a070 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Aug 2018 16:42:02 +0200 Subject: [PATCH 3/7] Combine key and prefix back into a single parameter --- packages/@aws-cdk/assets/lib/asset.ts | 27 +++---- .../integ.assets.directory.lit.expected.json | 16 ++-- .../test/integ.assets.file.lit.expected.json | 16 ++-- ...integ.assets.permissions.lit.expected.json | 16 ++-- .../test/integ.assets.refs.lit.expected.json | 73 +++++++++++++++++-- packages/@aws-cdk/assets/test/test.asset.ts | 11 ++- packages/@aws-cdk/cx-api/lib/cxapi.ts | 17 +++-- packages/aws-cdk/lib/api/toolkit-info.ts | 9 ++- packages/aws-cdk/lib/api/util/sdk.ts | 1 + packages/aws-cdk/lib/assets.ts | 7 +- 10 files changed, 134 insertions(+), 59 deletions(-) diff --git a/packages/@aws-cdk/assets/lib/asset.ts b/packages/@aws-cdk/assets/lib/asset.ts index d22cc3444d008..92e010de59983 100644 --- a/packages/@aws-cdk/assets/lib/asset.ts +++ b/packages/@aws-cdk/assets/lib/asset.ts @@ -67,7 +67,10 @@ export class Asset extends cdk.Construct { private readonly bucket: s3.BucketRef; - private readonly s3PrefixParam: cdk.Parameter; + /** + * The S3 prefix where all different versions of this asset are stored + */ + private readonly s3Prefix: cdk.Token; constructor(parent: cdk.Construct, id: string, props: GenericAssetProps) { super(parent, id); @@ -86,18 +89,15 @@ export class Asset extends cdk.Construct { description: `S3 bucket for asset "${this.path}"`, }); - this.s3PrefixParam = new cdk.Parameter(this, 'S3Prefix', { - type: 'String', - description: `S3 prefix for asset "${this.path}"` - }); - const keyParam = new cdk.Parameter(this, 'S3VersionKey', { type: 'String', description: `S3 key for asset version "${this.path}"` }); this.s3BucketName = bucketParam.value; - this.s3ObjectKey = keyParam.value; + this.s3Prefix = new cdk.FnSelect(0, new cdk.FnSplit(cxapi.ASSET_PREFIX_SEPARATOR, keyParam.value)); + const s3Filename = new cdk.FnSelect(1, new cdk.FnSplit(cxapi.ASSET_PREFIX_SEPARATOR, keyParam.value)); + this.s3ObjectKey = new cdk.FnConcat(this.s3Prefix, s3Filename); this.bucket = s3.BucketRef.import(parent, 'AssetBucket', { bucketName: this.s3BucketName @@ -106,25 +106,16 @@ export class Asset extends cdk.Construct { // form the s3 URL of the object key this.s3Url = this.bucket.urlForObject(this.s3ObjectKey); - // Get a unique identifier for this asset, which will be used - // as a folder to group different versions of the same asset together. - // - // Even though this code looks horrible, this is actually not a terrible thing - // to do. The generated ID will contain the *stack name* as well, which is a - // perfectly nice thing to do to disambiguate similarly named assets in different stacks. - const uniqueId = new cdk.HashedAddressingScheme().allocateAddress(this.path.split('/')); - // attach metadata to the lambda function which includes information // for tooling to be able to package and upload a directory to the // s3 bucket and plug in the bucket name and key in the correct // parameters. const asset: cxapi.AssetMetadataEntry = { path: this.assetPath, - id: uniqueId, + id: this.uniqueId, packaging: props.packaging, s3BucketParameter: bucketParam.logicalId, s3KeyParameter: keyParam.logicalId, - s3PrefixParameter: this.s3PrefixParam.logicalId }; this.addMetadata(cxapi.ASSET_METADATA, asset); @@ -142,7 +133,7 @@ export class Asset extends cdk.Construct { // different versions of the same file will have the same prefix // and we don't want to accidentally revoke permission on old versions // when deploying a new version. - this.bucket.grantRead(principal, new cdk.FnConcat(this.s3PrefixParam.value, "*")); + this.bucket.grantRead(principal, `${this.s3Prefix}*`); } } diff --git a/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json index 937a3fe810b53..0b84b721fffae 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.directory.lit.expected.json @@ -4,10 +4,6 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-test/SampleAsset\"" }, - "SampleAssetS3Prefix6E89595F": { - "Type": "String", - "Description": "S3 prefix for asset \"aws-cdk-asset-test/SampleAsset\"" - }, "SampleAssetS3VersionKey3E106D34": { "Type": "String", "Description": "S3 key for asset version \"aws-cdk-asset-test/SampleAsset\"" @@ -84,7 +80,17 @@ "", [ { - "Ref": "SampleAssetS3Prefix6E89595F" + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] }, "*" ] diff --git a/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json index 013773a690b8a..c62111674435a 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.file.lit.expected.json @@ -4,10 +4,6 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-file-test/SampleAsset\"" }, - "SampleAssetS3Prefix6E89595F": { - "Type": "String", - "Description": "S3 prefix for asset \"aws-cdk-asset-file-test/SampleAsset\"" - }, "SampleAssetS3VersionKey3E106D34": { "Type": "String", "Description": "S3 key for asset version \"aws-cdk-asset-file-test/SampleAsset\"" @@ -84,7 +80,17 @@ "", [ { - "Ref": "SampleAssetS3Prefix6E89595F" + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] }, "*" ] diff --git a/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json index a98abf17c408e..c39bebcc10145 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.permissions.lit.expected.json @@ -4,10 +4,6 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-refs/MyFile\"" }, - "MyFileS3PrefixF078A585": { - "Type": "String", - "Description": "S3 prefix for asset \"aws-cdk-asset-refs/MyFile\"" - }, "MyFileS3VersionKey568C3C9F": { "Type": "String", "Description": "S3 key for asset version \"aws-cdk-asset-refs/MyFile\"" @@ -84,7 +80,17 @@ "", [ { - "Ref": "MyFileS3PrefixF078A585" + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "MyFileS3VersionKey568C3C9F" + } + ] + } + ] }, "*" ] diff --git a/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json b/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json index dc8769adb0fdc..6f239e31e2a97 100644 --- a/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json +++ b/packages/@aws-cdk/assets/test/integ.assets.refs.lit.expected.json @@ -4,10 +4,6 @@ "Type": "String", "Description": "S3 bucket for asset \"aws-cdk-asset-refs/SampleAsset\"" }, - "SampleAssetS3Prefix6E89595F": { - "Type": "String", - "Description": "S3 prefix for asset \"aws-cdk-asset-refs/SampleAsset\"" - }, "SampleAssetS3VersionKey3E106D34": { "Type": "String", "Description": "S3 key for asset version \"aws-cdk-asset-refs/SampleAsset\"" @@ -24,7 +20,37 @@ }, "S3ObjectKey": { "Value": { - "Ref": "SampleAssetS3VersionKey3E106D34" + "Fn::Join": [ + "", + [ + { + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] + }, + { + "Fn::Select": [ + 1, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] + } + ] + ] }, "Export": { "Name": "aws-cdk-asset-refs:S3ObjectKey" @@ -50,7 +76,30 @@ }, "/", { - "Ref": "SampleAssetS3VersionKey3E106D34" + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] + }, + { + "Fn::Select": [ + 1, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] } ] ] @@ -131,7 +180,17 @@ "", [ { - "Ref": "SampleAssetS3Prefix6E89595F" + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "SampleAssetS3VersionKey3E106D34" + } + ] + } + ] }, "*" ] diff --git a/packages/@aws-cdk/assets/test/test.asset.ts b/packages/@aws-cdk/assets/test/test.asset.ts index 6e8319287ad7c..e78d02c18fa34 100644 --- a/packages/@aws-cdk/assets/test/test.asset.ts +++ b/packages/@aws-cdk/assets/test/test.asset.ts @@ -23,14 +23,12 @@ export = { packaging: 'zip', s3BucketParameter: 'MyAssetS3Bucket68C9B344', s3KeyParameter: 'MyAssetS3VersionKey68E1A45D', - s3PrefixParameter: 'MyAssetS3Prefix5E79A15D', }); // verify that now the template contains parameters for this asset const template = stack.toCloudFormation(); test.equal(template.Parameters.MyAssetS3Bucket68C9B344.Type, 'String'); test.equal(template.Parameters.MyAssetS3VersionKey68E1A45D.Type, 'String'); - test.equal(template.Parameters.MyAssetS3Prefix5E79A15D.Type, 'String'); test.done(); }, @@ -47,14 +45,12 @@ export = { id: 'MyAsset', s3BucketParameter: 'MyAssetS3Bucket68C9B344', s3KeyParameter: 'MyAssetS3VersionKey68E1A45D', - s3PrefixParameter: 'MyAssetS3Prefix5E79A15D', }); // verify that now the template contains parameters for this asset const template = stack.toCloudFormation(); test.equal(template.Parameters.MyAssetS3Bucket68C9B344.Type, 'String'); test.equal(template.Parameters.MyAssetS3VersionKey68E1A45D.Type, 'String'); - test.equal(template.Parameters.MyAssetS3Prefix5E79A15D.Type, 'String'); test.done(); }, @@ -82,8 +78,11 @@ export = { {"Fn::Join": ["", [ "arn", ":", {Ref: "AWS::Partition"}, ":", "s3", ":", "", ":", "", ":", {Ref: "MyAssetS3Bucket68C9B344"}]]}, "/", {"Fn::Join": ["", [ - {Ref: "MyAssetS3Prefix5E79A15D"}, - "*" + {"Fn::Select": [ + 0, + {"Fn::Split": [ "||", { Ref: "MyAssetS3VersionKey68E1A45D"}]} + ]}, + "*" ]]} ]]} ] diff --git a/packages/@aws-cdk/cx-api/lib/cxapi.ts b/packages/@aws-cdk/cx-api/lib/cxapi.ts index 5d0df2d3168e6..2f22a853c03e0 100644 --- a/packages/@aws-cdk/cx-api/lib/cxapi.ts +++ b/packages/@aws-cdk/cx-api/lib/cxapi.ts @@ -133,11 +133,6 @@ export interface AssetMetadataEntry { * Name of parameter where S3 key should be passed in */ s3KeyParameter: string; - - /** - * Name of parameter where S3 folder should be passed in - */ - s3PrefixParameter: string; } /** @@ -154,3 +149,15 @@ export const WARNING_METADATA_KEY = 'aws:cdk:warning'; * Metadata key used to print ERROR-level messages by the toolkit when an app is syntheized. */ export const ERROR_METADATA_KEY = 'aws:cdk:error'; + +/** + * Separator string that separates the prefix separator from the object key separator. + * + * Asset keys will look like: + * + * /assets/MyConstruct12345678/||abcdef12345.zip + * + * This allows us to encode both the prefix and the full location in a single + * CloudFormation Template Parameter. + */ +export const ASSET_PREFIX_SEPARATOR = '||'; \ No newline at end of file diff --git a/packages/aws-cdk/lib/api/toolkit-info.ts b/packages/aws-cdk/lib/api/toolkit-info.ts index ee16e0f13c9f2..68942ba845d3b 100644 --- a/packages/aws-cdk/lib/api/toolkit-info.ts +++ b/packages/aws-cdk/lib/api/toolkit-info.ts @@ -33,7 +33,7 @@ export class ToolkitInfo { s3KeyPrefix?: string, s3KeySuffix?: string, contentType?: string, - }): Promise<{ key: string, changed: boolean }> { + }): Promise<{ filename: string, key: string, changed: boolean }> { const s3 = await this.props.sdk.s3(this.props.environment, Mode.ForWriting); const s3KeyPrefix = props.s3KeyPrefix || ''; @@ -42,13 +42,14 @@ export class ToolkitInfo { const bucket = this.props.bucketName; const hash = md5hash(data); - const key = `${s3KeyPrefix}${hash}${s3KeySuffix}`; + const filename = `${hash}${s3KeySuffix}`; + const key = `${s3KeyPrefix}${filename}`; const url = `s3://${bucket}/${key}`; debug(`${url}: checking if already exists`); if (await objectExists(s3, bucket, key)) { debug(`${url}: found (skipping upload)`); - return { key, changed: false }; + return { filename, key, changed: false }; } debug(`${url}: uploading`); @@ -61,7 +62,7 @@ export class ToolkitInfo { debug(`${url}: upload complete`); - return { key, changed: true }; + return { filename, key, changed: true }; } } diff --git a/packages/aws-cdk/lib/api/util/sdk.ts b/packages/aws-cdk/lib/api/util/sdk.ts index b3e62dbacfd83..a6520d5ee8d08 100644 --- a/packages/aws-cdk/lib/api/util/sdk.ts +++ b/packages/aws-cdk/lib/api/util/sdk.ts @@ -31,6 +31,7 @@ export class SDK { const defaultCredentialProvider = makeCLICompatibleCredentialProvider(profile); this.defaultAwsAccount = new DefaultAWSAccount(defaultCredentialProvider); + this.credentialProviderCache = new CredentialProviderCache(this.defaultAwsAccount, defaultCredentialProvider); } public async cloudFormation(environment: Environment, mode: Mode): Promise { diff --git a/packages/aws-cdk/lib/assets.ts b/packages/aws-cdk/lib/assets.ts index e3c3dbf9d9495..64ada80f0fecd 100644 --- a/packages/aws-cdk/lib/assets.ts +++ b/packages/aws-cdk/lib/assets.ts @@ -1,4 +1,4 @@ -import { ASSET_METADATA, AssetMetadataEntry, StackMetadata, SynthesizedStack } from '@aws-cdk/cx-api'; +import { ASSET_METADATA, ASSET_PREFIX_SEPARATOR, AssetMetadataEntry, StackMetadata, SynthesizedStack } from '@aws-cdk/cx-api'; import { CloudFormation } from 'aws-sdk'; import fs = require('fs-extra'); import path = require('path'); @@ -71,7 +71,7 @@ async function prepareFileAsset( const s3KeyPrefix = `assets/${asset.id}/`; - const { key, changed } = await toolkitInfo.uploadIfChanged(data, { + const { filename, key, changed } = await toolkitInfo.uploadIfChanged(data, { s3KeyPrefix, s3KeySuffix: path.extname(filePath), contentType @@ -86,8 +86,7 @@ async function prepareFileAsset( return [ { ParameterKey: asset.s3BucketParameter, ParameterValue: toolkitInfo.bucketName }, - { ParameterKey: asset.s3KeyParameter, ParameterValue: key }, - { ParameterKey: asset.s3PrefixParameter, ParameterValue: s3KeyPrefix }, + { ParameterKey: asset.s3KeyParameter, ParameterValue: `${s3KeyPrefix}${ASSET_PREFIX_SEPARATOR}${filename}` }, ]; } From 7eb3d714240348bd3756e11b4c9d089150176834 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 16 Aug 2018 17:08:51 +0200 Subject: [PATCH 4/7] Fix Lambda asset test --- packages/@aws-cdk/aws-lambda/test/test.lambda.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/test/test.lambda.ts b/packages/@aws-cdk/aws-lambda/test/test.lambda.ts index 60af9d7dd575b..9c12f4e482579 100644 --- a/packages/@aws-cdk/aws-lambda/test/test.lambda.ts +++ b/packages/@aws-cdk/aws-lambda/test/test.lambda.ts @@ -307,9 +307,10 @@ export = { "S3Bucket": { "Ref": "MyLambdaCodeS3BucketC82A5870" }, - "S3Key": { - "Ref": "MyLambdaCodeS3ObjectKeyA7272AC7" - } + "S3Key": { "Fn::Join": [ "", [ + {"Fn::Select": [0, {"Fn::Split": ["||", {"Ref": "MyLambdaCodeS3VersionKey47762537"}]}]}, + {"Fn::Select": [1, {"Fn::Split": ["||", {"Ref": "MyLambdaCodeS3VersionKey47762537"}]}]}, + ]]} }, "Handler": "index.handler", "Role": { From dacefbb53fe60aa1b54cc62406d718ff0bf6c1b1 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 17 Aug 2018 10:24:27 +0200 Subject: [PATCH 5/7] WIP --- packages/aws-cdk/lib/assets.ts | 3 ++- packages/aws-cdk/test/test.archive.ts | 7 +++--- packages/aws-cdk/test/test.assets.ts | 33 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 packages/aws-cdk/test/test.assets.ts diff --git a/packages/aws-cdk/lib/assets.ts b/packages/aws-cdk/lib/assets.ts index 64ada80f0fecd..93fa8e3c6dd83 100644 --- a/packages/aws-cdk/lib/assets.ts +++ b/packages/aws-cdk/lib/assets.ts @@ -1,6 +1,7 @@ import { ASSET_METADATA, ASSET_PREFIX_SEPARATOR, AssetMetadataEntry, StackMetadata, SynthesizedStack } from '@aws-cdk/cx-api'; import { CloudFormation } from 'aws-sdk'; import fs = require('fs-extra'); +import os = require('os'); import path = require('path'); import { ToolkitInfo } from './api/toolkit-info'; import { zipDirectory } from './archive'; @@ -41,7 +42,7 @@ async function prepareAsset(asset: AssetMetadataEntry, toolkitInfo: ToolkitInfo) async function prepareZipAsset(asset: AssetMetadataEntry, toolkitInfo: ToolkitInfo): Promise { debug('Preparing zip asset from directory:', asset.path); - const staging = await fs.mkdtemp('/tmp/cdk-assets'); + const staging = await fs.mkdtemp(path.join(os.tmpdir(), 'cdk-assets')); try { const archiveFile = path.join(staging, 'archive.zip'); await zipDirectory(asset.path, archiveFile); diff --git a/packages/aws-cdk/test/test.archive.ts b/packages/aws-cdk/test/test.archive.ts index 48bdb9849ae99..4baf24c45a7e4 100644 --- a/packages/aws-cdk/test/test.archive.ts +++ b/packages/aws-cdk/test/test.archive.ts @@ -1,6 +1,7 @@ import { exec as _exec } from 'child_process'; import fs = require('fs-extra'); import { Test } from 'nodeunit'; +import os = require('os'); import path = require('path'); import { promisify } from 'util'; import { md5hash, zipDirectory } from '../lib/archive'; @@ -8,10 +9,10 @@ const exec = promisify(_exec); export = { async 'zipDirectory can take a directory and produce a zip from it'(test: Test) { - const stagingDir = await fs.mkdtemp('/tmp/test.archive'); + const stagingDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive')); const zipFile = path.join(stagingDir, 'output.zip'); const originalDir = path.join(__dirname, 'test-archive'); - const extractDir = await fs.mkdtemp('/tmp/test.archive.extract'); + const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive.extract')); await zipDirectory(originalDir, zipFile); // unzip and verify that the resulting tree is the same @@ -29,7 +30,7 @@ export = { }, async 'md5 hash of a zip stays consistent across invocations'(test: Test) { - const stagingDir = await fs.mkdtemp('/tmp/test.archive'); + const stagingDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive')); const zipFile1 = path.join(stagingDir, 'output.zip'); const zipFile2 = path.join(stagingDir, 'output.zip'); const originalDir = path.join(__dirname, 'test-archive'); diff --git a/packages/aws-cdk/test/test.assets.ts b/packages/aws-cdk/test/test.assets.ts new file mode 100644 index 0000000000000..d5990be969663 --- /dev/null +++ b/packages/aws-cdk/test/test.assets.ts @@ -0,0 +1,33 @@ +import { Test } from 'nodeunit'; +import { prepareAssets } from '../lib/assets'; + +export = { + async 'prepare assets'(test: Test) { + const stack = { + metadata: { + '/SomeStack/SomeResource': { + type: 'aws:cdk:asset', + data: { + path: 'abc.zip', + packaging: 'file', + s3BucketParameter: 'BucketParameter', + s3KeyParameter: 'KeyParameter' + }, + trace: [] + } + }, + template: { + Resources: { + SomeResource: { + Type: 'AWS::Something::Something' + } + } + } + }; + const toolkit = {}; + const params = await prepareAssets(stack, toolkit); + + console.log(params); + test.done(); + } +}; From 24a445cfcae3c3a892906dbe73273a28ad36f53a Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 17 Aug 2018 10:43:34 +0200 Subject: [PATCH 6/7] Add unit test on assets --- packages/aws-cdk/lib/api/toolkit-info.ts | 18 ++++++++--- packages/aws-cdk/test/test.assets.ts | 40 +++++++++++++++++++----- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/packages/aws-cdk/lib/api/toolkit-info.ts b/packages/aws-cdk/lib/api/toolkit-info.ts index 68942ba845d3b..f171d1deed3d6 100644 --- a/packages/aws-cdk/lib/api/toolkit-info.ts +++ b/packages/aws-cdk/lib/api/toolkit-info.ts @@ -8,6 +8,18 @@ import { BUCKET_DOMAIN_NAME_OUTPUT, BUCKET_NAME_OUTPUT } from './bootstrap-envi import { waitForStack } from './util/cloudformation'; import { SDK } from './util/sdk'; +export interface UploadProps { + s3KeyPrefix?: string, + s3KeySuffix?: string, + contentType?: string, +} + +export interface Uploaded { + filename: string; + key: string; + changed: boolean; +} + export class ToolkitInfo { constructor(private readonly props: { sdk: SDK, @@ -29,11 +41,7 @@ export class ToolkitInfo { * Uses md5 hash to render the full key and skips upload if an object * already exists by this key. */ - public async uploadIfChanged(data: any, props: { - s3KeyPrefix?: string, - s3KeySuffix?: string, - contentType?: string, - }): Promise<{ filename: string, key: string, changed: boolean }> { + public async uploadIfChanged(data: any, props: UploadProps): Promise { const s3 = await this.props.sdk.s3(this.props.environment, Mode.ForWriting); const s3KeyPrefix = props.s3KeyPrefix || ''; diff --git a/packages/aws-cdk/test/test.assets.ts b/packages/aws-cdk/test/test.assets.ts index d5990be969663..a270bde14592a 100644 --- a/packages/aws-cdk/test/test.assets.ts +++ b/packages/aws-cdk/test/test.assets.ts @@ -1,20 +1,25 @@ +import { AssetMetadataEntry } from '@aws-cdk/cx-api'; import { Test } from 'nodeunit'; +import { Uploaded, UploadProps } from '../lib'; import { prepareAssets } from '../lib/assets'; export = { async 'prepare assets'(test: Test) { + // GIVEN const stack = { + name: 'SomeStack', metadata: { - '/SomeStack/SomeResource': { + '/SomeStack/SomeResource': [{ type: 'aws:cdk:asset', data: { - path: 'abc.zip', + path: __filename, + id: 'SomeStackSomeResource4567', packaging: 'file', s3BucketParameter: 'BucketParameter', s3KeyParameter: 'KeyParameter' - }, + } as AssetMetadataEntry, trace: [] - } + }] }, template: { Resources: { @@ -24,10 +29,31 @@ export = { } } }; - const toolkit = {}; - const params = await prepareAssets(stack, toolkit); + const toolkit = new FakeToolkit(); + + // WHEN + const params = await prepareAssets(stack, toolkit as any); + + // THEN + test.deepEqual(params, [ + { ParameterKey: 'BucketParameter', ParameterValue: 'bucket' }, + { ParameterKey: 'KeyParameter', ParameterValue: 'assets/SomeStackSomeResource4567/||12345.js' }, + ]); - console.log(params); test.done(); } }; + +class FakeToolkit { + public bucketUrl: string = 'https://bucket'; + public bucketName: string = 'bucket'; + + public async uploadIfChanged(_data: any, props: UploadProps): Promise { + const filename = `12345${props.s3KeySuffix}`; + return { + filename, + changed: true, + key: `${props.s3KeyPrefix}${filename}` + }; + } +} \ No newline at end of file From f88dcb32a3778394d955c1a97ba6e5e0a5a98b65 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 17 Aug 2018 11:13:27 +0200 Subject: [PATCH 7/7] Update integ expectations --- .../test/integ.assets.file.expected.json | 56 +++++++++++++++++-- .../test/integ.assets.lit.expected.json | 56 +++++++++++++++++-- 2 files changed, 104 insertions(+), 8 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/test/integ.assets.file.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.assets.file.expected.json index 07396ecaa6393..8da50c070ca2b 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.assets.file.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.assets.file.expected.json @@ -104,7 +104,25 @@ }, "/", { - "Ref": "MyLambdaCodeS3ObjectKeyA7272AC7" + "Fn::Join": [ + "", + [ + { + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "MyLambdaCodeS3VersionKey47762537" + } + ] + } + ] + }, + "*" + ] + ] } ] ] @@ -130,7 +148,37 @@ "Ref": "MyLambdaCodeS3BucketC82A5870" }, "S3Key": { - "Ref": "MyLambdaCodeS3ObjectKeyA7272AC7" + "Fn::Join": [ + "", + [ + { + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "MyLambdaCodeS3VersionKey47762537" + } + ] + } + ] + }, + { + "Fn::Select": [ + 1, + { + "Fn::Split": [ + "||", + { + "Ref": "MyLambdaCodeS3VersionKey47762537" + } + ] + } + ] + } + ] + ] } }, "Handler": "index.main", @@ -153,9 +201,9 @@ "Type": "String", "Description": "S3 bucket for asset \"lambda-test-assets-file/MyLambda/Code\"" }, - "MyLambdaCodeS3ObjectKeyA7272AC7": { + "MyLambdaCodeS3VersionKey47762537": { "Type": "String", - "Description": "S3 object for asset \"lambda-test-assets-file/MyLambda/Code\"" + "Description": "S3 key for asset version \"lambda-test-assets-file/MyLambda/Code\"" } } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-lambda/test/integ.assets.lit.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.assets.lit.expected.json index 97e93682df598..8c3e4e874b294 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.assets.lit.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.assets.lit.expected.json @@ -104,7 +104,25 @@ }, "/", { - "Ref": "MyLambdaCodeS3ObjectKeyA7272AC7" + "Fn::Join": [ + "", + [ + { + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "MyLambdaCodeS3VersionKey47762537" + } + ] + } + ] + }, + "*" + ] + ] } ] ] @@ -130,7 +148,37 @@ "Ref": "MyLambdaCodeS3BucketC82A5870" }, "S3Key": { - "Ref": "MyLambdaCodeS3ObjectKeyA7272AC7" + "Fn::Join": [ + "", + [ + { + "Fn::Select": [ + 0, + { + "Fn::Split": [ + "||", + { + "Ref": "MyLambdaCodeS3VersionKey47762537" + } + ] + } + ] + }, + { + "Fn::Select": [ + 1, + { + "Fn::Split": [ + "||", + { + "Ref": "MyLambdaCodeS3VersionKey47762537" + } + ] + } + ] + } + ] + ] } }, "Handler": "index.main", @@ -153,9 +201,9 @@ "Type": "String", "Description": "S3 bucket for asset \"lambda-test-assets/MyLambda/Code\"" }, - "MyLambdaCodeS3ObjectKeyA7272AC7": { + "MyLambdaCodeS3VersionKey47762537": { "Type": "String", - "Description": "S3 object for asset \"lambda-test-assets/MyLambda/Code\"" + "Description": "S3 key for asset version \"lambda-test-assets/MyLambda/Code\"" } } } \ No newline at end of file