Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Fix issue with Route53 and zero weighted records #4427

Merged
merged 3 commits into from
Jan 4, 2016

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Dec 22, 2015

Route 53 allows weighted records, with a weight of 0 meaning various things depending on the context.

Terraform's internal can sometimes confuse 0 for "not specified", which is incorrect in this case. As a result, a 0 weight was not possible... UNTIL NOW!

Here we default the weight to -1 as a sentinel value*. A record uses this default when the user does not specify weight in the record. A weight of -1 is ignored.


*probably not a true sentinel value

@phinze
Copy link
Contributor

phinze commented Dec 22, 2015

*probably not a true sentinel value

Totally a sentinel value!

This LGTM - recommend updating the docs to with a similar note about the default and its meaning.

default in Terraform. This allows Terraform to distinquish between a `0` value
and an empty value in the configuration (none specified). As a result, a
`weight` of `-1` will be present in the statefile if `weight` is omitted in the
configuraiton.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/configuraiton/configuration/

@phinze
Copy link
Contributor

phinze commented Jan 4, 2016

One typo to fix and this looks good to go.

catsby added a commit that referenced this pull request Jan 4, 2016
provider/aws: Fix issue with Route53 and zero weighted records
@catsby catsby merged commit 25238ec into master Jan 4, 2016
@catsby catsby deleted the b-aws-r53-weights branch January 4, 2016 16:05
@scalp42
Copy link
Contributor

scalp42 commented Jan 19, 2016

@phinze or @catsby would that cause Terraform to delete and recreate the record ? Just brought chaos.

@phinze
Copy link
Contributor

phinze commented Jan 19, 2016

@scalp42 for aws_route53_record resources with weight unset, you'd see a diff going from "" => "-1" on this field, but the field is not ForceNew, so it should only trigger a ~ / update in the plan, and the update itself should be a noop.

Did you see different behavior here?

@scalp42
Copy link
Contributor

scalp42 commented Jan 19, 2016

I saw "0" => "-1" and we were down for couple seconds following the apply so I'm assuming it deleted/re-created the record.

We had the same issue with SGs where Terraform was deleting/re-creating rules with no changes (causing havoc again), so I'm assuming it's the same case.

@phinze
Copy link
Contributor

phinze commented Jan 20, 2016

Hm that's definitely not expected. Did the plan have a ~ or a -/+? If the plan shows ~ then the apply can't do a destroy/recreate out from under you. You'll also see Destroying... messages in the apply output if it does any recreating.

If you file a fresh issue with all the information you have, we can to dig in further to figure out what happened.

@scalp42
Copy link
Contributor

scalp42 commented Jan 20, 2016

@phinze Unfortunately I don't have the output anymore 🍶

Thanks for the feedback though.

@phinze
Copy link
Contributor

phinze commented Jan 20, 2016

Ah dang, too bad. Anytime you catch unexpected behavior definitely follow up with a fresh issue and we can help you run it down. 👍

@ghost
Copy link

ghost commented Apr 28, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants