Skip to content

Commit 625fa8c

Browse files
committed
feat(crd): Detect and prevent breaking schema changes in RGDs
ResourceGraphDefinitions generate CRDs dynamically. Schema changes like removing properties, changing types, or adding required fields can break existing resources. This adds a compatibility layer that compares schemas before CRD updates and blocks breaking changes. Introduces a `pkg/graph/crd/compat` package with `Compare()` and `CompareVersions()` functions that return a `Report` identifying breaking vs non-breaking changes. The CRD wrapper now validates compatibility before applying updates. Resolves #186
1 parent cb129b5 commit 625fa8c

File tree

7 files changed

+1423
-14
lines changed

7 files changed

+1423
-14
lines changed

pkg/client/crd.go

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ import (
2828
"k8s.io/apimachinery/pkg/types"
2929
"k8s.io/apimachinery/pkg/util/wait"
3030
logr "sigs.k8s.io/controller-runtime/pkg/log"
31+
32+
crdcompat "github.com/kubernetes-sigs/kro/pkg/graph/crd/compat"
3133
)
3234

3335
const (
@@ -110,52 +112,72 @@ func newCRDWrapper(cfg CRDWrapperConfig) *CRDWrapper {
110112
// Ensure ensures a CRD exists, up-to-date, and is ready. This can be
111113
// a dangerous operation as it will update the CRD if it already exists.
112114
//
113-
// The caller is responsible for ensuring the CRD, isn't introducing
114-
// breaking changes.
115-
func (w *CRDWrapper) Ensure(ctx context.Context, crd v1.CustomResourceDefinition) error {
115+
// If a CRD does exist, it will compare the existing CRD with the desired CRD
116+
// and update it if necessary. If the existing CRD has breaking changes, it
117+
// will return an error.
118+
func (w *CRDWrapper) Ensure(ctx context.Context, desired v1.CustomResourceDefinition) error {
116119
log := logr.FromContext(ctx)
117-
existing, err := w.Get(ctx, crd.Name)
120+
existing, err := w.Get(ctx, desired.Name)
118121
if err != nil {
119122
if !apierrors.IsNotFound(err) {
120123
return fmt.Errorf("failed to check for existing CRD: %w", err)
121124
}
122125

123-
log.Info("Creating CRD", "name", crd.Name)
124-
if err := w.create(ctx, crd); err != nil {
126+
log.Info("Creating CRD", "name", desired.Name)
127+
if err := w.create(ctx, desired); err != nil {
125128
return fmt.Errorf("failed to create CRD: %w", err)
126129
}
127130
} else {
128-
kroOwned, nameMatch, idMatch := metadata.CompareRGDOwnership(existing.ObjectMeta, crd.ObjectMeta)
131+
// Check ownership first
132+
kroOwned, nameMatch, idMatch := metadata.CompareRGDOwnership(existing.ObjectMeta, desired.ObjectMeta)
129133
if !kroOwned {
130134
return fmt.Errorf(
131-
"failed to update CRD %s: CRD already exists and is not owned by KRO", crd.Name,
135+
"failed to update CRD %s: CRD already exists and is not owned by KRO", desired.Name,
132136
)
133137
}
134138

135139
if !nameMatch {
136140
existingRGDName := existing.Labels[metadata.ResourceGraphDefinitionNameLabel]
137141
return fmt.Errorf(
138142
"failed to update CRD %s: CRD is owned by another ResourceGraphDefinition %s",
139-
crd.Name, existingRGDName,
143+
desired.Name, existingRGDName,
140144
)
141145
}
142146

143147
if nameMatch && !idMatch {
144148
log.Info(
145149
"Adopting CRD with different RGD ID - RGD may have been deleted and recreated",
146-
"crd", crd.Name,
150+
"crd", desired.Name,
147151
"existingRGDID", existing.Labels[metadata.ResourceGraphDefinitionIDLabel],
148-
"newRGDID", crd.Labels[metadata.ResourceGraphDefinitionIDLabel],
152+
"newRGDID", desired.Labels[metadata.ResourceGraphDefinitionIDLabel],
149153
)
150154
}
151155

152-
log.Info("Updating existing CRD", "name", crd.Name)
153-
if err := w.patch(ctx, crd); err != nil {
156+
// Check for breaking schema changes
157+
report, err := crdcompat.CompareVersions(existing.Spec.Versions, desired.Spec.Versions)
158+
if err != nil {
159+
return fmt.Errorf("failed to check schema compatibility: %w", err)
160+
}
161+
162+
// If there are no changes at all, we can skip the update
163+
if !report.HasChanges() {
164+
log.V(1).Info("CRD is up-to-date", "name", desired.Name)
165+
return nil
166+
}
167+
168+
// Check for breaking changes
169+
if !report.IsCompatible() {
170+
log.Info("Breaking changes detected in CRD update", "name", desired.Name, "breakingChanges", len(report.BreakingChanges), "summary", report)
171+
return fmt.Errorf("cannot update CRD %s: breaking changes detected: %s", desired.Name, report)
172+
}
173+
174+
log.Info("Updating existing CRD", "name", desired.Name)
175+
if err := w.patch(ctx, desired); err != nil {
154176
return fmt.Errorf("failed to patch CRD: %w", err)
155177
}
156178
}
157179

158-
return w.waitForReady(ctx, crd.Name)
180+
return w.waitForReady(ctx, desired.Name)
159181
}
160182

161183
// Get retrieves a CRD by name

pkg/graph/crd/compat/changes.go

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
// Copyright 2025 The Kube Resource Orchestrator Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License"). You may
4+
// not use this file except in compliance with the License. A copy of the
5+
// License is located at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// or in the "license" file accompanying this file. This file is distributed
10+
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
11+
// express or implied. See the License for the specific language governing
12+
// permissions and limitations under the License.
13+
14+
package compat
15+
16+
import (
17+
"fmt"
18+
"strings"
19+
)
20+
21+
// ChangeType represents the type of schema change
22+
type ChangeType string
23+
24+
const (
25+
// Breaking change types
26+
PropertyRemoved ChangeType = "PROPERTY_REMOVED"
27+
TypeChanged ChangeType = "TYPE_CHANGED"
28+
RequiredAdded ChangeType = "REQUIRED_ADDED"
29+
EnumRestricted ChangeType = "ENUM_RESTRICTED"
30+
PatternChanged ChangeType = "PATTERN_CHANGED"
31+
PatternAdded ChangeType = "PATTERN_ADDED"
32+
33+
// Non-breaking change types
34+
PropertyAdded ChangeType = "PROPERTY_ADDED"
35+
DescriptionChanged ChangeType = "DESCRIPTION_CHANGED"
36+
DefaultChanged ChangeType = "DEFAULT_CHANGED"
37+
RequiredRemoved ChangeType = "REQUIRED_REMOVED"
38+
EnumExpanded ChangeType = "ENUM_EXPANDED"
39+
PatternRemoved ChangeType = "PATTERN_REMOVED"
40+
)
41+
42+
// Change represents a single schema change
43+
type Change struct {
44+
// Path is the JSON path to the changed property
45+
Path string
46+
// ChangeType is the type of change
47+
ChangeType ChangeType
48+
// OldValue is the string representation of the old value (if applicable)
49+
OldValue string
50+
// NewValue is the string representation of the new value (if applicable)
51+
NewValue string
52+
}
53+
54+
// Report contains the full analysis of schema differences
55+
type Report struct {
56+
// BreakingChanges are changes that break backward compatibility
57+
BreakingChanges []Change
58+
// NonBreakingChanges are changes that maintain backward compatibility
59+
NonBreakingChanges []Change
60+
}
61+
62+
// IsCompatible returns true if no breaking changes were detected
63+
func (r *Report) IsCompatible() bool {
64+
return len(r.BreakingChanges) == 0
65+
}
66+
67+
// HasBreakingChanges returns true if breaking changes were detected
68+
func (r *Report) HasBreakingChanges() bool {
69+
return len(r.BreakingChanges) > 0
70+
}
71+
72+
// HasChanges returns true if any changes were detected
73+
func (r *Report) HasChanges() bool {
74+
return len(r.BreakingChanges) > 0 || len(r.NonBreakingChanges) > 0
75+
}
76+
77+
const maxBreakingChangesSummary = 3
78+
79+
// SummarizeBreakingChanges returns a user-friendly summary of breaking changes
80+
func (r *Report) String() string {
81+
if !r.HasBreakingChanges() {
82+
return "no breaking changes"
83+
}
84+
85+
changeDescs := make([]string, 0, maxBreakingChangesSummary)
86+
87+
for i, change := range r.BreakingChanges {
88+
// Cut off the summary if there are too many breaking changes
89+
if i >= maxBreakingChangesSummary {
90+
remaining := len(r.BreakingChanges) - i
91+
if remaining > 0 {
92+
changeDescs = append(changeDescs, fmt.Sprintf("and %d more changes", remaining))
93+
}
94+
break
95+
}
96+
changeDescs = append(changeDescs, change.Description())
97+
}
98+
99+
return strings.Join(changeDescs, "; ")
100+
}
101+
102+
// AddBreakingChange adds a breaking change to the result with automatically generated description
103+
func (r *Report) AddBreakingChange(path string, changeType ChangeType, oldValue, newValue string) {
104+
r.BreakingChanges = append(r.BreakingChanges, Change{
105+
Path: path,
106+
ChangeType: changeType,
107+
OldValue: oldValue,
108+
NewValue: newValue,
109+
})
110+
}
111+
112+
// AddNonBreakingChange adds a non-breaking change to the result with automatically generated description
113+
func (r *Report) AddNonBreakingChange(path string, changeType ChangeType, oldValue, newValue string) {
114+
r.NonBreakingChanges = append(r.NonBreakingChanges, Change{
115+
Path: path,
116+
ChangeType: changeType,
117+
OldValue: oldValue,
118+
NewValue: newValue,
119+
})
120+
}
121+
122+
// lastPathComponent extracts the last component from a JSON path
123+
func lastPathComponent(path string) string {
124+
parts := strings.Split(path, ".")
125+
if len(parts) == 0 {
126+
return path
127+
}
128+
return parts[len(parts)-1]
129+
}
130+
131+
// Description generates a human-readable description based on the change type
132+
func (c Change) Description() string {
133+
propName := lastPathComponent(c.Path)
134+
135+
switch c.ChangeType {
136+
case PropertyRemoved:
137+
return fmt.Sprintf("Property %s was removed", propName)
138+
case PropertyAdded:
139+
if c.NewValue == "required" {
140+
return fmt.Sprintf("Required property %s was added", propName)
141+
}
142+
return fmt.Sprintf("Optional property %s was added", propName)
143+
case TypeChanged:
144+
return fmt.Sprintf("Type changed from %s to %s", c.OldValue, c.NewValue)
145+
case RequiredAdded:
146+
return fmt.Sprintf("Field %s is newly required", c.NewValue)
147+
case RequiredRemoved:
148+
return fmt.Sprintf("Field %s is no longer required", c.OldValue)
149+
case EnumRestricted:
150+
return fmt.Sprintf("Enum value %s was removed", c.OldValue)
151+
case EnumExpanded:
152+
return fmt.Sprintf("Enum value %s was added", c.NewValue)
153+
case PatternChanged:
154+
return fmt.Sprintf("Validation pattern changed from %s to %s", c.OldValue, c.NewValue)
155+
case PatternAdded:
156+
return fmt.Sprintf("Validation pattern %s was added", c.NewValue)
157+
case PatternRemoved:
158+
return fmt.Sprintf("Validation pattern %s was removed", c.OldValue)
159+
case DescriptionChanged:
160+
return "Description field was changed"
161+
case DefaultChanged:
162+
return "Default value was changed"
163+
default:
164+
return fmt.Sprintf("Unknown change to %s", c.Path)
165+
}
166+
}

pkg/graph/crd/compat/compat.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Copyright 2025 The Kube Resource Orchestrator Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License"). You may
4+
// not use this file except in compliance with the License. A copy of the
5+
// License is located at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// or in the "license" file accompanying this file. This file is distributed
10+
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
11+
// express or implied. See the License for the specific language governing
12+
// permissions and limitations under the License.
13+
14+
package compat
15+
16+
import (
17+
"fmt"
18+
19+
v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
20+
)
21+
22+
// CompareVersions compares CRD versions and returns a compatibility report.
23+
// This is a convenience wrapper that extracts schemas from version slices.
24+
// It expects exactly one version in each slice.
25+
func CompareVersions(oldVersions, newVersions []v1.CustomResourceDefinitionVersion) (*Report, error) {
26+
if len(oldVersions) != 1 || len(newVersions) != 1 {
27+
return nil, fmt.Errorf("expected exactly one version in each CRD, got %d old and %d new versions",
28+
len(oldVersions), len(newVersions))
29+
}
30+
31+
oldVersion := oldVersions[0]
32+
newVersion := newVersions[0]
33+
34+
// Check version names match
35+
if oldVersion.Name != newVersion.Name {
36+
return &Report{
37+
BreakingChanges: []Change{
38+
{
39+
Path: "version",
40+
ChangeType: TypeChanged,
41+
OldValue: oldVersion.Name,
42+
NewValue: newVersion.Name,
43+
},
44+
},
45+
}, nil
46+
}
47+
48+
// Verify schemas exist
49+
if oldVersion.Schema == nil || oldVersion.Schema.OpenAPIV3Schema == nil {
50+
return nil, fmt.Errorf("old version %s has no schema", oldVersion.Name)
51+
}
52+
53+
if newVersion.Schema == nil || newVersion.Schema.OpenAPIV3Schema == nil {
54+
return nil, fmt.Errorf("new version %s has no schema", newVersion.Name)
55+
}
56+
57+
// Compare schemas first
58+
report := Compare(oldVersion.Schema.OpenAPIV3Schema, newVersion.Schema.OpenAPIV3Schema)
59+
60+
// Check version metadata changes that could break kro
61+
// Served going from true to false would break API access
62+
if oldVersion.Served && !newVersion.Served {
63+
report.AddBreakingChange("version.served", TypeChanged, "true", "false")
64+
}
65+
66+
// Status subresource removal would break status updates
67+
oldHasStatus := oldVersion.Subresources != nil && oldVersion.Subresources.Status != nil
68+
newHasStatus := newVersion.Subresources != nil && newVersion.Subresources.Status != nil
69+
if oldHasStatus && !newHasStatus {
70+
report.AddBreakingChange("version.subresources.status", PropertyRemoved, "", "")
71+
}
72+
73+
return report, nil
74+
}

0 commit comments

Comments
 (0)