resource/aws_route53_resolver_dnssec_config - new resource #17012

shuheiktgw · 2021-01-07T22:28:01Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

* **New Resource:** `aws_route53_resolver_dnssec_config`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSRoute53ResolverDnssecConfig_*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSRoute53ResolverDnssecConfig_* -timeout 120m
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_basic
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_basic
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_disappear
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_disappear
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_disappear_VPC
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_disappear_VPC
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_basic
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_disappear_VPC
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_disappear
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_disappear_VPC (159.43s)
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_basic (287.57s)
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_disappear (289.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	291.487s

Thank you for your review! 👍

aws/resource_aws_route53_resolver_dnssec_config.go

+	log.Printf("[DEBUG] Creating Route53 Resolver DNSSEC config: %#v", req)
+	resp, err := conn.UpdateResolverDnssecConfig(req)
+	if err != nil {
+		return fmt.Errorf("error creating Route53 Resolver DNSSEC config: %s", err)


aws/resource_aws_route53_resolver_dnssec_config.go

+
+	vpc, err := vpcDescribe(ec2Conn, d.Id())
+	if err != nil {
+		return fmt.Errorf("error getting VPC associated with Route53 Resolver DNSSEC config (%s): %s", d.Id(), err)


aws/resource_aws_route53_resolver_dnssec_config.go

+
+	raw, state, err := route53ResolverDnssecConfigRefresh(conn, d.Id())()
+	if err != nil {
+		return fmt.Errorf("error getting Route53 Resolver DNSSEC config (%s): %s", d.Id(), err)


aws/resource_aws_route53_resolver_dnssec_config.go

+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("error deleting Route53 Resolver DNSSEC config (%s): %s", d.Id(), err)


aws/resource_aws_route53_resolver_dnssec_config.go

+		MinTimeout: 5 * time.Second,
+	}
+	if _, err := stateConf.WaitForState(); err != nil {
+		return fmt.Errorf("error waiting for Route53 Resolver DNSSEC config (%s) to reach target state: %s", id, err)


ewbankkit · 2021-01-12T15:21:28Z

@shuheiktgw Thanks for the contribution 👏. Overall it looks great.
I left a few nits on wrapping errors (I think the Error Handling guide was only merged yesterday 😄).

DNSSEC configs seem to have ARNs even though it's not returned in the API. Could you please add a Computed arn attribute (#13624), ensuring that the correct account ID is used: #16978? Thanks.

I need to think about the cross-service call to verify the VPC's existence. I see why it is being used but we usually try and avoid such calls.

ewbankkit · 2021-01-12T18:06:02Z

aws/resource_aws_route53_resolver_dnssec_config.go

+		return fmt.Errorf("error creating Route53 Resolver DNSSEC config: %s", err)
+	}
+
+	d.SetId(aws.StringValue(resp.ResolverDNSSECConfig.ResourceId))


Use ResolvedDNSSECConfig.Id as that is the actual resource's ID.
Agree that we should use the resource ID on import as that is what users would expect (as this is surfaced per-VPC in the AWS Console).

ewbankkit · 2021-01-12T19:04:52Z

It seems that you can update a DNSSEC config even if the associated resource (VPC) has been deleted:

$ aws --region us-west-2 route53resolver list-resolver-dnssec-configs
{
    "ResolverDnssecConfigs": [
        {
            "Id": "rdsc-5a4960f0b41d3a5b",
            "OwnerId": "123456789012",
            "ResourceId": "vpc-0f2f5c603499f6c6b",
            "ValidationStatus": "DISABLED"
        },
        {
            "Id": "rdsc-9da58af749336ba",
            "OwnerId": "123456789012",
            "ResourceId": "vpc-0674233a9456aba43",
            "ValidationStatus": "DISABLED"
        },
        {
            "Id": "rdsc-be1866ecc1683e95",
            "OwnerId": "123456789012",
            "ResourceId": "vpc-0f993cbbf558be638",
            "ValidationStatus": "ENABLED"
        },
        {
            "Id": "rdsc-ca0760426d4a326b",
            "OwnerId": "123456789012",
            "ResourceId": "vpc-09925a270fdbb4206",
            "ValidationStatus": "ENABLED"
        }
    ]
}
$ aws --region us-west-2 route53resolver update-resolver-dnssec-config --resource-id "vpc-0f993cbbf558be638" --validation DISABLE
{
    "ResolverDNSSECConfig": {
        "Id": "rdsc-be1866ecc1683e95",
        "OwnerId": "123456789012",
        "ResourceId": "vpc-0f993cbbf558be638",
        "ValidationStatus": "DISABLING"
    }
}

I suggest moving the route53ResolverDnssecConfigRefresh and route53ResolverDnssecConfigWait functionality into ResolverDnssecConfigByID in aws/internal/service/route53resolver/finder/finder.go, DnssecConfigValidationStatus in aws/internal/service/route53resolver/waiter/status.go and DnssecConfigEnabled and DnssecConfigDisabled in aws/internal/service/route53resolver/waiter/waiter.go.
ResolverDnssecConfigByID should use the ListResolverDnssecConfigs method (which unfortunately doesn't seem to support any filters yet even though there's a field in the API) and find the DNSSEC config for the ID (as discussed above). Add a comment noting why we can't use GetResolverDnssecConfig. This will eliminate the need for the EC2 API call.

…urce id

shuheiktgw · 2021-01-14T00:32:38Z

@ewbankkit Thank you for your review! I believe I fixed them up so would you review the PR again? 🙏

ewbankkit

LGTM.

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSRoute53ResolverDnssecConfig_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSRoute53ResolverDnssecConfig_ -timeout 120m
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_basic
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_basic
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_disappear
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_disappear
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_basic
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_disappear
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_disappear (397.50s)
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_basic (399.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	399.527s

breathingdust · 2021-01-14T16:43:41Z

LGTM 🚀 Thanks @shuheiktgw!

Verified Acceptance Tests in Commercial (us-west-2)

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSRoute53ResolverDnssecConfig_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSRoute53ResolverDnssecConfig_ -timeout 120m
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_basic
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_basic
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_disappear
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_disappear
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_basic
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_disappear
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_disappear (380.47s)
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_basic (393.46s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	395.145s

Verified Acceptance Tests in GovCloud (us-gov-west-1)

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSRoute53ResolverDnssecConfig_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSRoute53ResolverDnssecConfig_ -timeout 120m
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_basic
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_basic
=== RUN   TestAccAWSRoute53ResolverDnssecConfig_disappear
=== PAUSE TestAccAWSRoute53ResolverDnssecConfig_disappear
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_basic
=== CONT  TestAccAWSRoute53ResolverDnssecConfig_disappear
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_disappear (121.67s)
--- PASS: TestAccAWSRoute53ResolverDnssecConfig_basic (123.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	124.742s

shuheiktgw · 2021-01-14T22:24:52Z

Thank you all for your review!

ghost · 2021-01-15T00:16:08Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2021-02-13T17:11:27Z

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!

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 7, 2021

shuheiktgw added 3 commits January 9, 2021 07:23

Add aws_route53_resolver_dnssec_config

5ac9c72

Add tests for aws_route53_resolver_dnssec_config

6440532

Add a doc for aws_route53_resolver_dnssec_config

d0a3545

shuheiktgw force-pushed the add_route53_resolver_dnssec_config branch from 35e004f to d0a3545 Compare January 8, 2021 22:23

ghost added the documentation Introduces or discusses updates to documentation. label Jan 8, 2021

shuheiktgw marked this pull request as ready for review January 8, 2021 22:26

shuheiktgw requested a review from a team as a code owner January 8, 2021 22:26

ewbankkit self-assigned this Jan 11, 2021

ewbankkit reviewed Jan 12, 2021

View reviewed changes

aws/resource_aws_route53_resolver_dnssec_config.go Outdated

vpc, err := vpcDescribe(ec2Conn, d.Id())

if err != nil {

return fmt.Errorf("error getting VPC associated with Route53 Resolver DNSSEC config (%s): %s", d.Id(), err)

This comment was marked as resolved.

Sign in to view

ewbankkit reviewed Jan 12, 2021

View reviewed changes

aws/resource_aws_route53_resolver_dnssec_config.go Outdated

raw, state, err := route53ResolverDnssecConfigRefresh(conn, d.Id())()

if err != nil {

return fmt.Errorf("error getting Route53 Resolver DNSSEC config (%s): %s", d.Id(), err)

This comment was marked as resolved.

Sign in to view

ewbankkit reviewed Jan 12, 2021

View reviewed changes

aws/resource_aws_route53_resolver_dnssec_config.go Outdated

return nil

}

if err != nil {

return fmt.Errorf("error deleting Route53 Resolver DNSSEC config (%s): %s", d.Id(), err)

This comment was marked as resolved.

Sign in to view

ewbankkit reviewed Jan 12, 2021

View reviewed changes

ewbankkit added thinking new-resource Introduces a new resource. and removed needs-triage Waiting for first response or review from a maintainer. provider Pertains to the provider itself, rather than any interaction with AWS. labels Jan 12, 2021

ewbankkit reviewed Jan 12, 2021

View reviewed changes

ewbankkit removed the thinking label Jan 12, 2021

shuheiktgw added 2 commits January 13, 2021 07:41

Replace %s with %w for the error messages

824d2d9

Add ARN support for aws_route53_resolver_dnssec_config

753dd9f

ghost added the provider Pertains to the provider itself, rather than any interaction with AWS. label Jan 12, 2021

shuheiktgw force-pushed the add_route53_resolver_dnssec_config branch from 06930aa to b2a8f22 Compare January 14, 2021 00:03

Use id instead of resource id for route53_resolver_dnssec_config reso…

e22f485

…urce id

shuheiktgw force-pushed the add_route53_resolver_dnssec_config branch from b2a8f22 to e22f485 Compare January 14, 2021 00:22

ewbankkit approved these changes Jan 14, 2021

View reviewed changes

breathingdust merged commit 60dac14 into hashicorp:master Jan 14, 2021

github-actions bot added this to the v3.24.0 milestone Jan 14, 2021

breathingdust added a commit that referenced this pull request Jan 14, 2021

Update CHANGELOG.md for #17012

678b364

shuheiktgw deleted the add_route53_resolver_dnssec_config branch January 14, 2021 22:24

cageyv mentioned this pull request Jan 15, 2021

Request - DNSSEC validation on the Route 53 Resolver in your VPC terraform-aws-modules/terraform-aws-vpc#559

Closed

ewbankkit mentioned this pull request Jan 15, 2021

r/mwaa_environment - new resource #16616

Merged

ghost locked as resolved and limited conversation to collaborators Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_route53_resolver_dnssec_config - new resource #17012

resource/aws_route53_resolver_dnssec_config - new resource #17012

shuheiktgw commented Jan 7, 2021 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

ewbankkit commented Jan 12, 2021

ewbankkit Jan 12, 2021

ewbankkit commented Jan 12, 2021 •

edited

Loading

shuheiktgw commented Jan 14, 2021

ewbankkit left a comment

breathingdust commented Jan 14, 2021

shuheiktgw commented Jan 14, 2021

ghost commented Jan 15, 2021

ghost commented Feb 13, 2021

resource/aws_route53_resolver_dnssec_config - new resource #17012

resource/aws_route53_resolver_dnssec_config - new resource #17012

Conversation

shuheiktgw commented Jan 7, 2021 • edited Loading

Community Note

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

ewbankkit commented Jan 12, 2021

ewbankkit Jan 12, 2021

Choose a reason for hiding this comment

ewbankkit commented Jan 12, 2021 • edited Loading

shuheiktgw commented Jan 14, 2021

ewbankkit left a comment

Choose a reason for hiding this comment

breathingdust commented Jan 14, 2021

shuheiktgw commented Jan 14, 2021

ghost commented Jan 15, 2021

ghost commented Feb 13, 2021

shuheiktgw commented Jan 7, 2021 •

edited

Loading

ewbankkit commented Jan 12, 2021 •

edited

Loading