Skip to content

Commit 31948c8

Browse files
mattia-eleuterilexfrei
mattia-eleuteri
authored and
lexfrei
committed
fix(api): redo the HelmRelease metadata carry-over on a conflict retry
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]>
1 parent 6d541c6 commit 31948c8

2 files changed

Lines changed: 124 additions & 39 deletions

File tree

‎pkg/registry/apps/application/rest.go‎

Lines changed: 58 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -575,52 +575,19 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje
575575
return nil, false, fmt.Errorf("conversion error: %v", err)
576576
}
577577

578-
// The conversion above rebuilds the HelmRelease from the Application
579-
// alone, but parts of the HelmRelease metadata are owned by other
580-
// controllers and must survive the update. Fetch the current object
581-
// and carry them over before issuing the full-replace PUT:
582-
// - finalizers: helm-controller's finalizers.fluxcd.io guarantees
583-
// `helm uninstall` runs on deletion. Stripping it here lets a
584-
// subsequent delete remove the HelmRelease instantly, orphaning
585-
// every resource of the release.
586-
// - ownerReferences: garbage collection and lineage.
587-
// - labels/annotations outside the apps.cozystack.io- prefix: set
588-
// directly on the HelmRelease by Flux or other controllers.
589-
// Keys under the prefix are owned by this API: stale ones are dropped
590-
// so deleting a label or annotation on the Application propagates.
578+
// Keep the conversion output pristine: the metadata assembly below reads the
579+
// live object, so a conflict retry has to redo it from this copy rather than
580+
// re-apply it on top of its own previous result.
581+
converted := helmRelease.DeepCopy()
582+
591583
cur := &helmv2.HelmRelease{}
592584
if err := r.c.Get(ctx, client.ObjectKey{Namespace: helmRelease.Namespace, Name: helmRelease.Name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}); err != nil {
593585
return nil, false, registry.WrapPreservingStatus("failed to fetch current HelmRelease", err, r.gvr.GroupResource(), name)
594586
}
595587
if helmRelease.ResourceVersion == "" {
596588
helmRelease.SetResourceVersion(cur.GetResourceVersion())
597589
}
598-
helmRelease.Finalizers = cur.Finalizers
599-
helmRelease.OwnerReferences = cur.OwnerReferences
600-
helmRelease.Labels = mergeMaps(omitPrefixedMap(cur.Labels, LabelPrefix), helmRelease.Labels)
601-
helmRelease.Annotations = mergeMaps(omitPrefixedMap(cur.Annotations, AnnotationPrefix), helmRelease.Annotations)
602-
603-
// Merge system labels (from config) directly
604-
helmRelease.Labels = mergeMaps(r.releaseConfig.Labels, helmRelease.Labels)
605-
// Merge user labels with prefix
606-
helmRelease.Labels = mergeMaps(helmRelease.Labels, addPrefixedMap(app.Labels, LabelPrefix))
607-
// Add application metadata labels
608-
if helmRelease.Labels == nil {
609-
helmRelease.Labels = make(map[string]string)
610-
}
611-
helmRelease.Labels[ApplicationKindLabel] = r.kindName
612-
helmRelease.Labels[ApplicationGroupLabel] = r.gvk.Group
613-
helmRelease.Labels[ApplicationNameLabel] = app.Name
614-
// Note: Annotations from config are not handled as r.releaseConfig.Annotations is undefined
615-
616-
// The flux-shard-operator assigns each tenant HelmRelease to a
617-
// helm-controller shard by rewriting this label at runtime. Rebuilding
618-
// the object from the Application reverts it to the ApplicationDefinition
619-
// default, which would bounce the HelmRelease off its shard on every
620-
// update, so the live value wins.
621-
if shard, ok := cur.Labels[fluxshard.ShardKeyLabel]; ok {
622-
helmRelease.Labels[fluxshard.ShardKeyLabel] = shard
623-
}
590+
r.applyLiveMetadata(helmRelease, cur, app)
624591

