Deleting the last BMH using a credentials secret, causes the secret to be also deleted.

[raffaelespazzoli]

What steps did you take and what happened:
Deleting the last BMH using a credentials secret, causes the secret to be also deleted.
This is not desirable.

What did you expect to happen:
The secret to be left alone.

Anything else you would like to add:
I don't understand what was the intention with using the owner reference in this case. It does seem unnecessary.

Environment:

• Baremetal Operator version: OCP 4.13
• Environment (metal3-dev-env or other):

/kind bug

[dtantsur]

@zaneb do you remember the logic behind setting the owner reference?

[zaneb]

I think the logic was that we should not leave credentials lying around when they are not used by anything.
Some more information about why it's undesirable would be helpful.

[zaneb]

We also user the owner reference as a reverse link from the Secret to the BMH that owns it, so we can easily tell which BMH(s) need to be reconciled when a Secret is modified without having to search all of them. controller-runtime makes this very simple to set up by just calling Owns(&corev1.Secret{}) on the controller manager, so that may have been part of the reason it was used initially.

[raffaelespazzoli]

the ownership concept in kubernetes generally refers to the fact a controller of resource A has created resource B in response to resource A configuration. So, in a way resource B exists only for the fact that resource A exists and therefore A owns B. The garbage collection logic aligns with this assumption and will delete B on behalf of controller A if A is deleted. This is to avoid writing trivial finalizers.
This is not the case for this BMH controller, you don't create the secret therefore you don't own it. I get that you get some convenient behavior by setting the owner, but what you are doing is conceptually wrong and it is hurting situations where the secret should not be deleted.

[Rozzii]

/cc @kashifest @lentzi90

[Rozzii]

/cc @hardys

[s3rj1k]

This is not the case for this BMH controller, you don't create the secret therefore you don't own it. I get that you get some convenient behavior by setting the owner, but what you are doing is conceptually wrong and it is hurting situations where the secret should not be deleted.

I agree, but what you propose, changing the behavior would be impactful to other people who already used to this behavior.

I am expecting that this would be configurable at least on global scale via CLI flags for BMO but preferably via annotation on BMH.

[lentzi90]

I agree that the current behavior is surprising. Perhaps a global flag would be cleanest?

[Rozzii]

So is there any conclusion as far as I can understand the closest to an acceptable compromise would be to turn this feature into a configurable one right ? @dtantsur @s3rj1k @lentzi90 @raffaelespazzoli @zaneb ?

[hardys]

This was also recently discussed in relation to networkData secrets - it seems we're not consistently setting ownership of those (and presumably meta/userData) so we might want to consider if there's a common (but configurable) way to solve this?

[zaneb]

We do consistently set ownership of user/meta/networkData and BMC Secrets.
We don't set ownership of preprovisioningNetworkData unless we're actually using it, so that's inconsistent.

[zaneb]

We do consistently set ownership of user/meta/networkData and BMC Secrets. We don't set ownership of preprovisioningNetworkData unless we're actually using it, so that's inconsistent.

Actually, scratch that. We do not set ownership of user/meta/networkData because it's not required that we trigger a reconcile when these change.

[zaneb]

So is there any conclusion as far as I can understand the closest to an acceptable compromise would be to turn this feature into a configurable one right ?

TBH I see this as an Open Source antipattern - the developers, who know and understand the software the best, can't agree so they punt the decision to the person least equipped to make it (in this case the operator of the metal³ deployment).

How can we ask the operator to decide at deployment time between timely responses to changes and keeping unreferenced Secrets around? How would they decide if we can't?

Any end user who wants to keep their unreferenced Secrets around is free to add another owner reference to the Secret that will prevent it being deleted. To my mind that's better than an annotation on the BMH because you can see what it does directly, and there's no bizarre side-effects like changes to the Secret being ignored until they suddenly take effect at unexpected times.

[lentzi90]

Any end user who wants to keep their unreferenced Secrets around is free to add another owner reference to the Secret that will prevent it being deleted. To my mind that's better than an annotation on the BMH because you can see what it does directly, and there's no bizarre side-effects like changes to the Secret being ignored until they suddenly take effect at unexpected times.

I would rather treat the current behavior as a bug and just change it. Metal3 is not the owner of these secrets and should therefore not put owner references on them. Finalizers on the other hand could make sense to prevent deletion while in use.

[metal3-io-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[Rozzii]

/remove-lifecycle stale

[metal3-io-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[Rozzii]

/remove-lifecycle stale

[Rozzii]

/lifecycle frozen

[Rozzii]

I think it is clear that this is an issue regardless of what will be the solution.
Calling it a bug might be a bit controversial but I don't know what "kind label" would apply better so in lack of a better option I will flag it as bug.

/kind bug
/triage accepted