feat(flux-react): disable Suspend when spec.suspend is GitOps-managed - #2004
Conversation
Server-side apply is field-level, so what decides whether an imperative suspend survives is not "is this resource GitOps-managed" but whether the applied manifest asserts `spec.suspend`. When it does, the applying controller force-takes the field back on its next apply — Flux's SSA always passes ForceOwnership — so the suspension silently undoes itself within one of *that* controller's intervals. Detect it from `metadata.managedFields`, which the API already returns: any entry with operation Apply owning `f:spec.f:suspend`. The toggle is disabled with a tooltip naming the owning manager(s). Not restricted to kustomize-controller, since helm-controller and a human `kubectl apply --server-side` have the same effect; Update-operation owners are ignored as they hold ownership without a desired state to restore. Reconcile stays enabled even on managed resources: the requestedAt annotation is never part of an applied manifest, and the controller records its value into status rather than clearing it, so there is no race to lose. Also set `?fieldManager=giantswarm-backstage` on writes. The apiserver otherwise derives the manager from the User-Agent, which for a proxied write is unpredictable and useless for auditing. Not masquerading as `flux` — nothing in Flux keys off that name, so it would buy nothing and destroy attribution. Corrects the ImagePolicy justification in the actionable-kinds list: the CLI does cover the kind (`flux reconcile|suspend|resume image policy`), so the stated reason for its original omission was wrong even though including it was right.
Code reviewFound 2 issues:
backstage/plugins/kubernetes-react/src/lib/k8s/FluxObject.ts Lines 80 to 84 in ab2e264 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…etection Five issues raised in review of #2004: - A `managedFields` entry is only rewritten by a write, so an `Apply` entry outlives the applier. Handing an object over for manual control with `reconcile: disabled` or `ssa: Ignore` left a stale entry naming the field and disabled the toggle forever, and `ssa: IfNotPresent` objects carry such an entry from creation onwards. Those three annotations now short-circuit the check. The PR description claimed these already worked; they did not. - Narrow the "Update owners never revert anything" claim. Client-side `kubectl apply` and `helm upgrade`'s three-way merge both keep a stored desired state and re-assert it while being recorded as `operation: Update`, so detection covers SSA appliers only. Documented as a best-effort signal rather than extended by guessing at manager names. - Restore the tooltip on disabled buttons. bui's disabled styling sets only `cursor: not-allowed`, so unlike MUI's ButtonBase the native <button> keeps swallowing the hover and only the native `title` showed. Adds `pointer-events: none` when disabled, to Suspend and the pre-existing Reconcile alike. - Deduplicate and Oxford-join the owner list: one manager can hold several entries (keyed by manager + operation + apiVersion + subresource), and three owners rendered as "a and b and c". - Reword the ImagePolicy note, which described a removal absent from history, and fix two stale comments still using ImagePolicy as the example of an unsupported kind.
Two issues from a second review pass on #2004, both tooltip copy: - A suspended *and* declaratively managed resource had Reconcile advising "Resume it first" while this PR disables Resume for being managed — dead-end advice. That state now gets its own hint pointing at the applier instead. - "Change it in Git instead" contradicted the ownership check, which is deliberately not Git-specific: helm-controller's drift correction and a human `kubectl apply --server-side` both count. Reworded to "Change it at the source that applies it."
|
Thanks @fiunchinho — both valid, both fixed in f3e2d93. Neither was resolved by the earlier review round, and I had missed this comment entirely (I enumerated review comments and review summaries, not issue comments). 1. Contradictory tooltips. Correct, and the state you name was covered by a test that asserted only that both buttons were disabled — never what they said, which is why it slipped through.
A test asserts that exact string and additionally that 2. Git-specific wording. Also correct, and a genuine internal contradiction: my own docstring says the check covers helm-controller drift correction and Worth noting the residual imprecision, since I would rather flag it than let it read as fully solved: the tooltip names the manager (e.g. 214 tests pass, |
2 participants
What does this PR do?
Follow-up to #2001, from Flux-team feedback about manipulating
.specfrom a UI. Three changes.1. Detect a contested
spec.suspendand disable the toggle.The concern raised was that a manual suspend on a GitOps-managed resource gets overwritten. That is right — but the precise condition is narrower than "the resource is GitOps-managed". Server-side apply is field-level: kustomize-controller only touches fields present in the manifest it applies, and removes a field only if it previously owned it and has since dropped it. So what matters is whether the applied manifest asserts
spec.suspend.When it does, the applying controller force-takes the field back on its next apply —
ssa/manager_apply.goin fluxcd/pkg always passesclient.ForceOwnership— and the suspension undoes itself within one of that controller's intervals, not the resource's own. Silently: no conflict error, nothing degraded.We can see this exactly, without guessing, from
metadata.managedFields(already in our GET responses): any entry with operationApplythat ownsf:spec.f:suspend. When present, the Suspend/Resume toggle is disabled with a tooltip naming the owning manager(s).Two details worth reviewing:
kustomize-controller. A humankubectl apply --server-side, or helm-controller's drift correction whenspec.driftDetectionis enabled, has the same effect. Any SSA applier of the field will revert us, so the check is generic.Update-operation owners are skipped, which correctly excludes our own merge patches (so our writes don't lock the button out) — but also misses the two common non-SSA declarative writers, which do keep a stored desired state and re-assert it: client-sidekubectl apply(kubectl-client-side-apply) and a plainhelm upgrade, whose three-way merge resets drift on chart-declared fields. So a chart shipping a Kustomization withspec.suspenddeclared still shows an enabled toggle whose change the next upgrade reverts. This is documented ongetApplyFieldOwnersrather than papered over by matching manager names, which would be a guess.managedFieldsentry is only rewritten by a write, so it outlives the applier — see the escape-hatch note below.Disabling is precisely correct here rather than merely conservative: the toggle always flips away from the current value, and the current value is what the apply-owner last asserted — so whenever the field is apply-owned, the action on offer is exactly the one that would be reverted.
2. Reconcile is deliberately left enabled, including on managed resources.
reconcile.fluxcd.io/requestedAtis never part of an applied manifest, so no apply-owner asserts or prunes it. kustomize-controller'sCleanup.Annotationslist is explicit and short (last-applied-configuration, two legacyfluxcd.iochecksums) and does not include it. The controller then copies the value intostatus.lastHandledReconcileAtand leaves the annotation in place — that pair is what our pending-request check compares. There is no race to lose, so gating Reconcile would be wrong. There's a comment saying so, to stop a future reader "fixing" it.3. An explicit field manager on our writes.
Writes now set
?fieldManager=giantswarm-backstage. Previously the apiserver derived the manager name from the request's User-Agent, which for a write proxied through the Backstage backend is both unpredictable and useless in an audit trail.The suggestion in the thread was to masquerade as
fluxso the controller would treat our writes like CLI writes. I checked the source, and there is nothing to imitate:internal/utils/utils.gobuilds the client asclient.NewWithWatch(cfg, client.Options{Scheme: scheme}). Themanager: fluxvisible in--show-managed-fieldsis just the apiserver deriving it from client-go's default User-Agent.kustomization_controller.go, feedingApplyOptions.Cleanup.FieldManagers) coverskubectl/Apply,kubectl/Update,before-first-apply/Update and its own name with Update, plus any--override-managervalues.fluxis absent, so a CLI suspend gets no special treatment in either direction.So impersonation would buy nothing and destroy attribution. An honest name also gives operators a value they can pass to a controller's
--override-managerif they want these changes force-reverted.4. Corrects the ImagePolicy justification from #2001.
The comment claimed Flux supports neither operation for
ImagePolicy. That was wrong: bothcrds.fluxcd.v1.ImagePolicyandv1beta2declarespec.suspendandstatus.lastHandledReconcileAt, and the CLI covers the kind too —cmd/flux/reconcile_image_policy.go,suspend_image_policy.go,resume_image_policy.goall exist. Including the kind was right; the reason given was not. (The claim also appeared in #2001's description and changeset, which are already merged — noted here for the record.)What is the effect of this change to users?
On a resource whose
spec.suspendcomes from Git, the Suspend/Resume button is now visibly disabled and says why, instead of appearing to work and then quietly undoing itself minutes later. This is the Adidas case from the thread, where Kustomizations in thedefaultnamespace setspec.suspendexplicitly.Everything else is unchanged: Reconcile works everywhere it did before, and Suspend/Resume still works on resources whose suspend field is not declaratively managed.
Accepted trade-off: you can no longer take a deliberate short-lived suspend on a managed resource from the UI, and if such a resource was suspended elsewhere you cannot resume it here. Both were already true of
flux suspend/resumein the sense that the change wouldn't stick; the difference is that now we say so up front rather than letting it fail silently. The fix in that situation is to change it in Git.How does it look like?
Any background context you can provide?
The Slack thread asked whether the Flux controllers handle a CLI-set
spec.suspendspecially — e.g. keying off the field manager. They don't; see point 3 above. The behaviour difference between "suspend sticks" and "suspend evaporates" is entirely about what's in the applied manifest.Three per-object annotations exclude a resource from apply and therefore make manual changes durable —
kustomize.toolkit.fluxcd.io/reconcile: disabled,kustomize.toolkit.fluxcd.io/ssa: Ignore, andkustomize.toolkit.fluxcd.io/ssa: IfNotPresent(fromapplyOptsinkustomization_controller.go).An earlier revision of this PR claimed these "will correctly show an enabled toggle, since no apply-owner claims the field". That was wrong, as pointed out in review: a
managedFieldsentry is only ever rewritten by a write, so it survives the applier walking away. An object handed over for manual control keeps its staleApplyentry, and anssa: IfNotPresentobject carries one from creation onwards despite never being applied again — so both would have shown a permanently disabled toggle. Those three annotations now short-circuit the check.Verify on a real MC with:
Do the docs need to be updated?
No.
Should this change be mentioned in the release notes?