Skip to content

Commit 5eb29be

Browse files
michaelhtma-hilaly
michaelhtm
and
a-hilaly
authored
fix: remove RGD owner ID check to allow RGDs to adopt their CRD (#826)
* fix: remove RGD owner ID check to allow RGDs to adopt their CRD Currently, when a user deletes an RGD, and recreates it, the new RGD is no longer able to adopt the CRD the first RGD left dangling. This change removes the RGD ID check, ensuring, if an RGD with the same name as the one that created the CRD is created, it can adopt it. * log errors if RGD ID conflicts * Rework pkg utility function and seperation of concerns --------- Co-authored-by: Amine <[email protected]>
1 parent 6145171 commit 5eb29be

File tree

4 files changed

+116
-216
lines changed

4 files changed

+116
-216
lines changed

‎pkg/client/crd.go‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,28 @@ func (w *CRDWrapper) Ensure(ctx context.Context, crd v1.CustomResourceDefinition
125125
return fmt.Errorf("failed to create CRD: %w", err)
126126
}
127127
} else {
128-
if err := w.verifyOwnership(existing, crd); err != nil {
129-
return err
128+
kroOwned, nameMatch, idMatch := metadata.CompareRGDOwnership(existing.ObjectMeta, crd.ObjectMeta)
129+
if !kroOwned {
130+
return fmt.Errorf(
131+
"failed to update CRD %s: CRD already exists and is not owned by KRO", crd.Name,
132+
)
133+
}
134+
135+
if !nameMatch {
136+
existingRGDName := existing.Labels[metadata.ResourceGraphDefinitionNameLabel]
137+
return fmt.Errorf(
138+
"failed to update CRD %s: CRD is owned by another ResourceGraphDefinition %s",
139+
crd.Name, existingRGDName,
140+
)
141+
}
142+
143+
if nameMatch && !idMatch {
144+
log.Info(
145+
"Adopting CRD with different RGD ID - RGD may have been deleted and recreated",
146+
"crd", crd.Name,
147+
"existingRGDID", existing.Labels[metadata.ResourceGraphDefinitionIDLabel],
148+
"newRGDID", crd.Labels[metadata.ResourceGraphDefinitionIDLabel],
149+
)
130150
}
131151

132152
log.Info("Updating existing CRD", "name", crd.Name)
@@ -200,19 +220,3 @@ func (w *CRDWrapper) waitForReady(ctx context.Context, name string) error {
200220
return false, nil
201221
})
202222
}
203-
204-
// verifyOwnership checks that an existing CRD is owned by KRO and by the same
205-
// ResourceGraphDefinition that is attempting to update it. This prevents conflicts
206-
// where multiple ResourceGraphDefinitions try to manage the same CRD, or where a
207-
// CRD created outside of KRO is accidentally overwritten.
208-
func (w *CRDWrapper) verifyOwnership(existingCRD *v1.CustomResourceDefinition, newCRD v1.CustomResourceDefinition) error {
209-
if !metadata.IsKROOwned(&existingCRD.ObjectMeta) {
210-
return fmt.Errorf("conflict detected: CRD %s already exists and is not owned by KRO", existingCRD.Name)
211-
}
212-
213-
if !metadata.HasMatchingKROOwner(existingCRD.ObjectMeta, newCRD.ObjectMeta) {
214-
return fmt.Errorf("conflict detected: CRD %s has ownership by another ResourceGraphDefinition", existingCRD.Name)
215-
}
216-
217-
return nil
218-
}

‎pkg/client/crd_test.go‎

Lines changed: 0 additions & 148 deletions
This file was deleted.

‎pkg/metadata/labels.go‎

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

56-
// HasMatchingKROOwner returns true if resources have the same RGD owner.
57-
// Note: The caller is responsible for ensuring that KRO labels exist on both
58-
// resources before calling this function.
59-
func HasMatchingKROOwner(a, b metav1.ObjectMeta) bool {
60-
aOwnerName := a.Labels[ResourceGraphDefinitionNameLabel]
61-
aOwnerID := a.Labels[ResourceGraphDefinitionIDLabel]
56+
// CompareRGDOwnership compares RGD ownership labels between two resources.
57+
// Returns three booleans:
58+
// - kroOwned: whether the existing resource is owned by KRO
59+
// - nameMatch: whether both resources have the same RGD name
60+
// - idMatch: whether both resources have the same RGD ID
61+
//
62+
// This allows callers to distinguish between different ownership scenarios:
63+
// - kroOwned=true, nameMatch=true, idMatch=true: same RGD, normal update
64+
// - kroOwned=true, nameMatch=true, idMatch=false: same RGD name, different ID (adoption)
65+
// - kroOwned=true, nameMatch=false: different RGD (conflict)
66+
// - kroOwned=false: not owned by KRO (conflict)
67+
func CompareRGDOwnership(existing, desired metav1.ObjectMeta) (kroOwned, nameMatch, idMatch bool) {
68+
kroOwned = IsKROOwned(&existing)
69+
if !kroOwned {
70+
return false, false, false
71+
}
72+
73+
existingOwnerName := existing.Labels[ResourceGraphDefinitionNameLabel]
74+
existingOwnerID := existing.Labels[ResourceGraphDefinitionIDLabel]
75+
76+
desiredOwnerName := desired.Labels[ResourceGraphDefinitionNameLabel]
77+
desiredOwnerID := desired.Labels[ResourceGraphDefinitionIDLabel]
6278

63-
bOwnerName := b.Labels[ResourceGraphDefinitionNameLabel]
64-
bOwnerID := b.Labels[ResourceGraphDefinitionIDLabel]
79+
nameMatch = existingOwnerName == desiredOwnerName
80+
idMatch = existingOwnerID == desiredOwnerID
6581

66-
return aOwnerName == bOwnerName && aOwnerID == bOwnerID
82+
return kroOwned, nameMatch, idMatch
6783
}
6884

6985
// SetKROOwned sets the OwnedLabel to true on the resource.

‎pkg/metadata/labels_test.go‎

Lines changed: 69 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -65,62 +65,90 @@ func TestIsKROOwned(t *testing.T) {
6565
}
6666
}
6767

