From 2de3ee5c48503d3b3214aef55ae7fd0dacc40457 Mon Sep 17 00:00:00 2001
From: Eric Chiang <eric.chiang.m@gmail.com>
Date: Wed, 11 Oct 2017 17:44:29 -0700
Subject: [PATCH] generic webhook: set a default timeout for webhook requests

Add a 30 second timeout for all HTTP requests that the webhook sends
so they timeout instead of hanging forever.
---
 .../apiserver/pkg/util/webhook/webhook.go     | 18 +++++-
 .../pkg/util/webhook/webhook_test.go          | 59 +++++++++++++++++++
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go
index b1a897a193a..3dc241813a2 100755
--- a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go
+++ b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook.go
@@ -32,6 +32,10 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 )
 
+// defaultRequestTimeout is set for all webhook request. This is the absolute
+// timeout of the HTTP request, including reading the response body.
+const defaultRequestTimeout = 30 * time.Second
+
 type GenericWebhook struct {
 	RestClient     *rest.RESTClient
 	initialBackoff time.Duration
@@ -39,6 +43,10 @@ type GenericWebhook struct {
 
 // NewGenericWebhook creates a new GenericWebhook from the provided kubeconfig file.
 func NewGenericWebhook(registry *registered.APIRegistrationManager, codecFactory serializer.CodecFactory, kubeConfigFile string, groupVersions []schema.GroupVersion, initialBackoff time.Duration) (*GenericWebhook, error) {
+	return newGenericWebhook(registry, codecFactory, kubeConfigFile, groupVersions, initialBackoff, defaultRequestTimeout)
+}
+
+func newGenericWebhook(registry *registered.APIRegistrationManager, codecFactory serializer.CodecFactory, kubeConfigFile string, groupVersions []schema.GroupVersion, initialBackoff, requestTimeout time.Duration) (*GenericWebhook, error) {
 	for _, groupVersion := range groupVersions {
 		if !registry.IsEnabledVersion(groupVersion) {
 			return nil, fmt.Errorf("webhook plugin requires enabling extension resource: %s", groupVersion)
@@ -53,6 +61,14 @@ func NewGenericWebhook(registry *registered.APIRegistrationManager, codecFactory
 	if err != nil {
 		return nil, err
 	}
+
+	// Kubeconfigs can't set a timeout, this can only be set through a command line flag.
+	//
+	// https://github.com/kubernetes/client-go/blob/master/tools/clientcmd/overrides.go
+	//
+	// Set this to something reasonable so request to webhooks don't hang forever.
+	clientConfig.Timeout = requestTimeout
+
 	codec := codecFactory.LegacyCodec(groupVersions...)
 	clientConfig.ContentConfig.NegotiatedSerializer = runtimeserializer.NegotiatedSerializerWrapper(runtime.SerializerInfo{Serializer: codec})
 
@@ -61,8 +77,6 @@ func NewGenericWebhook(registry *registered.APIRegistrationManager, codecFactory
 		return nil, err
 	}
 
-	// TODO(ericchiang): Can we ensure remote service is reachable?
-
 	return &GenericWebhook{restClient, initialBackoff}, nil
 }
 
diff --git a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go
index d3eb0593624..f47e088beeb 100644
--- a/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/util/webhook/webhook_test.go
@@ -428,6 +428,65 @@ func TestTLSConfig(t *testing.T) {
 	}
 }
 
+func TestRequestTimeout(t *testing.T) {
+	done := make(chan struct{})
+
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		<-done
+		return
+	}
+
+	// Create and start a simple HTTPS server
+	server, err := newTestServer(clientCert, clientKey, caCert, handler)
+	if err != nil {
+		t.Errorf("failed to create server: %v", err)
+		return
+	}
+	defer server.Close()
+	defer close(done) // done channel must be closed before server is.
+
+	// Create a Kubernetes client configuration file
+	configFile, err := newKubeConfigFile(v1.Config{
+		Clusters: []v1.NamedCluster{
+			{
+				Cluster: v1.Cluster{
+					Server: server.URL,
+					CertificateAuthorityData: caCert,
+				},
+			},
+		},
+		AuthInfos: []v1.NamedAuthInfo{
+			{
+				AuthInfo: v1.AuthInfo{
+					ClientCertificateData: clientCert,
+					ClientKeyData:         clientKey,
+				},
+			},
+		},
+	})
+	if err != nil {
+		t.Errorf("failed to create the client config file: %v", err)
+		return
+	}
+	defer os.Remove(configFile)
+
+	var requestTimeout = 10 * time.Millisecond
+
+	wh, err := newGenericWebhook(registered.NewOrDie(""), scheme.Codecs, configFile, groupVersions, retryBackoff, requestTimeout)
+	if err != nil {
+		t.Fatalf("failed to create the webhook: %v", err)
+	}
+
+	resultCh := make(chan rest.Result)
+
+	go func() { resultCh <- wh.RestClient.Get().Do() }()
+	select {
+	case <-time.After(time.Second * 5):
+		t.Errorf("expected request to timeout after %s", requestTimeout)
+	case <-resultCh:
+	}
+}
+
 // TestWithExponentialBackoff ensures that the webhook's exponential backoff support works as expected
 func TestWithExponentialBackoff(t *testing.T) {
 	count := 0 // To keep track of the requests