From 60e22e9c677c53eea7dd64743fa2d5daac45e24b Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 17 May 2022 09:27:36 -0400 Subject: [PATCH 1/4] oci: cap exec sync length Signed-off-by: Peter Hunt --- internal/oci/oci.go | 5 ++ internal/oci/runtime_oci.go | 16 ++++- internal/oci/runtime_oci_test.go | 58 +++++++++++++++ internal/oci/runtime_vm.go | 5 +- test/ctr.bats | 8 +++ .../kubernetes/pkg/kubelet/util/ioutils/BUILD | 29 ++++++++ .../pkg/kubelet/util/ioutils/ioutils.go | 70 +++++++++++++++++++ vendor/modules.txt | 1 + 8 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 internal/oci/runtime_oci_test.go create mode 100644 vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/BUILD create mode 100644 vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/ioutils.go diff --git a/internal/oci/oci.go b/internal/oci/oci.go index 5b3b2cd18f1..d7de6e2d627 100644 --- a/internal/oci/oci.go +++ b/internal/oci/oci.go @@ -32,6 +32,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 diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index 7dfa7bd2cfa..46fac1480be 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -491,7 +491,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, @@ -630,6 +630,20 @@ func pidAndpgidFromFile(pidFile string) (pid, pgid int, _ error) { return pid, pgid, err } +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 ioutil.ReadFile(path) +} + // UpdateContainer updates container resources func (r *runtimeOCI) UpdateContainer(c *Container, res *rspec.LinuxResources) error { if c.Spoofed() { diff --git a/internal/oci/runtime_oci_test.go b/internal/oci/runtime_oci_test.go new file mode 100644 index 00000000000..19da824856e --- /dev/null +++ b/internal/oci/runtime_oci_test.go @@ -0,0 +1,58 @@ +package oci_test + +import ( + "context" + "io/ioutil" + + "github.com/cri-o/cri-o/internal/oci" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +const ( + shortTimeout int64 = 1 + mediumTimeout int64 = 3 + longTimeout int64 = 15 +) + +// The actual test suite +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(ioutil.WriteFile(fileName, test.contents, 0644)).To(BeNil()) + found, err := oci.TruncateAndReadFile(context.Background(), fileName, test.size) + Expect(err).To(BeNil()) + Expect(found).To(Equal(test.expected)) + }) + } + }) +}) diff --git a/internal/oci/runtime_vm.go b/internal/oci/runtime_vm.go index 8b6d56d3e29..9da6662334c 100644 --- a/internal/oci/runtime_vm.go +++ b/internal/oci/runtime_vm.go @@ -31,6 +31,7 @@ import ( "golang.org/x/sys/unix" "k8s.io/client-go/tools/remotecommand" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" + kioutil "k8s.io/kubernetes/pkg/kubelet/util/ioutils" utilexec "k8s.io/utils/exec" ) @@ -309,8 +310,8 @@ func (r *runtimeVM) ExecSyncContainer(ctx context.Context, c *Container, command defer logrus.Debug("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(c, command, timeout, nil, stdout, stderr, c.terminal, nil) if err != nil { diff --git a/test/ctr.bats b/test/ctr.bats index 9a480003ca0..7117de72855 100644 --- a/test/ctr.bats +++ b/test/ctr.bats @@ -457,6 +457,14 @@ function wait_until_exit() { crictl rm -f "$ctr_id" } +@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 diff --git a/vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/BUILD b/vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/BUILD new file mode 100644 index 00000000000..43bd2626084 --- /dev/null +++ b/vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/BUILD @@ -0,0 +1,29 @@ +package(default_visibility = ["//visibility:public"]) + +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["ioutils.go"], + importpath = "k8s.io/kubernetes/pkg/kubelet/util/ioutils", +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], +) + +go_test( + name = "go_default_test", + srcs = ["ioutils_test.go"], + embed = [":go_default_library"], + deps = ["//vendor/github.com/stretchr/testify/assert:go_default_library"], +) diff --git a/vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/ioutils.go b/vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/ioutils.go new file mode 100644 index 00000000000..1b2b5a6d5dd --- /dev/null +++ b/vendor/k8s.io/kubernetes/pkg/kubelet/util/ioutils/ioutils.go @@ -0,0 +1,70 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ioutils + +import "io" + +// writeCloserWrapper represents a WriteCloser whose closer operation is noop. +type writeCloserWrapper struct { + Writer io.Writer +} + +func (w *writeCloserWrapper) Write(buf []byte) (int, error) { + return w.Writer.Write(buf) +} + +func (w *writeCloserWrapper) Close() error { + return nil +} + +// WriteCloserWrapper returns a writeCloserWrapper. +func WriteCloserWrapper(w io.Writer) io.WriteCloser { + return &writeCloserWrapper{w} +} + +// LimitWriter is a copy of the standard library ioutils.LimitReader, +// applied to the writer interface. +// LimitWriter returns a Writer that writes to w +// but stops with EOF after n bytes. +// The underlying implementation is a *LimitedWriter. +func LimitWriter(w io.Writer, n int64) io.Writer { return &LimitedWriter{w, n} } + +// A LimitedWriter writes to W but limits the amount of +// data returned to just N bytes. Each call to Write +// updates N to reflect the new amount remaining. +// Write returns EOF when N <= 0 or when the underlying W returns EOF. +type LimitedWriter struct { + W io.Writer // underlying writer + N int64 // max bytes remaining +} + +func (l *LimitedWriter) Write(p []byte) (n int, err error) { + if l.N <= 0 { + return 0, io.ErrShortWrite + } + truncated := false + if int64(len(p)) > l.N { + p = p[0:l.N] + truncated = true + } + n, err = l.W.Write(p) + l.N -= int64(n) + if err == nil && truncated { + err = io.ErrShortWrite + } + return +} diff --git a/vendor/modules.txt b/vendor/modules.txt index e1ed6482cef..978a6c0da0d 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1270,6 +1270,7 @@ k8s.io/kubernetes/pkg/kubelet/cri/streaming/remotecommand k8s.io/kubernetes/pkg/kubelet/leaky k8s.io/kubernetes/pkg/kubelet/types k8s.io/kubernetes/pkg/kubelet/util/format +k8s.io/kubernetes/pkg/kubelet/util/ioutils k8s.io/kubernetes/pkg/proxy k8s.io/kubernetes/pkg/proxy/config k8s.io/kubernetes/pkg/proxy/healthcheck From f68f2ee58df36ea953e8caad8d700d0b7370d86e Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 23 May 2022 15:19:33 -0400 Subject: [PATCH 2/4] add support for conmon log-global-size-max Signed-off-by: Peter Hunt --- internal/config/conmonmgr/conmonmgr.go | 21 +++++++++++++++++++-- internal/oci/runtime_oci.go | 3 +++ pkg/config/config.go | 4 ++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/internal/config/conmonmgr/conmonmgr.go b/internal/config/conmonmgr/conmonmgr.go index 9aef7ef446a..9b9bd7e5f5c 100644 --- a/internal/config/conmonmgr/conmonmgr.go +++ b/internal/config/conmonmgr/conmonmgr.go @@ -11,10 +11,12 @@ import ( ) var versionSupportsSync = semver.MustParse("2.0.19") +var 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 +39,7 @@ func New(conmonPath string) (*ConmonManager, error) { } c.initializeSupportsSync() + c.initializeSupportsLogGlobalSizeMax() return c, nil } @@ -49,6 +52,20 @@ func (c *ConmonManager) parseConmonVersion(versionString string) error { return nil } +func (c *ConmonManager) initializeSupportsLogGlobalSizeMax() { + c.supportsLogGlobalSizeMax = c.conmonVersion.GTE(versionSupportsLogGlobalSizeMax) + 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" diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index 46fac1480be..001a72df402 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -385,6 +385,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") } diff --git a/pkg/config/config.go b/pkg/config/config.go index 05109a1efc2..d5d1e928e09 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -993,6 +993,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) From 16f4bcc28c9e06879518da371e7a406df0732761 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Fri, 3 Jun 2022 13:37:44 -0400 Subject: [PATCH 3/4] conmonmgr: query help text to see if it supports log-global-size-max Signed-off-by: Peter Hunt --- internal/config/conmonmgr/conmonmgr.go | 11 +- internal/config/conmonmgr/conmonmgr_test.go | 119 +++++++++++++++++--- 2 files changed, 114 insertions(+), 16 deletions(-) diff --git a/internal/config/conmonmgr/conmonmgr.go b/internal/config/conmonmgr/conmonmgr.go index 9b9bd7e5f5c..2b51a012e5e 100644 --- a/internal/config/conmonmgr/conmonmgr.go +++ b/internal/config/conmonmgr/conmonmgr.go @@ -1,6 +1,7 @@ package conmonmgr import ( + "bytes" "path" "strings" @@ -39,7 +40,7 @@ func New(conmonPath string) (*ConmonManager, error) { } c.initializeSupportsSync() - c.initializeSupportsLogGlobalSizeMax() + c.initializeSupportsLogGlobalSizeMax(conmonPath) return c, nil } @@ -52,8 +53,14 @@ func (c *ConmonManager) parseConmonVersion(versionString string) error { return nil } -func (c *ConmonManager) initializeSupportsLogGlobalSizeMax() { +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" diff --git a/internal/config/conmonmgr/conmonmgr_test.go b/internal/config/conmonmgr/conmonmgr_test.go index a09731281af..2efd30b235c 100644 --- a/internal/config/conmonmgr/conmonmgr_test.go +++ b/internal/config/conmonmgr/conmonmgr_test.go @@ -61,20 +61,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), - ) - - // When - mgr, err := New(validPath) - - // Then - Expect(err).To(BeNil()) - Expect(mgr).ToNot(BeNil()) - }) - 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 @@ -174,4 +161,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)) + }) + }) }) From dbfdc42ca4b44f2e7628ec5564a876faff471ebd Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Fri, 3 Jun 2022 12:48:20 -0400 Subject: [PATCH 4/4] bump to v1.20.8 Signed-off-by: Peter Hunt --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index d34819f58e7..10cdd44ee59 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -21,7 +21,7 @@ import ( ) // Version is the version of the build. -const Version = "1.20.7" +const Version = "1.20.8" // Variables injected during build-time var (