From c6ddd3c625e0e4a9ddb974521b6e0ddb75a56748 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Wed, 10 Apr 2019 17:07:41 +0300 Subject: [PATCH 1/2] fix(elasticloadbalancingv2): dependency between ALB and logging bucket When access logs are enabled for an ALB, a dependency is added between the ALB and the logging bucket and it's policy to avoid a race condition where the ALB can't access the bucket. Fixes #1633 --- .../lib/alb/application-load-balancer.ts | 9 ++++----- .../test/alb/test.load-balancer.ts | 13 +++++++++++-- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts index f0d00b6109c7f..275679f7eb358 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts @@ -88,11 +88,10 @@ export class ApplicationLoadBalancer extends BaseLoadBalancer implements IApplic throw new Error(`Cannot enable access logging; don't know ELBv2 account for region ${region}`); } - // FIXME: can't use grantPut() here because that only takes IAM objects, not arbitrary principals - bucket.addToResourcePolicy(new iam.PolicyStatement() - .addPrincipal(new iam.AccountPrincipal(account)) - .addAction('s3:PutObject') - .addResource(bucket.arnForObjects(prefix || '', '*'))); + bucket.grantPut(new iam.AccountPrincipal(account)); + + // make sure the bucket is created before the ALB + this.node.addDependency(bucket); } /** diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts index 7b474d1fb7bc8..967f77eec5171 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts @@ -1,4 +1,4 @@ -import { expect, haveResource, ResourcePart } from '@aws-cdk/assert'; +import { expect, haveResource, ResourcePart, haveResourceLike } from '@aws-cdk/assert'; import ec2 = require('@aws-cdk/aws-ec2'); import s3 = require('@aws-cdk/aws-s3'); import cdk = require('@aws-cdk/cdk'); @@ -122,6 +122,8 @@ export = { lb.logAccessLogs(bucket); // THEN + + // verify that the LB attributes reference the bucket expect(stack).to(haveResource('AWS::ElasticLoadBalancingV2::LoadBalancer', { LoadBalancerAttributes: [ { @@ -134,12 +136,14 @@ export = { } ], })); + + // verify the bucket policy allows the ALB to put objects in the bucket expect(stack).to(haveResource('AWS::S3::BucketPolicy', { PolicyDocument: { Version: '2012-10-17', Statement: [ { - Action: "s3:PutObject", + Action: [ "s3:PutObject*", "s3:Abort*" ], Effect: 'Allow', Principal: { AWS: { "Fn::Join": [ "", [ "arn:", { Ref: "AWS::Partition" }, ":iam::127311923021:root" ] ] } }, Resource: { "Fn::Join": [ "", [ { "Fn::GetAtt": [ "AccessLoggingBucketA6D88F29", "Arn" ] }, "/*" ] ] } @@ -148,6 +152,11 @@ export = { } })); + // verify the ALB depends on the bucket *and* the bucket policy + expect(stack).to(haveResource('AWS::ElasticLoadBalancingV2::LoadBalancer', { + DependsOn: [ 'AccessLoggingBucketPolicy700D7CC6', 'AccessLoggingBucketA6D88F29' ] + }, ResourcePart.CompleteDefinition)); + test.done(); }, From 4e5f47155f16832ea8bb67c75e0b917ae4e719f5 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Thu, 11 Apr 2019 00:13:51 +0300 Subject: [PATCH 2/2] CR fixes --- .../lib/alb/application-load-balancer.ts | 5 +- .../test/alb/test.load-balancer.ts | 49 ++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts index 275679f7eb358..62e22c728243d 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts @@ -88,9 +88,10 @@ export class ApplicationLoadBalancer extends BaseLoadBalancer implements IApplic throw new Error(`Cannot enable access logging; don't know ELBv2 account for region ${region}`); } - bucket.grantPut(new iam.AccountPrincipal(account)); + prefix = prefix || ''; + bucket.grantPut(new iam.AccountPrincipal(account), prefix + '*'); - // make sure the bucket is created before the ALB + // make sure the bucket's policy is created before the ALB (see https://github.com/awslabs/aws-cdk/issues/1633) this.node.addDependency(bucket); } diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts index 967f77eec5171..af42eff905686 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.load-balancer.ts @@ -1,4 +1,4 @@ -import { expect, haveResource, ResourcePart, haveResourceLike } from '@aws-cdk/assert'; +import { expect, haveResource, ResourcePart } from '@aws-cdk/assert'; import ec2 = require('@aws-cdk/aws-ec2'); import s3 = require('@aws-cdk/aws-s3'); import cdk = require('@aws-cdk/cdk'); @@ -160,6 +160,53 @@ export = { test.done(); }, + 'access logging with prefix'(test: Test) { + // GIVEN + const stack = new cdk.Stack(undefined, undefined, { env: { region: 'us-east-1' }}); + const vpc = new ec2.VpcNetwork(stack, 'Stack'); + const bucket = new s3.Bucket(stack, 'AccessLoggingBucket'); + const lb = new elbv2.ApplicationLoadBalancer(stack, 'LB', { vpc }); + + // WHEN + lb.logAccessLogs(bucket, 'prefix-of-access-logs'); + + // THEN + // verify that the LB attributes reference the bucket + expect(stack).to(haveResource('AWS::ElasticLoadBalancingV2::LoadBalancer', { + LoadBalancerAttributes: [ + { + Key: "access_logs.s3.enabled", + Value: "true" + }, + { + Key: "access_logs.s3.bucket", + Value: { Ref: "AccessLoggingBucketA6D88F29" } + }, + { + Key: "access_logs.s3.prefix", + Value: "prefix-of-access-logs" + } + ], + })); + + // verify the bucket policy allows the ALB to put objects in the bucket + expect(stack).to(haveResource('AWS::S3::BucketPolicy', { + PolicyDocument: { + Version: '2012-10-17', + Statement: [ + { + Action: [ "s3:PutObject*", "s3:Abort*" ], + Effect: 'Allow', + Principal: { AWS: { "Fn::Join": [ "", [ "arn:", { Ref: "AWS::Partition" }, ":iam::127311923021:root" ] ] } }, + Resource: { "Fn::Join": [ "", [ { "Fn::GetAtt": [ "AccessLoggingBucketA6D88F29", "Arn" ] }, "/prefix-of-access-logs*" ] ] } + } + ] + } + })); + + test.done(); + }, + 'Exercise metrics'(test: Test) { // GIVEN const stack = new cdk.Stack();