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

Updating region on a cluster is broken #1673

Closed
benmoss opened this issue Mar 30, 2020 · 5 comments · Fixed by #1677
Closed

Updating region on a cluster is broken #1673

benmoss opened this issue Mar 30, 2020 · 5 comments · Fixed by #1677
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@benmoss
Copy link

benmoss commented Mar 30, 2020

/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
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 30, 2020
@detiber
Copy link
Member

detiber commented Mar 30, 2020

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

@k8s-ci-robot k8s-ci-robot added this to the v0.5.x milestone Mar 30, 2020
@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 30, 2020
@benmoss
Copy link
Author

benmoss commented Mar 30, 2020

/assign

@benmoss
Copy link
Author

benmoss commented Mar 30, 2020

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
Copy link
Member

detiber commented Mar 30, 2020

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
Copy link
Author

benmoss commented Mar 30, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants