diff --git a/cmd/kured/main.go b/cmd/kured/main.go index 957bc4f..8524f0e 100644 --- a/cmd/kured/main.go +++ b/cmd/kured/main.go @@ -28,8 +28,6 @@ import ( "k8s.io/client-go/rest" kubectldrain "k8s.io/kubectl/pkg/drain" - "github.com/google/shlex" - shoutrrr "github.com/containrrr/shoutrrr" "github.com/kubereboot/kured/pkg/alerts" "github.com/kubereboot/kured/pkg/daemonsetlock" @@ -744,20 +742,11 @@ func root(cmd *cobra.Command, args []string) { var checker checkers.Checker // An override of rebootsentinelcommand means a privileged command if rebootSentinelCommand != "" { - log.Infof("Sentinel checker is user provided command: %s", rebootSentinelCommand) - cmd, err := shlex.Split(rebootSentinelCommand) - if err != nil { - log.Fatalf("Error parsing provided sentinel command: %v", err) - } - checker = checkers.NsEnterRebootChecker{ - CustomCheckCommand: cmd, - NamespacePid: 1, - } + log.Infof("Sentinel checker is (privileged) user provided command: %s", rebootSentinelCommand) + checker = checkers.NewCommandChecker(rebootSentinelCommand) } else { log.Infof("Sentinel checker is (unprivileged) testing for the presence of: %s", rebootSentinelFile) - checker = checkers.UnprivilegedRebootChecker{ - CheckCommand: []string{"test", "-f", rebootSentinelFile}, - } + checker = checkers.NewFileRebootChecker(rebootSentinelFile) } go rebootAsRequired(nodeID, rebooter, checker, window, lockTTL, lockReleaseDelay) diff --git a/pkg/checkers/checker.go b/pkg/checkers/checker.go index 3a0fc01..6a75b95 100644 --- a/pkg/checkers/checker.go +++ b/pkg/checkers/checker.go @@ -1,27 +1,67 @@ package checkers import ( + "github.com/google/shlex" "github.com/kubereboot/kured/pkg/util" log "github.com/sirupsen/logrus" + "os" "os/exec" ) +// Checker is the standard interface to use to check +// if a reboot is required. Its types must implement a +// CheckRebootRequired method which returns a single boolean +// clarifying whether a reboot is expected or not. type Checker interface { CheckRebootRequired() bool } -// UnprivilegedRebootChecker is the default reboot checker. +// FileRebootChecker is the default reboot checker. // It is unprivileged, and tests the presence of a files -type UnprivilegedRebootChecker struct { - CheckCommand []string +type FileRebootChecker struct { + FilePath string } -// CheckRebootRequired runs the test command of the file +// CheckRebootRequired checks the file presence // needs refactoring to also return an error, instead of leaking it inside the code. // This needs refactoring to get rid of NewCommand // This needs refactoring to only contain file location, instead of CheckCommand -func (rc UnprivilegedRebootChecker) CheckRebootRequired() bool { - cmd := util.NewCommand(rc.CheckCommand[0], rc.CheckCommand[1:]...) +func (rc FileRebootChecker) CheckRebootRequired() bool { + if _, err := os.Stat(rc.FilePath); err == nil { + return true + } + return false +} + +// NewFileRebootChecker is the constructor for the file based reboot checker +// TODO: Add extra input validation on filePath string here +func NewFileRebootChecker(filePath string) *FileRebootChecker { + return &FileRebootChecker{ + FilePath: filePath, + } +} + +// 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 +// the given PID's namespace. +type CommandChecker struct { + CheckCommand []string + NamespacePid int + Privileged bool +} + +// CheckRebootRequired 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. +func (rc CommandChecker) CheckRebootRequired() 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:]...) if err := cmd.Run(); err != nil { switch err := err.(type) { case *exec.ExitError: @@ -42,33 +82,16 @@ func (rc UnprivilegedRebootChecker) CheckRebootRequired() bool { return true } -// NsEnterRebootChecker is using a custom command to check -// if a reboot is required, but therefore needs a pid for entering the namespace, -// on top of the required command. This requires elevation. -type NsEnterRebootChecker struct { - CustomCheckCommand []string - NamespacePid int -} - -func (rc NsEnterRebootChecker) CheckRebootRequired() bool { - privCommand := util.PrivilegedHostCommand(rc.NamespacePid, rc.CustomCheckCommand) - cmd := util.NewCommand(privCommand[0], privCommand[1:]...) - if err := cmd.Run(); err != nil { - switch err := err.(type) { - case *exec.ExitError: - // We assume a non-zero exit code means 'reboot not required', but of course - // the user could have misconfigured the sentinel command or something else - // went wrong during its execution. In that case, not entering a reboot loop - // 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()) - } - return false - default: - // Something was grossly misconfigured, such as the command path being wrong. - log.Fatalf("Error invoking sentinel command: %v", err) - } +// NewCommandChecker is the constructor for the commandChecker, and by default +// runs new commands in a privileged fashion. +func NewCommandChecker(sentinelCommand string) *CommandChecker { + cmd, err := shlex.Split(sentinelCommand) + if err != nil { + log.Fatalf("Error parsing provided sentinel command: %v", err) + } + return &CommandChecker{ + CheckCommand: cmd, + NamespacePid: 1, + Privileged: true, } - return true } diff --git a/pkg/checkers/checker_test.go b/pkg/checkers/checker_test.go index f3a6894..4df9870 100644 --- a/pkg/checkers/checker_test.go +++ b/pkg/checkers/checker_test.go @@ -32,7 +32,7 @@ func Test_rebootRequired(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - a := UnprivilegedRebootChecker{CheckCommand: tt.args.sentinelCommand} + a := CommandChecker{CheckCommand: tt.args.sentinelCommand, NamespacePid: 1, Privileged: false} if got := a.CheckRebootRequired(); got != tt.want { t.Errorf("rebootRequired() = %v, want %v", got, tt.want) } @@ -61,7 +61,7 @@ func Test_rebootRequired_fatals(t *testing.T) { for _, c := range cases { fatal = false - a := UnprivilegedRebootChecker{CheckCommand: c.param} + a := CommandChecker{CheckCommand: c.param, NamespacePid: 1, Privileged: false} a.CheckRebootRequired() assert.Equal(t, c.expectFatal, fatal) }