search

kubernetes / kubectl

Issue tracker and mirror of kubectl code
Apache License 2.0
2.82k stars 909 forks source link

Client-side validation is skipped if the cluster and resource support server-side validation #1625

Open james-mchugh opened 1 month ago

james-mchugh commented 1 month ago

What happened:

This "bug" (perhaps not a bug, maybe more of a breaking change) is not with the kubectl CLI per-se. It is with the kubectl Go package. A breaking change appears to have been introduced with the added support of server-side validation in v0.30.0.

helm (which utilizes the kubectl Go package under the hood) now prints warnings when resources fail validation instead of throwing an error (see https://github.com/helm/helm/issues/13053). This is because the schema returned by the new Validator factory no longer returns an error if the cluster supports server-side validation.

What you expected to happen:

helm throws a validation error if invalid fields are found in the manifest.

How to reproduce it (as minimally and precisely as possible):

In any version of Helm >= 3.9.0:

  1. Create a chart helm create foo.
  2. Modify the deployment.yaml to add an invalid field. Example:
apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "foo.fullname" . }}
  labels:
    {{- include "foo.labels" . | nindent 4 }}
spec:
  badField: false
  ...
  1. Run helm install foo ./foo
  2. Observe:
helm install foo foo
W0721 12:53:43.211370    4631 warnings.go:70] unknown field "spec.badField"
NAME: foo
LAST DEPLOYED: Sun Jul 21 12:53:43 2024
NAMESPACE: default
STATUS: deployed
REVISION: 1

When instead, this previously caused Helm to fail.

