Fix issue #194: Remove fields with value 0 when conditionals change

[ephico2real2]

Status (2026-09-05): superseded by #104. Following the review here, the fix is reworked as server-side apply in #104 (tracking issue #105), which removes fields the template stops rendering without null injection or a diff. #104 carries envtest-backed tests and a live validation run through the namespace-configuration-operator (results). This PR stays open only as the record of the original approach.

PR-194: Remove fields present in actual but missing in expected (handles zero-value cases like "0")

Summary

This change fixes a bug where fields with "zero-like" values (e.g., "0") were not removed when conditionals stop rendering them in templates. The comparison/patch logic previously didn’t emit deletions for keys missing in expected but present in actual.

Implementation

• operator-utils fork/branch/commit:
  • https://github.com/ephico2real2/operator-utils/tree/fix-issue-194-field-removal-zero-value
  • Commit: 9569465
  • New logic: createPatchWithNullFields + addNullFieldsForMissing inject null into JSON merge patches for keys present only in actual, so Kubernetes removes them.

How reviewers can reproduce and verify (using my operator fork)

• Test harness repo (namespace-configuration-operator):
  • https://github.com/ephico2real2/namespace-configuration-operator/tree/feature/finalizer-fixes-template-filtering-tests
  • Working branch: feature/finalizer-fixes-template-filtering-tests
• Consolidated docs in that repo/branch:
  • examples/test-and-logic/ISSUE-194-ROOT-CAUSE-SUMMARY.md
  • examples/test-and-logic/ISSUE-194-FIX-IMPLEMENTATION.md
  • examples/test-and-logic/ISSUE-194-VERIFICATION-GUIDE.md

Wire the fixed dependency

Option A (track branch):

# In namespace-configuration-operator
go get github.com/ephico2real2/operator-utils@fix-issue-194-field-removal-zero-value
go mod tidy

Option B (pin exact commit via pseudo-version):

check examples/test-and-logic/ISSUE-194-FIX-IMPLEMENTATION.md for directions.

// go.mod
replace github.com/redhat-cop/operator-utils => github.com/ephico2real2/operator-utils v0.0.0-20251208075852-9569465257c1

Build & run locally (in namespace-configuration-operator)

./build.sh -o bin/manager main.go
./run-go.sh --skip-build

Test config

examples/test-and-logic/test-issue-194-field-removal-namespaceconfig.yaml
- Matches namespaces labeled test-issue-194=true
- Conditional: if allow-pvc != "true", include spec.hard.persistentvolumeclaims: "0"

Verification steps

1. Initial (no annotation) → field present:

oс create namespace test-issue-194-ns || true
oc label namespace test-issue-194-ns test-issue-194=true --overwrite
oc apply -f examples/test-and-logic/test-issue-194-field-removal-namespaceconfig.yaml
oc get resourcequota test-issue-194-quota -n test-issue-194-ns -o jsonpath='{.spec.hard.persistentvolumeclaims}' && echo
# Expect: 0

2. Set annotation allow-pvc=true → field removed:

oc annotate namespace test-issue-194-ns allow-pvc=true --overwrite
sleep 8
oc get resourcequota test-issue-194-quota -n test-issue-194-ns -o jsonpath='{.spec.hard.persistentvolumeclaims}' && echo
# Expect: (empty)

3. Remove annotation → field added back:

oc annotate namespace test-issue-194-ns allow-pvc-
sleep 8
oc get resourcequota test-issue-194-quota -n test-issue-194-ns -o jsonpath='{.spec.hard.persistentvolumeclaims}' && echo
# Expect: 0

Real-time proof (timestamps + full YAML)

Server-side apply captures managedFields.time:

oc apply -f examples/test-and-logic/test-issue-194-field-removal-namespaceconfig.yaml --server-side --field-manager=issue-194-test
oc get namespaceconfig test-issue-194-field-removal -o json | jq -r '.metadata.managedFields | sort_by(.time) | last | .time'

Capture and diff YAML:

oc get resourcequota test-issue-194-quota -n test-issue-194-ns -o yaml > /tmp/rq-before.yaml
oc annotate namespace test-issue-194-ns allow-pvc=true --overwrite && sleep 8
oc get resourcequota test-issue-194-quota -n test-issue-194-ns -o yaml > /tmp/rq-after.yaml
diff -u /tmp/rq-before.yaml /tmp/rq-after.yaml | sed -n '1,200p'

Live YAML excerpt (after fix — allow-pvc=true) and the original template snippet are embedded in:

• examples/test-and-logic/ISSUE-194-VERIFICATION-GUIDE.md

Commands used to identify the correct module

grep -r "func.*UpdateLockedResources" controllers/

go doc github.com/redhat-cop/operator-utils/pkg/util/lockedresourcecontroller.EnforcingReconciler.UpdateLockedResources

grep -A 5 "type NamespaceConfigReconciler struct" controllers/namespaceconfig_controller.go

grep -B 2 -A 2 "UpdateLockedResources" controllers/namespaceconfig_controller.go

