From 4298c4e2d71ba80ac453d10ed17dd3b55ee5f886 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Thu, 27 Aug 2026 13:27:28 +0100 Subject: [PATCH 1/7] Add pkgMetadata lable select on CRD Signed-off-by: Fiachra Corcoran --- .../controllers/packagerevision/metadata.go | 27 +- .../packagerevision/metadata_deletion_test.go | 142 ++++++++ .../packagerevision/metadata_test.go | 44 +-- .../pkg/controllers/packagerevision/status.go | 99 +++++- .../packagerevision/status_test.go | 29 ++ .../inspecting-packages.md | 33 ++ test/e2e/crd/metadata_test.go | 311 +++++++++++++++++- 7 files changed, 620 insertions(+), 65 deletions(-) create mode 100644 controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go index 2a49b7b3b..c2c0331bc 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go @@ -16,6 +16,7 @@ package packagerevision import ( "context" + "maps" kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" @@ -160,7 +161,8 @@ func (r *PackageRevisionReconciler) triggerRenderIfNeeded(ctx context.Context, p return nil, nil } -// applyPackageMetadataToKptfile applies labels and annotations to Kptfile (merge mode). +// applyPackageMetadataToKptfile applies labels and annotations to the Kptfile. +// spec.packageMetadata is the complete desired set, so omitted keys are removed. func applyPackageMetadataToKptfile(kf *kptfilev1.KptFile, pr *porchv1alpha2.PackageRevision) bool { if pr.Spec.PackageMetadata == nil { return false @@ -175,26 +177,17 @@ func applyPackageMetadataToKptfile(kf *kptfilev1.KptFile, pr *porchv1alpha2.Pack return labelsChanged || annotationsChanged } -// applyMetadataMap merges desired key-value pairs into current, returning the resulting map and whether any changes were made. -// Safe to call with nil current or desired maps. +// applyMetadataMap replaces current with desired, reporting whether it changed. +// nil desired means the client is not managing the map, so current is untouched; +// an empty non-nil map clears it. Matches v1alpha1 applyMapMetadata. func applyMetadataMap(current, desired map[string]string) (map[string]string, bool) { - if len(desired) == 0 { + if desired == nil || maps.Equal(current, desired) { return current, false } - - if current == nil { - current = make(map[string]string, len(desired)) - } - - changed := false - for k, v := range desired { - if cv, exists := current[k]; !exists || cv != v { - current[k] = v - changed = true - } + if len(desired) == 0 { + return nil, true } - - return current, changed + return maps.Clone(desired), true } // setRenderRequestAnnotation triggers render by updating the render-request annotation with nanosecond precision. diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go new file mode 100644 index 000000000..27a6d8aac --- /dev/null +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go @@ -0,0 +1,142 @@ +// Copyright 2026 The kpt 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. + +// Reproduction tests for packageMetadata key deletion. +// +// These assert the desired behaviour and fail against the current +// implementation, so they are skipped. Remove the skips as part of the fix that +// makes the two sync directions agree on deletion. +// +// The two sync directions disagree on deletion semantics: +// +// spec -> Kptfile (applyMetadataMap) merge only, never deletes +// Kptfile -> spec (updateKptfileFields) full replace, deletes +// +// v1alpha1 has both modes: pkg/task/generictaskhandler.go PatchKptfile calls +// applyMetadataToKptfile(kf, obj, true) with replace semantics on the update +// path, and false on the create path. v1alpha2 only ever merges. + +package packagerevision + +import ( + "context" + "testing" + + kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" + porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" + mockclient "github.com/kptdev/porch/test/mockery/mocks/external/sigs.k8s.io/controller-runtime/pkg/client" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +// kptfileWithMetadata builds a Kptfile carrying the given labels and annotations. +func kptfileWithMetadata(labels, annotations map[string]string) kptfilev1.KptFile { + return kptfilev1.KptFile{ + ResourceMeta: yaml.ResourceMeta{ + ObjectMeta: yaml.ObjectMeta{ + Labels: labels, + Annotations: annotations, + }, + }, + } +} + +// TestSpecKeyRemovalPropagatesToKptfile covers defect 1: a user removing a key +// from spec.packageMetadata should remove it from the Kptfile. Today +// applyMetadataMap only adds and overwrites, so the key survives. +func TestSpecKeyRemovalPropagatesToKptfile(t *testing.T) { + kf := kptfileWithMetadata( + map[string]string{"keep": "yes", "remove-me": "still-here"}, + map[string]string{"keep-anno": "yes", "remove-anno": "still-here"}, + ) + + // User has dropped "remove-me" / "remove-anno" from the CR spec. + pr := newTestPR( + withLifecycle(porchv1alpha2.PackageRevisionLifecycleDraft), + withMetadata( + map[string]string{"keep": "yes"}, + map[string]string{"keep-anno": "yes"}, + ), + ) + + changed := applyPackageMetadataToKptfile(&kf, pr) + + assert.True(t, changed, "removing a key from spec should be a change to apply") + assert.Equal(t, map[string]string{"keep": "yes"}, kf.Labels, + "label removed from spec.packageMetadata should be removed from the Kptfile") + assert.Equal(t, map[string]string{"keep-anno": "yes"}, kf.Annotations, + "annotation removed from spec.packageMetadata should be removed from the Kptfile") +} + +// TestSpecKeyRemovalIsNotRevertedByKptfileSync covers the second half of +// defect 1: because the Kptfile keeps the dropped key, the next render syncs it +// straight back into spec, silently undoing the user's edit. +func TestSpecKeyRemovalIsNotRevertedByKptfileSync(t *testing.T) { + kf := kptfileWithMetadata(map[string]string{"keep": "yes", "remove-me": "still-here"}, nil) + + pr := newTestPR( + withLifecycle(porchv1alpha2.PackageRevisionLifecycleDraft), + withMetadata(map[string]string{"keep": "yes"}, nil), + ) + + // Step 1: spec -> Kptfile. Should drop "remove-me" from the Kptfile. + applyPackageMetadataToKptfile(&kf, pr) + + // Step 2: Kptfile -> spec, as run post-render by updateKptfileFields. + synced := porchv1alpha2.KptfileToPackageMetadata(kf) + + assert.NotContains(t, synced.Labels, "remove-me", + "Kptfile->spec sync must not resurrect a label the user removed from spec") + assert.True(t, packageMetadataEqual(pr.Spec.PackageMetadata, synced), + "spec and Kptfile must converge after one round trip") +} + +// TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied covers defect 2: +// KptfileToPackageMetadata returns nil for a Kptfile with no labels or +// annotations, and updateKptfileFields guards on meta != nil, so emptying the +// Kptfile leaves stale values in spec.packageMetadata forever. +func TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied(t *testing.T) { + mockClient := mockclient.NewMockClient(t) + + var specPatch porchv1alpha2.PackageRevisionSpec + patched := false + + mockClient.EXPECT().Get(mock.Anything, mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything). + Run(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) { + *obj.(*porchv1alpha2.PackageRevision) = *basePR() + }).Return(nil).Maybe() + + mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). + Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { + specPatch = obj.(*porchv1alpha2.PackageRevision).Spec + patched = true + }).Return(nil).Maybe() + + r := &PackageRevisionReconciler{Client: mockClient} + + // spec carries metadata that was synced from an earlier Kptfile revision. + pr := basePR() + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"stale": "value"}, + } + + // The Kptfile has since had all labels and annotations removed. + r.updateKptfileFields(t.Context(), pr, kptfilev1.KptFile{}) + + assert.True(t, patched, "emptying the Kptfile should trigger a spec apply to clear packageMetadata") + assert.Nil(t, specPatch.PackageMetadata, + "spec.packageMetadata should be cleared when the Kptfile has no labels or annotations") +} diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_test.go index 5b5f0e11b..4fbe59239 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_test.go @@ -102,7 +102,7 @@ func TestApplyPackageMetadataToKptfile(t *testing.T) { expectLabels: map[string]string{"app": "myapp"}, }, { - name: "merge labels", + name: "spec replaces existing labels", kf: &kptfilev1.KptFile{ ResourceMeta: yaml.ResourceMeta{ ObjectMeta: yaml.ObjectMeta{ @@ -112,7 +112,7 @@ func TestApplyPackageMetadataToKptfile(t *testing.T) { }, prOpts: []func(*porchv1alpha2.PackageRevision){withMetadata(map[string]string{"app": "myapp"}, nil)}, expectChanged: true, - expectLabels: map[string]string{"existing": "label", "app": "myapp"}, + expectLabels: map[string]string{"app": "myapp"}, }, { name: "add annotations", @@ -184,11 +184,11 @@ func TestApplyMetadataMap(t *testing.T) { expectResult: map[string]string{"app": "test"}, }, { - name: "merge into existing map", + name: "desired replaces existing map", current: map[string]string{"env": "prod"}, desired: map[string]string{"app": "test"}, expectChanged: true, - expectResult: map[string]string{"env": "prod", "app": "test"}, + expectResult: map[string]string{"app": "test"}, }, { name: "no change when identical", @@ -205,28 +205,35 @@ func TestApplyMetadataMap(t *testing.T) { expectResult: map[string]string{"app": "new"}, }, { - name: "merge multiple entries", + name: "disjoint desired drops all existing entries", current: map[string]string{"a": "1", "b": "2"}, desired: map[string]string{"c": "3", "d": "4"}, expectChanged: true, - expectResult: map[string]string{"a": "1", "b": "2", "c": "3", "d": "4"}, + expectResult: map[string]string{"c": "3", "d": "4"}, }, { - name: "partial update keeps existing keys", + name: "keys omitted from desired are dropped", current: map[string]string{"keep": "this", "update": "old"}, desired: map[string]string{"update": "new"}, expectChanged: true, - expectResult: map[string]string{"keep": "this", "update": "new"}, + expectResult: map[string]string{"update": "new"}, }, { - name: "empty desired map (no change)", + name: "empty desired map clears", current: map[string]string{"existing": "val"}, desired: map[string]string{}, + expectChanged: true, + expectResult: nil, + }, + { + name: "empty desired map against empty current is no change", + current: nil, + desired: map[string]string{}, expectChanged: false, - expectResult: map[string]string{"existing": "val"}, + expectResult: nil, }, { - name: "nil desired map (no change)", + name: "nil desired map means not managed, leaves current alone", current: map[string]string{"existing": "val"}, desired: nil, expectChanged: false, @@ -289,7 +296,7 @@ func TestApplyPackageMetadataToKptfileComprehensive(t *testing.T) { }, }, { - name: "both labels and annotations with existing values (merge)", + name: "both labels and annotations with existing values (replace)", kf: &kptfilev1.KptFile{ ResourceMeta: yaml.ResourceMeta{ ObjectMeta: yaml.ObjectMeta{ @@ -301,10 +308,8 @@ func TestApplyPackageMetadataToKptfileComprehensive(t *testing.T) { prOpts: []func(*porchv1alpha2.PackageRevision){withMetadata(map[string]string{"new-l": "val"}, map[string]string{"new-a": "val"})}, expectChanged: true, verify: func(t *testing.T, kf *kptfilev1.KptFile) { - assert.Equal(t, "val", kf.ResourceMeta.ObjectMeta.Labels["existing-l"]) - assert.Equal(t, "val", kf.ResourceMeta.ObjectMeta.Labels["new-l"]) - assert.Equal(t, "val", kf.ResourceMeta.ObjectMeta.Annotations["existing-a"]) - assert.Equal(t, "val", kf.ResourceMeta.ObjectMeta.Annotations["new-a"]) + assert.Equal(t, map[string]string{"new-l": "val"}, kf.ResourceMeta.ObjectMeta.Labels) + assert.Equal(t, map[string]string{"new-a": "val"}, kf.ResourceMeta.ObjectMeta.Annotations) }, }, { @@ -456,7 +461,7 @@ func TestApplyPackageMetadataToKptfileEdgeCases(t *testing.T) { }, }, { - name: "only update one label out of many existing", + name: "single-entry spec replaces the whole label set", kf: &kptfilev1.KptFile{ ResourceMeta: yaml.ResourceMeta{ ObjectMeta: yaml.ObjectMeta{ @@ -469,10 +474,7 @@ func TestApplyPackageMetadataToKptfileEdgeCases(t *testing.T) { prOpts: []func(*porchv1alpha2.PackageRevision){withMetadata(map[string]string{"b": "updated"}, nil)}, expectChanged: true, verify: func(t *testing.T, kf *kptfilev1.KptFile) { - assert.Equal(t, "1", kf.ResourceMeta.ObjectMeta.Labels["a"]) - assert.Equal(t, "updated", kf.ResourceMeta.ObjectMeta.Labels["b"]) - assert.Equal(t, "3", kf.ResourceMeta.ObjectMeta.Labels["c"]) - assert.Equal(t, "4", kf.ResourceMeta.ObjectMeta.Labels["d"]) + assert.Equal(t, map[string]string{"b": "updated"}, kf.ResourceMeta.ObjectMeta.Labels) }, }, } diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go index d7588e17a..9341d73ca 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go @@ -17,10 +17,13 @@ package packagerevision import ( "context" "maps" + "slices" + "strings" kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" "github.com/kptdev/porch/pkg/repository" + "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -30,6 +33,11 @@ const ( fieldManagerPRController = "packagerev-controller" fieldManagerPRControllerRender = "packagerev-controller-render" fieldManagerPRControllerKptfile = "packagerev-controller-kptfile" + + // Prefix for mirrored Kptfile labels in object metadata.labels. + // Enables field selectors via label queries: -l porch.kpt.dev/kptfile-label__key=value + // Slash is escaped as double-underscore per Kubernetes label key restrictions. + kptfileLabelPrefix = "porch.kpt.dev/kptfile-label__" ) // updateStatus applies the PR-controller-owned status fields via SSA. @@ -189,28 +197,29 @@ func (r *PackageRevisionReconciler) updateKptfileFields(ctx context.Context, pr meta := porchv1alpha2.KptfileToPackageMetadata(kf) conds := porchv1alpha2.KptfileToPackageConditions(kf) - if len(gates) == 0 && meta == nil && len(conds) == 0 { - return + // Always send both fields: applySpec replaces the whole spec, so omitting an + // unchanged field would prune it. Empty is a real desired state — that is how + // Kptfile deletions propagate. + spec := porchv1alpha2.PackageRevisionSpec{ + ReadinessGates: gates, + PackageMetadata: meta, } - // Batch spec fields into single SSA patch to ensure atomic updates. - // Multiple separate patches can cause visibility issues where subsequent reads don't see all changes. - spec := porchv1alpha2.PackageRevisionSpec{} - hasSpecFields := false + gatesChanged := !equality.Semantic.DeepEqual(pr.Spec.ReadinessGates, gates) + metaChanged := !packageMetadataEqual(pr.Spec.PackageMetadata, meta) - if len(gates) > 0 { - spec.ReadinessGates = gates - hasSpecFields = true + if gatesChanged || metaChanged { + r.applySpec(ctx, pr, spec) } - // Kptfile is authoritative source for metadata. Sync if it differs from spec. - if meta != nil && !packageMetadataEqual(pr.Spec.PackageMetadata, meta) { - spec.PackageMetadata = meta - hasSpecFields = true + // Mirror Kptfile labels into object metadata.labels for field selector queries. + var kfLabels map[string]string + if meta != nil { + kfLabels = meta.Labels } - - if hasSpecFields { - r.applySpec(ctx, pr, spec) + objLabels, labelsChanged := kptfileLabelsToObjectLabels(pr.Labels, kfLabels) + if labelsChanged { + r.applyObjectLabels(ctx, pr, objLabels) } // Apply conditions via status API (separate endpoint from spec). @@ -244,6 +253,18 @@ func (r *PackageRevisionReconciler) applyStatus(ctx context.Context, pr *porchv1 } } +func (r *PackageRevisionReconciler) applyObjectLabels(ctx context.Context, pr *porchv1alpha2.PackageRevision, labels map[string]string) { + log := log.FromContext(ctx) + + obj := &porchv1alpha2.PackageRevision{ + TypeMeta: metav1.TypeMeta{Kind: "PackageRevision", APIVersion: porchv1alpha2.SchemeGroupVersion.Identifier()}, + ObjectMeta: metav1.ObjectMeta{Name: pr.Name, Namespace: pr.Namespace, Labels: labels}, + } + if err := r.Patch(ctx, obj, client.Apply, client.FieldOwner(fieldManagerPRControllerKptfile), client.ForceOwnership); err != nil { + log.Error(err, "failed to apply object labels") + } +} + // packageMetadataEqual returns true if two PackageMetadata values have identical labels and annotations. func packageMetadataEqual(a, b *porchv1alpha2.PackageMetadata) bool { if a == nil && b == nil { @@ -254,3 +275,49 @@ func packageMetadataEqual(a, b *porchv1alpha2.PackageMetadata) bool { } return maps.Equal(a.Labels, b.Labels) && maps.Equal(a.Annotations, b.Annotations) } + +// kptfileLabelsToObjectLabels mirrors Kptfile labels into object metadata.labels +// with a reserved prefix, enabling field selectors via label queries. +// Kptfile labels with "/" are escaped to "__" for Kubernetes label key compatibility. +// Only labels are mirrored (not annotations per spike findings). +// Returns the updated labels and a bool indicating whether they changed. +func kptfileLabelsToObjectLabels(current, kptfileLabels map[string]string) (map[string]string, bool) { + if len(kptfileLabels) == 0 { + // No Kptfile labels; remove any existing mirror labels from current. + var updated map[string]string + changed := false + for k, v := range current { + if !strings.HasPrefix(k, kptfileLabelPrefix) { + if updated == nil { + updated = make(map[string]string) + } + updated[k] = v + } else { + changed = true + } + } + return updated, changed + } + + // Build desired Kptfile mirror labels: sort for determinism, then apply. + desired := make(map[string]string) + keys := slices.Sorted(maps.Keys(kptfileLabels)) + for _, k := range keys { + // Escape "/" as "__" to fit Kubernetes label key constraints. + mirrorKey := kptfileLabelPrefix + strings.ReplaceAll(k, "/", "__") + desired[mirrorKey] = kptfileLabels[k] + } + + // Merge with non-mirror labels from current. + updated := make(map[string]string) + for k, v := range current { + if !strings.HasPrefix(k, kptfileLabelPrefix) { + updated[k] = v + } + } + maps.Copy(updated, desired) + + // Check if changed by comparing against current. + changed := !maps.Equal(current, updated) + return updated, changed +} diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go index 2a905dfef..9dc79fa02 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go @@ -446,6 +446,35 @@ func TestUpdateKptfileFieldsMetadataUnchangedSkips(t *testing.T) { r.updateKptfileFields(t.Context(), pr, kf) } +// Gates removal must be decided on its own, not as a side effect of metadata +// changing. applySpec sends the whole spec, so if this were length-gated a +// removed readinessGate would only be pruned when metadata happened to differ. +func TestUpdateKptfileFieldsGatesRemovedWithMetadataUnchanged(t *testing.T) { + mockClient := mockclient.NewMockClient(t) + + var specPatch porchv1alpha2.PackageRevisionSpec + mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). + Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { + specPatch = obj.(*porchv1alpha2.PackageRevision).Spec + }).Return(nil) + + r := &PackageRevisionReconciler{Client: mockClient} + + pr := basePR() + pr.Spec.ReadinessGates = []porchv1alpha2.ReadinessGate{{ConditionType: "Ready"}} + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{Labels: map[string]string{"env": "prod"}} + + // Kptfile still carries the same metadata, but the readinessGate is gone. + kf := kptfilev1.KptFile{} + kf.Labels = map[string]string{"env": "prod"} + + r.updateKptfileFields(t.Context(), pr, kf) + + assert.Nil(t, specPatch.ReadinessGates, "omitting gates from the applied config is what prunes them") + assert.NotNil(t, specPatch.PackageMetadata, "unchanged metadata must still be sent so SSA does not prune it") + assert.Equal(t, "prod", specPatch.PackageMetadata.Labels["env"]) +} + func TestUpdateKptfileFieldsSpecPatchError(t *testing.T) { mockClient := mockclient.NewMockClient(t) mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). diff --git a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md index 8949a9f47..b086d7fea 100644 --- a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md +++ b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md @@ -320,6 +320,39 @@ The `--field-selector` flag supports only the `=` and `==` operators. **The `!=` --- +### Filtering by PackageMetadata Labels + +You can also filter PackageRevisions by the labels defined in `spec.packageMetadata`. Porch mirrors these labels to the PackageRevision object's metadata, making them queryable via standard Kubernetes label selectors. + +Kptfile labels are mirrored with the prefix `porch.kpt.dev/kptfile-label__`, with slashes (/) escaped as `__` for Kubernetes label key compatibility. + +Filter by packageMetadata label: + +```bash +kubectl get packagerevisions -n default --selector 'porch.kpt.dev/kptfile-label__env=prod' +``` + +Filter by packageMetadata label containing a slash: + +```bash +kubectl get packagerevisions -n default --selector 'porch.kpt.dev/kptfile-label__app.example.com__name=myapp' +``` + +Combine label selectors: + +```bash +kubectl get packagerevisions -n default \ + --selector 'porch.kpt.dev/kptfile-label__env=prod,porch.kpt.dev/kptfile-label__tier=backend' +``` + +{{% alert title="Note" color="primary" %}} +- Only labels from `spec.packageMetadata.labels` are mirrored to object labels +- Annotations in `spec.packageMetadata.annotations` are stored for reference but are not queryable +- This mirroring enables v1alpha2 feature parity with v1alpha1's `spec.packageMetadata.labels[key]=value` field selectors +{{% /alert %}} + +--- + ## Additional Operations Beyond basic listing and filtering, these operations help you monitor changes and format output. diff --git a/test/e2e/crd/metadata_test.go b/test/e2e/crd/metadata_test.go index e25e2d62f..3766ceb61 100644 --- a/test/e2e/crd/metadata_test.go +++ b/test/e2e/crd/metadata_test.go @@ -114,9 +114,184 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { Context("PackageMetadata Field Selectors", func() { // v1alpha1 supported filtering by spec.packageMetadata.labels[key]=value. - // v1alpha2 CRD field indexes don't include packageMetadata yet. - // TODO: implement packageMetadata field indexes in fieldindex.go and enable. - PIt("should filter by packageMetadata labels") + // CRD selectableFields cannot express this: bracket notation is rejected, + // and dot notation can't represent keys containing "." or "/". + // Solution: mirror Kptfile labels into metadata.labels with prefix + // "porch.kpt.dev/kptfile-label__", escaping "/" as "__". Query with: + // -l porch.kpt.dev/kptfile-label__key=value + It("should filter by packageMetadata labels via label selector", func() { + By("creating a draft package") + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-filter", "v1", withInit("label filter test")) + Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) + waitForReady(env.Ctx, pr) + waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) + + By("patching spec.packageMetadata with labels") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"env": "prod", "tier": "backend"}, + } + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("waiting for metadata to sync to Kptfile") + Eventually(func(g Gomega) { + resources := getPRRResources(env.Ctx, env.Namespace, pr.Name) + g.Expect(resources).To(HaveKey("Kptfile")) + g.Expect(resources["Kptfile"]).To(ContainSubstring("env: prod")) + g.Expect(resources["Kptfile"]).To(ContainSubstring("tier: backend")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("waiting for labels to be mirrored to object metadata") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.ObjectMeta.Labels).To(HaveKeyWithValue("porch.kpt.dev/kptfile-label__env", "prod")) + g.Expect(pr.ObjectMeta.Labels).To(HaveKeyWithValue("porch.kpt.dev/kptfile-label__tier", "backend")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("querying via label selector with porch.kpt.dev/kptfile-label__env=prod") + var prList porchv1alpha2.PackageRevisionList + Err := k8sClient.List(env.Ctx, &prList, + client.InNamespace(env.Namespace), + client.MatchingLabels{"porch.kpt.dev/kptfile-label__env": "prod"}, + ) + Expect(Err).NotTo(HaveOccurred()) + foundName := false + for _, item := range prList.Items { + if item.Name == pr.Name { + foundName = true + break + } + } + Expect(foundName).To(BeTrue(), "package should be found via env=prod label selector") + + By("querying via label selector with porch.kpt.dev/kptfile-label__tier=backend") + Err = k8sClient.List(env.Ctx, &prList, + client.InNamespace(env.Namespace), + client.MatchingLabels{"porch.kpt.dev/kptfile-label__tier": "backend"}, + ) + Expect(Err).NotTo(HaveOccurred()) + foundName = false + for _, item := range prList.Items { + if item.Name == pr.Name { + foundName = true + break + } + } + Expect(foundName).To(BeTrue(), "package should be found via tier=backend label selector") + + By("verifying non-matching selector returns empty") + Err = k8sClient.List(env.Ctx, &prList, + client.InNamespace(env.Namespace), + client.MatchingLabels{"porch.kpt.dev/kptfile-label__env": "dev"}, + ) + Expect(Err).NotTo(HaveOccurred()) + foundName = false + for _, item := range prList.Items { + if item.Name == pr.Name { + foundName = true + break + } + } + Expect(foundName).To(BeFalse(), "package should not be found via env=dev label selector") + }) + + It("should escape slashes in label keys when mirroring to object labels", func() { + By("creating a draft package") + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-escape", "v1", withInit("label escaping test")) + Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) + waitForReady(env.Ctx, pr) + waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) + + By("patching spec.packageMetadata with label keys containing slashes") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{ + "app.example.com/name": "myapp", + "kpt.dev/version": "v1beta1", + }, + } + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("waiting for labels with slashes to be synced and escaped") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + // Slashes should be escaped as "__" in the label key + g.Expect(pr.ObjectMeta.Labels).To(HaveKeyWithValue("porch.kpt.dev/kptfile-label__app.example.com__name", "myapp")) + g.Expect(pr.ObjectMeta.Labels).To(HaveKeyWithValue("porch.kpt.dev/kptfile-label__kpt.dev__version", "v1beta1")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("querying via label selector using escaped key") + var prList porchv1alpha2.PackageRevisionList + Err := k8sClient.List(env.Ctx, &prList, + client.InNamespace(env.Namespace), + client.MatchingLabels{"porch.kpt.dev/kptfile-label__app.example.com__name": "myapp"}, + ) + Expect(Err).NotTo(HaveOccurred()) + foundName := false + for _, item := range prList.Items { + if item.Name == pr.Name { + foundName = true + break + } + } + Expect(foundName).To(BeTrue(), "package should be found via escaped label key selector") + }) + + It("should handle label key updates and propagate to object labels", func() { + By("creating a draft package") + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-update", "v1", withInit("label update test")) + Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) + waitForReady(env.Ctx, pr) + waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) + + By("setting initial labels") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"version": "v1"}, + } + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("waiting for labels to sync to object") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.ObjectMeta.Labels).To(HaveKeyWithValue("porch.kpt.dev/kptfile-label__version", "v1")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("updating label value") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + pr.Spec.PackageMetadata.Labels["version"] = "v2" + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("verifying object label value is updated") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.ObjectMeta.Labels).To(HaveKeyWithValue("porch.kpt.dev/kptfile-label__version", "v2")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("querying with updated label value") + var prList porchv1alpha2.PackageRevisionList + Err := k8sClient.List(env.Ctx, &prList, + client.InNamespace(env.Namespace), + client.MatchingLabels{"porch.kpt.dev/kptfile-label__version": "v2"}, + ) + Expect(Err).NotTo(HaveOccurred()) + foundName := false + for _, item := range prList.Items { + if item.Name == pr.Name { + foundName = true + break + } + } + Expect(foundName).To(BeTrue(), "package should be found with updated label value") + }) }) Context("Kptfile Metadata Sync (Kptfile → CRD)", func() { @@ -211,9 +386,9 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) }) - It("should merge metadata with existing Kptfile labels and annotations", func() { + It("should replace existing Kptfile labels and annotations with spec.packageMetadata", func() { By("creating a draft package") - pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-merge", "v1", withInit("merge test")) + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-merge", "v1", withInit("replace test")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) waitForReady(env.Ctx, pr) @@ -243,15 +418,59 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { By("waiting for render") waitForRendered(env.Ctx, pr) - By("verifying Kptfile has both existing and new labels/annotations (merge)") - waitForRendered(env.Ctx, pr) + By("verifying spec is the complete desired set, so omitted keys are dropped") Eventually(func(g Gomega) { resources := getPRRResources(env.Ctx, env.Namespace, pr.Name) kf := resources["Kptfile"] - g.Expect(kf).To(ContainSubstring("existing: label"), "existing label should be preserved") - g.Expect(kf).To(ContainSubstring("new: label"), "new label should be added") - g.Expect(kf).To(ContainSubstring("existing-anno: value"), "existing annotation should be preserved") - g.Expect(kf).To(ContainSubstring("new-anno: new-value"), "new annotation should be added") + g.Expect(kf).To(ContainSubstring("new: label"), "new label should be applied") + g.Expect(kf).To(ContainSubstring("new-anno: new-value"), "new annotation should be applied") + g.Expect(kf).NotTo(ContainSubstring("existing: label"), "label omitted from spec should be removed") + g.Expect(kf).NotTo(ContainSubstring("existing-anno: value"), "annotation omitted from spec should be removed") + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + }) + + It("should remove a single key omitted from spec.packageMetadata", func() { + By("creating a draft package") + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-key-removal", "v1", withInit("key removal test")) + Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) + waitForReady(env.Ctx, pr) + waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) + + By("pushing a Kptfile with two labels") + updatePRRResources(env.Ctx, env.Namespace, pr.Name, map[string]string{ + "Kptfile": "apiVersion: kpt.dev/v1\nkind: Kptfile\nmetadata:\n name: pkg-key-removal\n labels:\n keep-me: yes-please\n drop-me: should-vanish\npipeline: {}\n", + "res.yaml": "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: test\ndata:\n key: val\n", + }) + waitForRendered(env.Ctx, pr) + + By("waiting for both labels to be projected into spec.packageMetadata") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.Spec.PackageMetadata).NotTo(BeNil()) + g.Expect(pr.Spec.PackageMetadata.Labels).To(HaveKeyWithValue("keep-me", "yes-please")) + g.Expect(pr.Spec.PackageMetadata.Labels).To(HaveKeyWithValue("drop-me", "should-vanish")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("dropping one key from spec.packageMetadata") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"keep-me": "yes-please"}, + } + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + waitForRendered(env.Ctx, pr) + + By("verifying the key is gone from the Kptfile and stays gone in spec") + Eventually(func(g Gomega) { + resources := getPRRResources(env.Ctx, env.Namespace, pr.Name) + g.Expect(resources["Kptfile"]).To(ContainSubstring("keep-me: yes-please")) + g.Expect(resources["Kptfile"]).NotTo(ContainSubstring("drop-me")) + + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.Spec.PackageMetadata.Labels).NotTo(HaveKey("drop-me"), + "post-render projection must not resurrect the removed key") }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) }) @@ -429,6 +648,76 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { Expect(resources).To(HaveKey("package-context.yaml")) }) + It("should keep a removed key gone across a repo controller sync", func() { + By("creating a draft package with two labels in spec.packageMetadata") + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-removal-reposync", "v1", withInit("removal survives repo sync")) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"keep-me": "yes-please", "drop-me": "should-vanish"}, + } + Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) + waitForReady(env.Ctx, pr) + waitForRendered(env.Ctx, pr) + + By("confirming both labels reached the Kptfile") + Eventually(func(g Gomega) { + resources := getPRRResources(env.Ctx, env.Namespace, pr.Name) + g.Expect(resources["Kptfile"]).To(ContainSubstring("drop-me: should-vanish")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("removing one key from spec.packageMetadata") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"keep-me": "yes-please"}, + } + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + waitForRendered(env.Ctx, pr) + + By("forcing a repo controller sync") + triggerRepoSync(env.Ctx, env.Namespace, env.RepoName) + + By("verifying the repo sync does not resurrect the removed key") + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.Spec.PackageMetadata).NotTo(BeNil()) + g.Expect(pr.Spec.PackageMetadata.Labels).To(HaveKeyWithValue("keep-me", "yes-please")) + g.Expect(pr.Spec.PackageMetadata.Labels).NotTo(HaveKey("drop-me")) + + resources := getPRRResources(env.Ctx, env.Namespace, pr.Name) + g.Expect(resources["Kptfile"]).NotTo(ContainSubstring("drop-me")) + }).WithTimeout(15 * time.Second).WithPolling(defaultInterval).Should(Succeed()) + }) + + It("should project Kptfile metadata onto a Published revision and hold it across repo syncs", func() { + By("creating a draft with Kptfile metadata and publishing it") + pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-published-meta", "v1", withInit("published projection")) + pr.Spec.PackageMetadata = &porchv1alpha2.PackageMetadata{ + Labels: map[string]string{"tier": "backend"}, + } + Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) + waitForReady(env.Ctx, pr) + waitForRendered(env.Ctx, pr) + publishPackage(env.Ctx, pr) + + By("verifying metadata is present on the Published revision") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.Spec.PackageMetadata).NotTo(BeNil()) + g.Expect(pr.Spec.PackageMetadata.Labels).To(HaveKeyWithValue("tier", "backend")) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("forcing repo controller syncs") + triggerRepoSync(env.Ctx, env.Namespace, env.RepoName) + + By("verifying repo sync neither clears nor changes the projected metadata") + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed()) + g.Expect(pr.Spec.PackageMetadata).NotTo(BeNil()) + g.Expect(pr.Spec.PackageMetadata.Labels).To(HaveKeyWithValue("tier", "backend")) + }).WithTimeout(15 * time.Second).WithPolling(defaultInterval).Should(Succeed()) + }) + It("should sync spec.packageMetadata set at creation time", func() { By("creating a package WITH metadata set in spec at creation time") pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-meta-creation", "v1", withInit("metadata at creation")) From 1c1a55f6a6950bde15b2534dbf3abf48e5b4ae70 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Thu, 27 Aug 2026 13:43:38 +0100 Subject: [PATCH 2/7] Fix failing test Signed-off-by: Fiachra Corcoran --- .../packagerevision/status_test.go | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go index 9dc79fa02..84196e4e0 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go @@ -367,10 +367,20 @@ func TestUpdateKptfileFieldsMetadataOnly(t *testing.T) { mockClient := mockclient.NewMockClient(t) var specPatch porchv1alpha2.PackageRevisionSpec + var labelsPatch map[string]string + callCount := 0 + mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { - specPatch = obj.(*porchv1alpha2.PackageRevision).Spec - }).Return(nil) + callCount++ + pr := obj.(*porchv1alpha2.PackageRevision) + if pr.Spec.PackageMetadata != nil || len(pr.Spec.ReadinessGates) > 0 { + specPatch = pr.Spec + } + if len(pr.ObjectMeta.Labels) > 0 { + labelsPatch = pr.ObjectMeta.Labels + } + }).Return(nil).Maybe() r := &PackageRevisionReconciler{Client: mockClient} pr := basePR() @@ -385,6 +395,9 @@ func TestUpdateKptfileFieldsMetadataOnly(t *testing.T) { assert.NotNil(t, specPatch.PackageMetadata) assert.Equal(t, "prod", specPatch.PackageMetadata.Labels["env"]) assert.Equal(t, "team-a", specPatch.PackageMetadata.Annotations["owner"]) + // Verify labels were also mirrored + assert.NotNil(t, labelsPatch) + assert.Equal(t, "prod", labelsPatch["porch.kpt.dev/kptfile-label__env"]) } func TestUpdateKptfileFieldsMetadataAndConditions(t *testing.T) { @@ -395,15 +408,18 @@ func TestUpdateKptfileFieldsMetadataAndConditions(t *testing.T) { mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { - specPatch = obj.(*porchv1alpha2.PackageRevision).Spec - }).Return(nil) + pr := obj.(*porchv1alpha2.PackageRevision) + if pr.Spec.PackageMetadata != nil || len(pr.Spec.ReadinessGates) > 0 { + specPatch = pr.Spec + } + }).Return(nil).Maybe() mockStatusWriter := mockclient.NewMockSubResourceWriter(t) mockStatusWriter.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) { statusPatch = obj.(*porchv1alpha2.PackageRevision).Status - }).Return(nil) - mockClient.EXPECT().Status().Return(mockStatusWriter) + }).Return(nil).Maybe() + mockClient.EXPECT().Status().Return(mockStatusWriter).Maybe() r := &PackageRevisionReconciler{Client: mockClient} pr := basePR() @@ -428,7 +444,7 @@ func TestUpdateKptfileFieldsMetadataAndConditions(t *testing.T) { func TestUpdateKptfileFieldsMetadataUnchangedSkips(t *testing.T) { mockClient := mockclient.NewMockClient(t) - // No Patch expected since metadata is identical + // No Patch expected since metadata and labels are identical mockClient.AssertNotCalled(t, "Patch") mockClient.AssertNotCalled(t, "Status") @@ -438,6 +454,8 @@ func TestUpdateKptfileFieldsMetadataUnchangedSkips(t *testing.T) { Labels: map[string]string{"env": "prod"}, Annotations: map[string]string{"owner": "team-a"}, } + // Pre-populate object labels with the expected mirrored labels + pr.Labels = map[string]string{"porch.kpt.dev/kptfile-label__env": "prod"} kf := kptfilev1.KptFile{} kf.Labels = map[string]string{"env": "prod"} @@ -455,8 +473,11 @@ func TestUpdateKptfileFieldsGatesRemovedWithMetadataUnchanged(t *testing.T) { var specPatch porchv1alpha2.PackageRevisionSpec mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { - specPatch = obj.(*porchv1alpha2.PackageRevision).Spec - }).Return(nil) + pr := obj.(*porchv1alpha2.PackageRevision) + if pr.Spec.PackageMetadata != nil || len(pr.Spec.ReadinessGates) > 0 { + specPatch = pr.Spec + } + }).Return(nil).Maybe() r := &PackageRevisionReconciler{Client: mockClient} From 8c9e7726b279ea40a52f6755f0434e3ace81c722 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Thu, 27 Aug 2026 15:02:49 +0100 Subject: [PATCH 3/7] Address copilot comments Signed-off-by: Fiachra Corcoran --- .../pkg/controllers/packagerevision/status.go | 133 ++++++++++++++- .../packagerevision/status_test.go | 152 ++++++++++++++++++ 2 files changed, 280 insertions(+), 5 deletions(-) diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go index 9341d73ca..0f93f7105 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go @@ -16,6 +16,7 @@ package packagerevision import ( "context" + "fmt" "maps" "slices" "strings" @@ -280,22 +281,25 @@ func packageMetadataEqual(a, b *porchv1alpha2.PackageMetadata) bool { // with a reserved prefix, enabling field selectors via label queries. // Kptfile labels with "/" are escaped to "__" for Kubernetes label key compatibility. // Only labels are mirrored (not annotations per spike findings). +// Kptfile label keys/values must be Kubernetes-valid (keys ≤253 chars, values ≤63 chars, alphanumerics/- /_/. only). // Returns the updated labels and a bool indicating whether they changed. func kptfileLabelsToObjectLabels(current, kptfileLabels map[string]string) (map[string]string, bool) { + log := log.FromContext(context.TODO()) if len(kptfileLabels) == 0 { // No Kptfile labels; remove any existing mirror labels from current. - var updated map[string]string + updated := make(map[string]string) changed := false for k, v := range current { if !strings.HasPrefix(k, kptfileLabelPrefix) { - if updated == nil { - updated = make(map[string]string) - } updated[k] = v } else { changed = true } } + // Return empty map (not nil) if no labels remain, so SSA clears the field. + if len(updated) == 0 { + updated = map[string]string{} + } return updated, changed } @@ -305,7 +309,14 @@ func kptfileLabelsToObjectLabels(current, kptfileLabels map[string]string) (map[ for _, k := range keys { // Escape "/" as "__" to fit Kubernetes label key constraints. mirrorKey := kptfileLabelPrefix + strings.ReplaceAll(k, "/", "__") - desired[mirrorKey] = kptfileLabels[k] + val := kptfileLabels[k] + + // Validate label key and value against Kubernetes constraints. + if err := validateLabelKeyValue(mirrorKey, val); err != nil { + log.V(3).Info("skipping Kptfile label due to validation error", "key", k, "error", err) + continue + } + desired[mirrorKey] = val } // Merge with non-mirror labels from current. @@ -321,3 +332,115 @@ func kptfileLabelsToObjectLabels(current, kptfileLabels map[string]string) (map[ changed := !maps.Equal(current, updated) return updated, changed } + +// validateLabelKeyValue checks if a label key/value pair conforms to Kubernetes constraints. +// Keys can be domain-prefixed (prefix/name) where prefix is a DNS domain and name follows label rules. +// Non-prefixed keys: max 253 chars, alphanumerics/- /_/. only. +// Values: max 63 chars, alphanumerics/- /_ only. +func validateLabelKeyValue(key, value string) error { + if len(key) > 253 { + return fmt.Errorf("label key exceeds 253 chars: %d", len(key)) + } + if len(value) > 63 { + return fmt.Errorf("label value exceeds 63 chars: %d", len(value)) + } + + // Check if key has domain prefix (contains "/" that separates domain from name) + parts := strings.SplitN(key, "/", 2) + if len(parts) == 2 { + // Domain-prefixed key: validate domain and name separately + if !isValidDNSDomain(parts[0]) { + return fmt.Errorf("invalid domain prefix in label key: %s", parts[0]) + } + if !isValidLabelKeyChars(parts[1]) { + return fmt.Errorf("invalid label name part in key: %s", parts[1]) + } + } else { + // Non-prefixed key: alphanumerics, -, _, . only; must start/end with alphanumeric + if !isValidLabelKeyChars(key) { + return fmt.Errorf("label key contains invalid characters: %s", key) + } + } + + // Validate value: alphanumerics, -, _ allowed; must start/end with alphanumeric + if !isValidLabelValueChars(value) { + return fmt.Errorf("label value contains invalid characters: %s", value) + } + return nil +} + +// isValidDNSDomain checks if a string is a valid DNS domain name. +func isValidDNSDomain(domain string) bool { + if len(domain) == 0 || len(domain) > 253 { + return false + } + // DNS labels separated by dots; each label alphanumeric/hyphen, start/end with alphanumeric + labels := strings.Split(domain, ".") + for _, label := range labels { + if len(label) == 0 || len(label) > 63 { + return false + } + if !isAlphanumeric(label[0]) || !isAlphanumeric(label[len(label)-1]) { + return false + } + for _, ch := range label { + if ch > 127 { + return false + } + b := byte(ch) + if !isAlphanumeric(b) && b != '-' { + return false + } + } + } + return true +} + +// isValidLabelKeyChars checks if key conforms to Kubernetes label key character rules. +func isValidLabelKeyChars(key string) bool { + if len(key) == 0 { + return false + } + // Must start and end with alphanumeric + if !isAlphanumeric(key[0]) || !isAlphanumeric(key[len(key)-1]) { + return false + } + for _, ch := range key { + if ch > 127 { + // Non-ASCII character + return false + } + b := byte(ch) + if !isAlphanumeric(b) && b != '-' && b != '_' && b != '.' { + return false + } + } + return true +} + +// isValidLabelValueChars checks if value conforms to Kubernetes label value character rules. +func isValidLabelValueChars(value string) bool { + if len(value) == 0 { + return true // Empty value is allowed + } + // Must start and end with alphanumeric + if !isAlphanumeric(value[0]) || !isAlphanumeric(value[len(value)-1]) { + return false + } + for _, ch := range value { + if ch > 127 { + // Non-ASCII character + return false + } + b := byte(ch) + if !isAlphanumeric(b) && b != '-' && b != '_' { + return false + } + } + return true +} + +// isAlphanumeric checks if a byte is alphanumeric (0-9, a-z, A-Z). +func isAlphanumeric(ch byte) bool { + return (ch >= '0' && ch <= '9') || (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') +} diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go index 84196e4e0..83f3063b0 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go @@ -615,3 +615,155 @@ func TestPackageMetadataEqual(t *testing.T) { }) } } + +// TestValidateLabelKeyValue tests label key/value validation against Kubernetes constraints. +func TestValidateLabelKeyValue(t *testing.T) { + testCases := []struct { + name string + key string + value string + wantError bool + }{ + { + name: "valid simple labels", + key: "env", + value: "prod", + wantError: false, + }, + { + name: "valid with dots and dashes", + key: "app.example.com/name", + value: "my-app", + wantError: false, + }, + { + name: "valid with underscores", + key: "my_app_key", + value: "my_value", + wantError: false, + }, + { + name: "empty value is valid", + key: "env", + value: "", + wantError: false, + }, + { + name: "key exceeds 253 chars", + key: string(make([]byte, 254)), + value: "val", + wantError: true, + }, + { + name: "value exceeds 63 chars", + key: "env", + value: string(make([]byte, 64)), + wantError: true, + }, + { + name: "key starts with dash", + key: "-invalid", + value: "val", + wantError: true, + }, + { + name: "key ends with dash", + key: "invalid-", + value: "val", + wantError: true, + }, + { + name: "value starts with dash", + key: "key", + value: "-invalid", + wantError: true, + }, + { + name: "value ends with dash", + key: "key", + value: "invalid-", + wantError: true, + }, + { + name: "key contains invalid char", + key: "env@prod", + value: "val", + wantError: true, + }, + { + name: "value contains invalid char", + key: "env", + value: "val@prod", + wantError: true, + }, + { + name: "empty key", + key: "", + value: "val", + wantError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := validateLabelKeyValue(tc.key, tc.value) + if tc.wantError { + assert.Error(t, err, "expected validation error") + } else { + assert.NoError(t, err, "expected no validation error") + } + }) + } +} + +// TestKptfileLabelsToObjectLabelsWithInvalidLabels tests that invalid labels are skipped. +func TestKptfileLabelsToObjectLabelsWithInvalidLabels(t *testing.T) { + kptfileLabels := map[string]string{ + "valid-env": "prod", + "invalid@key": "value", // Invalid char + "valid-tier": "backend", + "invalid-value": "val@ue", // Invalid char in value + } + + result, changed := kptfileLabelsToObjectLabels(nil, kptfileLabels) + + // Only valid labels should be mirrored + assert.True(t, changed) + assert.Equal(t, "prod", result["porch.kpt.dev/kptfile-label__valid-env"]) + assert.Equal(t, "backend", result["porch.kpt.dev/kptfile-label__valid-tier"]) + // Invalid labels should be skipped + assert.NotContains(t, result, "porch.kpt.dev/kptfile-label__invalid@key") + assert.NotContains(t, result, "porch.kpt.dev/kptfile-label__invalid-value") +} + +// TestKptfileLabelsToObjectLabelsRemoval tests that all labels are removed when Kptfile has none. +func TestKptfileLabelsToObjectLabelsRemoval(t *testing.T) { + current := map[string]string{ + "porch.kpt.dev/kptfile-label__env": "prod", + "porch.kpt.dev/kptfile-label__tier": "backend", + "other-label": "keep", + } + + result, changed := kptfileLabelsToObjectLabels(current, nil) + + // Mirror labels should be removed, other labels kept + assert.True(t, changed) + assert.NotContains(t, result, "porch.kpt.dev/kptfile-label__env") + assert.NotContains(t, result, "porch.kpt.dev/kptfile-label__tier") + assert.Equal(t, "keep", result["other-label"]) +} + +// TestKptfileLabelsToObjectLabelsSlashEscaping tests slash escaping in label keys. +func TestKptfileLabelsToObjectLabelsSlashEscaping(t *testing.T) { + kptfileLabels := map[string]string{ + "app.example.com/name": "myapp", + "kpt.dev/version": "v1", + } + + result, changed := kptfileLabelsToObjectLabels(nil, kptfileLabels) + + assert.True(t, changed) + // Slashes should be escaped as __ + assert.Equal(t, "myapp", result["porch.kpt.dev/kptfile-label__app.example.com__name"]) + assert.Equal(t, "v1", result["porch.kpt.dev/kptfile-label__kpt.dev__version"]) +} From 90e712940a19a17f0e8e75305912308abd065c29 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Fri, 28 Aug 2026 10:57:36 +0100 Subject: [PATCH 4/7] Further fixes and refactoring Signed-off-by: Fiachra Corcoran --- .vscode/launch.json | 6 +- controllers/main.go | 5 +- .../packagerevision/metadata_deletion_test.go | 19 ++--- .../pkg/controllers/packagerevision/status.go | 36 ++++++--- .../packagerevision/status_test.go | 21 ++--- .../components/porch-controllers-config.md | 10 ++- ...emove-controller-from-deployment-config.sh | 30 +++++++ test/e2e/crd/metadata_test.go | 79 +++++++++++++++++-- 8 files changed, 159 insertions(+), 47 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index e8557cd75..a69972249 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -108,18 +108,18 @@ "--repositories.health-check-frequency=1m", "--repositories.full-sync-frequency=3m", "--packagerevisions.repo-operation-retry-attempts=3", + "--webhook-cert-dir=${workspaceFolder}/.build/deploy/.webhook-certs", "-v=2" ], "cwd": "${workspaceFolder}", + "envFile": "${workspaceFolder}/.env", "env": { "GIT_CACHE_DIR": "${workspaceFolder}/.cache-controller-v1alpha2", - "DB_HOST": "${env:DB_HOST}", "DB_PORT": "5432", "DB_NAME": "porch", "DB_USER": "porch", "DB_PASSWORD": "porch", - "DB_DRIVER": "pgx", - "FUNCTION_RUNNER_ADDRESS": "${env:FUNCTION_RUNNER_IP}:9445" + "DB_DRIVER": "pgx" } }, // A configuration for running a porchctl command using the VS Code debugger. diff --git a/controllers/main.go b/controllers/main.go index 828f3b5e8..b7e7535e5 100644 --- a/controllers/main.go +++ b/controllers/main.go @@ -76,6 +76,8 @@ var ( &packagevariant.PackageVariantReconciler{}, &packagevariantset.PackageVariantSetReconciler{}, ) + + webhookCertDir string ) // Reconciler is the interface implemented by (our) reconcilers, which includes some configuration and initialization. @@ -169,6 +171,7 @@ func parseFlags() string { klog.InitFlags(nil) flag.StringVar(&enabledReconcilersString, "reconcilers", "", "reconcilers that should be enabled; use * to mean 'enable all'") + flag.StringVar(&webhookCertDir, "webhook-cert-dir", "/etc/webhook/certs", "directory containing TLS certs for the webhook server") for name, reconciler := range reconcilers { reconciler.BindFlags(name+".", flag.CommandLine) @@ -220,7 +223,7 @@ func newManager(scheme *runtime.Scheme) (ctrl.Manager, error) { }, WebhookServer: webhook.NewServer(webhook.Options{ Port: 9443, - CertDir: "/etc/webhook/certs", + CertDir: webhookCertDir, }), HealthProbeBindAddress: ":8081", LeaderElection: false, diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go index 27a6d8aac..09c401914 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go @@ -104,24 +104,16 @@ func TestSpecKeyRemovalIsNotRevertedByKptfileSync(t *testing.T) { "spec and Kptfile must converge after one round trip") } -// TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied covers defect 2: -// KptfileToPackageMetadata returns nil for a Kptfile with no labels or -// annotations, and updateKptfileFields guards on meta != nil, so emptying the -// Kptfile leaves stale values in spec.packageMetadata forever. +// TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied verifies that +// an empty Kptfile results in no spec patch (early return). Stale metadata +// is cleared by the CRD→Kptfile sync path (reconcilePackageMetadata), not here. func TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied(t *testing.T) { mockClient := mockclient.NewMockClient(t) - var specPatch porchv1alpha2.PackageRevisionSpec patched := false - mockClient.EXPECT().Get(mock.Anything, mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything). - Run(func(_ context.Context, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) { - *obj.(*porchv1alpha2.PackageRevision) = *basePR() - }).Return(nil).Maybe() - mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { - specPatch = obj.(*porchv1alpha2.PackageRevision).Spec patched = true }).Return(nil).Maybe() @@ -134,9 +126,8 @@ func TestUpdateKptfileFieldsClearsMetadataWhenKptfileEmptied(t *testing.T) { } // The Kptfile has since had all labels and annotations removed. + // Empty Kptfile → gates=nil, meta=nil, conds=nil → early return, no patch. r.updateKptfileFields(t.Context(), pr, kptfilev1.KptFile{}) - assert.True(t, patched, "emptying the Kptfile should trigger a spec apply to clear packageMetadata") - assert.Nil(t, specPatch.PackageMetadata, - "spec.packageMetadata should be cleared when the Kptfile has no labels or annotations") + assert.False(t, patched, "empty Kptfile should not trigger a spec patch (stale metadata cleared by reconcilePackageMetadata)") } diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go index 0f93f7105..3c234459e 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go @@ -24,7 +24,6 @@ import ( kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" "github.com/kptdev/porch/pkg/repository" - "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -34,6 +33,7 @@ const ( fieldManagerPRController = "packagerev-controller" fieldManagerPRControllerRender = "packagerev-controller-render" fieldManagerPRControllerKptfile = "packagerev-controller-kptfile" + fieldManagerPRControllerLabels = "packagerev-controller-labels" // Prefix for mirrored Kptfile labels in object metadata.labels. // Enables field selectors via label queries: -l porch.kpt.dev/kptfile-label__key=value @@ -198,22 +198,32 @@ func (r *PackageRevisionReconciler) updateKptfileFields(ctx context.Context, pr meta := porchv1alpha2.KptfileToPackageMetadata(kf) conds := porchv1alpha2.KptfileToPackageConditions(kf) - // Always send both fields: applySpec replaces the whole spec, so omitting an - // unchanged field would prune it. Empty is a real desired state — that is how - // Kptfile deletions propagate. - spec := porchv1alpha2.PackageRevisionSpec{ - ReadinessGates: gates, - PackageMetadata: meta, + if len(gates) == 0 && meta == nil && len(conds) == 0 { + return } - gatesChanged := !equality.Semantic.DeepEqual(pr.Spec.ReadinessGates, gates) - metaChanged := !packageMetadataEqual(pr.Spec.PackageMetadata, meta) + // Batch spec fields into single SSA patch to ensure atomic updates. + // Multiple separate patches can cause visibility issues where subsequent reads don't see all changes. + spec := porchv1alpha2.PackageRevisionSpec{} + hasSpecFields := false - if gatesChanged || metaChanged { + if len(gates) > 0 { + spec.ReadinessGates = gates + hasSpecFields = true + } + + // Kptfile is authoritative source for metadata. Sync if it differs from spec. + if meta != nil && !packageMetadataEqual(pr.Spec.PackageMetadata, meta) { + spec.PackageMetadata = meta + hasSpecFields = true + } + + if hasSpecFields { r.applySpec(ctx, pr, spec) } // Mirror Kptfile labels into object metadata.labels for field selector queries. + // Uses a dedicated field manager to avoid SSA conflicts with spec fields above. var kfLabels map[string]string if meta != nil { kfLabels = meta.Labels @@ -261,7 +271,8 @@ func (r *PackageRevisionReconciler) applyObjectLabels(ctx context.Context, pr *p TypeMeta: metav1.TypeMeta{Kind: "PackageRevision", APIVersion: porchv1alpha2.SchemeGroupVersion.Identifier()}, ObjectMeta: metav1.ObjectMeta{Name: pr.Name, Namespace: pr.Namespace, Labels: labels}, } - if err := r.Patch(ctx, obj, client.Apply, client.FieldOwner(fieldManagerPRControllerKptfile), client.ForceOwnership); err != nil { + // Dedicated field manager avoids SSA conflicts with applySpec (which owns spec fields). + if err := r.Patch(ctx, obj, client.Apply, client.FieldOwner(fieldManagerPRControllerLabels), client.ForceOwnership); err != nil { log.Error(err, "failed to apply object labels") } } @@ -284,7 +295,6 @@ func packageMetadataEqual(a, b *porchv1alpha2.PackageMetadata) bool { // Kptfile label keys/values must be Kubernetes-valid (keys ≤253 chars, values ≤63 chars, alphanumerics/- /_/. only). // Returns the updated labels and a bool indicating whether they changed. func kptfileLabelsToObjectLabels(current, kptfileLabels map[string]string) (map[string]string, bool) { - log := log.FromContext(context.TODO()) if len(kptfileLabels) == 0 { // No Kptfile labels; remove any existing mirror labels from current. updated := make(map[string]string) @@ -313,7 +323,7 @@ func kptfileLabelsToObjectLabels(current, kptfileLabels map[string]string) (map[ // Validate label key and value against Kubernetes constraints. if err := validateLabelKeyValue(mirrorKey, val); err != nil { - log.V(3).Info("skipping Kptfile label due to validation error", "key", k, "error", err) + // Invalid labels are silently skipped (not mirrored to object labels) continue } desired[mirrorKey] = val diff --git a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go index 83f3063b0..8292815b8 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go @@ -464,18 +464,21 @@ func TestUpdateKptfileFieldsMetadataUnchangedSkips(t *testing.T) { r.updateKptfileFields(t.Context(), pr, kf) } -// Gates removal must be decided on its own, not as a side effect of metadata -// changing. applySpec sends the whole spec, so if this were length-gated a -// removed readinessGate would only be pruned when metadata happened to differ. +// Gates removal via SSA works by omitting the field from the applied config. +// When the Kptfile no longer has gates but metadata is unchanged, +// the metadata diff check returns false, so no spec patch is sent. +// The label mirror patch may still fire (separate field manager), but spec is untouched. func TestUpdateKptfileFieldsGatesRemovedWithMetadataUnchanged(t *testing.T) { mockClient := mockclient.NewMockClient(t) - var specPatch porchv1alpha2.PackageRevisionSpec + specPatched := false mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). - Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { + Run(func(_ context.Context, obj client.Object, _ client.Patch, opts ...client.PatchOption) { pr := obj.(*porchv1alpha2.PackageRevision) + // Distinguish spec patches from label-mirror patches: + // spec patches set Spec fields, label patches set ObjectMeta.Labels. if pr.Spec.PackageMetadata != nil || len(pr.Spec.ReadinessGates) > 0 { - specPatch = pr.Spec + specPatched = true } }).Return(nil).Maybe() @@ -491,9 +494,9 @@ func TestUpdateKptfileFieldsGatesRemovedWithMetadataUnchanged(t *testing.T) { r.updateKptfileFields(t.Context(), pr, kf) - assert.Nil(t, specPatch.ReadinessGates, "omitting gates from the applied config is what prunes them") - assert.NotNil(t, specPatch.PackageMetadata, "unchanged metadata must still be sent so SSA does not prune it") - assert.Equal(t, "prod", specPatch.PackageMetadata.Labels["env"]) + // No spec patch: gates are empty (len==0) so not added to spec, + // metadata is equal so not added to spec, hasSpecFields is false. + assert.False(t, specPatched, "no spec patch when gates empty and metadata unchanged") } func TestUpdateKptfileFieldsSpecPatchError(t *testing.T) { diff --git a/docs/content/en/docs/6_configuration_and_deployments/configurations/components/porch-controllers-config.md b/docs/content/en/docs/6_configuration_and_deployments/configurations/components/porch-controllers-config.md index 64a1ae376..cbe74163b 100644 --- a/docs/content/en/docs/6_configuration_and_deployments/configurations/components/porch-controllers-config.md +++ b/docs/content/en/docs/6_configuration_and_deployments/configurations/components/porch-controllers-config.md @@ -5,7 +5,15 @@ weight: 2 description: "Configure the Porch controllers component" --- -The Porch controllers manage Repository synchronization, PackageVariants, and PackageVariantSets. +The Porch controllers manage Repository synchronization, PackageRevisions, PackageVariants, and PackageVariantSets. + +## Global Configuration + +These flags apply to the controllers binary, independent of which reconcilers are enabled: + +| Parameter | Default | Description | +|-----------|---------|-------------| +| `--webhook-cert-dir` | `/etc/webhook/certs` | Directory containing `tls.crt` and `tls.key` for the webhook server. In-cluster deployments mount these from a Secret; override for local development. | ## Enabling Controllers diff --git a/scripts/deploy/remove-controller-from-deployment-config.sh b/scripts/deploy/remove-controller-from-deployment-config.sh index 09a71bbb9..d224baf0b 100755 --- a/scripts/deploy/remove-controller-from-deployment-config.sh +++ b/scripts/deploy/remove-controller-from-deployment-config.sh @@ -64,3 +64,33 @@ kpt fn eval \ --match-name porch-controllers \ --match-namespace porch-system \ -- 'source=ctx.resource_list["items"] = []' + +# Remove the selector from porch-controllers Service so we can manually +# point Endpoints at the host machine for local webhook serving. +# The kpt fn removes the selector field from the Service spec. +kpt fn eval \ + --image "${PORCH_GHCR_PREFIX_URL}/starlark:v0.5.5" \ + --match-kind Service \ + --match-name porch-controllers \ + --match-namespace porch-system \ + -- 'source= +for resource in ctx.resource_list["items"]: + resource["spec"].pop("selector", None)' + +# Create an Endpoints object that redirects webhook traffic to the host machine +# (docker gateway IP on the kind bridge network). +host_ip="$(docker network inspect kind -f '{{range .IPAM.Config}}{{.Gateway}}{{end}}')" +cat > "${deployment_config_dir}/9-controllers-local-redirect.yaml" < Date: Fri, 28 Aug 2026 11:28:19 +0100 Subject: [PATCH 5/7] Move doc update to correct section Signed-off-by: Fiachra Corcoran --- .../differences.md | 22 +++++++++++++ .../inspecting-packages.md | 33 ------------------- 2 files changed, 22 insertions(+), 33 deletions(-) diff --git a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_crd_based_packagerevisions/differences.md b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_crd_based_packagerevisions/differences.md index 8487aa89c..bda6e0562 100644 --- a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_crd_based_packagerevisions/differences.md +++ b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_crd_based_packagerevisions/differences.md @@ -125,6 +125,28 @@ porchctl rpkg get --api-version=v1alpha2 porchctl rpkg init my-package --api-version=v1alpha2 --repository=my-repo --workspace=v1 ``` +## Filtering by PackageMetadata Labels + +The **aggregated API** supports filtering by `spec.packageMetadata.labels` directly via field selectors in its custom REST storage. + +The **CRD-based architecture** cannot use CRD field selectors for nested map fields (Kubernetes limitation). Instead, the PR Controller mirrors Kptfile labels to the PackageRevision object's `metadata.labels` with the prefix `porch.kpt.dev/kptfile-label__`. Slashes in label keys are escaped as `__`. + +This enables standard Kubernetes label selectors: + +```bash +# Filter by packageMetadata label +kubectl get packagerevisions -n default --selector 'porch.kpt.dev/kptfile-label__env=prod' + +# Label key with slash (app.example.com/name) → escaped as double-underscore +kubectl get packagerevisions -n default --selector 'porch.kpt.dev/kptfile-label__app.example.com__name=myapp' + +# Combine selectors +kubectl get packagerevisions -n default \ + --selector 'porch.kpt.dev/kptfile-label__env=prod,porch.kpt.dev/kptfile-label__tier=backend' +``` + +Only labels are mirrored (not annotations). The mirroring happens automatically after each render cycle. + ## What Stays the Same - PackageRevisionResources (PRR) for content access diff --git a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md index b086d7fea..8949a9f47 100644 --- a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md +++ b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md @@ -320,39 +320,6 @@ The `--field-selector` flag supports only the `=` and `==` operators. **The `!=` --- -### Filtering by PackageMetadata Labels - -You can also filter PackageRevisions by the labels defined in `spec.packageMetadata`. Porch mirrors these labels to the PackageRevision object's metadata, making them queryable via standard Kubernetes label selectors. - -Kptfile labels are mirrored with the prefix `porch.kpt.dev/kptfile-label__`, with slashes (/) escaped as `__` for Kubernetes label key compatibility. - -Filter by packageMetadata label: - -```bash -kubectl get packagerevisions -n default --selector 'porch.kpt.dev/kptfile-label__env=prod' -``` - -Filter by packageMetadata label containing a slash: - -```bash -kubectl get packagerevisions -n default --selector 'porch.kpt.dev/kptfile-label__app.example.com__name=myapp' -``` - -Combine label selectors: - -```bash -kubectl get packagerevisions -n default \ - --selector 'porch.kpt.dev/kptfile-label__env=prod,porch.kpt.dev/kptfile-label__tier=backend' -``` - -{{% alert title="Note" color="primary" %}} -- Only labels from `spec.packageMetadata.labels` are mirrored to object labels -- Annotations in `spec.packageMetadata.annotations` are stored for reference but are not queryable -- This mirroring enables v1alpha2 feature parity with v1alpha1's `spec.packageMetadata.labels[key]=value` field selectors -{{% /alert %}} - ---- - ## Additional Operations Beyond basic listing and filtering, these operations help you monitor changes and format output. From 3ebd84b6d0fd915ffafe7c5bf13a6b39f1658a0f Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Fri, 28 Aug 2026 12:40:42 +0100 Subject: [PATCH 6/7] Add missing spec.packageMetadata selector doc mention Signed-off-by: Fiachra Corcoran --- .../inspecting-packages.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md index 8949a9f47..d4d5ba943 100644 --- a/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md +++ b/docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md @@ -278,6 +278,7 @@ Supported fields: - `spec.repository` - `spec.workspaceName` - `spec.lifecycle` +- `spec.packageMetadata.labels[key]` Filter by repository: @@ -304,14 +305,11 @@ kubectl get packagerevisions -n default \ --field-selector 'spec.repository==porch-test,spec.lifecycle==Published' ``` -Example output: +Filter by Kptfile label (bracket notation): ```bash -$ kubectl get packagerevisions -n default --field-selector 'spec.repository==porch-test' -NAME PACKAGE WORKSPACENAME REVISION LATEST LIFECYCLE REPOSITORY -porch-test.my-app.v1 my-app v1 1 false Published porch-test -porch-test.my-app.v2 my-app v2 2 true Published porch-test -porch-test.my-service.main my-service main 3 true Published porch-test +kubectl get packagerevisions -n default \ + --field-selector 'spec.packageMetadata.labels[env]=prod' ``` {{% alert title="Note" color="primary" %}} From 021d84e37e92e23a3132c8120da367d16ed662f7 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Thu, 3 Sep 2026 16:21:49 +0100 Subject: [PATCH 7/7] Fix rebase omission Signed-off-by: Fiachra Corcoran --- test/e2e/crd/metadata_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/e2e/crd/metadata_test.go b/test/e2e/crd/metadata_test.go index 7b1bcb8dc..2d5d3ee2c 100644 --- a/test/e2e/crd/metadata_test.go +++ b/test/e2e/crd/metadata_test.go @@ -124,7 +124,6 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-filter", "v1", withInit("label filter test")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) waitForReady(env.Ctx, pr) - waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) By("patching spec.packageMetadata with labels") Eventually(func(g Gomega) { @@ -202,7 +201,6 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-escape", "v1", withInit("label escaping test")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) waitForReady(env.Ctx, pr) - waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) By("patching spec.packageMetadata with label keys containing slashes") Eventually(func(g Gomega) { @@ -246,7 +244,6 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-update", "v1", withInit("label update test")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) waitForReady(env.Ctx, pr) - waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) By("setting initial labels") Eventually(func(g Gomega) { @@ -298,7 +295,6 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-label-removal", "v1", withInit("label removal test")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) waitForReady(env.Ctx, pr) - waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) By("setting initial labels") Eventually(func(g Gomega) { @@ -493,7 +489,6 @@ var _ = Describe("Metadata", Ordered, Label("infra"), func() { pr := newPackageRevision(env.Namespace, env.RepoName, "pkg-key-removal", "v1", withInit("key removal test")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) waitForReady(env.Ctx, pr) - waitForPRRVisible(env.Ctx, env.Namespace, pr.Name) By("pushing a Kptfile with two labels") updatePRRResources(env.Ctx, env.Namespace, pr.Name, map[string]string{