Server Side Field Validation #2

kevindelgado · 2021-10-14T18:04:32Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Performs strict server side schema validation via the fieldValidation=[Strict,Warn,Ignore] query parameter.

This change is dependent on liggitt/json-stdlib and so this PR is opened on this fork in order to begin collecting feedback on it until liggitt/json-stdlib is merged into upstream k/k.

It introduces the fieldValidation query parameter that when set to Strict causes requests to error when unknown fields are present, when set to Warn succeeds the request but returns a warning, and when set to Ignore it ignores unknown fields.

For create/update requests, we use strict json unmarshalling to determine the unknown fields and error/warn based on those fields.

For JSON Patch requests (i.e. CRDs), we use the prune mechanism in the customresource handler to resolve any unknown fields.

For SMP Patch request, we use the the unstructured converter to resolve unknown fields.

Apply Patch requests already require strict schemas and will error on unknown fields.

Which issue(s) this PR fixes:

First step in solving kubernetes#39434 and kubernetes#5889

Does this PR introduce a user-facing change?

Performs strict server side schema validation requests via the `fieldValidation=[Strict,Warn,Ignore]` query parameter.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP-2885

[KEP-2885](https://github.com/kubernetes/enhancements/issues/2885)

kevindelgado · 2021-10-14T18:06:30Z

cc @liggitt @apelisse (I don't think I can add reviewers on a fork).

This is ready for review when y'all are free.

apelisse · 2021-10-20T20:36:16Z

pkg/features/kube_features.go

@@ -855,6 +855,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
 	genericfeatures.WarningHeaders:                      {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.24
 	genericfeatures.OpenAPIEnums:                        {Default: false, PreRelease: featuregate.Alpha},
 	genericfeatures.CustomResourceValidationExpressions: {Default: false, PreRelease: featuregate.Alpha},
+	genericfeatures.FieldValidation:                     {Default: false, PreRelease: featuregate.Alpha},


Not completely sure about the name yet, what about ... StrictValidation, or both StrictFieldsValidation?

apelisse · 2021-10-20T21:01:10Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+				return obj, gvk, err
+			}
 			return nil, gvk, err


I've never seen this type of return signature where one doesn't check for err != nil but obj != nil and I can see how it'd be confusing for people.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

apelisse · 2021-10-20T21:08:10Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -1321,10 +1327,12 @@ func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) error {
 	if err != nil {
 		return err
 	}
+
+	pruned := map[string]bool{}


You could possibly have used an empty struct here, struct{} rather than bool, though I don't know if it matters.

also prefer var pruned map[string]... which avoids an unused map allocation

apelisse · 2021-10-20T21:12:23Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+			pruned = structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version], false)
 			structuraldefaulting.PruneNonNullableNullsWithoutDefaults(u.Object, v.structuralSchemas[gv.Version])
 		}


So if we're not pruning, we can't fail on unknown fields?

that seems correct... if we're preserving fields not mentioned in the schema, we're effectively saying all field names are permissible

On the other hand, this mostly exists for compatibility reason, doesn't mean people necessarily wanted arbitrary fields? I think it's useful even as a warning for example.

apelisse · 2021-10-21T00:12:53Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

 			scope.err(err, w, req)
 			return
 		}

+		decoder := scope.Serializer.DecoderToVersion(s.Serializer, scope.HubGroupVersion)
+		trace.Step("About to convert to expected version")
+		obj, gvk, err := decoder.Decode(body, &defaultGVK, original)


That answers one of my question above. It doesn't look like ignore and warn are fundamentally different, do we need to keep ignore then? Or if we keep it, does it need to be the default?

apelisse · 2021-10-21T00:13:55Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

