✨ Allow credentials in multiple clusters

[lubedacht]

What is the purpose of this pull request/Why do we need it?

As we would like to allow referencing the same secret in multiple clusters, we need to make sure to not delete it when it is still in use and set the owner references accordingly.

Description of changes:

Updates the finalizer logic to ensure that a finalizer is not removed as long as the secret is referenced by multiple owners.

Special notes for your reviewer:

• An object in Kubernetes can only have one controller reference, but multiple owner references.

Checklist:

• Includes emojis

[mcbenjemaa]

LGTM

[piepmatz]

After the previous PR I thought you explicitly didn't want to allow that, but fine, I like it better this way.

I just think that the solution is not clean: As soon as the first cluster using the secret is deleted, the secret will forever have a deletionTimestamp, even though it can continue to exist forever.

This combination of setting owners ("please delete me") and finalizers ("but not yet") is not a clean design.

I have no clean alternative at hand though, that would achieve the same goals.
But it would be clean to say:

• We have a 1-to-1 mapping of secret and cluster and take care of the secret deletion.
• The secret is not bound to the cluster lifecycle and we don't delete it.

Any mixture of these seems problematic, I'm afraid.

[piepmatz]

From a UX point of view I think having a shared secret is more valuable than automatic secret deletion. But in that case our template shouldn't use the cluster name as part of the secret name.

[mcbenjemaa]

For security reasons, I don't suggest using 1:1 mapping

While shared secret make sense,
We can probably support both use cases.
either the secret will be controlled by finalizer and owner references and will be cleaned up, by setting an annotation in the cluster.

On the other hand, if no annotation provided secret will not be bound. And we don't engage with secret at all,
No finalizer, no OwnerReference.

I think there might be ephemeral environments where we need this feature.

What do you?

[piepmatz]

For security reasons, I don't suggest using 1:1 mapping

Can you please elaborate on the security reasons you had in mind?

We can probably support both use cases [...] by setting an annotation in the cluster.

Annotations to control behavior usually is a hack to work around a lack of options. Coming up with hacks at this early stage doesn't feel right. ;)

Even with the annotation misconfiguration is easy: Nobody stops you from creating a cluster w/o that annotation, but reusing the secret. The secret might then disappear at any time.
Moreover, the annotation does not solve the broken design of intentionally keeping around a secret having a deletionTimestamp.

[lubedacht]

One thing. The secret will not have a deletion timestamp. It will only have a deletion timestamp if you actually attempt to explicitly delete it with kubectl delete secret or kubectl delete -f cluster-manifest.

In other cases the secret will simply be garbage collected when no owner references exist anymore. The finalizer should only be a last resort to prevent the accidental deletion of a secret while it is being used by another cluster.

[piepmatz]

Good to know, thx, my understand of the consequences of having multiple owners was wrong then.

Then something like this could work. However, the current approach is subject to race conditions. With bad timing it can happen that all owners are gone but the finalizer is still present.
We could solve that by individual finalizers: Each cluster has its own finalizer that it always sets and removes, regardless of the number of owners.
A nice side effect is that looking at the secret shows which clusters rely on it. We use this approach elsewhere successfully.

[jriedel-ionos]

@lubedacht @piepmatz I haven't found a cleaner way to do it actually, I'm open for ideas, please.

[lubedacht]

Last changes from @jriedel-ionos are approved from my side

[avorima]

One thing. The secret will not have a deletion timestamp. It will only have a deletion timestamp if you actually attempt to explicitly delete it with kubectl delete secret or kubectl delete -f cluster-manifest.

In other cases the secret will simply be garbage collected when no owner references exist anymore. The finalizer should only be a last resort to prevent the accidental deletion of a secret while it is being used by another cluster.

This is not true. I just tested this with secrets and as soon as the finalizer from the parent was cleared and it was deleted the child also had a deletion timestamp.

apiVersion: v1
kind: Secret
metadata:
  creationTimestamp: "2024-05-13T10:51:49Z"
  deletionGracePeriodSeconds: 0
  deletionTimestamp: "2024-05-13T10:54:24Z"
  finalizers:
  - block/deletion
  name: child
  namespace: default
  ownerReferences:
  - apiVersion: v1
    kind: Secret
    name: parent
    uid: ad709537-3997-43bf-aeb4-cd8f87551dce
  resourceVersion: "35060329346"
  uid: d640f2ce-e784-4f7a-9bd0-8624a23314e8
type: Opaque

Sick leave

[avorima]

please update the comment on ensureSecretControlledByCluster

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud