From fb9b1d43320f46011cfd5e640faa4123593b09a0 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 9 Dec 2021 01:44:48 +0000 Subject: [PATCH 01/15] Field Validation beta KEP --- .../README.md | 356 +++++++++--------- .../kep.yaml | 4 +- 2 files changed, 180 insertions(+), 180 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 ffcf55b48bb..97001b394a0 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 @@ -88,25 +88,29 @@ tags, and then generate with `hack/update-toc.sh`. - [Risks and Mitigations](#risks-and-mitigations) - [Performance Considerations](#performance-considerations) - [Design Details](#design-details) - - [Opt-in API Mechanism](#opt-in-api-mechanism) - - [Content-Type Header](#content-type-header) - - [Query Parameter](#query-parameter) + - [Opt-in API Mechanism (Query Parameter)](#opt-in-api-mechanism-query-parameter) - [Create (POST) and Update (PUT)](#create-post-and-update-put) - [Patch (PATCH)](#patch-patch) - [JSON Patch](#json-patch) - [Strategic Merge Patch](#strategic-merge-patch) - [Apply Patch](#apply-patch) + - [Kubectl Flag](#kubectl-flag) - [Test Plan](#test-plan) - [Graduation Criteria](#graduation-criteria) - [Alpha](#alpha) + - [Beta](#beta) + - [Alpha Follow Ups](#alpha-follow-ups) - [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) + - [Troubleshooting](#troubleshooting) - [Implementation History](#implementation-history) - [Alternatives](#alternatives) + - [Content-Type Header](#content-type-header) + - [Other Alternatives Considered](#other-alternatives-considered) ## Release Signoff Checklist @@ -127,8 +131,8 @@ checklist items _must_ be updated for the enhancement to be released. Items marked with (R) are required *prior to targeting to a milestone / release*. -- [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` +- [ ] (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` - [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) @@ -137,7 +141,7 @@ Items marked with (R) are required *prior to targeting to a milestone / release* - [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) - [x] (R) Production readiness review completed -- [x] (R) Production readiness review approved +- [ ] (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 @@ -220,7 +224,9 @@ know that this has succeeded? * 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 +unknown field validation should be opt-in. +* kubectl should provide the ability for a user to request server-side + validation. ### Non-Goals @@ -248,26 +254,38 @@ implementation. What is the desired outcome and how do we measure success?. The "Design Details" section below is for the real nitty-gritty. --> -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. +We propose adding server-side schema validation, enabling the API server +to validate that objects received in the request body in POST, PUT, and +PATCH requests contain no unknown or duplicate fields. -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). +We introduce a query parameter that enables the user/client to indicate +that server-side validation should either not be performed at all, provide +warnings for the presence of unknown/duplicate fields, or error out in the +presence of unknown/duplicate fields. + +For beta, we propose modifying the kubectl `--validate` flag to enable users +to choose between client-side validation or server-side +validation (either strict or warning) ### 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. + +Upon successfully providing kubectl access to server-side validation via the +`--validate` flag, an open question will remain as to the future of client +side validation. + +While we would like kubectl to use server-side validation by default once it +is GA, the question remains whether client-side validation be optionally +available to kubectl users or completely deprecated. + +Maintaining client-side validation indefinitely provides the benefit of +allowing users to validate their objects without needing access to a server. +On the other hand, the downside of continuing to maintain client-side +validation is that it uses openapiv2 which will be less actively supported +as openapiv3 becomes standard and will cause client-side validation to +continue to diverge from server-side validation. -### Opt-in API Mechanism +### Opt-in API Mechanism (Query Parameter) 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. +field validation, we believe the best option is to introduce a new query +parameter to indicate the level of field validation the server should perform, +such as `?fieldValidation=Strict`. -#### 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 “?fieldValidation=Strict”. - -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. - -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. +Primary arguments for using query parameter include: +* Being able version them with the API version. +* Parameters are discoverable in openapi. * We have precedent for using query parameters for write requests already (via CreateOptions, PatchOptions, UpdateOptions) +An alternative to using a query parameter, using content-type header is +discussed in the [#alternatives](#alternatives) section. + For client-go support, we will add a `FieldValidation` field to CreateOptions, PatchOptions, and UpdateOptions that can supply the query parameter to the request. @@ -491,6 +476,34 @@ TypedValue](https://github.com/kubernetes/kubernetes/blob/9ff3b7e744b34c099c1405 object has any unknown or duplicate fields. +#### Kubectl Flag + +For beta, we will modify the kubectl `--validate` flag from being a bool flag to +a string flag that accepts the following values: + +* `server`: This sends a request with the fieldValidation param set to `Strict` +for strict server-side validation. +* `client`: This performs client-side validation in kubectl that breaks if it +encounters any validation errors before sending a request to the server. If no +client-side validation errors are found, the request sent to the server has +fieldValidation param set to `Ignore`. This will be the default behavior in beta +as it is identical to the current validation behavior pre-1.24 +* `ignore`: This performs no client-side validation or server-side validation, +sending a request to the server with fieldValidation param set to `Ignore`. +* `warn`: This sends a request to the server with the fieldValidation param set + to `Warn`. It performs server-side validation, but validation errors are + exposed as warnings in the result header rather than failing the request. This + will be the default once server-side validation goes GA. +* `strict`: same behavior as `--validate=server`. +* `true`: This is the same as `--validate=client`, but throws a warning along + the lines of "`--validate=true` is being deprecated, use `--validate=client` + instead". This is also the default behavior if `--validate` flag is passed + withouth a value set. +* `false`: This is the same as `--validate=ignore`, but throws a warning along + the lines of "`--validate=false` is being deprecated use `--validate=ignore` + instead". + + ### Test Plan #### Alpha -- Feature implemented behind a feature flag +- Feature implemented behind a feature gate - Integration tests added for all relevant verbs (POST, PUT, and PATCH) - -### Monitoring Requirements +For beta we will be warning kubectl users that `--validate={true,false}` is +being deprecated in favor of `--validate={client,server,strict,warn,ignore}`. -This section must be completed when targeting beta to a release. +### Monitoring Requirements - +One could create a metric to track the number of validation errors on the +server. We will not be implementing this for beta as it seems like overkill. + ###### 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 @@ -780,44 +804,21 @@ question. Pick one more of these and delete the rest. - [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 + - Metric name: apiserver_request_duration_seconds - Components exposing the metric: kube-apiserver - ### Dependencies -This section must be completed when targeting beta to a release. - -N/A - - +No ### Scalability @@ -829,40 +830,23 @@ For beta, this section is required: reviewers must answer these questions. For GA, this section is required: approvers should be able to confirm the previous answers based on experience in the field. +--> ###### Will enabling / using this feature result in any new API calls? -Describe them, providing: - - API call type (e.g. PATCH pods) - - estimated throughput - - originating component(s) (e.g. Kubelet, Feature-X-controller) -Focusing mostly on: - - components listing and/or watching resources they didn't before - - API calls that may be triggered by changes of some Kubernetes resources - (e.g. update of object X triggers new updates of object Y) - - periodic API calls to reconcile state (e.g. periodic fetching state, - heartbeats, leader election, etc.) +No ###### Will enabling / using this feature result in introducing new API types? -Describe them, providing: - - API type - - Supported number of objects per cluster - - Supported number of objects per namespace (for namespace-scoped objects) +No ###### Will enabling / using this feature result in any new calls to the cloud provider? -Describe them, providing: - - Which API(s): - - Estimated increase: +No ###### Will enabling / using this feature result in increasing size or count of the existing API objects? -Describe them, providing: - - API type(s): - - Estimated increase in size: (e.g., new annotation of size 32B) - - Estimated amount of new objects: (e.g., new Object X for every existing Pod) ---> +No ###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs? @@ -874,46 +858,29 @@ Think about adding additional work or introducing new steps in between [existing SLIs/SLOs]: https://git.k8s.io/community/sig-scalability/slos/slos.md#kubernetes-slisslos --> -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) +Mutating API calls that opt-in to validation will be slower. +Benchmarks of alpha changes indicate that performing validation results in +~2-5% slower request latency and ~4-8% more memory usage. - + +Opt out of server-side validation if the performance impact of it is not +tolerable. ## Implementation History @@ -927,8 +894,11 @@ Major milestones might include: - the version of Kubernetes where the KEP graduated to general availability - 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. +* 2021-11-19 Alpha Implementation + [PR](https://github.com/kubernetes/kubernetes/pull/105916) of Server Side Field Validation merged. +* 2021-12-07 Kubernetes 1.23 released with alpha implementation present. +* 2021-12-08 Better documentation for fieldValidation parameter + [merged](https://github.com/kubernetes/kubernetes/pull/106722). ## Alternatives -* Content-Type Header vs Query Param (see [#proposal](#proposal)) + +### Content-Type Header + +Instead of opting-in to server-side validation via a query parameter, we +considered passing a specific content-type header that would indiciate strict +schema validation. + +This could be done by passing a mime-type parameter indicating strict validation +such as `application/json;validation=strict` or we could use an entirely 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. + + +### Other Alternatives Considered + * 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 index c2b79aa2107..5514d447b2c 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 @@ -24,12 +24,12 @@ see-also: replaces: # The target maturity stage in the current dev cycle for this KEP. -stage: alpha +stage: beta # 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" +latest-milestone: "v1.24" # The milestone at which this feature was, or is targeted to be, at each stage. milestone: From 794ccccfdd1b1cfda63c708cc37eaed23ba3ff10 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Wed, 5 Jan 2022 01:36:50 +0000 Subject: [PATCH 02/15] expand on CS validation deprecation future work --- .../README.md | 15 +++++++++++++-- 1 file changed, 13 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 97001b394a0..aa8e38630f0 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 @@ -109,7 +109,7 @@ tags, and then generate with `hack/update-toc.sh`. - [Troubleshooting](#troubleshooting) - [Implementation History](#implementation-history) - [Alternatives](#alternatives) - - [Content-Type Header](#content-type-header) + - [HTTP header mechanism](#http-header-mechanism) - [Other Alternatives Considered](#other-alternatives-considered) @@ -287,6 +287,17 @@ validation is that it uses openapiv2 which will be less actively supported as openapiv3 becomes standard and will cause client-side validation to continue to diverge from server-side validation. +We propose offering a separate KEP to tackle deprecating client-side validation +once server-side validation is GA. Our initial thoughts on this are that in +order to deprecate client-side validation we will need to build a separate +out-of-tree client-side validation mechanism so that users can continue to +validate their configs. + +The [kubeval](https://www.kubeval.com/) project is an example of an out-of-tree solution that does this, and +we will look into expanding its support of open API to v3, and investigate +whether it makes sense as a permanent solution to client-side validation that +will allow for deprecation of current client-side validation. + Mutating API calls that opt-in to validation will be slower. Benchmarks of alpha changes indicate that performing validation results in -~2-5% slower request latency and ~4-8% more memory usage. +~2-5% slower request latency and ~4-8% more memory usage for json and yaml +requests. + +Given that the majority of high-frequency requests made by system +components use protobuf, we expect a negligible increase in overall resource +usage. ###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components? From d882529dfb415964cf7d4f7fd3cdcb9bd656fdae Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 6 Jan 2022 06:55:20 +0000 Subject: [PATCH 04/15] Aligning json and yaml errors section --- .../README.md | 23 +++++++++++++++++-- 1 file changed, 21 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 66e5c13bcca..3b5ef897061 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 @@ -303,6 +303,27 @@ we will look into expanding its support of open API to v3, and investigate whether it makes sense as a permanent solution to client-side validation that will allow for deprecation of current client-side validation. +##### Aligning json and yaml errors + +A few discrepancies between sigs.k8s.io/yaml and sigs.k8s.io/json make detecting +and reporting strict errors inconsistent and should be addressed at some point. + +1. sigs.k8s.io/yaml does not currently have a strict unmarshaling mechanism to + distinguish between strict and non-strict errors while sigs.k8s.io/json does. + This results in having to perform unmarshaling twice for yaml data in some + cases. + See [this + discussion](https://github.com/kubernetes/kubernetes/pull/105916#discussion_r748530682) from the the alpha PR and the follow-up [tracking + issue](https://github.com/kubernetes-sigs/yaml/issues/70). + +2. kyaml and kjson currently report strict decoding errors in different formats. + For example a duplicate field error appears from kyaml as `line 26: key + "imagePullPolicy" already set in map`, while kjson reports the error as + `duplicate field "spec.template.spec.containers[0].imagePullPolicy"`. For + consistencies sake, these two libraries should eventually report the same + errors in the same format. + +