Update AWS ASG termination policy code and tests

[pforman]

The initial PR to handle AWS autoscaling group termination policy was
unfinished (#506). It only worked on "create", and so had a needless
ForceNew that would rebuild autoscaling groups on any policy change.

It also used a HashString set, so it didn't preserve ordering of multiple
policies correctly. AWS applies termination policies in order, so that matters.

I added the "update" operation, and converted to a TypeList to preserve
ordering. In addition, removing the policy or setting it to a null list
will reset the policy to "Default", the standard AWS policy.

I updated the acceptance tests to verify the update code, but the null case is
difficult to test automatically. I manually verified correct behavior of everything
on a sample ASG as well.

The "Default" setting code might benefit from some polishing, Go is not one
of my best languages. That code's necessity may not be obvious, so I
commented it and added a debug-level log line for least surprise.

[catsby]

Hey @pforman – thanks for the contribution! It looks great from here, and seems more of a bug fix than a feature 😄 . There is an issue however, in that changing the type from Set to List is going to cause all users to see a diff on their first Terraform run after upgrading. From my limited testing, the change is idempotent, but it still may be a surprise to people.

It may be possible to avoid that with a migration, I'll take a look...

[pforman]

Yes, there is a change there, where the termination_policies go from their hashed values to 0,1,2 of the list. An example is in the acceptance tests. This should only affect users who have set "termination_policies" existing.

If there's a way to migrate those that would be great. They're currently applied to the API in hash order, so just mapping those values in order to 0,1,2 as necessary would work without rearranging existing policies.

Since it's just an update now without the ForceNew it doesn't have drastic effects either way, but would be nicer to the users to handle it behind the scenes.

This is definitely more of a bug fix than a feature, most of it is "completion of an existing feature".

I remembered after submitting that I didn't mention docs. I don't think any updates to the docs are necessary, as the way I went down this rabbit hole was that the behavior I observed didn't match what I expected from the documentation.

Let me know if I can help out or if you need anything else for this PR.

[patricklucas]

This is affecting me as well—any word when this could be merged?

[catsby]

Sorry for the delay – I plan to revisit this PR this week. I'm not sure it will be merged as-is, but I do hope to merge this week

[catsby]

We should fix #3052 in the PR as well, if we're not already...

[pforman]

#3052 should already be fixed in the implementation of the "update" operation. We'll read back the list and if it's different an update is issued. Changing from Set to List will preserve ordering now as well.

I also handled the degenerate case of removing the policy or setting it to an empty list (not allowed in AWS), in which case it's reset to "Default".

Thanks for looking into this again!

[nick-parry]

I would love to see this.

[JPodz]

+1 for getting this merged in.

[elblivion]

Awesome! Just ran into exactly the same problem. Looking forward to the merge!

[radeksimko]

Just a quick question - why is TypeSet unwanted here?

TypeList will introduce another issue if we won't sort the list each time we save output from the API (which is AFAIK not happening in this PR). API may return the list in different order and Terraform will try to change it back. TypeSet uses hashes of strings to sort the list which solves this issue.

I think the original problem was also not saving the data properly - i.e. as a hash.

This

d.Set("termination_policies", g.TerminationPolicies)

should be changed to something like this:

d.Set("termination_policies", schema.NewSet(schema.HashString, flattenStringList(g.TerminationPolicies)))

whereas flattenStringList comes from here. That d.Set isn't so magical, that it would dereference a list of pointers I'm afraid and I'm not sure if it should be so magical (?)

@catsby btw. following our recent discussion about (not) catching errors -> it would be useful to actually check error in this specific Set I think?

[pforman]

As I understand it, based on http://docs.aws.amazon.com/AutoScaling/latest/DeveloperGuide/AutoScalingBehavior.InstanceTermination.html#custom-termination-policy, if there are multiple policies the order is significant.

The relevant part:
'Choose one or more termination policies. If you choose multiple policies, list them in the order that you would like them to apply. If you use the Default policy, make it the last one in the list.'

That's why I changed the type from Set to List, specifically to preserve the ordering. As far as I know (based on the testing I did), the API does return them in order. The old code demonstrably broke ordering if there were multiple policies, using the hash set order instead of the specified order.

On re-reading that, I can definitely recall that I didn't add a special case to ensure that Default is the last in the list. I'm not sure if Terraform policy should be enforcing that or not.

Thanks for looking back into this! I figured the period of quiet was due to the prep for Hashiconf.

If there's anything else you need, just let me know. I may go quiet a bit in the next two weeks (re:Invent and then vacation), but I'm happy to do whatever to get this polished up and accepted.

[radeksimko]

Ah, you're right about the strict order, I should RTFM next time, before jumping to the comment 👊

On re-reading that, I can definitely recall that I didn't add a special case to ensure that Default is the last in the list. I'm not sure if Terraform policy should be enforcing that or not.

I don't think there's currently any nice way how to enforce this. We have ValidateFunc, but that only works for primitive types at the moment.

[pforman]

No problem, usually a Set is the right choice. I'm struggling to think of anywhere else that order matters in the AWS API.

It appears that AWS is picky about this as well. The API does enforce that ordering, and Default must be last if it exists. The patch as is will produce a clean plan with this config:

termination_policies = ["Default", "OldestInstance" ]

but an apply results in

* aws_autoscaling_group.main_asg: Error updating Autoscaling group: ValidationError: If Default termination policy is specified, it must be at the end of list of termination policies.
    status code: 400, request id: [1b698f72-6a57-11e5-ab26-6312edcd2335]

I'm not sure how to handle this. I think the PR as-is is much more correct than the existing code, but without ValidateFunc being able to check the contents of a List it's going to leave an issue (although a correctly reported and easily fixed issue). Is there a plan to extend ValidateFunc in the future?

I think that merging and then adding a deferred issue about the Default oddity will solve most people's problems with termination_policies, but I also understand if you'd prefer to wait.

[catsby]

Sorry for the long silence here!
We're going to merge this as is, thank you for the contribution!

We're going to make note that this will (likely) introduce some unexpected plans, but they're necessary to correct the ordering as specified.

Thanks again!

[pforman]

Great, thanks. I was just thinking I should write a clarification post, since the thread had gone stale again.

The "Default must be last" thing is a wart, but that was already broken in the existing code. If ValidateFunc is extended in the future it will be an easy fix. Until then, "don't do that" is probably sufficient.

Thanks for all the work on Terraform, I'm excited to keep seeing more API features checked off!

[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.