From f20a1ddd05b6d927e48530d97c565835571439dd Mon Sep 17 00:00:00 2001 From: Jean-Philippe Evrard Date: Tue, 5 Nov 2024 21:44:53 +0100 Subject: [PATCH 1/2] 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 --- go.mod | 2 - go.sum | 2 - internal/validators.go | 2 +- pkg/checkers/checker.go | 40 ++++++++++------- pkg/checkers/checker_test.go | 86 ++++++++++++++++++++++-------------- pkg/reboot/command.go | 20 ++++++--- pkg/reboot/command_test.go | 43 ++++++++++++++++++ pkg/util/util.go | 35 --------------- pkg/util/util_test.go | 31 ------------- 9 files changed, 136 insertions(+), 125 deletions(-) create mode 100644 pkg/reboot/command_test.go delete mode 100644 pkg/util/util.go delete mode 100644 pkg/util/util_test.go 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..736e7a239 100644 --- a/pkg/checkers/checker.go +++ b/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 } 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..7f27b6b30 100644 --- a/pkg/reboot/command.go +++ b/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 } 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) - } - }) - } -} From 94e73465adc370f8d54f40ed42d3ca234f5e21d3 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Evrard Date: Wed, 6 Nov 2024 08:38:55 +0100 Subject: [PATCH 2/2] Add stdout and stderr to log info Without this, we are loosing features based on previous logrus implementation. Now, we will log the stdout and stderr for each call. Next to this, we ensure the call of the log. methods will be ready for the switch to get rid of logrus in the future. Signed-off-by: Jean-Philippe Evrard --- pkg/checkers/checker.go | 7 ++++--- pkg/reboot/command.go | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/checkers/checker.go b/pkg/checkers/checker.go index 736e7a239..a696b41fd 100644 --- a/pkg/checkers/checker.go +++ b/pkg/checkers/checker.go @@ -7,6 +7,7 @@ import ( log "github.com/sirupsen/logrus" "os" "os/exec" + "strings" ) // Checker is the standard interface to use to check @@ -71,15 +72,15 @@ 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", cmd.Args, "stdout", bufStdout, "stderr", bufStderr) + log.Info("checking if reboot is required", "cmd", strings.Join(cmd.Args, " "), "stdout", bufStdout.String(), "stderr", bufStderr.String()) return true } diff --git a/pkg/reboot/command.go b/pkg/reboot/command.go index 7f27b6b30..869d8829c 100644 --- a/pkg/reboot/command.go +++ b/pkg/reboot/command.go @@ -6,6 +6,7 @@ import ( "github.com/google/shlex" log "github.com/sirupsen/logrus" "os/exec" + "strings" ) // CommandRebooter holds context-information for a reboot with command @@ -24,9 +25,9 @@ func (c CommandRebooter) Reboot() error { cmd.Stderr = bufStderr if err := cmd.Run(); err != nil { - return fmt.Errorf("error invoking reboot command %s: %v", c.RebootCommand, err) + 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", cmd.Args, "stdout", bufStdout, "stderr", bufStderr) + log.Info("Invoked reboot command", "cmd", strings.Join(cmd.Args, " "), "stdout", bufStdout.String(), "stderr", bufStderr.String()) return nil }