diff --git a/cmd/crossplane/validate/cmd.go b/cmd/crossplane/validate/cmd.go index 5eefbfb2..c3ef50e5 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" @@ -76,10 +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."` + 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 @@ -106,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 @@ -132,6 +141,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 +175,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/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..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 @@ -232,6 +251,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 { diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index ec1be1f6..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" @@ -45,22 +46,66 @@ 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 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 { - result.Resources = append(result.Resources, validateResource(ctx, r, schemaValidators, structurals, crds)) + // 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 + if old, ok := oldByKey[keyOf(r)]; ok { + oldObject = old.Object + } + result.Resources = append(result.Resources, validateResource(ctx, r, oldObject, schemaValidators, structurals, crds)) } result.Summary = computeSummary(result.Resources) 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 @@ -72,6 +117,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 +163,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..e9d199bb 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,51 @@ 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"}, + }} + // 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 - 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 +405,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{ @@ -345,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 @@ -353,7 +488,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) }