From e9372dcd11349841fac086f40a4431f47fcc57ae Mon Sep 17 00:00:00 2001 From: Lukasz Szaszkiewicz Date: Fri, 9 Apr 2021 13:20:51 +0200 Subject: [PATCH] DelegatingAuthenticationOptions TokenReview request timeout it turns out that setting a timeout on HTTP client affect 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 other 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. --- .../app/options/options_test.go | 2 +- .../authenticatorfactory/delegating.go | 5 +++- .../pkg/server/options/authentication.go | 24 ++++++++++------- .../authenticator/token/webhook/webhook.go | 27 +++++++++++++------ .../token/webhook/webhook_v1_test.go | 2 +- .../token/webhook/webhook_v1beta1_test.go | 2 +- .../cloud-provider/options/options_test.go | 4 +-- 7 files changed, 42 insertions(+), 24 deletions(-) diff --git a/cmd/kube-controller-manager/app/options/options_test.go b/cmd/kube-controller-manager/app/options/options_test.go index a04f30853a524..84dff1401547d 100644 --- a/cmd/kube-controller-manager/app/options/options_test.go +++ b/cmd/kube-controller-manager/app/options/options_test.go @@ -412,7 +412,7 @@ func TestAddFlags(t *testing.T) { }).WithLoopback(), Authentication: &apiserveroptions.DelegatingAuthenticationOptions{ CacheTTL: 10 * time.Second, - ClientTimeout: 10 * time.Second, + TokenRequestTimeout: 10 * time.Second, WebhookRetryBackoff: apiserveroptions.DefaultAuthWebhookRetryBackoff(), ClientCert: apiserveroptions.ClientCertAuthenticationOptions{}, RequestHeader: apiserveroptions.RequestHeaderAuthenticationOptions{ diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go index 83697bb5470b3..4b7105697dc23 100644 --- a/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go +++ b/staging/src/k8s.io/apiserver/pkg/authentication/authenticatorfactory/delegating.go @@ -44,6 +44,9 @@ type DelegatingAuthenticatorConfig struct { // TokenAccessReviewClient is a client to do token review. It can be nil. Then every token is ignored. TokenAccessReviewClient authenticationclient.TokenReviewInterface + // TokenAccessReviewTimeout specifies a time limit for requests made by the authorization webhook client. + TokenAccessReviewTimeout time.Duration + // WebhookRetryBackoff specifies the backoff parameters for the authentication webhook retry logic. // This allows us to configure the sleep time at each iteration and the maximum number of retries allowed // before we fail the webhook call in order to limit the fan out that ensues when the system is degraded. @@ -88,7 +91,7 @@ func (c DelegatingAuthenticatorConfig) New() (authenticator.Request, *spec.Secur if c.WebhookRetryBackoff == nil { return nil, nil, errors.New("retry backoff parameters for delegating authentication webhook has not been specified") } - tokenAuth, err := webhooktoken.NewFromInterface(c.TokenAccessReviewClient, c.APIAudiences, *c.WebhookRetryBackoff) + tokenAuth, err := webhooktoken.NewFromInterface(c.TokenAccessReviewClient, c.APIAudiences, *c.WebhookRetryBackoff, c.TokenAccessReviewTimeout) if err != nil { return nil, nil, err } diff --git a/staging/src/k8s.io/apiserver/pkg/server/options/authentication.go b/staging/src/k8s.io/apiserver/pkg/server/options/authentication.go index 75e9c8dffd1a0..423da84756c8e 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/options/authentication.go +++ b/staging/src/k8s.io/apiserver/pkg/server/options/authentication.go @@ -195,9 +195,9 @@ type DelegatingAuthenticationOptions struct { // before we fail the webhook call in order to limit the fan out that ensues when the system is degraded. WebhookRetryBackoff *wait.Backoff - // ClientTimeout specifies a time limit for requests made by the authorization webhook client. + // TokenRequestTimeout specifies a time limit for requests made by the authorization webhook client. // The default value is set to 10 seconds. - ClientTimeout time.Duration + TokenRequestTimeout time.Duration } func NewDelegatingAuthenticationOptions() *DelegatingAuthenticationOptions { @@ -211,7 +211,7 @@ func NewDelegatingAuthenticationOptions() *DelegatingAuthenticationOptions { ExtraHeaderPrefixes: []string{"x-remote-extra-"}, }, WebhookRetryBackoff: DefaultAuthWebhookRetryBackoff(), - ClientTimeout: 10 * time.Second, + TokenRequestTimeout: 10 * time.Second, } } @@ -220,9 +220,9 @@ func (s *DelegatingAuthenticationOptions) WithCustomRetryBackoff(backoff wait.Ba s.WebhookRetryBackoff = &backoff } -// WithClientTimeout sets the given timeout for the authentication webhook client. -func (s *DelegatingAuthenticationOptions) WithClientTimeout(timeout time.Duration) { - s.ClientTimeout = timeout +// WithRequestTimeout sets the given timeout for requests made by the authentication webhook client. +func (s *DelegatingAuthenticationOptions) WithRequestTimeout(timeout time.Duration) { + s.TokenRequestTimeout = timeout } func (s *DelegatingAuthenticationOptions) Validate() []error { @@ -274,9 +274,10 @@ func (s *DelegatingAuthenticationOptions) ApplyTo(authenticationInfo *server.Aut } cfg := authenticatorfactory.DelegatingAuthenticatorConfig{ - Anonymous: true, - CacheTTL: s.CacheTTL, - WebhookRetryBackoff: s.WebhookRetryBackoff, + Anonymous: true, + CacheTTL: s.CacheTTL, + WebhookRetryBackoff: s.WebhookRetryBackoff, + TokenAccessReviewTimeout: s.TokenRequestTimeout, } client, err := s.getClient() @@ -419,7 +420,10 @@ func (s *DelegatingAuthenticationOptions) getClient() (kubernetes.Interface, err // set high qps/burst limits since this will effectively limit API server responsiveness clientConfig.QPS = 200 clientConfig.Burst = 400 - clientConfig.Timeout = s.ClientTimeout + // do not set a timeout on the http client, instead use context for cancellation + // if multiple timeouts were set, the request will pick the smaller timeout to be applied, leaving other useless. + // + // see https://github.com/golang/go/blob/a937729c2c2f6950a32bc5cd0f5b88700882f078/src/net/http/client.go#L364 return kubernetes.NewForConfig(clientConfig) } diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go index 5bedf4e5985f6..41dd7a69e9b97 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook.go @@ -52,17 +52,18 @@ type tokenReviewer interface { } type WebhookTokenAuthenticator struct { - tokenReview tokenReviewer - retryBackoff wait.Backoff - implicitAuds authenticator.Audiences + tokenReview tokenReviewer + retryBackoff wait.Backoff + implicitAuds authenticator.Audiences + requestTimeout time.Duration } // NewFromInterface creates a webhook authenticator using the given tokenReview // client. It is recommend to wrap this authenticator with the token cache // authenticator implemented in // k8s.io/apiserver/pkg/authentication/token/cache. -func NewFromInterface(tokenReview authenticationv1client.TokenReviewInterface, implicitAuds authenticator.Audiences, retryBackoff wait.Backoff) (*WebhookTokenAuthenticator, error) { - return newWithBackoff(tokenReview, retryBackoff, implicitAuds) +func NewFromInterface(tokenReview authenticationv1client.TokenReviewInterface, implicitAuds authenticator.Audiences, retryBackoff wait.Backoff, requestTimeout time.Duration) (*WebhookTokenAuthenticator, error) { + return newWithBackoff(tokenReview, retryBackoff, implicitAuds, requestTimeout) } // New creates a new WebhookTokenAuthenticator from the provided kubeconfig @@ -74,12 +75,12 @@ func New(kubeConfigFile string, version string, implicitAuds authenticator.Audie if err != nil { return nil, err } - return newWithBackoff(tokenReview, retryBackoff, implicitAuds) + return newWithBackoff(tokenReview, retryBackoff, implicitAuds, time.Duration(0)) } // newWithBackoff allows tests to skip the sleep. -func newWithBackoff(tokenReview tokenReviewer, retryBackoff wait.Backoff, implicitAuds authenticator.Audiences) (*WebhookTokenAuthenticator, error) { - return &WebhookTokenAuthenticator{tokenReview, retryBackoff, implicitAuds}, nil +func newWithBackoff(tokenReview tokenReviewer, retryBackoff wait.Backoff, implicitAuds authenticator.Audiences, requestTimeout time.Duration) (*WebhookTokenAuthenticator, error) { + return &WebhookTokenAuthenticator{tokenReview, retryBackoff, implicitAuds, requestTimeout}, nil } // AuthenticateToken implements the authenticator.Token interface. @@ -105,7 +106,17 @@ func (w *WebhookTokenAuthenticator) AuthenticateToken(ctx context.Context, token var ( result *authenticationv1.TokenReview auds authenticator.Audiences + cancel context.CancelFunc ) + + // set a hard timeout if it was defined + // 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 and it will be propagate to the child + if w.requestTimeout > 0 { + ctx, cancel = context.WithTimeout(ctx, w.requestTimeout) + defer cancel() + } + // WithExponentialBackoff will return tokenreview create error (tokenReviewErr) if any. if err := webhook.WithExponentialBackoff(ctx, w.retryBackoff, func() error { var tokenReviewErr error diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1_test.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1_test.go index 913c5417050d4..004d1690bc8d8 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1_test.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1_test.go @@ -206,7 +206,7 @@ func newV1TokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte, return nil, err } - authn, err := newWithBackoff(c, testRetryBackoff, implicitAuds) + authn, err := newWithBackoff(c, testRetryBackoff, implicitAuds, 10*time.Second) if err != nil { return nil, err } diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1beta1_test.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1beta1_test.go index 93e1078e461a5..90e1cd9cf6ae6 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1beta1_test.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/webhook/webhook_v1beta1_test.go @@ -200,7 +200,7 @@ func newV1beta1TokenAuthenticator(serverURL string, clientCert, clientKey, ca [] return nil, err } - authn, err := newWithBackoff(c, testRetryBackoff, implicitAuds) + authn, err := newWithBackoff(c, testRetryBackoff, implicitAuds, 10*time.Second) if err != nil { return nil, err } diff --git a/staging/src/k8s.io/cloud-provider/options/options_test.go b/staging/src/k8s.io/cloud-provider/options/options_test.go index 2ea25bdcb33d2..80fa061077a50 100644 --- a/staging/src/k8s.io/cloud-provider/options/options_test.go +++ b/staging/src/k8s.io/cloud-provider/options/options_test.go @@ -104,7 +104,7 @@ func TestDefaultFlags(t *testing.T) { }).WithLoopback(), Authentication: &apiserveroptions.DelegatingAuthenticationOptions{ CacheTTL: 10 * time.Second, - ClientTimeout: 10 * time.Second, + TokenRequestTimeout: 10 * time.Second, WebhookRetryBackoff: apiserveroptions.DefaultAuthWebhookRetryBackoff(), ClientCert: apiserveroptions.ClientCertAuthenticationOptions{}, RequestHeader: apiserveroptions.RequestHeaderAuthenticationOptions{ @@ -240,7 +240,7 @@ func TestAddFlags(t *testing.T) { }).WithLoopback(), Authentication: &apiserveroptions.DelegatingAuthenticationOptions{ CacheTTL: 10 * time.Second, - ClientTimeout: 10 * time.Second, + TokenRequestTimeout: 10 * time.Second, WebhookRetryBackoff: apiserveroptions.DefaultAuthWebhookRetryBackoff(), ClientCert: apiserveroptions.ClientCertAuthenticationOptions{}, RequestHeader: apiserveroptions.RequestHeaderAuthenticationOptions{