KEP-4764: Kubectl supports WatchList to list resources #4767

xuzhenglun · 2024-07-18T09:01:33Z

One-line PR description: kubectl supports WatchList to list resources

Issue link: kubectl supports WatchList to list resources #4764

Other comments:

xuzhenglun · 2024-07-18T12:05:05Z

/hold

I just found that WatchList method has been merged into client-go in kubernetes/kubernetes#122657, so maybe we can dependent on it and avoid to implement it again.

But there is a problem(see kubernetes/kubernetes#126206) with the current implementation, that is, when ServerPrint is enabled, the k8s.io/initial-events-end=true annotation is on the metav1.Table.Rows instead of the top metav1.Table object. Maybe we should wait for a fix on the api side?

sftim

Thanks. Here are some suggestions for the text.

I've matched the style guide we use for documentation; our logical API verbs, such as watch, are usually written in lowercase bold.

(the HTTP-level verbs, such as HEAD or POST, or GET at HTTP level, are written uppercase without bold)

keps/sig-cli/4764-kubectl-watchlist/README.md

keps/sig-cli/4764-kubectl-watchlist/kep.yaml

keps/sig-cli/4764-kubectl-watchlist/README.md

sftim · 2024-07-27T16:21:29Z

keps/sig-cli/4764-kubectl-watchlist/README.md

+-->
+
+###### Will enabling / using this feature result in any new API calls?
+


💭 Is there a way for the denial of the watch to include a hint that a list would also fail;
for example, this hint could be included whenever sendInitialEvents is set and the API server is able to dry-run
check the equivalent list verb.

I don't quite understand what you mean by hint?

When watchlist failed, we can give a warning message and then fallback to list. And When rolling back to list, the behavior of kubectl should be consistent with the existing behavior, that is, the results of list are returned. I'm not sure about what this hint used to tell user?

oh I see the point now. you mean when the watchlist fails, the error is returned with a hint, to tell the client list would also fail. so it's unnecessary for client to try list again. Did I understand correctly?

I don't believe that we have a easy solution for this. As in my understand, this case can only happen when user have no permission to list resource. however, URL query parameters are decoded in the Handler, but authz happens before that, in the Filters.

A simpler and feasible solution is to initiate a SubjectAccessReview during watchlist and return the result as Hint. However, this will bring additional consumption to all watchlist, and it seems to be not much different from the client re-initiating the list request. After all, if the failure is caused by authz, the list has not actually started processing at this time, and it will not consume too many resources.

or we can consider the watch permission as a superset of the list permission. After all, users with the watch permission actually already have the list permission. In this case, as long as the user can pass the watch authz, they will be able to perform subsequent list operations for sure.

However, this is an incompatible change. I am not sure it is appropriate to discuss it in this KEP.

Kind of. WatchList is a client method not a logical API verb, though (so WatchList calls either watch or list).

Let's resolve this. I used the 💭 emoji in #4767 (comment) to try to imply that this was an aside, rather than any kind of blocking feedback.

oh, I see. thanks for your explanation. 😊

What do you think that if the API server only initiates "SubjectAccessReview" when "WatchList" is disabled or permission is denied? this could minimize the extra authz query.

Yes, that's how I'd imagined it.

OK, I will update KEP later to describe this. But let's focus this the basic feature in alpha, and make sure finish this enhancement this before Beta? After all, I think this feature needs to be in alpha for at least several versions. :)

What we discussed earlier have been updated into the KEP. PTAL

wojtek-t · 2024-07-29T12:21:23Z

keps/sig-cli/4764-kubectl-watchlist/README.md

+<<[UNRESOLVED Will **watchlist** replace **list** completely]>>
+**watchlist** was first released in version 1.27 and is still in the alpha stage, with continuous improvements 
+over the subsequent releases. For a feature in such an early stage, it is too soon to say whether we will fully
+rely on it without other fallback mechanisms.


kube-apiserver will support LIST requests forever for backward compatibility reasons.

But we expect that most clients (and libraries) will actually evolve towards using watchlist once this is graduated to GA.

I understand that API server will support List forever, but I'm not sure should we use List in client-side forever? or in far far future, client-go and kubectl will not send List to API Server, but only Watch?

Yes - I think the things like kubectl should eventually migrate to WatchList

wojtek-t · 2024-07-29T12:25:41Z

keps/sig-cli/4764-kubectl-watchlist/README.md

+objects changed events. A special `BOOKMARK` event with annotation `k8s.io/initial-events-end=true` will be sent 
+at the end to indicate the completion of the initial object push.
+
+Therefore, where kubectl originally performed a **list** operation using `List`, we can implement a new


Instead of managing the appropriate parameters in kubectl, you should rely on internal client-go implementation that can decide itself whether watchlist can be usef for a given request.
And you achieve it effectively by setting client-go feature gate.

I agree with that we should rely on internal client-go implementation to decide. However, due to current implementation issue, kubectl cannot directly use this feature-gate, which is what this KEP attempts to solve.

as we can see https://github.com/kubernetes/kubernetes/blob/fe3772890f650f9bcf020b43dc5a51fab0fa17f4/staging/src/k8s.io/cli-runtime/pkg/resource/helper.go#L108, kubectl use Do and Get method to list resource, not List. so the auto switch mechanism in client-go seems will not work here.

I described the introduction of a new environment variable earlier because I thought kubectl needed an independent switch to control it. If this is not necessary, I completely agree to reuse the environment variable switch in client-go to achieve this purpose. I will update KEP soon later.

I just updated the KEP to clarify this part. PTAL

wojtek-t · 2024-07-29T12:26:02Z

/cc @p0lyn0mial @deads2k

p0lyn0mial · 2024-07-30T08:41:25Z

keps/sig-cli/4764-kubectl-watchlist/README.md

+at the end to indicate the completion of the initial object push.
+
+This mechanism has been integrated into `client-go` in 1.31, and users who use typed or dynamic client can get
+this for free by setting env `KUBE_FEATURE_WatchListClient=true`. But unfortunately, in current implementation, 


does kubectl have a concept of feature gates ?
would it be more practical to set an env var or a command line option to disable a faulty feature ?

Yes, It has its own env feature-gate mechanism. But the basic idea is the same, relying on an env vairble to decide which code path to run. so in my understand, the key point is what the env name we should use.

As I user of kubectl how do I disable faulty KUBE_FEATURE_WatchListClient ?

In alpha stage, this feature is disabled by default. User who want to enable it need to set env variable KUBE_FEATURE_WatchListClient=true to enable this feature.

In the future, when it enables by default in beta stage, user can disable this feature by set env variable KUBE_FEATURE_WatchListClient=false explicitly.

p0lyn0mial · 2024-07-30T08:42:07Z

keps/sig-cli/4764-kubectl-watchlist/README.md

+
+This mechanism has been integrated into `client-go` in 1.31, and users who use typed or dynamic client can get
+this for free by setting env `KUBE_FEATURE_WatchListClient=true`. But unfortunately, in current implementation, 
+`kubectl` doesn't not use dynamic or typed client to list resource, it use raw `rest` client. So the auto switching


could kubectl actually use client-go instead of the raw rest client ?

I wish it could, but in my understanding, there is no easy way to do so. kubectl uses resource names instead of kinds to fetch resources. Even if it's possible, it will still be a big change.

keps/sig-cli/4764-kubectl-watchlist/README.md

xuzhenglun · 2024-08-05T06:54:18Z

@wojtek-t @p0lyn0mial @deads2k @sftim

Hi 👋 ,
Is anything else I need to do yet? It's will be nice if this proposal could reach the next 1.32 dev cycle.

Any suggestion I will be really appreciate.

xuzhenglun · 2024-08-19T11:42:49Z

/reopen

k8s-ci-robot · 2024-08-19T11:42:54Z

@xuzhenglun: Failed to re-open PR: state cannot be changed. There are no new commits on the xuzhenglun:master branch.

In response to this:

/reopen

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-sigs/prow repository.

xuzhenglun · 2024-08-19T11:46:46Z

/reopen

k8s-ci-robot · 2024-08-19T11:46:52Z

@xuzhenglun: Reopened this PR.

In response to this:

/reopen

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-sigs/prow repository.

k8s-ci-robot · 2024-08-19T11:47:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xuzhenglun
Once this PR has been reviewed and has the lgtm label, please assign eddiezane for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/sig-cli/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-08-19T11:47:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xuzhenglun
Once this PR has been reviewed and has the lgtm label, please assign eddiezane for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/sig-cli/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

xuzhenglun · 2024-08-19T11:56:39Z

sorry for the noise of closing and reopening, there was a slight problem when I was doing rebase.

I was thinking about the environment variable naming thing. As a kubectl feature, I still feel it will be better to use KUBECTL_ prefix and follow the customs of kubectl feature-gate.

PTAL is there anything need to do, I really want to make this happen in the next release cycle.

/cc @mpuckett159 @eddiezane @soltysh @p0lyn0mial

p0lyn0mial · 2024-08-26T06:59:10Z

@xuzhenglun Hey, I think we should hold off on this KEP until the streaming feature graduates to Beta. There are still things that need to be fixed, which might slightly change the code, especially on the client side. For the streaming API, we are targeting 1.32.

xuzhenglun · 2024-08-26T08:06:45Z

sure, as the conclusion in the last sig-cli meeting, this KEP will not merge any code only until watchlist is promoted into beta. https://docs.google.com/document/u/0/d/1I1UFGHMDO7mMbDbioQp52DEJXEhk1qymch3qL5-EN10/mobilebasic Please take your time to deal with that, and I will continue this enhancement once it’s resolved. Thank you.

xuzhenglun · 2024-10-17T11:30:34Z

hi @soltysh,
since WatchList has been promoted into Beta in kubernetes/kubernetes#128053, would you kindly review this enhanment please? 🙏

soltysh · 2024-10-28T07:58:48Z

hi @soltysh,
since WatchList has been promoted into Beta in kubernetes/kubernetes#128053, would you kindly review this enhanment please? 🙏

I'll try to find some time in the next weeks, but most likely after KubeCon NA.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2024

k8s-ci-robot requested review from mpuckett159 and soltysh July 18, 2024 09:01

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 18, 2024

xuzhenglun mentioned this pull request Jul 18, 2024

kubectl supports WatchList to list resources #4764

Open

4 tasks

xuzhenglun force-pushed the master branch from 02c7c13 to a620281 Compare July 18, 2024 09:11

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2024

sftim reviewed Jul 27, 2024

View reviewed changes

xuzhenglun force-pushed the master branch 5 times, most recently from 001c1be to 7ada715 Compare July 28, 2024 18:27

wojtek-t reviewed Jul 29, 2024

View reviewed changes

k8s-ci-robot requested review from deads2k and p0lyn0mial July 29, 2024 12:26

xuzhenglun force-pushed the master branch from e31114c to 6e87105 Compare July 30, 2024 07:29

p0lyn0mial reviewed Jul 30, 2024

View reviewed changes

vrutkovs reviewed Jul 30, 2024

View reviewed changes

keps/sig-cli/4764-kubectl-watchlist/README.md Outdated Show resolved Hide resolved

xuzhenglun force-pushed the master branch 3 times, most recently from 660fe1f to 2e78692 Compare August 5, 2024 06:36

xuzhenglun closed this Aug 19, 2024

xuzhenglun force-pushed the master branch from 2e78692 to eb019be Compare August 19, 2024 11:41

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 19, 2024

k8s-ci-robot reopened this Aug 19, 2024

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 19, 2024

k8s-ci-robot requested review from eddiezane and p0lyn0mial August 19, 2024 11:56

KEP-4764: Kubectl supports WatchList to list resources

e1f944a

xuzhenglun force-pushed the master branch from 0bb450b to e1f944a Compare August 19, 2024 12:12

		-->

		###### Will enabling / using this feature result in any new API calls?

KEP-4764: Kubectl supports WatchList to list resources #4767

Are you sure you want to change the base?

KEP-4764: Kubectl supports WatchList to list resources #4767

Conversation

xuzhenglun commented Jul 18, 2024

xuzhenglun commented Jul 18, 2024 • edited Loading

sftim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhenglun Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhenglun Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhenglun Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhenglun commented Aug 5, 2024

xuzhenglun commented Aug 19, 2024

k8s-ci-robot commented Aug 19, 2024

xuzhenglun commented Aug 19, 2024

k8s-ci-robot commented Aug 19, 2024

k8s-ci-robot commented Aug 19, 2024

k8s-ci-robot commented Aug 19, 2024

xuzhenglun commented Aug 19, 2024

p0lyn0mial commented Aug 26, 2024

xuzhenglun commented Aug 26, 2024 via email • edited Loading

xuzhenglun commented Oct 17, 2024

soltysh commented Oct 28, 2024

xuzhenglun commented Jul 18, 2024 •

edited

Loading

xuzhenglun Jul 29, 2024 •

edited

Loading

xuzhenglun Jul 29, 2024 •

edited

Loading

xuzhenglun Jul 31, 2024 •

edited

Loading

xuzhenglun commented Aug 26, 2024 via email •

edited

Loading