Merge pull request #127832 from skitt/securejoin

kubelet: use new securejoin API

pkg/kubelet/kubelet_server_journal.go

@@ -343,7 +343,7 @@ func copyFileLogs(ctx context.Context, w io.Writer, services []string) {
 	}
 
 	for _, service := range services {
-		heuristicsCopyFileLogs(ctx, w, service)
+		heuristicsCopyFileLogs(ctx, w, nodeLogDir, service)
 	}
 }
 
@@ -352,7 +352,7 @@ func copyFileLogs(ctx context.Context, w io.Writer, services []string) {
 // /var/log/service.log or
 // /var/log/service/service.log or
 // in that order stopping on first success.
-func heuristicsCopyFileLogs(ctx context.Context, w io.Writer, service string) {
+func heuristicsCopyFileLogs(ctx context.Context, w io.Writer, logDir, service string) {
 	logFileNames := [3]string{
 		service,
 		fmt.Sprintf("%s.log", service),
@@ -361,12 +361,7 @@ func heuristicsCopyFileLogs(ctx context.Context, w io.Writer, service string) {
 
 	var err error
 	for _, logFileName := range logFileNames {
-		var logFile string
+		err = heuristicsCopyFileLog(ctx, w, logDir, logFileName)
-		logFile, err = securejoin.SecureJoin(nodeLogDir, logFileName)
-		if err != nil {
-			break
-		}
-		err = heuristicsCopyFileLog(ctx, w, logFile)
 		if err == nil {
 			break
 		} else if errors.Is(err, os.ErrNotExist) {
@@ -407,30 +402,6 @@ func newReaderCtx(ctx context.Context, r io.Reader) io.Reader {
 	}
 }
 
-// heuristicsCopyFileLog returns the contents of the given logFile
-func heuristicsCopyFileLog(ctx context.Context, w io.Writer, logFile string) error {
-	fInfo, err := os.Stat(logFile)
-	if err != nil {
-		return err
-	}
-	// This is to account for the heuristics where logs for service foo
-	// could be in /var/log/foo/
-	if fInfo.IsDir() {
-		return os.ErrNotExist
-	}
-
-	f, err := os.Open(logFile)
-	if err != nil {
-		return err
-	}
-	defer f.Close()
-
-	if _, err := io.Copy(w, newReaderCtx(ctx, f)); err != nil {
-		return err
-	}
-	return nil
-}
-
 func safeServiceName(s string) error {
 	// Max length of a service name is 256 across supported OSes
 	if len(s) > maxServiceLength {

pkg/kubelet/kubelet_server_journal_linux.go

@@ -21,8 +21,13 @@ package kubelet
 import (
 	"context"
 	"fmt"
+	"io"
+	"os"
 	"os/exec"
 	"strings"
+
+	securejoin "github.com/cyphar/filepath-securejoin"
+	unix "golang.org/x/sys/unix"
 )
 
 // getLoggingCmd returns the journalctl cmd and arguments for the given nodeLogQuery and boot. Note that
@@ -72,3 +77,32 @@ func checkForNativeLogger(ctx context.Context, service string) bool {
 	// hence we search for it in the list of services known to journalctl
 	return strings.Contains(string(output), service+".service")
 }
+
+// heuristicsCopyFileLog returns the contents of the given logFile
+func heuristicsCopyFileLog(ctx context.Context, w io.Writer, logDir, logFileName string) error {
+	f, err := securejoin.OpenInRoot(logDir, logFileName)
+	if err != nil {
+		return err
+	}
+	// Ignoring errors when closing a file opened read-only doesn't cause data loss
+	defer func() { _ = f.Close() }()
+	fInfo, err := f.Stat()
+	if err != nil {
+		return err
+	}
+	// This is to account for the heuristics where logs for service foo
+	// could be in /var/log/foo/
+	if fInfo.IsDir() {
+		return os.ErrNotExist
+	}
+	rf, err := securejoin.Reopen(f, unix.O_RDONLY)
+	if err != nil {
+		return err
+	}
+	defer func() { _ = rf.Close() }()
+
+	if _, err := io.Copy(w, newReaderCtx(ctx, rf)); err != nil {
+		return err
+	}
+	return nil
+}

pkg/kubelet/kubelet_server_journal_nonlinux.go

@@ -0,0 +1,56 @@
+//go:build !linux
+
+/*
+Copyright 2024 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 kubelet
+
+import (
+	"context"
+	"io"
+	"os"
+
+	securejoin "github.com/cyphar/filepath-securejoin"
+)
+
+// heuristicsCopyFileLog returns the contents of the given logFile
+func heuristicsCopyFileLog(ctx context.Context, w io.Writer, logDir, logFileName string) error {
+	logFile, err := securejoin.SecureJoin(logDir, logFileName)
+	if err != nil {
+		return err
+	}
+	fInfo, err := os.Stat(logFile)
+	if err != nil {
+		return err
+	}
+	// This is to account for the heuristics where logs for service foo
+	// could be in /var/log/foo/
+	if fInfo.IsDir() {
+		return os.ErrNotExist
+	}
+
+	f, err := os.Open(logFile)
+	if err != nil {
+		return err
+	}
+	// Ignoring errors when closing a file opened read-only doesn't cause data loss
+	defer func() { _ = f.Close() }()
+
+	if _, err := io.Copy(w, newReaderCtx(ctx, f)); err != nil {
+		return err
+	}
+	return nil
+}

pkg/kubelet/kubelet_server_journal_others.go

@@ -24,11 +24,11 @@ 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(_ *nodeLogQuery, _ []string) (string, []string, error) {
 	return "", []string{}, errors.New("Operating System Not Supported")
 }
 
 // checkForNativeLogger on unsupported operating systems returns false
-func checkForNativeLogger(ctx context.Context, service string) bool {
+func checkForNativeLogger(_ context.Context, _ string) bool {
 	return false
 }

pkg/kubelet/kubelet_server_journal_test.go

@@ -17,7 +17,11 @@ limitations under the License.
 package kubelet
 
 import (
+	"bytes"
+	"context"
 	"net/url"
+	"os"
+	"path/filepath"
 	"reflect"
 	"runtime"
 	"strings"
@@ -27,6 +31,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/stretchr/testify/assert"
+	"k8s.io/utils/ptr"
 )
 
 func Test_getLoggingCmd(t *testing.T) {
@@ -92,15 +97,15 @@ func Test_newNodeLogQuery(t *testing.T) {
 		{query: url.Values{"untilTime": []string{validTimeValue}},
 			want: &nodeLogQuery{options: options{UntilTime: &validT}}},
 
-		{query: url.Values{"tailLines": []string{"100"}}, want: &nodeLogQuery{options: options{TailLines: intPtr(100)}}},
+		{query: url.Values{"tailLines": []string{"100"}}, want: &nodeLogQuery{options: options{TailLines: ptr.To(100)}}},
 		{query: url.Values{"tailLines": []string{"foo"}}, wantErr: true},
 		{query: url.Values{"tailLines": []string{" "}}, wantErr: true},
 
 		{query: url.Values{"pattern": []string{"foo"}}, want: &nodeLogQuery{options: options{Pattern: "foo"}}},
 
 		{query: url.Values{"boot": []string{""}}, want: nil},
-		{query: url.Values{"boot": []string{"0"}}, want: &nodeLogQuery{options: options{Boot: intPtr(0)}}},
+		{query: url.Values{"boot": []string{"0"}}, want: &nodeLogQuery{options: options{Boot: ptr.To(0)}}},
-		{query: url.Values{"boot": []string{"-23"}}, want: &nodeLogQuery{options: options{Boot: intPtr(-23)}}},
+		{query: url.Values{"boot": []string{"-23"}}, want: &nodeLogQuery{options: options{Boot: ptr.To(-23)}}},
 		{query: url.Values{"boot": []string{"foo"}}, wantErr: true},
 		{query: url.Values{"boot": []string{" "}}, wantErr: true},
 
@@ -130,12 +135,13 @@ func Test_validateServices(t *testing.T) {
 	var (
 		service1                 = "svc1"
 		service2                 = "svc2"
-		service3 = "svc.foo"
+		serviceDot               = "svc.foo"
-		service4 = "svc_foo"
+		serviceUnderscore        = "svc_foo"
-		service5 = "svc@foo"
+		serviceAt                = "svc@foo"
-		service6 = "svc:foo"
+		serviceColon             = "svc:foo"
-		invalid1 = "svc\n"
+		invalidServiceNewline    = "svc\n"
-		invalid2 = "svc.foo\n"
+		invalidServiceNewlineDot = "svc.foo\n"
+		invalidServiceSlash      = "svc/foo"
 	)
 	tests := []struct {
 		name     string
@@ -144,14 +150,15 @@ func Test_validateServices(t *testing.T) {
 	}{
 		{name: "one service", services: []string{service1}},
 		{name: "two services", services: []string{service1, service2}},
-		{name: "dot service", services: []string{service3}},
+		{name: "dot service", services: []string{serviceDot}},
-		{name: "underscore service", services: []string{service4}},
+		{name: "underscore service", services: []string{serviceUnderscore}},
-		{name: "at service", services: []string{service5}},
+		{name: "at service", services: []string{serviceAt}},
-		{name: "colon service", services: []string{service6}},
+		{name: "colon service", services: []string{serviceColon}},
-		{name: "invalid service new line", services: []string{invalid1}, wantErr: true},
+		{name: "invalid service new line", services: []string{invalidServiceNewline}, wantErr: true},
-		{name: "invalid service with dot", services: []string{invalid2}, wantErr: true},
+		{name: "invalid service with dot", services: []string{invalidServiceNewlineDot}, wantErr: true},
+		{name: "invalid service with slash", services: []string{invalidServiceSlash}, wantErr: true},
 		{name: "long service", services: []string{strings.Repeat(service1, 100)}, wantErr: true},
-		{name: "max number of services", services: []string{service1, service2, service3, service4, service5}, wantErr: true},
+		{name: "max number of services", services: []string{service1, service2, serviceDot, serviceUnderscore, serviceAt}, wantErr: true},
 	}
 	for _, tt := range tests {
 		errs := validateServices(tt.services)
@@ -198,10 +205,10 @@ func Test_nodeLogQuery_validate(t *testing.T) {
 		{name: "since until", Services: []string{service1}, options: options{SinceTime: &until, UntilTime: &since},
 			wantErr: true},
 		// boot is not supported on Windows.
-		{name: "boot", Services: []string{service1}, options: options{Boot: intPtr(-1)}, wantErr: runtime.GOOS == "windows"},
+		{name: "boot", Services: []string{service1}, options: options{Boot: ptr.To(-1)}, wantErr: runtime.GOOS == "windows"},
-		{name: "boot out of range", Services: []string{service1}, options: options{Boot: intPtr(1)}, wantErr: true},
+		{name: "boot out of range", Services: []string{service1}, options: options{Boot: ptr.To(1)}, wantErr: true},
-		{name: "tailLines", Services: []string{service1}, options: options{TailLines: intPtr(100)}},
+		{name: "tailLines", Services: []string{service1}, options: options{TailLines: ptr.To(100)}},
-		{name: "tailLines out of range", Services: []string{service1}, options: options{TailLines: intPtr(100000)}},
+		{name: "tailLines out of range", Services: []string{service1}, options: options{TailLines: ptr.To(100000)}},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -219,6 +226,63 @@ func Test_nodeLogQuery_validate(t *testing.T) {
 	}
 }
 
-func intPtr(i int) *int {
+func Test_heuristicsCopyFileLogs(t *testing.T) {
-	return &i
+	ctx := context.TODO()
+	buf := &bytes.Buffer{}
+
+	dir, err := os.MkdirTemp("", "logs")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = os.RemoveAll(dir) }()
+
+	// Check missing logs
+	heuristicsCopyFileLogs(ctx, buf, dir, "service.log")
+	if !strings.Contains(buf.String(), "log not found for service.log") {
+		t.Fail()
+	}
+	buf.Reset()
+
+	// Check missing service logs
+	heuristicsCopyFileLogs(ctx, buf, dir, "service")
+	if !strings.Contains(buf.String(), "log not found for service") {
+		t.Fail()
+	}
+	buf.Reset()
+
+	// Check explicitly-named files
+	if err := os.WriteFile(filepath.Join(dir, "service.log"), []byte("valid logs"), 0o444); err != nil {
+		t.Fatal(err)
+	}
+	heuristicsCopyFileLogs(ctx, buf, dir, "service.log")
+	if buf.String() != "valid logs" {
+		t.Fail()
+	}
+	buf.Reset()
+
+	// Check service logs
+	heuristicsCopyFileLogs(ctx, buf, dir, "service")
+	if buf.String() != "valid logs" {
+		t.Fail()
+	}
+	buf.Reset()
+
+	// Check that a directory doesn't cause errors
+	if err := os.Mkdir(filepath.Join(dir, "service"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	heuristicsCopyFileLogs(ctx, buf, dir, "service")
+	if buf.String() != "valid logs" {
+		t.Fail()
+	}
+	buf.Reset()
+
+	// Check that service logs return the first matching file
+	if err := os.WriteFile(filepath.Join(dir, "service", "service.log"), []byte("error"), 0o444); err != nil {
+		t.Fatal(err)
+	}
+	heuristicsCopyFileLogs(ctx, buf, dir, "service")
+	if buf.String() != "valid logs" {
+		t.Fail()
+	}
 }