KEP-2885: Add Server-Side Unknown Field Validation KEP #2886

kevindelgado · 2021-08-20T21:19:56Z

One-line PR description: Add initial data for KEP-2885

Issue link: Server Side Unknown Field Validation #2885

kevindelgado · 2021-08-20T21:20:34Z

smarterclayton · 2021-08-23T21:50:39Z

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

+
+Another argument against this method is that for patch, strictness is handled
+when decoding the *result* of the patch, so it wouldn’t make sense to use
+Content-Type which is providing information about the inbound request body.


Although like serialization, different patch types may have different rules about strictness. Can a strategic merge patch even be strict?

I'm slightly in favor of query parameters, mostly because we can version them by the API version. Content type has to be versioned with the type (i.e. at some point if we break compat with proto2 we will have to have application/vnd.kubernetes.protobuf.2, but i'd hate to do that if we had to change the name of the mime type).

Also, mime type is slightly less discoverable in openapi, whereas parameters are more discoverable.

Currently, no, a strategic merge patch cannot be strict. In theory though, this should be doable. The way I understand SMP to work is that in the mergeMap function it goes field-by-field through the existing object and checks to see if data should be merged in from the corresponding field on the patch. If there are any extra/unknown fields in the patch, they will never be encountered and will just be ignored.

The current plan is to modify mergeMap to track the fields of the patch as they are merged into the existing object. After all fields that can be merged from the patch are, we will know that any fields on the patch that were not merged are unknown fields and can error. Does this sound reasonable to you?

I’ve added the above along with the additional advantages of the query parameter approach to the doc.

Also, mime type is slightly less discoverable in openapi, whereas parameters are more discoverable.

There is the consumes field in paths of the OpenAPI, we could expose the modifiers there.

Yeah, but it's not called out per type (i.e. you can't describe the parameters of a mime type, just a combo of mime types, so you can document application/json;v1=blah;strict=true,as=Type but not just "the as parameter to a mime type" since they didn't expect composition). Parameters + openapi are more discoverable, and while I think this feels a bit closer to mime type, discoverability and documentation win for me.

apelisse · 2021-08-25T17:11:54Z

I've reviewed this and I'm happy with it.
/lgtm

I think we probably should get an early approval on that so that enhancement freeze is not as stressful. @deads2k @lavalamp

kevindelgado · 2021-08-31T00:19:37Z

Did a pass to make sure all is ready for the upcoming enhancements freeze. I think we're good here now.

Only recent change is that I discussed feature flags with @apelisse and we agree that a feature flag probably isn't necessary here. The query param should be sufficient to enable/disable this feature and since we default to disabled, a feature flag does not add much value. Happy to add it back if others disagree though.

Can I get a review/PRR review @deads2k @smarterclayton

kevindelgado · 2021-08-31T00:26:13Z

cc @sttts Antoine mentioned you might be interested too.

wojtek-t · 2021-08-31T13:13:12Z

/assign @deads2k

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

smarterclayton · 2021-09-03T17:43:11Z

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

+
+Alternatively, if we don’t like the idea of using Content-Type header to
+determine whether the apiserver should accept or fail on unknown fields, we
+could pass a query param such as “?validate=true”.


I'd probably argue for fieldValidation=Strict instead of a bool. The default would be fieldValidation=IgnoreUnknownFields.

That makes sense, I've updated it in the KEP to reflect this for now, but agree that we can finalize it during API review.

kevindelgado · 2021-09-08T13:40:10Z

gentle ping @deads2k

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

kevindelgado

All feedback addressed. PTAL @deads2k

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

deads2k · 2021-09-08T20:52:29Z

/label tide/merge-method-squash
/lgtm
/approve
/hold

holding to give time for other reviewers. Other reviewers have until 1pm eastern tomorrow.

k8s-ci-robot · 2021-09-08T20:52:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~keps/prod-readiness/OWNERS~~ [deads2k]
~~keps/sig-api-machinery/OWNERS~~ [deads2k]

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

wojtek-t

Some nits (can be addressed in a follow up).

LGTM from SIG scalability perspective (although I agree with Clayton that eventually we should look into optimizing it).

wojtek-t · 2021-09-09T11:25:41Z

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

+###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?
+
+Yes. From the cluster operator's side, they can restart the kube-apiserver without
+the UnknownFieldValidation flag set and this will disable the feature


nit: s/flag/feature gate/

wojtek-t · 2021-09-09T11:26:36Z

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md

+  - Metric name: Memory Consumption
+  - Components exposing the metric: kube-apiserver
+- [x] Metrics
+  - Metric name: Request Latency


Let's provide the exact metric name:
apiserver_request_duration_seconds

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

deads2k · 2021-09-09T17:13:48Z

Some nits (can be addressed in a follow up).

@kevindelgado These should be addressed in followups for clarity.

/hold cancel

* First draft - server-side unknown field validation * Respond to smarterclayton feedback on mergeMap and param advantages * add JSON Patch POC * Update TOC to fix CI * Update PRR * Remove feature flag, add prod-readiness, set to implementable * Add feature gate * Add note about client-go support * PRR feedback * Update TOC * rename query param * Update test plan * Address feedback around warning, update client-go option name * Warn note * spelling * Add support for warn option

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 20, 2021

k8s-ci-robot requested review from deads2k and fedebongio August 20, 2021 21:20

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 20, 2021

