Merge pull request from GHSA-fcm2-6c3h-pg6j

[1.24] oci: add support for capping memory and disk usage from exec sync output

internal/config/conmonmgr/conmonmgr.go

@@ -1,6 +1,7 @@
 package conmonmgr
 
 import (
+	"bytes"
 	"path"
 	"strings"
 
@@ -10,11 +11,15 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-var versionSupportsSync = semver.MustParse("2.0.19")
+var (
+	versionSupportsSync             = semver.MustParse("2.0.19")
+	versionSupportsLogGlobalSizeMax = semver.MustParse("2.1.2")
+)
 
 type ConmonManager struct {
-	conmonVersion *semver.Version
-	supportsSync  bool
+	conmonVersion            *semver.Version
+	supportsSync             bool
+	supportsLogGlobalSizeMax bool
 }
 
 // this function is heavily based on github.com/containers/common#probeConmon
@@ -37,6 +42,7 @@ func New(conmonPath string) (*ConmonManager, error) {
 	}
 
 	c.initializeSupportsSync()
+	c.initializeSupportsLogGlobalSizeMax(conmonPath)
 	return c, nil
 }
 
@@ -49,6 +55,26 @@ func (c *ConmonManager) parseConmonVersion(versionString string) error {
 	return nil
 }
 
+func (c *ConmonManager) initializeSupportsLogGlobalSizeMax(conmonPath string) {
+	c.supportsLogGlobalSizeMax = c.conmonVersion.GTE(versionSupportsLogGlobalSizeMax)
+	if !c.supportsLogGlobalSizeMax {
+		// Read help output as a fallback in case the feature was backported to conmon,
+		// but the version wasn't bumped.
+		helpOutput, err := cmdrunner.CombinedOutput(conmonPath, "--help")
+		c.supportsLogGlobalSizeMax = err == nil && bytes.Contains(helpOutput, []byte("--log-global-size-max"))
+	}
+	verb := "does not"
+	if c.supportsLogGlobalSizeMax {
+		verb = "does"
+	}
+
+	logrus.Infof("Conmon %s support the --log-global-size-max option", verb)
+}
+
+func (c *ConmonManager) SupportsLogGlobalSizeMax() bool {
+	return c.supportsLogGlobalSizeMax
+}
+
 func (c *ConmonManager) initializeSupportsSync() {
 	c.supportsSync = c.conmonVersion.GTE(versionSupportsSync)
 	verb := "does not"

internal/config/conmonmgr/conmonmgr_test.go

@@ -70,7 +70,7 @@ var _ = t.Describe("ConmonManager", func() {
 		It("should succeed when output expected", func() {
 			// Given
 			gomock.InOrder(
-				runner.EXPECT().CombinedOutput(gomock.Any(), gomock.Any()).Return([]byte("conmon version 2.0.0"), nil),
+				runner.EXPECT().CombinedOutput(gomock.Any(), gomock.Any()).Return([]byte("conmon version 2.2.2"), nil),
 			)
 
 			// When
@@ -170,4 +170,108 @@ var _ = t.Describe("ConmonManager", func() {
 			Expect(mgr.SupportsSync()).To(Equal(true))
 		})
 	})
+	t.Describe("initializeSupportsLogGlobalSizeMax", func() {
+		var mgr *ConmonManager
+		BeforeEach(func() {
+			runner = runnerMock.NewMockCommandRunner(mockCtrl)
+			cmdrunner.SetMocked(runner)
+			mgr = new(ConmonManager)
+		})
+		It("should be false when major version less", func() {
+			// Given
+			gomock.InOrder(
+				runner.EXPECT().CombinedOutput(gomock.Any(), gomock.Any()).Return([]byte{}, errors.New("cmd failed")),
+			)
+			err := mgr.parseConmonVersion("1.1.2")
+			Expect(err).To(BeNil())
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(false))
+		})
+		It("should be true when major version greater", func() {
+			// Given
+			err := mgr.parseConmonVersion("3.1.1")
+			Expect(err).To(BeNil())
+
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(true))
+		})
+		It("should be false when minor version less", func() {
+			// Given
+			gomock.InOrder(
+				runner.EXPECT().CombinedOutput(gomock.Any(), gomock.Any()).Return([]byte{}, errors.New("cmd failed")),
+			)
+			err := mgr.parseConmonVersion("2.0.2")
+			Expect(err).To(BeNil())
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(false))
+		})
+		It("should be true when minor version greater", func() {
+			// Given
+			err := mgr.parseConmonVersion("2.2.2")
+			Expect(err).To(BeNil())
+
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(true))
+		})
+		It("should be false when patch version less", func() {
+			// Given
+			gomock.InOrder(
+				runner.EXPECT().CombinedOutput(gomock.Any(), gomock.Any()).Return([]byte{}, errors.New("cmd failed")),
+			)
+			err := mgr.parseConmonVersion("2.1.1")
+			Expect(err).To(BeNil())
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(false))
+		})
+		It("should be true when patch version greater", func() {
+			// Given
+			err := mgr.parseConmonVersion("2.1.3")
+			Expect(err).To(BeNil())
+
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(true))
+		})
+		It("should be true when version equal", func() {
+			// Given
+			err := mgr.parseConmonVersion("2.1.2")
+			Expect(err).To(BeNil())
+
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(true))
+		})
+		It("should be true if feature backported", func() {
+			// Given
+			gomock.InOrder(
+				runner.EXPECT().CombinedOutput(gomock.Any(), gomock.Any()).Return([]byte("--log-global-size-max"), nil),
+			)
+			err := mgr.parseConmonVersion("0.0.0")
+			Expect(err).To(BeNil())
+
+			// When
+			mgr.initializeSupportsLogGlobalSizeMax("")
+
+			// Then
+			Expect(mgr.SupportsLogGlobalSizeMax()).To(Equal(true))
+		})
+	})
 })

