From 77f0429d91d0fc25a09142038243aa319c09ec05 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 | 39 +++++++++++ internal/oci/runtime_vm.go | 5 +- test/ctr.bats | 8 +++ .../pkg/kubelet/util/ioutils/ioutils.go | 70 +++++++++++++++++++ vendor/modules.txt | 1 + 7 files changed, 141 insertions(+), 3 deletions(-) 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 1ea398e1358..b377c9a9eb1 100644 --- a/internal/oci/oci.go +++ b/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 diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index c5856f1b18d..da02fd96b9c 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -661,7 +661,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 +680,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() { diff --git a/internal/oci/runtime_oci_test.go b/internal/oci/runtime_oci_test.go index 37ee8a1e9ea..7120f0872d1 100644 --- a/internal/oci/runtime_oci_test.go +++ b/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, diff --git a/internal/oci/runtime_vm.go b/internal/oci/runtime_vm.go index 9dcc54892bd..c49686ee3d9 100644 --- a/internal/oci/runtime_vm.go +++ b/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 { diff --git a/test/ctr.bats b/test/ctr.bats index 796a2361dc3..304f2ed5cec 100644 --- a/test/ctr.bats +++ b/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 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 32a73558dbd..30e34b18bec 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1738,6 +1738,7 @@ k8s.io/kubernetes/pkg/kubelet/cri/streaming k8s.io/kubernetes/pkg/kubelet/cri/streaming/portforward k8s.io/kubernetes/pkg/kubelet/cri/streaming/remotecommand k8s.io/kubernetes/pkg/kubelet/types +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 fc852b402f1698501cac71963c9ccc919c4b05fe 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 | 25 ++++++++++++++++++++++--- internal/oci/runtime_oci.go | 3 +++ pkg/config/config.go | 4 ++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/internal/config/conmonmgr/conmonmgr.go b/internal/config/conmonmgr/conmonmgr.go index 857437c3f54..371a1e22ca1 100644 --- a/internal/config/conmonmgr/conmonmgr.go +++ b/internal/config/conmonmgr/conmonmgr.go @@ -10,11 +10,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 +41,7 @@ func New(conmonPath string) (*ConmonManager, error) { } c.initializeSupportsSync() + c.initializeSupportsLogGlobalSizeMax() return c, nil } @@ -49,6 +54,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 da02fd96b9c..3ce912de25c 100644 --- a/internal/oci/runtime_oci.go +++ b/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") } diff --git a/pkg/config/config.go b/pkg/config/config.go index 45cabae3d41..01d00d7ab6a 100644 --- a/pkg/config/config.go +++ b/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) From 8acadd3f4b2cb322b2d93f72cbe6201bb1c58427 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 | 106 +++++++++++++++++++- 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/internal/config/conmonmgr/conmonmgr.go b/internal/config/conmonmgr/conmonmgr.go index 371a1e22ca1..e95e2748476 100644 --- a/internal/config/conmonmgr/conmonmgr.go +++ b/internal/config/conmonmgr/conmonmgr.go @@ -1,6 +1,7 @@ package conmonmgr import ( + "bytes" "path" "strings" @@ -41,7 +42,7 @@ func New(conmonPath string) (*ConmonManager, error) { } c.initializeSupportsSync() - c.initializeSupportsLogGlobalSizeMax() + c.initializeSupportsLogGlobalSizeMax(conmonPath) return c, nil } @@ -54,8 +55,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 fae450cb31c..dbcca92324c 100644 --- a/internal/config/conmonmgr/conmonmgr_test.go +++ b/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)) + }) + }) }) From 489819e33d71828561a389227e787833d3682611 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 6 Jun 2022 13:39:42 -0400 Subject: [PATCH 4/4] bump to v1.24.1 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 89a7ac477c9..9cac7813676 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.24.0" +const Version = "1.24.1" // Variables injected during build-time var (