k8s-ci-robot assigned apelisse Aug 20, 2021

kevindelgado force-pushed the ss-unknown-fields branch 3 times, most recently from c6552be to 68e3d74 Compare August 20, 2021 21:34

kevindelgado mentioned this pull request Aug 20, 2021

Server Side Unknown Field Validation #2885

Closed

26 tasks

First draft - server-side unknown field validation

d5f8058

kevindelgado force-pushed the ss-unknown-fields branch from 68e3d74 to d5f8058 Compare August 20, 2021 21:41

smarterclayton reviewed Aug 23, 2021

View reviewed changes

Respond to smarterclayton feedback on mergeMap and param advantages

e690a74

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

add JSON Patch POC

7473cff

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

kevindelgado added 3 commits August 30, 2021 19:55

Update TOC to fix CI

98c5134

Update PRR

4b1a17d

Remove feature flag, add prod-readiness, set to implementable

efcbc65

kevindelgado force-pushed the ss-unknown-fields branch from 76d5028 to efcbc65 Compare August 31, 2021 00:15

kevindelgado requested a review from smarterclayton August 31, 2021 00:19

k8s-ci-robot assigned deads2k Aug 31, 2021

deads2k reviewed Aug 31, 2021

View reviewed changes

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md Show resolved Hide resolved

smarterclayton reviewed Sep 3, 2021

View reviewed changes

kevindelgado added 2 commits September 3, 2021 20:30

rename query param

d179805

Update test plan

dd6f05a

deads2k reviewed Sep 8, 2021

View reviewed changes

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md Outdated Show resolved Hide resolved

deads2k reviewed Sep 8, 2021

View reviewed changes

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md Show resolved Hide resolved

deads2k reviewed Sep 8, 2021

View reviewed changes

keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md Show resolved Hide resolved

Address feedback around warning, update client-go option name

fcea565

kevindelgado commented Sep 8, 2021

View reviewed changes

Warn note

546a697

kevindelgado requested a review from deads2k September 8, 2021 20:21

kevindelgado added 2 commits September 8, 2021 20:23

spelling

8bc0f19

Add support for warn option

678a766

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 8, 2021

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

wojtek-t reviewed Sep 9, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2021

k8s-ci-robot merged commit cd041ee into kubernetes:master Sep 9, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 9, 2021

kevindelgado mentioned this pull request Sep 9, 2021

Follow up on KEP-2885 using more precise language #2970

Closed

kevindelgado mentioned this pull request Sep 15, 2021

WIP: Strict schema validation for POST and PUT kubernetes/kubernetes#104433

Closed

palnabarun mentioned this pull request Jun 12, 2022

canonical kep number json field palnabarun/enhancements#1

Closed

palnabarun mentioned this pull request Jul 26, 2022

KEP Manifest Generator palnabarun/enhancements#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2885: Add Server-Side Unknown Field Validation KEP #2886

KEP-2885: Add Server-Side Unknown Field Validation KEP #2886

kevindelgado commented Aug 20, 2021

kevindelgado commented Aug 20, 2021

smarterclayton Aug 23, 2021 •

edited

Loading

kevindelgado Aug 24, 2021 •

edited

Loading

apelisse Aug 25, 2021

smarterclayton Sep 3, 2021

apelisse commented Aug 25, 2021

kevindelgado commented Aug 31, 2021 •

edited

Loading

kevindelgado commented Aug 31, 2021

wojtek-t commented Aug 31, 2021

smarterclayton Sep 3, 2021

kevindelgado Sep 3, 2021

kevindelgado commented Sep 8, 2021

kevindelgado left a comment

deads2k commented Sep 8, 2021

k8s-ci-robot commented Sep 8, 2021

wojtek-t left a comment

wojtek-t Sep 9, 2021

wojtek-t Sep 9, 2021

deads2k commented Sep 9, 2021

KEP-2885: Add Server-Side Unknown Field Validation KEP #2886

KEP-2885: Add Server-Side Unknown Field Validation KEP #2886

Conversation

kevindelgado commented Aug 20, 2021

kevindelgado commented Aug 20, 2021

smarterclayton Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

kevindelgado Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

apelisse Aug 25, 2021

Choose a reason for hiding this comment

smarterclayton Sep 3, 2021

Choose a reason for hiding this comment

apelisse commented Aug 25, 2021

kevindelgado commented Aug 31, 2021 • edited Loading

kevindelgado commented Aug 31, 2021

wojtek-t commented Aug 31, 2021

smarterclayton Sep 3, 2021

Choose a reason for hiding this comment

kevindelgado Sep 3, 2021

Choose a reason for hiding this comment

kevindelgado commented Sep 8, 2021

kevindelgado left a comment

Choose a reason for hiding this comment

deads2k commented Sep 8, 2021

k8s-ci-robot commented Sep 8, 2021

wojtek-t left a comment

Choose a reason for hiding this comment

wojtek-t Sep 9, 2021

Choose a reason for hiding this comment

wojtek-t Sep 9, 2021

Choose a reason for hiding this comment

deads2k commented Sep 9, 2021

smarterclayton Aug 23, 2021 •

edited

Loading

kevindelgado Aug 24, 2021 •

edited

Loading

kevindelgado commented Aug 31, 2021 •

edited

Loading