Merge pull request #1015 from evrardjp/fix_goroutine_leak

Fix goroutine leak

go.mod

@@ -12,7 +12,6 @@ require (
 	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0
-	gotest.tools/v3 v3.5.1
 	k8s.io/api v0.29.9
 	k8s.io/apimachinery v0.29.9
 	k8s.io/client-go v0.29.9
@@ -39,7 +38,6 @@ require (
 	github.com/golang/protobuf v1.5.4 // indirect
 	github.com/google/btree v1.0.1 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/uuid v1.4.0 // indirect
 	github.com/gorilla/websocket v1.5.0 // indirect

go.sum

@@ -295,8 +295,6 @@ gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
-gotest.tools/v3 v3.5.1 h1:EENdUnS3pdur5nybKYIh2Vfgc8IUNBjxDPSjtiJcOzU=
-gotest.tools/v3 v3.5.1/go.mod h1:isy3WKz7GK6uNw/sbHzfKBLvlvXwUyV06n6brMxxopU=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 k8s.io/api v0.29.9 h1:FwdflpNsfMUYUOblMZNWJ4K/q0OSL5A4jGa0iOgcJco=

internal/validators.go

@@ -28,7 +28,7 @@ func NewRebootChecker(rebootSentinelCommand string, rebootSentinelFile string) (
 	// An override of rebootSentinelCommand means a privileged command
 	if rebootSentinelCommand != "" {
 		log.Infof("Sentinel checker is (privileged) user provided command: %s", rebootSentinelCommand)
-		return checkers.NewCommandChecker(rebootSentinelCommand)
+		return checkers.NewCommandChecker(rebootSentinelCommand, 1, true)
 	}
 	log.Infof("Sentinel checker is (unprivileged) testing for the presence of: %s", rebootSentinelFile)
 	return checkers.NewFileRebootChecker(rebootSentinelFile)

pkg/checkers/checker.go

@@ -1,12 +1,13 @@
 package checkers
 
 import (
+	"bytes"
 	"fmt"
 	"github.com/google/shlex"
-	"github.com/kubereboot/kured/pkg/util"
 	log "github.com/sirupsen/logrus"
 	"os"
 	"os/exec"
+	"strings"
 )
 
 // Checker is the standard interface to use to check
@@ -44,7 +45,7 @@ func NewFileRebootChecker(filePath string) (*FileRebootChecker, error) {
 
 // CommandChecker is using a custom command to check
 // if a reboot is required. There are two modes of behaviour,
-// if Privileged is granted, the NamespacePid is used to enter
+// if Privileged is granted, the NamespacePid is used to nsenter
 // the given PID's namespace.
 type CommandChecker struct {
 	CheckCommand []string
@@ -53,16 +54,15 @@ type CommandChecker struct {
 }
 
 // RebootRequired for CommandChecker runs a command without returning
-// any eventual error. THis should be later refactored to remove the util wrapper
-// and return the errors, instead of logging them here.
+// any eventual error. This should be later refactored to return the errors,
+// instead of logging and fataling them here.
 func (rc CommandChecker) RebootRequired() bool {
-	var cmdline []string
-	if rc.Privileged {
-		cmdline = util.PrivilegedHostCommand(rc.NamespacePid, rc.CheckCommand)
-	} else {
-		cmdline = rc.CheckCommand
-	}
-	cmd := util.NewCommand(cmdline[0], cmdline[1:]...)
+	bufStdout := new(bytes.Buffer)
+	bufStderr := new(bytes.Buffer)
+	cmd := exec.Command(rc.CheckCommand[0], rc.CheckCommand[1:]...)
+	cmd.Stdout = bufStdout
+	cmd.Stderr = bufStderr
+
 	if err := cmd.Run(); err != nil {
 		switch err := err.(type) {
 		case *exec.ExitError:
@@ -72,27 +72,38 @@ func (rc CommandChecker) RebootRequired() bool {
 			// is the right thing to do, and we are logging stdout/stderr of the command
 			// so it should be obvious what is wrong.
 			if cmd.ProcessState.ExitCode() != 1 {
-				log.Warnf("sentinel command ended with unexpected exit code: %v", cmd.ProcessState.ExitCode())
+				log.Warn(fmt.Sprintf("sentinel command ended with unexpected exit code: %v", cmd.ProcessState.ExitCode()), "cmd", strings.Join(cmd.Args, " "), "stdout", bufStdout.String(), "stderr", bufStderr.String())
 			}
 			return false
 		default:
 			// Something was grossly misconfigured, such as the command path being wrong.
-			log.Fatalf("Error invoking sentinel command: %v", err)
+			log.Fatal(fmt.Sprintf("Error invoking sentinel command: %v", err), "cmd", strings.Join(cmd.Args, " "), "stdout", bufStdout.String(), "stderr", bufStderr.String())
 		}
 	}
+	log.Info("checking if reboot is required", "cmd", strings.Join(cmd.Args, " "), "stdout", bufStdout.String(), "stderr", bufStderr.String())
 	return true
 }
 
 // NewCommandChecker is the constructor for the commandChecker, and by default
 // runs new commands in a privileged fashion.
-func NewCommandChecker(sentinelCommand string) (*CommandChecker, error) {
-	cmd, err := shlex.Split(sentinelCommand)
+// Privileged means wrapping the command with nsenter.
+// It allows to run a command from systemd's namespace for example (pid 1)
+// This relies on hostPID:true and privileged:true to enter host mount space
+// For info, rancher based need different pid, which should be user given.
+// until we have a better discovery mechanism.
+func NewCommandChecker(sentinelCommand string, pid int, privileged bool) (*CommandChecker, error) {
+	var cmd []string
+	if privileged {
+		cmd = append(cmd, "/usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", pid), "--")
+	}
+	parsedCommand, err := shlex.Split(sentinelCommand)
 	if err != nil {
 		return nil, fmt.Errorf("error parsing provided sentinel command: %v", err)
 	}
+	cmd = append(cmd, parsedCommand...)
 	return &CommandChecker{
 		CheckCommand: cmd,
-		NamespacePid: 1,
-		Privileged:   true,
+		NamespacePid: pid,
+		Privileged:   privileged,
 	}, nil
 }

pkg/checkers/checker_test.go

@@ -2,68 +2,86 @@ package checkers
 
 import (
 	log "github.com/sirupsen/logrus"
-	assert "gotest.tools/v3/assert"
+	"reflect"
 	"testing"
 )
 
-func Test_rebootRequired(t *testing.T) {
+func Test_nsEntering(t *testing.T) {
 	type args struct {
-		sentinelCommand []string
+		pid        int
+		command    string
+		privileged bool
 	}
 	tests := []struct {
 		name string
 		args args
-		want bool
+		want []string
+	}{
+		{
+			name: "Ensure command will run with nsenter",
+			args: args{pid: 1, command: "ls -Fal", privileged: true},
+			want: []string{"/usr/bin/nsenter", "-m/proc/1/ns/mnt", "--", "ls", "-Fal"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cc, _ := NewCommandChecker(tt.args.command, tt.args.pid, tt.args.privileged)
+			if !reflect.DeepEqual(cc.CheckCommand, tt.want) {
+				t.Errorf("command parsed as %v, want %v", cc.CheckCommand, tt.want)
+			}
+		})
+	}
+}
+
+func Test_rebootRequired(t *testing.T) {
+	type args struct {
+		sentinelCommand []string
+	}
+	tests := []struct {
+		name   string
+		args   args
+		want   bool
+		fatals bool
 	}{
 		{
 			name: "Ensure rc = 0 means reboot required",
 			args: args{
 				sentinelCommand: []string{"true"},
 			},
-			want: true,
+			want:   true,
+			fatals: false,
 		},
 		{
 			name: "Ensure rc != 0 means reboot NOT required",
 			args: args{
 				sentinelCommand: []string{"false"},
 			},
-			want: false,
+			want:   false,
+			fatals: false,
+		},
+		{
+			name: "Ensure a wrong command fatals",
+			args: args{
+				sentinelCommand: []string{"./babar"},
+			},
+			want:   true,
+			fatals: true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			defer func() { log.StandardLogger().ExitFunc = nil }()
+			fatal := false
+			log.StandardLogger().ExitFunc = func(int) { fatal = true }
+
 			a := CommandChecker{CheckCommand: tt.args.sentinelCommand, NamespacePid: 1, Privileged: false}
+
 			if got := a.RebootRequired(); got != tt.want {
 				t.Errorf("rebootRequired() = %v, want %v", got, tt.want)
 			}
+			if tt.fatals != fatal {
+				t.Errorf("fatal flag is %v, want fatal %v", fatal, tt.fatals)
+			}
 		})
 	}
 }
-
-func Test_rebootRequired_fatals(t *testing.T) {
-	cases := []struct {
-		param       []string
-		expectFatal bool
-	}{
-		{
-			param:       []string{"true"},
-			expectFatal: false,
-		},
-		{
-			param:       []string{"./babar"},
-			expectFatal: true,
-		},
-	}
-
-	defer func() { log.StandardLogger().ExitFunc = nil }()
-	var fatal bool
-	log.StandardLogger().ExitFunc = func(int) { fatal = true }
-
-	for _, c := range cases {
-		fatal = false
-		a := CommandChecker{CheckCommand: c.param, NamespacePid: 1, Privileged: false}
-		a.RebootRequired()
-		assert.Equal(t, c.expectFatal, fatal)
-	}
-
-}

pkg/reboot/command.go

@@ -1,10 +1,12 @@
 package reboot
 
 import (
+	"bytes"
 	"fmt"
 	"github.com/google/shlex"
-	"github.com/kubereboot/kured/pkg/util"
 	log "github.com/sirupsen/logrus"
+	"os/exec"
+	"strings"
 )
 
 // CommandRebooter holds context-information for a reboot with command
@@ -15,9 +17,17 @@ type CommandRebooter struct {
 // Reboot triggers the reboot command
 func (c CommandRebooter) Reboot() error {
 	log.Infof("Invoking command: %s", c.RebootCommand)
-	if err := util.NewCommand(c.RebootCommand[0], c.RebootCommand[1:]...).Run(); err != nil {
-		return fmt.Errorf("error invoking reboot command %s: %v", c.RebootCommand, err)
+
+	bufStdout := new(bytes.Buffer)
+	bufStderr := new(bytes.Buffer)
+	cmd := exec.Command(c.RebootCommand[0], c.RebootCommand[1:]...)
+	cmd.Stdout = bufStdout
+	cmd.Stderr = bufStderr
+
+	if err := cmd.Run(); err != nil {
+		return fmt.Errorf("error invoking reboot command %s: %v (stdout: %v, stderr: %v)", c.RebootCommand, err, bufStdout.String(), bufStderr.String())
 	}
+	log.Info("Invoked reboot command", "cmd", strings.Join(cmd.Args, " "), "stdout", bufStdout.String(), "stderr", bufStderr.String())
 	return nil
 }
 
@@ -28,10 +38,11 @@ func NewCommandRebooter(rebootCommand string) (*CommandRebooter, error) {
 	if rebootCommand == "" {
 		return nil, fmt.Errorf("no reboot command specified")
 	}
-	cmd, err := shlex.Split(rebootCommand)
+	cmd := []string{"/usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", 1), "--"}
+	parsedCommand, err := shlex.Split(rebootCommand)
 	if err != nil {
 		return nil, fmt.Errorf("error %v when parsing reboot command %s", err, rebootCommand)
 	}
-
-	return &CommandRebooter{RebootCommand: util.PrivilegedHostCommand(1, cmd)}, nil
+	cmd = append(cmd, parsedCommand...)
+	return &CommandRebooter{RebootCommand: cmd}, nil
 }

pkg/reboot/command_test.go

@@ -0,0 +1,43 @@
+package reboot
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestNewCommandRebooter(t *testing.T) {
+	type args struct {
+		rebootCommand string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    *CommandRebooter
+		wantErr bool
+	}{
+		{
+			name:    "Ensure command is nsenter wrapped",
+			args:    args{"ls -Fal"},
+			want:    &CommandRebooter{RebootCommand: []string{"/usr/bin/nsenter", "-m/proc/1/ns/mnt", "--", "ls", "-Fal"}},
+			wantErr: false,
+		},
+		{
+			name:    "Ensure empty command is erroring",
+			args:    args{""},
+			want:    nil,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := NewCommandRebooter(tt.args.rebootCommand)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("NewCommandRebooter() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("NewCommandRebooter() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}