68-
func TestHasMatchingKROOwner(t *testing.T) {
68+
func TestCompareRGDOwnership(t *testing.T) {
6969
cases := []struct {
70-
name string
71-
aOwnerName string
72-
aOwnerID string
73-
bOwnerName string
74-
bOwnerID string
75-
expected bool
70+
name string
71+
existingLabels map[string]string
72+
desiredLabels map[string]string
73+
expectedKROOwned bool
74+
expectedNameMatch bool
75+
expectedIDMatch bool
7676
}{
7777
{
78-
name: "matching owners",
79-
aOwnerName: "test-rgd",
80-
aOwnerID: "test-uid-123",
81-
bOwnerName: "test-rgd",
82-
bOwnerID: "test-uid-123",
83-
expected: true,
78+
name: "same RGD - normal update",
79+
existingLabels: map[string]string{
80+
OwnedLabel: "true",
81+
ResourceGraphDefinitionNameLabel: "test-rgd",
82+
ResourceGraphDefinitionIDLabel: "test-id",
83+
},
84+
desiredLabels: map[string]string{
85+
OwnedLabel: "true",
86+
ResourceGraphDefinitionNameLabel: "test-rgd",
87+
ResourceGraphDefinitionIDLabel: "test-id",
88+
},
89+
expectedKROOwned: true,
90+
expectedNameMatch: true,
91+
expectedIDMatch: true,
8492
},
8593
{
86-
name: "different owner names",
87-
aOwnerName: "test-rgd-a",
88-
aOwnerID: "test-uid-123",
89-
bOwnerName: "test-rgd-b",
90-
bOwnerID: "test-uid-123",
91-
expected: false,
94+
name: "not owned by KRO",
95+
existingLabels: map[string]string{
96+
ResourceGraphDefinitionNameLabel: "test-rgd",
97+
ResourceGraphDefinitionIDLabel: "test-id",
98+
},
99+
desiredLabels: map[string]string{
100+
OwnedLabel: "true",
101+
ResourceGraphDefinitionNameLabel: "test-rgd",
102+
ResourceGraphDefinitionIDLabel: "test-id",
103+
},
104+
expectedKROOwned: false,
105+
expectedNameMatch: false,
106+
expectedIDMatch: false,
92107
},
93108
{
94-
name: "different owner IDs",
95-
aOwnerName: "test-rgd",
96-
aOwnerID: "test-uid-123",
97-
bOwnerName: "test-rgd",
98-
bOwnerID: "test-uid-456",
99-
expected: false,
109+
name: "different RGD name - conflict",
110+
existingLabels: map[string]string{
111+
OwnedLabel: "true",
112+
ResourceGraphDefinitionNameLabel: "existing-rgd",
113+
ResourceGraphDefinitionIDLabel: "test-id",
114+
},
115+
desiredLabels: map[string]string{
116+
OwnedLabel: "true",
117+
ResourceGraphDefinitionNameLabel: "new-rgd",
118+
ResourceGraphDefinitionIDLabel: "test-id",
119+
},
120+
expectedKROOwned: true,
121+
expectedNameMatch: false,
122+
expectedIDMatch: true,
100123
},
101124
{
102-
name: "no owner labels",
103-
expected: true,
125+
name: "same RGD name, different ID - adoption",
126+
existingLabels: map[string]string{
127+
OwnedLabel: "true",
128+
ResourceGraphDefinitionNameLabel: "test-rgd",
129+
ResourceGraphDefinitionIDLabel: "existing-id",
130+
},
131+
desiredLabels: map[string]string{
132+
OwnedLabel: "true",
133+
ResourceGraphDefinitionNameLabel: "test-rgd",
134+
ResourceGraphDefinitionIDLabel: "new-id",
135+
},
136+
expectedKROOwned: true,
137+
expectedNameMatch: true,
138+
expectedIDMatch: false,
104139
},
105140
}
106141

107142
for _, tc := range cases {
108143
t.Run(tc.name, func(t *testing.T) {
109-
labelsA := map[string]string{}
110-
labelsB := map[string]string{}
111-
if tc.aOwnerName != "" {
112-
labelsA[ResourceGraphDefinitionNameLabel] = tc.aOwnerName
113-
labelsA[ResourceGraphDefinitionIDLabel] = tc.aOwnerID
114-
}
115-
if tc.bOwnerName != "" {
116-
labelsB[ResourceGraphDefinitionNameLabel] = tc.bOwnerName
117-
labelsB[ResourceGraphDefinitionIDLabel] = tc.bOwnerID
118-
}
144+
existing := metav1.ObjectMeta{Labels: tc.existingLabels}
145+
desired := metav1.ObjectMeta{Labels: tc.desiredLabels}
119146

120-
metaA := metav1.ObjectMeta{Labels: labelsA}
121-
metaB := metav1.ObjectMeta{Labels: labelsB}
122-
result := HasMatchingKROOwner(metaA, metaB)
123-
assert.Equal(t, tc.expected, result)
147+
kroOwned, nameMatch, idMatch := CompareRGDOwnership(existing, desired)
148+
149+
assert.Equal(t, tc.expectedKROOwned, kroOwned)
150+
assert.Equal(t, tc.expectedNameMatch, nameMatch)
151+
assert.Equal(t, tc.expectedIDMatch, idMatch)
124152
})
125153
}
126154
}

0 commit comments

Comments
 (0)