go list -m -versions github.com/redhat-cop/operator-utils

go list -m github.com/redhat-cop/operator-utils

Impact & compatibility

• Generic fix for any resource/field removed by conditional rendering.
• Uses standard JSON Merge Patch semantics (null deletes); respects excluded paths; supports nested structures.

Checklist

• Fix implemented with recursive null-injection for missing keys

• Verified locally and on a cluster with before/after YAML diffs

• No changes to public API of operator-utils

• Backwards compatible

• Add createPatchWithNullFields to set missing fields to null in merge patches

• Add addNullFieldsForMissing helper to recursively handle nested structures

• Ensures fields are properly removed when they should be absent

• Fixes bug where fields with value "0" are not removed when conditionals change from true to false

[Kajot-dev]

Using server side apply would be enough, no need to pass nulls and compute diffs

[ephico2real2]

Agreed on the mechanism, with three things the PR would need to handle before "enough" holds. I measured the four cases below against a live API server with apply --server-side and a fixed field manager, since they decide the design:

case	result
apply {a,b} then apply {a} with the same manager	b removed: the #194 case, solved with no nulls and no diff
object created through a plain create (an Update manager), then a smaller SSA apply with force	the stale field survives: SSA removes only what the apply manager owns
someone else adds a label, then a no-op SSA apply	label kept, resourceVersion unchanged: no write, no event
a managed field is edited by hand, SSA apply with force	value restored

So SSA solves #194 at the server and is also safer than this PR's null injection, which removes any live-only field (for example a ServiceAccount's controller-populated secrets), while SSA leaves fields owned by others alone. isEqual and the diff go away too, since a no-op apply costs nothing.

What the rework has to define:

1. Migration. Every object the enforcer created so far went through the non-apply path, so its fields are owned by an Update manager. A field the template later stops rendering is not removed on those objects (case 2). Either the library takes ownership once and removes what the desired object no longer carries, or it falls back to the current path when managedFields has no Apply entry for its manager, or this is documented as the operator's responsibility.
2. excludedPaths semantics. Today an excluded path is ignored in the comparison and stripped from the patch. Under SSA it would be stripped from the applied object, and if the manager previously owned that field the apply then removes it: excluding a path after the fact deletes the field instead of releasing it. Needs a stated rule and a test.
3. Write pattern. The child reconciler goes from GET-and-patch-on-drift to one apply per reconcile. No-op applies do not bump resourceVersion, so there is no event loop, but it is one request per reconcile per object; worth stating.

A side benefit worth noting: with SSA the wholesale .metadata exclusion is no longer needed for the reason it exists (an extra label added by someone else no longer causes endless patching), so label and annotation drift on managed objects could finally be corrected.

I'm happy to rework this PR onto ApplyPatchType with a fixed field manager and Force: true, drop the null injection and the diff, keep excludedPaths as strip-before-apply with the rule above, and add an envtest for case 1 and the case-2 migration. Would you prefer the migration handled in the library or documented?

[ephico2real2]

@Kajot-dev one clarification so I rework this the way you intend. Which of these did you mean?

1. Inside operator-utils: LockedResourceReconciler should apply the rendered object with server-side apply (ApplyPatchType, a fixed field manager, force) instead of the merge patch, so the API server drops fields the template no longer renders. That is a change to this file, replacing the null/diff logic in this PR.

2. Outside operator-utils: the consumer (in my case the namespace-configuration-operator, whose NamespaceConfig CR carries these templates) should use server-side apply on its side, and the library stays as it is.

My understanding is that the object which loses the field is the managed object (the RoleBinding or ConfigMap the template renders), and the only code that writes it is this reconciler, so the apply would have to happen here (reading 1). How the consumer applies its own CR would not change what this reconciler sends. If you meant reading 2, could you say where the apply would go?

[ephico2real2]

Following the discussion here, the server-side-apply approach is now a separate PR: #104. It replaces the null injection and diff computation with one forced apply per reconcile under a fixed field manager, keeps excludedPaths "set once, then left alone" by releasing ownership of them, and carries a concurrency-safe template cache and the excluded-path fixes. Every claim has an envtest-backed test; the review record is in the branch. I would suggest evaluating #104 and treating this one as superseded.

[ephico2real2]

@Kajot-dev, closing the loop on your suggestion: the server-side-apply rework is implemented in #104, with issue #105 as its tracking issue. It does what you described and no more than that: one forced apply per reconcile under a fixed field manager, so the API server removes a field the template stops rendering (the #194 case, "0" included) and leaves fields owned by others alone; excludedPaths keep their "set once, then left alone" meaning by releasing ownership rather than by omitting the field.

Proof on #104: envtest-backed tests for every claim, three recorded review passes, and a live run through the namespace-configuration-operator (25 checks, including the "0" removal, an atomic-list exclusion, a legacy client-side entry folded, and a no-op reconcile writing nothing across an operator restart).

I would prefer to close this PR in favour of #104; happy to split #104 (cache fix, path parsing, enforcer) if that helps the review.