From 94fa20ef08039b1bea28f593caa6a6f1eeb8e317 Mon Sep 17 00:00:00 2001 From: Chuan-Yen Chiang Date: Sun, 28 Jun 2026 14:14:43 +0200 Subject: [PATCH 1/4] Add --old-resources flag to validate for CEL transition rules 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 --- cmd/crossplane/validate/cmd.go | 19 +++++- cmd/crossplane/validate/help/validate.md | 30 +++++++++ pkg/validate/validate.go | 44 +++++++++++- pkg/validate/validate_test.go | 86 +++++++++++++++++++++++- 4 files changed, 172 insertions(+), 7 deletions(-) diff --git a/cmd/crossplane/validate/cmd.go b/cmd/crossplane/validate/cmd.go index 5eefbfb2..ca4b32d8 100644 --- a/cmd/crossplane/validate/cmd.go +++ b/cmd/crossplane/validate/cmd.go @@ -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" @@ -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 @@ -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:]) @@ -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") } diff --git a/cmd/crossplane/validate/help/validate.md b/cmd/crossplane/validate/help/validate.md index ba5f0076..af465d18 100644 --- a/cmd/crossplane/validate/help/validate.md +++ b/cmd/crossplane/validate/help/validate.md @@ -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 @@ -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/ +``` diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index ec1be1f6..39e8e77b 100644 --- a/pkg/validate/validate.go +++ b/pkg/validate/validate.go @@ -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" @@ -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 +} + // 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 @@ -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, @@ -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)) } diff --git a/pkg/validate/validate_test.go b/pkg/validate/validate_test.go index 6b604ac5..809f4b8b 100644 --- a/pkg/validate/validate_test.go +++ b/pkg/validate/validate_test.go @@ -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. @@ -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 @@ -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{ @@ -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) } From a87191fab23e16c6cabf96fa1050ef42d04248a7 Mon Sep 17 00:00:00 2001 From: Chuan-Yen Chiang Date: Thu, 2 Jul 2026 01:47:45 +0200 Subject: [PATCH 2/4] Add testdata for e2e testing Signed-off-by: Chuan-Yen Chiang --- .../validate/testdata/cmd/crd_transition.yaml | 30 ++++++++++++++++ .../cmd/resources_transition_new.yaml | 6 ++++ .../cmd/resources_transition_old.yaml | 6 ++++ .../validate/validate_integration_test.go | 34 +++++++++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 cmd/crossplane/validate/testdata/cmd/crd_transition.yaml create mode 100644 cmd/crossplane/validate/testdata/cmd/resources_transition_new.yaml create mode 100644 cmd/crossplane/validate/testdata/cmd/resources_transition_old.yaml diff --git a/cmd/crossplane/validate/testdata/cmd/crd_transition.yaml b/cmd/crossplane/validate/testdata/cmd/crd_transition.yaml new file mode 100644 index 00000000..d9d03b7e --- /dev/null +++ b/cmd/crossplane/validate/testdata/cmd/crd_transition.yaml @@ -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" diff --git a/cmd/crossplane/validate/testdata/cmd/resources_transition_new.yaml b/cmd/crossplane/validate/testdata/cmd/resources_transition_new.yaml new file mode 100644 index 00000000..71c372bd --- /dev/null +++ b/cmd/crossplane/validate/testdata/cmd/resources_transition_new.yaml @@ -0,0 +1,6 @@ +apiVersion: cmd.example.org/v1alpha1 +kind: Immutable +metadata: + name: immutable-instance +spec: + param: changed-value diff --git a/cmd/crossplane/validate/testdata/cmd/resources_transition_old.yaml b/cmd/crossplane/validate/testdata/cmd/resources_transition_old.yaml new file mode 100644 index 00000000..7302de47 --- /dev/null +++ b/cmd/crossplane/validate/testdata/cmd/resources_transition_old.yaml @@ -0,0 +1,6 @@ +apiVersion: cmd.example.org/v1alpha1 +kind: Immutable +metadata: + name: immutable-instance +spec: + param: original-value diff --git a/cmd/crossplane/validate/validate_integration_test.go b/cmd/crossplane/validate/validate_integration_test.go index be44b313..3002f52a 100644 --- a/cmd/crossplane/validate/validate_integration_test.go +++ b/cmd/crossplane/validate/validate_integration_test.go @@ -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 { From b98a06e74c8758534330ba3e416363e436a76faf Mon Sep 17 00:00:00 2001 From: Chuan-Yen Chiang Date: Fri, 3 Jul 2026 01:49:11 +0200 Subject: [PATCH 3/4] refactor based on feedbacks Signed-off-by: Chuan-Yen Chiang --- pkg/validate/validate.go | 41 ++++++++++------------------ pkg/validate/validate_test.go | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 27 deletions(-) diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index 39e8e77b..c850d936 100644 --- a/pkg/validate/validate.go +++ b/pkg/validate/validate.go @@ -18,7 +18,6 @@ package validate import ( "context" - "fmt" ext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -60,41 +59,29 @@ func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured, 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, objectOf(oldByKey[resourceKey(r)]), schemaValidators, structurals, crds)) + // Find this resource's previous state, if supplied, so CEL transition + // rules (those referencing oldSelf) can be evaluated. A resource is + // matched to its old state by GroupVersionKind, namespace, and name; + // with no match oldObject stays nil and transition rules are skipped, + // exactly as on a Kubernetes create. + gvk, namespace, name := r.GroupVersionKind(), r.GetNamespace(), getResourceName(r) + var oldObject map[string]any + for _, o := range oldResources { + if o.GroupVersionKind() == gvk && o.GetNamespace() == namespace && getResourceName(o) == name { + oldObject = o.Object + break + } + } + result.Resources = append(result.Resources, validateResource(ctx, r, oldObject, 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 -} - // 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 diff --git a/pkg/validate/validate_test.go b/pkg/validate/validate_test.go index 809f4b8b..ce249383 100644 --- a/pkg/validate/validate_test.go +++ b/pkg/validate/validate_test.go @@ -180,6 +180,56 @@ var testCRDNoMatchingVersion = &extv1.CustomResourceDefinition{ }, } +// TestSchemaValidateDistinguishesAmbiguousKeys is a regression test for +// old-resource matching. A naive "--" string key collides +// whenever a name or namespace contains the "-" separator: (name=app, +// namespace=test-1) and (name=app-test, namespace=1) both render as +// "-app-test-1". Under such a collision a resource gets matched to the +// WRONG previous state and its CEL transition rule is evaluated against the +// wrong old object. SchemaValidate matches on GroupVersionKind, namespace, and +// name as separate values, which keeps these two resources distinct. +func TestSchemaValidateDistinguishesAmbiguousKeys(t *testing.T) { + mk := func(name, namespace, param string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "test.org/v1alpha1", + "kind": "TestTransition", + "metadata": map[string]any{"name": name, "namespace": namespace}, + "spec": map[string]any{"param": param}, + }} + } + + // newA and newB collide to "-app-test-1" under a naive string key, as + // do their matching old resources. + newA := mk("app", "test-1", "keep") // unchanged vs oldA -> should be Valid + newB := mk("app-test", "1", "changed") // changed vs oldB -> should be Invalid + oldA := mk("app", "test-1", "keep") + oldB := mk("app-test", "1", "original") + + result, err := SchemaValidate( + t.Context(), + []*unstructured.Unstructured{newA, newB}, + []*unstructured.Unstructured{oldA, oldB}, + []*extv1.CustomResourceDefinition{testCRDWithTransition}, + ) + if err != nil { + t.Fatalf("SchemaValidate() unexpected error: %v", err) + } + + // Matching on separate fields, each resource sees its own old state. A naive + // string key would map both to the last old resource (oldB), wrongly + // flipping newA to Invalid and yielding Valid=0, Invalid=2. + want := ValidationSummary{Total: 2, Valid: 1, Invalid: 1} + if diff := cmp.Diff(want, result.Summary); diff != "" { + t.Errorf("Summary mismatch (-want +got):\n%s", diff) + } + if result.Resources[0].Status != ValidationStatusValid { + t.Errorf("newA (app/test-1) Status = %q; want Valid — matched to the wrong old resource?", result.Resources[0].Status) + } + if result.Resources[1].Status != ValidationStatusInvalid { + t.Errorf("newB (app-test/1) Status = %q; want Invalid", result.Resources[1].Status) + } +} + func TestSchemaValidate(t *testing.T) { validResource := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "test.org/v1alpha1", From 86fd4670b2315f668e226422f3775dc789d33e93 Mon Sep 17 00:00:00 2001 From: Chuan-Yen Chiang Date: Sat, 4 Jul 2026 00:27:03 +0200 Subject: [PATCH 4/4] Some improvements based on the feedback Signed-off-by: Chuan-Yen Chiang --- cmd/crossplane/validate/cmd.go | 19 +++- .../validate/validate_integration_test.go | 19 ++++ pkg/validate/validate.go | 46 ++++++-- pkg/validate/validate_test.go | 105 +++++++++--------- 4 files changed, 122 insertions(+), 67 deletions(-) diff --git a/cmd/crossplane/validate/cmd.go b/cmd/crossplane/validate/cmd.go index ca4b32d8..c3ef50e5 100644 --- a/cmd/crossplane/validate/cmd.go +++ b/cmd/crossplane/validate/cmd.go @@ -77,11 +77,11 @@ type Cmd struct { Resources string `arg:"" help:"Resource sources as a comma-separated list of files, directories, or '-' for standard input."` // Flags. Keep them in alphabetical order. - CacheDir string `default:"~/.crossplane/cache" help:"Absolute path to the cache directory for downloaded schemas." predictor:"directory"` + CacheDir string `default:"~/.crossplane/cache" help:"Absolute path to the cache directory for downloaded schemas." predictor:"directory"` 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."` + 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 CEL transition rules, as a comma-separated list of files, directories, or '-' for standard input."` // 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 @@ -108,8 +108,15 @@ func (c *Cmd) AfterApply() error { // Run validate. func (c *Cmd) Run(k *kong.Context, _ logging.Logger) error { - if c.Resources == "-" && c.Extensions == "-" { - return errors.New("cannot use stdin for both extensions and resources") + // stdin can only be consumed once, so at most one input may be "-". + stdinCount := 0 + for _, in := range []string{c.Extensions, c.Resources, c.OldResources} { + if in == "-" { + stdinCount++ + } + } + if stdinCount > 1 { + return errors.New("cannot use stdin for more than one input") } // Load all extensions diff --git a/cmd/crossplane/validate/validate_integration_test.go b/cmd/crossplane/validate/validate_integration_test.go index 3002f52a..d187a0df 100644 --- a/cmd/crossplane/validate/validate_integration_test.go +++ b/cmd/crossplane/validate/validate_integration_test.go @@ -85,6 +85,25 @@ func TestParseRejectsUnknownOutputFormat(t *testing.T) { } } +// TestRunRejectsMultipleStdinInputs asserts the guard in Run: stdin can only be +// consumed once, so at most one of extensions, resources, or --old-resources may +// be "-". The guard runs before any loader, so the file paths need not exist. +func TestRunRejectsMultipleStdinInputs(t *testing.T) { + cases := map[string][]string{ + "ExtensionsAndResources": {"-", "-"}, + "ResourcesAndOldResources": {"extensions.yaml", "-", "--old-resources=-"}, + "ExtensionsAndOldResources": {"-", "resources.yaml", "--old-resources=-"}, + } + for name, argv := range cases { + t.Run(name, func(t *testing.T) { + _, err := runCmd(t, append(argv, commonArgs...)...) + if err == nil || !strings.Contains(err.Error(), "stdin") { + t.Errorf("expected a stdin error, got: %v", err) + } + }) + } +} + // TestRun drives the validate command end-to-end through Kong, against // real fixture files and a pre-populated cache directory that keeps the // run offline. Nothing is mocked; the case table covers diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index c850d936..42b6d8eb 100644 --- a/pkg/validate/validate.go +++ b/pkg/validate/validate.go @@ -27,6 +27,7 @@ import ( "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" runtimeschema "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" celconfig "k8s.io/apiserver/pkg/apis/cel" @@ -59,22 +60,24 @@ func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured, return nil, errors.Wrap(err, "cannot create schema validators") } + // Index old resources by identity so each resource can be paired with its + // previous state in O(1). On duplicate keys the last entry wins. + oldByKey := make(map[oldResourceKey]*unstructured.Unstructured, len(oldResources)) + for _, o := range oldResources { + oldByKey[keyOf(o)] = o + } + result := &ValidationResult{ Resources: make([]ResourceValidationResult, 0, len(resources)), } for _, r := range resources { - // Find this resource's previous state, if supplied, so CEL transition - // rules (those referencing oldSelf) can be evaluated. A resource is - // matched to its old state by GroupVersionKind, namespace, and name; - // with no match oldObject stays nil and transition rules are skipped, - // exactly as on a Kubernetes create. - gvk, namespace, name := r.GroupVersionKind(), r.GetNamespace(), getResourceName(r) + // Look up this resource's previous state, if supplied, so CEL transition + // rules (those referencing oldSelf) can be evaluated. With no match + // oldObject stays nil and transition rules are skipped, exactly as on a + // Kubernetes create. var oldObject map[string]any - for _, o := range oldResources { - if o.GroupVersionKind() == gvk && o.GetNamespace() == namespace && getResourceName(o) == name { - oldObject = o.Object - break - } + if old, ok := oldByKey[keyOf(r)]; ok { + oldObject = old.Object } result.Resources = append(result.Resources, validateResource(ctx, r, oldObject, schemaValidators, structurals, crds)) } @@ -82,6 +85,27 @@ func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured, return result, nil } +// oldResourceKey identifies a resource for matching it to its previous state. It +// composes two native, comparable Kubernetes value types — a GroupVersionKind +// and a NamespacedName — so the key is unambiguous (unlike a joined string, +// where a name or namespace containing the separator could collide) and usable +// directly as a Go map key. +type oldResourceKey struct { + gvk runtimeschema.GroupVersionKind + name types.NamespacedName +} + +// keyOf derives the oldResourceKey for a resource. Name resolution goes through +// getResourceName so the match is consistent with how validateResource reports +// the resource name, including its composition-resource-name annotation +// fallback. +func keyOf(r *unstructured.Unstructured) oldResourceKey { + return oldResourceKey{ + gvk: r.GroupVersionKind(), + name: types.NamespacedName{Namespace: r.GetNamespace(), Name: getResourceName(r)}, + } +} + // 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 diff --git a/pkg/validate/validate_test.go b/pkg/validate/validate_test.go index ce249383..e9d199bb 100644 --- a/pkg/validate/validate_test.go +++ b/pkg/validate/validate_test.go @@ -180,56 +180,6 @@ var testCRDNoMatchingVersion = &extv1.CustomResourceDefinition{ }, } -// TestSchemaValidateDistinguishesAmbiguousKeys is a regression test for -// old-resource matching. A naive "--" string key collides -// whenever a name or namespace contains the "-" separator: (name=app, -// namespace=test-1) and (name=app-test, namespace=1) both render as -// "-app-test-1". Under such a collision a resource gets matched to the -// WRONG previous state and its CEL transition rule is evaluated against the -// wrong old object. SchemaValidate matches on GroupVersionKind, namespace, and -// name as separate values, which keeps these two resources distinct. -func TestSchemaValidateDistinguishesAmbiguousKeys(t *testing.T) { - mk := func(name, namespace, param string) *unstructured.Unstructured { - return &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "test.org/v1alpha1", - "kind": "TestTransition", - "metadata": map[string]any{"name": name, "namespace": namespace}, - "spec": map[string]any{"param": param}, - }} - } - - // newA and newB collide to "-app-test-1" under a naive string key, as - // do their matching old resources. - newA := mk("app", "test-1", "keep") // unchanged vs oldA -> should be Valid - newB := mk("app-test", "1", "changed") // changed vs oldB -> should be Invalid - oldA := mk("app", "test-1", "keep") - oldB := mk("app-test", "1", "original") - - result, err := SchemaValidate( - t.Context(), - []*unstructured.Unstructured{newA, newB}, - []*unstructured.Unstructured{oldA, oldB}, - []*extv1.CustomResourceDefinition{testCRDWithTransition}, - ) - if err != nil { - t.Fatalf("SchemaValidate() unexpected error: %v", err) - } - - // Matching on separate fields, each resource sees its own old state. A naive - // string key would map both to the last old resource (oldB), wrongly - // flipping newA to Invalid and yielding Valid=0, Invalid=2. - want := ValidationSummary{Total: 2, Valid: 1, Invalid: 1} - if diff := cmp.Diff(want, result.Summary); diff != "" { - t.Errorf("Summary mismatch (-want +got):\n%s", diff) - } - if result.Resources[0].Status != ValidationStatusValid { - t.Errorf("newA (app/test-1) Status = %q; want Valid — matched to the wrong old resource?", result.Resources[0].Status) - } - if result.Resources[1].Status != ValidationStatusInvalid { - t.Errorf("newB (app-test/1) Status = %q; want Invalid", result.Resources[1].Status) - } -} - func TestSchemaValidate(t *testing.T) { validResource := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "test.org/v1alpha1", @@ -282,6 +232,34 @@ func TestSchemaValidate(t *testing.T) { "metadata": map[string]any{"name": "test"}, "spec": map[string]any{"param": "original-value"}, }} + // ambiguousA and ambiguousB collide under a naive "--" + // string key: (name=app, namespace=test-1) and (name=app-test, namespace=1) + // both render as "-app-test-1". Matching on separate fields keeps them + // distinct, so each is paired with its own old state below. + ambiguousA := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "test.org/v1alpha1", + "kind": "TestTransition", + "metadata": map[string]any{"name": "app", "namespace": "test-1"}, + "spec": map[string]any{"param": "keep"}, + }} + ambiguousB := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "test.org/v1alpha1", + "kind": "TestTransition", + "metadata": map[string]any{"name": "app-test", "namespace": "1"}, + "spec": map[string]any{"param": "changed"}, + }} + ambiguousOldA := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "test.org/v1alpha1", + "kind": "TestTransition", + "metadata": map[string]any{"name": "app", "namespace": "test-1"}, + "spec": map[string]any{"param": "keep"}, + }} + ambiguousOldB := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "test.org/v1alpha1", + "kind": "TestTransition", + "metadata": map[string]any{"name": "app-test", "namespace": "1"}, + "spec": map[string]any{"param": "original"}, + }} type args struct { resources []*unstructured.Unstructured @@ -475,6 +453,33 @@ func TestSchemaValidate(t *testing.T) { }, }, }, + "AmbiguousKeysMatchedByIdentity": { + // Regression guard: a naive "--" string key + // collides for these two resources ((app, test-1) and (app-test, 1) + // both render as "-app-test-1"), which would pair a resource + // with the wrong old state. Matching on GVK, namespace, and name as + // separate values keeps them distinct: ambiguousA is unchanged + // (Valid) while ambiguousB changed its immutable param (Invalid). A + // string key would instead yield Valid=0, Invalid=2. + reason: "Resources whose naive string keys collide are matched to their own old state by GVK+namespace+name.", + args: args{ + resources: []*unstructured.Unstructured{ambiguousA, ambiguousB}, + oldResources: []*unstructured.Unstructured{ambiguousOldA, ambiguousOldB}, + crds: []*extv1.CustomResourceDefinition{testCRDWithTransition}, + }, + want: want{ + summary: ValidationSummary{Total: 2, Valid: 1, Invalid: 1}, + perRes: []expect{ + {status: ValidationStatusValid}, + { + status: ValidationStatusInvalid, + errors: []FieldValidationError{ + {Type: FieldErrorTypeCEL, Field: "spec"}, + }, + }, + }, + }, + }, } // Message and Value text comes from k8s validator libraries and changes