- Notifications
You must be signed in to change notification settings - Fork161
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(crd): Prevent breaking changes in RGD schemas#352
base:main
Are you sure you want to change the base?
Conversation
`ResourceGraphDefinitions` generate CRDs on the fly, which means one wrongmove in the schema can break existing resources. Deleting a critical fieldor switching types can cause immediate deployment failures.This patch introduces a `DiffSchema` function that systematically comparesold and new schemas, identifying breaking changes like property removals,type modifications, and constraint updates. It categorizes changes asbreaking or non breaking, allowing safer schema evolution while preventingaccidental resource invalidation.
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.
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.
Choose a reason for hiding this comment
The 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
| // 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.
Choose a reason for hiding this comment
The 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..
| if i >= maxBreakingChangesSummary { | ||
| remaining := len(r.BreakingChanges) - i | ||
| if remaining > 0 { | ||
| changeDescs = append(changeDescs, fmt.Sprintf("and %d more changes", remaining)) | ||
| } |
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.
💯
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit:?
| packagecompat | |
| packagecompare |
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.
I think the current name is ok, it is not comparing, it is attempting to determine compatibility
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.
LGTM
| // 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.
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
| // 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 comment
The reason will be displayed to describe this comment to others.Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it is too bad this is not easier to use but Knative madeFieldError 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.