From 3d08c10c8a06b45993dc29198ea3751abd8f1ae9 Mon Sep 17 00:00:00 2001
From: Kevin Hannon <kehannon@redhat.com>
Date: Fri, 8 Nov 2024 16:42:31 -0500
Subject: [PATCH] fix PodLogsQuerySplitStream if feature is enabled and using
 defaults

---
 pkg/registry/core/pod/rest/log.go      |  9 ++-
 pkg/registry/core/pod/rest/log_test.go | 90 ++++++++++++++++++++++++--
 2 files changed, 90 insertions(+), 9 deletions(-)

diff --git a/pkg/registry/core/pod/rest/log.go b/pkg/registry/core/pod/rest/log.go
index 55641b79323..188e2464769 100644
--- a/pkg/registry/core/pod/rest/log.go
+++ b/pkg/registry/core/pod/rest/log.go
@@ -33,6 +33,7 @@ import (
 	"k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/kubelet/client"
 	"k8s.io/kubernetes/pkg/registry/core/pod"
+	"k8s.io/utils/ptr"
 
 	// ensure types are installed
 	_ "k8s.io/kubernetes/pkg/apis/core/install"
@@ -88,8 +89,12 @@ func (r *LogREST) Get(ctx context.Context, name string, opts runtime.Object) (ru
 
 	countSkipTLSMetric(logOpts.InsecureSkipTLSVerifyBackend)
 
-	if !utilfeature.DefaultFeatureGate.Enabled(features.PodLogsQuerySplitStreams) {
-		logOpts.Stream = nil
+	if utilfeature.DefaultFeatureGate.Enabled(features.PodLogsQuerySplitStreams) {
+		// Even with defaulters, logOpts.Stream can be nil if no arguments are provided at all.
+		if logOpts.Stream == nil {
+			// Default to "All" to maintain backward compatibility.
+			logOpts.Stream = ptr.To(api.LogStreamAll)
+		}
 	}
 	if errs := validation.ValidatePodLogOptions(logOpts, utilfeature.DefaultFeatureGate.Enabled(features.PodLogsQuerySplitStreams)); len(errs) > 0 {
 		return nil, errors.NewInvalid(api.Kind("PodLogOptions"), name, errs)
diff --git a/pkg/registry/core/pod/rest/log_test.go b/pkg/registry/core/pod/rest/log_test.go
index 9833d8693ab..77c0b66c137 100644
--- a/pkg/registry/core/pod/rest/log_test.go
+++ b/pkg/registry/core/pod/rest/log_test.go
@@ -17,14 +17,20 @@ limitations under the License.
 package rest
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/generic"
 	genericregistry "k8s.io/apiserver/pkg/registry/generic/registry"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
+	"k8s.io/utils/ptr"
 )
 
 func TestPodLogValidates(t *testing.T) {
@@ -40,16 +46,86 @@ func TestPodLogValidates(t *testing.T) {
 	}
 	logRest := &LogREST{Store: store, KubeletConn: nil}
 
+	// This test will panic if you don't have a validation failure.
 	negativeOne := int64(-1)
-	testCases := []*api.PodLogOptions{
-		{SinceSeconds: &negativeOne},
-		{TailLines: &negativeOne},
+	testCases := []struct {
+		name               string
+		podOptions         api.PodLogOptions
+		podQueryLogOptions bool
+		invalidStreamMatch string
+	}{
+		{
+			name: "SinceSeconds",
+			podOptions: api.PodLogOptions{
+				SinceSeconds: &negativeOne,
+			},
+		},
+		{
+			name: "TailLines",
+			podOptions: api.PodLogOptions{
+				TailLines: &negativeOne,
+			},
+		},
+		{
+			name: "StreamWithGateOff",
+			podOptions: api.PodLogOptions{
+				SinceSeconds: &negativeOne,
+			},
+			podQueryLogOptions: false,
+		},
+		{
+			name: "StreamWithGateOnDefault",
+			podOptions: api.PodLogOptions{
+				SinceSeconds: &negativeOne,
+			},
+			podQueryLogOptions: true,
+		},
+		{
+			name: "StreamWithGateOnAll",
+			podOptions: api.PodLogOptions{
+				SinceSeconds: &negativeOne,
+				Stream:       ptr.To(api.LogStreamAll),
+			},
+			podQueryLogOptions: true,
+		},
+		{
+			name: "StreamWithGateOnStdErr",
+			podOptions: api.PodLogOptions{
+				SinceSeconds: &negativeOne,
+				Stream:       ptr.To(api.LogStreamStderr),
+			},
+			podQueryLogOptions: true,
+		},
+		{
+			name: "StreamWithGateOnStdOut",
+			podOptions: api.PodLogOptions{
+				SinceSeconds: &negativeOne,
+				Stream:       ptr.To(api.LogStreamStdout),
+			},
+			podQueryLogOptions: true,
+		},
+		{
+			name: "StreamWithGateOnAndBadValue",
+			podOptions: api.PodLogOptions{
+				Stream: ptr.To("nostream"),
+			},
+			podQueryLogOptions: true,
+			invalidStreamMatch: "nostream",
+		},
 	}
 
 	for _, tc := range testCases {
-		_, err := logRest.Get(genericapirequest.NewDefaultContext(), "test", tc)
-		if !errors.IsInvalid(err) {
-			t.Fatalf("Unexpected error: %v", err)
-		}
+		t.Run(tc.name, func(t *testing.T) {
+			featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodLogsQuerySplitStreams, tc.podQueryLogOptions)
+			_, err := logRest.Get(genericapirequest.NewDefaultContext(), "test", &tc.podOptions)
+			if !errors.IsInvalid(err) {
+				t.Fatalf("Unexpected error: %v", err)
+			}
+			if tc.invalidStreamMatch != "" {
+				if !strings.Contains(err.Error(), "nostream") {
+					t.Error(fmt.Printf("Expected %s got %s", err.Error(), "nostream"))
+				}
+			}
+		})
 	}
 }