Add KMS encryption support to the aws_athena_database resource.

[joestump]

What does this add?

Adds an encryption_key attribute to the aws_athena_database resource. This allows TF to create databases that read from encrypted S3 buckets.

Example HCL

resource "aws_kms_key" "test" {
  name = "athenta-test"
}

resource "aws_athena_database" "test" {
  name          = "athena-test"
  bucket        = "${aws_s3_bucket.test.bucket}"
  force_destroy = true

  encryption_key {
    type = "SSE_KMS"
    id   = "${aws_kms_key.test.arn}" 
  }
}

Test output

make testacc TESTARGS='-run=TestAccAWSAthenaDatabase'
TF_ACC=1 go test ./... -v -run=TestAccAWSAthenaDatabase -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAthenaDatabase_basic
--- PASS: TestAccAWSAthenaDatabase_basic (49.53s)
=== RUN   TestAccAWSAthenaDatabase_encryption
--- PASS: TestAccAWSAthenaDatabase_encryption (79.31s)
=== RUN   TestAccAWSAthenaDatabase_nameStartsWithUnderscore
--- PASS: TestAccAWSAthenaDatabase_nameStartsWithUnderscore (49.75s)
=== RUN   TestAccAWSAthenaDatabase_nameCantHaveUppercase
--- PASS: TestAccAWSAthenaDatabase_nameCantHaveUppercase (0.92s)
=== RUN   TestAccAWSAthenaDatabase_destroyFailsIfTablesExist
--- PASS: TestAccAWSAthenaDatabase_destroyFailsIfTablesExist (61.16s)
=== RUN   TestAccAWSAthenaDatabase_forceDestroyAlwaysSucceeds
--- PASS: TestAccAWSAthenaDatabase_forceDestroyAlwaysSucceeds (72.15s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	314.027s

[bflad]

Hi @joestump 👋 Thanks for submitting this! Please see the initial feedback below and let us know if you have any questions.

[bflad]

Since this project has a very flat package structure, we prefer to include the service name in the function name. We also refer to these generally as "expand" functions and it would be nice to reduce the signature to not use d directly. e.g.

func expandAthenaResultConfiguration(bucket string, encryptionConfigurationList []interface{}) (*athena.ResultConfiguration, error) {

[bflad]

Since the schema already has this set as Required: true this error handling is extraneous.

[bflad]

Does the API not provide a helpful error message already? Since this error handling occurs during apply time anyways, we should avoid adding logic the API already handles unless necessary.

[joestump]

It does; just felt dirty making an AWS call for known errors. If that’s the pattern, I’ll update though.

[bflad]

The API refers to this as encryption_configuration -- is there any reason to diverge from their naming?

[joestump]

I'd modeled it after the CodePipeline's HCL, which makes a bit more sense, but if the pattern is to copy the API directly to HCL I'll update.

[bflad]

It might make a bit more sense today, but not necessarily in the future should the API change. Here are a few reasons we prefer to stick with the API:

• We have gotten bitten in the past by trying to create abstractions and the API changes in a way that doesn't match those abstractions.
• Less of a maintenance burden to mentally work with the codebase if there are no abstractions/conversions.
• It keeps Terraform in line with the AWS API documentation, SDKs, and CloudFormation so operators coming from other documentation or systems can easily reference or convert between them.
• Different AWS services don't always line up with each other in terms of implementations. While this may look similar to CodePipeline today, Athena or CodePipeline may change tomorrow.

Hope this helps!

[bflad]

The API refers to this as kms_key -- is there any reason to diverge from their naming?

[bflad]

Two things:

• The API refers to this as encryption_option -- is there any reason to diverge from their naming?
• We should validate this attribute using the SDK provided constants:

ValidateFunc: validation.StringInSlice([]string{
  athena.EncryptionOptionCseKms,
  athena.EncryptionOptionSseKms,
  athena.EncryptionOptionSseS3,
}, false),

[joestump]

Good call on the validate. I mentioned why I diverged from the API above. I think the HCL I've proposed would look cleaner than this:

encryption_configuration {
  encryption_option = "CSE_KMS"
  kms_key = "${aws_kms_key.foo.id}"
}

It feels verbose to me. I think the CodePipeline API looks/feels more correct.

[joestump]

@bflad I've addressed most of the comments related to the Golang. Let me know what you think about my response on the HCL. I'm new to the AWS provider and will defer to the prevailing patterns. 👍

Thanks for the review!

[joestump]

@bflad I took a stab at adding your notes into the CONTRIBUTING.md. I believe all comments have been addressed now. 👍

[bflad]

Thanks so much, @joestump! 🚀

--- PASS: TestAccAWSAthenaDatabase_nameCantHaveUppercase (0.89s)
--- PASS: TestAccAWSAthenaDatabase_nameStartsWithUnderscore (26.71s)
--- PASS: TestAccAWSAthenaDatabase_basic (26.77s)
--- PASS: TestAccAWSAthenaDatabase_forceDestroyAlwaysSucceeds (31.01s)
--- PASS: TestAccAWSAthenaDatabase_destroyFailsIfTablesExist (36.16s)
--- PASS: TestAccAWSAthenaDatabase_encryption (49.10s)

[bflad]

This has been released in version 1.41.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!