LockedResourceReconciler: enforce with server-side apply; excluded paths by ownership; template cache concurrency-safe - #104
Open
ephico2real2 wants to merge 9 commits into
Conversation
The same files as on fix/template-cache-race-and-excluded-paths at bcf5ba5, without the #194 null-injection change to resource-reconciler.go: a concurrency-safe template cache holding one parsed base per text that is cloned and rebound to each caller's config, and excluded-path handling that converts indexes and quoted keys correctly, rejects negative indexes, malformed brackets and empty segments by name, and passes RFC 6901 pointers through. Every case has a test that failed before its fix.
The merge-patch enforcer could not express "a field the template no longer renders": a merge patch only adds and replaces, so a stale field survived until #194 proposed injecting nulls and computing diffs. Field ownership is the API server's answer to exactly that. Every reconcile now applies the rendered object under one field manager with force: drift on an owned field is restored, a field the template stops rendering is removed (the server deletes what its owner stops sending), a field another manager owns and the template never renders is left alone, and an apply that changes nothing does not write (resourceVersion unchanged). The diff, the null injection and the jsondiff dependency are gone. Excluded paths keep their meaning, "set once, then left alone", by ownership rather than by omission: the object is created from the whole rendered object, and before every later apply the reconciler drops every path under an excluded path from its own managedFields entry, so the server keeps the field as it is, owned by nobody, instead of deleting it when the reduced object arrives. The same step folds a client-side Update entry left by the previous enforcer (the binary's name, "manager" on a live cluster, taken from the user agent) into the apply manager with client-go's csaupgrade, so objects created before this change lose their stale fields too. One JSON patch, sent only when something changes, replacing resourceVersion so a concurrent write conflicts instead of being overwritten. Every claim is a test against a real API server (envtest): create and restore drift; a dropped field removed; an excluded field set at creation, changed by someone else, and kept through further reconciles with the reconciler not owning it; `.metadata` excluded keeps the template's labels and a stranger's; a legacy Update entry folded, its stale field removed, its excluded labels kept; a no-op reconcile keeps the resourceVersion; another manager's field left. The package's TestMain skips them with a message when KUBEBUILDER_ASSETS is unset.
The enforcer restarts a reconciler only for a resource whose key changed, and the key was the marshalled object alone, so editing a template's excludedPaths on a CR changed nothing until the operator restarted. Measured on a cluster: after `.metadata` was removed from a NamespaceConfig's excludedPaths, its reconcilers kept excluding it and a tampered label was not restored. The key now carries the sorted excluded paths. A changed key restarts the manager without deleting anything: the deletion set is computed by object identity (getToBeDeletdResources), not by key.
… "equal" verdict races the right object, lists measured FieldPath now says which segment the path wrote as a list index; before, any numeric segment was taken for one, so excluding ".data['0']" released all of data (measured). A quoted key is never an index; "[0]", a bare ".0" and a numeric pointer segment are. releaseOwnership returns the object the apply then races (the patched one, or the live one when nothing was sent), and Reconcile compares the apply's result with that: a release followed by a no-op apply logged "NOT equal" before, because the comparison used the first GET. Tests, all against envtest: the quoted numeric key set once and its sibling enforced; the log verdict after a release; Role rules as an atomic list (drift inside the list restored, a dropped element removed, and an exclusion inside the list documented as removing that element on the next reconcile, as the merge-patch enforcer did). The residual risk of folding a whole legacy entry (another controller with the same binary name writing the same object) is stated next to LegacyFieldManagers and accepted: folding only what the template renders would leave every stale field of a legacy object in place, the failure this change exists to end.
…p unit; the legacy fold is opt-in managedFields is schema-shaped: a granular map lists its keys, an atomic map (a Pod's nodeSelector) or an atomic list (RBAC rules) is one member with no children. An exclusion inside such a unit could not be released on its own, and applying the unit without the excluded child deleted the child (measured in review). Each excluded path now widens to the unit the server tracks when nothing is owned at or below it but a strict ancestor is; that unit is released and left out of the apply, "set once" at the granularity the server allows. Tests against envtest: a nodeSelector child survives and the map is not owned; an exclusion inside a Role's rules leaves the whole list alone (before this pass the excluded element was deleted on the next reconcile). LegacyFieldManagers is empty by default: the library folds nothing unless the consumer names a manager. Both reviewers rated the automatic fold the most important finding: a whole entry of that name is taken, so a field another actor wrote under the same binary name would be removed, which the merge-patch enforcer never did. A consumer that knows its history opts in; the namespace-configuration-operator names "manager". Test: with the default, a legacy entry is left alone and its field survives while the rendered field is enforced.
…t pass verdicts, decisions, live measurements
…conciler; the reduced object is built by name Both second-pass reviewers found the same defect: the widening read live ownership as if it were schema. After a granular exclusion was released, its parent looked like an atomic ancestor and the next reconcile widened to the whole map (a ConfigMap's data stopped being enforced); after an atomic map was released nothing recorded that it was atomic, so a later reconcile, or a restarted operator, sent the map without its excluded child and the server deleted the child. The evidence is now the server's own answer for the rendered object: a dry-run apply of the whole object returns the managedFields it would record, schema-shaped, and the unit of each excluded path is the deepest owned ancestor when that ancestor has no children, else the path itself. Computed once per reconciler (the rendered object does not change for its lifetime). The reduced object is built by deleting the units by name rather than through a JSON pointer, which rejected a ConfigMap key named "-1" (measured). An index into a map-keyed list (Deployment containers) still releases the whole list: the merge key is schema the reconciler does not have; documented and tested (the list is set once, nothing deleted; fields outside it stay enforced). Tests, all against envtest: the granular sibling stays enforced through further reconciles; the atomic map survives a fresh reconciler with no memory; a key named "-1" is set once; a Deployment with an index exclusion keeps both containers and keeps owning replicas; the fold writes nothing on its second reconcile.
…e-effect webhook stated
ephico2real2
added a commit
to ephico2real2/namespace-configuration-operator
that referenced
this pull request
Sep 5, 2026
… the SSA enforcer 25 checks on the sandbox against v1.2.6-142; results posted on redhat-cop/operator-utils#104.
Author
Live validation through a consumerRan against a cluster (OpenShift 4.x, single node) with the namespace-configuration-operator fork built on this branch's code: operator
25 ok, 0 failed — operator image sha256:5447cd413962, VERSION: v1.2.6-142-ga8b0ea9 Notes on two rows:
S9 restarted the operator pod: the probe objects' resourceVersions did not change, all 40 objects managed on this cluster were present, and the new pod logged zero error lines. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LockedResourceReconcilerenforces with server-side apply instead of a merge patch.Every reconcile applies the rendered object under one field manager (
lockedresourcecontroller) with force. Drift on an owned field is restored; a field the template no longer renders is removed (the server deletes what its owner stops sending); a field another manager owns and the template never renders is left alone; an apply that changes nothing does not write (resourceVersion unchanged). The diff, and thejsondiffdependency, are gone.excludedPathskeep their meaning, "set once, then left alone", by ownership rather than by omission: the object is created from the whole rendered object, and before every later apply the reconciler drops every path under an excluded path from its own managedFields entry (one JSON patch, sent only when something changes, replacing resourceVersion so a concurrent write conflicts). The server then keeps the field as it is, owned by nobody. The unit released is the one the server tracks: learned from a dry-run apply of the rendered object, once per reconciler, so an exclusion inside an atomic map or list (RBACrules) excludes that unit.Optional, off by default:
LegacyFieldManagersnames client-sideUpdateentries to fold into the apply manager on first contact (client-go'scsaupgrade), so objects created by the previous enforcer lose the fields their template no longer renders. A consumer that knows its history opts in; the fold takes the whole entry of that name, which is why it is not automatic.A locked resource's identity (
GetKey) now includes its excluded paths, so anexcludedPathsedit on a CR restarts its reconciler without an operator restart (it did not before). Also carried: a concurrency-safe template cache (one parsed base per text, cloned and rebound to each caller's config) and excluded-path handling that converts indexes and quoted keys correctly and reports malformed paths instead of silently retargeting them.Why
A merge patch cannot express "this field is no longer rendered". redhat-cop/namespace-configuration-operator#194 reports the symptom (a
ResourceQuotafield set to 0 survives after its conditional stops rendering it); I proposed the fix in #103 by injecting nulls and computing a diff, and the review of #103 suggested server-side apply as the simpler mechanism. This PR is that rework of my fix. Field ownership is the API server's answer to both that and to the.metadataproblem consumers work around by excluding it wholesale (a foreign label was a permanent difference under the merge patch).Proof
Every claim is an envtest-backed test in
pkg/util/lockedresourcecontroller/resource-reconciler_ssa_test.go(make test, orKUBEBUILDER_ASSETS=... go test -race ./pkg/util/lockedresourcecontroller/...): create and restore drift; a dropped field removed; an excluded field set at creation, changed by someone else and kept through further reconciles with the reconciler not owning it;.metadataexcluded keeps the template's labels and a stranger's; a legacy entry folded once; a no-op reconcile keeps the resourceVersion; another manager's field left; an atomic map's excluded child survives a fresh reconciler; an index into a map-keyed list releases the list; a key named-1set once. Also run on a live OpenShift cluster with the namespace-configuration-operator built against this branch.Three adversarial review passes with independent reviewers are recorded in
docs/REVIEW_ssa-enforcer.md, with every verdict re-measured and each decision written down. Supersedes #103.Resolves #105.