Return log messages in a single line for successful render with stderr (#1039)

* Return log messages in single line in generictaskhandler

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Create a helper function to manage string manipulation

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Add unit test for the helper function

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Address copilot comments

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Add unit tests for evaluator function and use flatline for all responses

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Remove accidentally created wrapper-server binary

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Add argument to wrapper server to switch between singleline/multiline log

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* compute flattenlog only when its enabled

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

* Update unit test

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

---------

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
Co-authored-by: Liam Fallon <35595825+liamfallon@users.noreply.github.com>

Author: kushnaidu

func/wrapper-server/main.go

@@ -25,6 +25,7 @@ import (
 	"os"
 	"os/exec"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/kptdev/krm-functions-sdk/go/fn"
@@ -60,6 +61,7 @@ func main() {
 	cmd.Flags().IntVar(&op.port, "port", 9446, "The server port")
 	cmd.Flags().IntVar(&op.maxGrpcMessageSize, "max-request-body-size", 6*1024*1024, "Maximum size of grpc messages in bytes.")
 	cmd.Flags().IntVar(&op.logLevel, "verbosity", 2, "Verbosity of logs.")
+	cmd.Flags().BoolVar(&op.flattenLog, "flatten-log", false, "Flatten multi-line stderr into a single line in response Log, gRPC error messages, and warning logs.")
 
 	flagSet := flag.NewFlagSet("log-level", flag.ContinueOnError)
 	klog.InitFlags(flagSet)
@@ -76,6 +78,7 @@ type options struct {
 	maxGrpcMessageSize int
 	entrypoint         []string
 	logLevel           int
+	flattenLog         bool
 }
 
 func (o *options) run() error {
@@ -100,6 +103,7 @@ func (o *options) run() error {
 
 	evaluator := &singleFunctionEvaluator{
 		entrypoint: o.entrypoint,
+		flattenLog: o.flattenLog,
 	}
 
 	klog.Infof("Listening on %s", address)
@@ -130,6 +134,7 @@ type singleFunctionEvaluator struct {
 	pb.UnimplementedFunctionEvaluatorServer
 
 	entrypoint []string
+	flattenLog bool
 }
 
 func (e *singleFunctionEvaluator) EvaluateFunction(ctx context.Context, req *pb.EvaluateFunctionRequest) (*pb.EvaluateFunctionResponse, error) {
@@ -145,6 +150,7 @@ func (e *singleFunctionEvaluator) EvaluateFunction(ctx context.Context, req *pb.
 	var exitErr *exec.ExitError
 	outbytes := stdout.Bytes()
 	stderrStr := stderr.String()
+	stderrLog := e.stderrOutput(stderrStr)
 
 	if err != nil {
 		klog.V(4).Infof("Input Resource List: %s\nOutput Resource List: %s", req.ResourceList, outbytes)
@@ -153,12 +159,12 @@ func (e *singleFunctionEvaluator) EvaluateFunction(ctx context.Context, req *pb.
 			rl, pe := fn.ParseResourceList(outbytes)
 			if pe != nil {
 				// If we can't parse the output resource list, we only surface the content in stderr.
-				return nil, status.Errorf(codes.Internal, "failed to parse the output of function %q with stderr '%v': %+v", req.Image, stderrStr, pe)
+				return nil, status.Errorf(codes.Internal, "failed to parse the output of function %q with stderr '%v': %+v", req.Image, stderrLog, pe)
 			}
 
-			return nil, status.Errorf(codes.Internal, "failed to evaluate function %q with structured results: %v and stderr: %v", req.Image, rl.Results.Error(), stderrStr)
+			return nil, status.Errorf(codes.Internal, "failed to evaluate function %q with structured results: %v and stderr: %v", req.Image, rl.Results.Error(), stderrLog)
 		} else {
-			return nil, status.Errorf(codes.Internal, "Failed to execute function %q: %s (%s)", req.Image, err, stderrStr)
+			return nil, status.Errorf(codes.Internal, "Failed to execute function %q: %s (%s)", req.Image, err, stderrLog)
 		}
 	}
 
@@ -168,15 +174,37 @@ func (e *singleFunctionEvaluator) EvaluateFunction(ctx context.Context, req *pb.
 	if pErr != nil {
 		klog.V(4).Infof("Input Resource List: %s\nOutput Resource List: %s", req.ResourceList, outbytes)
 		// If we can't parse the output resource list, we only surface the content in stderr.
-		return nil, status.Errorf(codes.Internal, "failed to parse the output of function %q with stderr '%v': %+v", req.Image, stderrStr, pErr)
+		return nil, status.Errorf(codes.Internal, "failed to parse the output of function %q with stderr '%v': %+v", req.Image, stderrLog, pErr)
 	}
 	if rl.Results.ExitCode() != 0 {
 		jsonBytes, _ := json.Marshal(rl.Results)
-		klog.Warningf("failed to evaluate function %q with structured results: %s and stderr: %v", req.Image, jsonBytes, stderrStr)
+		klog.Warningf("failed to evaluate function %q with structured results: %s and stderr: %v", req.Image, jsonBytes, stderrLog)
 	}
 
 	return &pb.EvaluateFunctionResponse{
 		ResourceList: outbytes,
-		Log:          []byte(stderrStr),
+		Log:          []byte(stderrLog),
 	}, nil
 }
+
+// flattenStderr normalizes multi-line stderr output into a single line for safe
+// embedding in log messages and gRPC error descriptions. It handles Windows (\r\n),
+// Unix (\n), and standalone carriage returns (\r, used by progress-style output).
+// Only leading/trailing newline characters are trimmed; other whitespace (spaces,
+// tabs) is preserved.
+func flattenStderr(s string) string {
+	s = strings.ReplaceAll(s, "\r\n", "\n")
+	s = strings.ReplaceAll(s, "\r", "\n")
+	s = strings.Trim(s, "\n")
+	return strings.ReplaceAll(s, "\n", " | ")
+}
+
+// stderrOutput returns the stderr content in the appropriate form.
+// When flattenLog is enabled, it normalizes multi-line stderr into a single line;
+// otherwise it returns the raw stderr as-is, avoiding unnecessary allocations.
+func (e *singleFunctionEvaluator) stderrOutput(raw string) string {
+	if e.flattenLog {
+		return flattenStderr(raw)
+	}
+	return raw
+}

func/wrapper-server/testdata/stderr_multiline_fail_test.sh

@@ -0,0 +1,26 @@
+#!/bin/bash
+
+# Copyright 2025 The kpt 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.
+
+# This script writes multi-line stderr and exits with a non-zero code.
+# Used to test that gRPC error messages contain flattened stderr.
+
+input=$(cat)
+printf "%s" "$input"
+
+echo "Error: validation failed" >&2
+echo "Field 'name' is required" >&2
+echo "Field 'namespace' is required" >&2
+exit 1

func/wrapper-server/testdata/stderr_multiline_test.sh

@@ -0,0 +1,26 @@
+#!/bin/bash
+
+# Copyright 2025 The kpt 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.
+
+# This script passes stdin through to stdout unchanged and writes multi-line
+# output to stderr. Used to test that EvaluateFunctionResponse.Log preserves
+# the raw multi-line stderr content.
+
+input=$(cat)
+printf "%s" "$input"
+
+echo "Starting mutation" >&2
+echo "Replacing value" >&2
+echo "Completed" >&2

func/wrapper-server/wrapper_server_test.go

@@ -18,10 +18,12 @@ import (
 	"bytes"
 	"context"
 	"flag"
+	"strings"
 	"testing"
 
 	pb "github.com/kptdev/porch/func/evaluator"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/kustomize/kyaml/kio"
 )
@@ -190,3 +192,168 @@ func createMockResourceList(pkg string) []byte {
 
 	return b.Bytes()
 }
+
+func TestFlattenStderr(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "single line no newline",
+			input:    "hello world",
+			expected: "hello world",
+		},
+		{
+			name:     "multiple lines",
+			input:    "Starting mutation\nReplacing value\nCompleted",
+			expected: "Starting mutation | Replacing value | Completed",
+		},
+		{
+			name:     "trailing newline",
+			input:    "Starting mutation\nReplacing value\n",
+			expected: "Starting mutation | Replacing value",
+		},
+		{
+			name:     "leading and trailing newlines",
+			input:    "\nStarting mutation\nReplacing value\n",
+			expected: "Starting mutation | Replacing value",
+		},
+		{
+			name:     "preserves leading spaces",
+			input:    "  indented line\nnext line",
+			expected: "  indented line | next line",
+		},
+		{
+			name:     "preserves trailing spaces",
+			input:    "line with trailing space \nanother line",
+			expected: "line with trailing space  | another line",
+		},
+		{
+			name:     "windows line endings",
+			input:    "Starting mutation\r\nReplacing value\r\nCompleted\r\n",
+			expected: "Starting mutation | Replacing value | Completed",
+		},
+		{
+			name:     "mixed line endings",
+			input:    "line1\r\nline2\nline3\r\n",
+			expected: "line1 | line2 | line3",
+		},
+		{
+			name:     "standalone carriage return",
+			input:    "line1\rline2",
+			expected: "line1 | line2",
+		},
+		{
+			name:     "trailing carriage return",
+			input:    "progress output\r",
+			expected: "progress output",
+		},
+		{
+			name:     "empty string",
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "single newline",
+			input:    "\n",
+			expected: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := flattenStderr(tt.input)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestEvaluateFunction_StderrLogIsFlattened(t *testing.T) {
+	// Integration test: verifies that when --flatten-log is enabled,
+	// EvaluateFunctionResponse.Log contains flattened single-line stderr.
+	evaluator := singleFunctionEvaluator{
+		entrypoint: []string{"./testdata/stderr_multiline_test.sh"},
+		flattenLog: true,
+	}
+	req := &pb.EvaluateFunctionRequest{
+		ResourceList: createMockResourceList("./testdata/deployment.yaml"),
+		Image:        "test-stderr",
+	}
+
+	resp, err := evaluator.EvaluateFunction(context.Background(), req)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	logStr := string(resp.Log)
+	assert.Equal(t, "Starting mutation | Replacing value | Completed", logStr)
+	assert.NotContains(t, logStr, "\n", "resp.Log should be flattened, not contain raw newlines")
+}
+
+func TestEvaluateFunction_StderrLogPreservesRawByDefault(t *testing.T) {
+	// Integration test: verifies that without --flatten-log,
+	// EvaluateFunctionResponse.Log preserves the raw multi-line stderr.
+	evaluator := singleFunctionEvaluator{
+		entrypoint: []string{"./testdata/stderr_multiline_test.sh"},
+		flattenLog: false,
+	}
+	req := &pb.EvaluateFunctionRequest{
+		ResourceList: createMockResourceList("./testdata/deployment.yaml"),
+		Image:        "test-stderr",
+	}
+
+	resp, err := evaluator.EvaluateFunction(context.Background(), req)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	logStr := string(resp.Log)
+	assert.Contains(t, logStr, "Starting mutation\n")
+	assert.Contains(t, logStr, "Replacing value\n")
+	assert.Contains(t, logStr, "Completed\n")
+	assert.NotContains(t, logStr, " | ", "resp.Log should preserve raw newlines when flatten-log is disabled")
+}
+
+func TestEvaluateFunction_StderrErrorContainsFlattenedMessage(t *testing.T) {
+	// Integration test: verifies that on failure with --flatten-log enabled,
+	// the gRPC error message contains flattened (single-line) stderr.
+	evaluator := singleFunctionEvaluator{
+		entrypoint: []string{"./testdata/stderr_multiline_fail_test.sh"},
+		flattenLog: true,
+	}
+	req := &pb.EvaluateFunctionRequest{
+		ResourceList: createMockResourceList("./testdata/deployment.yaml"),
+		Image:        "test-stderr-fail",
+	}
+
+	resp, err := evaluator.EvaluateFunction(context.Background(), req)
+	require.Error(t, err)
+	require.Nil(t, resp)
+
+	errMsg := err.Error()
+	// The error string should contain pipe-separated (flattened) stderr lines.
+	assert.True(t, strings.Contains(errMsg, " | "),
+		"error message should contain flattened stderr with ' | ' separator, got: %s", errMsg)
+	assert.NotContains(t, errMsg, "\n",
+		"error message should not contain raw newlines")
+}
+
+func TestEvaluateFunction_StderrErrorPreservesRawByDefault(t *testing.T) {
+	// Integration test: verifies that on failure without --flatten-log,
+	// the gRPC error message contains raw multi-line stderr.
+	evaluator := singleFunctionEvaluator{
+		entrypoint: []string{"./testdata/stderr_multiline_fail_test.sh"},
+		flattenLog: false,
+	}
+	req := &pb.EvaluateFunctionRequest{
+		ResourceList: createMockResourceList("./testdata/deployment.yaml"),
+		Image:        "test-stderr-fail",
+	}
+
+	resp, err := evaluator.EvaluateFunction(context.Background(), req)
+	require.Error(t, err)
+	require.Nil(t, resp)
+
+	errMsg := err.Error()
+	assert.Contains(t, errMsg, "\n", "error message should include raw newlines when flatten-log is disabled")
+	assert.NotContains(t, errMsg, " | ", "error message should preserve raw newlines when flatten-log is disabled")
+}