+				return
+			}
+			if validationDirective == runtime.WarnFieldValidation {
+				warning.AddWarning(req.Context(), "", err.Error())


I would have expected one warning per unknown field, this looks like it would build a very large warning

I've made an attempt at doing that now. Let me know if it's correct

apelisse · 2021-10-21T00:20:16Z

test/integration/apiserver/field_validation_test.go

+
+// smpTestSetup applies an object that will later be patched
+// in the actual test/benchmark.
+func smpTestSetup(t testing.TB, client clientset.Interface) {


I'm not a big fan of this function for tests in general

apelisse · 2021-10-21T00:20:42Z

test/integration/apiserver/field_validation_test.go

+// smpTestSetup applies an object that will later be patched
+// in the actual test/benchmark.
+func smpTestSetup(t testing.TB, client clientset.Interface) {
+	bodyBase, err := os.ReadFile("./testdata/deploy-small.json")


I'd also ideally avoid having files read from disk for tests, since it makes it harder to understand what the test does.

apelisse · 2021-10-21T00:22:58Z

test/integration/apiserver/field_validation_test.go

+	var testcases = []struct {
+		name         string
+		opts         metav1.UpdateOptions
+		errContains  string
+		warnContains string


For this test (and another), I don't know if I enjoy the value of table-driven testing here, as I feel this obfuscates what the test does for very limited code re-use.

apelisse · 2021-10-21T00:24:14Z

I don't have a strong opinion about any of my comments and this is super good and could probably be merged as is (except maybe we should answer to a few TODOs ;-). Thanks Kevin!

kevindelgado · 2021-10-22T00:31:14Z

I spent all of today focused on the SMP unstructured converter because that is where I felt the least comfortable and wanted to get some resolution on first.

In addressing your feedback @apelisse , I think I finally understood the "pre-flattening" algo we had discussed before that I was previously failing to grok. I've updated the converter to use this now which gets rid of the wonky "fields" field we had to add to the various method signatures that you had commented on (and clears up some of the other concerns you had).

On the performance side , the way I had implemented converter strict validation was about 300% slower and more memory intensive than what's currently in master "no validation" (and Ignore also suffered the penalty because it was doing the unknown fields check and just not returning the error). Now, I'm seeing roughly parity between strict validation and the old "no validation" (actually, strict appears to be slightly faster and less memory (which is weird and I need to look into it on monday)).

One thing I did not implement was the "full path to unknown field" that we discussed offline. I noticed that the json deserializer also does not indicate the full path, so I wasn't sure if we actually wanted to do that here in SMP. I think it's easy enough to add (although I think it will require changing the method signatures to store the path down successive recursive calls of fromUnstructured), but wanted to hold off until we talk to Jordan and agree we actually want to preserve the full path in the errors.

Anyways, I know this is a lot of text, so let's sync on Monday. If you want, feel free to take another look at converter.go, otherwise I will address the rest of your feedback on Monday.

liggitt · 2021-10-22T18:20:09Z

kubernetes#105030 is merged if you want to transition this to k/k before the next round of feedback

liggitt · 2021-10-22T19:09:12Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+	if len(pruned) > 0 && v.persistStrictDecodingErrors {
+		allStrictErrs := make([]error, len(pruned))
+		i := 0
+		for unknownField, _ := range pruned {


iterating over a map makes the order non-deterministic, which is not ideal

also see suggestion above about making this low-level function just return the list of unknown field paths and letting the caller turn that into warnings/messages

liggitt · 2021-10-22T19:09:53Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -1321,10 +1327,12 @@ func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) error {
 	if err != nil {
 		return err
 	}
+
+	pruned := map[string]bool{}


also prefer var pruned map[string]... which avoids an unused map allocation

liggitt · 2021-10-22T19:12:26Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+			pruned = structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version], false)
 			structuraldefaulting.PruneNonNullableNullsWithoutDefaults(u.Object, v.structuralSchemas[gv.Version])
 		}


that seems correct... if we're preserving fields not mentioned in the schema, we're effectively saying all field names are permissible

liggitt · 2021-10-22T19:13:04Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+	structuralSchemas           map[string]*structuralschema.Structural
+	structuralSchemaGK          schema.GroupKind
+	preserveUnknownFields       bool
+	persistStrictDecodingErrors bool


"persist" reads strangely... maybe returnUnknownFieldPaths?

liggitt · 2021-10-22T19:16:46Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

+	structuralSchemas           map[string]*structuralschema.Structural
+	structuralSchemaGK          schema.GroupKind
+	preserveUnknownFields       bool
+	persistStrictDecodingErrors bool
 }

 func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) error {


since this signature is private, would it be clearer to make the list of unknown fields be returned as a separate return value?

func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) (unknownFieldPaths []string, err error)

That would let the caller determine how to use the list of unknown field paths, would preserve order, and would distinguish between validation errors and unknown fields

liggitt · 2021-10-22T19:34:07Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

-		trace.Step("About to convert to expected version")
-		obj, gvk, err := decoder.Decode(body, &defaultGVK, original)
+
+		validationDirective, err := fieldValidation(req)


is there a reason not to do the validation in ValidateCreateOptions above (which presumably already extracted the desired directive from the req parameters), and use options.FieldValidation instead of a second local var? same question applies to all the places fieldValidation(req) is being used

Somehow I completely missed the Validate*Options. I've done that now

liggitt · 2021-10-22T19:37:06Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -424,7 +438,7 @@ type applyPatcher struct {
 	userAgent    string
 }

-func (p *applyPatcher) applyPatchToCurrentObject(obj runtime.Object) (runtime.Object, error) {
+func (p *applyPatcher) applyPatchToCurrentObject(_ context.Context, obj runtime.Object) (runtime.Object, error) {


is this a TODO? duplicate and unknown fields are detectable here, right?

This was not meant as a TODO, because SSA already defaults to strict validation on unknown fields. But I just realized that we had spoke about how SSA does not error on duplicate fields, so I’ve added a TODO to capture this.

liggitt · 2021-10-22T20:00:44Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+	if contentType != "" {
+		supported = false
+		for _, v := range strings.Split(contentType, ",") {
+			t, _, err := mime.ParseMediaType(v)


ParseMediaType has non-trivial cost and is already done in NegotiateInputSerializer for create/update, and is unused for patch (we require Content-Type to be exactly one of the supported patch types) ... I didn't expect to be doing this parsing a second time here

is there a reason we can't define a strict version of each deserializer and select that in create.go / update.go if FieldValidation is set to Warn or Strict?

removed this as part of the Validate*Options work

liggitt · 2021-10-22T20:01:33Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+		}
+	}
+
+	validationParam := req.URL.Query()["fieldValidation"]


pull this from create/update/patch options rather than re-extracting from the query

liggitt · 2021-10-22T20:01:47Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+	case 0:
+		return runtime.IgnoreFieldValidation, nil
+	case 1:
+		switch strings.ToLower(validationParam[0]) {


I'm not sure why we'd want to be case-insensitive here

Changed it to be case-sensitive. Wasn’t sure what the right way was based on this comment kubernetes#104433 (comment)

apelisse · 2021-10-26T17:11:20Z

Has this been re-opened in k/k? I wanted to do another pass.

kevindelgado

Addressed most of the feedback. Still a couple TODOs around test formatting and defaulting to warning, but wanted to get this up for another round of review ASAP.

Currently opening up a PR on k/k, will have it up in a few minutes.

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

kevindelgado · 2021-10-26T17:10:34Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go

+	// - Ignore: ignore's unknown/duplicate fields
+	// - Strict: fail the request on unknown/duplicate fields
+	// - Warn: respond with a warning, but successfully serve the request.


reordered the values. Waiting on another round of feedback before defaulting to warn to confirm that is what we want to do

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

kevindelgado · 2021-10-26T17:11:07Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm.go

@@ -23,7 +23,8 @@ import (
 // Prune removes object fields in obj which are not specified in s. It skips TypeMeta and ObjectMeta fields
 // if XEmbeddedResource is set to true, or for the root if isResourceRoot=true, i.e. it does not
 // prune unknown metadata fields.
-func Prune(obj interface{}, s *structuralschema.Structural, isResourceRoot bool) {
+// It returns the set of fields that it prunes.
+func Prune(obj interface{}, s *structuralschema.Structural, isResourceRoot bool) map[string]bool {


kevindelgado · 2021-10-26T17:18:56Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go

@@ -424,7 +438,7 @@ type applyPatcher struct {
 	userAgent    string
 }

-func (p *applyPatcher) applyPatchToCurrentObject(obj runtime.Object) (runtime.Object, error) {
+func (p *applyPatcher) applyPatchToCurrentObject(_ context.Context, obj runtime.Object) (runtime.Object, error) {


This was not meant as a TODO, because SSA already defaults to strict validation on unknown fields. But I just realized that we had spoke about how SSA does not error on duplicate fields, so I’ve added a TODO to capture this.

kevindelgado · 2021-10-26T17:19:29Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+		return runtime.IgnoreFieldValidation, nil
+	}
+
+	// TODO: Should we blocklist unsupportedContentTypes (just protobuf) rather than allowlisting everything that isn't protobuf?


removed as part of the Validation*Options work

kevindelgado · 2021-10-26T17:20:00Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+	if contentType != "" {
+		supported = false
+		for _, v := range strings.Split(contentType, ",") {
+			t, _, err := mime.ParseMediaType(v)


removed this as part of the Validate*Options work

kevindelgado · 2021-10-26T17:20:07Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+		}
+	}
+
+	validationParam := req.URL.Query()["fieldValidation"]


kevindelgado · 2021-10-26T17:20:20Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+	case 0:
+		return runtime.IgnoreFieldValidation, nil
+	case 1:
+		switch strings.ToLower(validationParam[0]) {


Changed it to be case-sensitive. Wasn’t sure what the right way was based on this comment kubernetes#104433 (comment)

This is the commit message for patch #2 (refresh-temp):

Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com> fix (#2) Signed-off-by: Monis Khan <mok@microsoft.com>

MVP cleanup #2

liggitt force-pushed the json-stdlib branch 5 times, most recently from fbe25a5 to 434ce43 Compare October 20, 2021 17:12

kevindelgado added 24 commits October 20, 2021 20:56

Strict schema validation for POST and PUT

e29d84f

Add benchmarking for strict validation

2591d7d

add benchmarking to integration tests

f424ec6

fieldValidation returns enum, error

9923290

WIP start addressing PR feedback

3cb499e

Address feedback, improve integration/bench tests

526e0cd

Strict Validation for CR jsonpatch

0564cca

determine strict serialization in the patch handler

0d8d87e

First pass at integration test

5b82235

cleanup test, move to apiserver_test.go

488bdf1

wip, stash logging

853193f

fix patch crd test params

3229e2a

benchmark working, test is broken

4194448

wip debugging

e5bb865

fixed it by getting rid of preserve unknown fieds in the test

8df8d69

structural pruning now returns the pruned fields

81f8e01

clean up debug logging in customresource_handler.go

9a2e946

benchmark remaining merge-patch possibilities

256d442

wip, stash logging

b3e58bd

wip more pritning

32d4bc4

wip save pre debug cleanup

802ec6d

WIP factor deleteFromKeys, stash pre help chat

9da68d7

IT WORKS

e19c591

wip, hacky test working pre cleanup

d45a665

kevindelgado added 13 commits October 20, 2021 20:56

wip, checkpoint patch crd cleanup

8c67e1e

cleanup crd-patch

98e0901

start smp cleanup

cc03962

move fieldValidationDirective enum to runtime pkg

35399e4

remove StrictSerializer from scope

73eb90a

refactor SMP

5ff723b

Add warning for POST/PUT

8339937

Add warning for SMP Patch

67ddac6

Add warn for PatchCRD

2e084cb

cleanup comment

9e24323

Implement {Create,Update,Patch}Options for field validation

d57bcc9

Add feature gate for FieldValidation

fb910cd

Add JSONPatch tests

8bc736a

kevindelgado force-pushed the validation-unify-all branch from f0caae4 to 8bc736a Compare October 20, 2021 21:09

apelisse reviewed Oct 21, 2021

View reviewed changes

kevindelgado added 3 commits October 21, 2021 23:20

wip: debug smp

d445e14

fix unstructured converter

7731220

SMP converter, remove debug printing and add commentary

adcd819

liggitt reviewed Oct 22, 2021

View reviewed changes

liggitt deleted the branch liggitt:json-stdlib October 23, 2021 13:43

liggitt closed this Oct 23, 2021

kevindelgado commented Oct 26, 2021

View reviewed changes

kevindelgado mentioned this pull request Oct 26, 2021

Server Side Strict Field Validation kubernetes/kubernetes#105916

Merged

liggitt pushed a commit that referenced this pull request Mar 10, 2023

Add e2e test for SELinux metrics

771c9be

This is the commit message for patch #2 (refresh-temp):

liggitt pushed a commit that referenced this pull request Oct 31, 2023

feat: updates encryption config file watch logic to polling

e95b7c6

Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com> fix (#2) Signed-off-by: Monis Khan <mok@microsoft.com>

liggitt pushed a commit that referenced this pull request Oct 8, 2024

Merge pull request kubernetes#122957 from richabanker/uvip-bugfix

3660a34

MVP cleanup #2

Server Side Field Validation #2

Server Side Field Validation #2

Conversation

kevindelgado commented Oct 14, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

kevindelgado commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Oct 21, 2021

kevindelgado commented Oct 22, 2021 • edited Loading

liggitt commented Oct 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Oct 26, 2021

kevindelgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado commented Oct 22, 2021 •

edited

Loading