plumb context with request deadline #96901

tkashem · 2020-11-27T05:12:40Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
plumb context with request deadline: A request deadline is:

the timeout specified in the request URL
60s is the default if the above is not present.

Plumbing:

as soon as a request is received by the apiserver, determine the timeout duration of the request and set a new request context with the appropriate deadline.
the timeout filter that times out non-long-running requests uses the request context with a deadline from above (as opposed to a fixed 60s wait today)
admission layer uses the same request context from above
storage layer uses the same request context to talk to etcd

This approach is:

more correct way of setting up a request deadline (as soon as it arrives to the apiserver)
more maintainable - the request deadline is setup inside a server filter and is used throughout. No other layer (storage or admission layer) needs to parse timeout from the request URI.

We use the default timeout enforced by the apiserver:

if the user has specified a timeout of 0s, this implies no timeout on the user's part.
if the user has specified a timeout that exceeds the maximum deadline allowed by the apiserver.

If the user specifies an invalid timeout duration specifier in the request URL then the request is failed with HTTP 400 and it is audited.

Does this PR introduce a user-facing change?:

Requests with invalid timeout parameters in the request URL now appear in the audit log correctly.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tkashem · 2020-11-27T15:52:50Z

/retest

tkashem · 2020-11-27T23:58:18Z

/retest

tkashem · 2020-11-30T03:28:45Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/fail_with_audit.go

+			return errorHandler
+		}
+
+		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {


this above handler has been copied from https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go#L35-L58 and made reusable, any server filter that appears before the audit filter can use it to audit a failed request.

If this looks good then I am happy to open a follow up PR to replace authn_audit.go.

tkashem · 2020-11-30T04:12:00Z

Hi @liggitt, I have addressed your feedback from #96061. Please review when you have time.

tkashem · 2020-11-30T04:12:57Z

/assign @liggitt @sttts @deads2k @smarterclayton

sttts · 2021-01-28T12:46:17Z

staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go

 		t.handler.ServeHTTP(w, r)
 		return
 	}

+	timeout := r.Context().Done()


nit: timeoutCh

sttts · 2021-01-28T12:47:11Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

@@ -76,7 +73,7 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int
 			}
 		}

-		ctx, cancel := context.WithTimeout(req.Context(), timeout)
+		ctx, cancel := context.WithTimeout(req.Context(), requestTimeout)


this needs explanation. Why is it good to put a 34s here now?

sttts · 2021-01-28T19:07:23Z

staging/src/k8s.io/apiserver/pkg/server/config.go

+	// context with deadline. The go-routine can keep running, while the timeout logic will return a timeout to the client.
+	handler = genericfilters.WithTimeoutForNonLongRunningRequests(handler, c.LongRunningFunc)
+
+	// WithRequestDeadline sets a deadline for the request context appropriately


this comment can go

- as soon as a request is received by the apiserver, determine the timeout of the request and set a new request context with the deadline. - the timeout filter that times out non-long-running requests should use the request context as opposed to a fixed 60s wait today. - admission and storage layer uses the same request context with the deadline specified. we use the default timeout enforced by the apiserver: - if the user has specified a timeout of 0s, this implies no timeout on the user's part. - if the user has specified a timeout that exceeds the maximum deadline allowed by the apiserver.

sttts · 2021-01-28T19:16:08Z

/lgtm
/approve

k8s-ci-robot · 2021-01-28T19:16:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, tkashem

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]
~~test/e2e/apimachinery/OWNERS~~ [sttts]
~~test/integration/apiserver/OWNERS~~ [sttts]

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

tkashem · 2021-01-28T20:16:24Z

/retest

k8s-ci-robot requested review from caesarxuchao and deads2k November 27, 2020 05:13

tkashem force-pushed the context-wiring branch from 2d63de1 to edadcb2 Compare November 27, 2020 07:49

tkashem force-pushed the context-wiring branch from edadcb2 to 1b45da0 Compare November 27, 2020 18:48

tkashem changed the title ~~[WIP] plumb context with request deadline~~ plumb context with request deadline Nov 29, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2020

tkashem force-pushed the context-wiring branch from 1b45da0 to cc9ac1f Compare November 30, 2020 03:12

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 30, 2020

tkashem force-pushed the context-wiring branch from cc9ac1f to 7804351 Compare November 30, 2020 03:21

tkashem commented Nov 30, 2020

View reviewed changes

k8s-ci-robot assigned deads2k and liggitt Nov 30, 2020

sttts reviewed Jan 28, 2021

View reviewed changes

tkashem force-pushed the context-wiring branch from 581d1f4 to 6fcd768 Compare January 28, 2021 18:56

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 28, 2021

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 28, 2021

sttts reviewed Jan 28, 2021

View reviewed changes

tkashem force-pushed the context-wiring branch from 6fcd768 to 5cd6e9b Compare January 28, 2021 19:10

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2021

tkashem added 2 commits January 28, 2021 14:15

apiserver: add e2e tests for request context deadline

df7a890

tkashem force-pushed the context-wiring branch from 5cd6e9b to df7a890 Compare January 28, 2021 19:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2021

k8s-ci-robot merged commit 0bf2618 into kubernetes:master Jan 28, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 28, 2021

openshift-ci-robot mentioned this pull request Feb 3, 2021

BUG 1924870: UPSTREAM: 96901: plumb context with request deadline openshift/kubernetes#549

Merged

tkashem mentioned this pull request Mar 29, 2021

[WIP] queue wait with request deadline #100634

Closed

liggitt mentioned this pull request Jul 27, 2021

Set idle and readheader timeouts #103958

Merged

openshift-ci-robot mentioned this pull request Sep 7, 2021

Bug 2003027: Rebase 1.20.10 openshift/kubernetes#935

Merged

tkashem mentioned this pull request Oct 26, 2021

Make sure that underlying goroutine finishes when requests times out #105884

Open

openshift-ci-robot mentioned this pull request May 6, 2022

Bug 2082029: Rebase 1.20.15 openshift/kubernetes#1253

Merged

JohnRusk mentioned this pull request Jan 16, 2023

Deletecollection failing after 34 seconds #115090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plumb context with request deadline #96901

plumb context with request deadline #96901

tkashem commented Nov 27, 2020 •

edited by liggitt

tkashem commented Nov 27, 2020

tkashem commented Nov 27, 2020

tkashem Nov 30, 2020 •

edited

tkashem commented Nov 30, 2020

tkashem commented Nov 30, 2020

sttts Jan 28, 2021

sttts Jan 28, 2021

sttts Jan 28, 2021

sttts commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

tkashem commented Jan 28, 2021

plumb context with request deadline #96901

plumb context with request deadline #96901

Conversation

tkashem commented Nov 27, 2020 • edited by liggitt

tkashem commented Nov 27, 2020

tkashem commented Nov 27, 2020

tkashem Nov 30, 2020 • edited

Choose a reason for hiding this comment

tkashem commented Nov 30, 2020

tkashem commented Nov 30, 2020

sttts Jan 28, 2021

Choose a reason for hiding this comment

sttts Jan 28, 2021

Choose a reason for hiding this comment

sttts Jan 28, 2021

Choose a reason for hiding this comment

sttts commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

tkashem commented Jan 28, 2021

tkashem commented Nov 27, 2020 •

edited by liggitt

tkashem Nov 30, 2020 •

edited