🐛Change ownership of auto-generated certificate Secrets to the Cluster so they're not deleted alongside a Machine and KubeadmConfig

[rudoi]

What this PR does / why we need it: This changes the owner of cert Secrets from the KubeadmConfig to the Cluster.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #274

[rudoi]

I don't think this func needs to take in config as a param any more?

oooo nice catch! removing params is my favorite.

[ncdc]

I'd also recommend retitling this PR to something like

Change ownership of auto-generated certificate secrets to the Cluster so they're not deleted alongside a Machine and KubeadmConfig

[rudoi]

@ncdc sounds good to me, provided you don't mind my blatant copypasta

[ncdc]

/approve

Do we need to do anything in the code to deal with preexisting clusters that have the ownerRefs set to the KubeadmConfig?

[detiber]

Would it make sense to handle updating of ownerRefs for secrets that currently list the current KubeadmConfig as an owner? Otherwise we should make sure that the release notes include a migration path for existing Clusters.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, rudoi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/blob/master/OWNERS)~~ [ncdc] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[rudoi]

Would it make sense to handle updating of ownerRefs for secrets that currently list the current KubeadmConfig as an owner? Otherwise we should make sure that the release notes include a migration path for existing Clusters.

I think it'd be good to do this in code. Separate issue perhaps?

[ncdc]

It'd be nicer to do it with this PR if you can?

[rudoi]

-sigh- anything for y'all! gimme a few 😄

[chuckha]

/hold

I don't believe certs being owned by the cluster is the correct path here.

There is a fix in amy's branch that addresses the secret deletions by updating the owning kubeadmconfig reference: https://github.com/vmware/cluster-api-upgrade-tool/pull/72/files#diff-4410eeb26984da2b31d9a58c8a250133R361

[detiber]

There is a fix in amy's branch that addresses the secret deletions by updating the owning kubeaconfig reference: https://github.com/vmware/cluster-api-upgrade-tool/pull/72/files#diff-4410eeb26984da2b31d9a58c8a250133R361

One worry I have with this approach is that it only solves the upgrade use case, replacing a failed control plane instance would need similar manual workarounds.

imo, control plane management should help out in this area for v1alpha3+ at least.

[chuckha]

One usecase this does not address is updating serving cert SANs. If the certs are owned by the cluster we have to delete the cluster to get the secrets to delete or delete them by hand. If they are owned by the kubeadmconfig then we can scale the control plane machines down to 0, modify the kubeadmconfig and bring up a control plane machine to generate new updated certificates.

[chuckha]

@rudoi after our discussion yesterday do you still feel this is a necessary change?

[rudoi]

@rudoi after our discussion yesterday do you still feel this is a necessary change?

No, not at this time. I think there's some theorycrafting to be done regarding cert ownership outside of the upgrade flow, but we can probably defer on that.