625592
// Suspension is not part of the Application, so the rebuilt object always
626593
// says suspend=false, and sending it as is resumes a release that an
@@ -646,13 +613,22 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje
646613
// is never a real spec conflict here: refresh the resourceVersion from the
647614
// live object and retry. Suspend is refreshed with it, because the write
648615
// that caused the conflict may be the one that suspended the release.
616+
//
617+
// The carried-over metadata is not derived from the Application, though, and
618+
// the writer that caused the conflict is usually the controller that owns it
619+
// — helm-controller re-adding finalizers.fluxcd.io is the case this whole
620+
// carry-over exists for. Keeping the copy read before the conflict would put
621+
// the stale metadata back and undo that write, so rebuild from the
622+
// conversion output against the object just read.
649623
err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
650624
updateErr := r.c.Update(ctx, helmRelease, registry.ClientUpdateOptions(options))
651625
if apierrors.IsConflict(updateErr) {
652626
cur := &helmv2.HelmRelease{}
653627
if getErr := r.c.Get(ctx, client.ObjectKey{Namespace: helmRelease.Namespace, Name: helmRelease.Name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}); getErr != nil {
654628
return getErr
655629
}
630+
helmRelease = converted.DeepCopy()
631+
r.applyLiveMetadata(helmRelease, cur, app)
656632
helmRelease.SetResourceVersion(cur.GetResourceVersion())
657633
helmRelease.Spec.Suspend = cur.Spec.Suspend
658634
}
@@ -1244,6 +1220,49 @@ func filterPrefixedMap(original map[string]string, prefix string) map[string]str
12441220
return processed
12451221
}
12461222

1223+
// applyLiveMetadata assembles the metadata of a HelmRelease rebuilt from an
1224+
// Application, given the live object it is about to replace. The PUT is a full
1225+
// replace, so anything not restated here is dropped:
1226+
// - finalizers: helm-controller's finalizers.fluxcd.io guarantees
1227+
// `helm uninstall` runs on deletion. Stripping it lets a subsequent delete
1228+
// remove the HelmRelease instantly, orphaning every resource of the release.
1229+
// - ownerReferences: garbage collection and lineage.
1230+
// - labels/annotations outside the apps.cozystack.io- prefix: set directly on
1231+
// the HelmRelease by Flux or other controllers. Keys under the prefix are
1232+
// owned by this API and recomputed from the Application, so stale ones are
1233+
// dropped and a deletion on the Application propagates.
1234+
//
1235+
// hr must be a fresh conversion output: the function overlays the live metadata
1236+
// and is not idempotent against its own result.
1237+
func (r *REST) applyLiveMetadata(hr, cur *helmv2.HelmRelease, app *appsv1alpha1.Application) {
1238+
hr.Finalizers = cur.Finalizers
1239+
hr.OwnerReferences = cur.OwnerReferences
1240+
hr.Labels = mergeMaps(omitPrefixedMap(cur.Labels, LabelPrefix), hr.Labels)
1241+
hr.Annotations = mergeMaps(omitPrefixedMap(cur.Annotations, AnnotationPrefix), hr.Annotations)
1242+
1243+
// Merge system labels (from config) directly
1244+
hr.Labels = mergeMaps(r.releaseConfig.Labels, hr.Labels)
1245+
// Merge user labels with prefix
1246+
hr.Labels = mergeMaps(hr.Labels, addPrefixedMap(app.Labels, LabelPrefix))
1247+
// Add application metadata labels
1248+
if hr.Labels == nil {
1249+
hr.Labels = make(map[string]string)
1250+
}
1251+
hr.Labels[ApplicationKindLabel] = r.kindName
1252+
hr.Labels[ApplicationGroupLabel] = r.gvk.Group
1253+
hr.Labels[ApplicationNameLabel] = app.Name
1254+
// Note: Annotations from config are not handled as r.releaseConfig.Annotations is undefined
1255+
1256+
// The flux-shard-operator assigns each tenant HelmRelease to a
1257+
// helm-controller shard by rewriting this label at runtime. Rebuilding
1258+
// the object from the Application reverts it to the ApplicationDefinition
1259+
// default, which would bounce the HelmRelease off its shard on every
1260+
// update, so the live value wins.
1261+
if shard, ok := cur.Labels[fluxshard.ShardKeyLabel]; ok {
1262+
hr.Labels[fluxshard.ShardKeyLabel] = shard
1263+
}
1264+
}
1265+
12471266
// omitPrefixedMap returns the entries of a map whose keys do NOT carry the
12481267
// predefined prefix, keeping the keys as-is. It is the complement of
12491268
// filterPrefixedMap: prefixed keys are owned by this API and recomputed from

