resource/aws_dynamodb_table: Fix DynamoDB TTL state detection

[goakley]

Fixes #3463

DynamoDB has TTL state always set to DISABLED unless it is otherwise enabled. The terraform logic decided that this meant if TTL is set to DISABLED, then there should be no ttl {} block at all. This is incorrect, as there can be a ttl {} block with the setting enabled = false. This change fixes that issue.

DynamoDB acceptance tests have been updated and run.

[goakley]

@radeksimko Is there anything that could be improved to make this change more amicable?

[danieladams456]

bump

[bflad]

Generally speaking: setting Computed: true is not an ideal situation because it prevents Terraform from being able to detect any sort of API drift when an attribute is not defined in the configuration. In this case, people would most likely be expecting that Terraform let's them know when TTL is enabled on a DynamoDB table outside of Terraform when its disabled by default and its not defined in their configuration.

We'll need to figure out how to support this without Computed: true most likely. An option might be setting this attribute up like how the aws_codebuild_project cache attribute is with a DiffSuppressFunc and changing enabled to Optional: true (and potentially Default: false).

[goakley]

Are the Computed, DiffSuppressFunc, etc properties documented anywhere? I could not find them and was not actually sure what Computed means. I didn't know about DiffSuppressFunc - I copied this strategy from the aws_s3_bucket resource, but could take that approach instead.

[bflad]

I believe this documentation page just went live today: https://www.terraform.io/docs/extend/schemas/schema-behaviors.html

[goakley]

@bflad the CR now uses DiffSuppressFunc and still works as expected.

[goakley]

Any chance this is acceptable now?

[goakley]

@bflad Are there any issues with this approach?

[goakley]

Is this something that shouldn't be fixed by design?

[danieladams456]

This fix would be very useful.

[pshastry]

This would be very helpful

[goakley]

@bflad is there any chance this can get looked at again?

[bflad]

Hi @goakley 👋 Sorry this got lost in the shuffle and thank you for your work here. 😖 I've provided a more thorough review below, which should help us simplify the handling of ttl overall and better match more modern Terraform AWS Provider coding conventions. Please reach out with any questions or if you do not have time to implement the updates.

One important thing item before merging is that we should add a covering acceptance test for this functionality to ensure we have fixed the original issue and try to prevent regressions in the future. e.g.

// Replacing existing TestAccAWSDynamoDbTable_ttl
// Removing TestAccAWSDynamoDbTable_importTimeToLive and testAccCheckDynamoDbTableTimeToLiveWasUpdated
func TestAccAWSDynamoDbTable_ttl(t *testing.T) {
	var table dynamodb.DescribeTableOutput
	rName := acctest.RandomWithPrefix("TerraformTestTable-")
	resourceName := "aws_dynamodb_table.test"

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckAWSDynamoDbTableDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccAWSDynamoDbConfigTimeToLive(rName, true),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInitialAWSDynamoDbTableExists(resourceName, &table),
					resource.TestCheckResourceAttr(resourceName, "ttl.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", "true"),
				),
			},
			{
				ResourceName:      resourceName,
				ImportState:       true,
				ImportStateVerify: true,
			},
			{
				Config: testAccAWSDynamoDbConfigTimeToLive(rName, false),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInitialAWSDynamoDbTableExists(resourceName, &table),
					resource.TestCheckResourceAttr(resourceName, "ttl.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", "false"),
				),
			},
			{
				Config: testAccAWSDynamoDbConfigTimeToLive(rName, true),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckInitialAWSDynamoDbTableExists(resourceName, &table),
					resource.TestCheckResourceAttr(resourceName, "ttl.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "ttl.0.enabled", "true"),
				),
			},
		},
	})
}

// Replacing testAccAWSDynamoDbConfigAddTimeToLive
func testAccAWSDynamoDbConfigTimeToLive(rName string, ttlEnabled bool) string {
	return fmt.Sprintf(`
resource "aws_dynamodb_table" "test" {
  hash_key       = "TestTableHashKey"
  name           = %[1]q
  read_capacity  = 1
  write_capacity = 1

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

  ttl {
    attribute_name = "TestTTL"
    enabled        = %[2]t
  }
}
`, rName, ttlEnabled)
}

Thanks again.

[bflad]

We can simplify this handling and prevent potential panics in the DiffSuppressFunc logic by always giving the Terraform state a sensible default value for enabled, then telling it to ignore when the configuration is missing:

"ttl": {
	Type:     schema.TypeList, // Purposeful change, TypeSet + MaxItems: 1 overly complicates creating/accessing a configuration block attribute
	Optional: true,
	MaxItems: 1,
	Elem: &schema.Resource{
		Schema: map[string]*schema.Schema{
			"attribute_name": {
				Type:     schema.TypeString,
				Required: true,
			},
			"enabled": {
				Type:     schema.TypeBool,
				Optional: true,
				Default:  false,
			},
		},
	},
	DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
		return old == "1" && new == "0"
	},
},

Replacing the flatten function to always return the configuration block with our expected default:

