feat(crd): Prevent breaking changes in RGD schemas #352
Conversation
`ResourceGraphDefinitions` generate CRDs on the fly, which means one wrong move in the schema can break existing resources. Deleting a critical field or switching types can cause immediate deployment failures. This patch introduces a `DiffSchema` function that systematically compares old and new schemas, identifying breaking changes like property removals, type modifications, and constraint updates. It categorizes changes as breaking or non breaking, allowing safer schema evolution while preventing accidental resource invalidation.
There was a problem hiding this comment.
Great job on this @a-hilaly!! 🚀🚀🚀
I left a few inline comments
| // 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) | ||
| } |
There was a problem hiding this comment.
could there be a way to override this? if someone doesn't care about the breaking changes
| // 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" |
There was a problem hiding this comment.
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..
| if i >= maxBreakingChangesSummary { | ||
| remaining := len(r.BreakingChanges) - i | ||
| if remaining > 0 { | ||
| changeDescs = append(changeDescs, fmt.Sprintf("and %d more changes", remaining)) | ||
| } |
| // express or implied. See the License for the specific language governing | ||
| // permissions and limitations under the License. | ||
|
|
||
| package compat |
There was a problem hiding this comment.
nit:?
| package compat | |
| package compare |
There was a problem hiding this comment.
I think the current name is ok, it is not comparing, it is attempting to determine compatibility
|
|
||
| // 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) |
There was a problem hiding this comment.
Might want to make this a trace level comment to reduce the log chatter
| // 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.
- removal of sub-resources might matter?
- flopping Deprecated could be bad
- setting served or storage to false would break KRO
| // express or implied. See the License for the specific language governing | ||
| // permissions and limitations under the License. | ||
|
|
||
| package compat |
There was a problem hiding this comment.
I think the current name is ok, it is not comparing, it is attempting to determine compatibility
| path+".type", | ||
| TypeChanged, | ||
| string(oldSchema.Type), | ||
| string(newSchema.Type), |
There was a problem hiding this comment.
nit: .Type is a string already
| // existing properties, since new properties are handled in compareProperties. | ||
| func compareRequiredFields(path string, oldSchema, newSchema *v1.JSONSchemaProps, result *DiffResult) { | ||
| // Use length checks instead of nil checks | ||
| if len(oldSchema.Required) == 0 && len(newSchema.Required) == 0 { |
There was a problem hiding this comment.
you might want to test that old and new schema are not nil as well just to protect yourself. It is likely someone attempts to delete the schema to work around something.
| for req := range newRequiredSet { | ||
| // Only consider requirements for properties that already existed | ||
| if existingProps[req] && !oldRequiredSet[req] { | ||
| result.AddBreakingChange(path+".required", RequiredAdded, "", req) |
There was a problem hiding this comment.
it is too bad this is not easier to use but Knative made FieldError to help with pathed errrors: https://github.com/knative/pkg/blob/b7bbf4be5dbd9a8b8b60e06094bd8930cf241357/apis/field_error.go#L63
ResourceGraphDefinitionsgenerate CRDs on the fly, which means one wrongmodification in the schema can break existing resources/crds. e.g deleting a required
field or changing types can be problematic.
This patch introduces a
DiffSchemafunction that systematically comparesold and new schemas, identifying breaking changes like property removals,
type modifications, and constraint updates. It categorizes changes as
breaking or non breaking, allowing safer schema evolution while preventing
accidental resource invalidation.