‎pkg/registry/apps/application/rest_update_metadata_test.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,17 @@ package application
22

33
import (
44
"context"
5+
"errors"
56
"testing"
67

78
helmv2 "github.com/fluxcd/helm-controller/api/v2"
9+
apierrors "k8s.io/apimachinery/pkg/api/errors"
810
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
"k8s.io/apimachinery/pkg/runtime/schema"
912
"k8s.io/apimachinery/pkg/types"
1013
"k8s.io/apiserver/pkg/endpoints/request"
1114
"sigs.k8s.io/controller-runtime/pkg/client"
15+
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
1216

1317
appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
1418
)
@@ -144,3 +148,65 @@ func TestCreate_DoesNotMutateSharedConfigLabels(t *testing.T) {
144148
t.Errorf("expected shared config labels to be untouched, got %v", configLabels)
145149
}
146150
}
151+
152+
// TestUpdate_ReappliesForeignMetadataAfterConflict covers the metadata
153+
// carry-over on the conflict-retry path. A 409 on the PUT means the live
154+
// HelmRelease changed after the read the carry-over was built from, and
155+
// the writer behind it is normally helm-controller — which is exactly
156+
// the controller whose finalizer the carry-over exists to protect, and
157+
// which re-adds it on the reconcile following a metadata-only update.
158+
// Retrying with the metadata read before the conflict writes the stale
159+
// copy back and drops the finalizer that write had just restored.
160+
func TestUpdate_ReappliesForeignMetadataAfterConflict(t *testing.T) {
161+
var updates int
162+
r := newOptionsTestREST(t, interceptor.Funcs{
163+
Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
164+
updates++
165+
if updates > 1 {
166+
return c.Update(ctx, obj, opts...)
167+
}
168+
live := &helmv2.HelmRelease{}
169+
if err := c.Get(ctx, client.ObjectKey{Namespace: optionsTestNamespace, Name: "postgresql-example"}, live); err != nil {
170+
return err
171+
}
172+
live.Finalizers = append(live.Finalizers, "finalizers.fluxcd.io")
173+
live.Labels["kustomize.toolkit.fluxcd.io/name"] = "core"
174+
if err := c.Update(ctx, live); err != nil {
175+
return err
176+
}
177+
return apierrors.NewConflict(
178+
schema.GroupResource{Group: "helm.toolkit.fluxcd.io", Resource: "helmreleases"},
179+
"postgresql-example", errors.New("object was modified"))
180+
},
181+
}, optionsTestHelmRelease())
182+
183+
ctx := request.WithNamespace(context.Background(), optionsTestNamespace)
184+
if _, _, err := r.Update(
185+
ctx,
186+
"example",
187+
newDefaultUpdatedObjectInfo(optionsTestApplication()),
188+
nil,
189+
nil,
190+
false,
191+
&metav1.UpdateOptions{},
192+
); err != nil {
193+
t.Fatalf("unexpected error: %v", err)
194+
}
195+
if updates < 2 {
196+
t.Fatalf("expected the conflict to be retried, got %d update calls", updates)
197+
}
198+
199+
got := &helmv2.HelmRelease{}
200+
if err := r.c.Get(ctx, client.ObjectKey{Namespace: optionsTestNamespace, Name: "postgresql-example"}, got); err != nil {
201+
t.Fatalf("failed to fetch updated HelmRelease: %v", err)
202+
}
203+
if len(got.Finalizers) != 1 || got.Finalizers[0] != "finalizers.fluxcd.io" {
204+
t.Errorf("expected the finalizer added during the conflict to survive, got %v", got.Finalizers)
205+
}
206+
if got.Labels["kustomize.toolkit.fluxcd.io/name"] != "core" {
207+
t.Errorf("expected the foreign label added during the conflict to survive, got labels %v", got.Labels)
208+
}
209+
if got.Labels[ApplicationNameLabel] != "example" {
210+
t.Errorf("expected application metadata labels to remain, got labels %v", got.Labels)
211+
}
212+
}

0 commit comments

Comments
 (0)