Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exec probes should not be unbounded #82514

Merged
merged 3 commits into from Sep 12, 2019

Conversation

@dims
Copy link
Member

commented Sep 10, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
In 1f270ef, we added 10KB as the read
limit for http probes. we should do the same for exec probes as well.

Co-Authored-By: Tim Allclair tallclair@google.com

Change-Id: If154c5c4e669829ab94839c56260a894a6714f0f

Which issue(s) this PR fixes:

Fixes #70890

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Limit the body length of exec readiness/liveness probes. remote CRIs and Docker shim read a max of 16MB output of which the exec probe itself inspects 10kb.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

pkg/probe/exec/exec.go Outdated Show resolved Hide resolved
@dims dims force-pushed the dims:limit-exec-probe-bytes-read branch 2 times, most recently from 7ef84fc to 02dbe86 Sep 10, 2019
@dims dims changed the title [WIP] Exec probes should not be unbounded Exec probes should not be unbounded Sep 10, 2019
@dims dims force-pushed the dims:limit-exec-probe-bytes-read branch 2 times, most recently from a4bdcbd to 46b9484 Sep 10, 2019
@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

/priority important-soon

@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 11, 2019
@dims dims force-pushed the dims:limit-exec-probe-bytes-read branch 3 times, most recently from d58b5f1 to 448696e Sep 11, 2019
@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

tested manually using the following patch as well (see liveness probe yaml below as well)

diff --git a/pkg/kubelet/dockershim/docker_streaming.go b/pkg/kubelet/dockershim/docker_streaming.go
index 76b2fecd11b8..2328b07fc121 100644
--- a/pkg/kubelet/dockershim/docker_streaming.go
+++ b/pkg/kubelet/dockershim/docker_streaming.go
@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"k8s.io/klog"
 	"math"
 	"time"
 
@@ -95,6 +96,8 @@ func (ds *dockerService) ExecSync(_ context.Context, req *runtimeapi.ExecSyncReq
 
 		exitCode = int32(exitError.ExitStatus())
 	}
+	klog.V(4).Infof(">>>> dockerService response size : %d", len(stdoutBuffer.Bytes()))
+	klog.V(4).Infof(">>>> dockerService response text : %s", string(stdoutBuffer.Bytes()))
 	return &runtimeapi.ExecSyncResponse{
 		Stdout:   stdoutBuffer.Bytes(),
 		Stderr:   stderrBuffer.Bytes(),
diff --git a/pkg/kubelet/prober/prober.go b/pkg/kubelet/prober/prober.go
index dd94f1705b23..ba5d62266100 100644
--- a/pkg/kubelet/prober/prober.go
+++ b/pkg/kubelet/prober/prober.go
@@ -258,7 +258,10 @@ type execInContainer struct {
 
 func (pb *prober) newExecInContainer(container v1.Container, containerID kubecontainer.ContainerID, cmd []string, timeout time.Duration) exec.Cmd {
 	return &execInContainer{run: func() ([]byte, error) {
-		return pb.runner.RunInContainer(containerID, cmd, timeout)
+		bytes, err := pb.runner.RunInContainer(containerID, cmd, timeout)
+		klog.V(4).Infof(">>>> newExecInContainer response size : %d", len(bytes))
+		klog.V(4).Infof(">>>> newExecInContainer response text : %s", string(bytes))
+		return bytes, err
 	}}
 }
 
diff --git a/pkg/probe/exec/exec.go b/pkg/probe/exec/exec.go
index b8cfe0d2ad7e..6ad5e9e9e2d3 100644
--- a/pkg/probe/exec/exec.go
+++ b/pkg/probe/exec/exec.go
@@ -57,6 +57,8 @@ func (pr execProber) Probe(e exec.Cmd) (probe.Result, string, error) {
 	}
 	data := dataBuffer.Bytes()
 
+	klog.V(4).Infof(">>>> Exec probe response length: %d", len(data))
+	klog.V(4).Infof(">>>> Exec probe response text: %s", string(data))
 	klog.V(4).Infof("Exec probe response: %q", string(data))
 	if err != nil {
 		exit, ok := err.(exec.ExitError)

apiVersion: v1
kind: Pod
metadata:
  labels:
    test: liveness
  name: liveness-exec
spec:
  containers:
  - name: liveness
    #image: k8s.gcr.io/busybox
    image: ubuntu
    args:
    - /bin/sh
    - -c
    - echo ok >/tmp/health; sleep 600
    #- echo ok >/tmp/health; sleep 10; rm -rf /tmp/health; sleep 600
    livenessProbe:
      exec:
        command:
        - cat
        - /tmp/health
      initialDelaySeconds: 15
      failureThreshold: 1

@dims dims changed the title [WIP] Exec probes should not be unbounded Exec probes should not be unbounded Sep 11, 2019
tallclair and others added 3 commits Sep 10, 2019
Change-Id: Ia86cfdb9bdaf994d30216621f78aebc6c555cf4a
In 1f270ef, we added 10KB as the read
limit for http probes. we should do the same for exec probes as well.

Change-Id: If154c5c4e669829ab94839c56260a894a6714f0f
@dims dims force-pushed the dims:limit-exec-probe-bytes-read branch from 448696e to 5706a13 Sep 11, 2019
@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

/milestone v1.17

this isn't release-blocking, but can be picked to a patch release after soaking in master a bit

@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 11, 2019
@tallclair

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@dims Can you run a test with a probe using the yes command (or some other infinite outputting command) as in the original report?

@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

@tallclair trying that now (command "yes")

@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

@tallclair the PR is holding up with the scenario in the bug repo.

@tallclair

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

/lgtm
/approve

We should follow up to cap all CombinedOutput() calls, but that's lower priority.

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 11, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 579e0c7 into kubernetes:master Sep 12, 2019
25 checks passed
25 checks passed
cla/linuxfoundation dims authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Skipped.
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Sorry to be a little delayed in reviewing this but lgtm :)

@dims

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

/kind bug

k8s-ci-robot added a commit that referenced this pull request Oct 1, 2019
…pstream-release-1.16-take-2

Automated cherry pick of #82514: Exec probes should not be unbounded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.