search

kubernetes / kubectl

Issue tracker and mirror of kubectl code
Apache License 2.0
2.89k stars 924 forks source link

Kubectl edit: `--show-managed-fields` does nothing #1322

Open alvaroaleman opened 2 years ago

alvaroaleman commented 2 years ago

What happened?

I edited an object like so kubectl edit svc my-server --show-managed-fields and epected to see managed fields but didn't. When using kubectl get -oyaml --show-managed-fields they are shown

What did you expect to happen?

See managed fields

How can we reproduce it (as minimally and precisely as possible)?

kubectl edit some-type some-name --show-managed-fields

Anything else we need to know?

No response

Kubernetes version

```console $ kubectl version $ k version --client --short Flag --short has been deprecated, and will be removed in the future. The --short output will become the default. Client Version: v1.25.3 Kustomize Version: v4.5.7 # paste output here ```

Cloud provider

AWS

OS version

```console # On Linux: $ cat /etc/os-release # paste output here $ uname -a # paste output here # On Windows: C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture # paste output here ```

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

k8s-ci-robot commented 2 years ago

@alvaroaleman: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

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/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
alvaroaleman commented 2 years ago

/sig cli

ardaguclu commented 2 years ago

/transfer kubectl

eddiezane commented 2 years ago

This is inherited from the JSON printer. We need a refactor of this to its own flag struct that can be added to the commands that use it.

https://github.com/kubernetes/kubernetes/blob/37e73b419e455db34f5fe3e8d815418680ab23df/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/json_yaml_flags.go#L39

/triage accepted /priority backlog /assign @henvic

k8s-ci-robot commented 2 years ago

@eddiezane: GitHub didn't allow me to assign the following users: henvic.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide

In response to [this](https://github.com/kubernetes/kubectl/issues/1322#issuecomment-1309090355): >This is inherited from the JSON printer. We need a refactor of this to its own flag struct that can be added to the commands that use it. > >https://github.com/kubernetes/kubernetes/blob/37e73b419e455db34f5fe3e8d815418680ab23df/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/json_yaml_flags.go#L39 > >/triage accepted >/priority backlog >/assign @henvic 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/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
henvic commented 2 years ago

/assign

henvic commented 2 years ago

I'm working on this change, but when I was investigating ShowManagedFields I noticed some remarks that might be worth discussing before opening a PR:

From the direction server-side apply took (see Kubernetes 1.18 Feature Server-side Apply Beta 2), it seems that the --show-managed-fields flag is useful for conflicts resolution.

However, I've also read some comments that made me wonder if we should consider deprecating the flag for this command.

For example, I've found this two-years-old comment on the task #91946: Strip .meta.managedFields for kubectl edit command that indicates it shouldn't be removed, but on a later comment I see @lavalamp suggested that the command could fail with a message saying that kubectl edit couldn't be used to edit managed fields.

Comments? /cc @lavalamp, @eddiezane

Further reference:

alvaroaleman commented 2 years ago

kubectl edit, contrary to say -o yaml uses an actual editor to open the manifest, so you get highlighting. Not showing it by default as for other commands makes total sense, but it would be nice to have the option to see it.

managedFields is read only and set by the apiserver. I would have assumed that kubectl knows this through openapi and refuses an edit that changes it already though?

henvic commented 2 years ago

@alvaroaleman, is the reason why you want this change just to open the YAML on your editor for viewing (and not really editing)?

If it's the case, do you typically just go to the editor and ^c the kubectl process immediately to avoid editing your resources by mistake?

Not sure if this is appropriate, but have you considered suggesting an --editor flag for -o yaml?

lavalamp commented 2 years ago

kubectl edit actively uses apply, either client or server-side to accomplish what you ask it to do. As a consequence, it CANNOT modify managedFields, since those are part of the mechanism that makes that work. So, we will always hide this field in kubectl edit. We should probably remove the flag from kubectl edit? @apelisse @seans3

If you want to modify managedFields--which is rare but occasionally necessary, mostly by logic transitioning objects from CSA to SSA--you CAN do it, but you have to use GET / PUT.

apelisse commented 2 years ago

There's no SSA with kubectl edit it always sends an SMP or merge patch (for CRs), but I've sometimes tried to fix (or try things) managed fields myself using kubectl edit and that bug prevents me from doing so, I think it'd be nice to fix it.

That being said, I think editing managed fields though kubectl edit is very likely to be racy (and won't use resourceVersion), so I think Daniel is right and we should outright forbid it.

alvaroaleman commented 2 years ago

kubectl edit actively uses apply, either client or server-side to accomplish what you ask it to do. As a consequence, it CANNOT modify managedFields, since those are part of the mechanism that makes that wo

Right, I don't want to actually edit them. I want to look at them with proper highlighting in a proper editor without having to do a get -o yaml > tmpfile && $EDITOR tmpfile

I don't think I am the only one doing this, because in a project of mine someone asked for edit functionality for this reason, without actually wanting to edit anything: https://github.com/alvaroaleman/static-kas/issues/4

Not sure if this is appropriate, but have you considered suggesting an --editor flag for -o yaml?

That seems like a great idea.

lavalamp commented 2 years ago

I think we should make edit use SSA in the future, then.

I guess I don't object to showing the field as long as it prevents you from modifying it? If people get into the habit of modifying it we wouldn't be able to use SSA in the future...

henvic commented 2 years ago

Should we drop this in favor of #1328 for now? It seems more appropriate to the issue @alvaroaleman is trying to solve, and while I understand some might want to be able to edit managedFields using the command, it is unclear to me if the trade-off of making it editable is worth the price and the other option (returning it but not making them editable) seems to carry some learning curve / cognitive load (and might lead to unwanted surprising behavior) as well. What do you think?

apelisse commented 2 years ago

Right, I don't want to actually edit them. I want to look at them with proper highlighting in a proper editor without having to do a get -o yaml > tmpfile && $EDITOR tmpfile

I was trying to trace back to the reasons why you wanted to do that. There it is :-)

I think we should honor the --show-managed-fields flag in kubectl edit to show them, but then we should probably remove them BEFORE computing the patch so that they are never included. That sounds fairly easy, non-controversial, and useful for at least @alvaroaleman's use-case.

lavalamp commented 2 years ago

I'm fine with that as long as users are warned or get an error if they do try to change them (rather than silently not doing what they asked for).

ardaguclu commented 1 year ago

@henvic are you planning to work on this issue?

henvic commented 1 year ago

Hi @ardaguclu,

I've just opened a pull request for this feature at https://github.com/kubernetes/kubernetes/pull/115121

k8s-triage-robot commented 10 months ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

k8s-triage-robot commented 7 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 6 months ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten