Invalid values provided for CloudFormation UpdatePolicy template

[aclevername]

What were you trying to accomplish?
Scale a nodegroup

What happened?
Scaling failed as we recently introduced parsing of templates using CloudFormation template struct, rather than just assuming the json/yaml response back from CloudFormation was valid #3170. failed to parse GetStackTemplate response: json: cannot unmarshal string into Go struct field Template.Resources of type float64 ERROR: json: cannot unmarshal string into Go struct field AutoScalingRollingUpdate.UpdatePolicy.AutoScalingRollingUpdate.MaxBatchSize of type float64

How to reproduce it?
Attempt to scale a nodegroup using latest commit on master f3fe3a3

Cause of the problem

We provide string values for the MaxBatchSize and MinInstancesInService fields in the CloudFormation UpdatePolicy template but its expecting integer values, this has been in since the very beginning of eksctl #132.

https://github.com/weaveworks/eksctl/blob/71e49b5b39759e549fc95fad8d1039b4f5afa373/pkg/cfn/builder/nodegroup.go#L346-L349

This hasn't caused us any issues so far, so I'm unsure if these fields are really necessary to set.

If I attempt to set the values to what should be invalid in both string and int form, nothing occurs. For example I set the MinInstancesInService value to 10, for a nodegroup whos min/desired/max were all below that. I then attempted to scale a stack that had the value in string and another in int, no errors occurred. It might be that behind the scenes it's failing and defaulting back to the default values of 0 for MinInstancesInService and 1 for MaxBatchSize (which is what we are trying to set them to anyway). Note to attempt the above with string values I manually modified the goformation, see weaveworks/goformation@604a018.

If I edit the CLI not to set these two fields and create a nodegroup it succeeds and can be scaled successfully. Worth noting that the docs say it defaults the value to 0 and 1, but it doesn't actually set that in the template so it must be defaulting somewhere else behind the scenes.

How to proceed

We have two issues to address:

1. Incorrect values being provided to UpdatePolicy template
2. How we handle parsing templates

Solution A

We stop setting values for MaxBatchSize and MinInstancesInService in the UpdatePolicy. It seems like it hasn't been doing anything this whole time, so removing it is an option. If we remove it we could then update our goformation library to expect string values for MaxBatchSize and MinInstancesInService, this should fix the parsing problems. Changing the goformation library is quite a minimal change: weaveworks/goformation@604a018

Solution B

We update our code to set the int values for UpdatePolicy and introduce a migration codepath to change existing nodegroups to have the correct values. The problem with this is how we handle parsing the templates, as we now know that we will have to deal with a mix of string and int values.

Logs

./eksctl scale nodegroup --cluster jk-cf --name ng-updated-10-str --nodes-min 2 --nodes 3 --nodes-max 4
[ℹ]  scaling nodegroup "ng-updated-10-str" in cluster jk-cf
Error: failed to scale nodegroup for cluster "jk-cf", error: error getting stack template eksctl-jk-cf-nodegroup-ng-updated-10-str: failed to parse GetStackTemplate response: json: cannot unm
arshal string into Go struct field Template.Resources of type float64
ERROR: json: cannot unmarshal string into Go struct field AutoScalingRollingUpdate.UpdatePolicy.AutoScalingRollingUpdate.MaxBatchSize of type float64

Anything else we need to know?

Versions
eksctl built off master f3fe3a3

[aclevername]

I've opened #3193 encase we don't fix this before we intend to release

[aclevername]

@weaveworks/eksctl-reviewers thoughts? I'm leaning with solution A atm as its a quick fix.

[michaelbeaumont]

I think better would be to replace them with goformation types.Value but solution A otherwise.

[aclevername]

I think better would be to replace them with goformation types.Value but solution A otherwise.

Do you mean setting the type as types.Value instead of string in solution A?

[michaelbeaumont]

sorry, yeah exactly