Missing China regions in elb_service_accounts

bohnjamin commented 11 months ago

Description

PR: https://github.com/terraform-aws-modules/terraform-aws-s3-bucket/pull/264

In release 3.8.2, a fix was made to support newer AWS regions which use a different log delivery policy: https://github.com/terraform-aws-modules/terraform-aws-s3-bucket/commit/3c094b32333a177a07477c4079ef3bd8cc56eea8#diff-dc46acf24afd63ef8c556b77c126ccc6e578bc87e3aa09a931f33d9bf2532fbb

The fix was essentially "if the old region exists in this list, use the old way, otherwise use the new way". Unfortunately, the China regions were left out of this list, so this module treats them as though they're new regions, and sets the Principal to "Service": "logdelivery.elasticloadbalancing.amazonaws.com, when it should be the old format: "AWS": "arn:aws-cn:iam::638102146993:root" (example given is for cn-north-1)

Documentation here describes regions excluding China: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/enable-access-logging.html#attach-bucket-policy

China regions are listed here: https://docs.amazonaws.cn/en_us/elasticloadbalancing/latest/application/enable-access-logging.html#attach-bucket-policy

[x] ✋ I have searched the open/closed issues and my issue is not listed.

Versions

Module version [Required]: 3.15.1 (really, any version since 3.8.2)
Terraform version:
```
Terraform v1.4.5
on darwin_amd64
```
provider registry.terraform.io/hashicorp/aws v5.29.0
provider registry.terraform.io/hashicorp/random v3.6.0
Provider version(s):
```
Terraform v1.4.5
on darwin_amd64
```
provider registry.terraform.io/hashicorp/aws v5.29.0
provider registry.terraform.io/hashicorp/random v3.6.0

Reproduction Code [Required]

Included example code in PR, but pasting the main body here:
provider "aws" {
  region = local.region

  # Make it faster by skipping something
  skip_metadata_api_check     = true
  skip_region_validation      = true
  skip_credentials_validation = true
  skip_requesting_account_id  = true
}

data "aws_availability_zones" "available" {}
data "aws_caller_identity" "current" {}

locals {
  bucket_name = "s3-bucket-${random_pet.this.id}"
  region      = "cn-north-1"
  name        = "china-alb-fail-${random_pet.this.id}"
  vpc_cidr    = "10.0.0.0/16"

  azs = slice(data.aws_availability_zones.available.names, 0, 3)
}

##################################################################
# S3 log bucket
##################################################################

resource "random_pet" "this" {
  length = 2
}

resource "aws_kms_key" "objects" {
  description             = "KMS key is used to encrypt bucket objects"
  deletion_window_in_days = 7
}

resource "aws_iam_role" "this" {
  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "ec2.amazonaws.com"
      },
      "Effect": "Allow",
      "Sid": ""
    }
  ]
}
EOF
}

module "log_bucket" {
  source = "terraform-aws-modules/s3-bucket/aws"
  #  source = "git::https://github.com/bohnjamin/terraform-aws-s3-bucket.git?ref=add-china-regions"

  bucket        = "logs-${random_pet.this.id}"
  force_destroy = true

  control_object_ownership = true

  attach_elb_log_delivery_policy        = true
  attach_lb_log_delivery_policy         = true
  attach_access_log_delivery_policy     = true
  attach_deny_insecure_transport_policy = true
  attach_require_latest_tls_policy      = true

  access_log_delivery_policy_source_accounts = [data.aws_caller_identity.current.account_id]
  access_log_delivery_policy_source_buckets  = ["arn:aws:s3:::${local.bucket_name}"]
}

##################################################################
# Application Load Balancer
##################################################################

module "alb" {
  source = "terraform-aws-modules/alb/aws"

  name    = local.name
  vpc_id  = module.vpc.vpc_id
  subnets = module.vpc.public_subnets

  # For example only
  enable_deletion_protection = false

  # Security Group
  security_group_ingress_rules = {
    all_http = {
      from_port   = 80
      to_port     = 80
      ip_protocol = "tcp"
      description = "HTTP web traffic"
      cidr_ipv4   = "0.0.0.0/0"
    }
  }
  security_group_egress_rules = {
    all = {
      ip_protocol = "-1"
      cidr_ipv4   = "10.0.0.0/16"
    }
  }

  access_logs = {
    bucket = module.log_bucket.s3_bucket_id
  }

  listeners = {
    ex-fixed-response = {
      port     = 89
      protocol = "HTTP"
      fixed_response = {
        content_type = "text/plain"
        message_body = "Fixed message"
        status_code  = "200"
      }
    }
  }

}

################################################################################
# Supporting resources
################################################################################

module "vpc" {
  source  = "terraform-aws-modules/vpc/aws"
  version = "~> 5.0"

  name = local.name
  cidr = local.vpc_cidr

  azs             = local.azs
  private_subnets = [for k, v in local.azs : cidrsubnet(local.vpc_cidr, 4, k)]
  public_subnets  = [for k, v in local.azs : cidrsubnet(local.vpc_cidr, 8, k + 48)]

}

Steps to reproduce the behavior: Create an ELB log bucket in S3 in an AWS China region, with attach_elb_log_delivery_policy and attach_lb_log_delivery_policy set to true, then create an ALB and set access_logs to your bucket ID.

Expected behavior

bucket should get created with correct permissions, and the ALB should be able to write to it

Actual behavior

The incorrect principal gets set in the bucket permissions, and the log delivery fails. This causes terraform to fail with an error like this:

│ Error: failure configuring LB attributes: InvalidConfigurationRequest: Access Denied for bucket: logs-wanted-mule. Please check S3bucket permission
│   status code: 400, request id: <redacted>
│
│   with module.alb.aws_lb.this[0],
│   on .terraform/modules/alb/main.tf line 12, in resource "aws_lb" "this":
│   12: resource "aws_lb" "this" {

Terminal Output Screenshot(s)

Additional context

Submitted PR with example code here: https://github.com/terraform-aws-modules/terraform-aws-s3-bucket/pull/264

github-actions[bot] commented 10 months ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

antonbabenko commented 10 months ago

Fixed by #264

github-actions[bot] commented 9 months ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

terraform-aws-modules / terraform-aws-s3-bucket