Consider Removing Access Logs Module

[ekristen]

what

• Removed access_logs module

why

• Disabling the access_logs module by supplying existing bucket_id results in errors from terraform and count for the alb_service_account not knowing how many resources to create because terraform hasn't been created yet.
• This makes the module more simple while allowing someone to easily spin up the same module code at the same level as the alb module and pass in the bucket information

important

• this could be considered a breaking change and necessary to rev the major, however it wouldn't be difficult to migrate the module state up one level as an upgrade path

example

module "access_logs" {
  source                             = "cloudposse/lb-s3-bucket/aws"
  version                            = "0.14.1"
  enabled                            = module.this.enabled && var.access_logs_enabled && var.access_logs_s3_bucket_id == null
  attributes                         = compact(concat(module.this.attributes, ["alb", "access", "logs"]))
  lifecycle_rule_enabled             = var.lifecycle_rule_enabled
  enable_glacier_transition          = var.enable_glacier_transition
  expiration_days                    = var.expiration_days
  glacier_transition_days            = var.glacier_transition_days
  noncurrent_version_expiration_days = var.noncurrent_version_expiration_days
  noncurrent_version_transition_days = var.noncurrent_version_transition_days
  standard_transition_days           = var.standard_transition_days
  force_destroy                      = var.alb_access_logs_s3_bucket_force_destroy
  context                            = module.this.context
}

module "alb" {
    source = "cloudposse/alb/aws"
    # Cloud Posse recommends pinning every module to a specific version
    # version     = "x.x.x"
    namespace                               = var.namespace
    stage                                   = var.stage
    name                                    = var.name
    attributes                              = var.attributes
    delimiter                               = var.delimiter
    vpc_id                                  = module.vpc.vpc_id
    security_group_ids                      = [module.vpc.vpc_default_security_group_id]
    subnet_ids                              = module.subnets.public_subnet_ids
    internal                                = var.internal
    http_enabled                            = var.http_enabled
    http_redirect                           = var.http_redirect
    access_logs_enabled                     = var.access_logs_enabled
    access_logs_s3_bucket_id                = module.access_logs.bucket_id
    cross_zone_load_balancing_enabled       = var.cross_zone_load_balancing_enabled
    http2_enabled                           = var.http2_enabled
    idle_timeout                            = var.idle_timeout
    ip_address_type                         = var.ip_address_type
    deletion_protection_enabled             = var.deletion_protection_enabled
    deregistration_delay                    = var.deregistration_delay
    health_check_path                       = var.health_check_path
    health_check_timeout                    = var.health_check_timeout
    health_check_healthy_threshold          = var.health_check_healthy_threshold
    health_check_unhealthy_threshold        = var.health_check_unhealthy_threshold
    health_check_interval                   = var.health_check_interval
    health_check_matcher                    = var.health_check_matcher
    target_group_port                       = var.target_group_port
    target_group_target_type                = var.target_group_target_type
    stickiness                              = var.stickiness
    tags                                    = var.tags
  }

[bridgecrew]

Bridgecrew has found infrastructure configuration errors in this PR ⬇️

[bridgecrew]

  Ensure ALB redirects HTTP requests into HTTPS ones
    Resource: aws_lb.default | ID: BC_AWS_NETWORKING_49

How to Fix

resource "aws_lb" "lb_good" {
}


resource "aws_lb_listener" "listener_good" {
  load_balancer_arn = aws_lb.lb_good.arn
  port              = "80"
  protocol          = "HTTP"

  default_action {
    type = "redirect"

    redirect {
      port        = "443"
      protocol    = "HTTPS"
      status_code = "HTTP_301"
    }

  }
}

Description

TBA

Dependent Resources

Path	Resource	Connecting Attribute
/main.tf	aws_lb_listener.http_forward	load_balancer_arn
/main.tf	aws_lb_listener.http_redirect	load_balancer_arn
/main.tf	aws_lb_listener.https	load_balancer_arn

[bridgecrew]

  Ensure public facing ALB are protected by AWS Web Application Firewall v2 (AWS WAFv2)
    Resource: aws_lb.default | ID: BC_AWS_NETWORKING_58

Description

TBD Dependent Resources

Path	Resource	Connecting Attribute
/main.tf	aws_lb_listener.http_forward	load_balancer_arn
/main.tf	aws_lb_listener.http_redirect	load_balancer_arn
/main.tf	aws_lb_listener.https	load_balancer_arn

[mergify]

This pull request is now in conflict. Could you fix it @ekristen? 🙏

[Gowiem]

@ekristen while I agree, this probably should have been built with composition in mind instead of inheritence... I think we're probably stuck with this for now and it's not worth making the breaking change. I think the thing to fix would be this:

Disabling the access_logs module by supplying existing bucket_id results in errors from terraform and count for the alb_service_account not knowing how many resources to create because terraform hasn't been created yet.

Are you still interested in this update? Want to discuss what fixing that looks like and take a crack at that? If not, no worries, but I think we will close this PR since it has sat open and untouched for a while without much interest sadly.