v1alpha2: packageMetadata label/annotation deletions are ignored and reverted

[efiacor]

Summary

The two packageMetadata sync directions in the v1alpha2 PR controller disagree on deletion semantics, so a user can never remove a label or annotation once it has been synced. Removals are silently ignored, then reverted on the next render.

Direction	Implementation	Semantics
spec.packageMetadata → Kptfile	applyMetadataMap (controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go:178)	merge only, never deletes
Kptfile → spec.packageMetadata	updateKptfileFields (controllers/packagerevisions/pkg/controllers/packagerevision/status.go:185)	full replace, deletes

This is a v1alpha1 parity regression. v1alpha1 has both modes: PatchKptfile calls applyMetadataToKptfile(kf, obj, true) with replace semantics on the update path (pkg/task/generictaskhandler.go:459), and false on the create path (:103). v1alpha2 only ever merges.

Defect 1 — key removal via spec is ignored, then reverted

1. Kptfile has {keep: yes, remove-me: still-here}, synced into spec.packageMetadata.
2. User removes remove-me from spec.packageMetadata.labels.
3. reconcilePackageMetadata runs applyMetadataMap, which only adds and overwrites. No change detected, nothing written, no render triggered. The Kptfile keeps remove-me.
4. On the next render for any reason (PRR push, another spec edit), updateKptfileFields reads the Kptfile, finds {keep, remove-me} != spec {keep} via packageMetadataEqual, and applies the Kptfile map back into spec — restoring the key the user deleted.

Between steps 3 and 4 the CR and the Kptfile are silently divergent.

Defect 2 — clearing all Kptfile metadata never clears spec

KptfileToPackageMetadata (api/porch/v1alpha2/kptdata_conversion.go:51) returns nil when the Kptfile has no labels and no annotations. updateKptfileFields guards on meta != nil, and additionally early-returns when gates, meta and conditions are all empty. So emptying the Kptfile leaves stale values in spec.packageMetadata indefinitely.

The comment at status.go:207 ("Kptfile is authoritative source for metadata") holds for additions and updates but not for deletions.

Defect 3 — split SSA map ownership blocks pruning

Not reproduced, inferred from SSA semantics plus the generated schema — worth confirming against a live cluster.

spec.packageMetadata.labels has no x-kubernetes-map-type in api/generated/crds/porch.kpt.dev_packagerevisions.yaml, so SSA tracks the map granularly per key. The repo controller seeds keys under field manager repository-controller-seed without ForceOwnership (controllers/repositories/pkg/controllers/repository/pkgrevsync.go:203). packagerev-controller-kptfile only takes ownership of keys it actually sends. Keys seeded on create but absent from a later Kptfile therefore stay owned by the seed manager and are never pruned.

Reproduction

Three tests on branch pkgmeta_deletion_repro, in controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go. They assert the desired behaviour and currently fail:

--- FAIL: TestSpecKeyRemovalPropagatesToKptfile
    label removed from spec.packageMetadata should be removed from the Kptfile
    expected: map[string]string{"keep":"yes"}
    actual  : map[string]string{"keep":"yes", "remove-me":"still-here"}
    annotation removed from spec.packageMetadata should be removed from the Kptfile
    expected: map[string]string{"keep-anno":"yes"}
    actual  : map[string]string{"keep-anno":"yes", "remove-anno":"still-here"}

--- FAIL: TestSpecKeyRemovalIsNotRevertedByKptfileSync
    Kptfile->spec sync must not resurrect a label the user removed from spec
    map[string]string{"keep":"yes", "remove-me":"still-here"} should not contain "remove-me"
    spec and Kptfile must converge after one round trip

--- FAIL: TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied
    emptying the Kptfile should trigger a spec apply to clear packageMetadata

Why existing tests do not catch this

The current merge-only behaviour is deliberately asserted, so this is a design asymmetry rather than code diverging from intent. Any fix has to update these:

• TestApplyMetadataMap / "partial update keeps existing keys" — asserts merge
• TestUpdateKptfileFieldsSkipsWhenEmpty — asserts the empty Kptfile no-op
• e2e test/e2e/crd/metadata_test.go "should not create infinite reconciliation loops" only ever overwrites the same key across iterations, and the merge case only asserts additive behaviour. No e2e case removes a key.

Suggested direction

Decide the ownership model explicitly, then make both directions agree:

• If the Kptfile is authoritative, updateKptfileFields should apply unconditionally (drop the meta != nil guard) so an emptied Kptfile clears spec, and spec.packageMetadata becomes effectively read-only.
• If spec is authoritative for Draft packages, applyMetadataMap needs a replace mode matching v1alpha1's applyMetadataToKptfile(..., true), and the ownership split in defect 3 needs resolving so a single field manager owns the map.

Either way defect 3 should be settled before #947 mirrors these values into metadata.labels.

Relation to #947

#947 proposes mirroring Kptfile labels into metadata.labels for server-side filtering. If that mirror is derived from spec.packageMetadata it inherits all three defects and users get stale query results. Deriving it from the parsed Kptfile at the updateKptfileFields call site, under a single field manager and applied unconditionally, avoids that. Worth fixing this first.