Add new feature for resoure acmpca_certificate_authority

[vnguyen7]

Related #4994

Changes proposed in this pull request:

Add new feature permanent_deletion_time_in_days for resource acmpca_certificate_authority

[bflad]

Hi @vnguyen7 👋 Thanks for this contribution. Apologies we don't currently have time to spend review cycles working on this pull request together to be accepted as we have an immediate need to get this functionality in, but I left some hopefully detailed notes during work on the followup commit (3f862bc) that should help for your next submission. Thanks again.

Output from acceptance testing after code updates:

--- PASS: TestAccAwsAcmpcaCertificateAuthority_Basic (24.05s)
--- PASS: TestAccAwsAcmpcaCertificateAuthority_Tags (53.09s)
--- PASS: TestAccAwsAcmpcaCertificateAuthority_RevocationConfiguration_CrlConfiguration_ExpirationInDays (70.87s)
--- PASS: TestAccAwsAcmpcaCertificateAuthority_RevocationConfiguration_CrlConfiguration_Enabled (81.25s)
--- PASS: TestAccAwsAcmpcaCertificateAuthority_RevocationConfiguration_CrlConfiguration_CustomCname (100.37s)

[bflad]

A few things here:

• The attribute name includes an extra space at the end
• The attribute should include Default: 30, to match the API default
• Since there is no ability to call d.Set() of the attribute properly during the Read function, the attribute needs to have d.Set() called during the import process, otherwise Terraform will report a plan difference immediately after import like "" => "30"

To support the new attribute with a default, the resource should include a Terraform state migration to correctly set the new default in the state (since it cannot be added to the Read function with d.Set()), e.g.

In the resource schema:

MigrateState:  resourceAwsAcmpcaCertificateAuthorityMigrateState,
SchemaVersion: 1,

In a new aws/resource_aws_acmpca_certificate_authority_migrate.go file:

package aws

import (
	"fmt"
	"log"

	"github.com/hashicorp/terraform/terraform"
)

func resourceAwsAcmpcaCertificateAuthorityMigrateState(v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
	switch v {
	case 0:
		log.Println("[INFO] Found ACMPCA Certificate Authority state v0; migrating to v1")
		return migrateAcmpcaCertificateAuthorityStateV0toV1(is)
	default:
		return is, fmt.Errorf("Unexpected schema version: %d", v)
	}
}

func migrateAcmpcaCertificateAuthorityStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
	if is.Empty() || is.Attributes == nil {
		log.Println("[DEBUG] Empty ACMPCA Certificate Authority state; nothing to migrate.")
		return is, nil
	}

	log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)

	// Add permanent_deletion_time_in_days virtual attribute with Default
	is.Attributes["permanent_deletion_time_in_days"] = "30"

	log.Printf("[DEBUG] Attributes after migration: %#v", is.Attributes)

	return is, nil
}

And unit tested in a new file named aws/resource_aws_acmpca_certificate_authority_migrate_test.go:

package aws

import (
	"testing"

	"github.com/hashicorp/terraform/terraform"
)

func TestAwsAcmpcaCertificateAuthorityMigrateState(t *testing.T) {
	testCases := map[string]struct {
		StateVersion int
		Attributes   map[string]string
		Expected     map[string]string
		Meta         interface{}
	}{
		"v0_to_v1": {
			StateVersion: 0,
			Attributes: map[string]string{
				"permanent_deletion_time_in_days": "",
			},
			Expected: map[string]string{
				"permanent_deletion_time_in_days": "30",
			},
		},
	}

	for testName, testCase := range testCases {
		instanceState := &terraform.InstanceState{
			ID:         "some_id",
			Attributes: testCase.Attributes,
		}

		tfResource := resourceAwsAcmpcaCertificateAuthority()

		if tfResource.MigrateState == nil {
			t.Fatalf("bad: %s, err: missing MigrateState function in resource", testName)
		}

		instanceState, err := tfResource.MigrateState(testCase.StateVersion, instanceState, testCase.Meta)
		if err != nil {
			t.Fatalf("bad: %s, err: %#v", testName, err)
		}

		for key, expectedValue := range testCase.Expected {
			if instanceState.Attributes[key] != expectedValue {
				t.Fatalf(
					"bad: %s\n\n expected: %#v -> %#v\n got: %#v -> %#v\n in: %#v",
					testName, key, expectedValue, key, instanceState.Attributes[key], instanceState.Attributes)
			}
		}
	}
}

To fix the import portion, it can be handled by switching from schema.ImportStatePassthrough to a custom import function, e.g.

Importer: &schema.ResourceImporter{
	State: func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
		d.Set("permanent_deletion_time_in_days", 30)

		return []*schema.ResourceData{d}, nil
	},
},

[bflad]

The attribute includes an extra space in the name here as well, but more importantly this logic should be moved above the log line so operators and code maintainers can more easily debug this new parameter to the API call.

[bflad]

I'm not sure if there was a reason behind removing the input variable here, but we prefer that coding style and the PermanentDeletionTimeInDays: aws.Int64(int64(7)), line could have just been added to the struct. In the updated code below it is missing the required CertificateAuthorityArn: aws.String(arn),.

input := &acmpca.DeleteCertificateAuthorityInput{
	CertificateAuthorityArn:     aws.String(arn),
	PermanentDeletionTimeInDays: aws.Int64(int64(7)),
}

[bflad]

Nit: Looks like spaces were switched to tabs here and below.

[bflad]

Nit: Missing space before backtick which is breaking formatting:

Suggested change

	*`permanent_deletion_time_in_days` - (Optional) The number of days to make a CA restorable after it has been deleted, must be between 7 to 30 days, with default to 30 days.
	* `permanent_deletion_time_in_days` - (Optional) The number of days to make a CA restorable after it has been deleted, must be between 7 to 30 days, with default to 30 days.

[bflad]

Some of these test configurations include the new permanent_deletion_time_in_days argument inside other configuration blocks, which prevents the tests from running.

[bflad]

This has been released in version 2.5.0 of the Terraform 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!