Skip to content

Commit 7c07de3

Browse files
efiacorefiacor
authored
fix: add Controller and BlockOwnerDeletion to ownerReferences (kptdev#1048)
* fix: add Controller and BlockOwnerDeletion to ownerReferences Both PR controller and repo controller now set Controller: true and BlockOwnerDeletion: true on PackageRevision ownerReferences. Existing CRDs with incomplete ownerRefs are self-healed on next reconcile. Fixes kptdev#921 Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Address copilot comments Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Address copilot comment Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> * Address copilot comment: validate APIVersion and UID in hasOwnerReference Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech> --------- Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
1 parent b55c934 commit 7c07de3

5 files changed

Lines changed: 254 additions & 25 deletions

File tree

‎controllers/packagerevisions/pkg/controllers/packagerevision/ownership.go‎

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,19 @@ func (r *PackageRevisionReconciler) ensureFinalizerAndOwner(ctx context.Context,
120120
func hasOwnerReference(pr *porchv1alpha2.PackageRevision, repoName string) bool {
121121
for _, ref := range pr.OwnerReferences {
122122
if ref.Kind == configapi.TypeRepository.Kind && ref.Name == repoName {
123-
return true
123+
if ref.APIVersion != configapi.GroupVersion.Identifier() {
124+
// Wrong apiVersion — needs replacement.
125+
continue
126+
}
127+
if ref.UID == "" {
128+
// Missing UID — needs replacement.
129+
continue
130+
}
131+
if ref.Controller != nil && *ref.Controller && ref.BlockOwnerDeletion != nil && *ref.BlockOwnerDeletion {
132+
return true
133+
}
134+
// ownerRef exists but missing Controller/BlockOwnerDeletion — needs update.
135+
// Continue checking in case a complete ref exists later in the slice.
124136
}
125137
}
126138
return false
@@ -131,12 +143,24 @@ func (r *PackageRevisionReconciler) setOwnerReference(ctx context.Context, pr *p
131143
if err := r.Get(ctx, types.NamespacedName{Namespace: pr.Namespace, Name: pr.Spec.RepositoryName}, &repo); err != nil {
132144
return err
133145
}
134-
pr.OwnerReferences = append(pr.OwnerReferences, metav1.OwnerReference{
135-
APIVersion: configapi.GroupVersion.Identifier(),
136-
Kind: configapi.TypeRepository.Kind,
137-
Name: repo.Name,
138-
UID: repo.UID,
139-
})
146+
controller := true
147+
blockOwnerDeletion := true
148+
desired := metav1.OwnerReference{
149+
APIVersion: configapi.GroupVersion.Identifier(),
150+
Kind: configapi.TypeRepository.Kind,
151+
Name: repo.Name,
152+
UID: repo.UID,
153+
Controller: &controller,
154+
BlockOwnerDeletion: &blockOwnerDeletion,
155+
}
156+
// Replace existing incomplete ref or append new one.
157+
for i, ref := range pr.OwnerReferences {
158+
if ref.Kind == configapi.TypeRepository.Kind && ref.Name == repo.Name {
159+
pr.OwnerReferences[i] = desired
160+
return nil
161+
}
162+
}
163+
pr.OwnerReferences = append(pr.OwnerReferences, desired)
140164
return nil
141165
}
142166

‎controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller_test.go‎

Lines changed: 204 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,19 @@ func newTestReconciler(mockClient *mockclient.MockClient, cache *mockrepository.
4444
// readyObjectMeta returns a standard ObjectMeta with finalizer and ownerReference already set,
4545
// representing a PR that has already been reconciled at least once.
4646
func readyObjectMeta(name, namespace, repoName string) metav1.ObjectMeta {
47+
controller := true
48+
blockOwnerDeletion := true
4749
return metav1.ObjectMeta{
4850
Name: name,
4951
Namespace: namespace,
5052
Finalizers: []string{porchv1alpha2.PackageRevisionFinalizer},
5153
OwnerReferences: []metav1.OwnerReference{{
52-
APIVersion: configapi.GroupVersion.Identifier(),
53-
Kind: configapi.TypeRepository.Kind,
54-
Name: repoName,
55-
UID: "repo-uid",
54+
APIVersion: configapi.GroupVersion.Identifier(),
55+
Kind: configapi.TypeRepository.Kind,
56+
Name: repoName,
57+
UID: "repo-uid",
58+
Controller: &controller,
59+
BlockOwnerDeletion: &blockOwnerDeletion,
5660
}},
5761
}
5862
}
@@ -118,6 +122,10 @@ func TestReconcileFinalizerAddedWhenMissing(t *testing.T) {
118122
assert.Equal(t, "my-repo", pr.OwnerReferences[0].Name)
119123
assert.Equal(t, types.UID("repo-uid-123"), pr.OwnerReferences[0].UID)
120124
assert.Equal(t, configapi.TypeRepository.Kind, pr.OwnerReferences[0].Kind)
125+
require.NotNil(t, pr.OwnerReferences[0].Controller)
126+
assert.True(t, *pr.OwnerReferences[0].Controller)
127+
require.NotNil(t, pr.OwnerReferences[0].BlockOwnerDeletion)
128+
assert.True(t, *pr.OwnerReferences[0].BlockOwnerDeletion)
121129
}).Return(nil)
122130

123131
r := newTestReconciler(mockClient, mockrepository.NewMockContentCache(t))
@@ -681,6 +689,44 @@ func TestReconcileOwnerRefAlreadySet(t *testing.T) {
681689
ctx := t.Context()
682690
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-pr", Namespace: "default"}}
683691

692+
controller := true
693+
blockOwnerDeletion := true
694+
pr := &porchv1alpha2.PackageRevision{
695+
ObjectMeta: metav1.ObjectMeta{
696+
Name: "test-pr",
697+
Namespace: "default",
698+
Finalizers: []string{porchv1alpha2.PackageRevisionFinalizer},
699+
OwnerReferences: []metav1.OwnerReference{{
700+
APIVersion: configapi.GroupVersion.Identifier(),
701+
Kind: configapi.TypeRepository.Kind,
702+
Name: "my-repo",
703+
UID: "repo-uid-123",
704+
Controller: &controller,
705+
BlockOwnerDeletion: &blockOwnerDeletion,
706+
}},
707+
},
708+
Spec: porchv1alpha2.PackageRevisionSpec{RepositoryName: "my-repo"},
709+
}
710+
711+
mockClient := mockclient.NewMockClient(t)
712+
mockClient.EXPECT().Get(mock.Anything, req.NamespacedName, mock.AnythingOfType("*v1alpha2.PackageRevision")).
713+
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
714+
*obj.(*porchv1alpha2.PackageRevision) = *pr
715+
}).Return(nil)
716+
// No Patch, no repo Get — both finalizer and ownerRef already present.
717+
718+
r := newTestReconciler(mockClient, mockrepository.NewMockContentCache(t))
719+
result, err := r.Reconcile(ctx, req)
720+
721+
assert.NoError(t, err)
722+
assert.Equal(t, ctrl.Result{}, result)
723+
}
724+
725+
func TestReconcileOwnerRefIncompleteGetsUpdated(t *testing.T) {
726+
ctx := t.Context()
727+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-pr", Namespace: "default"}}
728+
729+
// Seed an ownerRef with matching Kind/Name/UID but nil boolean pointers.
684730
pr := &porchv1alpha2.PackageRevision{
685731
ObjectMeta: metav1.ObjectMeta{
686732
Name: "test-pr",
@@ -691,17 +737,154 @@ func TestReconcileOwnerRefAlreadySet(t *testing.T) {
691737
Kind: configapi.TypeRepository.Kind,
692738
Name: "my-repo",
693739
UID: "repo-uid-123",
740+
// Controller and BlockOwnerDeletion intentionally nil
694741
}},
695742
},
696743
Spec: porchv1alpha2.PackageRevisionSpec{RepositoryName: "my-repo"},
697744
}
698745

746+
repo := &configapi.Repository{
747+
ObjectMeta: metav1.ObjectMeta{Name: "my-repo", Namespace: "default", UID: "repo-uid-123"},
748+
}
749+
699750
mockClient := mockclient.NewMockClient(t)
700751
mockClient.EXPECT().Get(mock.Anything, req.NamespacedName, mock.AnythingOfType("*v1alpha2.PackageRevision")).
701752
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
702753
*obj.(*porchv1alpha2.PackageRevision) = *pr
703754
}).Return(nil)
704-
// No Patch, no repo Get — both finalizer and ownerRef already present.
755+
mockClient.EXPECT().Get(mock.Anything, types.NamespacedName{Namespace: "default", Name: "my-repo"}, mock.AnythingOfType("*v1alpha1.Repository")).
756+
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
757+
*obj.(*configapi.Repository) = *repo
758+
}).Return(nil)
759+
mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything).
760+
Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
761+
patched := obj.(*porchv1alpha2.PackageRevision)
762+
require.Len(t, patched.OwnerReferences, 1)
763+
ref := patched.OwnerReferences[0]
764+
assert.Equal(t, "my-repo", ref.Name)
765+
assert.Equal(t, types.UID("repo-uid-123"), ref.UID)
766+
assert.Equal(t, configapi.TypeRepository.Kind, ref.Kind)
767+
require.NotNil(t, ref.Controller, "Controller should be set after self-healing")
768+
assert.True(t, *ref.Controller)
769+
require.NotNil(t, ref.BlockOwnerDeletion, "BlockOwnerDeletion should be set after self-healing")
770+
assert.True(t, *ref.BlockOwnerDeletion)
771+
}).Return(nil)
772+
773+
r := newTestReconciler(mockClient, mockrepository.NewMockContentCache(t))
774+
result, err := r.Reconcile(ctx, req)
775+
776+
assert.NoError(t, err)
777+
assert.Equal(t, ctrl.Result{}, result)
778+
}
779+
780+
func TestReconcileOwnerRefWrongAPIVersionGetsUpdated(t *testing.T) {
781+
ctx := t.Context()
782+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-pr", Namespace: "default"}}
783+
784+
controller := true
785+
blockOwnerDeletion := true
786+
// Seed an ownerRef with correct Kind/Name/UID and booleans but wrong APIVersion.
787+
pr := &porchv1alpha2.PackageRevision{
788+
ObjectMeta: metav1.ObjectMeta{
789+
Name: "test-pr",
790+
Namespace: "default",
791+
Finalizers: []string{porchv1alpha2.PackageRevisionFinalizer},
792+
OwnerReferences: []metav1.OwnerReference{{
793+
APIVersion: "wrong.api/v1",
794+
Kind: configapi.TypeRepository.Kind,
795+
Name: "my-repo",
796+
UID: "repo-uid-123",
797+
Controller: &controller,
798+
BlockOwnerDeletion: &blockOwnerDeletion,
799+
}},
800+
},
801+
Spec: porchv1alpha2.PackageRevisionSpec{RepositoryName: "my-repo"},
802+
}
803+
804+
repo := &configapi.Repository{
805+
ObjectMeta: metav1.ObjectMeta{Name: "my-repo", Namespace: "default", UID: "repo-uid-123"},
806+
}
807+
808+
mockClient := mockclient.NewMockClient(t)
809+
mockClient.EXPECT().Get(mock.Anything, req.NamespacedName, mock.AnythingOfType("*v1alpha2.PackageRevision")).
810+
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
811+
*obj.(*porchv1alpha2.PackageRevision) = *pr
812+
}).Return(nil)
813+
mockClient.EXPECT().Get(mock.Anything, types.NamespacedName{Namespace: "default", Name: "my-repo"}, mock.AnythingOfType("*v1alpha1.Repository")).
814+
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
815+
*obj.(*configapi.Repository) = *repo
816+
}).Return(nil)
817+
mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything).
818+
Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
819+
patched := obj.(*porchv1alpha2.PackageRevision)
820+
require.Len(t, patched.OwnerReferences, 1)
821+
ref := patched.OwnerReferences[0]
822+
assert.Equal(t, configapi.GroupVersion.Identifier(), ref.APIVersion, "APIVersion should be corrected")
823+
assert.Equal(t, "my-repo", ref.Name)
824+
assert.Equal(t, types.UID("repo-uid-123"), ref.UID)
825+
require.NotNil(t, ref.Controller)
826+
assert.True(t, *ref.Controller)
827+
require.NotNil(t, ref.BlockOwnerDeletion)
828+
assert.True(t, *ref.BlockOwnerDeletion)
829+
}).Return(nil)
830+
831+
r := newTestReconciler(mockClient, mockrepository.NewMockContentCache(t))
832+
result, err := r.Reconcile(ctx, req)
833+
834+
assert.NoError(t, err)
835+
assert.Equal(t, ctrl.Result{}, result)
836+
}
837+
838+
func TestReconcileOwnerRefEmptyUIDGetsUpdated(t *testing.T) {
839+
ctx := t.Context()
840+
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-pr", Namespace: "default"}}
841+
842+
controller := true
843+
blockOwnerDeletion := true
844+
// Seed an ownerRef with correct Kind/Name/APIVersion and booleans but empty UID.
845+
pr := &porchv1alpha2.PackageRevision{
846+
ObjectMeta: metav1.ObjectMeta{
847+
Name: "test-pr",
848+
Namespace: "default",
849+
Finalizers: []string{porchv1alpha2.PackageRevisionFinalizer},
850+
OwnerReferences: []metav1.OwnerReference{{
851+
APIVersion: configapi.GroupVersion.Identifier(),
852+
Kind: configapi.TypeRepository.Kind,
853+
Name: "my-repo",
854+
UID: "",
855+
Controller: &controller,
856+
BlockOwnerDeletion: &blockOwnerDeletion,
857+
}},
858+
},
859+
Spec: porchv1alpha2.PackageRevisionSpec{RepositoryName: "my-repo"},
860+
}
861+
862+
repo := &configapi.Repository{
863+
ObjectMeta: metav1.ObjectMeta{Name: "my-repo", Namespace: "default", UID: "repo-uid-123"},
864+
}
865+
866+
mockClient := mockclient.NewMockClient(t)
867+
mockClient.EXPECT().Get(mock.Anything, req.NamespacedName, mock.AnythingOfType("*v1alpha2.PackageRevision")).
868+
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
869+
*obj.(*porchv1alpha2.PackageRevision) = *pr
870+
}).Return(nil)
871+
mockClient.EXPECT().Get(mock.Anything, types.NamespacedName{Namespace: "default", Name: "my-repo"}, mock.AnythingOfType("*v1alpha1.Repository")).
872+
Run(func(_ context.Context, _ types.NamespacedName, obj client.Object, _ ...client.GetOption) {
873+
*obj.(*configapi.Repository) = *repo
874+
}).Return(nil)
875+
mockClient.EXPECT().Patch(mock.Anything, mock.AnythingOfType("*v1alpha2.PackageRevision"), mock.Anything).
876+
Run(func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) {
877+
patched := obj.(*porchv1alpha2.PackageRevision)
878+
require.Len(t, patched.OwnerReferences, 1)
879+
ref := patched.OwnerReferences[0]
880+
assert.Equal(t, configapi.GroupVersion.Identifier(), ref.APIVersion)
881+
assert.Equal(t, "my-repo", ref.Name)
882+
assert.Equal(t, types.UID("repo-uid-123"), ref.UID, "UID should be populated after self-healing")
883+
require.NotNil(t, ref.Controller)
884+
assert.True(t, *ref.Controller)
885+
require.NotNil(t, ref.BlockOwnerDeletion)
886+
assert.True(t, *ref.BlockOwnerDeletion)
887+
}).Return(nil)
705888

