From 0090e27bd3a271e199a232296b45e120dfa9808a Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Wed, 18 Nov 2020 12:01:27 -0500 Subject: [PATCH 1/2] use default value when the specified timeout is 0s --- .../apiserver/pkg/endpoints/filters/request_deadline.go | 5 ++++- .../pkg/endpoints/filters/request_deadline_test.go | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline.go index 82b8d8d6c31..9e1f0c1e992 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline.go @@ -69,7 +69,10 @@ func WithRequestDeadline(handler http.Handler, longRunning request.LongRunningRe return } - timeout = userSpecifiedTimeout + // if the user has specified a timeout of 0s, this means no timeout, so we should use the maximum timeout allowed. + if userSpecifiedTimeout != 0 { + timeout = userSpecifiedTimeout + } } ctx, cancel := context.WithTimeout(ctx, timeout) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline_test.go index a3739dabe60..9ece6149ba1 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/request_deadline_test.go @@ -94,6 +94,15 @@ func TestWithRequestDeadline(t *testing.T) { deadlineExpected: 14 * time.Second, // to account for the delay in verification statusCodeExpected: http.StatusOK, }, + { + name: "the specified timeout is 0s, default deadline is expected to be set", + requestURL: "/api/v1/namespaces?timeout=0s", + longRunning: false, + handlerCallCountExpected: 1, + hasDeadlineExpected: true, + deadlineExpected: requestTimeoutMaximum - time.Second, // to account for the delay in verification + statusCodeExpected: http.StatusOK, + }, { name: "the user does not specify any request timeout, default deadline is expected to be set", requestURL: "/api/v1/namespaces?timeout=", From 2e6cb784d4d6a4fc869178b2e511be50db694223 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Wed, 18 Nov 2020 12:01:49 -0500 Subject: [PATCH 2/2] add e2e tests for request timeout --- test/e2e/apimachinery/BUILD | 1 + test/e2e/apimachinery/request_timeout.go | 113 +++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 test/e2e/apimachinery/request_timeout.go diff --git a/test/e2e/apimachinery/BUILD b/test/e2e/apimachinery/BUILD index 5feb3fcc5e6..91421fe07e8 100644 --- a/test/e2e/apimachinery/BUILD +++ b/test/e2e/apimachinery/BUILD @@ -25,6 +25,7 @@ go_library( "health_handlers.go", "namespace.go", "protocol.go", + "request_timeout.go", "resource_quota.go", "server_version.go", "table_conversion.go", diff --git a/test/e2e/apimachinery/request_timeout.go b/test/e2e/apimachinery/request_timeout.go new file mode 100644 index 00000000000..38a6341c807 --- /dev/null +++ b/test/e2e/apimachinery/request_timeout.go @@ -0,0 +1,113 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package apimachinery + +import ( + "io/ioutil" + "net/http" + "strings" + + "github.com/onsi/ginkgo" + "k8s.io/client-go/rest" + "k8s.io/kubernetes/test/e2e/framework" +) + +const ( + invalidTimeoutMessageExpected = "invalid timeout specified in the request URL" + timeoutExceedsMaximumExpected = "timeout specified in the request URL exceeds the maximum timeout allowed by the server" +) + +var _ = SIGDescribe("Server request timeout", func() { + f := framework.NewDefaultFramework("request-timeout") + + ginkgo.It("should return HTTP status code 400 if the user specifies an invalid timeout in the request URL", func() { + rt := getRoundTripper(f) + req := newRequest(f, "invalid") + + response, err := rt.RoundTrip(req) + framework.ExpectNoError(err) + defer response.Body.Close() + + if response.StatusCode != http.StatusBadRequest { + framework.Failf("expected HTTP status code: %d, but got: %d", http.StatusBadRequest, response.StatusCode) + } + + messageGot := readBody(response) + if !strings.Contains(messageGot, invalidTimeoutMessageExpected) { + framework.Failf("expected HTTP status message to contain: %s, but got: %s", invalidTimeoutMessageExpected, messageGot) + } + }) + + ginkgo.It("should return HTTP status code 400 if the specified timeout in the request URL exceeds maximum allowed", func() { + rt := getRoundTripper(f) + // Choose a timeout that exceeds the default timeout (60s) enforced by the apiserver + req := newRequest(f, "3m") + + response, err := rt.RoundTrip(req) + framework.ExpectNoError(err) + defer response.Body.Close() + + if response.StatusCode != http.StatusBadRequest { + framework.Failf("expected HTTP status code: %d, but got: %d", http.StatusBadRequest, response.StatusCode) + } + + messageGot := readBody(response) + if !strings.Contains(messageGot, timeoutExceedsMaximumExpected) { + framework.Failf("expected HTTP status message to contain: %s, but got: %s", timeoutExceedsMaximumExpected, messageGot) + } + }) + + ginkgo.It("default timeout should be used if the specified timeout in the request URL is 0s", func() { + rt := getRoundTripper(f) + req := newRequest(f, "0s") + + response, err := rt.RoundTrip(req) + framework.ExpectNoError(err) + defer response.Body.Close() + + if response.StatusCode != http.StatusOK { + framework.Failf("expected HTTP status code: %d, but got: %d", http.StatusOK, response.StatusCode) + } + }) +}) + +func getRoundTripper(f *framework.Framework) http.RoundTripper { + config := rest.CopyConfig(f.ClientConfig()) + + // ensure we don't enforce any transport timeout from the client side. + config.Timeout = 0 + + roundTripper, err := rest.TransportFor(config) + framework.ExpectNoError(err) + + return roundTripper +} + +func newRequest(f *framework.Framework, timeout string) *http.Request { + req, err := http.NewRequest(http.MethodGet, f.ClientSet.CoreV1().RESTClient().Get(). + Param("timeout", timeout).AbsPath("version").URL().String(), nil) + framework.ExpectNoError(err) + + return req +} + +func readBody(response *http.Response) string { + raw, err := ioutil.ReadAll(response.Body) + framework.ExpectNoError(err) + + return string(raw) +}