Skip to content

ssa: Add pre-dry-run field manager takeover #1276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
dipti-pai wants to merge 1 commit into
base: main
Choose a base branch
from
+360 −0
Draft

ssa: Add pre-dry-run field manager takeover #1276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions ssa/manager_apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@ type ApplyCleanupOptions struct {
// FieldManagers defines which `metadata.managedFields` managers should be removed from in-cluster objects.
FieldManagers []FieldManager `json:"fieldManagers,omitempty"`

// FieldManagersBeforeDryRun defines which `metadata.managedFields` managers
// are taken over on the in-cluster object BEFORE the server-side dry-run.
//
// Use this to reclaim a required field co-owned by a stale or foreign manager
// that would otherwise be orphaned on prune and fail the dry-run before the
// post-dry-run cleanup can run.
//
// It is a deliberately narrow, opt-in list, separate from FieldManagers,
// because taking over ownership before the dry-run mutates managedFields even
// if the apply later fails — unlike the post-dry-run cleanup.
FieldManagersBeforeDryRun []FieldManager `json:"fieldManagersBeforeDryRun,omitempty"`

// Exclusions determines which in-cluster objects are skipped from cleanup
// based on the specified key-value pairs.
Exclusions map[string]string `json:"exclusions"`
Expand Down Expand Up @@ -138,6 +150,20 @@ func (m *ResourceManager) Apply(ctx context.Context, object *unstructured.Unstru
}
}

// Take over fields held by stale or foreign managers before the dry-run so
// that the merged object is valid. Without this, a manager co-owning a
// required field can leave that field orphaned when the applier prunes it,
// failing the dry-run before the post-dry-run cleanup can run.
if len(opts.Cleanup.FieldManagersBeforeDryRun) > 0 && getError == nil {
tookOver, err := m.takeoverManagedFields(ctx, object, existingObject,
opts.Cleanup.FieldManagersBeforeDryRun, opts.Cleanup)
if err != nil {
return nil, fmt.Errorf("%s metadata.managedFields takeover failed: %w",
utils.FmtUnstructured(existingObject), err)
}
patched = patched || tookOver
}

