Merge pull request #75416 from tallclair/local-redirects

Don't follow non-local redirects from HTTP probes

pkg/kubelet/events/event.go

@@ -83,6 +83,7 @@ const (
 
 	// Probe event reason list
 	ContainerUnhealthy    = "Unhealthy"
+	ContainerProbeWarning = "ProbeWarning"
 
 	// Pod worker event reason list
 	FailedSync = "FailedSync"

pkg/kubelet/prober/prober.go

@@ -66,10 +66,11 @@ func newProber(
 	refManager *kubecontainer.RefManager,
 	recorder record.EventRecorder) *prober {
 
+	const followNonLocalRedirects = false
 	return &prober{
 		exec:          execprobe.New(),
-		readinessHttp: httprobe.New(),
-		livenessHttp:  httprobe.New(),
+		readinessHttp: httprobe.New(followNonLocalRedirects),
+		livenessHttp:  httprobe.New(followNonLocalRedirects),
 		tcp:           tcprobe.New(),
 		runner:        runner,
 		refManager:    refManager,
@@ -96,7 +97,7 @@ func (pb *prober) probe(probeType probeType, pod *v1.Pod, status v1.PodStatus, c
 	}
 
 	result, output, err := pb.runProbeWithRetries(probeType, probeSpec, pod, status, container, containerID, maxProbeRetries)
-	if err != nil || result != probe.Success {
+	if err != nil || (result != probe.Success && result != probe.Warning) {
 		// Probe failed in one way or another.
 		ref, hasRef := pb.refManager.GetRef(containerID)
 		if !hasRef {
@@ -115,7 +116,14 @@ func (pb *prober) probe(probeType probeType, pod *v1.Pod, status v1.PodStatus, c
 		}
 		return results.Failure, err
 	}
+	if result == probe.Warning {
+		if ref, hasRef := pb.refManager.GetRef(containerID); hasRef {
+			pb.recorder.Eventf(ref, v1.EventTypeWarning, events.ContainerProbeWarning, "%s probe warning: %s", probeType, output)
+		}
+		klog.V(3).Infof("%s probe for %q succeeded with a warning: %s", probeType, ctrName, output)
+	} else {
 		klog.V(3).Infof("%s probe for %q succeeded", probeType, ctrName)
+	}
 	return results.Success, nil
 }
 

pkg/kubelet/prober/prober_test.go

@@ -233,6 +233,11 @@ func TestProbe(t *testing.T) {
 			execResult:     probe.Success,
 			expectedResult: results.Success,
 		},
+		{ // Probe result is warning
+			probe:          execProbe,
+			execResult:     probe.Warning,
+			expectedResult: results.Success,
+		},
 		{ // Probe result is unknown
 			probe:          execProbe,
 			execResult:     probe.Unknown,

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.
@@ -56,11 +61,17 @@ type Prober interface {
 
 type httpProber struct {
 	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

pkg/registry/core/componentstatus/validator.go

@@ -68,7 +68,8 @@ func (server *Server) DoServerCheck() (probe.Result, string, error) {
 		if server.Prober != nil {
 			return
 		}
-		server.Prober = httpprober.NewWithTLSConfig(server.TLSConfig)
+		const followNonLocalRedirects = true
+		server.Prober = httpprober.NewWithTLSConfig(server.TLSConfig, followNonLocalRedirects)
 	})
 
 	scheme := "http"

test/e2e/common/BUILD

@@ -48,6 +48,7 @@ go_library(
         "//pkg/client/clientset_generated/internalclientset:go_default_library",
         "//pkg/client/conditions:go_default_library",
         "//pkg/kubelet:go_default_library",
+        "//pkg/kubelet/events:go_default_library",
         "//pkg/kubelet/images:go_default_library",
         "//pkg/kubelet/sysctl:go_default_library",
         "//pkg/security/apparmor:go_default_library",

test/e2e/common/container_probe.go

@@ -18,13 +18,16 @@ package common
 
 import (
 	"fmt"
+	"net/url"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
+	"k8s.io/kubernetes/pkg/kubelet/events"
 	"k8s.io/kubernetes/test/e2e/framework"
 	testutils "k8s.io/kubernetes/test/utils"
 
@@ -305,6 +308,82 @@ var _ = framework.KubeDescribe("Probing container", func() {
 			},
 		}, 1, defaultObservationTimeout)
 	})
+
+	/*
+		Release : v1.14
+		Testname: Pod http liveness probe, redirected to a local address
+		Description: A Pod is created with liveness probe on http endpoint /redirect?loc=healthz. The http handler on the /redirect will redirect to the /healthz endpoint, which will return a http error after 10 seconds since the Pod is started. This MUST result in liveness check failure. The Pod MUST now be killed and restarted incrementing restart count to 1.
+	*/
+	It("should be restarted with a local redirect http liveness probe", func() {
+		runLivenessTest(f, &v1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "liveness-http-redirect",
+				Labels: map[string]string{"test": "liveness"},
+			},
+			Spec: v1.PodSpec{
+				Containers: []v1.Container{
+					{
+						Name:    "liveness",
+						Image:   imageutils.GetE2EImage(imageutils.Liveness),
+						Command: []string{"/server"},
+						LivenessProbe: &v1.Probe{
+							Handler: v1.Handler{
+								HTTPGet: &v1.HTTPGetAction{
+									Path: "/redirect?loc=" + url.QueryEscape("/healthz"),
+									Port: intstr.FromInt(8080),
+								},
+							},
+							InitialDelaySeconds: 15,
+							FailureThreshold:    1,
+						},
+					},
+				},
+			},
+		}, 1, defaultObservationTimeout)
+	})
+
+	/*
+		Release : v1.14
+		Testname: Pod http liveness probe, redirected to a non-local address
+		Description: A Pod is created with liveness probe on http endpoint /redirect with a redirect to http://0.0.0.0/. The http handler on the /redirect should not follow the redirect, but instead treat it as a success and generate an event.
+	*/
+	It("should *not* be restarted with a non-local redirect http liveness probe", func() {
+		pod := &v1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "liveness-http-redirect",
+				Labels: map[string]string{"test": "liveness"},
+			},
+			Spec: v1.PodSpec{
+				Containers: []v1.Container{
+					{
+						Name:    "liveness",
+						Image:   imageutils.GetE2EImage(imageutils.Liveness),
+						Command: []string{"/server"},
+						LivenessProbe: &v1.Probe{
+							Handler: v1.Handler{
+								HTTPGet: &v1.HTTPGetAction{
+									Path: "/redirect?loc=" + url.QueryEscape("http://0.0.0.0/"),
+									Port: intstr.FromInt(8080),
+								},
+							},
+							InitialDelaySeconds: 15,
+							FailureThreshold:    1,
+						},
+					},
+				},
+			},
+		}
+		runLivenessTest(f, pod, 0, defaultObservationTimeout)
+		// Expect an event of type "ProbeWarning".
+		expectedEvent := fields.Set{
+			"involvedObject.kind":      "Pod",
+			"involvedObject.name":      pod.Name,
+			"involvedObject.namespace": f.Namespace.Name,
+			"reason":                   events.ContainerProbeWarning,
+		}.AsSelector().String()
+		framework.ExpectNoError(framework.WaitTimeoutForPodEvent(
+			f.ClientSet, pod.Name, f.Namespace.Name, expectedEvent, "0.0.0.0", framework.PodEventTimeout))
+	})
 })
 
 func getContainerStartedTime(p *v1.Pod, containerName string) (time.Time, error) {

test/images/liveness/VERSION

@@ -1 +1 @@
-1.0
+1.1

test/images/liveness/server.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"log"
 	"net/http"
+	"net/url"
 	"time"
 )
 
@@ -42,5 +43,13 @@ func main() {
 			w.Write([]byte("ok"))
 		}
 	})
+	http.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
+		loc, err := url.QueryUnescape(r.URL.Query().Get("loc"))
+		if err != nil {
+			http.Error(w, fmt.Sprintf("invalid redirect: %q", r.URL.Query().Get("loc")), http.StatusBadRequest)
+			return
+		}
+		http.Redirect(w, r, loc, http.StatusFound)
+	})
 	log.Fatal(http.ListenAndServe(":8080", nil))
 }

test/utils/image/manifest.go

@@ -215,7 +215,7 @@ func initImageConfigs() map[int]Config {
 	configs[Iperf] = Config{e2eRegistry, "iperf", "1.0"}
 	configs[JessieDnsutils] = Config{e2eRegistry, "jessie-dnsutils", "1.0"}
 	configs[Kitten] = Config{e2eRegistry, "kitten", "1.0"}
-	configs[Liveness] = Config{e2eRegistry, "liveness", "1.0"}
+	configs[Liveness] = Config{e2eRegistry, "liveness", "1.1"}
 	configs[LogsGenerator] = Config{e2eRegistry, "logs-generator", "1.0"}
 	configs[Mounttest] = Config{e2eRegistry, "mounttest", "1.0"}
 	configs[MounttestUser] = Config{e2eRegistry, "mounttest-user", "1.0"}