Add checker interface

This will be useful to refactor the checkers loop.

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

cmd/kured/main.go

@@ -4,11 +4,11 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"github.com/kubereboot/kured/pkg/checkers"
 	"math/rand"
 	"net/http"
 	"net/url"
 	"os"
-	"os/exec"
 	"reflect"
 	"regexp"
 	"sort"
@@ -309,28 +309,6 @@ func flagToEnvVar(flag string) string {
 	return fmt.Sprintf("%s_%s", EnvPrefix, envVarSuffix)
 }
 
-func rebootRequired(sentinelCommand []string) bool {
-	cmd := util.NewCommand(sentinelCommand[0], sentinelCommand[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)
-		}
-	}
-	return true
-}
-
 // RebootBlocker interface should be implemented by types
 // to know if their instantiations should block a reboot
 type RebootBlocker interface {
@@ -540,9 +518,9 @@ func uncordon(client *kubernetes.Clientset, node *v1.Node) error {
 	return nil
 }
 
-func maintainRebootRequiredMetric(nodeID string, sentinelCommand []string) {
+func maintainRebootRequiredMetric(nodeID string, checker checkers.Checker) {
 	for {
-		if rebootRequired(sentinelCommand) {
+		if checker.CheckRebootRequired() {
 			rebootRequiredGauge.WithLabelValues(nodeID).Set(1)
 		} else {
 			rebootRequiredGauge.WithLabelValues(nodeID).Set(0)
@@ -630,7 +608,7 @@ func updateNodeLabels(client *kubernetes.Clientset, node *v1.Node, labels []stri
 	}
 }
 
-func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand []string, window *timewindow.TimeWindow, TTL time.Duration, releaseDelay time.Duration) {
+func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, checker checkers.Checker, window *timewindow.TimeWindow, TTL time.Duration, releaseDelay time.Duration) {
 	config, err := rest.InClusterConfig()
 	if err != nil {
 		log.Fatal(err)
@@ -671,7 +649,7 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [
 			// And (2) check if we previously annotated the node that it was in the process of being rebooted,
 			// And finally (3) if it has that annotation, to delete it.
 			// This indicates to other node tools running on the cluster that this node may be a candidate for maintenance
-			if annotateNodes && !rebootRequired(sentinelCommand) {
+			if annotateNodes && !checker.CheckRebootRequired() {
 				if _, ok := node.Annotations[KuredRebootInProgressAnnotation]; ok {
 					err := deleteNodeAnnotation(client, nodeID, KuredRebootInProgressAnnotation)
 					if err != nil {
@@ -690,7 +668,7 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [
 	preferNoScheduleTaint := taints.New(client, nodeID, preferNoScheduleTaintName, v1.TaintEffectPreferNoSchedule)
 
 	// Remove taint immediately during startup to quickly allow scheduling again.
-	if !rebootRequired(sentinelCommand) {
+	if !checker.CheckRebootRequired() {
 		preferNoScheduleTaint.Disable()
 	}
 
@@ -709,7 +687,7 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [
 			continue
 		}
 
-		if !rebootRequired(sentinelCommand) {
+		if !checker.CheckRebootRequired() {
 			log.Infof("Reboot not required")
 			preferNoScheduleTaint.Disable()
 			continue
@@ -788,19 +766,6 @@ func rebootAsRequired(nodeID string, rebooter reboot.Rebooter, sentinelCommand [
 	}
 }
 
-// buildSentinelCommand creates the shell command line which will need wrapping to escape
-// the container boundaries
-func buildSentinelCommand(rebootSentinelFile string, rebootSentinelCommand string) []string {
-	if rebootSentinelCommand != "" {
-		cmd, err := shlex.Split(rebootSentinelCommand)
-		if err != nil {
-			log.Fatalf("Error parsing provided sentinel command: %v", err)
-		}
-		return cmd
-	}
-	return []string{"test", "-f", rebootSentinelFile}
-}
-
 func root(cmd *cobra.Command, args []string) {
 	if logFormat == "json" {
 		log.SetFormatter(&log.JSONFormatter{})
@@ -817,8 +782,6 @@ func root(cmd *cobra.Command, args []string) {
 		log.Fatalf("Failed to build time window: %v", err)
 	}
 
-	sentinelCommand := buildSentinelCommand(rebootSentinelFile, rebootSentinelCommand)
-
 	log.Infof("Node ID: %s", nodeID)
 	log.Infof("Lock Annotation: %s/%s:%s", dsNamespace, dsName, lockAnnotation)
 	if lockTTL > 0 {
@@ -834,7 +797,7 @@ func root(cmd *cobra.Command, args []string) {
 	log.Infof("PreferNoSchedule taint: %s", preferNoScheduleTaintName)
 	log.Infof("Blocking Pod Selectors: %v", podSelectors)
 	log.Infof("Reboot schedule: %v", window)
-	log.Infof("Reboot check command: %s every %v", sentinelCommand, period)
+	log.Infof("Reboot period %v", period)
 	log.Infof("Concurrency: %v", concurrency)
 	log.Infof("Reboot method: %s", rebootMethod)
 
@@ -855,18 +818,31 @@ func root(cmd *cobra.Command, args []string) {
 		log.Fatalf("Invalid reboot-method configured: %s", rebootMethod)
 	}
 
+	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,
+		}
+	} else {
+		log.Infof("Sentinel checker is (unprivileged) testing for the presence of: %s", rebootSentinelFile)
+		checker = checkers.UnprivilegedRebootChecker{
+			CheckCommand: []string{"test", "-f", rebootSentinelFile},
+		}
+	}
+
 	if annotateNodes {
 		log.Infof("Will annotate nodes during kured reboot operations")
 	}
 
-	// Only wrap sentinel-command with nsenter, if a custom-command was configured, otherwise use the host-path mount
-	hostSentinelCommand := sentinelCommand
-	if rebootSentinelCommand != "" {
-		hostSentinelCommand = util.PrivilegedHostCommand(1, sentinelCommand)
-	}
-
-	go rebootAsRequired(nodeID, rebooter, hostSentinelCommand, window, lockTTL, lockReleaseDelay)
-	go maintainRebootRequiredMetric(nodeID, hostSentinelCommand)
+	go rebootAsRequired(nodeID, rebooter, checker, window, lockTTL, lockReleaseDelay)
+	go maintainRebootRequiredMetric(nodeID, checker)
 
 	http.Handle("/metrics", promhttp.Handler())
 	log.Fatal(http.ListenAndServe(fmt.Sprintf("%s:%d", metricsHost, metricsPort), nil))

cmd/kured/main_test.go

@@ -1,30 +1,11 @@
 package main
 
 import (
-	"github.com/kubereboot/kured/pkg/util"
+	"github.com/spf13/cobra"
 	"reflect"
 	"testing"
-
-	"github.com/kubereboot/kured/pkg/alerts"
-	log "github.com/sirupsen/logrus"
-	"github.com/spf13/cobra"
-	assert "gotest.tools/v3/assert"
-
-	papi "github.com/prometheus/client_golang/api"
 )
 
-type BlockingChecker struct {
-	blocking bool
-}
-
-func (fbc BlockingChecker) isBlocked() bool {
-	return fbc.blocking
-}
-
-var _ RebootBlocker = BlockingChecker{}       // Verify that Type implements Interface.
-var _ RebootBlocker = (*BlockingChecker)(nil) // Verify that *Type implements Interface.
-
-
 func Test_flagCheck(t *testing.T) {
 	var cmd *cobra.Command
 	var args []string
@@ -108,156 +89,3 @@ func Test_stripQuotes(t *testing.T) {
 		})
 	}
 }
-
-
-func Test_rebootBlocked(t *testing.T) {
-       noCheckers := []RebootBlocker{}
-       nonblockingChecker := BlockingChecker{blocking: false}
-       blockingChecker := BlockingChecker{blocking: true}
-
-       // Instantiate a prometheusClient with a broken_url
-       promClient, err := alerts.NewPromClient(papi.Config{Address: "broken_url"})
-       if err != nil {
-               log.Fatal("Can't create prometheusClient: ", err)
-       }
-       brokenPrometheusClient := PrometheusBlockingChecker{promClient: promClient, filter: nil, firingOnly: false}
-
-       type args struct {
-               blockers []RebootBlocker
-       }
-       tests := []struct {
-               name string
-               args args
-               want bool
-       }{
-               {
-                       name: "Do not block on no blocker defined",
-                       args: args{blockers: noCheckers},
-                       want: false,
-               },
-               {
-                       name: "Ensure a blocker blocks",
-                       args: args{blockers: []RebootBlocker{blockingChecker}},
-                       want: true,
-               },
-               {
-                       name: "Ensure a non-blocker doesn't block",
-                       args: args{blockers: []RebootBlocker{nonblockingChecker}},
-                       want: false,
-               },
-               {
-                       name: "Ensure one blocker is enough to block",
-                       args: args{blockers: []RebootBlocker{nonblockingChecker, blockingChecker}},
-                       want: true,
-               },
-               {
-                       name: "Do block on error contacting prometheus API",
-                       args: args{blockers: []RebootBlocker{brokenPrometheusClient}},
-                       want: true,
-               },
-       }
-       for _, tt := range tests {
-               t.Run(tt.name, func(t *testing.T) {
-                       if got := rebootBlocked(tt.args.blockers...); got != tt.want {
-                               t.Errorf("rebootBlocked() = %v, want %v", got, tt.want)
-                       }
-               })
-       }
-}
-
-
-func Test_buildSentinelCommand(t *testing.T) {
-	type args struct {
-		rebootSentinelFile    string
-		rebootSentinelCommand string
-	}
-	tests := []struct {
-		name string
-		args args
-		want []string
-	}{
-		{
-			name: "Ensure a sentinelFile generates a shell 'test' command with the right file",
-			args: args{
-				rebootSentinelFile:    "/test1",
-				rebootSentinelCommand: "",
-			},
-			want: []string{"test", "-f", "/test1"},
-		},
-		{
-			name: "Ensure a sentinelCommand has priority over a sentinelFile if both are provided (because sentinelFile is always provided)",
-			args: args{
-				rebootSentinelFile:    "/test1",
-				rebootSentinelCommand: "/sbin/reboot-required -r",
-			},
-			want: []string{"/sbin/reboot-required", "-r"},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := buildSentinelCommand(tt.args.rebootSentinelFile, tt.args.rebootSentinelCommand); !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("buildSentinelCommand() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
-
-func Test_rebootRequired(t *testing.T) {
-	type args struct {
-		sentinelCommand []string
-	}
-	tests := []struct {
-		name string
-		args args
-		want bool
-	}{
-		{
-			name: "Ensure rc = 0 means reboot required",
-			args: args{
-				sentinelCommand: []string{"true"},
-			},
-			want: true,
-		},
-		{
-			name: "Ensure rc != 0 means reboot NOT required",
-			args: args{
-				sentinelCommand: []string{"false"},
-			},
-			want: false,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := rebootRequired(tt.args.sentinelCommand); got != tt.want {
-				t.Errorf("rebootRequired() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
-
-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
-		rebootRequired(c.param)
-		assert.Equal(t, c.expectFatal, fatal)
-	}
-
-}

pkg/checkers/checker.go

@@ -0,0 +1,74 @@
+package checkers
+
+import (
+	"github.com/kubereboot/kured/pkg/util"
+	log "github.com/sirupsen/logrus"
+	"os/exec"
+)
+
+type Checker interface {
+	CheckRebootRequired() bool
+}
+
+// UnprivilegedRebootChecker is the default reboot checker.
+// It is unprivileged, and tests the presence of a files
+type UnprivilegedRebootChecker struct {
+	CheckCommand []string
+}
+
+// CheckRebootRequired runs the test command of the file
+// 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:]...)
+	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)
+		}
+	}
+	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)
+		}
+	}
+	return true
+}

pkg/checkers/checker_test.go

@@ -0,0 +1,69 @@
+package checkers
+
+import (
+	log "github.com/sirupsen/logrus"
+	assert "gotest.tools/v3/assert"
+	"testing"
+)
+
+func Test_rebootRequired(t *testing.T) {
+	type args struct {
+		sentinelCommand []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "Ensure rc = 0 means reboot required",
+			args: args{
+				sentinelCommand: []string{"true"},
+			},
+			want: true,
+		},
+		{
+			name: "Ensure rc != 0 means reboot NOT required",
+			args: args{
+				sentinelCommand: []string{"false"},
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			a := UnprivilegedRebootChecker{CheckCommand: tt.args.sentinelCommand}
+			if got := a.CheckRebootRequired(); got != tt.want {
+				t.Errorf("rebootRequired() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+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 := UnprivilegedRebootChecker{CheckCommand: c.param}
+		a.CheckRebootRequired()
+		assert.Equal(t, c.expectFatal, fatal)
+	}
+
+}