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

Use valid names for Finalizers #1450

Closed
7 tasks
guettli opened this issue Aug 16, 2024 · 1 comment
Closed
7 tasks

Use valid names for Finalizers #1450

guettli opened this issue Aug 16, 2024 · 1 comment

Comments

@guettli
Copy link
Contributor

guettli commented Aug 16, 2024

/kind bug

What steps did you take and what happened:

While reading the CAPI issue "Our finalizers are not domain-qualified, and therefore do not conform to requirements" I realized that our Finalizers and Annotations are not valid. The linked issue is about name of finalizers, but the same applies to Annotations.

Annotations usually a domain name (representing the owner) and a name.

mygroup.example.com/finalizier

We used for example

hetznerbaremetalhost.infrastructure.cluster.x-k8s.io

Tasks (for Finalizers)

  • Define new names
  • Create update strategy
  • Write down how to test it (manual).
  • Be sure that the release notes contain an explanation.

Our Finalizers are not part of the public API. We don't document the names.

Tasks (for Annotations)

  • Create a list of all Annotations we use in caph.
  • Define new names
  • Make an upgrade strategy

Related

Kubernetes issue 119445 which introduced the warning in 1.29.

How other projects did the update:

https://github.com/vmware-tanzu/vm-operator/pull/545/files#diff-94266af62f6987e14d8bcc31454437775dc7bafb5d3bed637062867a8d5b84d7R146-R155

Official Finalizers Docs

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers

Identifiers of custom finalizers consist of a domain name, a forward slash and the name of the finalizer

apiVersion: "stable.example.com/v1"
kind: CronTab
metadata:
  finalizers:
  - stable.example.com/finalizer

Update Strategy

At the beginning of the Reconcile() method the code would update the old names to the new names.

I would keep that code for the next releases in caph.

Then we are able to deal with old names, even if the customer skips some versions of caph.

Rejected

We can't use that pattern:

apiVersion: "mygroup.example.com/v1"
kind: Foo
metadata:
  finalizers:
  - mygroup.example.com/finalizer

Because our apiVersion domain is "infrastructure.cluster.x-k8s.io".

This would not reflect, that caph is responsible for the finalizer:

infrastructure.cluster.x-k8s.io/finalizer

old --> new

"hetznercluster" is just an example. We would do that for all finalizers.

old

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: HetznerCluster
metadata:
  finalizers:
  - hetznercluster.infrastructure.cluster.x-k8s.io # <=========

new

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: HetznerCluster
metadata:
  finalizers:
  - infrastructure.cluster.x-k8s.io/hetznercluster # <=========

Manual Testing

Create a cluster with the old version of caph.

Dump all our resources:

#!/bin/bash
# dump-our-yaml.sh 

trap 'echo "ERROR: A command has failed. Exiting the script. Line was ($0:$LINENO): $(sed -n "${LINENO}p" "$0")"; exit 3' ERR
set -Eeuo pipefail

kubectl api-resources -o name | grep -P 'hetzner|hcloud' | while read -r r; do
    kubectl get "$r" -A -o yaml
done

Run it while the original controller is running:

❯ ./dump-our-yaml.sh > step1.yaml

Run it after the new controller was installed. Write to step2.yml

❯ ./dump-our-yaml.sh > step2.yaml

Compare

❯ diff step1.yaml step2.yaml

Have a look at step2.yaml and check all finalizers.

Test deduplication

k edit hcloudmachine your-hcloud-machine

set the finalizers to:

  - infrastructure.cluster.x-k8s.io/hcloudmachine
  - infrastructure.cluster.x-k8s.io/hcloudmachine
  - hcloudmachine.infrastructure.cluster.x-k8s.io
  - hcloudmachine.infrastructure.cluster.x-k8s.io

Save.

Edit again. Now there should be only one entry (the new string).

Test finalizer of secret

 k edit secret hetzner

Test: Delete the cluster.

k delete cluster ...
@guettli guettli changed the title Use valid names for Annotations Use valid names for Annotations (and Finalizers) Aug 19, 2024
@guettli guettli changed the title Use valid names for Annotations (and Finalizers) Use valid names for Finalizers (and Annotations) Aug 20, 2024
guettli added a commit that referenced this issue Aug 20, 2024
old: foo.infrastructure.cluster.x-k8s.io
new: infrastructure.cluster.x-k8s.io/foo

Related: #1450
@guettli guettli changed the title Use valid names for Finalizers (and Annotations) Use valid names for Finalizers Aug 21, 2024
guettli added a commit that referenced this issue Aug 22, 2024
* 🌱 Update names of finalizers.

old: foo.infrastructure.cluster.x-k8s.io
new: infrastructure.cluster.x-k8s.io/foo

Related: #1450
@guettli
Copy link
Contributor Author

guettli commented Aug 29, 2024

Implemented via #1455

@guettli guettli closed this as completed Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant