Add Route 53 Resolver rule and rule association resources #7799
Conversation
ghost
added
documentation
| @@ -62,7 +63,7 @@ func resourceAwsRoute53ResolverEndpoint() *schema.Resource { | |||
| }, | |||
| }, | |||
| }, | |||
| Set: route53ResolverHashIPAddress, | |||
| Set: route53ResolverEndpointHashIpAddress, | |||
There was a problem hiding this comment.
Renamed route53ResolverHashIPAddress to route53ResolverEndpointHashIpAddress as it applies only to aws_route53_resolver_endpoint.
There was a problem hiding this comment.
Minor nit on name route53ResolverEndpointIpAddressHash
| @@ -303,7 +303,11 @@ func route53ResolverEndpointRefresh(conn *route53resolver.Route53Resolver, epId | |||
| return nil, "", err | |||
| } | |||
|
|
|||
| return resp.ResolverEndpoint, aws.StringValue(resp.ResolverEndpoint.Status), nil | |||
| status := aws.StringValue(resp.ResolverEndpoint.Status) | |||
There was a problem hiding this comment.
Report status message where relevant.
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return fmt.Errorf("Route 53 Resolver endpoint still exists: %s", rs.Primary.ID) |
There was a problem hiding this comment.
Return error if resolver endpoint still exists.
bflad
added
new-resource
|
Hi @ewbankkit thanks for taking the lead on the Route53 resolver rule resource. I'm going to close PR #6571 in favor of pushing this one forward. |
|
Nice people of Hashicorp, Do you think this could be added to the 2.1.0 milestone alongside #6574 ? We're excited for Route 53 Resolver support to be available, but it's useless to us without rule support. |
| return resp.ResolverEndpoint, aws.StringValue(resp.ResolverEndpoint.Status), nil | ||
| status := aws.StringValue(resp.ResolverEndpoint.Status) | ||
| if status == route53resolver.ResolverEndpointStatusActionNeeded { | ||
| return nil, status, errors.New(aws.StringValue(resp.ResolverEndpoint.StatusMessage)) |
There was a problem hiding this comment.
General rule is for the caller not trust non-error return values if an error is returned. Is there a reason to return status along with the error?
Should we provide context to this error or is StatusMessage clear enough so that the end user knows how to act on it?
There was a problem hiding this comment.
Good point. Setting the error means that status is ignored in WaitForState.
Cleanest may be to return nil error and status and log the StatusMessage?
There was a problem hiding this comment.
Let me think about this a bit before making any suggestions.
There was a problem hiding this comment.
I used the same logic as in aws_ram_principal_association: https://github.com/terraform-providers/terraform-provider-aws/blob/190da406cb71111424b7b91f99dc1bccf5873c62/aws/resource_aws_ram_principal_association.go#L186-L189.
|
I take it this missed the 2.1.0 release that just went out the door a few minutes ago? |
There was a problem hiding this comment.
Hi @ewbankkit thanks again for sticking with me through the review. This is looking great! I have a few items that I would like for you to address before pulling this in.
Unfortunately, this means that these two resources will be rolled into the next upcoming release.
| DomainName: aws.String(d.Get("domain_name").(string)), | ||
| RuleType: aws.String(d.Get("rule_type").(string)), | ||
| } | ||
| if v, ok := d.GetOk("name"); ok && v.(string) != "" { |
There was a problem hiding this comment.
GetOk("..") will check for the existence of the key and validate that it is not its zero value so you can drop the v.(string) != "" check.
There was a problem hiding this comment.
I'll also correct other occurrences in aws_route53_resolver_rule, aws_route53_resolver_rule_association and aws_route53_resolver_endpoint.
| return resp.ResolverEndpoint, aws.StringValue(resp.ResolverEndpoint.Status), nil | ||
| status := aws.StringValue(resp.ResolverEndpoint.Status) | ||
| if status == route53resolver.ResolverEndpointStatusActionNeeded { | ||
| return nil, status, errors.New(aws.StringValue(resp.ResolverEndpoint.StatusMessage)) |
There was a problem hiding this comment.
I have a better understanding of how the status is being handled upstream from the caller so in this case returning the status makes sense.
There was a problem hiding this comment.
OK, I'll just return the status and log any StatusMessage at INFO - I think a status message is returned for all statuses so I don't want WARN or ERROR.
There was a problem hiding this comment.
I'll also correct the occurrences in aws_route53_resolver_rule_association and aws_route53_resolver_endpoint.
| ip = "192.0.2.6" | ||
| } | ||
| } | ||
| `, testAccRoute53ResolverEndpointConfig_initial(rInt, "OUTBOUND", name), name) |
There was a problem hiding this comment.
Generally it is preferred to keep the test functions within the same test files and not reused across resources.
| ## Example Usage | ||
|
|
||
| ```hcl | ||
| resource "aws_route53_resolver_rule" "example" { |
There was a problem hiding this comment.
It would be great to see examples for each of the rule types. Regarding the FORWARD type should the optional resolver_endpoint_id be part of the example?
There was a problem hiding this comment.
Yes, I'll add resolver_endpoint_id to the Forward rule example.
| Provides a Route53 Resolver rule association. | ||
| --- | ||
|
|
||
| # aws_route53_resolver_rule |
There was a problem hiding this comment.
| # aws_route53_resolver_rule | |
| # aws_route53_resolver_rule_association |
|
|
||
| ```hcl | ||
| resource "aws_route53_resolver_rule_association" "example" { | ||
| resolver_rule_id = "rslvr-rr-0123456789abcdef0" |
There was a problem hiding this comment.
For examples it's generally preferred to use reference values from other examples, in this case I would expect to see:
resource "aws_route53_resolver_rule_association" "example" {
resolver_rule_id = "${aws_route53_resolver_rule.example.id}"
vpc_id = "${aws_vpc.example.id}"
}| Route53 Resolver rule associations can be imported using the `id`, e.g. | ||
|
|
||
| ``` | ||
| $ terraform import aws_route53_resolver_rule_association.example rslvr-rr-0123456789abcdef0 |
There was a problem hiding this comment.
| $ terraform import aws_route53_resolver_rule_association.example rslvr-rr-0123456789abcdef0 | |
| $ terraform import aws_route53_resolver_rule_association.example rslvr-rrassoc-97242eaf88example |
| ), | ||
| }, | ||
| { | ||
| ResourceName: resourceName, |
There was a problem hiding this comment.
We should include an import test in each of the test cases to ensure that a resource imports properly. By properly I mean that it contains each of the provided optional attributes.
| resource.TestCheckResourceAttr(resourceName, "domain_name", "example.com."), | ||
| resource.TestCheckResourceAttr(resourceName, "rule_type", "SYSTEM"), | ||
| resource.TestCheckResourceAttr(resourceName, "share_status", "NOT_SHARED"), | ||
| resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), |
There was a problem hiding this comment.
This is a great test case to have in place. My only recommendation is that instead of being here we add it to the basic test check. In fact, it is usually preferred to see default value checks within the basic test to protect against unexpected behavior around optional attributes.
ghost
added
the
service/route53
|
Made all suggested changes. $ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverRule_ -timeout 120m
=== RUN TestAccAwsRoute53ResolverRule_basic
=== PAUSE TestAccAwsRoute53ResolverRule_basic
=== RUN TestAccAwsRoute53ResolverRule_tags
=== PAUSE TestAccAwsRoute53ResolverRule_tags
=== RUN TestAccAwsRoute53ResolverRule_updateName
=== PAUSE TestAccAwsRoute53ResolverRule_updateName
=== RUN TestAccAwsRoute53ResolverRule_forward
=== PAUSE TestAccAwsRoute53ResolverRule_forward
=== CONT TestAccAwsRoute53ResolverRule_basic
=== CONT TestAccAwsRoute53ResolverRule_forward
=== CONT TestAccAwsRoute53ResolverRule_updateName
=== CONT TestAccAwsRoute53ResolverRule_tags
--- PASS: TestAccAwsRoute53ResolverRule_basic (36.84s)
--- PASS: TestAccAwsRoute53ResolverRule_tags (57.21s)
--- PASS: TestAccAwsRoute53ResolverRule_updateName (57.41s)
--- PASS: TestAccAwsRoute53ResolverRule_forward (311.22s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 311.239s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverRuleAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverRuleAssociation_ -timeout 120m
=== RUN TestAccAwsRoute53ResolverRuleAssociation_basic
=== PAUSE TestAccAwsRoute53ResolverRuleAssociation_basic
=== CONT TestAccAwsRoute53ResolverRuleAssociation_basic
--- PASS: TestAccAwsRoute53ResolverRuleAssociation_basic (210.75s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 210.769s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverEndpoint_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverEndpoint_ -timeout 120m
=== RUN TestAccAwsRoute53ResolverEndpoint_basicInbound
=== PAUSE TestAccAwsRoute53ResolverEndpoint_basicInbound
=== RUN TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== PAUSE TestAccAwsRoute53ResolverEndpoint_updateOutbound
=== CONT TestAccAwsRoute53ResolverEndpoint_basicInbound
=== CONT TestAccAwsRoute53ResolverEndpoint_updateOutbound
--- PASS: TestAccAwsRoute53ResolverEndpoint_basicInbound (121.95s)
--- PASS: TestAccAwsRoute53ResolverEndpoint_updateOutbound (549.86s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 549.882s |
nywilken
added
waiting-response
| Required: true, | ||
| ForceNew: true, | ||
| DiffSuppressFunc: suppressRoute53ZoneNameWithTrailingDot, | ||
| ValidateFunc: validation.StringLenBetween(1, 256), |
There was a problem hiding this comment.
domain_name = "." is failing ValidateFunc with
aws_route53_resolver_rule.dot: error creating Route 53 Resolver rule: InvalidParameter: 1 validation error(s) found.
- minimum field size of 1, CreateResolverRuleInput.DomainName.
I also tested with domain_name = ".." to see if a . was being dropped, but that fails with an entirely reasonable
* aws_route53_resolver_rule.dot: error creating Route 53 Resolver rule: InvalidParameterException: [RSLVR-00711] Invalid domain name```
There was a problem hiding this comment.
Yes, with domain_name = "." terraform plan shows no domain_name attribute.
I guess that's because according to suppressRoute53ZoneNameWithTrailingDot() "." and "" are the same.
There was a problem hiding this comment.
Can you update suppressRoute53ZoneNameWithTrailingDot() for this case then, since "." is valid here, and in fact explicitly suggested as a catch-all rule called ForwardAll in AWS training materials? (Here: http://www.youtube.com/watch?v=Rka2rs0J9BI&t=24m30s )
I don't think changing the func will impact the other R53 code that uses the same DiffSuppressFunc, because it's for Diff Suppression, not Validation. Also, AWS will happily throw Invalid domain name for cases when "." can't be used.
There was a problem hiding this comment.
If I fix that DiffSuppressFunc we can create rule for domain_name = ".":
$ terraform apply
aws_route53_resolver_rule.example: Creating...
arn: "" => "<computed>"
domain_name: "" => "."
name: "" => "terraform-testacc-r53-resolver-foofoo"
owner_id: "" => "<computed>"
rule_type: "" => "SYSTEM"
share_status: "" => "<computed>"
aws_route53_resolver_rule.example: Still creating... (10s elapsed)
aws_route53_resolver_rule.example: Creation complete after 11s (ID: rslvr-rr-00000000000000000)
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.There was a problem hiding this comment.
The other user of suppressRoute53ZoneNameWithTrailingDot(), aws_route43_zone has the same problem with name = "." but nobody would have spotted this as you can't create such a Route 53 Zone:
$ terraform apply
aws_route53_zone.test: Creating...
comment: "" => "Managed by Terraform"
force_destroy: "" => "false"
name: "" => "."
name_servers.#: "" => "<computed>"
vpc_id: "" => "<computed>"
vpc_region: "" => "<computed>"
zone_id: "" => "<computed>"
Error applying plan:
1 error(s) occurred:
* aws_route53_zone.test: 1 error(s) occurred:
* aws_route53_zone.test: error creating Route53 Hosted Zone: InvalidDomainName: .
status code: 400, request id: d9c6051b-41da-11e9-83dd-b3dabfee8d3aThere was a problem hiding this comment.
suppressRoute53ZoneNameWithTrailingDot() fixed and unit tests added.
Acceptance tests re-run:
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverRule_ -timeout 120m
=== RUN TestAccAwsRoute53ResolverRule_basic
=== PAUSE TestAccAwsRoute53ResolverRule_basic
=== RUN TestAccAwsRoute53ResolverRule_tags
=== PAUSE TestAccAwsRoute53ResolverRule_tags
=== RUN TestAccAwsRoute53ResolverRule_updateName
=== PAUSE TestAccAwsRoute53ResolverRule_updateName
=== RUN TestAccAwsRoute53ResolverRule_forward
=== PAUSE TestAccAwsRoute53ResolverRule_forward
=== CONT TestAccAwsRoute53ResolverRule_basic
=== CONT TestAccAwsRoute53ResolverRule_forward
=== CONT TestAccAwsRoute53ResolverRule_updateName
=== CONT TestAccAwsRoute53ResolverRule_tags
--- PASS: TestAccAwsRoute53ResolverRule_basic (37.16s)
--- PASS: TestAccAwsRoute53ResolverRule_updateName (57.16s)
--- PASS: TestAccAwsRoute53ResolverRule_tags (57.59s)
--- PASS: TestAccAwsRoute53ResolverRule_forward (384.63s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 384.647s
|
I changed the security group rules, which forced a new endpoint, but it didn't disassociate the resolver_rules from the endpoint first and failed. plan did show the resolver_rules were tainted resolver_rules should be disassociated before changing the endpoint. A |
There was a problem hiding this comment.
Hi @ewbankkit this looks good. I left one comment around re-ordering the sidebar, but I can address that once merged.
@mikecook thanks for testing out the resolver resources and calling out potential bugs. For this particular case would you mind sharing a gist with your redacted Terraform configuration? Or maybe opening an issue? This sounds like an issue with the |
|
As an example, Maybe something like that is needed for aws_route53_resolver_rule? It correctly identified that the It did not correctly do/order the operations, since the |
|
This is because |
|
The issue is that although |
|
Added the $ make testacc TEST=./aws/ TESTARGS='-run=TestAccAwsRoute53ResolverRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAwsRoute53ResolverRule_ -timeout 120m
=== RUN TestAccAwsRoute53ResolverRule_basic
=== PAUSE TestAccAwsRoute53ResolverRule_basic
=== RUN TestAccAwsRoute53ResolverRule_tags
=== PAUSE TestAccAwsRoute53ResolverRule_tags
=== RUN TestAccAwsRoute53ResolverRule_updateName
=== PAUSE TestAccAwsRoute53ResolverRule_updateName
=== RUN TestAccAwsRoute53ResolverRule_forward
=== PAUSE TestAccAwsRoute53ResolverRule_forward
=== RUN TestAccAwsRoute53ResolverRule_forwardEndpointRecreate
=== PAUSE TestAccAwsRoute53ResolverRule_forwardEndpointRecreate
=== CONT TestAccAwsRoute53ResolverRule_basic
=== CONT TestAccAwsRoute53ResolverRule_forward
=== CONT TestAccAwsRoute53ResolverRule_updateName
=== CONT TestAccAwsRoute53ResolverRule_tags
=== CONT TestAccAwsRoute53ResolverRule_forwardEndpointRecreate
--- PASS: TestAccAwsRoute53ResolverRule_basic (39.53s)
--- PASS: TestAccAwsRoute53ResolverRule_updateName (62.52s)
--- PASS: TestAccAwsRoute53ResolverRule_tags (64.66s)
--- PASS: TestAccAwsRoute53ResolverRule_forward (425.78s)
--- PASS: TestAccAwsRoute53ResolverRule_forwardEndpointRecreate (740.00s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 740.053s |
|
@ewbankkit @mikecook my apologies for not being able to get back sooner to your latest comments. Thank you for carrying the PR along. I will pull down the latest and give this another look over. @ewbankkit thanks for updating the sidebar.
I called out the |
|
@nywilken, @ewbankkit resolved that issue with his latest patch. I've successfully tested his change and it fixes the issue of an endpoint changing when there are rules referencing that endpoint. The output shows a var because I made a local This is really great work @ewbankkit, thanks! |
|
@nywilken You are correct, Terraform is now detecting the recreation of the Resolver Rule correctly (after this commit) and |
|
@ewbankkit thanks for all the great work here. @mikecook thanks for testing with your infrastructure and validating the functionality. 💯 💯 The two resources have been merged and will be released in version v2.2.0 of the provider. |
|
This has been released in version 2.2.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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! |
ghost
locked and limited conversation to collaborators
Fixes #6550.
Completes the work started in #6571.
Finishes the tasks in #6525.
Acceptance tests: