Server Side Strict Field Validation #105916

kevindelgado · 2021-10-26T17:31:44Z

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.

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 #39434 and #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-26T17:33:19Z

/assign @liggitt
/assign @apelisse

Addressed most of the feedback from liggitt#2. Left responses to comments on that PR. Still a couple TODOs around test formatting and defaulting to warning, but wanted to get this up for another round of review ASAP.

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

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

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

staging/src/k8s.io/apimachinery/pkg/util/net/http.go

test/integration/apiserver/field_validation_test.go

k8s-triage-robot · 2021-10-26T19:49:32Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

caesarxuchao · 2021-10-26T20:12:12Z

/triage accepted

kevindelgado

Writing tests now, but it's going pretty slowly. Will hopefully add a bunch more tomorrow. Probably wont address the converter stuff until I've made it through all the tests. Feel free to hold off on reviewing until I've added more tests, but if you do want to take a look I made some changes the to jsonPatcher in order to collect decoding errors that arise from the patch itself having invalid json, and started updating a few of the tests to be more inline with your testing comment.

Relavent commits are 9bd487a and dd91910

test/integration/apiserver/field_validation_test.go

kevindelgado

I've done a big pass based on previous feedback. In particular, I revamped the integration testing suite based on Jordan's previous comments. It caught a bunch of bugs.

I figured this would be a good time to check in and ask for a review especially around the testing strategy.

In parallel, I have a few isolated issues I'm working on, but I don't want this to block reviewing. These are mainly:
~~1. kjson.UnmarshalStrict doesn't seem to recognizing duplicate fields when unmarshalling into an unstructured object. I need to dig into this further.~~ (fixed by unmarshalling directly into the object)
2. I still need to implement full field path collection to the converter (and pruning) so that the errors reported contain the full path to the field (i.e /spec/unknownField rather than just unknownField)(Done)
~~3. The SMP/warn test case is not returning the full list of unknown fields. I need to dig into this further.~~(Fixed via the new converter implementation)

Also, I've cordoned off the benchmark tests for now. Still working on them, but the ones I had I think lost their utility and were hard to maintain as I was rapidly iterating on the tests. I will rewrite them once the test suite gets reviewed and is a little more stable.

test/integration/apiserver/field_validation_test.go

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

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

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

staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

kevindelgado

All feedback addressed, benchstats still look good (ignore case is completely inert, but slightly more allocs in the strict case than before):

Ignore

name                                      old time/op    new time/op    delta
FromUnstructuredWithValidation/Ignore-16     149µs ± 1%     148µs ± 1%   ~     (p=0.222 n=5+5)

name                                      old alloc/op   new alloc/op   delta
FromUnstructuredWithValidation/Ignore-16    26.8kB ± 0%    26.8kB ± 0%   ~     (p=0.524 n=5+5)

name                                      old allocs/op  new allocs/op  delta
FromUnstructuredWithValidation/Ignore-16       641 ± 0%       641 ± 0%   ~     (all equal)

Strict

name                                      old time/op    new time/op    delta
FromUnstructuredWithValidation/Strict-16     149µs ± 1%     167µs ± 1%  +11.87%  (p=0.008 n=5+5)

name                                      old alloc/op   new alloc/op   delta
FromUnstructuredWithValidation/Strict-16    26.8kB ± 0%    33.5kB ± 0%  +25.00%  (p=0.008 n=5+5)

name                                      old allocs/op  new allocs/op  delta
FromUnstructuredWithValidation/Strict-16       641 ± 0%       745 ± 0%  +16.22%  (p=0.008 n=5+5)

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

liggitt · 2021-11-19T15:50:35Z

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

+	// alpha: v1.23
+	//
+	// Enables server-side field validation.
+	StrictFieldValidation featuregate.Feature = "StrictFieldValidation"


nit: would ServerSideFieldValidation be a better name, since the feature enables warnings in addition to strict handling? should be an easy find/replace if so

yea I agree, done

follow up with a PR updating the KEP and a PR updating website docs

staging/src/k8s.io/apimachinery/pkg/runtime/types.go

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/recognizer/recognizer.go

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go

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

staging/src/k8s.io/apimachinery/pkg/runtime/converter.go

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

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

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

kevindelgado

all feedback addressed

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

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

+	// alpha: v1.23
+	//
+	// Enables server-side field validation.
+	StrictFieldValidation featuregate.Feature = "StrictFieldValidation"


yea I agree, done

staging/src/k8s.io/apimachinery/pkg/runtime/types.go

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/recognizer/recognizer.go

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

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

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go

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

liggitt · 2021-11-19T19:24:22Z

/lgtm
/approve

k8s-ci-robot · 2021-11-19T19:28:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevindelgado, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apimachinery/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/code-generator/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [liggitt]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [liggitt]
~~test/integration/apiserver/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

leilajal · 2021-11-19T20:45:29Z

/lgtm

k8s-ci-robot · 2021-11-19T21:16:26Z

