Skip to content

feat: Add support for old resources in validate #166

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.

Sign up for GitHub

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

Open
cychiang wants to merge 2 commits into
base: main
Choose a base branch
from
+248 −7
Open

feat: Add support for old resources in validate #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
505dabb
Add --old-resources flag to validate for CEL transition rules
cychiang Jun 28, 2026
642a44a
Add testdata for e2e testing
cychiang Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion cmd/crossplane/validate/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"github.com/alecthomas/kong"
"github.com/spf13/afero"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

"github.com/crossplane/crossplane-runtime/v2/pkg/errors"
"github.com/crossplane/crossplane-runtime/v2/pkg/logging"
Expand Down Expand Up @@ -80,6 +81,7 @@ type Cmd struct {
CleanCache bool `help:"Clean the cache directory before downloading package schemas."`
CrossplaneImage string `default:"xpkg.crossplane.io/crossplane/crossplane:stable" help:"Specify the Crossplane image for validating built-in schemas."`
ErrorOnMissingSchemas bool `default:"false" help:"Return non zero exit code if missing schemas."`
OldResources string `help:"Previous resource state for evaluating CEL transition rules."`
// rendererFlag.Decode rejects unknown formats, which is what Kong's
// "enum" tag would normally enforce — but enum doesn't apply to
// MapperValue-backed fields. The help text is the user-facing list
Expand Down Expand Up @@ -132,6 +134,21 @@ func (c *Cmd) Run(k *kong.Context, _ logging.Logger) error {
return errors.Wrapf(err, "cannot load resources from %q", c.Resources)
}

// Load old resources, if provided, so CEL transition rules can compare the
// resources under validation against their previous state.
var oldResources []*unstructured.Unstructured
if c.OldResources != "" {
oldResourceLoader, err := load.NewLoader(c.OldResources)
if err != nil {
return errors.Wrapf(err, "cannot load old resources from %q", c.OldResources)
}

oldResources, err = oldResourceLoader.Load()
if err != nil {
return errors.Wrapf(err, "cannot load old resources from %q", c.OldResources)
}
}

if strings.HasPrefix(c.CacheDir, "~/") {
homeDir, _ := os.UserHomeDir()
c.CacheDir = filepath.Join(homeDir, c.CacheDir[2:])
Expand All @@ -151,7 +168,7 @@ func (c *Cmd) Run(k *kong.Context, _ logging.Logger) error {

// Validate resources against schemas, render in the requested format,
// and return a CLI-shaped error when validation didn't pass.
result, err := pkgvalidate.SchemaValidate(context.Background(), resources, m.crds)
result, err := pkgvalidate.SchemaValidate(context.Background(), resources, oldResources, m.crds)
if err != nil {
return errors.Wrapf(err, "cannot validate resources")
}
Expand Down
30 changes: 30 additions & 0 deletions cmd/crossplane/validate/help/validate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,29 @@ spec:
# message: "replicas should be in between minReplicas and maxReplicas."
```

### Validate CEL transition rules against previous state

Some CEL rules are *transition rules*: they reference `oldSelf` to compare a
resource against its previous state, for example to enforce that a field is
immutable:

```yaml
# spec.x-kubernetes-validations:
# - rule: "self.param == oldSelf.param"
# message: "param is immutable"
```

These rules only fire when a previous state is available. Supply it with
`--old-resources`, which accepts the same comma-separated list of files or
directories as the resource argument. Old resources are matched to the
resources under validation by API version, kind, name, and namespace. A
resource with no matching old state is validated as a create, so its transition
rules are skipped:

```shell
crossplane resource validate extensionsDir/ resourceDir/ --old-resources oldResourceDir/
```

## Validate against a directory of schemas

`validate` can also take a directory of schema YAML files to use for
Expand Down Expand Up @@ -156,3 +179,10 @@ Use a custom cache directory and clean it before downloading schemas:
```shell
crossplane resource validate extensionsDir/ resourceDir/ --cache-dir .cache --clean-cache
```

Validate resources, using a directory of previous states so CEL rules that
reference `oldSelf` (such as immutability constraints) can be evaluated:

```shell
crossplane resource validate extensionsDir/ resourceDir/ --old-resources oldResourceDir/
```
30 changes: 30 additions & 0 deletions cmd/crossplane/validate/testdata/cmd/crd_transition.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: immutables.cmd.example.org
spec:
group: cmd.example.org
names:
kind: Immutable
listKind: ImmutableList
plural: immutables
singular: immutable
scope: Cluster
versions:
- name: v1alpha1
served: true
storage: true
schema:
openAPIV3Schema:
type: object
properties:
spec:
type: object
required:
- param
properties:
param:
type: string
x-kubernetes-validations:
- rule: "self.param == oldSelf.param"
message: "param is immutable"
6 changes: 6 additions & 0 deletions cmd/crossplane/validate/testdata/cmd/resources_transition_new.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: cmd.example.org/v1alpha1
kind: Immutable
metadata:
name: immutable-instance
spec:
param: changed-value
6 changes: 6 additions & 0 deletions cmd/crossplane/validate/testdata/cmd/resources_transition_old.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: cmd.example.org/v1alpha1
kind: Immutable
metadata:
name: immutable-instance
spec:
param: original-value
34 changes: 34 additions & 0 deletions cmd/crossplane/validate/validate_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,40 @@ func TestRun(t *testing.T) {
}
},
},
"OldResourcesTransitionViolationExitsNonZero": {
reason: "With --old-resources supplying the previous state, a CEL transition rule (immutable field changed) fires: the resource is Invalid with a CEL error and the command exits non-zero.",
extensions: "testdata/cmd/crd_transition.yaml",
resources: "testdata/cmd/resources_transition_new.yaml",
extraArgs: []string{"--output=json", "--old-resources=testdata/cmd/resources_transition_old.yaml"},
wantErr: true,
assertJSON: func(t *testing.T, r *pkgvalidate.ValidationResult) {
t.Helper()
if r.Summary.Invalid != 1 {
t.Errorf("Summary.Invalid = %d; want 1", r.Summary.Invalid)
}
if len(r.Resources) != 1 || r.Resources[0].Status != pkgvalidate.ValidationStatusInvalid {
t.Errorf("Resources = %+v; want one Invalid entry", r.Resources)
}
if len(r.Resources[0].Errors) == 0 || r.Resources[0].Errors[0].Type != pkgvalidate.FieldErrorTypeCEL {
t.Errorf("Resources[0].Errors = %+v; want a CEL error", r.Resources[0].Errors)
}
},
},
"OldResourcesTransitionSkippedWithoutFlag": {
reason: "Without --old-resources the same resource is Valid: the transition rule references oldSelf and is skipped, exactly as on a create.",
extensions: "testdata/cmd/crd_transition.yaml",
resources: "testdata/cmd/resources_transition_new.yaml",
extraArgs: []string{"--output=json"},
assertJSON: func(t *testing.T, r *pkgvalidate.ValidationResult) {
t.Helper()
if r.Summary.Total != 1 || r.Summary.Valid != 1 {
t.Errorf("Summary = %+v; want Total=1 Valid=1", r.Summary)
}
if len(r.Resources) != 1 || r.Resources[0].Status != pkgvalidate.ValidationStatusValid {
t.Errorf("Resources = %+v; want one Valid entry", r.Resources)
}
},
},
}

for name, tc := range cases {
Expand Down
44 changes: 41 additions & 3 deletions pkg/validate/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package validate

import (
"context"
"fmt"

ext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Expand Down Expand Up @@ -45,22 +46,55 @@ import (
// Caller-owned resources are not mutated. SchemaValidate operates on a deep
// copy of each input, so the structural defaulting and unknown-field pruning
// it performs internally are not visible after the call returns.
func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured, crds []*extv1.CustomResourceDefinition) (*ValidationResult, error) {
//
// oldResources holds the previous state of the resources under validation so
// that CEL transition rules — those referencing oldSelf, such as immutability
// constraints — can be evaluated. They are matched to resources by
// GroupVersionKind, name, and namespace; a resource with no matching old state
// is validated with a nil old object, so its transition rules are skipped
// exactly as they are on a Kubernetes create. Pass nil when there is no
// previous state.
func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured, oldResources []*unstructured.Unstructured, crds []*extv1.CustomResourceDefinition) (*ValidationResult, error) {
schemaValidators, structurals, err := newValidatorsAndStructurals(crds)
if err != nil {
return nil, errors.Wrap(err, "cannot create schema validators")
}

// 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
}

Comment on lines +63 to +97

@coderabbitai coderabbitai Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

// validateResource runs every check (schema, CEL, unknown fields, defaulting)
// against a single resource and returns its ResourceValidationResult. It is
// the per-resource decomposition of SchemaValidate; pulling it out keeps the
Expand All @@ -72,6 +106,7 @@ func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured,
func validateResource(
ctx context.Context,
in *unstructured.Unstructured,
oldObject map[string]any,
schemaValidators map[runtimeschema.GroupVersionKind]*validation.SchemaValidator,
structurals map[runtimeschema.GroupVersionKind]*schema.Structural,
crds []*extv1.CustomResourceDefinition,
Expand Down Expand Up @@ -117,8 +152,11 @@ func validateResource(
rvr.Errors = append(rvr.Errors, fieldErrorToFieldValidationError(e, FieldErrorTypeUnknownField))
}

// oldObject feeds CEL transition rules (those referencing oldSelf). It is
// nil when no previous state was supplied for this resource, in which case
// the CEL validator skips transition rules — matching a Kubernetes create.
celValidator := cel.NewValidator(s, true, celconfig.PerCallLimit)
celErrs, _ := celValidator.Validate(ctx, nil, s, r.Object, nil, celconfig.PerCallLimit)
celErrs, _ := celValidator.Validate(ctx, nil, s, r.Object, oldObject, celconfig.PerCallLimit)
for _, e := range celErrs {
rvr.Errors = append(rvr.Errors, fieldErrorToFieldValidationError(e, FieldErrorTypeCEL))
}
Expand Down
86 changes: 83 additions & 3 deletions pkg/validate/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,45 @@ var testCRDWithCEL = &extv1.CustomResourceDefinition{
},
}

// testCRDWithTransition has a CEL transition rule: spec.param is immutable.
// The rule references oldSelf, so it only fires when a previous state (an old
// resource) is supplied; on a create (nil old object) it is skipped.
var testCRDWithTransition = &extv1.CustomResourceDefinition{
TypeMeta: metav1.TypeMeta{
APIVersion: "apiextensions.k8s.io/v1",
Kind: "CustomResourceDefinition",
},
ObjectMeta: metav1.ObjectMeta{Name: "test-transition"},
Spec: extv1.CustomResourceDefinitionSpec{
Group: "test.org",
Names: extv1.CustomResourceDefinitionNames{
Kind: "TestTransition", ListKind: "TestTransitionList", Plural: "testtransitions", Singular: "testtransition",
},
Scope: "Cluster",
Versions: []extv1.CustomResourceDefinitionVersion{{
Name: "v1alpha1", Served: true, Storage: true,
Schema: &extv1.CustomResourceValidation{
OpenAPIV3Schema: &extv1.JSONSchemaProps{
Type: "object",
Properties: map[string]extv1.JSONSchemaProps{
"spec": {
Type: "object",
XValidations: extv1.ValidationRules{{
Rule: "self.param == oldSelf.param",
Message: "param is immutable",
}},
Properties: map[string]extv1.JSONSchemaProps{
"param": {Type: "string"},
},
Required: []string{"param"},
},
},
},
},
}},
},
}

// testCRDNoMatchingVersion is a CRD that shares group+kind with testCRD but
// only declares v1beta1. When used BEFORE testCRD in the crds slice,
// applyDefaults matches it first and fails because v1alpha1 is missing.
Expand Down Expand Up @@ -181,10 +220,23 @@ func TestSchemaValidate(t *testing.T) {
"metadata": map[string]any{"name": "def-fail"},
"spec": map[string]any{"replicas": int64(1)},
}}
transitionResource := &unstructured.Unstructured{Object: map[string]any{
"apiVersion": "test.org/v1alpha1",
"kind": "TestTransition",
"metadata": map[string]any{"name": "test"},
"spec": map[string]any{"param": "changed-value"},
}}
transitionOldResource := &unstructured.Unstructured{Object: map[string]any{
"apiVersion": "test.org/v1alpha1",
"kind": "TestTransition",
"metadata": map[string]any{"name": "test"},
"spec": map[string]any{"param": "original-value"},
}}

type args struct {
resources []*unstructured.Unstructured
crds []*extv1.CustomResourceDefinition
resources []*unstructured.Unstructured
oldResources []*unstructured.Unstructured
crds []*extv1.CustomResourceDefinition
}
// expect declares everything we assert about a single resource's result:
// its Status and the exact set of FieldValidationErrors. Message and
Expand Down Expand Up @@ -325,6 +377,34 @@ func TestSchemaValidate(t *testing.T) {
perRes: nil,
},
},
"TransitionRuleViolatedWithOldResource": {
reason: "When a matching old resource is supplied, an immutability CEL transition rule (self.param == oldSelf.param) fires and the resource is Invalid with a cel-type error.",
args: args{
resources: []*unstructured.Unstructured{transitionResource},
oldResources: []*unstructured.Unstructured{transitionOldResource},
crds: []*extv1.CustomResourceDefinition{testCRDWithTransition},
},
want: want{
summary: ValidationSummary{Total: 1, Invalid: 1},
perRes: []expect{{
status: ValidationStatusInvalid,
errors: []FieldValidationError{
{Type: FieldErrorTypeCEL, Field: "spec"},
},
}},
},
},
"TransitionRuleSkippedWithoutOldResource": {
reason: "Without an old resource, the transition rule references oldSelf and is skipped (as on a create), so the same resource is Valid. Confirms the no-previous-state path leaves behaviour unchanged.",
args: args{
resources: []*unstructured.Unstructured{transitionResource},
crds: []*extv1.CustomResourceDefinition{testCRDWithTransition},
},
want: want{
summary: ValidationSummary{Total: 1, Valid: 1},
perRes: []expect{{status: ValidationStatusValid}},
},
},
"MixedOrder": {
reason: "Resources are returned in input order with their respective statuses.",
args: args{
Expand Down Expand Up @@ -353,7 +433,7 @@ func TestSchemaValidate(t *testing.T) {

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
result, err := SchemaValidate(t.Context(), tc.args.resources, tc.args.crds)
result, err := SchemaValidate(t.Context(), tc.args.resources, tc.args.oldResources, tc.args.crds)
if (err != nil) != tc.want.wantErr {
t.Fatalf("%s\nSchemaValidate() err = %v, wantErr = %v", tc.reason, err, tc.want.wantErr)
}
Expand Down