// Replacing flattenDynamoDbTtl
func flattenDynamoDbTtl(ttlOutput *dynamodb.DescribeTimeToLiveOutput) []interface{} {
	m := map[string]interface{}{
		"enabled": false,
	}

	if ttlOutput == nil || ttlOutput.TimeToLiveDescription == nil {
		return []interface{}{m}
	}

	m["attribute_name"] = aws.StringValue(ttlDesc.AttributeName)
	m["enabled"] = (aws.StringValue(ttlDesc.TimeToLiveStatus) == dynamodb.TimeToLiveStatusEnabled)

	return []interface{}{m}
}

// in resourceAwsDynamoDbTableRead and dataSourceAwsDynamoDbTableRead
if err := d.Set("ttl", flattenDynamoDbTtl(ttlOut)); err != nil {
	return fmt.Errorf("error setting ttl: %s", err)
}

And simplifying the update functionality:

// in resourceAwsDynamoDbTableCreate
	if d.Get("ttl.0.enabled").(bool) {
		if err := updateDynamoDbTimeToLive(d.Id(), d.Get("ttl").([]interface{}), conn); err != nil {
			return err
		}
	}
// in resourceAwsDynamoDbTableUpdate
	if d.HasChange("ttl") {
		if err := updateDynamoDbTimeToLive(d.Id(), d.Get("ttl").([]interface{}), conn); err != nil {
			return err
		}
	}
// Replacing updateDynamoDbTimeToLive
func updateDynamoDbTimeToLive(tableName string, ttlList []interface{}, conn *dynamodb.DynamoDB) error {
	ttlMap := ttlList[0].(map[string]interface{})

	input := &dynamodb.UpdateTimeToLiveInput{
		TableName: aws.String(tableName),
		TimeToLiveSpecification: &dynamodb.TimeToLiveSpecification{
			AttributeName: aws.String(ttlMap["attribute_name"].(string)),
			Enabled:       aws.Bool(ttlMap["enabled"].(bool)),
		},
	}

	log.Printf("[DEBUG] Updating DynamoDB Table (%s) Time To Live: %s", tableName, input)
	if _, err := conn.UpdateTimeToLive(input); err != nil {
		return fmt.Errorf("error updating DynamoDB Table (%s) Time To Live: %s", tableName, err)
	}

	log.Printf("[DEBUG] Waiting for DynamoDB Table (%s) Time to Live update to complete", tableName)
	if err := waitForDynamoDbTtlUpdateToBeCompleted(tableName, ttlMap["enabled"].(bool), conn); err != nil {
		return fmt.Errorf("error waiting for DynamoDB Table (%s) Time To Live update: %s", err)
	}

	return nil
}

[bflad]

Since its been about two weeks without response, we went ahead and added a followup commit (aed75eb) with the review feedback and to fix the merge conflict. Thanks for your work on this, @goakley 🚀

Output from acceptance testing:

--- PASS: TestAccAWSDynamoDbTable_streamSpecificationValidation (3.67s)
--- PASS: TestAccAWSDynamoDbTable_attributeUpdateValidation (5.01s)
--- PASS: TestAccAWSDynamoDbTable_disappears (20.45s)
--- PASS: TestAccAWSDynamoDbTable_basic (24.59s)
--- PASS: TestAccAWSDynamoDbTable_Ttl_Enabled (116.79s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateCapacity (123.98s)
--- PASS: TestAccAWSDynamoDbTable_Ttl_Disabled (126.82s)
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (127.31s)
--- PASS: TestAccAWSDynamoDbTable_BillingMode_PayPerRequestToProvisioned (136.21s)
--- PASS: TestAccAWSDynamoDbTable_extended (139.28s)
--- PASS: TestAccAWSDynamoDbTable_importBasic (143.27s)
--- PASS: TestAccAWSDynamoDbTable_BillingMode_GSI_PayPerRequestToProvisioned (154.62s)
--- PASS: TestAccAWSDynamoDbTable_tags (173.85s)
--- PASS: TestAccAWSDynamoDbTable_importTags (174.06s)
--- PASS: TestAccAWSDynamoDbTable_encryption (224.06s)
--- PASS: TestAccAWSDynamoDbTable_disappears_PayPerRequestWithGSI (261.18s)
--- PASS: TestAccAWSDynamoDbTable_enablePitr (306.63s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateNonKeyAttributes (430.18s)
--- PASS: TestAccAWSDynamoDbTable_attributeUpdate (427.20s)
--- PASS: TestAccAWSDynamoDbTable_gsiUpdateOtherAttributes (742.24s)
--- PASS: TestAccAWSDynamoDbTable_BillingMode_ProvisionedToPayPerRequest (1324.62s)
--- PASS: TestAccAWSDynamoDbTable_BillingMode_GSI_ProvisionedToPayPerRequest (2391.74s)

[goakley]

Thanks @bflad! I've been away until this week and I'm glad to see this was addressed.

[bflad]

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

[inge4pres]

Added some info regarding the TTL problem here

[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!