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

Support for /watch in api paths should be removed #8337

Closed
nikhiljindal opened this Issue May 15, 2015 · 42 comments

Comments

Projects
None yet
@nikhiljindal
Member

nikhiljindal commented May 15, 2015

I still see watch in the path at many places: http://kubernetes.io/third_party/swagger-ui/#!/v1beta3/watchEndpointslist

I know we were working on making it a query param and removing it from the path.

@smarterclayton @bgrant0607

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 15, 2015

Contributor

Once we drop v1beta1/2 we should switch all the clients to using the new query parameter.

----- Original Message -----

I still see watch in the path at many places:
http://kubernetes.io/third_party/swagger-ui/#!/v1beta3/watchEndpointslist

I know we were working on making it a query param and removing it from the
path.

@smarterclayton @bgrant0607


Reply to this email directly or view it on GitHub:
#8337

Contributor

smarterclayton commented May 15, 2015

Once we drop v1beta1/2 we should switch all the clients to using the new query parameter.

----- Original Message -----

I still see watch in the path at many places:
http://kubernetes.io/third_party/swagger-ui/#!/v1beta3/watchEndpointslist

I know we were working on making it a query param and removing it from the
path.

@smarterclayton @bgrant0607


Reply to this email directly or view it on GitHub:
#8337

@nikhiljindal

This comment has been minimized.

Show comment
Hide comment
@nikhiljindal

nikhiljindal May 15, 2015

Member

Do we want to support both: as a query param and in path for v1beta3 and v1?

Member

nikhiljindal commented May 15, 2015

Do we want to support both: as a query param and in path for v1beta3 and v1?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 15, 2015

Contributor

I would prefer not to. Although we don't support watch on a single resource with the query param option (you have to pass metadata.name). That's slightly less user friendly. The benefit is that everything for watch goes through the same path. @lavalamp probably has an opinion.

----- Original Message -----

Do we want to support both: as a query param and in path for v1beta3 and v1?


Reply to this email directly or view it on GitHub:
#8337 (comment)

Contributor

smarterclayton commented May 15, 2015

I would prefer not to. Although we don't support watch on a single resource with the query param option (you have to pass metadata.name). That's slightly less user friendly. The benefit is that everything for watch goes through the same path. @lavalamp probably has an opinion.

----- Original Message -----

Do we want to support both: as a query param and in path for v1beta3 and v1?


Reply to this email directly or view it on GitHub:
#8337 (comment)

@bgrant0607 bgrant0607 added this to the v1.0 milestone May 15, 2015

@bgrant0607 bgrant0607 added the area/api label May 15, 2015

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 15, 2015

Member

Is there a compelling reason for using a query parameter as opposed to a custom verb?

Member

lavalamp commented May 15, 2015

Is there a compelling reason for using a query parameter as opposed to a custom verb?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 15, 2015

Contributor

Not having a custom verb, clear versioning, general restfulness. It makes admission control and policy easier to reason about. Watch returns a list of events which are another representation of the object.

With that change we would not have any custom verbs (proxy is a subresource now for pods and nodes).

On May 15, 2015, at 6:10 PM, Daniel Smith notifications@github.com wrote:

Is there a compelling reason for using a query parameter as opposed to a custom verb?


Reply to this email directly or view it on GitHub.

Contributor

smarterclayton commented May 15, 2015

Not having a custom verb, clear versioning, general restfulness. It makes admission control and policy easier to reason about. Watch returns a list of events which are another representation of the object.

With that change we would not have any custom verbs (proxy is a subresource now for pods and nodes).

On May 15, 2015, at 6:10 PM, Daniel Smith notifications@github.com wrote:

Is there a compelling reason for using a query parameter as opposed to a custom verb?


Reply to this email directly or view it on GitHub.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607
Member

bgrant0607 commented May 15, 2015

Ref #8087

@nikhiljindal

This comment has been minimized.

Show comment
Hide comment
@nikhiljindal

nikhiljindal May 15, 2015

Member

IIUC, Shouldnt this be part of #7018 as well to not support watch in path in v1?

