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

feature/kms-optional: Making Encryption configuration of SNS less constraining (also to opt out) #32

Closed
wants to merge 79 commits into from

Conversation

azec-pdx
Copy link
Contributor

@azec-pdx azec-pdx commented May 12, 2021

what

  • I was using cloudposse/terraform-aws-sns-topic to deploy SNS Topic and subscriber SQS queues for routing Bounce and Complaint notifications from AWS SES service. AWS SES won't accept SNS Topic as the notifications target unless it has enough permissions for KMS key that is configured for SNS Topic Encryption settings. With module cloudposse/terraform-aws-sns-topic using default AWS KMS key alias/aws/sns, this is limiting in two ways:
    1. It forces users of cloudposse/terraform-aws-sns-topic TF module to use encryption even if they don't provide their own KMS key. Users don't have option to deploy SNS Topic with Encryption disabled.
    2. Since users are already forced to use SNS Topic Encryption, their only option becomes to configure their own KMS key and pass it to cloudposse/terraform-aws-sns-topic module, but then there is additional requirement for users to do more IAM permissions on KMS key to allow AWS SES service access to the key to encrypt messages as they are fanned out to SNS Topic.
  • This PR makes an Encryption settings of SNS Topic deployed with cloudposse/terraform-aws-sns-topic optional by providing users with some better choices:
    1. To use their own KMS key if they really want fully customized Encryption configuration of SNS Topic
    2. To opt-in to use default AWS SNS KMS key alias/aws/sns
    3. To have Encryption disabled on SNS Topic by default if they don't put effort into 1. and 2.

why

  • Business case: mostly described above, to be able to have better defaults when needing SNS Topic with Encryption disabled in order to work with other AWS service (SES in this case) - especially since it is all in the same AWS account.

  • The PR uses cascading logic for configuring Encryption settings of SNS Topic resource. By introducing use_default_aws_sns_kms_master_key_id, and defaulting kms_master_key_id to "", following cascading logic is applied:

    • If use_default_aws_sns_kms_master_key_id is set explicitly to true, no other variables are analyzed. In this case Encryption becomes enabled on SNS Topic and default KMS key alias/aws/sns is used in the Encryption configuration.
    • If use_default_aws_sns_kms_master_key_id is not set, then variable kms_master_key_id is analyzed (see below subcases).
      • If kms_master_key_id is set to explicit non-empty value, the Encryption becomes enabled on SNS Topic and user-specified KMS key is used in the Encryption configuration.
      • If kms_master_key_id is not passed, then module default value "" is passed to aws_sns_topic resource which then behaves in a way that Encryption on SNS Topic becomes disabled (this was tested and verified).
  • This now makes possible to deploy SNS Topic without Encryption by being totally lazy with something like:

     module "sns" {
        #source = "git::https://github.com/cloudposse/terraform-aws-sns-topic.git//?ref=tags/0.16.0"
        source = "git::https://github.com/SkywardIO/terraform-aws-sns-topic.git//?ref=feature/kms-optional"
        name   = module.this.id
    
        subscribers                            = var.subscribers
        allowed_aws_services_for_sns_published = var.allowed_aws_services_for_sns_published
        sqs_dlq_enabled                        = false # Until valid case neeed for this and until CP fixes https://github.com/cloudposse/terraform-aws-sns-topic/blob/master/main.tf#L20
    }

references

  • N/A?

Maxim Mironenko and others added 30 commits January 15, 2019 13:14
Initial Implementation of `terraform-aws-s3-bucket` module
* Use splat + join synatx on s3_bucket arn

As we are using a count on this resource and are currently getting
the following warning message:

```
Warning: output "s3_bucket_arn": must use splat syntax to access aws_s3_bucket.default attribute "arn", because it has "count" set; use aws_s3_bucket.default.*.arn to obtain a list of the attributes across all instances
```

* docs fix for lint check to pass
* added policy to allow only encrypted uploads
* Convert to TF 0.12. Add tests. Add Codefresh test pipeline

* Update outputs
* adding dynamic to allow for other expiration types for non versioned buckets

* adding support for abort_incomplete_multipart_upload_days

* Fixing tags in lifecycle when multipart upload is enabled

Co-authored-by: PePe (Jose) Amengual <jamengual@sonatype.com>
* adding dynamic to allow for other expiration types for non versioned buckets

* adding support for abort_incomplete_multipart_upload_days

* Fixing tags in lifecycle when multipart upload is enabled

* Adding option to disable any transition

* Adding option to disable any transition

* Fixing condition for transition days

* Update main.tf

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Update main.tf

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Re-adding deleted trasition dynamic

Co-authored-by: PePe (Jose) Amengual <jamengual@sonatype.com>
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
* Add the ability to specify cors-rules

* Update readme, description

Co-authored-by: Maxim Mironenko <maxim@cloudposse.com>
* add workflow

* fix repo

* change image

* run docker iamge

* fix yaml

* login then test

* login then test

* pull private image

* use dockerhub

* init

* fix workflow rebuild readme

* checkout code

* add all tests

* refactor

* set default

* annotate workflow

* pass secrets

* annotate workflow

* Fix readme

* parameterize test image

* refactor into multiple jobs

* refactor and restore linting

* fix templates

* Update .github/workflows/slash-command-dispatch.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Update .github/workflows/slash-command-dispatch.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Update .github/workflows/test.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Update .github/workflows/test.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Update .github/workflows/test.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
* add chatops

* add reaction

* Update .github/workflows/chatops.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

* Update .github/workflows/chatops.yml

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>

Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
* drop reaction

* also build on master

* fix reaction
* run terratest only when secrets exist

* remove conditional
Co-authored-by: Erik Osterman <erik@cloudposse.com>
* use github status updater

* workaround

* use rusn syntax

* use with

* use with

* use with

* update sha

* use arg

* hack the context

* hack the context

* dispatch commands to actions

* move reactions to upstream
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Co-authored-by: Erik Osterman <erik@cloudposse.com>
…#27)

* Optionally allow public access to the bucket, default is now to block

* Add depends_on to block a race condition in policy application
* feat(grants): support S3 grants

* docs: tweaked the grants description

* chore: remove TF_DATA_DIR for parallel testing

* chore: use temp folder for terratest

* feat(test): update testing approach

* chore: remove test data dir

* chore: test fixes

* docs: added ACL examples

* chore(test): turn off parallel tests

* chore: text updates

* chore: support attributes in the examples

* chore: fix test expects

* chore: test expect fix
* Fix s3 user tagging

* Updated README.md

* Updated README.md

Co-authored-by: actions-bot <58130806+actions-bot@users.noreply.github.com>
## What

1. Update Version Pinning for Terraform to support 0.13

## Why

1. This is a relatively minor update that the CloudPosse module already likely supports.
1. This allows module consumers to not individually update our Terraform module to support Terraform 0.13.
* Update main.tf

* Updated README.md

Co-authored-by: actions-bot <58130806+actions-bot@users.noreply.github.com>
* Remove hardcoded arn partition information

This change will allow us to seamlessly support other AWS partitions
such as aws-us-gov.

* Updated README.md

Co-authored-by: actions-bot <58130806+actions-bot@users.noreply.github.com>
@azec-pdx azec-pdx closed this Jul 16, 2021
@azec-pdx
Copy link
Contributor Author

Reopened as #34 after some rebasing issues of our fork ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.