Support encryption at rest for DynamoDB tables #3303

ewbankkit · 2018-02-09T01:25:14Z

Fixes #3298.
Added an encrypt_at_rest block to aws_dynamodb_table resource and data source, similar to aws_elasticsearch_domain.

Acceptance tests:

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSDynamoDbTable_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSDynamoDbTable_basic -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (199.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	199.527s

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSDynamoDbTable_encryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSDynamoDbTable_encryption -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_encryption
--- PASS: TestAccAWSDynamoDbTable_encryption (55.71s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	55.729s

make testacc TEST=./aws/ TESTARGS='-run=TestAccDataSourceAwsDynamoDbTable_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccDataSourceAwsDynamoDbTable_basic -timeout 120m
=== RUN   TestAccDataSourceAwsDynamoDbTable_basic
--- PASS: TestAccDataSourceAwsDynamoDbTable_basic (49.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	49.304s

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSDynamoDbTable_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSDynamoDbTable_basic -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (199.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	199.527s

ctreatma · 2018-03-01T16:08:55Z

What more needs to be done in order to get this in to a release?

bflad · 2018-03-01T17:12:38Z

I'll try to give this a proper review this week.

bflad

Hi @ewbankkit, excellent job! This is looking pretty decent but I left some comments below. Please let me know if you have any questions or don't have time to finish this up. Thanks!

Also this is kinda funny when I ran it (likely not your fault 😂 ):

=== RUN   TestAccAWSDynamoDbTable_encryption
--- FAIL: TestAccAWSDynamoDbTable_encryption (108.17s)
    testing.go:513: Step 0 error: Check failed: Check 2/2 error: Provisioned write capacity was 842352298560, not 2!

bflad · 2018-03-01T20:13:57Z

aws/data_source_aws_dynamodb_table.go

@@ -167,6 +167,20 @@ func dataSourceAwsDynamoDbTable() *schema.Resource {
 Type: schema.TypeInt,
 Computed: true,
 },
+ "encrypt_at_rest": {


😅 Sorry for this, but could you rename all these encrypt_at_rest references to server_side_encryption so they better match the DynamoDB API SSE?

bflad · 2018-03-01T20:19:46Z

aws/resource_aws_dynamodb_table.go

@@ -250,6 +265,16 @@ func resourceAwsDynamoDbTableCreate(d *schema.ResourceData, meta interface{}) er
 }
 }

+ if v, ok := d.GetOk("encrypt_at_rest"); ok {
+ options := v.([]interface{})
+ if options[0] == nil {


I think this needs to check len(options) == 0

s := make([]interface{}, 0) if s[0] == nil { log.Println("All is well") } # panic: runtime error: index out of range s := make([]interface{}, 0) if len(s) == 0 { log.Println("All is well") } # 2018/03/01 15:19:00 All is well

Generally speaking though, we forego error checking against our own schema.

Doesn't GetOk() guard against len(options) == 0?

bflad · 2018-03-01T20:23:22Z

aws/resource_aws_dynamodb_table_test.go

+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckInitialAWSDynamoDbTableExists("aws_dynamodb_table.basic-dynamodb-table", &conf),
+ testAccCheckInitialAWSDynamoDbTableConf("aws_dynamodb_table.basic-dynamodb-table", true),
+ ),


Can you please check the Terraform state here?

resource.TestCheckResourceAttr("aws_dynamodb_table.basic-dynamodb-table", "server_side_encryption.#", "1"), resource.TestCheckResourceAttr("aws_dynamodb_table.basic-dynamodb-table", "server_side_encryption.0.enabled", "true"),

By doing this and removing the extraneous attributes I can now revert the modifications to testAccCheckInitialAWSDynamoDbTableConf so as not to need the sse bool parameter

bflad · 2018-03-01T20:26:14Z

aws/resource_aws_dynamodb_table_test.go

+ return fmt.Sprintf(`
+resource "aws_dynamodb_table" "basic-dynamodb-table" {
+ name = "%s"
+ read_capacity = 10


Can you please set the test capacities to 1? 💸

bflad · 2018-03-01T20:26:57Z

aws/resource_aws_dynamodb_table_test.go

+ type = "S"
+ }
+
+ attribute {


Can you please remove extraneous attributes/indexes here? We only really care about the encryption setting.

bflad · 2018-03-01T20:27:37Z

aws/resource_aws_dynamodb_table_test.go

+ projection_type = "KEYS_ONLY"
+ }
+
+ encrypt_at_rest {


Nitpick: Indentation got thrown off here

Will do, copy and paste flub

This reverts commit 1aa8400.

ewbankkit · 2018-03-02T14:39:43Z

Re-ran acceptance tests:

make testacc TEST=./aws/ TESTARGS='-run=TestAccDataSourceAwsDynamoDbTable_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccDataSourceAwsDynamoDbTable_basic -timeout 120m
=== RUN   TestAccDataSourceAwsDynamoDbTable_basic
--- PASS: TestAccDataSourceAwsDynamoDbTable_basic (63.78s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	63.792s

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSDynamoDbTable_basic'==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSDynamoDbTable_basic -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (232.25s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	232.265s

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSDynamoDbTable_encryption'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSDynamoDbTable_encryption -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_encryption
--- PASS: TestAccAWSDynamoDbTable_encryption (27.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	27.535s

ewbankkit · 2018-03-02T14:46:40Z

@bflad I have made the changes you suggested (question about the len(options) == 0 comment above).

I have noticed that if I create a table

resource "aws_dynamodb_table" "basic-dynamodb-table" {
  name = "test-enc-0"
  read_capacity = 1
  write_capacity = 1
  hash_key = "TestTableHashKey"

  attribute {
    name = "TestTableHashKey"
    type = "S"
  }
}

and then change the code to

resource "aws_dynamodb_table" "basic-dynamodb-table" {
  name = "test-enc-0"
  read_capacity = 1
  write_capacity = 1
  hash_key = "TestTableHashKey"

  attribute {
    name = "TestTableHashKey"
    type = "S"
  }

  server_side_encryption {
    enabled = false
  }
}

terraform plan shows a diff

-/+ aws_dynamodb_table.basic-dynamodb-table (new resource required)
      id:                               "test-enc-0" => <computed> (forces new resource)
      arn:                              "arn:aws:dynamodb:us-west-2:000000000000:table/test-enc-0" => <computed>
      attribute.#:                      "1" => "1"
      attribute.2990477658.name:        "TestTableHashKey" => "TestTableHashKey"
      attribute.2990477658.type:        "S" => "S"
      hash_key:                         "TestTableHashKey" => "TestTableHashKey"
      name:                             "test-enc-0" => "test-enc-0"
      read_capacity:                    "1" => "1"
      server_side_encryption.#:         "0" => "1"
      server_side_encryption.0.enabled: "" => "false" (forces new resource)
      stream_arn:                       "" => <computed>
      stream_label:                     "" => <computed>
      stream_view_type:                 "" => <computed>
      write_capacity:                   "1" => "1"

It looks like I'll need to add a DiffSuppressFunc.

ewbankkit · 2018-03-02T17:01:56Z

Ended up using the CustomizeDiff functionality to do this as DiffSuppressFunc doesn't support lists/maps.

bflad · 2018-03-02T19:01:42Z

@ewbankkit sounds like we should add an acceptance test for that case so its not made a regression in the future. I was able to get the existing testing in this PR passing with Optional: true and Default: false on the enabled attribute (removing the CustomizeDiff) but I did not test your new scenario.

Maybe we should rework the test suite slightly:

Create a func testAccAWSDynamoDbConfig_basic(rName string) string { that's basically a duplicate of the new testAccAWSDynamoDbConfigInitialStateWithEncryption without the SSE config block
Add enabled bool parameter to testAccAWSDynamoDbConfigInitialStateWithEncryption and
use enabled = %t
Update TestAccAWSDynamoDbTable_encryption (or create a new separate test) to include new scenario you mentioned as two test steps
Rename existing TestAccAWSDynamoDbTable_basic into something like TestAccAWSDynamoDbTable_extended (ugh naming is hard, but its just testing way too many optional configurations right now)
Recreate TestAccAWSDynamoDbTable_basic at the new testAccAWSDynamoDbConfig_basic and check all the basic attributes in the state

If you don't have time to work on the above, definitely an above and beyond ask, please let us know and we can add commits after yours. 👍

ewbankkit · 2018-03-05T14:00:59Z

@bflad I'll make those suggested changes to the acceptance tests; Should be able to get to them today.

ewbankkit · 2018-03-05T23:31:40Z

@bflad I made those changes to the aws_dynamodb_table resource acceptance tests, using the names you suggested.
In TestAccAWSDynamoDbTable_encryption I use the table creation timestamp to make sure that the table gets recreated when the enabled value changes and doesn't get recreated when it shouldn't.

bflad

This looks fantastic, thanks for your great work! 🚀

20 tests passed (all tests)
=== RUN   TestAccAWSDynamoDbTable_streamSpecificationValidation
--- PASS: TestAccAWSDynamoDbTable_streamSpecificationValidation (3.03s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (16.83s)
=== RUN   TestAccAWSDynamoDbTableItem_rangeKey
--- PASS: TestAccAWSDynamoDbTableItem_rangeKey (23.38s)
=== RUN   TestAccAWSDynamoDbTableItem_withMultipleItems
--- PASS: TestAccAWSDynamoDbTableItem_withMultipleItems (23.70s)
=== RUN   TestAccAWSDynamoDbTableItem_basic
--- PASS: TestAccAWSDynamoDbTableItem_basic (23.69s)
=== RUN   TestAccAWSDynamoDbTableItem_update
--- PASS: TestAccAWSDynamoDbTableItem_update (28.04s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (33.45s)
=== RUN   TestAccAWSDynamoDbTable_encryption
--- PASS: TestAccAWSDynamoDbTable_encryption (45.27s)
=== RUN   TestAccAWSDynamoDbGlobalTable_import
--- PASS: TestAccAWSDynamoDbGlobalTable_import (51.21s)
=== RUN   TestAccAWSDynamoDbGlobalTable_basic
--- PASS: TestAccAWSDynamoDbGlobalTable_basic (59.28s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (72.82s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (73.55s)
=== RUN   TestAccAWSDynamoDbGlobalTable_multipleRegions
--- PASS: TestAccAWSDynamoDbGlobalTable_multipleRegions (86.71s)
=== RUN   TestAccAWSDynamoDbTable_ttl
--- PASS: TestAccAWSDynamoDbTable_ttl (102.01s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateCapacity
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateCapacity (103.15s)
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (110.20s)
=== RUN   TestAccAWSDynamoDbTable_importTimeToLive
--- PASS: TestAccAWSDynamoDbTable_importTimeToLive (118.67s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes (158.37s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes (455.33s)
=== RUN   TestAccAWSDynamoDbTable_extended
--- PASS: TestAccAWSDynamoDbTable_extended (470.47s)

bflad · 2018-03-09T19:57:42Z

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

ghost · 2020-04-07T17:39:24Z

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!

Support encryption at rest for DynamoDB tables.

2114a3b

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 9, 2018

Ninir added enhancement Requests to existing resources that expand the functionality or scope. service/dynamodb Issues and PRs that pertain to the dynamodb service. labels Feb 9, 2018

bflad added this to the v1.11.0 milestone Mar 1, 2018

bflad self-requested a review March 1, 2018 17:12

bflad reviewed Mar 1, 2018

View reviewed changes

ewbankkit added 4 commits March 2, 2018 08:49

Extra check on empty SSE options.

1aa8400

Acceptance test changes after review.

774d415

'encrypt_at_rest' -> 'server_side_encryption'.

80af905

Revert "Extra check on empty SSE options."

5223968

This reverts commit 1aa8400.

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 2, 2018

'encrypt_at_rest' -> 'server_side_encryption'.

b4ddab7

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 2, 2018

ewbankkit added 2 commits March 5, 2018 18:08

Update 'CustomizeDiff' to prevent incorrect 'ForceNew's.

5c58790

Additional encryption acceptance test steps.

0eb8914

ewbankkit force-pushed the issue-3298 branch from 0693e2d to 0eb8914 Compare March 5, 2018 23:28

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 5, 2018

bflad approved these changes Mar 6, 2018

View reviewed changes

bflad merged commit c92ee2a into hashicorp:master Mar 6, 2018

bflad added a commit that referenced this pull request Mar 6, 2018

Update CHANGELOG for #3303

e4c3350

ewbankkit deleted the issue-3298 branch March 9, 2018 13:55

darend mentioned this pull request Mar 16, 2018

Enable encryption and do not request a specific role cloudposse/terraform-aws-dynamodb#3

Merged

aknysh mentioned this pull request Apr 3, 2018

invalid or unknown key: server_side_encryption cloudposse/terraform-aws-tfstate-backend#2

Closed

mmdriley mentioned this pull request Apr 11, 2018

Diff seeing changes even when there are none pulumi/pulumi#1147

Closed

bflad mentioned this pull request Jun 22, 2018

Dynamodb use server_side_encryption #4949

Closed

ewbankkit mentioned this pull request Aug 21, 2018

r/aws_dynamodb_table: Support 11/15/2018 Server-Side Encryption enhancements #5637

Closed

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support encryption at rest for DynamoDB tables #3303

Support encryption at rest for DynamoDB tables #3303

ewbankkit commented Feb 9, 2018

ctreatma commented Mar 1, 2018

bflad commented Mar 1, 2018

bflad left a comment

bflad Mar 1, 2018

ewbankkit Mar 2, 2018

bflad Mar 1, 2018

bflad Mar 1, 2018

ewbankkit Mar 2, 2018

bflad Mar 1, 2018

ewbankkit Mar 2, 2018

ewbankkit Mar 2, 2018

bflad Mar 1, 2018

ewbankkit Mar 2, 2018

bflad Mar 1, 2018

ewbankkit Mar 2, 2018

bflad Mar 1, 2018

ewbankkit Mar 2, 2018 •

edited

Loading

ewbankkit commented Mar 2, 2018

ewbankkit commented Mar 2, 2018 •

edited

Loading

ewbankkit commented Mar 2, 2018

bflad commented Mar 2, 2018

ewbankkit commented Mar 5, 2018

ewbankkit commented Mar 5, 2018

bflad left a comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

Support encryption at rest for DynamoDB tables #3303

Support encryption at rest for DynamoDB tables #3303

Conversation

ewbankkit commented Feb 9, 2018

ctreatma commented Mar 1, 2018

bflad commented Mar 1, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewbankkit Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

ewbankkit commented Mar 2, 2018

ewbankkit commented Mar 2, 2018 • edited Loading

ewbankkit commented Mar 2, 2018

bflad commented Mar 2, 2018

ewbankkit commented Mar 5, 2018

ewbankkit commented Mar 5, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Mar 9, 2018

ghost commented Apr 7, 2020

ewbankkit Mar 2, 2018 •

edited

Loading

ewbankkit commented Mar 2, 2018 •

edited

Loading