Cluster-api-provider-aws: Updating region on a cluster is broken

[benmoss]

/kind bug

What steps did you take and what happened:
Updating the spec.Region on an AWSCluster results in the cluster going into a broken state. I don't think we want to support changing a cluster's region at all, so we probably should enforce that it is an immutable field.

What did you expect to happen:
Either the cluster tears down resources in one region and recreates them in another, or more plausibly refuses to allow users to change the region.

Environment:

• Cluster-api-provider-aws version: master
• Kubernetes version: (use kubectl version): 1.17.3

[detiber]

+1 to adding validation to enforce this field being immutable
/priority important-soon
/milestone v0.5.x

[benmoss]

/assign

[benmoss]

We don't have any validations on AWSClusters at the moment, so I was thinking of what else we want to restrict:

• networkSpec: Only allow updating this if its unmanaged?
• sshKeyName: probably should be immutable
• controlPlaneEndpoint: same as networkSpec?
• additionalTags: do we support updating tags on already created instances?
• controlPlaneLoadBalancer: pretty sure we don't handle this now but it seems feasible we could change the scheme. Maybe make it immutable until we decide to implement it?
• imageLookupOrg: this seems slightly hard, since it's possible defaults have already been applied to running instances. I'd be somewhat inclined to let this be "caveat emptor", let people change it but don't do anything wild like change existing instances.
• imageLookupBaseOS: same as above
• bastion: same as controlPlaneLoadBalancer, in theory we could tear down the bastion but since that's more work we can just validate it until we implement that

[detiber]

networkSpec: Only allow updating this if its unmanaged?

It might be tricky to determine managed vs unmanaged here.

sshKeyName: probably should be immutable

It should be fine to update this afterwards, it would just change the cluster-wide default for new instances created.

controlPlaneEndpoint: same as networkSpec?

+1 on immutability here, otherwise it will break the cluster.

additionalTags: do we support updating tags on already created instances?

Sort of... I'm not sure if it will affect existing instances created (other than bastion), but it should affect the cluster infrastructure that was created. We might need to figure out what type of behavior we want here for existing AWSMachine owned instances, though.

controlPlaneLoadBalancer: pretty sure we don't handle this now but it seems feasible we could ?change the scheme. Maybe make it immutable until we decide to implement it?

immutability for now sounds good here to me.

imageLookupOrg: this seems slightly hard, since it's possible defaults have already been applied to running instances. I'd be somewhat inclined to let this be "caveat emptor", let people change it but don't do anything wild like change existing instances.

Yes, we should likely allow for changing this to affect the default behavior for future instance creation and it should not impact existing instances.

imageLookupBaseOS: same as above

Yes

bastion: same as controlPlaneLoadBalancer, in theory we could tear down the bastion but since that's more work we can just validate it until we implement that

We should allow for the addition of a bastion after the AWSCluster is created, but should likely block the removal of a bastion.

[benmoss]

Sounds good, I was starting to look into managed/unmanaged and realized that wouldn't be easy. I think I was split on whether a stance of "this applies to new instances going forward" vs "this should be reconciled as if I applied this when I created the cluster" was the what users would want / expect. I think the former is good enough to start until there's a real demand.