internal/oci/oci.go

@@ -35,6 +35,11 @@ const (
 	// killContainerTimeout is the timeout that we wait for the container to
 	// be SIGKILLed.
 	killContainerTimeout = 2 * time.Minute
+
+	// maxExecSyncSize is the maximum size of exec sync output CRI-O will process.
+	// It is set to the amount of logs allowed in the dockershim implementation:
+	// https://github.com/kubernetes/kubernetes/pull/82514
+	maxExecSyncSize = 16 * 1024 * 1024
 )
 
 // Runtime is the generic structure holding both global and specific

internal/oci/runtime_oci.go

@@ -486,6 +486,9 @@ func (r *runtimeOCI) ExecSyncContainer(ctx context.Context, c *Container, comman
 	if r.config.ConmonSupportsSync() {
 		args = append(args, "--sync")
 	}
+	if r.config.ConmonSupportsLogGlobalSizeMax() {
+		args = append(args, "--log-global-size-max", strconv.Itoa(maxExecSyncSize))
+	}
 	if c.terminal {
 		args = append(args, "-t")
 	}
@@ -661,7 +664,7 @@ func (r *runtimeOCI) ExecSyncContainer(ctx context.Context, c *Container, comman
 	// ExecSyncResponse we have to read the logfile.
 	// XXX: Currently runC dups the same console over both stdout and stderr,
 	//      so we can't differentiate between the two.
-	logBytes, err := ioutil.ReadFile(logPath)
+	logBytes, err := TruncateAndReadFile(ctx, logPath, maxExecSyncSize)
 	if err != nil {
 		return nil, &ExecSyncError{
 			Stdout:   stdoutBuf,
@@ -680,6 +683,20 @@ func (r *runtimeOCI) ExecSyncContainer(ctx context.Context, c *Container, comman
 	}, nil
 }
 
+func TruncateAndReadFile(ctx context.Context, path string, size int64) ([]byte, error) {
+	info, err := os.Stat(path)
+	if err != nil {
+		return nil, err
+	}
+	if info.Size() > size {
+		log.Errorf(ctx, "Exec sync output in file %s has size %d which is longer than expected size of %d", path, info.Size(), size)
+		if err := os.Truncate(path, size); err != nil {
+			return nil, err
+		}
+	}
+	return os.ReadFile(path)
+}
+
 // UpdateContainer updates container resources
 func (r *runtimeOCI) UpdateContainer(ctx context.Context, c *Container, res *rspec.LinuxResources) error {
 	if c.Spoofed() {

internal/oci/runtime_oci_test.go

@@ -3,6 +3,7 @@ package oci_test
 import (
 	"context"
 	"math/rand"
+	"os"
 	"os/exec"
 	"time"
 
@@ -142,6 +143,44 @@ var _ = t.Describe("Oci", func() {
 			})
 		}
 	})
+	Context("TruncateAndReadFile", func() {
+		tests := []struct {
+			title    string
+			contents []byte
+			expected []byte
+			fail     bool
+			size     int64
+		}{
+			{
+				title:    "should read file if size is smaller than limit",
+				contents: []byte("abcd"),
+				expected: []byte("abcd"),
+				size:     5,
+			},
+			{
+				title:    "should read only size if size is same as limit",
+				contents: []byte("abcd"),
+				expected: []byte("abcd"),
+				size:     4,
+			},
+			{
+				title:    "should read only size if size is larger than limit",
+				contents: []byte("abcd"),
+				expected: []byte("abc"),
+				size:     3,
+			},
+		}
+		for _, test := range tests {
+			test := test
+			It(test.title, func() {
+				fileName := t.MustTempFile("to-read")
+				Expect(os.WriteFile(fileName, test.contents, 0o644)).To(BeNil())
+				found, err := oci.TruncateAndReadFile(context.Background(), fileName, test.size)
+				Expect(err).To(BeNil())
+				Expect(found).To(Equal(test.expected))
+			})
+		}
+	})
 })
 
 func waitContainerStopAndFailAfterTimeout(ctx context.Context,

internal/oci/runtime_vm.go

@@ -39,6 +39,7 @@ import (
 	"k8s.io/client-go/tools/remotecommand"
 	types "k8s.io/cri-api/pkg/apis/runtime/v1"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	kioutil "k8s.io/kubernetes/pkg/kubelet/util/ioutils"
 	utilexec "k8s.io/utils/exec"
 )
 
@@ -307,8 +308,8 @@ func (r *runtimeVM) ExecSyncContainer(ctx context.Context, c *Container, command
 	defer log.Debugf(ctx, "RuntimeVM.ExecSyncContainer() end")
 
 	var stdoutBuf, stderrBuf bytes.Buffer
-	stdout := cioutil.NewNopWriteCloser(&stdoutBuf)
-	stderr := cioutil.NewNopWriteCloser(&stderrBuf)
+	stdout := kioutil.WriteCloserWrapper(kioutil.LimitWriter(&stdoutBuf, maxExecSyncSize))
+	stderr := kioutil.WriteCloserWrapper(kioutil.LimitWriter(&stderrBuf, maxExecSyncSize))
 
 	exitCode, err := r.execContainerCommon(ctx, c, command, timeout, nil, stdout, stderr, c.terminal, nil)
 	if err != nil {

internal/version/version.go

@@ -21,7 +21,7 @@ import (
 )
 
 // Version is the version of the build.
-const Version = "1.24.0"
+const Version = "1.24.1"
 
 // Variables injected during build-time
 var (

pkg/config/config.go

@@ -1179,6 +1179,10 @@ func (c *RuntimeConfig) ConmonSupportsSync() bool {
 	return c.conmonManager.SupportsSync()
 }
 
+func (c *RuntimeConfig) ConmonSupportsLogGlobalSizeMax() bool {
+	return c.conmonManager.SupportsLogGlobalSizeMax()
+}
+
 func (c *RuntimeConfig) ValidatePinnsPath(executable string) error {
 	var err error
 	c.PinnsPath, err = validateExecutablePath(executable, c.PinnsPath)

test/ctr.bats

@@ -502,6 +502,14 @@ function check_oci_annotation() {
 	crictl exec --sync "$ctr_id" /bin/sh -c "[[ -t 1 ]]"
 }
 
+@test "ctr execsync should cap output" {
+	start_crio
+
+	ctr_id=$(crictl run "$TESTDATA"/container_sleep.json "$TESTDATA"/sandbox_config.json)
+
+	[[ $(crictl exec --sync "$ctr_id" /bin/sh -c "for i in $(seq 1 50000000); do echo -n 'a'; done" | wc -c) -le 16777216 ]]
+}
+
 @test "ctr device add" {
 	# In an user namespace we can only bind mount devices from the host, not mknod
 	# https://github.com/opencontainers/runc/blob/master/libcontainer/rootfs_linux.go#L480-L481