Cleanup checkers

Without this, the checkers are only shell calls: test -f
sentinelFile, or sentinelCommand.

This changes the behaviour of existing code to test file for
sentinelFile checker, and to keep the sentinel command as
a command.

However, to avoid having validation in the root loop, it moves
to use a constructor to cleanup the code.

Signed-off-by: Jean-Philippe Evrard <open-source@a.spamming.party>

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)

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
 }

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)
 	}