Repository navigation
fix(api): preserve foreign HelmRelease metadata on Application update - #2914
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthrough
ChangesMetadata Preservation on Application Update
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant REST_Update as REST.Update
participant KubernetesAPI as Kubernetes API
Client->>REST_Update: Application update request
REST_Update->>REST_Update: Convert Application
REST_Update->>KubernetesAPI: Get live HelmRelease
KubernetesAPI-->>REST_Update: Live metadata and resourceVersion
REST_Update->>REST_Update: Apply live metadata
REST_Update->>KubernetesAPI: Update HelmRelease
KubernetesAPI-->>REST_Update: Conflict
REST_Update->>KubernetesAPI: Get latest HelmRelease
REST_Update->>REST_Update: Rebuild metadata from pristine conversion
REST_Update->>KubernetesAPI: Retry HelmRelease update
Merge Risk: ⚪ Minimal · up to The change is mergeable after normal checks; no actionable issue is established by the supplied review evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change reduces the risk that updating and then deleting an Application leaves its managed resources behind. The update and retry paths now preserve controller-owned metadata, although an update-and-delete race has not been validated end to end. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where updating an Application caused the backing HelmRelease to lose essential metadata, including Flux finalizers and owner references. By stripping these fields, the system risked orphaning resources during deletion. The changes ensure that foreign metadata is carried over during the update process while maintaining proper ownership of API-specific labels and annotations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that foreign metadata (such as finalizers, ownerReferences, and non-prefixed labels/annotations) on a HelmRelease is preserved during Application updates by fetching the current object and carrying them over. It also adds a unit test to verify this behavior. Feedback highlights a critical bug where mutating helmRelease.Labels can cause concurrent map write panics due to mergeMaps returning a shared configuration map, and suggests an optimization in omitPrefixedMap to avoid unnecessary map allocations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| helmRelease.Finalizers = cur.Finalizers | ||
| helmRelease.OwnerReferences = cur.OwnerReferences | ||
| helmRelease.Labels = mergeMaps(omitPrefixedMap(cur.Labels, LabelPrefix), helmRelease.Labels) |
There was a problem hiding this comment.
Critical Bug: Potential Concurrent Map Write Panic / Shared Map Mutation
The helper function mergeMaps(a, b) returns a directly if b is nil/empty.
When called as mergeMaps(r.releaseConfig.Labels, helmRelease.Labels) (on line 570), if helmRelease.Labels is empty, it returns the shared r.releaseConfig.Labels map directly.
Subsequent lines then mutate helmRelease.Labels in-place:
helmRelease.Labels[ApplicationKindLabel] = r.kindNameBecause r.releaseConfig is a long-lived shared configuration object, this:
- Corrupts the shared configuration for all other requests.
- Causes a concurrent map writes panic (crashing the apiserver process) if multiple requests attempt to create or update applications concurrently.
Recommendation:
Update the mergeMaps function (defined around line 1132) to be safe against returning shared maps. For example:
func mergeMaps(a, b map[string]string) map[string]string {
if len(a) == 0 {
return b
}
if len(b) == 0 {
merged := make(map[string]string, len(a))
for k, v := range a {
merged[k] = v
}
return merged
}
merged := make(map[string]string, len(a)+len(b))
for k, v := range a {
merged[k] = v
}
for k, v := range b {
merged[k] = v
}
return merged
}This ensures that if b is empty but a is not, we return a safe, newly-allocated copy of a instead of the shared map reference.
There was a problem hiding this comment.
Confirmed, though the hazard predates this PR: the same in-place writes after mergeMaps(r.releaseConfig.Labels, ...) exist on the Create path, where an Application with no labels makes mergeMaps return the shared config map directly. On the Update path after this PR it is practically unreachable (the merge with the current HelmRelease labels always yields a fresh non-empty map, since the HelmRelease carries the application metadata labels), but Create is exposed.
Fixed in 2b15238 by making mergeMaps always allocate and never return an input map, with a regression test (TestCreate_DoesNotMutateSharedConfigLabels) that fails on the old implementation with exactly the described cross-contamination.
| func omitPrefixedMap(original map[string]string, prefix string) map[string]string { | ||
| if original == nil { | ||
| return nil | ||
| } | ||
| processed := make(map[string]string) | ||
| for k, v := range original { | ||
| if !strings.HasPrefix(k, prefix) { | ||
| processed[k] = v | ||
| } | ||
| } | ||
| return processed | ||
| } |
There was a problem hiding this comment.
Optimization: Avoid unnecessary map allocation
If all keys in original start with prefix (or if original is empty), omitPrefixedMap currently allocates an empty map and returns it.
By delaying the allocation of processed until we actually find a key to keep, we can return nil instead. This allows mergeMaps to take its fast path (returning the other map directly) and avoids unnecessary allocations and copying.
func omitPrefixedMap(original map[string]string, prefix string) map[string]string {
if original == nil {
return nil
}
var processed map[string]string
for k, v := range original {
if !strings.HasPrefix(k, prefix) {
if processed == nil {
processed = make(map[string]string)
}
processed[k] = v
}
}
return processed
}There was a problem hiding this comment.
Keeping the eager allocation: with 2b15238 mergeMaps always allocates a fresh map (to fix the shared-map mutation above), so the nil fast-path this optimization targets no longer exists. The current form also mirrors filterPrefixedMap directly above it; the empty-map allocation on this admission path is negligible next to the conversion work around it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/registry/apps/application/rest.go (1)
585-585:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate caller-supplied
UpdateOptionsto the HelmRelease update.Line 585 hardcodes
&metav1.UpdateOptions{}and drops request options (for exampledryRun,fieldManager,fieldValidation). This breaks Update API semantics and can persist writes that were requested as dry-run.🔧 Suggested fix
- err = r.c.Update(ctx, helmRelease, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) + err = r.c.Update(ctx, helmRelease, &client.UpdateOptions{Raw: options})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/apps/application/rest.go` at line 585, The call to r.c.Update(..., &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) drops caller-supplied UpdateOptions (losing dryRun/fieldManager/fieldValidation); fix by passing through the incoming client.UpdateOptions instance (rather than a hardcoded empty metav1.UpdateOptions) when calling r.c.Update with helmRelease so the request's Raw metav1.UpdateOptions are preserved (i.e., forward the provided client.UpdateOptions to r.c.Update for the Update operation on helmRelease).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/registry/apps/application/rest.go`:
- Line 585: The call to r.c.Update(..., &client.UpdateOptions{Raw:
&metav1.UpdateOptions{}}) drops caller-supplied UpdateOptions (losing
dryRun/fieldManager/fieldValidation); fix by passing through the incoming
client.UpdateOptions instance (rather than a hardcoded empty
metav1.UpdateOptions) when calling r.c.Update with helmRelease so the request's
Raw metav1.UpdateOptions are preserved (i.e., forward the provided
client.UpdateOptions to r.c.Update for the Update operation on helmRelease).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4d04724-1054-449a-8d28-7e8082d68b3b
📒 Files selected for processing (2)
pkg/registry/apps/application/rest.gopkg/registry/apps/application/rest_update_metadata_test.go
|
This conflicts with main at the Update path in |
Aleksei Sviridkin (lexfrei)
added
the
kind/backport
2b15238 to
59d12c7
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the root cause is correctly located and the fix is minimal and well-tested.
Business context: the aggregated apps.cozystack.io API rebuilds the HelmRelease from the Application on Update and issues a full-replace PUT, which strips metadata owned by other controllers (finalizers, ownerReferences, non-prefixed labels/annotations); losing helm-controller's finalizers.fluxcd.io lets a delete landing in that window remove the HelmRelease without running helm uninstall, orphaning the release's resources.
The carry-over is correct: foreign (non-prefixed) labels/annotations and the live finalizers/ownerReferences are preserved, while keys under the apps.cozystack.io- prefix are recomputed from the Application so deletions still propagate. TestUpdate_PreservesForeignHelmReleaseMetadata pins that contract, and mergeMaps was hardened to always allocate (never alias the shared releaseConfig.Labels), with TestCreate_DoesNotMutateSharedConfigLabels pinning it. Both tests pass locally, and the full package is green.
Non-blocking follow-up
Update calls r.c.Update(ctx, helmRelease, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) — a hardcoded empty options struct that drops the caller-supplied options (notably dryRun, plus fieldManager / fieldValidation). This is pre-existing and outside this PR's scope, but it means a server-side dry-run update persists instead of being simulated. Since it lives in the same Update method, folding a one-line Raw: options fix in here — or a small separate PR — would close it while the context is fresh.
|
An independent reproduction on v1.6.2 (single node, helm-controller v1.5.0), in A No uninstall followed. The One detail that makes the window wider than it looks. helm-controller watches For anyone who hits this before it merges, this apiVersion: admissionregistration.k8s.io/v1beta1
kind: MutatingAdmissionPolicy
metadata:
name: helmrelease-finalizer-guard
spec:
failurePolicy: Ignore
reinvocationPolicy: Never
matchConstraints:
resourceRules:
- apiGroups: ["helm.toolkit.fluxcd.io"]
apiVersions: ["*"]
operations: ["UPDATE"]
resources: ["helmreleases"]
matchConditions:
- name: written-by-cozystack-api
expression: >-
request.userInfo.username == 'system:serviceaccount:cozy-system:cozystack-api'
- name: drops-a-finalizer
expression: >-
oldObject.metadata.?finalizers.orValue([]).exists(f,
!(f in object.metadata.?finalizers.orValue([])))
variables:
- name: dropped
expression: >-
oldObject.metadata.?finalizers.orValue([]).filter(f,
!(f in object.metadata.?finalizers.orValue([])))
mutations:
- patchType: JSONPatch
jsonPatch:
expression: >-
has(object.metadata.finalizers) ?
variables.dropped.map(f, JSONPatch{op: "add", path: "/metadata/finalizers/-", value: f}) :
[JSONPatch{op: "add", path: "/metadata/finalizers", value: variables.dropped}]
---
apiVersion: admissionregistration.k8s.io/v1beta1
kind: MutatingAdmissionPolicyBinding
metadata:
name: helmrelease-finalizer-guard
spec:
policyName: helmrelease-finalizer-guardWe have run it since 2026-09-13. With a controller still patching every |
59d12c7 to
0781128
Compare
|
Rebased on Agreed on the backport, and One thing the rebase surfaced. The conflict-retry loop around the PUT refreshes only the resourceVersion and re-sends the metadata read before the conflict. A 409 there means the live object changed after that read, and the writer is normally helm-controller — the reconcile that re-adds Also worth noting for whoever reads the diff next: the explicit shard-label carry-over is now redundant. |
|
Thanks — the audit-log pair and the predicate detail are the useful part, and the second one is why the finalizer stays off for minutes rather than milliseconds. Following it into the code turned up a gap in this PR: the conflict-retry loop around the HelmRelease PUT refreshed only the resourceVersion and re-sent the metadata read before the conflict. A 409 there means the live object changed after that read, and on your timeline the writer is precisely the reconcile that puts The predicate window itself is helm-controller's behaviour rather than something this repository can assert on, so it is not getting a test here; what the tests pin is the invariant that closes it, namely that the finalizer never leaves the object in the first place. The |
Update rebuilds the HelmRelease from the Application and issues a full-replace PUT, which stripped metadata owned by other controllers: finalizers, ownerReferences, and any labels or annotations outside the apps.cozystack.io- prefix. Losing helm-controller's finalizers.fluxcd.io finalizer is the harmful case: a delete landing before helm-controller re-adds it removes the HelmRelease without running helm uninstall, orphaning every resource of the release (VMs, databases, PVCs). A label patch immediately followed by a delete - a common client pattern - hits that window almost every time. Fetch the current HelmRelease before the PUT and carry over its finalizers, ownerReferences, and non-prefixed labels and annotations. Keys under the apps.cozystack.io- prefix remain owned by the API and are recomputed from the Application, so label/annotation deletions still propagate. Signed-off-by: mattia-eleuteri <[email protected]>
mergeMaps returned one of its inputs whenever the other was nil. Create and Update seed the HelmRelease labels with mergeMaps(r.releaseConfig.Labels, ...) and then write the application metadata labels into the result in place, so for an Application with no labels the writes landed on the long-lived config map shared by every request for the kind: requests cross-contaminate (application.name varies per request) and concurrent map writes crash the apiserver. Signed-off-by: mattia-eleuteri <[email protected]>
A 409 on the HelmRelease PUT means the live object changed after the read the carry-over was built from, and the writer behind it is normally helm-controller - the controller whose finalizer the carry-over exists to protect, and which re-adds it on the reconcile following a metadata-only update. Retrying with the metadata read before the conflict wrote that stale copy back and dropped the finalizer the reconcile had just restored, reopening the orphaning window the carry-over closes. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
0781128 to
31948c8
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri LGTM. Updating an Application no longer drops the finalizers, ownerReferences and non-prefixed labels and annotations that other controllers put on the HelmRelease, so a delete right after an update runs helm uninstall again.
I rebased the branch onto current main. main had added its own carry-over of spec.suspend in the same Update path, so I kept both: suspend is still taken from the live object after the metadata carry-over, on the first write and on the conflict retry. The retry comment keeps main's paragraph about suspend and adds yours.
Nothing from the Application can land as a non-prefixed key: the conversion prefixes every Application label and annotation, and only the live HelmRelease feeds the unprefixed side. Removing the finalizer carry-over, the retry rebuild, or the mergeMaps allocation each turns one of the new tests red.
Two things that don't block:
- The PR body lists only the first commit. It becomes the merge commit message, so the
mergeMapsfix and the conflict-retry rebuild are worth a line each there. - A label removed from an ApplicationDefinition's
release.labelsnow stays on existing HelmReleases. Update keeps it as a foreign key, and the ApplicationDefinition reconciler only adds and changes labels, never removes them. Before this change the next Application update dropped it. I don't see a way to tell such a key apart from a foreign one without tracking ownership, so I'd accept it, just worth knowing.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-2914-to-release-1.6
git worktree add --checkout .worktree/backport-2914-to-release-1.6 backport-2914-to-release-1.6
cd .worktree/backport-2914-to-release-1.6
git reset --hard HEAD^
git cherry-pick -x a18cbc581c4829b657ebef0095f4ef0afc4cfc12 6d541c63eb658983175cda34d24c762fea249fd0 31948c88383e98314b2de3ff4d9329badcd69610
git push --force-with-lease |
## What this PR does An application deleted with foreground or orphan propagation (`kubectl delete --cascade=foreground`, or any client that sends that `propagationPolicy`) could stay in deletion forever. Helm uninstalled the release and helm-controller removed `finalizers.fluxcd.io`, but the HelmRelease kept `foregroundDeletion` in `metadata.finalizers`, so the HelmRelease and its Application stayed terminating. We hit this on v1.6.4 with a Bucket and an OpenBAO application. An Application and its HelmRelease share one UID, so the garbage collector keeps a single graph node for both and may finalize it through the Application endpoint. The API never returned finalizers on an Application, and every update restored the HelmRelease finalizers from the live object. [`removeFinalizer`](https://github.com/kubernetes/kubernetes/blob/v1.33.1/pkg/controller/garbagecollector/operations.go#L85-L123) reads the owner, does not find the finalizer there and returns as if it had already been removed, so nothing ever took it off the HelmRelease. This PR: - returns `foregroundDeletion` and `orphan` in `metadata.finalizers` of an Application, and only those two; - carries an add or a remove of either of them from an Application update to the HelmRelease, as a change against the finalizers the update started from, so the result is the same when helm-controller or the garbage collector changes the list during the conflict retry; - keeps controller finalizers such as `finalizers.fluxcd.io` owned by the live object, so `helm uninstall` still runs before the release goes away; - copies the deletion timestamp and grace period from the live object into the rebuilt HelmRelease, so an update of a terminating release stays valid. `rest_garbage_collection_test.go` covers both finalizers with Helm finishing first and with the garbage collector finishing first, concurrent finalizer changes during the conflict retry, controller finalizers surviving an update, the cached HelmRelease not being mutated by the conversion, and an `UpdatedObject` that edits the object in place. All five tests fail on the current `main` without this change. `go test -race ./pkg/registry/...` passes. The tests use the controller-runtime fake client, so this has not been run against a live garbage collector with a patched cozystack-api yet. The same bug is in release-1.6. There the Update path has no finalizer carry-over at all, because #2914 is not backported yet, and its automatic backport #4577 currently carries conflict markers. A 1.6 backport of this fix should go on top of a working backport of #2914. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff: it only changes how cozystack-api handles `metadata.finalizers` of Applications in `pkg/registry/apps/application`. No package, values, schema, CRD, `hack/` file or convention is touched. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(api): an application deleted with foreground or orphan propagation no longer leaves its HelmRelease stuck with the foregroundDeletion or orphan finalizer ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Application updates now preserve Kubernetes deletion behavior, including foreground deletion and orphaning, even when concurrent changes occur. * Helm-managed protection metadata is retained during updates, and deletion timing details remain intact. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…a and repair empty required fields (#4919) ## What this PR does Backport of #2914 and #3956 to `release-1.6`. It sits on top of #4683, which is merged here now. #2914: `Update` on an Application rebuilds the HelmRelease and sends a full-replace PUT, so metadata owned by other controllers was dropped, helm-controller's `finalizers.fluxcd.io` among it. A delete landing before helm-controller added the finalizer back removed the HelmRelease without `helm uninstall` and orphaned every resource of the release. Now `Update` carries over finalizers, ownerReferences and labels/annotations outside the `apps.cozystack.io-` prefix from the live object, `mergeMaps` no longer returns the shared config map that requests wrote into, and a conflict retry rebuilds the carried metadata from the object it just read instead of writing the stale copy back. #3956: a required spec field written with nothing under it (`replicas:`) reached Helm as null, Helm coalescing deletes it together with the chart default before schema validation, and the whole release failed with "missing property" for a property the spec does contain. Such a null is removed on Create and Update now so the chart default applies, and the client gets a warning with the field name. Optional fields, free-form subtrees, user-keyed map entries and array elements keep their nulls. Admission validates the normalized copy, the caller's object is not mutated anymore. On this branch #4683 inlined the garbage collection finalizer merge and the deletion metadata copy, because `applyLiveMetadata` didn't exist yet. They move into `applyLiveMetadata` here, so `Update` and its helpers match main. Suspend preservation (#4411) is not on this branch and not part of this backport. The example field in #3956, `talos.registryMirrors`, doesn't exist in the kubernetes chart on this branch, so the tests use required fields that do (`talos.installerRepository`, `talos.version`, `controlPlane.replicas`). The node group null is its own case, because `nodeGroups` is still a typed map here and its entries require `instanceType`. Three test helpers main has from #3315 are copied into a test-only file. Production code needed one import beyond the cherry-picks. ### Validation - `go build ./...` and `go vet ./pkg/registry/...` are clean and `go test ./pkg/registry/...` passes on the head, rebased onto release-1.6 after #4683 merged. - The Helm coalescing test renders this branch's own chart values and schemas, Helm rejects the empty required fields before the repair and accepts them after it. - Reverting `applyLiveMetadata` to the plain live finalizer list, skipping the retry rebuild, or validating the un-normalized object each turns the related tests red. ### Downstream repositories The change touches the Application REST handler and its tests only: no chart, values, schema, CRD, version enum or `hack/` file, so none of the trigger map entries match. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(api): updating an Application no longer strips finalizers, ownerReferences and foreign labels/annotations from its HelmRelease, which could make a following delete skip helm uninstall and orphan the release's resources. A required field written with no value in an application spec is treated as unset so the chart default applies, instead of failing the whole release. ```
What this PR does
The aggregated
apps.cozystack.ioAPI rebuilds the HelmRelease from the Application onUpdateand issues a full-replace PUT. This strips metadata owned by other controllers:metadata.finalizers— including helm-controller'sfinalizers.fluxcd.iometadata.ownerReferencesapps.cozystack.io-prefix (e.g.reconcile.fluxcd.io/requestedAt)Losing the Flux finalizer is the harmful case: helm-controller re-adds it on the next reconcile, but a delete landing in that window removes the HelmRelease without running
helm uninstall, orphaning every resource of the release (VMs, databases, PVCs...). A label patch immediately followed by a delete — a common client pattern — hits that window almost every time. We traced ~106 orphaned releases across 39 tenants on a production cluster back to this.Reproduction:
kubectl patch <any apps.cozystack.io resource> --type merge -p '{"metadata":{"labels":{"foo":"bar"}}}'→ the backing HelmRelease losesfinalizers.fluxcd.iokubectl deletethe resource right after → HelmRelease is removed instantly, Helm uninstall never runs, child resources stay aliveChanges:
Updatenow fetches the current HelmRelease and carries overfinalizers,ownerReferences, and non-prefixed labels/annotations before the PUTapps.cozystack.io-prefix remain owned by the API and are recomputed from the Application, so label/annotation deletions still propagateTestUpdate_PreservesForeignHelmReleaseMetadatapins the carry-over contract (fails on the previous behavior with exactly the stripped fields)Release note
Summary by CodeRabbit
Bug Fixes
Tests