Repository: crossplane/crossplane
Pull request #5791 (2024-06-19 13:55:34)

Add unknown field check to `beta validate`
<!--
Thank you for helping to improve Crossplane! Please read the contribution docs
(linked below) if this is your first Crossplane pull request.
-->

### Description of your changes

<!--
Briefly describe what this pull request does, and how it is covered by tests.
Be proactive - direct your reviewers' attention to anything that needs special
consideration.

We love pull requests that fix an open issue. If yours does, use the below line
to indicate which issue it fixes, for example "Fixes #500".
-->

This PR implements a validation logic that use [pruning](k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning) to get the unknown fields of a resource. With the help of this new validation step we can get informed about the typos and indentation errors.

Here are some examples of this new validation: 

1. XR validation: 


[xrd.yaml](https://github.com/jbw976/demo-upbound/blob/main/xfn/xrd.yaml)
xr.yaml: 
```yaml
apiVersion: example.crossplane.io/v1beta1
kind: UserAccess
metadata:
  name: example
spec:
  count: 2
  accesKey: true  # an unknown field
```
Output:
```bash
go run cmd/crank/main.go beta validate ./.work/xfn/ ./.work/xfn/xr.yaml
[x] schema validation error example.crossplane.io/v1beta1, Kind=UserAccess, example : spec.accesKey: Invalid value: "accesKey": unknown field: "accesKey"
Total 1 resources: 0 missing schemas, 0 success cases, 1 failure cases
crossplane: error: cannot validate resources: could not validate all resources
exit status 1
```

2. Piping `crossplane beta render` function with field errors at composition: 

Uses https://github.com/jbw976/demo-upbound/commit/38017fa7b5f8cbd1fd33e5540953087f4550a06f which has a typo, indentation error and unknown field
```bash
crossplane beta render .work/xfn/xr.yaml  .work/xfn/composition.yaml  .work/xfn/functions.yaml --include-full-xr | go run cmd/crank/main.go beta validate .work/xfn/ - 
package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-aws-iam:v1.6.0
[x] schema validation error example.crossplane.io/v1beta1, Kind=UserAccess, example : spec.accesKey: Invalid value: "accesKey": unknown field: "accesKey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-0 : spec.forProvider.userSelectorFoo: Invalid value: "userSelectorFoo": unknown field: "userSelectorFoo"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-0 : spec.pgpKey: Invalid value: "pgpKey": unknown field: "pgpKey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-0 : spec.writeConnectionSecretToRef.fookey: Invalid value: "fookey": unknown field: "fookey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-1 : spec.forProvider.userSelectorFoo: Invalid value: "userSelectorFoo": unknown field: "userSelectorFoo"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-1 : spec.pgpKey: Invalid value: "pgpKey": unknown field: "pgpKey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-1 : spec.writeConnectionSecretToRef.fookey: Invalid value: "fookey": unknown field: "fookey"
[✓] iam.aws.upbound.io/v1beta1, Kind=User, test-user-0 validated successfully
[✓] iam.aws.upbound.io/v1beta1, Kind=User, test-user-1 validated successfully
Total 5 resources: 0 missing schemas, 2 success cases, 3 failure cases
crossplane: error: cannot validate resources: could not validate all resources
exit status 1

```



Fixes: #5376 

I added the relevant changes made in this PR to docs: https://github.com/crossplane/docs/pull/790

I have: <!--You MUST either [x] check or [ ] ~strike through~ every item.-->

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `earthly +reviewable` to ensure this PR is ready for review.
- [x] Added or updated unit tests.
- [ ] ~Added or updated e2e tests.~
- [x] Linked a PR or a [docs tracking issue] to [document this change].
- [ ] ~Added `backport release-x.y` labels to auto-backport this PR.~

Need help with this checklist? See the [cheat sheet].

[contribution process]: https://github.com/crossplane/crossplane/tree/master/contributing
[docs tracking issue]: https://github.com/crossplane/docs/issues/new
[document this change]: https://docs.crossplane.io/contribute/contribute
[cheat sheet]: https://github.com/crossplane/crossplane/tree/master/contributing#checklist-cheat-sheet



Patch:
diff --git a/cmd/crank/beta/validate/unknown_fields.go b/cmd/crank/beta/validate/unknown_fields.go
new file mode 100644
index 000000000..4f918e033
--- /dev/null
+++ b/cmd/crank/beta/validate/unknown_fields.go
@@ -0,0 +1,42 @@
+/*
+Copyright 2024 The Crossplane Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validate
+
+import (
+	"fmt"
+	"strings"
+
+	"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
+	"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+// validateUnknownFields Validates the resource's unknown fields against the given schema and returns a list of errors.
+func validateUnknownFields(mr map[string]interface{}, sch *schema.Structural) field.ErrorList {
+	opts := schema.UnknownFieldPathOptions{
+		TrackUnknownFieldPaths: true, // to get the list of pruned unknown fields
+	}
+	errs := field.ErrorList{}
+
+	uf := pruning.PruneWithOptions(mr, sch, true, opts)
+	for _, f := range uf {
+		strPath := strings.Split(f, ".")
+		child := strPath[len(strPath)-1]
+		errs = append(errs, field.Invalid(field.NewPath(f), child, fmt.Sprintf("unknown field: \"%s\"", child)))
+	}
+	return errs
+}
diff --git a/cmd/crank/beta/validate/validate.go b/cmd/crank/beta/validate/validate.go
index 2716d1bf2..7da0f6509 100644
--- a/cmd/crank/beta/validate/validate.go
+++ b/cmd/crank/beta/validate/validate.go
@@ -28,6 +28,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/util/validation/field"
 	celconfig "k8s.io/apiserver/pkg/apis/cel"
 
 	"github.com/crossplane/crossplane-runtime/pkg/errors"
@@ -102,6 +103,7 @@ func SchemaValidation(resources []*unstructured.Unstructured, crds []*extv1.Cust
 	for i, r := range resources {
 		gvk := r.GetObjectKind().GroupVersionKind()
 		sv, ok := schemaValidators[gvk]
+		s := structurals[gvk] // if we have a schema validator, we should also have a structural
 		if !ok {
 			missingSchemas++
 			if _, err := fmt.Fprintf(w, "[!] could not find CRD/XRD for: %s\n", r.GroupVersionKind().String()); err != nil {
@@ -112,9 +114,10 @@ func SchemaValidation(resources []*unstructured.Unstructured, crds []*extv1.Cust
 		}
 
 		rf := 0
-
+		re := field.ErrorList{}
 		for _, v := range sv {
-			re := validation.ValidateCustomResource(nil, r, *v)
+			re = append(re, validation.ValidateCustomResource(nil, r, *v)...)
+			re = append(re, validateUnknownFields(r.UnstructuredContent(), s)...)
 			for _, e := range re {
 				rf++
 				if _, err := fmt.Fprintf(w, "[x] schema validation error %s, %s : %s\n", r.GroupVersionKind().String(), getResourceName(r), e.Error()); err != nil {
@@ -122,8 +125,6 @@ func SchemaValidation(resources []*unstructured.Unstructured, crds []*extv1.Cust
 				}
 			}
 
-			s := structurals[gvk] // if we have a schema validator, we should also have a structural
-
 			celValidator := cel.NewValidator(s, true, celconfig.PerCallLimit)
 			re, _ = celValidator.Validate(context.TODO(), nil, s, resources[i].Object, nil, celconfig.PerCallLimit)
 			for _, e := range re {


Test patch:
diff --git a/cmd/crank/beta/validate/validate_test.go b/cmd/crank/beta/validate/validate_test.go
index cb252df13..7f7ec9f47 100644
--- a/cmd/crank/beta/validate/validate_test.go
+++ b/cmd/crank/beta/validate/validate_test.go
@@ -22,8 +22,10 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/utils/ptr"
 
 	"github.com/crossplane/crossplane-runtime/pkg/errors"
@@ -1338,3 +1340,89 @@ func TestValidateResources(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateUnknownFields(t *testing.T) {
+	type args struct {
+		mr  map[string]interface{}
+		sch *schema.Structural
+	}
+	type want struct {
+		errs field.ErrorList
+	}
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"UnknownFieldPresent": {
+			reason: "Should detect unknown fields in the resource and return an error",
+			args: args{
+				mr: map[string]interface{}{
+					"apiVersion": "test.org/v1alpha1",
+					"kind":       "Test",
+					"metadata": map[string]interface{}{
+						"name": "test-instance",
+					},
+					"spec": map[string]interface{}{
+						"replicas":     3,
+						"unknownField": "should fail", // This field is not defined in the CRD schema
+					},
+				},
+				sch: &schema.Structural{
+					Properties: map[string]schema.Structural{
+						"spec": {
+							Properties: map[string]schema.Structural{
+								"replicas": {
+									Generic: schema.Generic{Type: "integer"},
+								},
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				errs: field.ErrorList{
+					field.Invalid(field.NewPath("spec.unknownField"), "unknownField", `unknown field: "unknownField"`),
+				},
+			},
+		},
+		"UnknownFieldNotPresent": {
+			reason: "Should not return an error when no unknown fields are present",
+			args: args{
+				mr: map[string]interface{}{
+					"apiVersion": "test.org/v1alpha1",
+					"kind":       "Test",
+					"metadata": map[string]interface{}{
+						"name": "test-instance",
+					},
+					"spec": map[string]interface{}{
+						"replicas": 3, // No unknown fields
+					},
+				},
+				sch: &schema.Structural{
+					Properties: map[string]schema.Structural{
+						"spec": {
+							Properties: map[string]schema.Structural{
+								"replicas": {
+									Generic: schema.Generic{Type: "integer"},
+								},
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				errs: field.ErrorList{},
+			},
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			errs := validateUnknownFields(tc.args.mr, tc.args.sch)
+			if diff := cmp.Diff(tc.want.errs, errs, test.EquateErrors()); diff != "" {
+				t.Errorf("%s\nvalidateUnknownFields(...): -want errs, +got errs:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}