How to handle deleting machine.cluster.x-k8s.io/certificates-expiry annotation on the kubeadmconfig

[ykakarap]

Detailed Description

This issue is to discuss how to handle user deleting the machine.cluster.x-k8s.io/certificates-expiry annotation on KubeadmConfig.

The option proposed was:

• Block the deletion of the annotation in the webhook of KubeadmConfig.
• Simply document the need and use of annotation

Ref: #6983 (comment)

[sbueringer]

Just wanted to add that when we implement in-place propagation of labels/annotations we have to take care that we don't overwrite this annotation on the Machine and on the KubeadmConfig. But I guess that's already a requirement there for any additional labels/annotations anyway.

Back to topic: I think validating in the validate update hook of KubeadmConfig that this annotation can never be removed once it has been added is the ideal solution. It protects against users accidentally deleting the annotation and against bugs in our implementation.

I think I wouldn't do anything for the Machine so folks can do whatever they want there.

[fabriziopandini]

+1 to prevent users from removing / modifying the annotation
/triage accepted

[killianmuldoon]

Aside: Could the same be done for other pieces of metadata that CAPI relies on as a source of truth? e.g. the topologyOwned label?

[sbueringer]

For the ones that should be immutable - yes

[fabriziopandini]

thinking a little bit more about this.
I'm not sure we should go down the path of making labels immutable (it makes sense, but even in KK they are not, and also this goes behind the scope of this issue), so this is a broader discussion.

The problem with this specific issue is that we should prevent the annotation to be created with the wrong expiration date, which can lead to certificate expiration. An idea could be:
if the data secret has been already generated, and the annotation is missing, set a condition with a warning

[fabriziopandini]

in this use case API server warnings would be an even better solution because they give an immediate feedback to the users, but we have to wait for controller runtime to support them

[vincepri]

While can be a problem, there are a number of similar issues where deleting a label or annotation could result in the system not behaving accordingly. While we could go down the path of preventing the annotation to be removed through the webhook (which I'm fine with), we could also choose to keep it as-is and document the side effects in case that happens

[ykakarap]

we could also choose to keep it as-is and document the side effects in case that happens

We dont explicitly call it out now, but I think we should add a section about this in the new Certificate Renewal doc that we are adding. Ref PR: #7268

[sbueringer]

I think we don't care about this case anymore, given that KCP will just add it again on the next reconcile

@ykakarap @fabriziopandini WDYT, can we close this issue?

[fabriziopandini]

makes sense for me
/close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

makes sense for me
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.