Merge pull request #41 from ActiDoo/codex/fix-variable-substitution-for-rc_node_name

Expand reboot command environment placeholders

Author: msander

cmd/clusterrebootd/main.go

@@ -12,7 +12,6 @@ import (
 	"net/http"
 	"os"
 	"os/signal"
-	"strconv"
 	"strings"
 	"syscall"
 	"time"
@@ -116,7 +115,7 @@ func commandRunWithWriters(args []string, stdout, stderr io.Writer) int {
 		return exitConfigError
 	}
 
-	baseEnv := buildBaseEnvironment(cfg)
+	baseEnv := cfg.BaseEnvironment()
 
 	tlsConfig, err := buildEtcdTLSConfig(cfg.EtcdTLS)
 	if err != nil {
@@ -190,7 +189,10 @@ func commandRunWithWriters(args []string, stdout, stderr io.Writer) int {
 
 	reporter := orchestrator.NewStructuredReporter(cfg.NodeName, jsonLogger, metricsCollector)
 
-	runner, err := orchestrator.NewRunner(cfg, engine, healthRunner, locker, orchestrator.WithReporter(reporter))
+	runner, err := orchestrator.NewRunner(cfg, engine, healthRunner, locker,
+		orchestrator.WithReporter(reporter),
+		orchestrator.WithCommandEnvironment(baseEnv),
+	)
 	if err != nil {
 		fmt.Fprintf(stderr, "failed to initialise orchestrator: %v\n", err)
 		return exitRunError
@@ -293,7 +295,7 @@ func commandStatusWithWriters(args []string, stdout, stderr io.Writer) int {
 		return exitConfigError
 	}
 
-	baseEnv := buildBaseEnvironment(&cfgCopy)
+	baseEnv := cfgCopy.BaseEnvironment()
 	if *skipHealth {
 		baseEnv["RC_SKIP_HEALTH"] = "true"
 	}
@@ -349,7 +351,7 @@ func commandStatusWithWriters(args []string, stdout, stderr io.Writer) int {
 		defer etcdManager.Close()
 	}
 
-	runnerOptions := []orchestrator.Option{orchestrator.WithMaxLockAttempts(1)}
+	runnerOptions := []orchestrator.Option{orchestrator.WithMaxLockAttempts(1), orchestrator.WithCommandEnvironment(baseEnv)}
 	if *skipLock {
 		runnerOptions = append(runnerOptions, orchestrator.WithLockAcquisition(false, "lock acquisition skipped (--skip-lock)"))
 	}
@@ -378,39 +380,6 @@ func commandStatusWithWriters(args []string, stdout, stderr io.Writer) int {
 	return exitCodeForOutcome(outcome)
 }
 
-func buildBaseEnvironment(cfg *config.Config) map[string]string {
-	env := map[string]string{
-		"RC_NODE_NAME": cfg.NodeName,
-		"RC_DRY_RUN":   strconv.FormatBool(cfg.DryRun),
-	}
-	if cfg.LockKey != "" {
-		env["RC_LOCK_KEY"] = cfg.LockKey
-	}
-	if len(cfg.EtcdEndpoints) > 0 {
-		env["RC_ETCD_ENDPOINTS"] = strings.Join(cfg.EtcdEndpoints, ",")
-	}
-	if cfg.KillSwitchFile != "" {
-		env["RC_KILL_SWITCH_FILE"] = cfg.KillSwitchFile
-	}
-	if cfg.ClusterPolicies.MinHealthyFraction != nil {
-		env["RC_CLUSTER_MIN_HEALTHY_FRACTION"] = strconv.FormatFloat(*cfg.ClusterPolicies.MinHealthyFraction, 'f', -1, 64)
-	}
-	if cfg.ClusterPolicies.MinHealthyAbsolute != nil {
-		env["RC_CLUSTER_MIN_HEALTHY_ABSOLUTE"] = strconv.Itoa(*cfg.ClusterPolicies.MinHealthyAbsolute)
-	}
-	env["RC_CLUSTER_FORBID_IF_ONLY_FALLBACK_LEFT"] = strconv.FormatBool(cfg.ClusterPolicies.ForbidIfOnlyFallbackLeft)
-	if len(cfg.ClusterPolicies.FallbackNodes) > 0 {
-		env["RC_CLUSTER_FALLBACK_NODES"] = strings.Join(cfg.ClusterPolicies.FallbackNodes, ",")
-	}
-	if len(cfg.Windows.Allow) > 0 {
-		env["RC_WINDOWS_ALLOW"] = strings.Join(cfg.Windows.Allow, ",")
-	}
-	if len(cfg.Windows.Deny) > 0 {
-		env["RC_WINDOWS_DENY"] = strings.Join(cfg.Windows.Deny, ",")
-	}
-	return env
-}
-
 func commandValidate(args []string) int {
 	return commandValidateWithWriters(args, os.Stdout, os.Stderr)
 }

cmd/clusterrebootd/main_test.go

@@ -732,7 +732,7 @@ func TestBuildBaseEnvironmentIncludesPolicyContext(t *testing.T) {
 		},
 	}
 
-	env := buildBaseEnvironment(cfg)
+	env := cfg.BaseEnvironment()
 
 	if got := env["RC_NODE_NAME"]; got != cfg.NodeName {
 		t.Fatalf("expected RC_NODE_NAME %q, got %q", cfg.NodeName, got)
@@ -766,7 +766,7 @@ func TestBuildBaseEnvironmentIncludesPolicyContext(t *testing.T) {
 func TestBuildBaseEnvironmentOmitsUnsetPolicyContext(t *testing.T) {
 	cfg := &config.Config{}
 
-	env := buildBaseEnvironment(cfg)
+	env := cfg.BaseEnvironment()
 
 	if _, ok := env["RC_CLUSTER_MIN_HEALTHY_FRACTION"]; ok {
 		t.Fatalf("expected RC_CLUSTER_MIN_HEALTHY_FRACTION to be absent")

docs/OPERATIONS.md

@@ -52,7 +52,7 @@ it for your environment, and run the daemon with `clusterrebootd run
 2. **Define the health script** – Point `health_script` at an absolute path and
    configure `health_timeout_sec` so the runner can cancel long-running checks.
    The orchestration loop executes the script before and after lock acquisition,
-   injecting context that indicates the current phase and lock state.【F:examples/config.yaml†L39-L55】【F:cmd/clusterrebootd/main.go†L384-L409】
+  injecting context that indicates the current phase and lock state.【F:examples/config.yaml†L39-L55】【F:pkg/orchestrator/runner.go†L485-L500】
 3. **Configure the distributed lock** – Supply at least one etcd endpoint,
    namespace, and `lock_key`; ensure `lock_ttl_sec` exceeds the health timeout so
    the lease outlives the slowest permissible health check.  Enable mutual TLS by
@@ -87,13 +87,13 @@ Health scripts are the final safeguard before a reboot.  Follow these practices:
 - **Use the injected environment** – The coordinator exports static context such
   as `RC_NODE_NAME`, `RC_DRY_RUN`, `RC_LOCK_KEY`, etcd endpoints, kill switch
   location, cluster policy thresholds, fallback node list, and maintenance
-  windows so scripts do not need to re-read the YAML file.【F:cmd/clusterrebootd/main.go†L381-L409】
+  windows so scripts do not need to re-read the YAML file.【F:pkg/config/config.go†L230-L263】
 - **React to runtime hints** – Each invocation adds `RC_PHASE` (`pre-lock` or
   `post-lock`), `RC_LOCK_ENABLED`, `RC_LOCK_HELD`, and `RC_LOCK_ATTEMPTS` so
   scripts can distinguish dry runs, skipped locks, and contention scenarios.
   Diagnostics invoked with `status --skip-health` or `--skip-lock` set
   `RC_SKIP_HEALTH`/`RC_SKIP_LOCK` to `true`, allowing scripts to short-circuit
-  optional checks when operators intentionally bypass them.【F:cmd/clusterrebootd/main.go†L293-L322】【F:pkg/orchestrator/runner.go†L430-L466】
+  optional checks when operators intentionally bypass them.【F:cmd/clusterrebootd/main.go†L298-L305】【F:pkg/orchestrator/runner.go†L485-L500】
 - **Return meaningful exit codes** – Exit `0` to allow the reboot, non-zero to
   block it.  Write concise status details to stdout/stderr; they are captured in
   the JSON logs and CLI output for incident response.【F:cmd/clusterrebootd/main.go†L482-L517】
@@ -164,7 +164,7 @@ backoff, aligning with the orchestrator's internal retry logic.【F:docs/PACKAGI
 | CLI exits with code `2` and `invalid configuration` | Schema or semantic error (e.g. missing node name, TTL too small) | Run `validate-config` and fix the listed problems; the loader aggregates all validation failures to minimise iterations.【F:pkg/config/config.go†L90-L171】 |
 | `status` reports `health_blocked` with non-zero exit codes | Health script failed or timed out | Review stdout/stderr in the command output, inspect the script logs, and adjust cluster policy checks or timeouts as needed.【F:cmd/clusterrebootd/main.go†L482-L517】 |
 | `status` reports `lock_unavailable` | etcd unreachable or contended | Confirm network reachability, validate TLS credentials, and inspect the lock key metadata (node, PID, timestamp) to identify the current holder before retrying.【F:cmd/clusterrebootd/main.go†L193-L252】【F:pkg/orchestrator/runner.go†L133-L211】 |
-| Orchestration skipped with `window_denied`/`window_outside_allow` | Current time falls inside a deny window or outside all allow windows | Adjust the `windows` expressions or wait for the next permitted slot; the decision is also exported to the health script via maintenance window environment variables.【F:pkg/windows/windows.go†L29-L123】【F:cmd/clusterrebootd/main.go†L381-L409】 |
+| Orchestration skipped with `window_denied`/`window_outside_allow` | Current time falls inside a deny window or outside all allow windows | Adjust the `windows` expressions or wait for the next permitted slot; the decision is also exported to the health script via maintenance window environment variables.【F:pkg/windows/windows.go†L29-L123】【F:pkg/config/config.go†L230-L263】 |
 | Metrics server fails to start | Address already in use or invalid listen string | Update `metrics.listen` to a free address/port combination and restart the daemon; the listener prints an error during startup when binding fails.【F:cmd/clusterrebootd/main.go†L193-L252】 |
 
 ## Additional References

docs/STATE.md

@@ -35,6 +35,9 @@
 - The health script base environment now includes cluster policy thresholds,
   fallback node lists, and configured maintenance windows so gating logic can
   enforce operator intent without re-reading the configuration file.
+- Reboot command execution now expands the same environment placeholders (e.g.
+  `RC_NODE_NAME`) so the logged and invoked command reflects the active node
+  context without depending on shell-specific substitution.
 - Health script executions are annotated with runtime lock context via
   `RC_PHASE`, `RC_LOCK_ENABLED`, `RC_LOCK_HELD`, and `RC_LOCK_ATTEMPTS`, giving
   scripts enough detail to reason about contention and ensure post-lock checks

pkg/config/config.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"strconv"
 	"strings"
 	"time"
 
@@ -226,6 +227,42 @@ func (c *Config) applyDefaults() {
 	}
 }
 
+// BaseEnvironment returns the static environment variables derived from the configuration.
+// The resulting map can be extended with runtime annotations (metrics endpoints, skip flags)
+// before injecting it into health scripts or reboot command expansion.
+func (c *Config) BaseEnvironment() map[string]string {
+	env := map[string]string{
+		"RC_NODE_NAME": c.NodeName,
+		"RC_DRY_RUN":   strconv.FormatBool(c.DryRun),
+	}
+	if strings.TrimSpace(c.LockKey) != "" {
+		env["RC_LOCK_KEY"] = c.LockKey
+	}
+	if len(c.EtcdEndpoints) > 0 {
+		env["RC_ETCD_ENDPOINTS"] = strings.Join(c.EtcdEndpoints, ",")
+	}
+	if strings.TrimSpace(c.KillSwitchFile) != "" {
+		env["RC_KILL_SWITCH_FILE"] = c.KillSwitchFile
+	}
+	if c.ClusterPolicies.MinHealthyFraction != nil {
+		env["RC_CLUSTER_MIN_HEALTHY_FRACTION"] = strconv.FormatFloat(*c.ClusterPolicies.MinHealthyFraction, 'f', -1, 64)
+	}
+	if c.ClusterPolicies.MinHealthyAbsolute != nil {
+		env["RC_CLUSTER_MIN_HEALTHY_ABSOLUTE"] = strconv.Itoa(*c.ClusterPolicies.MinHealthyAbsolute)
+	}
+	env["RC_CLUSTER_FORBID_IF_ONLY_FALLBACK_LEFT"] = strconv.FormatBool(c.ClusterPolicies.ForbidIfOnlyFallbackLeft)
+	if len(c.ClusterPolicies.FallbackNodes) > 0 {
+		env["RC_CLUSTER_FALLBACK_NODES"] = strings.Join(c.ClusterPolicies.FallbackNodes, ",")
+	}
+	if len(c.Windows.Allow) > 0 {
+		env["RC_WINDOWS_ALLOW"] = strings.Join(c.Windows.Allow, ",")
+	}
+	if len(c.Windows.Deny) > 0 {
+		env["RC_WINDOWS_DENY"] = strings.Join(c.Windows.Deny, ",")
+	}
+	return env
+}
+
 func (d *DetectorConfig) applyDefaults(index int) {
 	if strings.TrimSpace(d.Name) != "" {
 		return

pkg/orchestrator/runner.go

@@ -72,6 +72,7 @@ type Runner struct {
 	lockEnabled    bool
 	lockSkipReason string
 	now            func() time.Time
+	commandEnv     map[string]string
 }
 
 // Option configures a Runner.
@@ -130,6 +131,14 @@ func WithLockAcquisition(enabled bool, reason string) Option {
 	}
 }
 
+// WithCommandEnvironment overrides the environment used to expand reboot command arguments.
+// The provided map is copied to avoid accidental mutations by callers.
+func WithCommandEnvironment(env map[string]string) Option {
+	return func(r *Runner) {
+		r.commandEnv = cloneEnv(env)
+	}
+}
+
 // WithTimeSource injects a custom time source, enabling deterministic tests.
 func WithTimeSource(fn func() time.Time) Option {
 	return func(r *Runner) {
@@ -166,6 +175,7 @@ func NewRunner(cfg *config.Config, detectors DetectorEvaluator, healthRunner Hea
 		maxLockTries:   5,
 		reporter:       NoopReporter{},
 		lockEnabled:    true,
+		commandEnv:     cfg.BaseEnvironment(),
 	}
 
 	for _, opt := range opts {
@@ -190,6 +200,9 @@ func NewRunner(cfg *config.Config, detectors DetectorEvaluator, healthRunner Hea
 	if runner.now == nil {
 		runner.now = time.Now
 	}
+	if runner.commandEnv == nil {
+		runner.commandEnv = cfg.BaseEnvironment()
+	}
 
 	windowsEval, err := windows.NewEvaluator(cfg.Windows.Allow, cfg.Windows.Deny)
 	if err != nil {
@@ -268,9 +281,7 @@ func (r *Runner) RunOnce(ctx context.Context) (out Outcome, err error) {
 		}
 		out.Message = msg
 		out.DryRun = r.cfg.DryRun
-		if len(r.cfg.RebootCommand) > 0 {
-			out.Command = append([]string(nil), r.cfg.RebootCommand...)
-		}
+		out.Command = r.expandCommand(r.cfg.RebootCommand)
 		return out, nil
 	}
 
@@ -323,11 +334,47 @@ func (r *Runner) RunOnce(ctx context.Context) (out Outcome, err error) {
 	out.Status = OutcomeReady
 	out.Message = "reboot prerequisites satisfied"
 	out.DryRun = r.cfg.DryRun
-	out.Command = append([]string(nil), r.cfg.RebootCommand...)
+	out.Command = r.expandCommand(r.cfg.RebootCommand)
 
 	return out, nil
 }
 
+func cloneEnv(src map[string]string) map[string]string {
+	if len(src) == 0 {
+		return nil
+	}
+	dst := make(map[string]string, len(src))
+	for k, v := range src {
+		dst[k] = v
+	}
+	return dst
+}
+
+func (r *Runner) expandCommand(command []string) []string {
+	if len(command) == 0 {
+		return nil
+	}
+	expanded := make([]string, len(command))
+	for i, arg := range command {
+		expanded[i] = expandWithEnv(arg, r.commandEnv)
+	}
+	return expanded
+}
+
+func expandWithEnv(input string, env map[string]string) string {
+	if !strings.Contains(input, "$") {
+		return input
+	}
+	return os.Expand(input, func(key string) string {
+		if env != nil {
+			if val, ok := env[key]; ok {
+				return val
+			}
+		}
+		return os.Getenv(key)
+	})
+}
+
 func (r *Runner) acquireLock(ctx context.Context) (lock.Lease, bool, int, error) {
 	minBackoff, maxBackoff := r.cfg.BackoffBounds()
 	if minBackoff <= 0 {

pkg/orchestrator/runner_test.go

@@ -548,6 +548,38 @@ func TestRunnerReadyDryRun(t *testing.T) {
 	}
 }
 
+func TestRunnerExpandsRebootCommandPlaceholders(t *testing.T) {
+	cfg := baseConfig()
+	cfg.RebootCommand = []string{"/sbin/shutdown", "-r", "now", "coordinated reboot for ${RC_NODE_NAME}"}
+	lease := &fakeLease{}
+	engine := &fakeEngine{steps: []evalStep{{requires: true}, {requires: true}}}
+	healthRunner := &fakeHealth{steps: []healthStep{{result: health.Result{ExitCode: 0}}, {result: health.Result{ExitCode: 0}}}}
+	locker := &fakeLocker{outcomes: []acquireOutcome{{lease: lease}}}
+
+	runner, err := NewRunner(cfg, engine, healthRunner, locker)
+	if err != nil {
+		t.Fatalf("failed to create runner: %v", err)
+	}
+
+	outcome, err := runner.RunOnce(context.Background())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if outcome.Status != OutcomeReady {
+		t.Fatalf("expected OutcomeReady, got %s", outcome.Status)
+	}
+	if len(outcome.Command) != len(cfg.RebootCommand) {
+		t.Fatalf("expected command length %d, got %d", len(cfg.RebootCommand), len(outcome.Command))
+	}
+	want := "coordinated reboot for " + cfg.NodeName
+	if got := outcome.Command[len(outcome.Command)-1]; got != want {
+		t.Fatalf("expected expanded command argument %q, got %q", want, got)
+	}
+	if cfg.RebootCommand[len(cfg.RebootCommand)-1] != "coordinated reboot for ${RC_NODE_NAME}" {
+		t.Fatalf("expected original reboot command to remain unchanged, got %q", cfg.RebootCommand[len(cfg.RebootCommand)-1])
+	}
+}
+
 func TestRunnerHealthError(t *testing.T) {
 	cfg := baseConfig()
 	engine := &fakeEngine{steps: []evalStep{{requires: true}}}