diff --git a/keps/prod-readiness/sig-api-machinery/2885.yaml b/keps/prod-readiness/sig-api-machinery/2885.yaml index e4c8157dcbd..6673b70ae63 100644 --- a/keps/prod-readiness/sig-api-machinery/2885.yaml +++ b/keps/prod-readiness/sig-api-machinery/2885.yaml @@ -1,3 +1,5 @@ kep-number: 2885 alpha: approver: "@deads2k" +beta: + approver: "@deads2k" 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..2eecf9fdc6d 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 @@ -85,28 +85,33 @@ tags, and then generate with `hack/update-toc.sh`. - [Proposal](#proposal) - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional) - [Future Work](#future-work) + - [Out-of-Tree Alternatives to Client Side Validation](#out-of-tree-alternatives-to-client-side-validation) + - [Aligning json and yaml errors](#aligning-json-and-yaml-errors) - [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) - [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) + - [HTTP header mechanism](#http-header-mechanism) + - [Other Alternatives Considered](#other-alternatives-considered) ## Release Signoff Checklist @@ -127,8 +132,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 +142,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 +225,12 @@ 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 default to server-side validation against servers that support + it. +* kubectl should provide the ability for a user to request no validation, + or warning only validation, instead of strict server-side validation on + a per-request basis. ### Non-Goals @@ -236,7 +246,10 @@ on version X send a request to the server of version less than X which does not 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. - +* Enhanced offline validation. Current client-side validation has been used as a + form of offline validation to simply validate configs that have no intention + of being actually applied to a server. Ideally users would have a supported + way to perform offline validation, but that is outside the scope of this KEP. ## Proposal @@ -248,26 +261,80 @@ 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. + +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. -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). +When server-side validation is enabled on the server (as will be the default +starting in beta), we propose modifying the kubectl `--validate` flag to +default to server-side strict validation and allow the user to choose between +strict, warn-only, or no validation. + +We propose documenting that client-side validation is deprecated and server-side +validation will be used instead by default if available. We will fallback to +client-side validation in the mean time if kubectl does not connect to a server +with server-side validation enabled (either because the server it is connected +to is older or has the feature-gate disabled, or because kubectl is not +connected to any server at all). + +When we fallback to client-side validation we will send a warning to let users +know that their request is being validated by the deprecated client-side +validation. ### 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. + +##### Out-of-Tree Alternatives to Client Side Validation +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. + +Kubectl will use server-side validation by default starting in beta for servers +that have it enabled. If kubectl does not recognize a server with server-side +validation enabled, however, it will fall back to using client-side validation +with a deprecation warning. + +Even though we feel deprecating client-side validation is justified due to the +inevitability of it always being less correct than server-side validation, we +still acknowledge that there are benefits to giving users a way to validate +their objects without needing access to a server (which is a current use-case of +client-side validation, albeit one that is error-prone and not officially +supported). + +Long-term, we want to favor using out-of-tree solutions for client-side +validation, though this idea is still in its infancy. + +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. + +##### 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. + -### 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). +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`. -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 “?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: +* 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 +524,33 @@ 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: + +* `true` or `strict`: If server-side validation is enabled on the server it sends + a request with the fieldValidation param set to `Strict`, otherwise it falls + back to client-side validation. It will also fall back to client-side + validation if the request has `--dry-run=client`, because the request is not + actually being sent to a server-side validation enabled server. + The default remains `true`, as it currently is today. +* `false` or `ignore`: This performs no server-side validation or client-side validation, +sending a request to the server with fieldValidation param set to `Ignore` if +server-side validation is enabled. +* `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. + +We will introduce a mechanism similar to the cli-runtime +[DryRunVerifier](https://github.com/kubernetes/cli-runtime/blob/cfe3fe3837db26e9afb75e9491348080a4f0ef23/pkg/resource/dry_run_verifier.go#L73:26) +to read the server's published OpenAPI to determine whether or not server-side +validation is enabled on the server. + +The goal here is for the flag to be intent based, whether we use client-side or +server-side under the hood is based solely on whether or not server-side +validation is supported by the apiserver kubectl is connected to. + ### 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 +No deprecations or removals however the kubectl flag `--validate=true` is being changed +to perform server-side validation instead of client-side validation (for servers +that support it). -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 +840,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 +866,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 +894,34 @@ 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 for json and yaml +requests. - + +Operators can disable server-side validation by setting the feature-gate to false if +the performance impact of it is not tolerable. ## Implementation History @@ -927,8 +935,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)) + +### HTTP header mechanism + +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: