Skip to content

Commit d92d623

Browse files
a-bucka-buck
committed
bug: verify existing ownership of CRD managed by ResourceGraphDefinition to prevent conflict
this prevents 2 ResourceGraphDefinitions fighting to manage the same CRD
1 parent 0c65b16 commit d92d623

File tree

3 files changed

+163
-1
lines changed

3 files changed

+163
-1
lines changed

‎pkg/client/crd.go‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"fmt"
2121
"time"
2222

23+
"github.com/kro-run/kro/pkg/metadata"
2324
v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2425
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
2526
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -95,7 +96,7 @@ func newCRDWrapper(cfg CRDWrapperConfig) *CRDWrapper {
9596
// breaking changes.
9697
func (w *CRDWrapper) Ensure(ctx context.Context, crd v1.CustomResourceDefinition) error {
9798
log := logr.FromContext(ctx)
98-
_, err := w.Get(ctx, crd.Name)
99+
existing, err := w.Get(ctx, crd.Name)
99100
if err != nil {
100101
if !apierrors.IsNotFound(err) {
101102
return fmt.Errorf("failed to check for existing CRD: %w", err)
@@ -106,6 +107,10 @@ func (w *CRDWrapper) Ensure(ctx context.Context, crd v1.CustomResourceDefinition
106107
return fmt.Errorf("failed to create CRD: %w", err)
107108
}
108109
} else {
110+
if err := w.verifyNoConflict(existing, crd); err != nil {
111+
return err
112+
}
113+
109114
log.Info("Updating existing CRD", "name", crd.Name)
110115
if err := w.patch(ctx, crd); err != nil {
111116
return fmt.Errorf("failed to patch CRD: %w", err)
@@ -177,3 +182,15 @@ func (w *CRDWrapper) waitForReady(ctx context.Context, name string) error {
177182
return false, nil
178183
})
179184
}
185+
186+
func (w *CRDWrapper) verifyNoConflict(existingCRD *v1.CustomResourceDefinition, newCRD v1.CustomResourceDefinition) error {
187+
if !metadata.IsKROOwned(existingCRD.ObjectMeta) {
188+
return fmt.Errorf("conflict detected: CRD %s already exists and is not owned by KRO", existingCRD.Name)
189+
}
190+
191+
if !metadata.HasMatchingKROOwner(existingCRD.ObjectMeta, newCRD.ObjectMeta) {
192+
return fmt.Errorf("conflict detected: CRD %s has ownership by another ResourceGraphDefinition", existingCRD.Name)
193+
}
194+
195+
return nil
196+
}

‎pkg/client/crd_test.go‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
package client
2+
3+
import (
4+
"testing"
5+
6+
v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
7+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
8+
9+
"github.com/kro-run/kro/pkg/metadata"
10+
"github.com/stretchr/testify/assert"
11+
)
12+
13+
func TestCRDWrapper_verifyNoConflict(t *testing.T) {
14+
tests := []struct {
15+
name string
16+
existingCRD *v1.CustomResourceDefinition
17+
newCRD v1.CustomResourceDefinition
18+
wantErr bool
19+
errMsg string
20+
}{
21+
{
22+
name: "successful verification - same owner",
23+
existingCRD: &v1.CustomResourceDefinition{
24+
ObjectMeta: metav1.ObjectMeta{
25+
Name: "test.crd",
26+
Labels: map[string]string{
27+
metadata.OwnedLabel: "true",
28+
metadata.ResourceGraphDefinitionNameLabel: "test-rgd",
29+
metadata.ResourceGraphDefinitionIDLabel: "test-id",
30+
},
31+
},
32+
},
33+
newCRD: v1.CustomResourceDefinition{
34+
ObjectMeta: metav1.ObjectMeta{
35+
Name: "test.crd",
36+
Labels: map[string]string{
37+
metadata.OwnedLabel: "true",
38+
metadata.ResourceGraphDefinitionNameLabel: "test-rgd",
39+
metadata.ResourceGraphDefinitionIDLabel: "test-id",
40+
},
41+
},
42+
},
43+
wantErr: false,
44+
},
45+
{
46+
name: "conflict - not owned by KRO",
47+
existingCRD: &v1.CustomResourceDefinition{
48+
ObjectMeta: metav1.ObjectMeta{
49+
Name: "test.crd",
50+
Labels: map[string]string{
51+
metadata.ResourceGraphDefinitionNameLabel: "test-rgd",
52+
metadata.ResourceGraphDefinitionIDLabel: "test-id",
53+
},
54+
},
55+
},
56+
newCRD: v1.CustomResourceDefinition{
57+
ObjectMeta: metav1.ObjectMeta{
58+
Name: "test.crd",
59+
Labels: map[string]string{
60+
metadata.OwnedLabel: "true",
61+
metadata.ResourceGraphDefinitionNameLabel: "test-rgd",
62+
metadata.ResourceGraphDefinitionIDLabel: "test-id",
63+
},
64+
},
65+
},
66+
wantErr: true,
67+
errMsg: "conflict detected: CRD test.crd already exists and is not owned by KRO",
68+
},
69+
{
70+
name: "conflict - different RGD name",
71+
existingCRD: &v1.CustomResourceDefinition{
72+
ObjectMeta: metav1.ObjectMeta{
73+
Name: "test.crd",
74+
Labels: map[string]string{
75+
metadata.OwnedLabel: "true",
76+
metadata.ResourceGraphDefinitionNameLabel: "existing-rgd",
77+
metadata.ResourceGraphDefinitionIDLabel: "test-id",
78+
},
79+
},
80+
},
81+
newCRD: v1.CustomResourceDefinition{
82+
ObjectMeta: metav1.ObjectMeta{
83+
Name: "test.crd",
84+
Labels: map[string]string{
85+
metadata.OwnedLabel: "true",
86+
metadata.ResourceGraphDefinitionNameLabel: "new-rgd",
87+
metadata.ResourceGraphDefinitionIDLabel: "test-id",
88+
},
89+
},
90+
},
91+
wantErr: true,
92+
errMsg: "conflict detected: CRD test.crd has ownership by another ResourceGraphDefinition",
93+
},
94+
{
95+
name: "conflict - different RGD ID",
96+
existingCRD: &v1.CustomResourceDefinition{
97+
ObjectMeta: metav1.ObjectMeta{
98+
Name: "test.crd",
99+
Labels: map[string]string{
100+
metadata.OwnedLabel: "true",
101+
metadata.ResourceGraphDefinitionNameLabel: "test-rgd",
102+
metadata.ResourceGraphDefinitionIDLabel: "existing-id",
103+
},
104+
},
105+
},
106+
newCRD: v1.CustomResourceDefinition{
107+
ObjectMeta: metav1.ObjectMeta{
108+
Name: "test.crd",
109+
Labels: map[string]string{
110+
metadata.OwnedLabel: "true",
111+
metadata.ResourceGraphDefinitionNameLabel: "test-rgd",
112+
metadata.ResourceGraphDefinitionIDLabel: "new-id",
113+
},
114+
},
115+
},
116+
wantErr: true,
117+
errMsg: "conflict detected: CRD test.crd has ownership by another ResourceGraphDefinition",
118+
},
119+
}
120+
121+
for _, tt := range tests {
122+
t.Run(tt.name, func(t *testing.T) {
123+
w := &CRDWrapper{}
124+
err := w.verifyNoConflict(tt.existingCRD, tt.newCRD)
125+
126+
if tt.wantErr {
127+
assert.Error(t, err)
128+
assert.Equal(t, tt.errMsg, err.Error())
129+
} else {
130+
assert.NoError(t, err)
131+
}
132+
})
133+
}
134+
}

‎pkg/metadata/labels.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,17 @@ func IsKROOwned(meta metav1.ObjectMeta) bool {
5353
return ok && booleanFromString(v)
5454
}
5555

56+
// HasMatchingKROOwner returns true if resources have the same RGD owner
57+
func HasMatchingKROOwner(a, b metav1.ObjectMeta) bool {
58+
aOwnerName := a.Labels[ResourceGraphDefinitionNameLabel]
59+
aOwnerID := a.Labels[ResourceGraphDefinitionIDLabel]
60+
61+
bOwnerName := b.Labels[ResourceGraphDefinitionNameLabel]
62+
bOwnerID := b.Labels[ResourceGraphDefinitionIDLabel]
63+
64+
return aOwnerName == bOwnerName && aOwnerID == bOwnerID
65+
}
66+
5667
// SetKROOwned sets the OwnedLabel to true on the resource.
5768
func SetKROOwned(meta metav1.ObjectMeta) {
5869
setLabel(&meta, OwnedLabel, stringFromBoolean(true))

0 commit comments

Comments
 (0)