Additionally, invalid fields (e.g., setting matchLabels to an array instead of object, cause helm install to install the chart and then fail, requiring it to then be uninstalled. Previously, the client-side validation would have prevented the chart from ever being installed in the first place.

Anything else we need to know?:

This appears to have been introduced in https://github.com/kubernetes/kubernetes/commit/fe3772890f650f9bcf020b43dc5a51fab0fa17f4 when server-side validation support was added to Kubectl. This changed the schema returned by the Validator factory, which in turn changed the ValidateBytes function used to validate the resource. Specifically, https://github.com/kubernetes/kubectl/blob/b315eb8455a7d5c11ed788d1592b4afeca85771d/pkg/validation/schema.go#L140 made it so that client side validation never runs if the cluster supports server-side validation of the resource. This was a breaking change, as previously, any errors that could have been detected client-side would have been detected here.

I experimented a bit with a potential solution for this. It is still very much a WIP/POC, but if you believe it will be valuable here, I can continue working on it. The potential solution (https://github.com/kubernetes/kubectl/compare/master...james-mchugh:kubectl:bugfix/client-side-validation?expand=1) adds a preferredStrategy field to the paramVerifyingSchema struct and Validator function, so callers of Validator can specify if they want server-side or client-side validation to be preferred. If client-side is picked, the ValidateBytes function will continue with validation regardless of whether the cluster supports server-side validation.

If you think this is something that should be fixed, I would be happy to contribute a fix for it.

Environment:

k8s-ci-robot commented 1 month ago

This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
james-mchugh commented 1 month ago

Another potential solution for this that would not result in as many changes as https://github.com/kubernetes/kubectl/compare/master...james-mchugh:kubectl:bugfix/client-side-validation?expand=1 would be to introduce some sort of ClientValidator function, that utilizes the same logic as Validator prior to https://github.com/kubernetes/kubernetes/commit/fe3772890f650f9bcf020b43dc5a51fab0fa17f4.

mpuckett159 commented 3 weeks ago

/close This is a helm issue because the change that happened is that server side validation became the default behavior, and according to the documentation for server side apply the default mode is to only warn on validation failures. It is possible to set validation behavior to strict, however that must be implemented by helm itself.

k8s-ci-robot commented 3 weeks ago

@mpuckett159: Closing this issue.

In response to [this](https://github.com/kubernetes/kubectl/issues/1625#issuecomment-2289312798): >/close >This is a helm issue because the change that happened is that server side validation became the default behavior, and according to the documentation for server side apply the default mode is to only warn on validation failures. It is possible to set validation behavior to strict, however that must be implemented by helm itself. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
james-mchugh commented 3 weeks ago

/close This is a helm issue because the change that happened is that server side validation became the default behavior, and according to the documentation for server side apply the default mode is to only warn on validation failures. It is possible to set validation behavior to strict, however that must be implemented by helm itself.

/reopen

Unless I'm misunderstanding, I don't think that fully explains the situation. Helm currently has strict validation enabled (validate=true is the default behavior when commands are executed).

The issue appears to be that kubectl no longer runs client-side validation at all if the server supports server-side validation. Specifically, https://github.com/kubernetes/kubectl/blob/b315eb8455a7d5c11ed788d1592b4afeca85771d/pkg/validation/schema.go#L140 appears to only run client-side validation if it detects that the server does not support server-side validation. A way to validate this is by trying to run the same helm commands above in a version of Kubernetes prior to server-side validation being introduced, as mentioned here.

james-mchugh commented 3 weeks ago

/reopen

k8s-ci-robot commented 3 weeks ago

@james-mchugh: Reopened this issue.

In response to [this](https://github.com/kubernetes/kubectl/issues/1625#issuecomment-2292177661): >/reopen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
ardaguclu commented 3 weeks ago

@james-mchugh thank for filing this issue and great investigation. Client side validation in k8s has always some issues and defects that can't be resolved easily and always server side validation is considered to be the better alternative. That's why, server side validation is not an additive feature to client side validation, instead it is a replacement of client side validation. I hope that this explains better why this commit https://github.com/kubernetes/kubectl/commit/61f42e595f688124f4a74c099a677377bcb3243b forces to use the server side validation.

In this particular case, I understand the inconvenience. However, in current structure, this https://github.com/helm/helm/blob/847369c184d93fc4d36e9ec86a388b60331ab37a/pkg/kube/client.go#L344-L349 is not correct usage because nowhere passes the validation directive to helper functions to be taken into account by server. That https://github.com/helm/helm/pull/13123 is definitely a good attempt because until validation directive (i.e. strict) is passed to helper, all the invalid fields will be manifested as warning (because server side validation's default value is warning).

That said actually this code block https://github.com/helm/helm/blob/847369c184d93fc4d36e9ec86a388b60331ab37a/pkg/kube/client.go#L344-L349 should only be executed if the server does not support server side just like kubectl and the only remaining requirement is to change the default value to strict (or warn, ignore, etc. according to user needs).

I hope this makes sense and why we push towards server side validation usage from the kubectl point of view.

james-mchugh commented 3 weeks ago

Thank you for the response.

So to confirm, there is no way on the Helm side to continue to use client-side validation using the latest kubectl package. Is that correct? Ultimately, I would expect the goal for Helm would be to move to server-side validation as attempted in https://github.com/helm/helm/pull/13123, but it would be useful if there was a way to continue to use client-side validation until the necessary updates were finalized.

Or are you suggesting that the changes to Helm needed to support server-side validation should be trivial enough that it wouldn't even be necessary to try using client-side validation in the short-term?

That said actually this code block https://github.com/helm/helm/blob/847369c184d93fc4d36e9ec86a388b60331ab37a/pkg/kube/client.go#L344-L349 should only be executed if the server does not support server side just like kubectl and the only remaining requirement is to change the default value to strict (or warn, ignore, etc. according to user needs).

Is that check necessary at that point? Given it will ultimately call https://github.com/kubernetes/kubectl/blob/b315eb8455a7d5c11ed788d1592b4afeca85771d/pkg/validation/schema.go#L140 which performs the check. For example, this is the stack of helm install 

k8s.io/kubectl/pkg/validation.(*paramVerifyingSchema).ValidateBytes(schema.go:139)
k8s.io/cli-runtime/pkg/resource.ValidateSchema(visitor.go:262)
k8s.io/cli-runtime/pkg/resource.(*StreamVisitor).Visit(visitor.go:605)
k8s.io/cli-runtime/pkg/resource.EagerVisitorList.Visit(visitor.go:241)
k8s.io/cli-runtime/pkg/resource.(*EagerVisitorList).Visit(<autogenerated>:1)
k8s.io/cli-runtime/pkg/resource.FlattenListVisitor.Visit(visitor.go:424)
k8s.io/cli-runtime/pkg/resource.(*FlattenListVisitor).Visit(<autogenerated>:1)
k8s.io/cli-runtime/pkg/resource.FlattenListVisitor.Visit(visitor.go:424)
k8s.io/cli-runtime/pkg/resource.(*FlattenListVisitor).Visit(<autogenerated>:1)
k8s.io/cli-runtime/pkg/resource.ContinueOnErrorVisitor.Visit(visitor.go:387)
k8s.io/cli-runtime/pkg/resource.(*ContinueOnErrorVisitor).Visit(<autogenerated>:1)
k8s.io/cli-runtime/pkg/resource.DecoratedVisitor.Visit(visitor.go:359)
k8s.io/cli-runtime/pkg/resource.(*DecoratedVisitor).Visit(<autogenerated>:1)
k8s.io/cli-runtime/pkg/resource.(*Result).Infos(result.go:122)
helm.sh/helm/v3/pkg/kube.(*Client).Build(client.go:357)
helm.sh/helm/v3/pkg/action.(*Install).RunWithContext(install.go:330)
main.runInstall(install.go:316)
main.newInstallCmd.func2(install.go:156)
github.com/spf13/cobra.(*Command).execute(command.go:983)
github.com/spf13/cobra.(*Command).ExecuteC(command.go:1115)
github.com/spf13/cobra.(*Command).Execute(command.go:1039)
main.main(helm.go:83)
runtime.main(proc.go:271)
runtime.goexit(asm_amd64.s:1695)
<unknown>(:0)
runtime.newproc(<autogenerated>:1)

It ultimately hits https://github.com/kubernetes/kubectl/blob/b315eb8455a7d5c11ed788d1592b4afeca85771d/pkg/validation/schema.go#L140 with the variables

c = {*validation.paramVerifyingSchema | 0xc004061680} 
 schema = {validation.Schema | validation.ConjunctiveSchema} len:2, cap:2
 verifier = {resource.Verifier | *resource.fallbackQueryParamVerifier} 
 directive = {string} "Strict"
data = {[]uint8} len:524, cap:576
err = {error} nil
obj = {interface{} | map[string]interface{}} 
gvk = {schema.GroupVersionKind} 
errs = {[]error} nil

so it prevents the client-side validation from ever running (unless the resource/server does not support server-side validation).

(Edit: I realized I never finished actually finished my sentence in my first paragraph)