provider/aws: add support for geolocation and latency records to route53

[acm1]

This should fulfill the feature request in #2746.

[robzienert]

👍

[apparentlymart]

The usual convention is to have the Terraform properties match with the AWS API or UI labels for things, but this name seems rather confusing and I think you agree since you put a comment on it trying to elaborate. 😀

Perhaps this should be called latency_routing_region. My intent with this name is to indicate that we're giving the region that will be used for latency routing, and not (as would normally be the case where "region" is used elsewhere in Terraform) where this record resource will be hosted.

Alternatively, riffing off how this is arranged in the Route53 UI, I might consider having a routing_policy child block that contains all of these routing policy options. This is similar to how it's placed in a separate box in the Route53 console, distinct from the TTL and value.

resource "aws_route53_record" "foo" {
    zone_id = "${aws_route53_zone.primary.zone_id}"
    name = "www.example.com"
    type = "A"
    ttl = "300"
    records = ["${aws_eip.lb.public_ip}"]

    routing_policy {
        region = "us-west-2"
    }
}

Then at least it's a bit clearer that this region is routing-related. Your other set of mutually-exclusive fields

That last alternative still misses the "routing policy" field from the UI where you choose a type of routing, indicating that this is latency-based routing, so maybe this is better:

resource "aws_route53_record" "foo" {
    zone_id = "${aws_route53_zone.primary.zone_id}"
    name = "www.example.com"
    type = "A"
    ttl = "300"
    records = ["${aws_eip.lb.public_ip}"]

    latency_routing_policy {
        region = "us-west-2"
    }
}

...and then there could be other mutually-exclusive child blocks for weighted_routing_policy, failover_routing_policy and geolocation_routing_policy, each containing the set of fields that gets displayed when you select the corresponding option for routing policy in the Route53 console.

Not sure which of these options I like the best. I think I lean towards the latter since I think it's most user-friendly to prefer to mimic the AWS UI rather than the API when they are in conflict, but YMMV.

[apparentlymart]

Looking forward to having these features available in Terraform! Thanks for doing this.

If you've got the time it'd be nice to also update the website docs for this resource, which you can find in website/source/docs/providers/aws/r/route53_record.html.markdown.

[catsby]

I agree with @apparentlymart regarding the naming of the region attribute, what do you think the suggestions made, @acm1 ?

[farridav]

+1 Thanks for putting this up, keen to see it in.. as Ill be needing this feature very soon

[farridav]

*bump .. any movement on this @acm1 ? ... Thanks

[acm1]

I'll take a look at getting @apparentlymart's suggestions into the PR this week.

[farridav]

Awesome, thanks @acm1

[shanielh]

Looking forward for this one to be merged

[catsby]

Hey @acm1 any chance you're still working on this?

[catsby]

Another follow up here, sorry...

These additions look very promising and there seems to be good desire from the community to have, so thank you for the start!

In order to merge this PR I'd like to see the following:

• Change to the block style that @apparentlymart had suggested
• Acceptance tests for the behavior
• Documentation for the new attributes

If you have questions on how to convert to the block style, let us know! You should be able to use Route53's alias attribute as a reference.

For acceptance tests, you can probably use the same alias's test as an example:

• https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_route53_record_test.go#L194

For documentation, look in the website folder in the root of the Terraform project, and the README will help get you started on running the site locally. From there, it's adding documentation in Markdown format

• https://github.com/hashicorp/terraform/tree/master/website

Let us know if you have other questions!

[acm1]

This one got away from me. I'll dig into it this week -- thanks for the guidance @catsby.

[catsby]

Great to hear @acm1 , thanks! Let me know if you have questions

[acm1]

@catsby I've pushed changes to implement @apparentlymart's suggested block style along with acceptance tests and documentation. Let me know what you think.

[TheLinuxNinja]

Has this been released, yet?

[catsby]

@TheLinuxNinja no. I recently refactored Route53 Records internals a bit so this will need a rebase (sorry about that, but it was necessary)

Let me know if you (@acm1) will be able to do that, thanks!

[acm1]

@catsby I've rebased.

[catsby]

excellent, thanks! the Travis error is a thing being worked out, so I should merge this early next week

[acm1]

Besides my code comment above, I was thinking maybe it makes more sense to put set_identifier inside the routing blocks instead of outside. E.g.

resource "aws_route53_record" "foo" {
    ...
    latency_routing_policy {
        set_identifier = "Oregon"
        region = "us-west-2"
    }
}

vs.

resource "aws_route53_record" "foo" {
    ...
    set_identifier = "Oregon"
    latency_routing_policy {
        region = "us-west-2"
    }
}

Thoughts?

[oli-g]

When this is going to be merged?

[catsby]

Hey all – I'm re-reviewing this now/today. I need to think on some things as this will introduce a backwards incompatibility regarding weighted records that we already have.

@acm1 regarding the set_identifier, I think we're OK to leave it out of the block as it is now. My understanding is that it's applicable to all of these policy types, and each individual record can only have one of those types (weighted, latency, etc)

[acm1]

I'm thinking I should leave this line in so that terraform doesn't try to modify every route53 resource already on record since the default value changes from -1 to 0 if this line is removed.

[catsby]

We can address this in a migration, e.g:

• https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_security_group_rule_migrate.go

We'll need to do that for all -1 entries.

[romansky]

Any news on this front?

[oli-g]

I think a lot of people needs this feature!

[simple-guy]

+1

[acm1]

@catsby I've reworked this a bit and addressed your comments by adding a state migration and checking for errors when setting existing values. Thoughts?

[gechr]

Just compiled/tested this and can confirm the new geolocation_routing_policy works as advertised.

@catsby From your point of view, what's left here before this can be merged? Keen to move back to an upstream branch, rather than relying on @acm1's fork! 😄

[catsby]

Hey all – I'm going to continue this work in #6954 ! Thank you @acm1 !

[catsby]

#6954 was just merged! Thank you @acm1 and I sincerely apologize for the long, long, long delay in getting this in 😄

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.