feat: Add support for old resources in validate#166
Open
cychiang wants to merge 2 commits into
Open
Conversation
Thread previous resource state into CEL validation so transition rules that reference oldSelf (such as immutability constraints) are evaluated. Old resources are matched to resources by GVK, name, and namespace; a resource with no match is validated as a create, leaving its transition rules skipped. Signed-off-by: Chuan-Yen Chiang <cychiang0823@gmail.com>
Signed-off-by: Chuan-Yen Chiang <cychiang0823@gmail.com>
cychiang
force-pushed
the
feat-add-validate-old-resources
branch
from
df837b5 to
642a44a
Compare
Contributor
📝 WalkthroughWalkthroughThis PR adds support for supplying previous resource state to ChangesOld resources transition validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Thanks for adding this — nicely closes the gap between create-only validation and real transition-rule testing! 🎉 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crossplane/validate/cmd.go (1)
110-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThanks for adding this — but the stdin guard needs to grow with the new flag.
c.OldResourcescan also be"-"(it goes through the sameload.NewLoader), but this check only guards againstResources+Extensionsboth being stdin. If a user runs something like... resources.yaml - --old-resources -(or extensions+old-resources both-), two loaders will race to read the same stdin stream — one gets the real payload, the other silently gets nothing (or errors confusingly). Could we generalize the guard to cover all three inputs?🐛 Proposed fix generalizing the stdin guard
- if c.Resources == "-" && c.Extensions == "-" { - return errors.New("cannot use stdin for both extensions and resources") - } + stdinInputs := 0 + for _, in := range []string{c.Resources, c.Extensions, c.OldResources} { + if in == "-" { + stdinInputs++ + } + } + if stdinInputs > 1 { + return errors.New("cannot use stdin for more than one input") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/validate/cmd.go` around lines 110 - 113, The stdin guard in Cmd.Run only checks Resources and Extensions, but OldResources also uses load.NewLoader and can consume stdin. Update the validation in Cmd.Run to reject any combination where more than one of Resources, Extensions, or OldResources is "-" so only a single loader can read stdin, and keep the error message aligned with this broader guard.
🧹 Nitpick comments (1)
cmd/crossplane/validate/cmd.go (1)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider enriching the help text to match the sibling
Resources/Extensionsargs.
Resourcesdocuments its accepted forms ("files, directories, or-for standard input"), butOldResources's help string doesn't mention format at all, even though it goes through the same loader. Spelling that out here would help users discover-/directory/comma-separated support without digging into the docs.♻️ Proposed help text tweak
- OldResources string `help:"Previous resource state for evaluating CEL transition rules."` + OldResources string `help:"Previous resource state for evaluating CEL transition rules, as a comma-separated list of files, directories, or '-' for standard input."`As per path instructions, "Review CLI commands for proper flag handling, help text, and error messages."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/validate/cmd.go` at line 84, The OldResources flag help text is too vague compared with Resources and Extensions. Update the OldResources help string in the validate command definition to mention the accepted input forms handled by the shared loader, such as files, directories, comma-separated values, and - for standard input, so users can discover the supported formats directly from the CLI help.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/validate/validate.go`:
- Around line 63-97: `resourceKey` is vulnerable to collisions because it
concatenates GVK, name, and namespace into a single string with hyphens, so
distinct resources can map to the same key and mispair old/new objects in
`Validate`; replace the string-based keying in `Validate` and `resourceKey` with
a struct or other unambiguous composite key built from `GroupVersionKind`,
`getResourceName`, and `GetNamespace`, and update the `oldByKey` map accordingly
so duplicate handling cannot silently match the wrong resource. If `fmt` is only
used by `resourceKey`, remove that import after the refactor.
---
Outside diff comments:
In `@cmd/crossplane/validate/cmd.go`:
- Around line 110-113: The stdin guard in Cmd.Run only checks Resources and
Extensions, but OldResources also uses load.NewLoader and can consume stdin.
Update the validation in Cmd.Run to reject any combination where more than one
of Resources, Extensions, or OldResources is "-" so only a single loader can
read stdin, and keep the error message aligned with this broader guard.
---
Nitpick comments:
In `@cmd/crossplane/validate/cmd.go`:
- Line 84: The OldResources flag help text is too vague compared with Resources
and Extensions. Update the OldResources help string in the validate command
definition to mention the accepted input forms handled by the shared loader,
such as files, directories, comma-separated values, and - for standard input, so
users can discover the supported formats directly from the CLI help.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20f428bb-1258-4af1-831d-3f1b3de61f27
⛔ Files ignored due to path filters (3)
cmd/crossplane/validate/testdata/cmd/crd_transition.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_transition_new.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_transition_old.yamlis excluded by!**/testdata/**and included by**/*.yaml
📒 Files selected for processing (5)
cmd/crossplane/validate/cmd.gocmd/crossplane/validate/help/validate.mdcmd/crossplane/validate/validate_integration_test.gopkg/validate/validate.gopkg/validate/validate_test.go
Comment on lines
+63
to
+97
| // Index old resources by GVK+name+namespace so each resource can be paired | ||
| // with its previous state. On duplicate keys the last entry wins. | ||
| oldByKey := make(map[string]*unstructured.Unstructured, len(oldResources)) | ||
| for _, o := range oldResources { | ||
| oldByKey[resourceKey(o)] = o | ||
| } | ||
|
|
||
| result := &ValidationResult{ | ||
| Resources: make([]ResourceValidationResult, 0, len(resources)), | ||
| } | ||
| for _, r := range resources { | ||
| result.Resources = append(result.Resources, validateResource(ctx, r, schemaValidators, structurals, crds)) | ||
| result.Resources = append(result.Resources, validateResource(ctx, r, objectOf(oldByKey[resourceKey(r)]), schemaValidators, structurals, crds)) | ||
| } | ||
| result.Summary = computeSummary(result.Resources) | ||
| return result, nil | ||
| } | ||
|
|
||
| // resourceKey identifies a resource by GroupVersionKind, name, and namespace. | ||
| // It is used to match a resource under validation to its previous state so CEL | ||
| // transition rules see the right old object. | ||
| func resourceKey(r *unstructured.Unstructured) string { | ||
| gvk := r.GetObjectKind().GroupVersionKind() | ||
| return fmt.Sprintf("%s-%s-%s", gvk.String(), getResourceName(r), r.GetNamespace()) | ||
| } | ||
|
|
||
| // objectOf returns the unstructured content of u, or nil when u is nil. It | ||
| // keeps the nil check for an unmatched old resource in one place so callers can | ||
| // pass the result straight to the CEL validator's oldObject argument. | ||
| func objectOf(u *unstructured.Unstructured) map[string]any { | ||
| if u == nil { | ||
| return nil | ||
| } | ||
| return u.Object | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Thanks for wiring this up — but resourceKey has a collision bug that can silently mismatch old/new resources.
fmt.Sprintf("%s-%s-%s", gvk.String(), name, namespace) concatenates name and namespace with a plain -, but Kubernetes names and namespaces can themselves contain hyphens. Two different resources can produce the identical key: e.g. name="baz-foo", namespace="bar" and name="baz", namespace="foo-bar" both yield ...-baz-foo-bar. Since duplicate keys "last one wins" (line 64-65), this could silently pair a resource with the wrong old object — either hiding a real transition-rule violation or falsely triggering one, with no error surfaced to the user.
Could we switch to a struct-based key so there's no encoding ambiguity at all?
🐛 Proposed fix using a struct key instead of string concatenation
- oldByKey := make(map[string]*unstructured.Unstructured, len(oldResources))
+ oldByKey := make(map[resourceKey]*unstructured.Unstructured, len(oldResources))
for _, o := range oldResources {
- oldByKey[resourceKey(o)] = o
+ oldByKey[keyOf(o)] = o
}
result := &ValidationResult{
Resources: make([]ResourceValidationResult, 0, len(resources)),
}
for _, r := range resources {
- result.Resources = append(result.Resources, validateResource(ctx, r, objectOf(oldByKey[resourceKey(r)]), schemaValidators, structurals, crds))
+ result.Resources = append(result.Resources, validateResource(ctx, r, objectOf(oldByKey[keyOf(r)]), schemaValidators, structurals, crds))
}
result.Summary = computeSummary(result.Resources)
return result, nil
}
-// resourceKey identifies a resource by GroupVersionKind, name, and namespace.
+// resourceKey identifies a resource by GroupVersionKind, name, and namespace.
// It is used to match a resource under validation to its previous state so CEL
// transition rules see the right old object.
-func resourceKey(r *unstructured.Unstructured) string {
- gvk := r.GetObjectKind().GroupVersionKind()
- return fmt.Sprintf("%s-%s-%s", gvk.String(), getResourceName(r), r.GetNamespace())
+type resourceKey struct {
+ gvk runtimeschema.GroupVersionKind
+ name string
+ namespace string
+}
+
+func keyOf(r *unstructured.Unstructured) resourceKey {
+ return resourceKey{
+ gvk: r.GetObjectKind().GroupVersionKind(),
+ name: getResourceName(r),
+ namespace: r.GetNamespace(),
+ }
}Note: if fmt isn't used elsewhere in the file after this change, the fmt import at line 21 should be dropped too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Index old resources by GVK+name+namespace so each resource can be paired | |
| // with its previous state. On duplicate keys the last entry wins. | |
| oldByKey := make(map[string]*unstructured.Unstructured, len(oldResources)) | |
| for _, o := range oldResources { | |
| oldByKey[resourceKey(o)] = o | |
| } | |
| result := &ValidationResult{ | |
| Resources: make([]ResourceValidationResult, 0, len(resources)), | |
| } | |
| for _, r := range resources { | |
| result.Resources = append(result.Resources, validateResource(ctx, r, schemaValidators, structurals, crds)) | |
| result.Resources = append(result.Resources, validateResource(ctx, r, objectOf(oldByKey[resourceKey(r)]), schemaValidators, structurals, crds)) | |
| } | |
| result.Summary = computeSummary(result.Resources) | |
| return result, nil | |
| } | |
| // resourceKey identifies a resource by GroupVersionKind, name, and namespace. | |
| // It is used to match a resource under validation to its previous state so CEL | |
| // transition rules see the right old object. | |
| func resourceKey(r *unstructured.Unstructured) string { | |
| gvk := r.GetObjectKind().GroupVersionKind() | |
| return fmt.Sprintf("%s-%s-%s", gvk.String(), getResourceName(r), r.GetNamespace()) | |
| } | |
| // objectOf returns the unstructured content of u, or nil when u is nil. It | |
| // keeps the nil check for an unmatched old resource in one place so callers can | |
| // pass the result straight to the CEL validator's oldObject argument. | |
| func objectOf(u *unstructured.Unstructured) map[string]any { | |
| if u == nil { | |
| return nil | |
| } | |
| return u.Object | |
| } | |
| // Index old resources by GVK+name+namespace so each resource can be paired | |
| // with its previous state. On duplicate keys the last entry wins. | |
| oldByKey := make(map[resourceKey]*unstructured.Unstructured, len(oldResources)) | |
| for _, o := range oldResources { | |
| oldByKey[keyOf(o)] = o | |
| } | |
| result := &ValidationResult{ | |
| Resources: make([]ResourceValidationResult, 0, len(resources)), | |
| } | |
| for _, r := range resources { | |
| result.Resources = append(result.Resources, validateResource(ctx, r, objectOf(oldByKey[keyOf(r)]), schemaValidators, structurals, crds)) | |
| } | |
| result.Summary = computeSummary(result.Resources) | |
| return result, nil | |
| } | |
| // resourceKey identifies a resource by GroupVersionKind, name, and namespace. | |
| // It is used to match a resource under validation to its previous state so CEL | |
| // transition rules see the right old object. | |
| type resourceKey struct { | |
| gvk runtimeschema.GroupVersionKind | |
| name string | |
| namespace string | |
| } | |
| func keyOf(r *unstructured.Unstructured) resourceKey { | |
| return resourceKey{ | |
| gvk: r.GetObjectKind().GroupVersionKind(), | |
| name: getResourceName(r), | |
| namespace: r.GetNamespace(), | |
| } | |
| } | |
| // objectOf returns the unstructured content of u, or nil when u is nil. It | |
| // keeps the nil check for an unmatched old resource in one place so callers can | |
| // pass the result straight to the CEL validator's oldObject argument. | |
| func objectOf(u *unstructured.Unstructured) map[string]any { | |
| if u == nil { | |
| return nil | |
| } | |
| return u.Object | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/validate/validate.go` around lines 63 - 97, `resourceKey` is vulnerable
to collisions because it concatenates GVK, name, and namespace into a single
string with hyphens, so distinct resources can map to the same key and mispair
old/new objects in `Validate`; replace the string-based keying in `Validate` and
`resourceKey` with a struct or other unambiguous composite key built from
`GroupVersionKind`, `getResourceName`, and `GetNamespace`, and update the
`oldByKey` map accordingly so duplicate handling cannot silently match the wrong
resource. If `fmt` is only used by `resourceKey`, remove that import after the
refactor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Fixes crossplane/crossplane#7532
To run e2e tests:
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.