@kevindelgado: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-network-proxy-grpc	b3347064b17f0c6eafac8e30136bc03bed9a2782	link	false	`/test pull-kubernetes-e2e-gce-network-proxy-grpc`
pull-kubernetes-e2e-gce-network-proxy-http-connect	b3347064b17f0c6eafac8e30136bc03bed9a2782	link	true	`/test pull-kubernetes-e2e-gce-network-proxy-http-connect`
pull-kubernetes-e2e-gce-large-performance	c6954828a21e4f5e4a714ecfdf36bf169a55abfa	link	true	`/test pull-kubernetes-e2e-gce-large-performance`
pull-kubernetes-e2e-gce-big-performance	c6954828a21e4f5e4a714ecfdf36bf169a55abfa	link	true	`/test pull-kubernetes-e2e-gce-big-performance`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Implements server side field validation behind the `ServerSideFieldValidation` feature gate. With the feature enabled, any create/update/patch request with the `fieldValidation` query param set to "Strict" will error if the object in the request body have unknown fields. A value of "Warn" (also the default when the feautre is enabled) will succeed the request with a warning. When the feature is disabled (or the query param has a value of "Ignore"), the request will succeed as it previously had with no indications of any unknown or duplicate fields.

leilajal · 2021-11-19T21:27:23Z

/lgtm

k8s-ci-robot requested review from cheftako and enisoc October 26, 2021 17:32

k8s-ci-robot assigned apelisse and liggitt Oct 26, 2021

This was referenced Oct 26, 2021

WIP: Strict schema validation for POST and PUT #104433

Closed

WIP: Strict Schema Validation for JSON Patch Requests #104619

Closed

liggitt reviewed Oct 26, 2021

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go Outdated Show resolved Hide resolved

liggitt reviewed Oct 26, 2021

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. area/dependency Issues or PRs related to dependency changes and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 26, 2021

kevindelgado commented Oct 28, 2021

View reviewed changes

test/integration/apiserver/field_validation_test.go Show resolved Hide resolved

kevindelgado force-pushed the validation-unify-all branch from e8b931a to dd91910 Compare October 28, 2021 01:11

kevindelgado commented Nov 3, 2021

View reviewed changes

liggitt reviewed Nov 19, 2021

View reviewed changes

kevindelgado commented Nov 19, 2021

View reviewed changes

kevindelgado force-pushed the validation-unify-all branch from c6d7e04 to be7e61d Compare November 19, 2021 08:41

liggitt reviewed Nov 19, 2021

View reviewed changes

kevindelgado force-pushed the validation-unify-all branch from be7e61d to a46c5f9 Compare November 19, 2021 18:35

kevindelgado commented Nov 19, 2021

View reviewed changes

liggitt reviewed Nov 19, 2021

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go Outdated Show resolved Hide resolved

kevindelgado force-pushed the validation-unify-all branch from a46c5f9 to 8cd4bf0 Compare November 19, 2021 19:06

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2021

kevindelgado force-pushed the validation-unify-all branch from 8cd4bf0 to bea263b Compare November 19, 2021 20:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021

k8s-ci-robot assigned leilajal Nov 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021

liggitt moved this from In progress to API review completed, 1.23 in API Reviews Nov 19, 2021

kevindelgado force-pushed the validation-unify-all branch from bea263b to e50e2bb Compare November 19, 2021 21:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2021

k8s-ci-robot merged commit 8f9dd0a into kubernetes:master Nov 19, 2021

aojea mentioned this pull request Nov 21, 2021

[Failing test] integration-master #106573

Closed

liggitt mentioned this pull request Jan 14, 2022

fix disallow unknown fields when decoding into object #105822

Closed

liggitt mentioned this pull request Jan 25, 2022

Disallowing unknown fields doesn't work when decoding into an object in the JSON serializer #104661

Closed

kevindelgado mentioned this pull request Feb 9, 2022

Stricter server-side validation of JSON entities #5889

Closed

alexzielenski mentioned this pull request Mar 29, 2022

REQUEST: New membership for alexzielenski kubernetes/org#3343

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server Side Strict Field Validation #105916

Server Side Strict Field Validation #105916

kevindelgado commented Oct 26, 2021

kevindelgado commented Oct 26, 2021

k8s-triage-robot commented Oct 26, 2021

caesarxuchao commented Oct 26, 2021

kevindelgado left a comment •

edited

kevindelgado left a comment •

edited

kevindelgado left a comment

liggitt Nov 19, 2021

kevindelgado Nov 19, 2021

liggitt Nov 19, 2021

kevindelgado left a comment

kevindelgado Nov 19, 2021

liggitt commented Nov 19, 2021

k8s-ci-robot commented Nov 19, 2021

leilajal commented Nov 19, 2021

k8s-ci-robot commented Nov 19, 2021 •

edited

leilajal commented Nov 19, 2021

Server Side Strict Field Validation #105916

Server Side Strict Field Validation #105916

Conversation

kevindelgado commented Oct 26, 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 26, 2021

k8s-triage-robot commented Oct 26, 2021

caesarxuchao commented Oct 26, 2021

kevindelgado left a comment • edited

Choose a reason for hiding this comment

kevindelgado left a comment • edited

Choose a reason for hiding this comment

kevindelgado left a comment

Choose a reason for hiding this comment

Ignore

Strict

liggitt Nov 19, 2021

Choose a reason for hiding this comment

kevindelgado Nov 19, 2021

Choose a reason for hiding this comment

liggitt Nov 19, 2021

Choose a reason for hiding this comment

kevindelgado left a comment

Choose a reason for hiding this comment

kevindelgado Nov 19, 2021

Choose a reason for hiding this comment

liggitt commented Nov 19, 2021

k8s-ci-robot commented Nov 19, 2021

leilajal commented Nov 19, 2021

k8s-ci-robot commented Nov 19, 2021 • edited

leilajal commented Nov 19, 2021

kevindelgado left a comment •

edited

kevindelgado left a comment •

edited

k8s-ci-robot commented Nov 19, 2021 •

edited