Don't follow non-local redirects for http probes

pkg/probe/http/BUILD

@@ -22,7 +22,12 @@ go_test(
     name = "go_default_test",
     srcs = ["http_test.go"],
     embed = [":go_default_library"],
-    deps = ["//pkg/probe:go_default_library"],
+    deps = [
+        "//pkg/probe:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
+    ],
 )
 
 filegroup(

pkg/probe/http/http.go

@@ -18,6 +18,7 @@ package http
 
 import (
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -32,13 +33,17 @@ import (
 )
 
 // New creates Prober that will skip TLS verification while probing.
-func New() Prober {
+// followNonLocalRedirects configures whether the prober should follow redirects to a different hostname.
+//   If disabled, redirects to other hosts will trigger a warning result.
+func New(followNonLocalRedirects bool) Prober {
 	tlsConfig := &tls.Config{InsecureSkipVerify: true}
-	return NewWithTLSConfig(tlsConfig)
+	return NewWithTLSConfig(tlsConfig, followNonLocalRedirects)
 }
 
 // NewWithTLSConfig takes tls config as parameter.
-func NewWithTLSConfig(config *tls.Config) Prober {
+// followNonLocalRedirects configures whether the prober should follow redirects to a different hostname.
+//   If disabled, redirects to other hosts will trigger a warning result.
+func NewWithTLSConfig(config *tls.Config, followNonLocalRedirects bool) Prober {
 	// We do not want the probe use node's local proxy set.
 	transport := utilnet.SetTransportDefaults(
 		&http.Transport{
@@ -46,7 +51,7 @@ func NewWithTLSConfig(config *tls.Config) Prober {
 			DisableKeepAlives: true,
 			Proxy:             http.ProxyURL(nil),
 		})
-	return httpProber{transport}
+	return httpProber{transport, followNonLocalRedirects}
 }
 
 // Prober is an interface that defines the Probe function for doing HTTP readiness/liveness checks.
@@ -55,12 +60,18 @@ type Prober interface {
 }
 
 type httpProber struct {
-	transport *http.Transport
+	transport               *http.Transport
+	followNonLocalRedirects bool
 }
 
 // Probe returns a ProbeRunner capable of running an HTTP check.
 func (pr httpProber) Probe(url *url.URL, headers http.Header, timeout time.Duration) (probe.Result, string, error) {
-	return DoHTTPProbe(url, headers, &http.Client{Timeout: timeout, Transport: pr.transport})
+	client := &http.Client{
+		Timeout:       timeout,
+		Transport:     pr.transport,
+		CheckRedirect: redirectChecker(pr.followNonLocalRedirects),
+	}
+	return DoHTTPProbe(url, headers, client)
 }
 
 // GetHTTPInterface is an interface for making HTTP requests, that returns a response and error.
@@ -102,9 +113,30 @@ func DoHTTPProbe(url *url.URL, headers http.Header, client GetHTTPInterface) (pr
 	}
 	body := string(b)
 	if res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusBadRequest {
+		if res.StatusCode >= http.StatusMultipleChoices { // Redirect
+			klog.V(4).Infof("Probe terminated redirects for %s, Response: %v", url.String(), *res)
+			return probe.Warning, body, nil
+		}
 		klog.V(4).Infof("Probe succeeded for %s, Response: %v", url.String(), *res)
 		return probe.Success, body, nil
 	}
 	klog.V(4).Infof("Probe failed for %s with request headers %v, response body: %v", url.String(), headers, body)
 	return probe.Failure, fmt.Sprintf("HTTP probe failed with statuscode: %d", res.StatusCode), nil
 }
+
+func redirectChecker(followNonLocalRedirects bool) func(*http.Request, []*http.Request) error {
+	if followNonLocalRedirects {
+		return nil // Use the default http client checker.
+	}
+
+	return func(req *http.Request, via []*http.Request) error {
+		if req.URL.Hostname() != via[0].URL.Hostname() {
+			return http.ErrUseLastResponse
+		}
+		// Default behavior: stop after 10 redirects.
+		if len(via) >= 10 {
+			return errors.New("stopped after 10 redirects")
+		}
+		return nil
+	}
+}

pkg/probe/http/http_test.go

@@ -28,6 +28,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/probe"
 )
 
@@ -64,7 +67,7 @@ func TestHTTPProbeProxy(t *testing.T) {
 	defer unsetEnv("no_proxy")()
 	defer unsetEnv("NO_PROXY")()
 
-	prober := New()
+	prober := New(true)
 
 	go func() {
 		http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
@@ -119,7 +122,7 @@ func TestHTTPProbeChecker(t *testing.T) {
 		}
 	}
 
-	prober := New()
+	prober := New(true)
 	testCases := []struct {
 		handler    func(w http.ResponseWriter, r *http.Request)
 		reqHeaders http.Header
@@ -223,7 +226,7 @@ func TestHTTPProbeChecker(t *testing.T) {
 		},
 	}
 	for i, test := range testCases {
-		func() {
+		t.Run(fmt.Sprintf("case-%2d", i), func(t *testing.T) {
 			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				test.handler(w, r)
 			}))
@@ -258,6 +261,56 @@ func TestHTTPProbeChecker(t *testing.T) {
 					t.Errorf("Expected response not to contain %v, got %v", test.notBody, output)
 				}
 			}
-		}()
+		})
+	}
+}
+
+func TestHTTPProbeChecker_NonLocalRedirects(t *testing.T) {
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/redirect":
+			loc, _ := url.QueryUnescape(r.URL.Query().Get("loc"))
+			http.Redirect(w, r, loc, http.StatusFound)
+		case "/loop":
+			http.Redirect(w, r, "/loop", http.StatusFound)
+		case "/success":
+			w.WriteHeader(http.StatusOK)
+		default:
+			http.Error(w, "", http.StatusInternalServerError)
+		}
+	})
+	server := httptest.NewServer(handler)
+	defer server.Close()
+
+	newportServer := httptest.NewServer(handler)
+	defer newportServer.Close()
+
+	testCases := map[string]struct {
+		redirect             string
+		expectLocalResult    probe.Result
+		expectNonLocalResult probe.Result
+	}{
+		"local success":   {"/success", probe.Success, probe.Success},
+		"local fail":      {"/fail", probe.Failure, probe.Failure},
+		"newport success": {newportServer.URL + "/success", probe.Success, probe.Success},
+		"newport fail":    {newportServer.URL + "/fail", probe.Failure, probe.Failure},
+		"bogus nonlocal":  {"http://0.0.0.0/fail", probe.Warning, probe.Failure},
+		"redirect loop":   {"/loop", probe.Failure, probe.Failure},
+	}
+	for desc, test := range testCases {
+		t.Run(desc+"-local", func(t *testing.T) {
+			prober := New(false)
+			target, err := url.Parse(server.URL + "/redirect?loc=" + url.QueryEscape(test.redirect))
+			require.NoError(t, err)
+			result, _, _ := prober.Probe(target, nil, wait.ForeverTestTimeout)
+			assert.Equal(t, test.expectLocalResult, result)
+		})
+		t.Run(desc+"-nonlocal", func(t *testing.T) {
+			prober := New(true)
+			target, err := url.Parse(server.URL + "/redirect?loc=" + url.QueryEscape(test.redirect))
+			require.NoError(t, err)
+			result, _, _ := prober.Probe(target, nil, wait.ForeverTestTimeout)
+			assert.Equal(t, test.expectNonLocalResult, result)
+		})
 	}
 }

pkg/probe/probe.go

@@ -22,6 +22,8 @@ type Result string
 const (
 	// Success Result
 	Success Result = "success"
+	// Warning Result. Logically success, but with additional debugging information attached.
+	Warning Result = "warning"
 	// Failure Result
 	Failure Result = "failure"
 	// Unknown Result