dryRunObject := object.DeepCopy()
if err := m.dryRunApply(ctx, dryRunObject); err != nil {
if !errors.IsNotFound(getError) && m.shouldForceApply(object, existingObject, opts, err) {
Expand Down Expand Up @@ -249,6 +275,18 @@ func (m *ResourceManager) ApplyAll(ctx context.Context, objects []*unstructured.
}
}

// Take over fields held by stale or foreign managers before the
// dry-run so that the merged object is valid (see Apply for details).
if len(opts.Cleanup.FieldManagersBeforeDryRun) > 0 && getError == nil {
tookOver, err := m.takeoverManagedFields(ctx, object, existingObject,
opts.Cleanup.FieldManagersBeforeDryRun, opts.Cleanup)
if err != nil {
return fmt.Errorf("%s metadata.managedFields takeover failed: %w",
utils.FmtUnstructured(existingObject), err)
}
patched = patched || tookOver
}

dryRunObject := object.DeepCopy()
if err := m.dryRunApply(ctx, dryRunObject); err != nil {
// We cannot have an immutable error (and therefore shouldn't force-apply) if the resource doesn't
Expand Down Expand Up @@ -516,6 +554,50 @@ func (m *ResourceManager) cleanupMetadata(ctx context.Context,
return true, m.client.Patch(ctx, object, patch, client.FieldOwner(m.owner.Field))
}

// takeoverManagedFields reassigns the given field managers' ownership to the
// applier on the in-cluster object via a JSON patch and updates object in-place
// with the server's response. It returns true if a patch was applied.
//
// Unlike cleanupMetadata, this only touches managedFields and is intended to run
// before the dry-run, so the applier reclaims ownership of fields held by stale
// or foreign managers and the merged object stays valid. The managers to take
// over are passed explicitly (rather than read from opts.FieldManagers) so the
// caller can scope the pre-dry-run takeover to a narrow set, independent of the
// post-dry-run cleanup list.
func (m *ResourceManager) takeoverManagedFields(ctx context.Context,
desiredObject *unstructured.Unstructured,
object *unstructured.Unstructured,
managers []FieldManager,
opts ApplyCleanupOptions) (bool, error) {
if len(managers) == 0 {
return false, nil
}
if object == nil || object.GetResourceVersion() == "" {
return false, nil
}
if utils.AnyInMetadata(desiredObject, opts.Exclusions) || utils.AnyInMetadata(object, opts.Exclusions) {
return false, nil
}

patches, err := PatchReplaceFieldsManagers(object, managers, m.owner.Field)
if err != nil {
return false, err
}

// no patching is needed, exit early
if len(patches) == 0 {
return false, nil
}

rawPatch, err := json.Marshal(patches)
if err != nil {
return false, err
}
patch := client.RawPatch(types.JSONPatchType, rawPatch)

return true, m.client.Patch(ctx, object, patch, client.FieldOwner(m.owner.Field))
}

// shouldForceApply determines based on the apply error and ApplyOptions if the object should be recreated.
// An object is recreated if the apply error was due to immutable field changes and if the object
// contains a label or annotation which matches the ApplyOptions.ForceSelector.
Expand Down
278 changes: 278 additions & 0 deletions ssa/manager_apply_takeover_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,278 @@
/*
Copyright 2026 The Flux authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package ssa

import (
"context"
"encoding/json"
"strings"
"testing"
"time"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// TestApply_FieldManagersBeforeDryRun reproduces the "break-glass" wedge:
//
// A workload is managed by Flux via server-side apply. During an incident an
// operator disables Flux and hand-patches the workload with client-side PUTs
// from a legacy controller (operation: Update). Re-enabling Flux leaves a stale
// `legacy-controller / Update` managedFields entry co-owning a container's
// identity ("." + f:name) but NOT its required f:image. When a later build
// removes that container, server-side apply cannot prune it (the legacy Update
// manager still owns the element) but DOES drop the field Flux solely owned
// (f:image), leaving a half-removed, invalid container:
//
// spec.template.spec.containers[N].image: Required value
//
// This fails at the dry-run on every reconcile, before any write. The standard
// post-dry-run FieldManagers cleanup never runs, so --override-manager cannot
// recover it. FieldManagersBeforeDryRun hoists the takeover before the dry-run,
// which repairs ownership and lets the container prune cleanly.
func TestApply_FieldManagersBeforeDryRun(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
defer cancel()

const (
fluxFM = "resource-manager" // matches manager.owner.Field
legacyFM = "legacy-controller"
gitImage = "registry.k8s.io/pause:3.9"
bgImage = "registry.k8s.io/pause:3.8"
)

id := generateName("takeover")

// Namespace for the workload.
ns := &unstructured.Unstructured{Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Namespace",
"metadata": map[string]interface{}{"name": id},
}}
if err := manager.client.Patch(ctx, ns, client.Apply, client.FieldOwner(fluxFM)); err != nil {
t.Fatal(err)
}

// daemonset builds a DaemonSet with the given container name/image pairs.
daemonset := func(containers ...[2]string) *unstructured.Unstructured {
cs := make([]interface{}, 0, len(containers))
for _, c := range containers {
cs = append(cs, map[string]interface{}{"name": c[0], "image": c[1]})
}
return &unstructured.Unstructured{Object: map[string]interface{}{
"apiVersion": "apps/v1",
"kind": "DaemonSet",
"metadata": map[string]interface{}{
"name": id,
"namespace": id,
},
"spec": map[string]interface{}{
"selector": map[string]interface{}{
"matchLabels": map[string]interface{}{"app": id},
},
"template": map[string]interface{}{
"metadata": map[string]interface{}{
"labels": map[string]interface{}{"app": id},
},
"spec": map[string]interface{}{"containers": cs},
},
},
}}
}

// legacyPut emulates a break-glass client-side PUT: read the in-cluster
// object, replace its containers, and Update it under the legacy manager
// (operation: Update).
legacyPut := func(t *testing.T, containers ...[2]string) {
t.Helper()
cur := daemonset()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(cur), cur); err != nil {
t.Fatal(err)
}
cs := make([]interface{}, 0, len(containers))
for _, c := range containers {
cs = append(cs, map[string]interface{}{"name": c[0], "image": c[1]})
}
if err := unstructured.SetNestedSlice(cur.Object, cs, "spec", "template", "spec", "containers"); err != nil {
t.Fatal(err)
}
if err := manager.client.Update(ctx, cur, client.FieldOwner(legacyFM)); err != nil {
t.Fatal(err)
}
}

// pdhOwnership reports, per manager/operation, whether it owns the pdh
// container element ("."), its f:name, and its f:image.
type ownership struct{ self, name, image bool }
pdhOwnership := func(t *testing.T) map[string]ownership {
t.Helper()
cur := daemonset()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(cur), cur); err != nil {
t.Fatal(err)
}
out := map[string]ownership{}
for _, e := range cur.GetManagedFields() {
if e.Subresource != "" || e.FieldsV1 == nil {
continue
}
var raw map[string]interface{}
if err := json.Unmarshal(e.FieldsV1.Raw, &raw); err != nil {
t.Fatal(err)
}
pdh := digPath(raw, "f:spec", "f:template", "f:spec", "f:containers", `k:{"name":"pdh"}`)
if pdh == nil {
continue
}
o := ownership{}
if _, ok := pdh["."]; ok {
o.self = true
}
if _, ok := pdh["f:name"]; ok {
o.name = true
}
if _, ok := pdh["f:image"]; ok {
o.image = true
}
out[string(e.Manager)+"/"+string(e.Operation)] = o
}
return out
}

// --- Set up the exact production ownership shape ---------------------------

// 1. GitOps healthy: Flux SSA-applies main + pdh (git image).
if _, err := manager.Apply(ctx, daemonset([2]string{"main", gitImage}, [2]string{"pdh", gitImage}), DefaultApplyOptions()); err != nil {
t.Fatalf("initial flux apply: %v", err)
}

// 2. Break-glass: operator removes broken pdh, then re-adds it (hotfix image).
// The legacy Update manager that re-creates pdh takes the element "." + f:name.
legacyPut(t, [2]string{"main", gitImage})
legacyPut(t, [2]string{"main", gitImage}, [2]string{"pdh", bgImage})

// 3. Re-enable Flux: it reasserts the git manifest. Because the image value
// differs, Flux's apply takes f:image exclusively; the legacy Update keeps
// "." + f:name but loses f:image
forceOpts := DefaultApplyOptions()
forceOpts.Force = false
if _, err := manager.Apply(ctx, daemonset([2]string{"main", gitImage}, [2]string{"pdh", gitImage}), forceOpts); err != nil {
t.Fatalf("re-enable flux apply: %v", err)
}

own := pdhOwnership(t)
flux, okFlux := own[fluxFM+"/Apply"]
legacy, okLegacy := own[legacyFM+"/Update"]
if !okFlux || !flux.image {
t.Fatalf("expected %s/Apply to own pdh f:image, ownership=%+v", fluxFM, own)
}
if !okLegacy || !legacy.self || legacy.image {
t.Fatalf("expected %s/Update to anchor pdh (self, not image), ownership=%+v", legacyFM, own)
}
t.Logf("ownership set up: flux=%+v legacy=%+v", flux, legacy)

// FieldManagers to take over (the legacy break-glass manager).
fms := []FieldManager{
{Name: legacyFM, OperationType: metav1.ManagedFieldsOperationUpdate},
{Name: legacyFM, OperationType: metav1.ManagedFieldsOperationApply},
}

// --- The wedge and the fix -------------------------------------------------

t.Run("without pre-dry-run takeover, dry-run is Invalid", func(t *testing.T) {
opts := DefaultApplyOptions()
// Post-dry-run cleanup only (current --override-manager behavior): the
// takeover is gated behind a passing dry-run, so it never runs here.
opts.Cleanup.FieldManagers = fms

_, err := manager.Apply(ctx, daemonset([2]string{"main", gitImage}), opts)
if err == nil {
t.Fatal("expected dry-run to fail with Required value, got nil")
}
if !strings.Contains(err.Error(), "Required value") {
t.Fatalf("expected 'Required value' dry-run error, got: %v", err)
}
t.Logf("reproduced wedge: %v", err)

// pdh must still be present (nothing was applied).
cur := daemonset()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(cur), cur); err != nil {
t.Fatal(err)
}
if !hasContainer(t, cur, "pdh") {
t.Fatal("expected pdh to still be present after failed dry-run")
}
})

t.Run("with pre-dry-run takeover, apply succeeds and pdh is pruned", func(t *testing.T) {
opts := DefaultApplyOptions()
opts.Cleanup.FieldManagersBeforeDryRun = fms

entry, err := manager.Apply(ctx, daemonset([2]string{"main", gitImage}), opts)
if err != nil {
t.Fatalf("expected apply to succeed after takeover, got: %v", err)
}
if entry.Action != ConfiguredAction {
t.Errorf("expected ConfiguredAction, got %s", entry.Action)
}

cur := daemonset()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(cur), cur); err != nil {
t.Fatal(err)
}
if hasContainer(t, cur, "pdh") {
t.Fatal("expected pdh to be pruned after takeover apply")
}
if !hasContainer(t, cur, "main") {
t.Fatal("expected main container to remain")
}
})
}

// hasContainer reports whether the workload has a container with the given name.
func hasContainer(t *testing.T, obj *unstructured.Unstructured, name string) bool {
t.Helper()
cs, found, err := unstructured.NestedSlice(obj.Object, "spec", "template", "spec", "containers")
if err != nil {
t.Fatal(err)
}
if !found {
return false
}
for _, c := range cs {
if m, ok := c.(map[string]interface{}); ok {
if n, _, _ := unstructured.NestedString(m, "name"); n == name {
return true
}
}
}
return false
}

// digPath walks nested map[string]interface{} by the given keys, returning the
// terminal map or nil.
func digPath(m map[string]interface{}, keys ...string) map[string]interface{} {
cur := m
for _, k := range keys {
next, ok := cur[k].(map[string]interface{})
if !ok {
return nil
}
cur = next
}
return cur
}
Loading