Fix goroutine leak

Without this patch, we use WriterLevel, which spawns
go routines. As we do it at every call of the util commands,
we spawn goroutines at every check.

This is a problem as it leads to memory management issues.

This fixes it by using a buffer for stdout and stderr, then
logging the results after the command was executed.

To make sure the logging happened at the same place, I inlined
the code from utils. This results in duplicated the code.

However, this is not a big problem as:
- It makes the code more readable
- The implementation between checkers and rebooters _ARE_
  different -- One definitely NEEDS privileges, while the other
  does not... Which could lead to later improvements.

Removing a "utils" package is not really a big deal (it
is kinda a win in itself, as it is an anti-pattern), as the
test coverage was kept.

Partial-Fix: #1004
Fixes: #1013
Signed-off-by: Jean-Philippe Evrard <[email protected]>

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,9 +1,9 @@
 package checkers
 
 import (
+	"bytes"
 	"fmt"
 	"github.com/google/shlex"
-	"github.com/kubereboot/kured/pkg/util"
 	log "github.com/sirupsen/logrus"
 	"os"
 	"os/exec"
@@ -44,7 +44,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 +53,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:
@@ -80,19 +79,30 @@ func (rc CommandChecker) RebootRequired() bool {
 			log.Fatalf("Error invoking sentinel command: %v", err)
 		}
 	}
+	log.Info("checking if reboot is required", "cmd", cmd.Args, "stdout", bufStdout, "stderr", bufStderr)
 	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,11 @@
 package reboot
 
 import (
+	"bytes"
 	"fmt"
 	"github.com/google/shlex"
-	"github.com/kubereboot/kured/pkg/util"
 	log "github.com/sirupsen/logrus"
+	"os/exec"
 )
 
 // CommandRebooter holds context-information for a reboot with command
@@ -15,9 +16,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 {
+
+	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", c.RebootCommand, err)
 	}
+	log.Info("Invoked reboot command", "cmd", cmd.Args, "stdout", bufStdout, "stderr", bufStderr)
 	return nil
 }
 
@@ -28,10 +37,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)
+			}
+		})
+	}
+}