From 4df8d79291abc706e72177f38edd8844c616f342 Mon Sep 17 00:00:00 2001 From: Fiachra Corcoran Date: Wed, 24 Jun 2026 14:20:53 +0100 Subject: [PATCH] Preserve labels and annos from v1alpha1 to CRD during migration Signed-off-by: Fiachra Corcoran --- api/go.mod | 1 + api/go.sum | 2 + .../pkg/controllers/repository/pkgrevsync.go | 56 ++++++- .../controllers/repository/pkgrevsync_test.go | 156 ++++++++++++++++- test/e2e/crd/migration_metadata_test.go | 157 ++++++++++++++++++ 5 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 test/e2e/crd/migration_metadata_test.go diff --git a/api/go.mod b/api/go.mod index a48c85fcd..782f99fce 100644 --- a/api/go.mod +++ b/api/go.mod @@ -4,6 +4,7 @@ go 1.26.0 require ( github.com/kptdev/kpt/api v0.0.1 + github.com/pkg/errors v0.9.1 github.com/stretchr/testify v1.11.1 k8s.io/api v0.36.1 k8s.io/apiextensions-apiserver v0.36.1 diff --git a/api/go.sum b/api/go.sum index 3af2239ca..34eafb528 100644 --- a/api/go.sum +++ b/api/go.sum @@ -57,6 +57,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U= github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= +github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= +github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= diff --git a/controllers/repositories/pkg/controllers/repository/pkgrevsync.go b/controllers/repositories/pkg/controllers/repository/pkgrevsync.go index a3fc41a29..1899d896c 100644 --- a/controllers/repositories/pkg/controllers/repository/pkgrevsync.go +++ b/controllers/repositories/pkg/controllers/repository/pkgrevsync.go @@ -149,19 +149,30 @@ func (r *RepositoryReconciler) applyPackageRevision(ctx context.Context, pr *por } // applySeedFields applies non-repo-controller-owned fields (lifecycle, revision, -// Kptfile-derived, publish metadata) on initial creation only. Uses a -// separate field manager without ForceOwnership so these fields seed the value -// for discovered packages but never overwrite values already set by the user -// or the PR controller. +// Kptfile-derived, publish metadata, source labels/annotations) on initial +// creation only. Uses a separate field manager without ForceOwnership so these +// fields seed the value for discovered packages but never overwrite values +// already set by the user or the PR controller. func (r *RepositoryReconciler) applySeedFields(ctx context.Context, repo *configapi.Repository, pkgRev repository.PackageRevision, crd *porchv1alpha2.PackageRevision) { log := log.FromContext(ctx) lifecycle := porchv1alpha2.PackageRevisionLifecycle(pkgRev.Lifecycle(ctx)) kf, _ := pkgRev.GetKptfile(ctx) + // Carry over labels/annotations from the source PackageRevision's metadata. + // System-managed labels are excluded (owned by the main field manager). + seedObjMeta := metav1.ObjectMeta{Name: crd.Name, Namespace: crd.Namespace} + labels, annotations := sourceMetadata(pkgRev) + if len(labels) > 0 { + seedObjMeta.Labels = labels + } + if len(annotations) > 0 { + seedObjMeta.Annotations = annotations + } + seedSpec := &porchv1alpha2.PackageRevision{ TypeMeta: crd.TypeMeta, - ObjectMeta: metav1.ObjectMeta{Name: crd.Name, Namespace: crd.Namespace}, + ObjectMeta: seedObjMeta, Spec: porchv1alpha2.PackageRevisionSpec{ Lifecycle: lifecycle, ReadinessGates: porchv1alpha2.KptfileToReadinessGates(kf), @@ -343,3 +354,38 @@ func packageRevisionLabels(repoName string, pkgRev repository.PackageRevision) m } return labels } + +// systemLabels are labels managed by controllers that should not be carried +// over as source metadata. Includes both v1alpha1 and v1alpha2 variants. +var systemLabels = map[string]bool{ + porchv1alpha2.RepositoryLabelKey: true, + porchv1alpha2.LatestPackageRevisionKey: true, + "kpt.dev/latest-revision": true, // v1alpha1 variant +} + +// sourceMetadata extracts labels and annotations from the source package +// revision's Kubernetes ObjectMeta. System-managed labels are excluded. +func sourceMetadata(pkgRev repository.PackageRevision) (map[string]string, map[string]string) { + meta := pkgRev.GetMeta() + + var labels map[string]string + for k, v := range meta.Labels { + if systemLabels[k] { + continue + } + if labels == nil { + labels = make(map[string]string) + } + labels[k] = v + } + + var annotations map[string]string + for k, v := range meta.Annotations { + if annotations == nil { + annotations = make(map[string]string) + } + annotations[k] = v + } + + return labels, annotations +} diff --git a/controllers/repositories/pkg/controllers/repository/pkgrevsync_test.go b/controllers/repositories/pkg/controllers/repository/pkgrevsync_test.go index 63fb95c14..30b6ef7ff 100644 --- a/controllers/repositories/pkg/controllers/repository/pkgrevsync_test.go +++ b/controllers/repositories/pkg/controllers/repository/pkgrevsync_test.go @@ -93,6 +93,7 @@ type fakePackageRevision struct { commitAuthor string isLatest bool resources map[string]string + meta metav1.ObjectMeta } func (f *fakePackageRevision) KubeObjectNamespace() string { return f.key.RKey().Namespace } @@ -100,7 +101,7 @@ func (f *fakePackageRevision) KubeObjectName() string func (f *fakePackageRevision) Key() repository.PackageRevisionKey { return f.key } func (f *fakePackageRevision) UID() types.UID { return "" } func (f *fakePackageRevision) ResourceVersion() string { return "" } -func (f *fakePackageRevision) GetMeta() metav1.ObjectMeta { return metav1.ObjectMeta{} } +func (f *fakePackageRevision) GetMeta() metav1.ObjectMeta { return f.meta } func (f *fakePackageRevision) SetMeta(_ context.Context, _ metav1.ObjectMeta) error { return nil } func (f *fakePackageRevision) Lifecycle(_ context.Context) porchv1alpha1.PackageRevisionLifecycle { return f.lifecycle @@ -759,3 +760,156 @@ func TestSeedFieldsNotCalledOnUpdate(t *testing.T) { err := r.syncPackageRevisions(ctx, repo, []repository.PackageRevision{pkgRev}) assert.NoError(t, err) } + +// --- Tests: sourceMetadata --- + +func TestSourceMetadata(t *testing.T) { + t.Run("empty meta returns nil", func(t *testing.T) { + pkgRev := &fakePackageRevision{} + labels, annotations := sourceMetadata(pkgRev) + assert.Nil(t, labels) + assert.Nil(t, annotations) + }) + + t.Run("system labels are excluded", func(t *testing.T) { + pkgRev := &fakePackageRevision{ + meta: metav1.ObjectMeta{ + Labels: map[string]string{ + porchv1alpha2.RepositoryLabelKey: "my-repo", + porchv1alpha2.LatestPackageRevisionKey: "true", + "kpt.dev/latest-revision": "true", // v1alpha1 variant + "app.kubernetes.io/name": "my-app", + }, + }, + } + labels, annotations := sourceMetadata(pkgRev) + assert.Equal(t, map[string]string{"app.kubernetes.io/name": "my-app"}, labels) + assert.Nil(t, annotations) + }) + + t.Run("only system labels returns nil", func(t *testing.T) { + pkgRev := &fakePackageRevision{ + meta: metav1.ObjectMeta{ + Labels: map[string]string{ + porchv1alpha2.RepositoryLabelKey: "my-repo", + porchv1alpha2.LatestPackageRevisionKey: "true", + "kpt.dev/latest-revision": "true", + }, + }, + } + labels, annotations := sourceMetadata(pkgRev) + assert.Nil(t, labels) + assert.Nil(t, annotations) + }) + + t.Run("annotations are passed through", func(t *testing.T) { + pkgRev := &fakePackageRevision{ + meta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "team": "platform", + "purpose": "infra", + }, + }, + } + labels, annotations := sourceMetadata(pkgRev) + assert.Nil(t, labels) + assert.Equal(t, map[string]string{"team": "platform", "purpose": "infra"}, annotations) + }) + + t.Run("mixed labels and annotations", func(t *testing.T) { + pkgRev := &fakePackageRevision{ + meta: metav1.ObjectMeta{ + Labels: map[string]string{ + porchv1alpha2.RepositoryLabelKey: "repo", + "env": "production", + "app": "frontend", + }, + Annotations: map[string]string{ + "description": "production frontend", + }, + }, + } + labels, annotations := sourceMetadata(pkgRev) + assert.Equal(t, map[string]string{"env": "production", "app": "frontend"}, labels) + assert.Equal(t, map[string]string{"description": "production frontend"}, annotations) + }) +} + +// --- Tests: applySeedFields with source metadata --- + +func TestApplySeedFieldsWithSourceMetadata(t *testing.T) { + ctx := context.Background() + repo := newTestRepo() + + t.Run("source labels included in seed spec apply", func(t *testing.T) { + pkgRev := newFakePkgRev("labeled-pkg", "v1", porchv1alpha2.PackageRevisionLifecyclePublished) + pkgRev.key.Revision = 1 + pkgRev.meta = metav1.ObjectMeta{ + Labels: map[string]string{ + porchv1alpha2.RepositoryLabelKey: "my-repo", // system — should be excluded + "team": "networking", // source — should be included + "env": "staging", // source — should be included + }, + Annotations: map[string]string{ + "contact": "team-net@example.com", + }, + } + + // Build the CRD first (as applyDesiredPackageRevisions would) + crd, err := buildPackageRevision(ctx, repo, pkgRev, true) + require.NoError(t, err) + + mockClient := mockclient.NewMockClient(t) + + // Expect seed spec Patch with source labels in ObjectMeta + var seedSpecObj *porchv1alpha2.PackageRevision + mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything). + Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { + seedSpecObj = obj.(*porchv1alpha2.PackageRevision) + }).Return(nil).Once() + + // Expect seed status Patch + sw := mockclient.NewMockSubResourceWriter(t) + mockClient.EXPECT().Status().Return(sw) + sw.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything).Return(nil).Once() + + r := &RepositoryReconciler{Client: mockClient} + r.applySeedFields(ctx, repo, pkgRev, crd) + + // Verify source labels (excluding system) are in the seed apply ObjectMeta + require.NotNil(t, seedSpecObj) + assert.Equal(t, "networking", seedSpecObj.Labels["team"]) + assert.Equal(t, "staging", seedSpecObj.Labels["env"]) + assert.NotContains(t, seedSpecObj.Labels, porchv1alpha2.RepositoryLabelKey) + + // Verify annotations are included + assert.Equal(t, "team-net@example.com", seedSpecObj.Annotations["contact"]) + }) + + t.Run("no source metadata means no labels on seed ObjectMeta", func(t *testing.T) { + pkgRev := newFakePkgRev("bare-pkg", "v1", porchv1alpha2.PackageRevisionLifecycleDraft) + // meta left empty — no source labels/annotations + + crd, err := buildPackageRevision(ctx, repo, pkgRev, false) + require.NoError(t, err) + + mockClient := mockclient.NewMockClient(t) + + var seedSpecObj *porchv1alpha2.PackageRevision + mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything). + Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) { + seedSpecObj = obj.(*porchv1alpha2.PackageRevision) + }).Return(nil).Once() + + sw := mockclient.NewMockSubResourceWriter(t) + mockClient.EXPECT().Status().Return(sw) + sw.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything, mock.Anything).Return(nil).Once() + + r := &RepositoryReconciler{Client: mockClient} + r.applySeedFields(ctx, repo, pkgRev, crd) + + require.NotNil(t, seedSpecObj) + assert.Nil(t, seedSpecObj.Labels) + assert.Nil(t, seedSpecObj.Annotations) + }) +} diff --git a/test/e2e/crd/migration_metadata_test.go b/test/e2e/crd/migration_metadata_test.go new file mode 100644 index 000000000..e06e95e8b --- /dev/null +++ b/test/e2e/crd/migration_metadata_test.go @@ -0,0 +1,157 @@ +// 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. + +package crd + +import ( + porchv1alpha1 "github.com/kptdev/porch/api/porch/v1alpha1" + porchv1alpha2 "github.com/kptdev/porch/api/porch/v1alpha2" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("Migration Metadata Preservation", Ordered, Label("migration"), func() { + // Tests that labels and annotations set on v1alpha1 PackageRevision + // resources are preserved on the v1alpha2 CRDs after migration. + // + // Flow: + // 1. Register a repo as v1alpha1 (no migration annotation) + // 2. Create a package via v1alpha1 API with labels and annotations + // 3. Publish the package + // 4. Set additional labels/annotations on the published PR + // 5. Enable v1alpha2 migration annotation + // 6. Verify the v1alpha2 CRD carries those labels and annotations + + const ( + repoName = "mig-meta-repo" + pkgName = "labeled-pkg" + workspace = "v1" + ) + + var env *testEnv + + BeforeAll(func() { + env = sharedEnv() + + By("creating a fresh gitea repo for migration metadata test") + deleteGiteaRepo(repoName) + createGiteaRepo(repoName) + }) + + AfterAll(func() { + cleanupMigrationResources(env.Ctx, env.Namespace, repoName) + deleteGiteaRepo(repoName) + }) + + It("should preserve labels and annotations during migration", func() { + By("registering repo as v1alpha1 (no migration annotation)") + registerV1Alpha1Repo(env.Ctx, env.Namespace, repoName) + + By("creating and publishing a package via v1alpha1 API") + published := createAndPublishV1Alpha1Package(env.Ctx, env.Namespace, repoName, pkgName, workspace) + + By("setting custom labels and annotations on the v1alpha1 PackageRevision") + Eventually(func(g Gomega) { + pr := &porchv1alpha1.PackageRevision{} + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKey{Namespace: env.Namespace, Name: published.name}, pr)).To(Succeed()) + + if pr.Labels == nil { + pr.Labels = map[string]string{} + } + pr.Labels["team"] = "networking" + pr.Labels["env"] = "production" + + if pr.Annotations == nil { + pr.Annotations = map[string]string{} + } + pr.Annotations["contact"] = "team-net@example.com" + pr.Annotations["purpose"] = "infra-base" + + g.Expect(k8sClient.Update(env.Ctx, pr)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("verifying labels are persisted on v1alpha1") + pr := &porchv1alpha1.PackageRevision{} + Expect(k8sClient.Get(env.Ctx, client.ObjectKey{Namespace: env.Namespace, Name: published.name}, pr)).To(Succeed()) + Expect(pr.Labels).To(HaveKeyWithValue("team", "networking")) + Expect(pr.Labels).To(HaveKeyWithValue("env", "production")) + Expect(pr.Annotations).To(HaveKeyWithValue("contact", "team-net@example.com")) + Expect(pr.Annotations).To(HaveKeyWithValue("purpose", "infra-base")) + + By("enabling v1alpha2 migration and triggering sync") + Eventually(func(g Gomega) { + repo := &configapi.Repository{} + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKey{Namespace: env.Namespace, Name: repoName}, repo)).To(Succeed()) + if repo.Annotations == nil { + repo.Annotations = map[string]string{} + } + repo.Annotations["porch.kpt.dev/v1alpha2-migration"] = "true" + now := metav1.Now() + if repo.Spec.Sync == nil { + repo.Spec.Sync = &configapi.RepositorySync{} + } + repo.Spec.Sync.RunOnceAt = &now + g.Expect(k8sClient.Update(env.Ctx, repo)).To(Succeed()) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("waiting for v1alpha2 CRD to appear with seeded labels") + v2Name := crdName(repoName, pkgName, workspace) + Eventually(func(g Gomega) { + v2pr := &porchv1alpha2.PackageRevision{} + g.Expect(k8sClient.Get(env.Ctx, client.ObjectKey{Namespace: env.Namespace, Name: v2Name}, v2pr)).To(Succeed()) + // Wait for the seed apply to complete — labels appear after the main apply + g.Expect(v2pr.Labels).To(HaveKeyWithValue("team", "networking"), + "seed labels not yet applied, current labels: %v", v2pr.Labels) + }).WithTimeout(defaultTimeout).WithPolling(defaultInterval).Should(Succeed()) + + By("verifying source labels are preserved on the v1alpha2 CRD") + v2pr := &porchv1alpha2.PackageRevision{} + Expect(k8sClient.Get(env.Ctx, client.ObjectKey{Namespace: env.Namespace, Name: v2Name}, v2pr)).To(Succeed()) + + Expect(v2pr.Labels).To(HaveKeyWithValue("team", "networking")) + Expect(v2pr.Labels).To(HaveKeyWithValue("env", "production")) + + By("verifying source annotations are preserved on the v1alpha2 CRD") + Expect(v2pr.Annotations).To(HaveKeyWithValue("contact", "team-net@example.com")) + Expect(v2pr.Annotations).To(HaveKeyWithValue("purpose", "infra-base")) + + By("verifying system labels are also present (not clobbered)") + Expect(v2pr.Labels).To(HaveKeyWithValue(porchv1alpha2.RepositoryLabelKey, repoName)) + Expect(v2pr.Labels).To(HaveKey(porchv1alpha2.LatestPackageRevisionKey)) + }) + + It("should not carry system-only labels as source metadata", func() { + // The v1alpha1 PR will also have system labels (latest-revision, repository). + // These must NOT be double-applied via the seed — they are already managed + // by the main repo controller field manager. + v2Name := crdName(repoName, pkgName, workspace) + v2pr := &porchv1alpha2.PackageRevision{} + Expect(k8sClient.Get(env.Ctx, client.ObjectKey{Namespace: env.Namespace, Name: v2Name}, v2pr)).To(Succeed()) + + // The repo label should be the repo name (set by repo controller, not seed) + Expect(v2pr.Labels[porchv1alpha2.RepositoryLabelKey]).To(Equal(repoName)) + + // Count non-system labels — should be exactly our 2 custom ones + nonSystemCount := 0 + for k := range v2pr.Labels { + if k != porchv1alpha2.RepositoryLabelKey && k != porchv1alpha2.LatestPackageRevisionKey { + nonSystemCount++ + } + } + Expect(nonSystemCount).To(Equal(2), "only custom source labels should be present") + }) +})