DelegatingAuthenticationOptions: TokenReview request timeout #100959
Conversation
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
area/apiserver
area/cloudprovider
sig/api-machinery
| // if the context expires sooner than our hard requestTimeout use it, | ||
| // otherwise set the request timeout to whatever is defined in requestTimeout | ||
| if w.requestTimeout > 0 { | ||
| if oldCtx := ctx; timeBeforeContextDeadline(time.Now().Add(w.requestTimeout), oldCtx) { |
There was a problem hiding this comment.
alternatively, we could only set a deadline iff it wasn't set in the context
There was a problem hiding this comment.
I think we can do the following, since contexts are hierarchical:
if w.requestTimeout > 0 {
child, cancel := context.WithTimoeut(ctx, w.requestTimeout)
defer cancel()
ctx = child
}
if the child has a shorter deadline then it will expire first, otherwise if the parent has a shorter deadline then the parent will expire it will cause the child to expire as well even though the child has a larger deadline.
I think that's how it should work, I would also recommend adding a unit test to assert this behavior. :)
There was a problem hiding this comment.
yes, that would work too :)
p0lyn0mial
force-pushed
the
upstream-delegated-authn-timeout
branch
from
63a2068 to
65166f9
Compare
|
I would like to see an integration test that proves (somehow) that we don't have short timeout watches. The load created by these is fairly significant in some clusters. |
I added an integration test to #101022 |
|
/retest |
| // if the context expires sooner than our hard requestTimeout use it, | ||
| // otherwise set the request timeout to whatever is defined in requestTimeout | ||
| if w.requestTimeout > 0 { | ||
| if oldCtx := ctx; timeBeforeContextDeadline(time.Now().Add(w.requestTimeout), oldCtx) { |
There was a problem hiding this comment.
I think @tkashem's suggestion is easier to read.
| @@ -214,7 +214,7 @@ func NewDelegatingAuthenticationOptions() *DelegatingAuthenticationOptions { | |||
| ExtraHeaderPrefixes: []string{"x-remote-extra-"}, | |||
| }, | |||
| WebhookRetryBackoff: DefaultAuthWebhookRetryBackoff(), | |||
| ClientTimeout: 10 * time.Second, | |||
| RequestTimeout: 10 * time.Second, | |||
There was a problem hiding this comment.
This is narrowly now a TokenRequestTimeout, right?
| @@ -60,6 +60,9 @@ type DelegatingAuthenticatorConfig struct { | |||
| APIAudiences authenticator.Audiences | |||
|
|
|||
| RequestHeaderConfig *RequestHeaderConfig | |||
|
|
|||
| // RequestTimeout specifies a time limit for requests made by the authorization webhook client. | |||
| RequestTimeout time.Duration | |||
There was a problem hiding this comment.
TokenAccessReviewTimeout and move up to beneath the client
|
I'd like to tweak the names to indicate what they are for. But this approach is ok. |
p0lyn0mial
force-pushed
the
upstream-delegated-authn-timeout
branch
from
65166f9 to
49291e0
Compare
k8s-ci-robot
added
triage/accepted
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @cheftako for |
k8s-ci-robot
added
release-note
|
/approve [The changes in cloud-provider are mechanical.] |
k8s-ci-robot
added
the
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, dims, p0lyn0mial, wojtek-t 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:
Approvers can indicate their approval by writing |
/retest |
|
/retest |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
…100959-upstream-release-1.20 Automated cherry pick of #100959: DelegatingAuthenticationOptions TokenReview request timeout
…100959-upstream-release-1.21 Automated cherry pick of #100959: DelegatingAuthenticationOptions TokenReview request timeout
liggitt
added
the
kind/regression
What type of PR is this?
/kind bug
What this PR does / why we need it:
It turns out that setting a timeout on HTTP client affects watch requests made by the delegated authentication component.
With a 10 second timeout watch requests are being re-established exactly after 10 seconds even though the default request timeout for them is ~5 minutes.
This is because if multiple timeouts were set, the stdlib picks the smaller timeout to be applied, leaving others useless.
For more details see https://github.com/golang/go/blob/a937729c2c2f6950a32bc5cd0f5b88700882f078/src/net/http/client.go#L364
Instead of setting a timeout on the HTTP client, we should use context for cancellation.
This has the potential of being scattered across the codebase, perhaps we should seek a broader solution.
Here is a reproducer for re-establish watch requests when
http.Client.Timeoutis set with standardclient-golibraryp0lyn0mial/simple-watch#2
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: