- 
                Notifications
    You must be signed in to change notification settings 
- Fork 250
feat(crd): Prevent breaking changes in RGD schemas #352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -25,6 +25,8 @@ import ( | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
|  | ||
| crdcompat "github.com/kro-run/kro/pkg/graph/crd/compat" | ||
| ) | ||
|  | ||
| const ( | ||
|  | @@ -92,27 +94,46 @@ func newCRDWrapper(cfg CRDWrapperConfig) *CRDWrapper { | |
| // Ensure ensures a CRD exists, up-to-date, and is ready. This can be | ||
| // a dangerous operation as it will update the CRD if it already exists. | ||
| // | ||
| // The caller is responsible for ensuring the CRD, isn't introducing | ||
| // breaking changes. | ||
| func (w *CRDWrapper) Ensure(ctx context.Context, crd v1.CustomResourceDefinition) error { | ||
| _, err := w.Get(ctx, crd.Name) | ||
| // If a CRD does exist, it will compare the existing CRD with the desired CRD | ||
| // and update it if necessary. If the existing CRD has breaking changes, it | ||
| // will return an error. | ||
| func (w *CRDWrapper) Ensure(ctx context.Context, desired v1.CustomResourceDefinition) error { | ||
| current, err := w.Get(ctx, desired.Name) | ||
| if err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("failed to check for existing CRD: %w", err) | ||
| } | ||
|  | ||
| w.log.Info("Creating CRD", "name", crd.Name) | ||
| if err := w.create(ctx, crd); err != nil { | ||
| w.log.Info("Creating CRD", "name", desired.Name) | ||
| if err := w.create(ctx, desired); err != nil { | ||
| return fmt.Errorf("failed to create CRD: %w", err) | ||
| } | ||
| } else { | ||
| w.log.Info("Updating existing CRD", "name", crd.Name) | ||
| if err := w.patch(ctx, crd); err != nil { | ||
| // CRD exists, check compatibility | ||
| diffResult, err := crdcompat.DiffSchema(current.Spec.Versions, desired.Spec.Versions) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to check schema compatibility: %w", err) | ||
| } | ||
|  | ||
| // If there are no changes at all, we can skip the update | ||
| if !diffResult.HasChanges() { | ||
| w.log.Info("CRD is up-to-date", "name", desired.Name) | ||
| return nil | ||
| } | ||
|  | ||
| // Check for breaking changes | ||
| if diffResult.HasBreakingChanges() { | ||
| w.log.Info("Breaking changes detected in CRD update", "name", desired.Name, "breakingChanges", len(diffResult.BreakingChanges), "summary", diffResult) | ||
| return fmt.Errorf("cannot update CRD %s: breaking changes detected: %s", desired.Name, diffResult) | ||
| } | ||
| 
      Comment on lines
    
      +124
     to 
      +128
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could there be a way to override this? if someone doesn't care about the breaking changes | ||
|  | ||
| w.log.Info("Updating existing CRD", "name", desired.Name) | ||
| if err := w.patch(ctx, desired); err != nil { | ||
| return fmt.Errorf("failed to patch CRD: %w", err) | ||
| } | ||
| } | ||
|  | ||
| return w.waitForReady(ctx, crd.Name) | ||
| return w.waitForReady(ctx, desired.Name) | ||
| } | ||
|  | ||
| // Get retrieves a CRD by name | ||
|  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| // Copyright 2025 The Kube Resource Orchestrator Authors. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"). You may | ||
| // not use this file except in compliance with the License. A copy of the | ||
| // License is located at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // or in the "license" file accompanying this file. This file 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 compat | ||
|         
                  a-hilaly marked this conversation as resolved.
              Show resolved
            Hide resolved | ||
|  | ||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| ) | ||
|  | ||
| // ChangeType represents the type of schema change | ||
| type ChangeType string | ||
|  | ||
| const ( | ||
| // Breaking change types | ||
| PropertyRemoved ChangeType = "PROPERTY_REMOVED" | ||
| TypeChanged ChangeType = "TYPE_CHANGED" | ||
| RequiredAdded ChangeType = "REQUIRED_ADDED" | ||
| EnumRestricted ChangeType = "ENUM_RESTRICTED" | ||
| PatternChanged ChangeType = "PATTERN_CHANGED" | ||
|  | ||
| // Non-breaking change types | ||
| PropertyAdded ChangeType = "PROPERTY_ADDED" | ||
| DescriptionChanged ChangeType = "DESCRIPTION_CHANGED" | ||
| DefaultChanged ChangeType = "DEFAULT_CHANGED" | ||
| RequiredRemoved ChangeType = "REQUIRED_REMOVED" | ||
| EnumExpanded ChangeType = "ENUM_EXPANDED" | ||
| 
      Comment on lines
    
      +25
     to 
      +37
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can add number changes as well..I saw a PR approved for adding maximum and minimum to the schema. We should also track those changes.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are those considered breaking changes? should we really inspect the defaults/max/min and basically every attribute in https://kro.run/docs/concepts/simple-schema#supported-markers ? cc @jakobmoellerdev | ||
| ) | ||
|  | ||
| // Change represents a single schema change | ||
| type Change struct { | ||
| // Path is the JSON path to the changed property | ||
| Path string | ||
| // ChangeType is the type of change | ||
| ChangeType ChangeType | ||
| // OldValue is the string representation of the old value (if applicable) | ||
| OldValue string | ||
| // NewValue is the string representation of the new value (if applicable) | ||
| NewValue string | ||
| } | ||
|  | ||
| // DiffResult contains the full analysis of schema differences | ||
| type DiffResult struct { | ||
| // BreakingChanges are changes that break backward compatibility | ||
| BreakingChanges []Change | ||
| // NonBreakingChanges are changes that maintain backward compatibility | ||
| NonBreakingChanges []Change | ||
| } | ||
|  | ||
| // HasBreakingChanges returns true if breaking changes were detected | ||
| func (r *DiffResult) HasBreakingChanges() bool { | ||
| return len(r.BreakingChanges) > 0 | ||
| } | ||
|  | ||
| // HasChanges returns true if any changes were detected | ||
| func (r *DiffResult) HasChanges() bool { | ||
| return len(r.BreakingChanges) > 0 || len(r.NonBreakingChanges) > 0 | ||
| } | ||
|  | ||
| const maxBreakingChangesSummary = 3 | ||
|  | ||
| // SummarizeBreakingChanges returns a user-friendly summary of breaking changes | ||
| func (r *DiffResult) String() string { | ||
| if !r.HasBreakingChanges() { | ||
| return "no breaking changes" | ||
| } | ||
|  | ||
| changeDescs := make([]string, 0, maxBreakingChangesSummary) | ||
|  | ||
| for i, change := range r.BreakingChanges { | ||
| // Cut off the summary if there are too many breaking changes | ||
| if i >= maxBreakingChangesSummary { | ||
| remaining := len(r.BreakingChanges) - i | ||
| if remaining > 0 { | ||
| changeDescs = append(changeDescs, fmt.Sprintf("and %d more changes", remaining)) | ||
| } | ||
| 
      Comment on lines
    
      +82
     to 
      +86
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 | ||
| break | ||
| } | ||
| changeDescs = append(changeDescs, change.Description()) | ||
| } | ||
|  | ||
| return strings.Join(changeDescs, "; ") | ||
| } | ||
|  | ||
| // AddBreakingChange adds a breaking change to the result with automatically generated description | ||
| func (r *DiffResult) AddBreakingChange(path string, changeType ChangeType, oldValue, newValue string) { | ||
| r.BreakingChanges = append(r.BreakingChanges, Change{ | ||
| Path: path, | ||
| ChangeType: changeType, | ||
| OldValue: oldValue, | ||
| NewValue: newValue, | ||
| }) | ||
| } | ||
|  | ||
| // AddNonBreakingChange adds a non-breaking change to the result with automatically generated description | ||
| func (r *DiffResult) AddNonBreakingChange(path string, changeType ChangeType, oldValue, newValue string) { | ||
| r.NonBreakingChanges = append(r.NonBreakingChanges, Change{ | ||
| Path: path, | ||
| ChangeType: changeType, | ||
| OldValue: oldValue, | ||
| NewValue: newValue, | ||
| }) | ||
| } | ||
|  | ||
| // lastPathComponent extracts the last component from a JSON path | ||
| func lastPathComponent(path string) string { | ||
| parts := strings.Split(path, ".") | ||
| if len(parts) == 0 { | ||
| return path | ||
| } | ||
| return parts[len(parts)-1] | ||
| } | ||
|  | ||
| // Description generates a human-readable description based on the change type | ||
| func (c Change) Description() string { | ||
| propName := lastPathComponent(c.Path) | ||
|  | ||
| switch c.ChangeType { | ||
| case PropertyRemoved: | ||
| return fmt.Sprintf("Property %s was removed", propName) | ||
| case PropertyAdded: | ||
| if c.NewValue == "required" { | ||
| return fmt.Sprintf("Required property %s was added", propName) | ||
| } | ||
| return fmt.Sprintf("Optional property %s was added", propName) | ||
| case TypeChanged: | ||
| return fmt.Sprintf("Type changed from %s to %s", c.OldValue, c.NewValue) | ||
| case RequiredAdded: | ||
| return fmt.Sprintf("Field %s is newly required", c.NewValue) | ||
| case RequiredRemoved: | ||
| return fmt.Sprintf("Field %s is no longer required", c.OldValue) | ||
| case EnumRestricted: | ||
| return fmt.Sprintf("Enum value %s was removed", c.OldValue) | ||
| case EnumExpanded: | ||
| return fmt.Sprintf("Enum value %s was added", c.NewValue) | ||
| case PatternChanged: | ||
| return fmt.Sprintf("Validation pattern changed from %s to %s", c.OldValue, c.NewValue) | ||
| case DescriptionChanged: | ||
| return "Description field was changed" | ||
| case DefaultChanged: | ||
| return "Default value was changed" | ||
| default: | ||
| return fmt.Sprintf("Unknown change to %s", c.Path) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| // Copyright 2025 The Kube Resource Orchestrator Authors. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"). You may | ||
| // not use this file except in compliance with the License. A copy of the | ||
| // License is located at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // or in the "license" file accompanying this file. This file 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 compat | ||
|  | ||
| import ( | ||
| "fmt" | ||
|  | ||
| v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| ) | ||
|  | ||
| // DiffSchema compares schema versions and returns compatibility details. | ||
| // This function expects exactly one version in each slice. If more versions are present, | ||
| // it will return an error as multi-version support is not implemented. | ||
| func DiffSchema(oldVersions []v1.CustomResourceDefinitionVersion, newVersions []v1.CustomResourceDefinitionVersion) (*DiffResult, error) { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
 | ||
| // Validate single version assumption | ||
| if len(oldVersions) != 1 || len(newVersions) != 1 { | ||
| return nil, fmt.Errorf("expected exactly one version in each CRD, got %d old and %d new versions", | ||
| len(oldVersions), len(newVersions)) | ||
| } | ||
|  | ||
| oldVersion := oldVersions[0] | ||
| newVersion := newVersions[0] | ||
|  | ||
| // Check version names match | ||
| if oldVersion.Name != newVersion.Name { | ||
| return &DiffResult{ | ||
| BreakingChanges: []Change{ | ||
| { | ||
| Path: "version", | ||
| ChangeType: TypeChanged, | ||
| OldValue: oldVersion.Name, | ||
| NewValue: newVersion.Name, | ||
| }, | ||
| }, | ||
| }, nil | ||
| } | ||
|  | ||
| // Verify schemas exist | ||
| if oldVersion.Schema == nil || oldVersion.Schema.OpenAPIV3Schema == nil { | ||
| return nil, fmt.Errorf("old version %s has no schema", oldVersion.Name) | ||
| } | ||
|  | ||
| if newVersion.Schema == nil || newVersion.Schema.OpenAPIV3Schema == nil { | ||
| return nil, fmt.Errorf("new version %s has no schema", newVersion.Name) | ||
| } | ||
|  | ||
| // Compare schemas | ||
| return compareSchemas( | ||
| fmt.Sprintf("version.%s.schema", oldVersion.Name), | ||
| oldVersion.Schema.OpenAPIV3Schema, | ||
| newVersion.Schema.OpenAPIV3Schema, | ||
| ) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to make this a trace level comment to reduce the log chatter