706889
r := newTestReconciler(mockClient, mockrepository.NewMockContentCache(t))
707890
result, err := r.Reconcile(ctx, req)
@@ -745,15 +928,19 @@ func TestReconcileEmptyLifecycle(t *testing.T) {
745928
ctx := t.Context()
746929
req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-pr", Namespace: "default"}}
747930

931+
controller := true
932+
blockOwnerDeletion := true
748933
pr := &porchv1alpha2.PackageRevision{
749934
ObjectMeta: metav1.ObjectMeta{
750935
Name: "test-pr", Namespace: "default",
751936
Finalizers: []string{porchv1alpha2.PackageRevisionFinalizer},
752937
OwnerReferences: []metav1.OwnerReference{{
753-
APIVersion: configapi.GroupVersion.Identifier(),
754-
Kind: configapi.TypeRepository.Kind,
755-
Name: "my-repo",
756-
UID: "repo-uid",
938+
APIVersion: configapi.GroupVersion.Identifier(),
939+
Kind: configapi.TypeRepository.Kind,
940+
Name: "my-repo",
941+
UID: "repo-uid",
942+
Controller: &controller,
943+
BlockOwnerDeletion: &blockOwnerDeletion,
757944
}},
758945
},
759946
Spec: porchv1alpha2.PackageRevisionSpec{RepositoryName: "my-repo"},
@@ -1562,11 +1749,15 @@ func TestReconcileRenderErrorSetsStatus(t *testing.T) {
15621749
pr.Spec.Lifecycle = porchv1alpha2.PackageRevisionLifecycleDraft
15631750
pr.Spec.RepositoryName = "my-repo"
15641751
pr.Finalizers = []string{porchv1alpha2.PackageRevisionFinalizer}
1752+
controller := true
1753+
blockOwnerDeletion := true
15651754
pr.OwnerReferences = []metav1.OwnerReference{{
1566-
APIVersion: configapi.GroupVersion.Identifier(),
1567-
Kind: configapi.TypeRepository.Kind,
1568-
Name: "my-repo",
1569-
UID: "repo-uid",
1755+
APIVersion: configapi.GroupVersion.Identifier(),
1756+
Kind: configapi.TypeRepository.Kind,
1757+
Name: "my-repo",
1758+
UID: "repo-uid",
1759+
Controller: &controller,
1760+
BlockOwnerDeletion: &blockOwnerDeletion,
15701761
}}
15711762

15721763
mockClient := mockclient.NewMockClient(t)

‎controllers/repositories/pkg/controllers/repository/pkgrevsync.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/kptdev/porch/pkg/repository"
2424
"k8s.io/apimachinery/pkg/api/equality"
2525
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26+
"k8s.io/utils/ptr"
2627
"sigs.k8s.io/controller-runtime/pkg/client"
2728
"sigs.k8s.io/controller-runtime/pkg/log"
2829
)
@@ -258,10 +259,12 @@ func buildPackageRevision(ctx context.Context, repo *configapi.Repository, pkgRe
258259
Labels: packageRevisionLabelsWithLatest(repo.Name, isLatest),
259260
OwnerReferences: []metav1.OwnerReference{
260261
{
261-
APIVersion: configapi.GroupVersion.Identifier(),
262-
Kind: configapi.TypeRepository.Kind,
263-
Name: repo.Name,
264-
UID: repo.UID,
262+
APIVersion: configapi.GroupVersion.Identifier(),
263+
Kind: configapi.TypeRepository.Kind,
264+
Name: repo.Name,
265+
UID: repo.UID,
266+
Controller: ptr.To(true),
267+
BlockOwnerDeletion: ptr.To(true),
265268
},
266269
},
267270
},

‎controllers/repositories/pkg/controllers/repository/pkgrevsync_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/kptdev/porch/pkg/repository"
2828
"github.com/stretchr/testify/assert"
2929
"github.com/stretchr/testify/mock"
30+
"github.com/stretchr/testify/require"
3031
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3132
"k8s.io/apimachinery/pkg/types"
3233
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -177,6 +178,10 @@ func TestBuildPackageRevision(t *testing.T) {
177178
assert.Equal(t, "default", crd.Namespace)
178179
assert.Equal(t, repo.Name, crd.OwnerReferences[0].Name)
179180
assert.Equal(t, repo.UID, crd.OwnerReferences[0].UID)
181+
require.NotNil(t, crd.OwnerReferences[0].Controller)
182+
assert.True(t, *crd.OwnerReferences[0].Controller)
183+
require.NotNil(t, crd.OwnerReferences[0].BlockOwnerDeletion)
184+
assert.True(t, *crd.OwnerReferences[0].BlockOwnerDeletion)
180185

181186
// Spec — repo-owned identity fields
182187
assert.Equal(t, "path/to/my-pkg", crd.Spec.PackageName)

‎test/e2e/crd/repository_test.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,8 +370,14 @@ var _ = Describe("Repository", Ordered, Label("infra"), func() {
370370
waitForReady(env.Ctx, pr)
371371
publishPackage(env.Ctx, pr)
372372

373-
By("verifying the package exists")
373+
By("verifying the package exists with correct ownerReference fields")
374374
Expect(k8sClient.Get(env.Ctx, client.ObjectKeyFromObject(pr), pr)).To(Succeed())
375+
controllerRef := metav1.GetControllerOf(pr)
376+
Expect(controllerRef).NotTo(BeNil(), "expected a controlling ownerReference")
377+
Expect(controllerRef.Kind).To(Equal("Repository"))
378+
Expect(controllerRef.Name).To(Equal(repoName))
379+
Expect(controllerRef.BlockOwnerDeletion).NotTo(BeNil())
380+
Expect(*controllerRef.BlockOwnerDeletion).To(BeTrue())
375381

376382
By("deleting the repository")
377383
repo := &configapi.Repository{}

0 commit comments

Comments
 (0)