From 99e085dbdb27ac7a32d8512ef340652849dd5f5c Mon Sep 17 00:00:00 2001
From: Elad Ben-Israel <benisrae@amazon.com>
Date: Thu, 11 Apr 2019 09:15:34 +0300
Subject: [PATCH] fix(elasticloadbalancingv2): dependency between ALB and
 logging bucket (#2221)

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      | 10 ++--
 .../test/alb/test.load-balancer.ts            | 58 ++++++++++++++++++-
 2 files changed, 62 insertions(+), 6 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..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,11 +88,11 @@ 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 || '', '*')));
+    prefix = prefix || '';
+    bucket.grantPut(new iam.AccountPrincipal(account), prefix + '*');
+
+    // 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 7b474d1fb7bc8..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
@@ -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,58 @@ 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();
+  },
+
+  '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();
   },