✨ bastion should be mutable

[bavarianbidi]

/kind feature

Describe the solution you'd like
tl;dr: changing values in OpenStackCluster.Spec.Bastion should be possible without doing the disable/enable with new spec foo

We know the fact, that it's currently possible to e.g. change the image of the bastion host by disabling the host and afterwards re-enabling the host with the new spec again.
But this circumstance doesn't really fit in a git-ops approach.

From a admin/user perspective It should be possible to change values in OpenStackCluster.Spec.Bastion.Instance + OpenStackCluster.Spec.Bastion.AvailabilityZone and rely on CAPO that the bastion host in the desired state got created.

Anything else you would like to add:

From what i've seen so far:

• this is a feature which is not tracked in a separate issue (maybe this could used for that)
• that's something which is wanted but not implemented for some reasons

  • most of the discussion regarding the current implementation was done in ✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070 and docs: Explain OpenStackCluster.Spec.Bastion mutability in CRD #1180

    Matthew already proposed a possible solution in docs: Explain OpenStackCluster.Spec.Bastion mutability in CRD #1180 (at first glance i can't see anything bad about it):

    Yes, I think I still stand by my older comment. If it doesn't work we should reflect that in the API. It's also user-hostile to pretend something works if it doesn't.

    It would be best if it did work, of course. Do CAPA and CAPZ have a solution for this? There's always the solution of hashing the bastion spec and storing it in the status. If the status hash doesn't match the current hash you delete and recreate. Is there a neater solution?

    discussion in ✨ Allow webhook changes to OpenStackCluster.Spec.Bastion #1070 starts here

    Matthew:

    This is an interesting one. I note that in this unit test we're changing both the bastion spec and the enabled flag. This is fine: we will create a new Bastion with the new spec.

    However, note we also permit changing the bastion spec without flipping enabled, but this won't work. It won't fail, but the bastion will not be updated until the enabled flag is flipped to false and the bastion is deleted.

    What do we want to do about this? Options:

    1. Ignore it
    2. Document it
    3. Don't allow changing bastion spec if status.Bastion != nil
    4. Make it work

    For now I feel like 2 + 3 would be user friendly and relatively simple.

    Tobias:

    I totally agree that we should document this behavior. Regarding 3: do you think it's worth the effort to update our reconcileBastion to delete the bastion if sth has changed? Or is it enough to only allow changes if the enabled flag is false? Both are fine for me, I just think from the customer's perspective. WDYT?

    Matthew:

    I feel like automatically deleting and recreating the bastion if something changed would be the most user friendly, but I should strongly caveat that with: I don't feel like I understand all the potential use cases here. Have we firmly set an expectation that we can blow the bastion away? Nobody's running anything on the bastion which is going to suffer data loss? I'd hope not.

    I also think it would be fine to do 3 and do 4 later. I don't think they're incompatible: if the user writes orchestration to delete and recreate the bastion if they need to change it, that will continue to work even if we remove the requirement to do it. We're also being nice to them in the meantime by pointing out that it doesn't work.

[bavarianbidi]

cc @kopiczko / @erkanerol

[tobiasgiese]

We already discussed this in #1070 (comment)

But let's discuss it here again with a larger audience

already in the description :)

[kopiczko]

Just for the context why we would like to have it:

We have plenty clusters in different providers and we have bastions enabled at all times to make debugging possible. We have tooling built to be able to SSH to a cluster. The tooling doesn't know about the provider and we would like it to stay that way. And bastion enabling in CAPO is very provider-specific thing.

We use immutable OS for bastions and of course upgrading the OS for bastion is very important. We are managing clusters with GitOps (at least some of them). Upgrading bastion with GitOps as of right now is 3 commits:

• disable bastion (commit 1)
• update bastion image (commit 2)
• enable bastion (commit 3)

That isn't very declarative approach and it doesn't fit GitOps well. That's why we'd like to have the feature.

I don't think it hurts. If there are folks who want to keep bastions disabled they still can. It doesn't introduce any breaking change in any way AFAICT.

[jichenjc]

We use immutable OS for bastions and of course upgrading the OS for bastion is very important. We are managing clusters with GitOps (at least some of them). Upgrading bastion with GitOps as of right now is 3 commits:

I am +1 to avoid such manual 3 steps in order to do an update to the cluster components like bastion
I Think it make sense to remove the older one then re-create the new one in case anything changed
just like LB or other resource that need

is your use case related to OS upgrade? which is older OS is immutable then sometimes later you want to upgrade the OS image due to compliance etc then gitops changes the image to newer one then you expect to recreate the bastion from new?

[tobiasgiese]

So I think a possible solution is to check all bastion specs against the current bastion. If something has changed a re-creation should be triggered, leaning on this comment from Vince.

I can to take a look at it
/assign

[kopiczko]

Yes once image is changed we'd expect bastion to be recreated. Thanks @tobiasgiese and @jichenjc for looking into this!