Honor max_output_length for custom plugin stdout capture

Custom plugin stdout was read into a hardcoded 4 KiB buffer (maxCustomPluginBufferBytes = 1024*4) and only then truncated to the configured max_output_length, so the effective limit was min(4096, max_output_length). Any plugin configured with max_output_length > 4096 was silently capped at 4096 bytes, and a JSON-emitting plugin would be cut mid-token into invalid output.

Drive the stdout read limit from max_output_length, bounded by a 1 MiB safety ceiling against a runaway plugin. stderr keeps a small fixed cap since it is only used for diagnostic logging and never becomes the plugin output.

Adds a regression test and fixture asserting that a plugin emitting more than 4 KiB is captured up to max_output_length rather than the old fixed buffer size.

Signed-off-by: Vineeth Rao Kanaparthi <vkanaparthi@twitter.com>

Author: Vineeth Rao Kanaparthi

pkg/custompluginmonitor/plugin/plugin.go

@@ -33,9 +33,18 @@ import (
 	"k8s.io/node-problem-detector/pkg/util/tomb"
 )
 
-// maxCustomPluginBufferBytes is the max bytes that a custom plugin is allowed to
-// send to stdout/stderr. Any bytes exceeding this value will be truncated.
-const maxCustomPluginBufferBytes = 1024 * 4
+// maxCustomPluginStdoutCeilingBytes is a hard safety ceiling on how many bytes
+// of stdout NPD reads from a custom plugin, bounding memory use from a runaway
+// plugin. The per-plugin max_output_length is what actually governs the message
+// size; this ceiling only caps the (misconfigured) case where max_output_length
+// itself exceeds it. It must be >= the largest supported max_output_length so a
+// plugin's configured limit is never silently truncated by the read buffer.
+const maxCustomPluginStdoutCeilingBytes = 1024 * 1024
+
+// maxCustomPluginStderrBytes caps stderr. run() uses stderr only for diagnostic
+// logging (logPluginStderr) and never includes it in the returned output, so it
+// does not need to honor max_output_length and keeps a small fixed cap.
+const maxCustomPluginStderrBytes = 1024 * 4
 
 type Plugin struct {
 	config     cpmtypes.CustomPluginConfig
@@ -210,14 +219,22 @@ func (p *Plugin) run(rule cpmtypes.CustomRule) (exitStatus cpmtypes.Status, outp
 		stderrErr error
 	)
 
+	// Capture enough stdout to honor the configured max_output_length, bounded
+	// by a hard safety ceiling. Previously this was a fixed 4 KiB buffer that
+	// silently truncated plugins configured with a larger max_output_length.
+	stdoutCapture := int64(*p.config.PluginGlobalConfig.MaxOutputLength)
+	if stdoutCapture > maxCustomPluginStdoutCeilingBytes {
+		stdoutCapture = maxCustomPluginStdoutCeilingBytes
+	}
+
 	wg.Add(2)
 	go func() {
 		defer wg.Done()
-		stdout, stdoutErr = readFromReader(stdoutPipe, maxCustomPluginBufferBytes)
+		stdout, stdoutErr = readFromReader(stdoutPipe, stdoutCapture)
 	}()
 	go func() {
 		defer wg.Done()
-		stderr, stderrErr = readFromReader(stderrPipe, maxCustomPluginBufferBytes)
+		stderr, stderrErr = readFromReader(stderrPipe, maxCustomPluginStderrBytes)
 	}()
 	// This will wait for the reads to complete. If the execution times out, the pipes
 	// will be closed and the wait group unblocks.

pkg/custompluginmonitor/plugin/plugin_test.go

@@ -18,6 +18,7 @@ package plugin
 
 import (
 	"runtime"
+	"strings"
 	"testing"
 	"time"
 
@@ -121,3 +122,44 @@ func TestNewPluginRun(t *testing.T) {
 		})
 	}
 }
+
+// TestPluginRunCaptureRespectsMaxOutputLength verifies that a plugin emitting
+// more than the old hardcoded 4 KiB capture buffer is captured up to the
+// configured max_output_length, not silently truncated at 4096 bytes.
+func TestPluginRunCaptureRespectsMaxOutputLength(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("fixture is a shell script; this path is exercised on non-Windows platforms")
+	}
+
+	ruleTimeout := 1 * time.Second
+	// Larger than the old 4096-byte capture buffer, and smaller than the
+	// 16384 bytes the fixture emits, so the result must be exactly this many
+	// bytes if (and only if) capture honors max_output_length.
+	maxOutputLength := 8192
+
+	conf := cpmtypes.CustomPluginConfig{}
+	// Set before ApplyConfiguration so it is not overwritten by the default,
+	// and use a fresh pointer so we don't mutate the package-level default.
+	conf.PluginGlobalConfig.MaxOutputLength = &maxOutputLength
+	if err := (&conf).ApplyConfiguration(); err != nil {
+		t.Fatalf("Failed to apply configuration: %v", err)
+	}
+	p := Plugin{config: conf}
+
+	rule := cpmtypes.CustomRule{
+		Path:    "./test-data/large-stdout-with-ok-exit-status.sh",
+		Timeout: &ruleTimeout,
+	}
+	gotStatus, gotOutput := p.run(rule)
+
+	if gotStatus != cpmtypes.OK {
+		t.Errorf("exit status: got %v, want %v", gotStatus, cpmtypes.OK)
+	}
+	if len(gotOutput) != maxOutputLength {
+		t.Errorf("output length: got %d, want %d (must be capped at max_output_length, not a fixed 4 KiB buffer)",
+			len(gotOutput), maxOutputLength)
+	}
+	if want := strings.Repeat("a", maxOutputLength); gotOutput != want {
+		t.Errorf("output content mismatch: got %d bytes, want %d 'a' bytes", len(gotOutput), len(want))
+	}
+}

pkg/custompluginmonitor/plugin/test-data/large-stdout-with-ok-exit-status.sh

@@ -0,0 +1,8 @@
+#!/usr/bin/env bash
+
+# Emits 16384 bytes of stdout (larger than the old hardcoded 4 KiB capture
+# buffer and larger than the test's max_output_length) with a success exit
+# code. Used to verify that plugin output capture honors max_output_length
+# rather than a fixed internal buffer size.
+head -c 16384 /dev/zero | tr '\0' 'a'
+exit 0