Merge pull request #258 from kwilczynski/feature/backport-upstream-timeout

APPSRE-7970: Add support for configuring upstream timeout

Author: openshift-merge-robot

README.md

@@ -265,6 +265,7 @@ Usage of oauth-proxy:
   -tls-cert string: path to certificate file
   -tls-key string: path to private key file
   -upstream value: the http url(s) of the upstream endpoint or file:// paths for static files. Routing is based on the path
+  -upstream-timeout duration: maximum amount of time the server will wait for a response from the upstream (default 30s)
   -validate-url string: Access token validation endpoint
   -version: print version string
 ```

main.go

@@ -103,6 +103,8 @@ func main() {
 	flagSet.String("signature-key", "", "GAP-Signature request signature key (algorithm:secretkey)")
 	flagSet.Var(upstreamCAs, "upstream-ca", "paths to CA roots for the Upstream (target) Server (may be given multiple times, defaults to system trust store).")
 
+	flagSet.Duration("upstream-timeout", DefaultUpstreamTimeout, "maximum amount of time the server will wait for a response from the upstream")
+
 	providerOpenShift := openshift.New()
 	providerOpenShift.Bind(flagSet)
 

oauthproxy.go

@@ -109,27 +109,34 @@ func (u *UpstreamProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 }
 
-func NewReverseProxy(target *url.URL, upstreamFlush time.Duration, rootCAs []string) (*httputil.ReverseProxy, error) {
+func NewReverseProxy(target *url.URL, opts *Options) (*httputil.ReverseProxy, error) {
 	proxy := httputil.NewSingleHostReverseProxy(target)
-	proxy.FlushInterval = upstreamFlush
+	proxy.FlushInterval = opts.UpstreamFlush
 
-	transport := &http.Transport{
-		MaxIdleConnsPerHost: 500,
-		IdleConnTimeout:     1 * time.Minute,
-	}
-	if len(rootCAs) > 0 {
-		pool, err := util.GetCertPool(rootCAs, false)
+	// Inherit default transport options from Go's stdlib
+	transport := http.DefaultTransport.(*http.Transport).Clone()
+
+	transport.MaxIdleConnsPerHost = 500
+	transport.IdleConnTimeout = 1 * time.Minute
+
+	// Change default duration for waiting for an upstream response
+	transport.ResponseHeaderTimeout = opts.Timeout
+
+	if len(opts.UpstreamCAs) > 0 {
+		pool, err := util.GetCertPool(opts.UpstreamCAs, false)
 		if err != nil {
 			return nil, err
 		}
 		transport.TLSClientConfig = oscrypto.SecureTLSConfig(&tls.Config{RootCAs: pool})
 	}
 	if err := http2.ConfigureTransport(transport); err != nil {
-		if len(rootCAs) > 0 {
+		if len(opts.UpstreamCAs) > 0 {
 			return nil, err
 		}
 		log.Printf("WARN: Failed to configure http2 transport: %v", err)
 	}
+
+	// Apply the customized transport to our proxy before returning it
 	proxy.Transport = transport
 
 	return proxy, nil
@@ -160,7 +167,7 @@ func NewFileServer(path string, filesystemPath string) (proxy http.Handler) {
 
 func NewWebSocketOrRestReverseProxy(u *url.URL, opts *Options, auth hmacauth.HmacAuth) (restProxy http.Handler) {
 	u.Path = ""
-	proxy, err := NewReverseProxy(u, opts.UpstreamFlush, opts.UpstreamCAs)
+	proxy, err := NewReverseProxy(u, opts)
 	if err != nil {
 		log.Fatal("Failed to initialize Reverse Proxy: ", err)
 	}

oauthproxy_test.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/18F/hmacauth"
 	"github.com/bmizerany/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/openshift/oauth-proxy/providers"
 	"golang.org/x/net/websocket"
@@ -105,55 +106,99 @@ func TestWebSocketProxy(t *testing.T) {
 }
 
 func TestNewReverseProxy(t *testing.T) {
+	opts := NewOptions()
+	opts.UpstreamFlush = 5 * time.Millisecond
+
 	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		hostname, _, _ := net.SplitHostPort(r.Host)
 		w.Write([]byte(hostname))
 	}))
 	defer backend.Close()
 
-	backendURL, _ := url.Parse(backend.URL)
-	backendHostname, backendPort, _ := net.SplitHostPort(backendURL.Host)
-	backendHost := net.JoinHostPort(backendHostname, backendPort)
-	proxyURL, _ := url.Parse(backendURL.Scheme + "://" + backendHost + "/")
+	proxyURL, err := url.Parse(backend.URL)
+	require.NoError(t, err)
+
+	backendHost, _, err := net.SplitHostPort(proxyURL.Host)
+	require.NoError(t, err)
+
+	proxyHandler, err := NewReverseProxy(proxyURL, opts)
+	require.NoError(t, err)
 
-	proxyHandler, _ := NewReverseProxy(proxyURL, 5*time.Millisecond, nil)
 	setProxyUpstreamHostHeader(proxyHandler, proxyURL)
 	frontend := httptest.NewServer(proxyHandler)
 	defer frontend.Close()
 
-	getReq, _ := http.NewRequest("GET", frontend.URL, nil)
-	res, _ := http.DefaultClient.Do(getReq)
-	bodyBytes, _ := ioutil.ReadAll(res.Body)
-	if g, e := string(bodyBytes), backendHostname; g != e {
-		t.Errorf("got body %q; expected %q", g, e)
-	}
+	getReq, err := http.NewRequest("GET", frontend.URL, nil)
+	require.NoError(t, err)
+
+	res, err := http.DefaultClient.Do(getReq)
+	require.NoError(t, err)
+
+	bodyBytes, err := ioutil.ReadAll(res.Body)
+	require.NoError(t, err)
+
+	assert.Equal(t, backendHost, string(bodyBytes))
 }
 
 func TestEncodedSlashes(t *testing.T) {
+	opts := NewOptions()
+	opts.UpstreamFlush = 5 * time.Millisecond
+
 	var seen string
 	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		seen = r.RequestURI
 	}))
 	defer backend.Close()
 
-	b, _ := url.Parse(backend.URL)
-	proxyHandler, _ := NewReverseProxy(b, 5*time.Millisecond, nil)
+	b, err := url.Parse(backend.URL)
+	require.NoError(t, err)
+
+	proxyHandler, err := NewReverseProxy(b, opts)
+	require.NoError(t, err)
+
 	setProxyDirector(proxyHandler)
 	frontend := httptest.NewServer(proxyHandler)
 	defer frontend.Close()
 
-	f, _ := url.Parse(frontend.URL)
+	f, err := url.Parse(frontend.URL)
+	require.NoError(t, err)
+
 	encodedPath := "/a%2Fb/?c=1"
 	getReq := &http.Request{URL: &url.URL{Scheme: "http", Host: f.Host, Opaque: encodedPath}}
-	_, err := http.DefaultClient.Do(getReq)
-	if err != nil {
-		t.Fatalf("err %s", err)
-	}
-	if seen != encodedPath {
-		t.Errorf("got bad request %q expected %q", seen, encodedPath)
-	}
+	_, err = http.DefaultClient.Do(getReq)
+	require.NoError(t, err)
+
+	assert.Equal(t, encodedPath, seen)
+}
+
+func TestNewReverseProxyWithTimeOut(t *testing.T) {
+	opts := NewOptions()
+	opts.Timeout = 1 * time.Millisecond
+
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(5 * time.Millisecond)
+	}))
+	defer backend.Close()
+
+	proxyURL, err := url.Parse(backend.URL)
+	require.NoError(t, err)
+
+	proxyHandler, err := NewReverseProxy(proxyURL, opts)
+	require.NoError(t, err)
+
+	setProxyUpstreamHostHeader(proxyHandler, proxyURL)
+	frontend := httptest.NewServer(proxyHandler)
+	defer frontend.Close()
+
+	getReq, err := http.NewRequest("GET", frontend.URL, nil)
+	require.NoError(t, err)
+
+	res, err := http.DefaultClient.Do(getReq)
+	require.NoError(t, err)
+
+	assert.Equal(t, "502 Bad Gateway", res.Status)
 }
 
 func TestRobotsTxt(t *testing.T) {

options.go

@@ -20,6 +20,11 @@ import (
 	"github.com/openshift/oauth-proxy/providers/openshift"
 )
 
+const (
+	// DefaultUpstreamTimeout is the maximum duration a network dial to a upstream server for a response.
+	DefaultUpstreamTimeout = 30 * time.Second
+)
+
 // Configuration Options that can be set by Command Line Flag, or Config File
 type Options struct {
 	ProxyPrefix      string        `flag:"proxy-prefix" cfg:"proxy-prefix"`
@@ -101,6 +106,10 @@ type Options struct {
 	// provider to destroy their single sign-on session.
 	LogoutRedirectURL string `flag:"logout-url" cfg:"logout_url"`
 
+	// Timeout is the maximum duration the server will wait for a response from the upstream server.
+	// Defaults to 30 seconds.
+	Timeout time.Duration `flag:"upstream-timeout" cfg:"upstream_timeout"`
+
 	// internal values that are set after config validation
 	redirectURL       *url.URL
 	proxyURLs         []*url.URL
@@ -137,6 +146,7 @@ func NewOptions() *Options {
 		PassHostHeader:      true,
 		ApprovalPrompt:      "force",
 		RequestLogging:      true,
+		Timeout:             DefaultUpstreamTimeout,
 	}
 }