Member

nikhiljindal commented May 15, 2015

IIUC, Shouldnt this be part of #7018 as well to not support watch in path in v1?

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 May 16, 2015

Member

Watch is already supported as a query parameter. Eliminating the legacy form is part of getting rid of the legacy versions.

Member

bgrant0607 commented May 16, 2015

Watch is already supported as a query parameter. Eliminating the legacy form is part of getting rid of the legacy versions.

@davidopp

This comment has been minimized.

Show comment
Hide comment
@davidopp

davidopp May 20, 2015

Member

We should re-title this issue to make it clearer, so someone can pick it up (it is currently unassigned and marked for 1.0). IIUC from @bgrant0607 's last entry, the work here is simply to remove watch-in-the-path everywhere. Is that right?

Member

davidopp commented May 20, 2015

We should re-title this issue to make it clearer, so someone can pick it up (it is currently unassigned and marked for 1.0). IIUC from @bgrant0607 's last entry, the work here is simply to remove watch-in-the-path everywhere. Is that right?

@bgrant0607 bgrant0607 changed the title from Is watch still supported in the path? to Support for /watch in api paths should be removed May 20, 2015

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 May 20, 2015

Member

@smarterclayton Could someone from Redhat tackle this?

Member

bgrant0607 commented May 20, 2015

@smarterclayton Could someone from Redhat tackle this?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 20, 2015

Contributor

Yeah

Contributor

smarterclayton commented May 20, 2015

Yeah

@smarterclayton smarterclayton self-assigned this May 20, 2015

@brendandburns

This comment has been minimized.

Show comment
Hide comment
@brendandburns

brendandburns Jun 5, 2015

Contributor

@smarterclayton @bgrant0607
Is this in flight? Is this just going to happen by default when we disable v1beta3?

Contributor

brendandburns commented Jun 5, 2015

@smarterclayton @bgrant0607
Is this in flight? Is this just going to happen by default when we disable v1beta3?

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jun 5, 2015

Member

We're not working on it here.

No, it won't happen for free. The registration of the paths is unconditional:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/apiserver/api_installer.go#L308

Actually, @caesarxuchao was looking for something to do now that the v1 api is done.

Member

bgrant0607 commented Jun 5, 2015

We're not working on it here.

No, it won't happen for free. The registration of the paths is unconditional:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/apiserver/api_installer.go#L308

Actually, @caesarxuchao was looking for something to do now that the v1 api is done.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jun 5, 2015

Member

IIRC, watch using the query param was added by PR #5763

Member

bgrant0607 commented Jun 5, 2015

IIRC, watch using the query param was added by PR #5763

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jun 7, 2015

Contributor

If we could memoize APIRequestInfo for use in both the limit check and the authorizer then I would say we should do that. In theory the context for a request should be populated early with the desired API request (version, resource, etc) structure, then all the subordinate / wrapped handlers can reuse that. The ultimate goal is to be able to make intelligent statements about a request before it is processed by the authenticator / authorizer / limiter / metrics / API serving code, and to do so consistently across all of them.

If we moved the authorizer logic into an early stage filter for the API server, that would accomplish part of that goal, although it's still very difficult to ensure all requests are properly authenticated. @deads2k and @liggitt and I went around and around on this and establishing APIRequestInfo early (and reusing it later, or at least testing it is consistent with what is served) is still probably the best way forward.

