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

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

Open
alvaroaleman opened this issue Nov 7, 2022 · 21 comments
Open

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

alvaroaleman opened this issue Nov 7, 2022 · 21 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@alvaroaleman
Copy link
Member

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

$ 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

# 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)

@alvaroaleman alvaroaleman added the kind/bug Categorizes issue or PR as related to a bug. label Nov 7, 2022
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 7, 2022
@k8s-ci-robot
Copy link
Contributor

@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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@alvaroaleman
Copy link
Member Author

/sig cli

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 7, 2022
@ardaguclu
Copy link
Member

/transfer kubectl

@k8s-ci-robot k8s-ci-robot transferred this issue from kubernetes/kubernetes Nov 7, 2022
@eddiezane
Copy link
Member

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
Copy link
Contributor

@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:

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 9, 2022
@henvic
Copy link

henvic commented Nov 9, 2022

/assign

@henvic
Copy link

henvic commented Nov 16, 2022

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
Copy link
Member Author

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
Copy link

henvic commented Nov 16, 2022

@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
Copy link
Member

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
Copy link
Member

apelisse commented Nov 16, 2022

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
Copy link
Member Author

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: alvaroaleman/static-kas#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
Copy link
Member

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
Copy link

henvic commented Nov 30, 2022

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
Copy link
Member

apelisse commented Nov 30, 2022

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
Copy link
Member

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
Copy link
Member

@henvic are you planning to work on this issue?

henvic added a commit to henvic/kubernetes that referenced this issue Jan 17, 2023
When the flag --show-managed-fields is set, the command kubectl edit
should show the managedFields in the editor.

If the user tries to edit change any property, it lets them but returns
a warning that editing managed fields does not use Server-Side Apply.

Task: kubernetes/kubectl#1322
@henvic
Copy link

henvic commented Jan 17, 2023

Hi @ardaguclu,

I've just opened a pull request for this feature at kubernetes/kubernetes#115121

@k8s-triage-robot
Copy link

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

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

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

/remove-triage accepted

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

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

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

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

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

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 19, 2024
@k8s-triage-robot
Copy link

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:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

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

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 19, 2024
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. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
Status: Backlog
8 participants