From d5f8058bd1821b7ed9f909e1d30faccde3413a1b Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Fri, 20 Aug 2021 21:16:46 +0000 Subject: [PATCH 01/16] First draft - server-side unknown field validation --- .../README.md | 887 ++++++++++++++++++ .../kep.yaml | 48 + 2 files changed, 935 insertions(+) create mode 100644 keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md create mode 100644 keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md new file mode 100644 index 00000000000..4193f2eaf53 --- /dev/null +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -0,0 +1,887 @@ + +# KEP-2885: Server Side Unknown Field Validation + + + + + + +- [Release Signoff Checklist](#release-signoff-checklist) +- [Summary](#summary) +- [Motivation](#motivation) + - [Goals](#goals) + - [Non-Goals](#non-goals) +- [Proposal](#proposal) + - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional) + - [Risks and Mitigations](#risks-and-mitigations) +- [Design Details](#design-details) + - [Test Plan](#test-plan) +- [Production Readiness Review Questionnaire](#production-readiness-review-questionnaire) + - [Feature Enablement and Rollback](#feature-enablement-and-rollback) + - [Scalability](#scalability) +- [Implementation History](#implementation-history) +- [Alternatives](#alternatives) +- [Infrastructure Needed (Optional)](#infrastructure-needed-optional) + + +## Release Signoff Checklist + + + +Items marked with (R) are required *prior to targeting to a milestone / release*. + +- [ ] (R) Enhancement issue in release milestone, which links to KEP dir in [kubernetes/enhancements] (not the initial KEP PR) +- [ ] (R) KEP approvers have approved the KEP status as `implementable` +- [ ] (R) Design details are appropriately documented +- [ ] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input (including test refactors) + - [ ] e2e Tests for all Beta API Operations (endpoints) + - [ ] (R) Ensure GA e2e tests for meet requirements for [Conformance Tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md) + - [ ] (R) Minimum Two Week Window for GA e2e tests to prove flake free +- [ ] (R) Graduation criteria is in place + - [ ] (R) [all GA Endpoints](https://github.com/kubernetes/community/pull/1806) must be hit by [Conformance Tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md) +- [ ] (R) Production readiness review completed +- [ ] (R) Production readiness review approved +- [ ] "Implementation History" section is up-to-date for milestone +- [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io] +- [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes + + + +[kubernetes.io]: https://kubernetes.io/ +[kubernetes/enhancements]: https://git.k8s.io/enhancements +[kubernetes/kubernetes]: https://git.k8s.io/kubernetes +[kubernetes/website]: https://git.k8s.io/website + +## Summary + + +As a client sending a create, update, or patch request to the server, I want to +be able to instruct the server to fail when the kubernetes object I send has +fields that are not valid fields of the kubernetes resource. + +This will allow us to remove client-side validation from kubectl while +maintaining the same core functionality of erroring out on requests that contain +unknown or invalid fields. + +## Motivation + + +`kubectl –validate=true` is the current mechanism to indicate that a request +should fail if it specifies unknown fields on the object. + +There are a few issues with this as highlighted by the [previous +effort](https://docs.google.com/document/d/18nrtJ0gizVHnIhx5NIkGXvSaQ1wT2i6XVDUmmQqc7_4/edit#heading=h.q9616qdce0l9) to do +server-side validation, primarily these issues include: +* Bug Fixes are utilized slower because client-side upgrades are hard to get in +people’s hands. +* Each client needs to implement validation. + +This is the last remaining step for removing client-side validation according to +[this +comment](https://github.com/kubernetes/kubernetes/issues/39434#issuecomment-270486105) +* Client-side validation is [very + painful](https://github.com/kubernetes/kubernetes/issues/39434#issuecomment-270443399) + +Additional problems have been highlighted in the relevant github issues: +* https://github.com/kubernetes/kubernetes/issues/39434 +* https://github.com/kubernetes/kubernetes/issues/5889 +The community has been asking for this feature as recently as [August 8th, +2021](https://github.com/kubernetes/kubernetes/issues/104090#issuecomment-895228540) + +### Goals + + +* Server should validate that no extra fields are present or invalid (e.g. +misspelled), nor are any fields duplicated (for json and yaml data). +* We must maintain compatibility with all existing clients, thus server side +unknown field validation should be opt-in + +### Non-Goals + + +* Complete (business-logic) server-side validation of every aspect of an object +(i.e. we’re only focused on mismatched fields between the object in the request +body and its schema). +* Protobuf support. Theoretically, unknown protobuf fields could occur if clients +on version X send a request to the server of version less than X which does not recognize +some of the fields (or a similar situation). We do not think it is worth +supporting this use case initially and will error if clients attempt to validate +schema server-side with protobuf data. + + +## Proposal + + +We propose using an opt-in API mechanism (such as content-type header or query +param) to indicate to the server that it should fail when the kubernetes object +in the request body supplied to POST, PUT, and PATCH requests contains +extra/unknown fields. + +Clients such as kubectl will continue to use the `--validate=true` flag as +before, but instead of triggering validation on the client-side, it will +instruct the server to validate for unknown fields server-side. + +This change will be made in at least two steps, one where we introduce the +server side validation and a second where we modify kubectl to use the +server-side validation (and mark the existing client-side validation as +deprecated). + +### Notes/Constraints/Caveats (Optional) + +#### Future Work +After server side unknown field validation is implemented we can begin work to +deprecate and remove client side validate and have kubectl use server side +validation instead. A separate KEP will be published for that. + + + +### Risks and Mitigations + + + +### Performance Considerations +It is worth noting that checking for unknown fields whether via a json unmarshal +that explicitly breaks if it encounters unknown fields (as in the case of +create, update, or json patch) or extra logic around a merge step (as in +strategic merge patch or apply) will be less performant than not doing so. +[Initial +benchmarks](https://github.com/kubernetes/kubernetes/pull/104433#issuecomment-901398507) estimate ~20% slower 25-30% more memory consumption. + +This is deemed acceptable and insignificant because the use case we are +replacing is client side validation that happens from kubectl (i.e. via a human +that should not be too impacted by a slight performance hit). Any existing +automated clients that would be impacted by performance do not currently have a +way of leveraging server side validation and thus will remain opted-out of this +by default (or will be willing to tradeoff the performance if they do choose to +opt-in to server side schema validation). + + +## Design Details + + +### Opt-in API Mechanism + +There are a few ways we could allow a client to opt-in to server-side unknown +field validation, we present two options **Content-Type Header** and **query +parameter**. + +#### Content-Type Header + +Requests that contain a kubernetes object, also pass along with it a +content-type header such as “application/json”. One way to indicate to the +server that it should use strict schema validation and fail when unknown fields +are passed is to send a mime-type parameter indicating the strict validation +such as “application/json;validation=strict”. Alternatively we could use a new +header such as “X-Kubernetes-Validation:strict”. + +One could argue that this is the more appropriate way to parameterize opting-in +to server side schema validation because on the server we are fundamentally +treating the content as a different type (“application/json” is json data that +we don’t care if it has extra fields, “application/json;validation=strict” is +json data that is sensitive to extra fields). + +A precedent for using a mime-type parameter is from how we [receive resources as +tables](https://kubernetes.io/docs/reference/using-api/_print/#receiving-resources-as-tables). + +On the other hand, one could also argue that interpreting input strictly or not +according to the target schema is independent of the content type and that this +is an inappropriate way to parameterize validation opt-in. + +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. + +#### Query Parameter + +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”. + +This might make it more obvious to consumers of the API that strict schema +validation is a choice of the client. On the other hand, query parameters are +more typically used for filtering/sorting data returned from the API server. + +Precedent for using query parameters is that we already have CreateOptions, +PatchOptions, UpdateOptions for write requests that are communicated via query +parameters. + +We believe that using a query parameter is the best approach. + +### Create (POST) and Update (PUT) + +Implementation of this validation for Patch requests differ significantly and +are discussed in the Patch section below. + +At a high level, for create and update requests we have the opportunity to +validate the object when we unmarshal the object from wire format into the go +type. + +This happens in the [serializer +implementation](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go#L264-L291), where depending on if a “strict” +option is set (or not), the unmarshalling step will error (or not) if +extra/malformatted fields exist on the data being unmarshalled. + +This in turn, gets used by +[create](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go#L95-L120) and [update](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go#L106-L107) handlers when they attempt to +decode the request body. + +One major advantage of the “Content-Type” header approach is that it requires +little to no change existing code in the request handlers. + +When the API server is started and we create the [negotiated +serializer](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go#L616), we +create a [separate serializer](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go#L167) for each content type/media type. To support strict +validation all we would need to do is add [another +serializer](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/codec_factory.go#L51-L106) to the +NegotiatedSerializer for strict validation that is identical to the serializer +of “application/json” but has the “strict” option set (as well as ones for +yaml). + +The request handler will then proceed unchanged, and the [Decode +step](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go#L120) will fail +if it encounters fields in the request body that are not part of the schema. + +### Patch (PATCH) + +Unlike create and update requests, patch requests are more complicated because +they do not ever unmarshal/decode the request body into a kubernetes object. +Instead patching involves, serializing the existing object to json, then using a +jsonpatch or mergepatch library to combine the patch sent from the client with +the existing object, and finally deserializing the combined object into the +kubernetes object. + +During the jsonpatch or mergepatch step we don’t currently check if the patch +sent by the client has any erroneous fields. Now we need to. + +This all happens in the +[patchMechanism](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L293-L296) used by the patch handler. + +There are three types of patchers used by the patch handler: jsonPatcher, +smpPatcher, and applyPatcher that are each discussed individually. + +#### JSON Patch + +JSON patching is most commonly called when client-side apply looks to patch an +existing custom resource. + +For the jsonPatch implementation of +[applyPatchToCurrentObject](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L304), This is similar +to create and update where after using the jsonpatch library to generate the +[patchedObjJS](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L312) we then call [DecodeInto](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L319). + +Based on the presence of the validate query param, we should be able to use a +codec that is or is not doing strict decoding that will fail, in the strict +case, when invalid fields are present on the patched json blob. + +Currently, like create and update, the handler generates the [serializer from the +scope](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L136-L146) and the codec from the serializer. We just need to ensure that it +conditionally uses a StrictSerializer based on the presence of the validate +query param. + +#### Strategic Merge Patch + +Strategic merge patching is most commonly called when client-side apply attempts +to patch an existing object of a builtin type. + +For the strategic merge patch (SMP) implementation, it calls into the +apimachinery strategicpatch library to update the fields of the original object +with the data from the patch. This goes through a few layers but it eventually +boils down to this +[mergeMap](https://github.com/kubernetes/kubernetes/blob/dadecb2c8932fd28de9dfb94edbc7bdac7d0d28f/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go#L1280) call that does the actual merge of the patch into +the original. + +mergeMap takes a mergeOptions argument that we can update to have some +configuration for strict validation. Within mergeMap, we can add a branch of +code to fail with an error if mergeOptions specify strict validation and we +encounter fields on the patch that do not exist on the original. + +#### Apply Patch + +Apply Patching is called whenever we send a server side apply request to the +server. + +For apply patch, server side schema validation already occurs and is not +optional, no new behavior is needed here. The exception to this is that one can +create an arbitrary schema that is still a valid structural schema (see [docs](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema) on +structural schema). If one manages to do this and permits unknown fields, then +server-side schema validation will not be responsible for invalidating these. + +For further context, the applyPatch path of the patch handler calls into the +fieldmanager’s [Apply](https://github.com/kubernetes/kubernetes/blob/9ff3b7e744b34c099c1405d9add192adbef0b6b1/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L441) method in order to generate the newly patched object that +will be stored to etcd + +This in turn, calls +[apply](https://github.com/kubernetes/kubernetes/blob/9ff3b7e744b34c099c1405d9add192adbef0b6b1/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go#L106) on it’s own internal fieldmanager (which is what +actually implements the joining of the live object to the patch object to +produce the new object). It attempts to [convert the patch object into a +TypedValue](https://github.com/kubernetes/kubernetes/blob/9ff3b7e744b34c099c1405d9add192adbef0b6b1/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go#L129). This checks the object against its schema and will error if the +object has any unknown or duplicate fields. + + +### Test Plan + + +We can unit test and benchmark the performance of each endpoint in +[apiserver/pkg/endpoints/apiserver_test.go](https://github.com/kubernetes/kubernetes/blob/dadecb2c8932fd28de9dfb94edbc7bdac7d0d28f/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go). We will need to test that requests +fail with invalid fields for all types of fields (embedding, free-form fields, +etc). + +We will also have additional testing for changes to the strategic merge patch +logic in +[apimachinery/pkg/util/strategicpatch/patch_test.go](https://github.com/kubernetes/kubernetes/blob/dadecb2c8932fd28de9dfb94edbc7bdac7d0d28f/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go) + + + +## Production Readiness Review Questionnaire + + + +### Feature Enablement and Rollback + + + +###### How can this feature be enabled / disabled in a live cluster? + + + +- [x] Feature gate (also fill in values in `kep.yaml`) + - Feature gate name: UnknownFieldValidation + - Components depending on the feature gate: kube-apiserver +- [x] Other + - Describe the mechanism: query parameter + - Will enabling / disabling the feature require downtime of the control + plane? NO + - Will enabling / disabling the feature require downtime or reprovisioning + of a node? (Do not assume `Dynamic Kubelet Config` feature is enabled). NO + + + +### Scalability + + + +###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs? + + +Mutating API calls that opt-in to validation will be slower ([initial +benchmarks](https://github.com/kubernetes/kubernetes/pull/104433#issuecomment-901398507) +estimate ~20% slower 25-30% more memory consumption) + + + +## Implementation History + + +* Proof of Concept [PR](https://github.com/kubernetes/kubernetes/pull/104433) for Create and Update. + + + +## Alternatives +* Content-Type Header vs Query Param (see #proposal) +* Passing multiple decoders around in the request scope +* Change the Decode signature itself (or adding a new DecodeStrict that Decode + calls into) + + + + diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml new file mode 100644 index 00000000000..15e17f22ae6 --- /dev/null +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml @@ -0,0 +1,48 @@ +title: Server Side Unknown Field Validation +kep-number: 2885 +authors: + - "@kevindelgado" +owning-sig: sig-api-machinery +participating-sigs: +status: provisional +creation-date: 2021-08-20 +reviewers: + - "@apelisse" + - "@jpbetz" +approvers: + - "@lavalamp" + +##### WARNING !!! ###### +# prr-approvers has been moved to its own location +# You should create your own in keps/prod-readiness +# Please make a copy of keps/prod-readiness/template/nnnn.yaml +# to keps/prod-readiness/sig-xxxxx/00000.yaml (replace with kep number) +#prr-approvers: + +see-also: +replaces: + +# The target maturity stage in the current dev cycle for this KEP. +stage: alpha + +# The most recent milestone for which work toward delivery of this KEP has been +# done. This can be the current (upcoming) milestone, if it is being actively +# worked on. +latest-milestone: "v1.23" + +# The milestone at which this feature was, or is targeted to be, at each stage. +milestone: + alpha: "v1.23" + beta: "v1.24" + stable: "v1.25" + +# The following PRR answers are required at alpha release +# List the feature gate name and the components for which it must be enabled +feature-gates: + - name: UnknownFieldValidation + components: + - kube-apiserver +disable-supported: true + +# The following PRR answers are required at beta release +metrics: From e690a74b3dfc1a9cf77d13ab5bdbf51aefb61b49 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Tue, 24 Aug 2021 04:45:19 +0000 Subject: [PATCH 02/16] Respond to smarterclayton feedback on mergeMap and param advantages --- .../README.md | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md index 4193f2eaf53..05ef87a7271 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -343,9 +343,13 @@ This might make it more obvious to consumers of the API that strict schema validation is a choice of the client. On the other hand, query parameters are more typically used for filtering/sorting data returned from the API server. -Precedent for using query parameters is that we already have CreateOptions, -PatchOptions, UpdateOptions for write requests that are communicated via query -parameters. +Arguments for using query parameter include: +* Being able version them with the API version (unlike content-type which must + be versioned with the type) +* Parameters are more discoverable in openapi, less so for mime types. +* We have precedent for using query parameters for write requests already (via CreateOptions, + PatchOptions, UpdateOptions) + We believe that using a query parameter is the best approach. @@ -432,10 +436,16 @@ boils down to this [mergeMap](https://github.com/kubernetes/kubernetes/blob/dadecb2c8932fd28de9dfb94edbc7bdac7d0d28f/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go#L1280) call that does the actual merge of the patch into the original. -mergeMap takes a mergeOptions argument that we can update to have some -configuration for strict validation. Within mergeMap, we can add a branch of -code to fail with an error if mergeOptions specify strict validation and we -encounter fields on the patch that do not exist on the original. +`mergeMap` goest field-by-field through the existing object, checking to see if +data should be merged in from the patch. Currently, if there are any +extra/unknown fields in patch, they will never be encountered and will just be +ignored. + +`mergeMap` takes a mergeOptions argument that we can update to have some +configuration for strict validation. Within `mergeMap`, we can track the fields +of the patch as they are meged in 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 out. #### Apply Patch From 7473cff81f8181a06622b90c745099d449b5cf9e Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 26 Aug 2021 22:16:15 +0000 Subject: [PATCH 03/16] add JSON Patch POC --- .../2885-server-side-unknown-field-validation/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md index 05ef87a7271..e1beb687fdb 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -869,6 +869,7 @@ Major milestones might include: - when the KEP was retired or superseded --> * Proof of Concept [PR](https://github.com/kubernetes/kubernetes/pull/104433) for Create and Update. +* Proof of Concept [PR](https://github.com/kubernetes/kubernetes/pull/104619) for JSON Patch. ## Release Signoff Checklist From 4b1a17d942ebf1402ed0e1dc36bc2ab7d7a6f233 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Mon, 30 Aug 2021 20:09:12 +0000 Subject: [PATCH 05/16] Update PRR --- .../README.md | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md index 980ada2963d..20b72d687cc 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -99,6 +99,9 @@ tags, and then generate with `hack/update-toc.sh`. - [Test Plan](#test-plan) - [Production Readiness Review Questionnaire](#production-readiness-review-questionnaire) - [Feature Enablement and Rollback](#feature-enablement-and-rollback) + - [Rollout, Upgrade and Rollback Planning](#rollout-upgrade-and-rollback-planning) + - [Monitoring Requirements](#monitoring-requirements) + - [Dependencies](#dependencies) - [Scalability](#scalability) - [Implementation History](#implementation-history) - [Alternatives](#alternatives) @@ -640,32 +643,30 @@ Pick one of these and delete the rest. - Will enabling / disabling the feature require downtime or reprovisioning of a node? (Do not assume `Dynamic Kubelet Config` feature is enabled). NO - ### Monitoring Requirements This section must be completed when targeting beta to a release. +N/A + ### Dependencies This section must be completed when targeting beta to a release. +N/A + + - [x] Feature gate (also fill in values in `kep.yaml`) - Feature gate name: UnknownFieldValidation - Components depending on the feature gate: kube-apiserver +--> - [x] Other - Describe the mechanism: query parameter - Will enabling / disabling the feature require downtime of the control @@ -649,8 +649,8 @@ No, strict validation is false by default. ###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)? -Yes, disabling the feature flag will cause us to ignore the validation query -param, effectively rolling back this feature. +Yes, not setting the query param will ensure we are not doing strict validation +(i.e. disable the feature) ###### What happens if we reenable the feature if it was previously rolled back? diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml index 15e17f22ae6..4025e3fcf6d 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml @@ -4,13 +4,14 @@ authors: - "@kevindelgado" owning-sig: sig-api-machinery participating-sigs: -status: provisional +status: implementable creation-date: 2021-08-20 reviewers: - "@apelisse" - "@jpbetz" approvers: - - "@lavalamp" + - "@smarterclayton" + - "@deads2k" ##### WARNING !!! ###### # prr-approvers has been moved to its own location @@ -38,10 +39,6 @@ milestone: # The following PRR answers are required at alpha release # List the feature gate name and the components for which it must be enabled -feature-gates: - - name: UnknownFieldValidation - components: - - kube-apiserver disable-supported: true # The following PRR answers are required at beta release From accbe4740e10d43ec3f6734e138cfd902067b55c Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Tue, 31 Aug 2021 20:04:01 +0000 Subject: [PATCH 07/16] Add feature gate --- .../2885-server-side-unknown-field-validation/README.md | 3 +-- .../2885-server-side-unknown-field-validation/kep.yaml | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md index 140fcfe59e4..948acae1390 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -631,11 +631,10 @@ This section must be completed when targeting alpha to a release. - [x] Feature gate (also fill in values in `kep.yaml`) - Feature gate name: UnknownFieldValidation - Components depending on the feature gate: kube-apiserver ---> - [x] Other - Describe the mechanism: query parameter - Will enabling / disabling the feature require downtime of the control diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml index 4025e3fcf6d..3b390137138 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/kep.yaml @@ -39,6 +39,10 @@ milestone: # The following PRR answers are required at alpha release # List the feature gate name and the components for which it must be enabled +feature-gates: + - name: UnknownFieldValidation + components: + - kube-apiserver disable-supported: true # The following PRR answers are required at beta release From 469c95114b09db61280eba40b494e89e88a7c600 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Wed, 1 Sep 2021 17:22:15 +0000 Subject: [PATCH 08/16] Add note about client-go support --- .../2885-server-side-unknown-field-validation/README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md index 948acae1390..6ccd9f7a575 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -362,6 +362,9 @@ Arguments for using query parameter include: * We have precedent for using query parameters for write requests already (via CreateOptions, PatchOptions, UpdateOptions) +For client-go support, we will add a `UnknownFieldValidation` field to +CreateOptions, PatchOptions, and UpdateOptions that can supply the query +parameter to the request. We believe that using a query parameter is the best approach. From 6e21736d87a928d3174ae156c3a371764f80906b Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Wed, 1 Sep 2021 20:07:17 +0000 Subject: [PATCH 09/16] PRR feedback --- .../README.md | 78 +++++++++++++------ 1 file changed, 55 insertions(+), 23 deletions(-) diff --git a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md index 6ccd9f7a575..4c6461a7f0a 100644 --- a/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md +++ b/keps/sig-api-machinery/2885-server-side-unknown-field-validation/README.md @@ -125,18 +125,18 @@ checklist items _must_ be updated for the enhancement to be released. Items marked with (R) are required *prior to targeting to a milestone / release*. -- [ ] (R) Enhancement issue in release milestone, which links to KEP dir in [kubernetes/enhancements] (not the initial KEP PR) -- [ ] (R) KEP approvers have approved the KEP status as `implementable` -- [ ] (R) Design details are appropriately documented -- [ ] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input (including test refactors) +- [x] (R) Enhancement issue in release milestone, which links to KEP dir in [kubernetes/enhancements] (not the initial KEP PR) +- [x] (R) KEP approvers have approved the KEP status as `implementable` +- [x] (R) Design details are appropriately documented +- [x] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input (including test refactors) - [ ] e2e Tests for all Beta API Operations (endpoints) - [ ] (R) Ensure GA e2e tests for meet requirements for [Conformance Tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md) - [ ] (R) Minimum Two Week Window for GA e2e tests to prove flake free -- [ ] (R) Graduation criteria is in place +- [x] (R) Graduation criteria is in place - [ ] (R) [all GA Endpoints](https://github.com/kubernetes/community/pull/1806) must be hit by [Conformance Tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md) -- [ ] (R) Production readiness review completed -- [ ] (R) Production readiness review approved -- [ ] "Implementation History" section is up-to-date for milestone +- [x] (R) Production readiness review completed +- [x] (R) Production readiness review approved +- [x] "Implementation History" section is up-to-date for milestone - [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io] - [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes @@ -513,9 +513,8 @@ We will also have additional testing for changes to the strategic merge patch logic in [apimachinery/pkg/util/strategicpatch/patch_test.go](https://github.com/kubernetes/kubernetes/blob/dadecb2c8932fd28de9dfb94edbc7bdac7d0d28f/staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go) - #### Alpha - Feature implemented behind a feature flag -- Initial e2e tests completed and enabled +- Integration tests added for all relevant verbs (POST, PUT, and PATCH) + ###### How can someone using this feature know that it is working for their instance? +The easiest way to know that the feature is enabled and working is to query the +API server with the query param set and passing an object with deliberately +invalid, unknown fields. An error from the server will indicate that the feature +is enabled as expected. + +Alternatively, and end-user can cross-reference the results of client-side +validation (i.e. `kubectl --validate=true`) with the results of server-side. If +client-side validation indicates an error but server-side does not, then the +feature is disabled. + + ###### What are the reasonable SLOs (Service Level Objectives) for the enhancement? +Users should expect to see an increase in request latency and memory usage +(~20-25% increase) for requests that desire server side schema validation. + +Cluster operators can also use cpu usage monitoring to determine whether +increased usage of server-side schema validation dramatically increases CPU usage after feature enablement. + + ###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service? Pick one more of these and delete the rest. -- [ ] Metrics - - Metric name: - - [Optional] Aggregation method: - - Components exposing the metric: -- [ ] Other (treat as last resort) - - Details: +- [x] Metrics + - Metric name: CPU Usage + - Components exposing the metric: kube-apiserver +- [x] Metrics + - Metric name: Memory Consumption + - Components exposing the metric: kube-apiserver +- [x] Metrics + - Metric name: Request Latency + - Components exposing the metric: kube-apiserver + ## Alternatives -* Content-Type Header vs Query Param (see #proposal) +* Content-Type Header vs Query Param (see [#proposal](#proposal)) * Passing multiple decoders around in the request scope * Change the Decode signature itself (or adding a new DecodeStrict that Decode calls into) +* Instead of erroring on unknown fields, we could warn on unknown fields and + then more generally have a `TreatWarningsAsErrors` option to fail on requests + with unknown fields. One drawback here is that it would be difficult to + flag unkown fields separately from other warnings that we might not want to + error on (e.g. using a deprecated API).