Possibly a post 1.0 thing, but:

  1. Decompose request early into APIRequestInfo and store with the context
  2. Use precalc'd APIRequestInfo in limiter and authorizer
  3. Have API server check the request info matches the expected call
  4. Rate limiter can check known endpoints ("subresource" in "proxy,logs,exec", "method"="get" and "hasparam="watch") and limit independently (we need to limit the first 3 as well).

Long term I think we need changes to our HTTP server code to let us set timeouts per connection / request more effectively as well.

On Jun 5, 2015, at 5:48 PM, Brian Grant notifications@github.com wrote:

IIRC, watch using the query param was added by PR #5763


Reply to this email directly or view it on GitHub.

Contributor

smarterclayton commented Jun 7, 2015

If we could memoize APIRequestInfo for use in both the limit check and the authorizer then I would say we should do that. In theory the context for a request should be populated early with the desired API request (version, resource, etc) structure, then all the subordinate / wrapped handlers can reuse that. The ultimate goal is to be able to make intelligent statements about a request before it is processed by the authenticator / authorizer / limiter / metrics / API serving code, and to do so consistently across all of them.

If we moved the authorizer logic into an early stage filter for the API server, that would accomplish part of that goal, although it's still very difficult to ensure all requests are properly authenticated. @deads2k and @liggitt and I went around and around on this and establishing APIRequestInfo early (and reusing it later, or at least testing it is consistent with what is served) is still probably the best way forward.

Possibly a post 1.0 thing, but:

  1. Decompose request early into APIRequestInfo and store with the context
  2. Use precalc'd APIRequestInfo in limiter and authorizer
  3. Have API server check the request info matches the expected call
  4. Rate limiter can check known endpoints ("subresource" in "proxy,logs,exec", "method"="get" and "hasparam="watch") and limit independently (we need to limit the first 3 as well).

Long term I think we need changes to our HTTP server code to let us set timeouts per connection / request more effectively as well.

On Jun 5, 2015, at 5:48 PM, Brian Grant notifications@github.com wrote:

IIRC, watch using the query param was added by PR #5763


Reply to this email directly or view it on GitHub.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jun 8, 2015

Member

@smarterclayton Perhaps you intended to post your latest comment to another issue?

Member

bgrant0607 commented Jun 8, 2015

@smarterclayton Perhaps you intended to post your latest comment to another issue?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jun 8, 2015

Contributor

It was combining #6207 which rate limited watch separately, with the removal of the thing that allow watches to be detected. We need a better way to unambiguously identify watch requests so that they can be rate limited in a separate bucket. If we remove watch here we have no effective way to bucket regular calls and watch calls separately, so in theory watches can exhaust the regular pool of the apiserver rate limiter. We can look for the "watch" query param in the short term, but that doesn't fix "pod/proxy" or "pod/exec" or "pod/logs" which are all bound in the same rate limit.

Contributor

smarterclayton commented Jun 8, 2015

It was combining #6207 which rate limited watch separately, with the removal of the thing that allow watches to be detected. We need a better way to unambiguously identify watch requests so that they can be rate limited in a separate bucket. If we remove watch here we have no effective way to bucket regular calls and watch calls separately, so in theory watches can exhaust the regular pool of the apiserver rate limiter. We can look for the "watch" query param in the short term, but that doesn't fix "pod/proxy" or "pod/exec" or "pod/logs" which are all bound in the same rate limit.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Feb 10, 2016

Member

@krousey can you do an audit of the code to see if there's work to be done here? It would be good to finish this for 1.2 if it's no longer risky.

Member

lavalamp commented Feb 10, 2016

@krousey can you do an audit of the code to see if there's work to be done here? It would be good to finish this for 1.2 if it's no longer risky.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Feb 10, 2016

Member

to be clear, this is for removing use of it, not support for it under /v1/watch/..., correct?

Member

liggitt commented Feb 10, 2016

to be clear, this is for removing use of it, not support for it under /v1/watch/..., correct?

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Feb 10, 2016

Member

@liggitt If you think we're still using it, I'll just kick it out of the milestone now...

Actually, I think we should just not support this for non-v1 paths, and never remove it from the v1 paths, that would be a breaking change.

On further thought I don't think we should spend time on this right now.

Member

lavalamp commented Feb 10, 2016

@liggitt If you think we're still using it, I'll just kick it out of the milestone now...

Actually, I think we should just not support this for non-v1 paths, and never remove it from the v1 paths, that would be a breaking change.

On further thought I don't think we should spend time on this right now.

@lavalamp lavalamp modified the milestones: next-candidate, v1.2 Feb 10, 2016

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Feb 10, 2016

Member

Action item: make api installer not have the watch/ path for any new APIs or api versions.

Member

lavalamp commented Feb 10, 2016

Action item: make api installer not have the watch/ path for any new APIs or api versions.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Feb 10, 2016

Member

I think we should just not support this for non-v1 paths, and never remove it from the v1 paths, that would be a breaking change.

exactly

Member

liggitt commented Feb 10, 2016

I think we should just not support this for non-v1 paths, and never remove it from the v1 paths, that would be a breaking change.

exactly

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Mar 29, 2016

Member

Another more immediate action item here is that right now, this works as expected:

/api/v1/namespaces/default/pods?resourceVersion=6374&watch=true

... results in a watch on the pod list.

But the same thing with a pod name appended:

/api/v1/namespaces/default/pods/hello-www-2798522470-18wnc?resourceVersion=6374&watch=true

...simply returns the pod without a watch. Expected is that it would give you a watch, as above.

This does work:

/api/v1/watch/namespaces/default/pods/hello-www-2798522470-18wnc?resourceVersion=6374&watch=true

This pattern and result seems to hold true for all objects, including those in /apis/extensions/v1beta1 (e.g. deployment).

+1 on removing the watch verb (another benefit of param over verb is that selfLink works very nicely with simply appending a watch param), especially in extensions/v1beta1, but in the mean time, it would be good to add proper support for the watch param for individual items.

Member

ghodss commented Mar 29, 2016

Another more immediate action item here is that right now, this works as expected:

/api/v1/namespaces/default/pods?resourceVersion=6374&watch=true

... results in a watch on the pod list.

But the same thing with a pod name appended:

/api/v1/namespaces/default/pods/hello-www-2798522470-18wnc?resourceVersion=6374&watch=true

...simply returns the pod without a watch. Expected is that it would give you a watch, as above.

This does work:

/api/v1/watch/namespaces/default/pods/hello-www-2798522470-18wnc?resourceVersion=6374&watch=true

This pattern and result seems to hold true for all objects, including those in /apis/extensions/v1beta1 (e.g. deployment).

+1 on removing the watch verb (another benefit of param over verb is that selfLink works very nicely with simply appending a watch param), especially in extensions/v1beta1, but in the mean time, it would be good to add proper support for the watch param for individual items.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Mar 29, 2016

Member

Watch on an individual item has weird semantics if the item doesn't exist. Watching the list filtered to the item is more consistent.

Member

liggitt commented Mar 29, 2016

Watch on an individual item has weird semantics if the item doesn't exist. Watching the list filtered to the item is more consistent.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Mar 25, 2017

Member

All in tree clients were converted to use the watch param in #41722

Member

liggitt commented Mar 25, 2017

All in tree clients were converted to use the watch param in #41722

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Mar 25, 2017

Contributor

Definitely the oldest feature we've been trying to deprecate and remove

Contributor

smarterclayton commented Mar 25, 2017

Definitely the oldest feature we've been trying to deprecate and remove

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Dec 22, 2017

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

fejta-bot commented Dec 22, 2017

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Jan 21, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot commented Jan 21, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle rotten
/remove-lifecycle stale

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Feb 20, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

fejta-bot commented Feb 20, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Feb 20, 2018

Member

/reopen
/remove-lifecycle rotten
/cc @mikedanese

Member

liggitt commented Feb 20, 2018

/reopen
/remove-lifecycle rotten
/cc @mikedanese

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Feb 20, 2018

Contributor

@liggitt: you can't re-open an issue/PR unless you authored it or you are assigned to it.

In response to this:

/reopen
/remove-lifecycle rotten
/cc @mikedanese

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.

Contributor

k8s-ci-robot commented Feb 20, 2018

@liggitt: you can't re-open an issue/PR unless you authored it or you are assigned to it.

In response to this:

/reopen
/remove-lifecycle rotten
/cc @mikedanese

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.

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot May 21, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot commented May 21, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Jun 20, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot commented Jun 20, 2018

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@fejta-bot

This comment has been minimized.

Show comment
Hide comment
@fejta-bot

fejta-bot Jul 20, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

fejta-bot commented Jul 20, 2018

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment