diff --git a/pkg/kubelet/events/event.go b/pkg/kubelet/events/event.go index f6f468ee7d..b948edbda1 100644 --- a/pkg/kubelet/events/event.go +++ b/pkg/kubelet/events/event.go @@ -82,7 +82,8 @@ const ( FreeDiskSpaceFailed = "FreeDiskSpaceFailed" // Probe event reason list - ContainerUnhealthy = "Unhealthy" + ContainerUnhealthy = "Unhealthy" + ContainerProbeWarning = "ProbeWarning" // Pod worker event reason list FailedSync = "FailedSync" diff --git a/pkg/kubelet/prober/prober.go b/pkg/kubelet/prober/prober.go index cc69ec02f7..c489bcc9b7 100644 --- a/pkg/kubelet/prober/prober.go +++ b/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 } - klog.V(3).Infof("%s probe for %q succeeded", probeType, ctrName) + 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 } diff --git a/pkg/kubelet/prober/prober_test.go b/pkg/kubelet/prober/prober_test.go index 2f9bb06457..90a4d6c041 100644 --- a/pkg/kubelet/prober/prober_test.go +++ b/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, diff --git a/pkg/probe/http/BUILD b/pkg/probe/http/BUILD index 98387df305..a789305c89 100644 --- a/pkg/probe/http/BUILD +++ b/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( diff --git a/pkg/probe/http/http.go b/pkg/probe/http/http.go index 6c31bed6f2..0d777b949e 100644 --- a/pkg/probe/http/http.go +++ b/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 + } +} diff --git a/pkg/probe/http/http_test.go b/pkg/probe/http/http_test.go index 3afa8ff077..096cb43043 100644 --- a/pkg/probe/http/http_test.go +++ b/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) + }) } } diff --git a/pkg/probe/probe.go b/pkg/probe/probe.go index d57bf43874..b204885e7b 100644 --- a/pkg/probe/probe.go +++ b/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 diff --git a/pkg/registry/core/componentstatus/validator.go b/pkg/registry/core/componentstatus/validator.go index 09c0f6a34c..ebfad4bf02 100644 --- a/pkg/registry/core/componentstatus/validator.go +++ b/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"