Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Depend on bucket and policy before configuring ELB logging #1633

Closed
schof opened this issue Jan 29, 2019 · 4 comments · Fixed by #2221 · May be fixed by MechanicalRock/account-reaper#6
Closed

Depend on bucket and policy before configuring ELB logging #1633

schof opened this issue Jan 29, 2019 · 4 comments · Fixed by #2221 · May be fixed by MechanicalRock/account-reaper#6
Assignees
Labels
@aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing bug This issue is a bug. effort/small Small work item – less than a day of effort pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason.

Comments

@schof
Copy link
Contributor

schof commented Jan 29, 2019

I tried adding logging support to my VPC using the following:

    const alb = new Alb.ApplicationLoadBalancer(this, 'LB', {
      vpc,
      internetFacing: true
    })

    const bucket = new Bucket(this, 'Bucket', {
      encryption: BucketEncryption.KmsManaged
    })
    alb.logAccessLogs(bucket)

When running cdk deploy I got an expected warning about IAM changes and the permission looks to be correct.

+ │ ${Bucket.Arn}/* │ Allow │ s3:PutObject │ AWS:arn:${AWS::Partition}:iam::127311923021:root

It appears that there is a bug in the CF template which is not waiting on the bucket policy to finish completion before it attempts to add the logging in the VPC.

Access Denied for bucket: [BUCKET NAME]. Please check S3bucket permission (Service: AmazonElasticLoadBalancingV2; Status Code: 400; Error Code: InvalidConfigurationRequest; Request ID: 657ff61c-23dd-11e9-94e9-c57251c19c33)

I confirmed this was the case by checking the CF events in the console.

@schof schof changed the title Incorrect permissions added for ELB logging to S3 Wait for bucket policy to be completed before configuring ELB logging Jan 29, 2019
@schof schof changed the title Wait for bucket policy to be completed before configuring ELB logging Wait for bucket policy to complete before configuring ELB logging Jan 29, 2019
@schof schof changed the title Wait for bucket policy to complete before configuring ELB logging Depend on bucket and policy before configuring ELB logging Jan 29, 2019
@schof
Copy link
Contributor Author

schof commented Jan 29, 2019

I found a temporary workaround but it's not perfect since I couldn't figure out a way to add DependsOn for the BucketPolicy that is also being created.

// Temporary Hack (https://github.com/awslabs/aws-cdk/issues/1633)
const albResource = alb.node.findChild('Resource') as Alb.CfnLoadBalancer
const bucketResource = bucket.node.findChild('Resource') as cdk.Resource
albResource.addDependency(bucketResource)

It only works because the ALB takes so long to create (from scratch) that the bucket stuff is generally done in time. I tried working up a fix but I ran into some difficulties with test-region and my general unfamiliarity with the source.

@RomainMuller RomainMuller added bug This issue is a bug. @aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing labels Jan 30, 2019
@RomainMuller
Copy link
Contributor

This will be much easier to fix (and much cleaner, too) once #1583 has landed. At this point we'll be able to simply register the dependency properly in the logAccesLogs method.

@RomainMuller RomainMuller added the pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason. label Jan 30, 2019
@eladb eladb self-assigned this Apr 2, 2019
eladb pushed a commit that referenced this issue Apr 10, 2019
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
eladb pushed a commit that referenced this issue Apr 11, 2019
#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
@fulghum fulghum added the effort/small Small work item – less than a day of effort label Apr 15, 2019
@gbooth27
Copy link

I just ran into this, any ETA on a fix?

@pepito-j
Copy link

pepito-j commented Oct 11, 2023

Hey team. Wanted to chime in as I think I found an issue with how the dependency is being placed. If the ApplicationLoadBalancer Construct gets extended and the user configures bucket access logs, this is going to create a circular dependency because of this line:

The dependency should be written as this.node.defaultChild!.node.addDependency(bucket) instead since I think we're only concerned with the CfnLoadBalancer depending on the L2 Bucket. Otherwise, we can maybe have the CfnLoadBalancer directly depend on the CfnBucketPolicy since the reference is already implied between the CfnLoadBalancer and the CfnBucket.

The parent ALB, if extended, will end up depending on it's own children which will then trigger the children to depend on one another. For example:

export class CustomLB extends ApplicationLoadBalancer {

  constructor(scope: Construct, id: string, vpc: cdk.aws_ec2.IVpc) {
    super(scope, id, { vpc });

    const accessLogsBucket = new Bucket(this, `ALBAccessLogsBucket`, {      
      blockPublicAccess: BlockPublicAccess.BLOCK_ALL,
      encryption: BucketEncryption.S3_MANAGED,
      versioned: true,
      serverAccessLogsPrefix: 'selflog/',
      enforceSSL: true,
    });

    this.logAccessLogs(accessLogsBucket);
  }
}

Returns the following dependencies:

 "Resources": {
  "ALBAEE750D2": {
   "Type": "AWS::ElasticLoadBalancingV2::LoadBalancer",
   "Properties": {...},
   "DependsOn": [
    "ALBALBAccessLogsBucketPolicy05AC83C7",
    "ALBALBAccessLogsBucket01758B82"
   ],
   "Metadata": {
    "aws:cdk:path": "Stack/ALB/Resource"
   }
  },
  "ALBALBAccessLogsBucket01758B82": {
   "Type": "AWS::S3::Bucket",
   "Properties": {...},
   "DependsOn": [
    "ALBALBAccessLogsBucketPolicy05AC83C7" // <----- Circular Dependency. BucketPolicy already references Bucket
   ],
   "UpdateReplacePolicy": "Retain",
   "DeletionPolicy": "Retain",
   "Metadata": {
    "aws:cdk:path": "Stack/ALB/ALBAccessLogsBucket/Resource"
   }
  },
  "ALBALBAccessLogsBucketPolicy05AC83C7": {
   "Type": "AWS::S3::BucketPolicy",
   "Properties": {
    "Bucket": {
     "Ref": "ALBALBAccessLogsBucket01758B82"
    },
    "PolicyDocument": {...}
   },
   "DependsOn": [
    "ALBALBAccessLogsBucket01758B82"
   ],
   "Metadata": {
    "aws:cdk:path": "Stack/ALB/ALBAccessLogsBucket/Policy/Resource"
   }
  },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing bug This issue is a bug. effort/small Small work item – less than a day of effort pr/blocked This PR cannot be merged or reviewed, because it is blocked for some reason.
Projects
None yet
6 participants