-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Confirm release belongs to HR before upgrading #2123
Conversation
aa99331
to
1418c30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, a reasonable use of the mechanisms available, to fix the problem 👍
A minor suggestion, that need not hold this up: to make the description more readable, in case a human looks at it, could that be "Owned by: "+fhrResourceID
perhaps.
After running some tests I came to the conclusion it needs further tweaking, as in case no description is given, Helm will write its own description ( As these messages are simple strings in the Helm code base (and not public constants as I hoped they would be), writing (and maintaining) logic based on this does not seem preferable to me. The other option I could think of to fix this problem is the |
Hidde brought up some problems, so let's remove the green light for now
5ee12bd
to
6775c68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, yep this works as a "who last touched it" check, which is a reasonable way to detect whether some other HelmRelease is trying to be in control.
There's a couple of situations where it won't detect what's happening -- but I don't know how it could:
- Some other automated process is releasing the helm chart to the same release name. In this case, it won't annotate the resources, and helm-op will treat that as indicating that it can go ahead. Arguably, it should re-release what it's been told to, since the whole idea is to use HelmReleases declare what is supposed to be there.
- If another HelmRelease has released a chart, but the annotation step fails. There's not much we can do about that -- the release has already happened by that point. At least that situation is likely to be transient, and should eventually just correct itself.
Before this change, when a user would create multiple HelmReleases with the same release name configured, the operator would attempt to upgrade the release indefinitely. To overcome this issue, the operator now injects the resource ID of the HelmRelease into the description of the release and confirms this value equals the resource ID of the HelmRelease it is going to run an upgrade for. If the value diverges, the status of the HelmRelease is updated to make it visible to the user and a message is logged. For backwards compatibility, and to be able to migrate existing releases to a HelmRelease, we see empty descriptions as a positive.
Due to Helm creating its own descriptions when the user does not supply one, the previous approach got shot as we would be unable to determine ownership after manual helm actions. The new approach is to check if the resources created by the release are patched with the correct antecedent annotation, which we create ourselves and have full control over (and survives rollbacks). To be able to migrate existing releases to a HelmRelease, empty (missing) annotations are still handled as true / owned by.
6775c68
to
aadb430
Compare
Before this change, when a user would create multiple HelmReleases
with the same release name configured, the operator would attempt
to upgrade the release indefinitely.
To overcome this issue, the operator now injects the resource IDof the HelmRelease into the description of the release and confirms
this value equals the resource ID of the HelmRelease it is going to
run an upgrade for. If the value diverges, the status of the
HelmRelease is updated to make it visible to the user and a message
is logged.
To overcome this issue, we now check if the resources created by the
release are patched with the correct antecedent annotation, which we
create ourselves and have full control over (and survives rollbacks).
For backwards compatibility, and to be able to migrate existing
releases to a HelmRelease, we see empty descriptions as a positive.
Fixes #2101