diff --git a/go.mod b/go.mod index 3b6e2b6b8..86892995d 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 001e5b5bf..841172484 100644 --- a/go.sum +++ b/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= diff --git a/internal/validators.go b/internal/validators.go index 59251e124..b0e32381d 100644 --- a/internal/validators.go +++ b/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) diff --git a/pkg/checkers/checker.go b/pkg/checkers/checker.go index d76fb0088..a696b41fd 100644 --- a/pkg/checkers/checker.go +++ b/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 } diff --git a/pkg/checkers/checker_test.go b/pkg/checkers/checker_test.go index 683bddcd0..481cca153 100644 --- a/pkg/checkers/checker_test.go +++ b/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) - } - -} diff --git a/pkg/reboot/command.go b/pkg/reboot/command.go index 6cfb61d75..869d8829c 100644 --- a/pkg/reboot/command.go +++ b/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 } diff --git a/pkg/reboot/command_test.go b/pkg/reboot/command_test.go new file mode 100644 index 000000000..f1bdc270b --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/util/util.go b/pkg/util/util.go deleted file mode 100644 index ffe748063..000000000 --- a/pkg/util/util.go +++ /dev/null @@ -1,35 +0,0 @@ -package util - -import ( - "fmt" - "os/exec" - - log "github.com/sirupsen/logrus" -) - -// NewCommand creates a new Command with stdout/stderr wired to our standard logger -func NewCommand(name string, arg ...string) *exec.Cmd { - cmd := exec.Command(name, arg...) - cmd.Stdout = log.NewEntry(log.StandardLogger()). - WithField("cmd", cmd.Args[0]). - WithField("std", "out"). - WriterLevel(log.InfoLevel) - - cmd.Stderr = log.NewEntry(log.StandardLogger()). - WithField("cmd", cmd.Args[0]). - WithField("std", "err"). - WriterLevel(log.WarnLevel) - - return cmd -} - -// PrivilegedHostCommand wraps 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 PrivilegedHostCommand(pid int, command []string) []string { - cmd := []string{"/usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", pid), "--"} - cmd = append(cmd, command...) - return cmd -} diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go deleted file mode 100644 index 650fee142..000000000 --- a/pkg/util/util_test.go +++ /dev/null @@ -1,31 +0,0 @@ -package util - -import ( - "reflect" - "testing" -) - -func Test_buildHostCommand(t *testing.T) { - type args struct { - pid int - command []string - } - tests := []struct { - name string - args args - want []string - }{ - { - name: "Ensure command will run with nsenter", - args: args{pid: 1, command: []string{"ls", "-Fal"}}, - want: []string{"/usr/bin/nsenter", "-m/proc/1/ns/mnt", "--", "ls", "-Fal"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := PrivilegedHostCommand(tt.args.pid, tt.args.command); !reflect.DeepEqual(got, tt.want) { - t.Errorf("buildHostCommand() = %v, want %v", got, tt.want) - } - }) - } -}