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" diff --git a/test/e2e/common/BUILD b/test/e2e/common/BUILD index 204408fa0d..2b0d983a55 100644 --- a/test/e2e/common/BUILD +++ b/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", diff --git a/test/e2e/common/container_probe.go b/test/e2e/common/container_probe.go index 9988a29919..f4b8f224be 100644 --- a/test/e2e/common/container_probe.go +++ b/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) { diff --git a/test/images/liveness/VERSION b/test/images/liveness/VERSION index d3827e75a5..9459d4ba2a 100644 --- a/test/images/liveness/VERSION +++ b/test/images/liveness/VERSION @@ -1 +1 @@ -1.0 +1.1 diff --git a/test/images/liveness/server.go b/test/images/liveness/server.go index a32ac92676..7b6d7118a1 100644 --- a/test/images/liveness/server.go +++ b/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)) } diff --git a/test/utils/image/manifest.go b/test/utils/image/manifest.go index 26f3e4b1c9..2f9e3b40b1 100644 --- a/test/utils/image/manifest.go +++ b/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"}