s3 bucket policy flaps
mburns opened this issue · 5 comments
Describe the Bug
When calling this module and running Terraform apply, the aws_s3_bucket_policy.default[0] created (by a nested call here) adds the following policy block:
+ {
+ Action = "s3:*"
+ Condition = {
+ Bool = {
+ aws:SecureTransport = [
+ "false",
]
}
}
+ Effect = "Deny"
+ Principal = "*"
+ Resource = [
+ "arn:aws:s3:::dev-alb-logs/*",
+ "arn:aws:s3:::dev-alb-logs",
]
+ Sid = "ForceSSLOnlyAccess"
(which is expected)
If that plan is applied and I immediately re-run terraform plan, I get a flapping behavior where it wants to remove that same block:
# [...].module.s3_bucket.aws_s3_bucket.default[0] will be updated in-place
~ resource "aws_s3_bucket" "default" {
...
~ policy = jsonencode(
~ {
~ Statement = [
...
- {
- Action = "s3:*"
- Condition = {
- Bool = {
- aws:SecureTransport = "false"
}
}
- Effect = "Deny"
- Principal = "*"
- Resource = [
- "arn:aws:s3:::dev-alb-logs/*",
- "arn:aws:s3:::dev-alb-logs",
]
- Sid = "ForceSSLOnlyAccess"
},
]
Version = "2012-10-17"
}
)
This is repeatable over multiple terraform apply's.
Expected Behavior
Initial apply should add the ForceSSLOnlyAccess block (if it is not already present) and subsequent runs should keep it in place without trying to delete it.
Steps to Reproduce
I'm calling the latest version of the module as follows:
module "alb_logs" {
source = "git::https://github.com/cloudposse/terraform-aws-lb-s3-bucket.git?ref=tags/0.15.0"
namespace = var.namespace
stage = var.stage
name = "alb-logs"
expiration_days = 180
enable_glacier_transition = false
}
$ terraform -v
Terraform v0.13.7
Apologies if this is the wrong place to file the bug.
I guess my real question is: Should I post up a PR to explicitly set allow_ssl_requests_only = true when calling the s3 module or am I just doing something wrong?
@mburns Yes, please on the PR. This is hitting me also and https://github.com/cloudposse/terraform-aws-config-storage/ seems to allow passing of allow_ssl_requests_only to cloudposse/s3-log-storage/aws which should quiet this flapping.
Sorry, but I'm not sure #51 fixes the issue.
I've just tried forking this module, adding explicit allow_ssl_requests_only = true to the module.s3_bucket (much cruder than what the PR does, but essentially the same thing - I was lazy to pull and apply the whole branch) and the policy still flaps back and forth.
I'm really not sure what's going on there and suspect it might be a bug in the s3-log-storage module (or, rather, s3-bucket module now - because newer versions of s3-log-storage uses it instead). Probably has something to do with the aggregated_policy data source because for some reason it decides it no longer sees the policy:
# module.s3_lb_logs.module.s3_bucket.data.aws_iam_policy_document.aggregated_policy[0] will be read during apply
# (config refers to values not yet known)
<= data "aws_iam_policy_document" "aggregated_policy" {
...
~ json = jsonencode(
{
- Statement = [
- {
- Action = "s3:PutObject"
...
},
- {
- Action = "s3:PutObject"
...
},
- {
- Action = "s3:GetBucketAcl"
...
},
- {
- Action = "s3:*"
- Condition = {
- Bool = {
- aws:SecureTransport = [
- "false",
]
}
}
- Effect = "Deny"
- Principal = "*"
- Resource = [
...
]
- Sid = "ForceSSLOnlyAccess"
},
]
- Version = "2012-10-17"
}
) -> (known after apply)
~ override_json = jsonencode(
{
- Statement = [
- {
- Action = "s3:*"
- Condition = {
- Bool = {
- aws:SecureTransport = "false"
}
}
- Effect = "Deny"
- Principal = "*"
- Resource = [
...
]
- Sid = "ForceSSLOnlyAccess"
},
]
- Version = "2012-10-17"
}
) -> (known after apply)
- version = "2012-10-17" -> null
# (1 unchanged attribute hidden)
}
I found this bug looking for a solve for the bucket policy flapping, but I think the bug is actually related to this line of code in the s3 bucket module: https://github.com/cloudposse/terraform-aws-s3-bucket/blob/master/main.tf#L44
@drdaeman, do you have empty tags passed into the module calling the alb / s3 bucket that's linking to this module?
For me the policy wasn't flapping because of some kind of interpolation change, it was a local map that was applied as tags and tags_all in the s3-bucket module (called by s3-log-storage, which is called by lb-s3-bucket). Some of the keys had blank values. I didn't think the two were related at first, but removing the tags missing the values solved both issues. I think the buckets having tag applied triggered a taint on the the policy because the data source aws_partition in this module would think the s3 bucket needed to be re-evaluated because the tags were changed
My plans included multiple resources each time, including
# module.SOME_MODULE.module.access_logs.module.s3_bucket.module.aws_s3_bucket.aws_s3_bucket.default[0] will be updated in-place
~ resource "aws_s3_bucket" "default" {
id = "super-private-bucket-name"
~ tags = {
+ "map-migrated" = ""
+ "repository" = ""
# (13 unchanged elements hidden)
}
~ tags_all = {
+ "map-migrated" = (known after apply)
+ "repository" = (known after apply)
# (13 unchanged elements hidden)
}
# (10 unchanged attributes hidden)
# <snip>
module.SOME_MODULE.module.access_logs.module.s3_bucket.module.aws_s3_bucket.aws_s3_bucket_policy.default[0] will be updated in-place
~ resource "aws_s3_bucket_policy" "default" {
id = "super-private-bucket-name"
~ policy = jsonencode(
{
- Statement = [
- {
- Action = "s3:*"
- Condition = {
- Bool = {
- "aws:SecureTransport" = [
- "false",
]
}
}
- Effect = "Deny"
- Principal = "*"
- Resource = [
- "arn:aws:s3:::super-private-bucket-name/*",
- "arn:aws:s3:::super-private-bucket-name",
I solved it by modifying the local variable tags in the module calling terraform-aws-lb-s3-bucket to filter out any tags without values at the parent module. I could also see a similar filter working here:
tags = { for k, v in merge(
local.business_tags,
local.technical_tags,
local.automation_tags,
local.security_tags,
local.map_tags,
var.tags
) : k => v if v != "" }
I think I didn't realize that "local variables" make it all the way down to the nested modules because of the child modules being called with context = module.this.context (passing down the local variables)