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.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..09c401914 --- /dev/null +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go @@ -0,0 +1,133 @@ +// 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 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) + + patched := 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) { + 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. + // Empty Kptfile → gates=nil, meta=nil, conds=nil → early return, no patch. + r.updateKptfileFields(t.Context(), pr, kptfilev1.KptFile{}) + + 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/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..3c234459e 100644 --- a/controllers/packagerevisions/pkg/controllers/packagerevision/status.go +++ b/controllers/packagerevisions/pkg/controllers/packagerevision/status.go @@ -16,7 +16,10 @@ package packagerevision import ( "context" + "fmt" "maps" + "slices" + "strings" kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" @@ -30,6 +33,12 @@ 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 + // 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. @@ -213,6 +222,17 @@ func (r *PackageRevisionReconciler) updateKptfileFields(ctx context.Context, pr 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 + } + objLabels, labelsChanged := kptfileLabelsToObjectLabels(pr.Labels, kfLabels) + if labelsChanged { + r.applyObjectLabels(ctx, pr, objLabels) + } + // Apply conditions via status API (separate endpoint from spec). if len(conds) > 0 { log.V(3).Info("syncing package conditions from Kptfile", "conditionCount", len(conds)) @@ -244,6 +264,19 @@ 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}, + } + // 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") + } +} + // 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 +287,170 @@ 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). +// 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) { + if len(kptfileLabels) == 0 { + // No Kptfile labels; remove any existing mirror labels from current. + updated := make(map[string]string) + changed := false + for k, v := range current { + if !strings.HasPrefix(k, kptfileLabelPrefix) { + 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 + } + + // 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, "/", "__") + val := kptfileLabels[k] + + // Validate label key and value against Kubernetes constraints. + if err := validateLabelKeyValue(mirrorKey, val); err != nil { + // Invalid labels are silently skipped (not mirrored to object labels) + continue + } + desired[mirrorKey] = val + } + + // 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 +} + +// 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 2a905dfef..8292815b8 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"} @@ -446,6 +464,41 @@ func TestUpdateKptfileFieldsMetadataUnchangedSkips(t *testing.T) { r.updateKptfileFields(t.Context(), pr, kf) } +// 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) + + 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, 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 { + specPatched = true + } + }).Return(nil).Maybe() + + 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) + + // 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) { mockClient := mockclient.NewMockClient(t) mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything, mock.Anything). @@ -565,3 +618,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"]) +} 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 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" %}} 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" <