kubelet: use env vars in node log query PS command

- Use environment variables to pass string arguments in the node log
  query PS command
- Split getLoggingCmd into getLoggingCmdEnv and getLoggingCmdArgs
  for better modularization

pkg/features/kube_features.go

@@ -524,7 +524,8 @@ const (
 	// alpha: v1.27
 	// beta: v1.30
 	//
-	// Enables querying logs of node services using the /logs endpoint
+	// Enables querying logs of node services using the /logs endpoint. Enabling this feature has security implications.
+	// The recommendation is to enable it on a need basis for debugging purposes and disabling otherwise.
 	NodeLogQuery featuregate.Feature = "NodeLogQuery"
 
 	// owner: @xing-yang @sonasingh46

pkg/generated/openapi/zz_generated.openapi.go

@@ -62689,7 +62689,7 @@ func schema_k8sio_kubelet_config_v1beta1_KubeletConfiguration(ref common.Referen
 					},
 					"enableSystemLogQuery": {
 						SchemaProps: spec.SchemaProps{
-							Description: "enableSystemLogQuery enables the node log query feature on the /logs endpoint. EnableSystemLogHandler has to be enabled in addition for this feature to work. Default: false",
+							Description: "enableSystemLogQuery enables the node log query feature on the /logs endpoint. EnableSystemLogHandler has to be enabled in addition for this feature to work. Enabling this feature has security implications. The recommendation is to enable it on a need basis for debugging purposes and disabling otherwise. Default: false",
 							Type:        []string{"boolean"},
 							Format:      "",
 						},

pkg/kubelet/apis/config/types.go

@@ -408,6 +408,8 @@ type KubeletConfiguration struct {
 	EnableSystemLogHandler bool
 	// EnableSystemLogQuery enables the node log query feature on the /logs endpoint.
 	// EnableSystemLogHandler has to be enabled in addition for this feature to work.
+	// Enabling this feature has security implications. The recommendation is to enable it on a need basis for debugging
+	// purposes and disabling otherwise.
 	// +featureGate=NodeLogQuery
 	// +optional
 	EnableSystemLogQuery bool

pkg/kubelet/kubelet_server_journal.go

@@ -316,7 +316,7 @@ func (n *nodeLogQuery) splitNativeVsFileLoggers(ctx context.Context) ([]string,
 // copyServiceLogs invokes journalctl or Get-WinEvent with the provided args. Note that
 // services are explicitly passed here to account for the heuristics.
 func (n *nodeLogQuery) copyServiceLogs(ctx context.Context, w io.Writer, services []string, previousBoot int) {
-	cmdStr, args, err := getLoggingCmd(n, services)
+	cmdStr, args, cmdEnv, err := getLoggingCmd(n, services)
 	if err != nil {
 		fmt.Fprintf(w, "\nfailed to get logging cmd: %v\n", err)
 		return
@@ -324,6 +324,7 @@ func (n *nodeLogQuery) copyServiceLogs(ctx context.Context, w io.Writer, service
 	cmd := exec.CommandContext(ctx, cmdStr, args...)
 	cmd.Stdout = w
 	cmd.Stderr = w
+	cmd.Env = append(os.Environ(), cmdEnv...)
 
 	if err := cmd.Run(); err != nil {
 		if _, ok := err.(*exec.ExitError); ok {

pkg/kubelet/kubelet_server_journal_linux.go

@@ -26,9 +26,13 @@ import (
 )
 
 // getLoggingCmd returns the journalctl cmd and arguments for the given nodeLogQuery and boot. Note that
-// services are explicitly passed here to account for the heuristics
+// services are explicitly passed here to account for the heuristics.
-func getLoggingCmd(n *nodeLogQuery, services []string) (string, []string, error) {
+// The return values are:
-	args := []string{
+// - cmd: the command to be executed
+// - args: arguments to the command
+// - cmdEnv: environment variables when the command will be executed
+func getLoggingCmd(n *nodeLogQuery, services []string) (cmd string, args []string, cmdEnv []string, err error) {
+	args = []string{
 		"--utc",
 		"--no-pager",
 		"--output=short-precise",
@@ -55,7 +59,7 @@ func getLoggingCmd(n *nodeLogQuery, services []string) (string, []string, error)
 		args = append(args, "--boot", fmt.Sprintf("%d", *n.Boot))
 	}
 
-	return "journalctl", args, nil
+	return "journalctl", args, nil, nil
 }
 
 // checkForNativeLogger checks journalctl output for a service

pkg/kubelet/kubelet_server_journal_others.go

@@ -24,8 +24,8 @@ import (
 )
 
 // getLoggingCmd on unsupported operating systems returns the echo command and a warning message (as strings)
-func getLoggingCmd(n *nodeLogQuery, services []string) (string, []string, error) {
+func getLoggingCmd(n *nodeLogQuery, services []string) (cmd string, args []string, cmdEnv []string, err error) {
-	return "", []string{}, errors.New("Operating System Not Supported")
+	return "", args, cmdEnv, errors.New("Operating System Not Supported")
 }
 
 // checkForNativeLogger on unsupported operating systems returns false

pkg/kubelet/kubelet_server_journal_test.go

@@ -30,31 +30,62 @@ import (
 )
 
 func Test_getLoggingCmd(t *testing.T) {
+	var emptyCmdEnv []string
 	tests := []struct {
 		name              string
 		args              nodeLogQuery
+		services          []string
 		wantLinux         []string
 		wantWindows       []string
-		wantOtherOS []string
+		wantLinuxCmdEnv   []string
+		wantWindowsCmdEnv []string
 	}{
 		{
+			name:              "basic",
 			args:              nodeLogQuery{},
+			services:          []string{},
 			wantLinux:         []string{"--utc", "--no-pager", "--output=short-precise"},
+			wantLinuxCmdEnv:   emptyCmdEnv,
 			wantWindows:       []string{"-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", "Get-WinEvent -FilterHashtable @{LogName='Application'} | Sort-Object TimeCreated | Format-Table -AutoSize -Wrap"},
+			wantWindowsCmdEnv: emptyCmdEnv,
+		},
+		{
+			name:              "two providers",
+			args:              nodeLogQuery{},
+			services:          []string{"p1", "p2"},
+			wantLinux:         []string{"--utc", "--no-pager", "--output=short-precise", "--unit=p1", "--unit=p2"},
+			wantLinuxCmdEnv:   emptyCmdEnv,
+			wantWindows:       []string{"-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", "Get-WinEvent -FilterHashtable @{LogName='Application'; ProviderName=$Env:kubelet_provider0,$Env:kubelet_provider1} | Sort-Object TimeCreated | Format-Table -AutoSize -Wrap"},
+			wantWindowsCmdEnv: []string{"kubelet_provider0=p1", "kubelet_provider1=p2"},
+		},
+		{
+			name:              "empty provider",
+			args:              nodeLogQuery{},
+			services:          []string{"p1", "", "p2"},
+			wantLinux:         []string{"--utc", "--no-pager", "--output=short-precise", "--unit=p1", "--unit=p2"},
+			wantLinuxCmdEnv:   emptyCmdEnv,
+			wantWindows:       []string{"-NonInteractive", "-ExecutionPolicy", "Bypass", "-Command", "Get-WinEvent -FilterHashtable @{LogName='Application'; ProviderName=$Env:kubelet_provider0,$Env:kubelet_provider2} | Sort-Object TimeCreated | Format-Table -AutoSize -Wrap"},
+			wantWindowsCmdEnv: []string{"kubelet_provider0=p1", "kubelet_provider2=p2"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			_, got, err := getLoggingCmd(&tt.args, []string{})
+			_, got, gotCmdEnv, err := getLoggingCmd(&tt.args, tt.services)
 			switch os := runtime.GOOS; os {
 			case "linux":
 				if !reflect.DeepEqual(got, tt.wantLinux) {
 					t.Errorf("getLoggingCmd() = %v, want %v", got, tt.wantLinux)
 				}
+				if !reflect.DeepEqual(gotCmdEnv, tt.wantLinuxCmdEnv) {
+					t.Errorf("gotCmdEnv %v, wantLinuxCmdEnv %v", gotCmdEnv, tt.wantLinuxCmdEnv)
+				}
 			case "windows":
 				if !reflect.DeepEqual(got, tt.wantWindows) {
 					t.Errorf("getLoggingCmd() = %v, want %v", got, tt.wantWindows)
 				}
+				if !reflect.DeepEqual(gotCmdEnv, tt.wantWindowsCmdEnv) {
+					t.Errorf("gotCmdEnv %v, wantWindowsCmdEnv %v", gotCmdEnv, tt.wantWindowsCmdEnv)
+				}
 			default:
 				if err == nil {
 					t.Errorf("getLoggingCmd() = %v, want err", got)

pkg/kubelet/kubelet_server_journal_windows.go

@@ -27,43 +27,107 @@ import (
 
 const powershellExe = "PowerShell.exe"
 
-// getLoggingCmd returns the powershell cmd and arguments for the given nodeLogQuery and boot
+// getLoggingCmd returns the powershell cmd, arguments, and environment variables for the given nodeLogQuery and boot.
-func getLoggingCmd(n *nodeLogQuery, services []string) (string, []string, error) {
+// All string inputs are environment variables to stop subcommands expressions from being executed.
-	args := []string{
+// The return values are:
+// - cmd: the command to be executed
+// - args: arguments to the command
+// - cmdEnv: environment variables when the command will be executed
+func getLoggingCmd(n *nodeLogQuery, services []string) (cmd string, args []string, cmdEnv []string, err error) {
+	cmdEnv = getLoggingCmdEnv(n, services)
+
+	var includeSinceTime, includeUntilTime, includeTailLines, includePattern bool
+	if n.SinceTime != nil {
+		includeSinceTime = true
+	}
+	if n.UntilTime != nil {
+		includeUntilTime = true
+	}
+	if n.TailLines != nil {
+		includeTailLines = true
+	}
+	if len(n.Pattern) > 0 {
+		includePattern = true
+	}
+
+	var includeServices []bool
+	for _, service := range services {
+		includeServices = append(includeServices, len(service) > 0)
+	}
+
+	args = getLoggingCmdArgs(includeSinceTime, includeUntilTime, includeTailLines, includePattern, includeServices)
+
+	return powershellExe, args, cmdEnv, nil
+}
+
+// getLoggingCmdArgs returns arguments that need to be passed to powershellExe
+func getLoggingCmdArgs(includeSinceTime, includeUntilTime, includeTailLines, includePattern bool, services []bool) (args []string) {
+	args = []string{
 		"-NonInteractive",
 		"-ExecutionPolicy", "Bypass",
 		"-Command",
 	}
 
-	psCmd := "Get-WinEvent -FilterHashtable @{LogName='Application'"
+	psCmd := `Get-WinEvent -FilterHashtable @{LogName='Application'`
-	if n.SinceTime != nil {
+
-		psCmd += fmt.Sprintf("; StartTime='%s'", n.SinceTime.Format(dateLayout))
+	if includeSinceTime {
+		psCmd += fmt.Sprintf(`; StartTime="$Env:kubelet_sinceTime"`)
 	}
-	if n.UntilTime != nil {
+	if includeUntilTime {
-		psCmd += fmt.Sprintf("; EndTime='%s'", n.UntilTime.Format(dateLayout))
+		psCmd += fmt.Sprintf(`; EndTime="$Env:kubelet_untilTime"`)
 	}
+
 	var providers []string
-	for _, service := range services {
+	for i := range services {
-		if len(service) > 0 {
+		if services[i] {
-			providers = append(providers, "'"+service+"'")
+			providers = append(providers, fmt.Sprintf("$Env:kubelet_provider%d", i))
 		}
 	}
+
 	if len(providers) > 0 {
 		psCmd += fmt.Sprintf("; ProviderName=%s", strings.Join(providers, ","))
 	}
-	psCmd += "}"
+
-	if n.TailLines != nil {
+	psCmd += `}`
-		psCmd += fmt.Sprintf(" -MaxEvents %d", *n.TailLines)
+	if includeTailLines {
+		psCmd += fmt.Sprint(` -MaxEvents $Env:kubelet_tailLines`)
 	}
-	psCmd += " | Sort-Object TimeCreated"
+	psCmd += ` | Sort-Object TimeCreated`
-	if len(n.Pattern) > 0 {
+
-		psCmd += fmt.Sprintf(" | Where-Object -Property Message -Match '%s'", n.Pattern)
+	if includePattern {
+		psCmd += fmt.Sprintf(` | Where-Object -Property Message -Match "$Env:kubelet_pattern"`)
 	}
-	psCmd += " | Format-Table -AutoSize -Wrap"
+	psCmd += ` | Format-Table -AutoSize -Wrap`
 
 	args = append(args, psCmd)
 
-	return powershellExe, args, nil
+	return args
+}
+
+// getLoggingCmdEnv returns the environment variables that will be present when powershellExe is executed
+func getLoggingCmdEnv(n *nodeLogQuery, services []string) (cmdEnv []string) {
+	if n.SinceTime != nil {
+		cmdEnv = append(cmdEnv, fmt.Sprintf("kubelet_sinceTime=%s", n.SinceTime.Format(dateLayout)))
+	}
+	if n.UntilTime != nil {
+		cmdEnv = append(cmdEnv, fmt.Sprintf("kubelet_untilTime=%s", n.UntilTime.Format(dateLayout)))
+	}
+
+	for i, service := range services {
+		if len(service) > 0 {
+			cmdEnv = append(cmdEnv, fmt.Sprintf("kubelet_provider%d=%s", i, service))
+		}
+	}
+
+	if n.TailLines != nil {
+		cmdEnv = append(cmdEnv, fmt.Sprintf("kubelet_tailLines=%d", *n.TailLines))
+	}
+
+	if len(n.Pattern) > 0 {
+		cmdEnv = append(cmdEnv, fmt.Sprintf("kubelet_pattern=%s", n.Pattern))
+	}
+
+	return cmdEnv
 }
 
 // checkForNativeLogger always returns true for Windows

staging/src/k8s.io/kubelet/config/v1beta1/types.go

@@ -720,6 +720,8 @@ type KubeletConfiguration struct {
 	EnableSystemLogHandler *bool `json:"enableSystemLogHandler,omitempty"`
 	// enableSystemLogQuery enables the node log query feature on the /logs endpoint.
 	// EnableSystemLogHandler has to be enabled in addition for this feature to work.
+	// Enabling this feature has security implications. The recommendation is to enable it on a need basis for debugging
+	// purposes and disabling otherwise.
 	// Default: false
 	// +featureGate=NodeLogQuery
 	// +optional