Merge pull request #599 from smarthall/improve_proxy_experience

Speed up proxy selection code

Author: openshift-merge-bot[bot]

pkg/cli/config/config.go

@@ -1,11 +1,13 @@
 package config
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/url"
 	"os"
 	"path/filepath"
+	"sync"
 	"time"
 
 	logger "github.com/sirupsen/logrus"
@@ -52,6 +54,7 @@ const (
 	JiraConfigForAccessRequestsKey = "jira-config-for-access-requests"
 	prodEnvNameDefaultValue        = "production"
 	JiraBaseURLDefaultValue        = "https://issues.redhat.com"
+	proxyTestTimeout               = 10 * time.Second
 )
 
 var JiraConfigForAccessRequestsDefaultValue = AccessRequestsJiraConfiguration{
@@ -190,43 +193,115 @@ func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) {
 	return bpConfig, nil
 }
 
-var clientDo = func(client *http.Client, req *http.Request) (*http.Response, error) {
-	return client.Do(req)
+var testProxy = func(ctx context.Context, testURL string, proxyURL url.URL) error {
+	// Try call the test URL via the proxy
+	client := &http.Client{
+		Transport: &http.Transport{Proxy: http.ProxyURL(&proxyURL)},
+	}
+	req, _ := http.NewRequestWithContext(ctx, "GET", testURL, nil)
+	resp, err := client.Do(req)
+
+	// Check the result
+	if err != nil {
+		return fmt.Errorf("proxy returned an error %v", err)
+	}
+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("expected response code 200 but got %d", resp.StatusCode)
+	}
+
+	return nil
 }
 
 func (config *BackplaneConfiguration) getFirstWorkingProxyURL(s []string) string {
-	bpURL := config.URL + "/healthz"
+	if len(s) == 0 {
+		logger.Debug("No proxy to use")
+		return ""
+	}
 
-	client := &http.Client{
-		Timeout: 5 * time.Second,
+	// If we only have one proxy, there is no need to waste time on tests, just use that one
+	if len(s) == 1 {
+		logger.Debug("Only one proxy to choose from, automatically using it")
+		return s[0]
 	}
 
+	// Context to time out or cancel all tests once we are done
+	ctx, cancel := context.WithTimeout(context.Background(), proxyTestTimeout)
+	var wg sync.WaitGroup
+	ch := make(chan *url.URL)
+
+	bpURL := config.URL + "/healthz"
+
+	failures := 0
 	for _, p := range s {
+		// Parse the proxy URL
 		proxyURL, err := url.ParseRequestURI(p)
 		if err != nil {
 			logger.Debugf("proxy-url: '%v' could not be parsed.", p)
+			failures++
 			continue
 		}
 
-		client.Transport = &http.Transport{Proxy: http.ProxyURL(proxyURL)}
-		req, _ := http.NewRequest("GET", bpURL, nil)
-		resp, err := clientDo(client, req)
-		if err != nil {
-			logger.Infof("Proxy: %s returned an error: %s", proxyURL, err)
-			continue
-		}
-		if resp.StatusCode == http.StatusOK {
-			return p
-		}
-		logger.Infof("proxy: %s did not pass healthcheck, expected response code 200, got %d, discarding", p, resp.StatusCode)
+		wg.Add(1)
+		go func(proxyURL url.URL) {
+			defer wg.Done()
+
+			// Do the proxy test
+			proxyErr := testProxy(ctx, bpURL, proxyURL)
+			if proxyErr != nil {
+				logger.Infof("Discarding proxy %s due to error: %s", proxyURL.String(), proxyErr)
+				ch <- nil
+				return
+			}
+
+			// This test succeeded, send to the main thread
+			ch <- &proxyURL
+		}(*proxyURL)
 	}
 
-	if len(s) > 0 {
-		logger.Infof("falling back to first proxy-url after all proxies failed health checks: %s", s[0])
-		return s[0]
+	// Default to the first
+	chosenURL := s[0]
+
+	// Loop until all tests have failed or we get a single success
+loop:
+	for failures < len(s) {
+		select {
+		case proxyURL := <-ch: // A proxy returned a result
+			// nil means the test failed
+			if proxyURL == nil {
+				failures++
+				continue
+			}
+
+			// This proxy passed
+			chosenURL = proxyURL.String()
+			logger.Infof("proxy that responded first was %s", chosenURL)
+
+			break loop
+
+		case <-ctx.Done(): // We timed out waiting for a proxy to pass
+			logger.Warnf("falling back to first proxy-url after all proxies timed out: %s", s[0])
+
+			break loop
+		}
 	}
 
-	return ""
+	// Cancel any remaining requests
+	cancel()
+
+	// Ignore any other valid proxies, until the channel is closed
+	go func() {
+		for lateProxy := range ch {
+			if lateProxy != nil {
+				logger.Infof("proxy %s responded too late", lateProxy)
+			}
+		}
+	}()
+
+	// Wait for goroutines to end, then close the channel
+	wg.Wait()
+	close(ch)
+
+	return chosenURL
 }
 
 func validateConfig() error {

pkg/cli/config/config_test.go

@@ -1,8 +1,11 @@
 package config
 
 import (
+	"context"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"testing"
 
@@ -86,10 +89,10 @@ func TestGetBackplaneConnection(t *testing.T) {
 
 func TestBackplaneConfiguration_getFirstWorkingProxyURL(t *testing.T) {
 	tests := []struct {
-		name         string
-		proxies      []string
-		clientDoFunc func(client *http.Client, req *http.Request) (*http.Response, error)
-		want         string
+		name     string
+		proxies  []string
+		testFunc func(ctx context.Context, testURL string, proxyURL url.URL) error
+		want     string
 	}{
 		{
 			name:    "invalid-format-proxy",
@@ -102,26 +105,30 @@ func TestBackplaneConfiguration_getFirstWorkingProxyURL(t *testing.T) {
 			want:    "-",
 		},
 		{
-			name:    "valid-proxies",
+			name:    "single-valid-proxy",
 			proxies: []string{"https://proxy.invalid"},
-			clientDoFunc: func(client *http.Client, req *http.Request) (*http.Response, error) {
-				return &http.Response{StatusCode: http.StatusOK}, nil
+			testFunc: func(ctx context.Context, testURL string, proxyURL url.URL) error {
+				return nil
 			},
 			want: "https://proxy.invalid",
 		},
 		{
-			name:    "multiple-valid-proxies",
-			proxies: []string{"https://proxy.invalid", "https://dummy.proxy.invalid"},
-			clientDoFunc: func(client *http.Client, req *http.Request) (*http.Response, error) {
-				return &http.Response{StatusCode: http.StatusOK}, nil
+			name:    "one-proxy-fails",
+			proxies: []string{"http://this.proxy.succeeds", "http://this.proxy.fails"},
+			testFunc: func(ctx context.Context, testURL string, proxyURL url.URL) error {
+				if proxyURL.Host == "this.proxy.succeeds" {
+					return nil
+				}
+
+				return fmt.Errorf("Testing Error")
 			},
-			want: "https://proxy.invalid",
+			want: "http://this.proxy.succeeds",
 		},
 		{
 			name:    "multiple-mixed-proxies",
 			proxies: []string{"-", "gellso", "https://proxy.invalid"},
-			clientDoFunc: func(client *http.Client, req *http.Request) (*http.Response, error) {
-				return &http.Response{StatusCode: http.StatusOK}, nil
+			testFunc: func(ctx context.Context, testURL string, proxyURL url.URL) error {
+				return nil
 			},
 			want: "https://proxy.invalid",
 		},
@@ -132,7 +139,7 @@ func TestBackplaneConfiguration_getFirstWorkingProxyURL(t *testing.T) {
 				_, _ = w.Write([]byte("dummy data"))
 			}))
 
-			clientDo = tt.clientDoFunc
+			testProxy = tt.testFunc
 
 			config := &BackplaneConfiguration{
 				URL: svr.URL,