Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server-Side Apply - a merge mistake in "main" branch shows wrong application/ssa/nginx-deployment-replicas-only.yaml file #39108

Closed
antaloala opened this issue Jan 26, 2023 · 9 comments · Fixed by #39144
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@antaloala
Copy link
Contributor

antaloala commented Jan 26, 2023

The content of the application/ssa/nginx-deployment-replicas-only.yaml file version showed in Server-Side Apply - Transferring Ownership section is wrong.

Previous versions where showing the right content, e.g. the one in dev-1.26 branch.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 26, 2023
@tengqm
Copy link
Contributor

tengqm commented Jan 26, 2023

The one in the dev-1.26 branch is not a valid Deployment manifest. The API server will reject this YAML since it is missing some required fields.

@sftim
Copy link
Contributor

sftim commented Jan 26, 2023

Ah, the problem might be that we don't want a YAML file that is a valid Deployment manifest, since this is actually being used to generate a special application/apply-patch+yaml document used for a PATCH with custom Kubernetes semantics.

/kind bug

I didn't actually know that kubectl would accept a thing that's not a valid manifest for kubectl apply; that's going to be hard to teach / document. Is that even a supported / recommended way to do the change? (I can see that it works; whether it works is very different from whether we endorse it).

I think this is a valid report. Maybe we should use an inline example rather than an external file; maybe we need a way to exempt this one YAML file from validation.

If we use an inline example, that would mean changing https://kubernetes.io/docs/reference/using-api/server-side-apply/#transferring-ownership

  • we can keep kubectl apply -f https://k8s.io/examples/application/ssa/nginx-deployment.yaml --server-side
  • kubectl apply -f https://k8s.io/examples/application/ssa/nginx-deployment-replicas-only.yaml --server-side --field-manager=handover-to-hpa --validate=false would need to change.

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 26, 2023
@tengqm
Copy link
Contributor

tengqm commented Jan 26, 2023

I think this is a valid report. Maybe we should use an inline example rather than an external file; maybe we need a way to exempt this one YAML file from validation.

If this YAML has to be an "invalid" Deployment manifest, using an inline example is more appropriate IMO.

@sftim
Copy link
Contributor

sftim commented Jan 26, 2023

I'll also ask SIG Architecture if this use of kubectl is one we endorse.

@antaloala
Copy link
Contributor Author

Exactly, this is one of the "tricks" behind SSA: you only provide (to the kubectl --server-side command) the fields you want to change (i.e. the fields you want to take ownership on behalf of the field-manager you indicate).
Providing a fully compliant (from an schema pov) Deployment manifest would actually be a mistake if you only want to modify the .spec.replicas field using SSA.

Kubernetes client-go was updated to support these non-fully-conformant-from-an-schema-pov manifests: defining the new apply configurations types for this to be possible as described in this k8s blog entry.

@tengqm
Copy link
Contributor

tengqm commented Jan 29, 2023

/triage accepted
/assign

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 29, 2023
@sftim
Copy link
Contributor

sftim commented Jan 29, 2023

OK, so the input to kubectl apply is no longer defined to be a manifest: it's actually an apply configuration, and all valid manifests are also valid apply configurations. However, kubectl apply defaults to validating that the supplied apply configuration is a valid manifest. You can then turn off that validation by using --validate=false.

Have I got that right?

If so, I think we missed a key docs update when we made that change to kubectl / client-go. https://kubernetes.io/blog/2021/08/06/server-side-apply-ga/ is the only mention of the term apply configuration on the Kubernetes site; the docs don't describe this at all.


The rest of this comment assumes that the apply configuration concept does need documenting.

I think we should clearly document which kubectl subcommands work with manifests and which with apply configurations. For example, I'd have to turn to the source code to answer which kind the kubectl diff subcommand expects, and that's not something an end user should need to be doing.

A few subcommands and plugins to consider:

  • kubectl create
  • kubectl replace
  • kubectl diff
  • kubectl convert

@sftim
Copy link
Contributor

sftim commented Jan 29, 2023

I also think we should track a new issue for to document the apply configuration concept, and leave this issue to cover the specific bug that @antaloala reported.

@antaloala
Copy link
Contributor Author

#39144 is solving the reported fault (so properly closing this issue), thanks for it.
I think there is still room for improvement in the "Transferring Ownership" example this issue is targeting (maybe to consider it in a future update of these pages?):

  • For the "more advanced" case (the one targeted by this issue) it is proposed to use a partial manifest; used term here should be "fully specified intent" as it is the introduced term at the beginning of this same SSA doc. page to name these "shorter" yamls we can use with SSA.
  • In the final kubectl --server-side --field-manager=handover-to-hpa ... command the --validate=false is not needed, better to remove it to not confuse the reader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants