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

feat: Creating SNS/SQS policies should be optional #54

Merged
merged 5 commits into from
Oct 30, 2020
Merged

feat: Creating SNS/SQS policies should be optional #54

merged 5 commits into from
Oct 30, 2020

Conversation

bmurphey
Copy link
Contributor

Description

Make the creation of SNS/SQS policies conditional based on two new variables, create_sns_policy and create_sqs_policy.

Motivation and Context

We have a use case (as I'm sure others do) where we need to send S3 bucket notifications to an external account that we don't control, and so we can't create the related SNS/SQS policies in that account (and they aren't necessary). The rest of the notification setup is valid when using the external resource ARNs, however.

Breaking Changes

None; the variables default to true so behavior will be the same.

How Has This Been Tested?

$ terraform -v
Terraform v0.12.29
+ provider.aws v3.10.0
+ provider.null v2.1.2
+ provider.random v2.3.0

Using the module as published failed to apply the notification due to the inability create the policy, so I added it manually and imported it. This snippet...:

module "redacted-bucket" {
  source  = "terraform-aws-modules/s3-bucket/aws"
  version = "v1.15.0"

  bucket        = "redacted-bucket"
  acl           = "private"
  force_destroy = false

  tags = {
    creator     = "terraform"
  }

  versioning = {
    enabled = false
  }

  # S3 bucket-level Public Access Block configuration
  block_public_acls       = true
  block_public_policy     = true
  ignore_public_acls      = true
  restrict_public_buckets = true
}

module "redacted-bucket-notifications" {
  source  = "terraform-aws-modules/s3-bucket/aws//modules/notification"

  bucket            = module.redacted-bucket.this_s3_bucket_id

  # Common error - Error putting S3 notification configuration: InvalidArgument: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the same event type.

  sqs_notifications = {
    sqs1 = {
      queue_arn     = "redacted-queue-arn"
      events        = ["s3:ObjectCreated:Put"]
      filter_suffix = ".gz"
    }
  }
}

... results in this plan:

Terraform will perform the following actions:

  # module.redacted-bucket-notifications.aws_sqs_queue_policy.allow["sqs1"] will be created
  + resource "aws_sqs_queue_policy" "allow" {
      + id        = (known after apply)
      + policy    = jsonencode(
            {
              + Statement = [
                  + {
                      + Action    = "sqs:SendMessage"
                      + Condition = {
                          + ArnEquals = {
                              + aws:SourceArn = "arn:aws:s3:::redacted-bucket"
                            }
                        }
                      + Effect    = "Allow"
                      + Principal = {
                          + Service = "s3.amazonaws.com"
                        }
                      + Resource  = "redacted-queue-arn"
                      + Sid       = "AllowSQSS3BucketNotification"
                    },
                ]
              + Version   = "2012-10-17"
            }
        )
      + queue_url = "redacted-queue-url"
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Changing the module to use this branch like so...:

module "redacted-bucket-notifications" {
  source  = "github.com/bmurphey/terraform-aws-s3-bucket//modules/notification?ref=optional-create-policy"

  bucket                   = module.redacted-bucket.this_s3_bucket_id
  create_sqs_policy = false

  # Common error - Error putting S3 notification configuration: InvalidArgument: Configuration is ambiguously defined. Cannot have overlapping suffixes in two rules if the prefixes are overlapping for the same event type.

  sqs_notifications = {
    sqs1 = {
      queue_arn     = "redacted-queue-arn"
      events        = ["s3:ObjectCreated:Put"]
      filter_suffix = ".gz"
    }
  }
}

... results in No changes. Infrastructure is up-to-date.. It appears to me that the SNS policy is created in the same manner, so I added a variable to make that policy creation conditional as well, but that hasn't been tested.

In cases where the bucket notifications are being sent to an external
account that we don't control, we are unable (and it is unnecessary) to
create IAM policies to attach to SQS/SNS.
@bmurphey bmurphey changed the title Creating SNS/SQS policies should be optional feat: Creating SNS/SQS policies should be optional Oct 28, 2020
@antonbabenko antonbabenko merged commit 5312d97 into terraform-aws-modules:master Oct 30, 2020
@antonbabenko
Copy link
Member

Thanks for this addition!

v1.16.0 has been just released.

@bmurphey
Copy link
Contributor Author

No problem, thank you!

@bmurphey bmurphey deleted the optional-create-policy branch October 30, 2020 15:17
@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants