fix!: properly check script exit codes, properly fail scripts if != 0

Previous code was not distinguishing between failures related to shell/parsing libraries vs the scripts themselves returning non-zero exit codes and failing. This reworks the `shell.Run()` func to actually return the scripts exit code, as well as updates the callsites where it's used to better track/log the info for users.
tjhop · Jun 17, 2024 · 987581a · 987581a
1 parent 7476e5c
commit 987581a
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 20 deletions.
diff --git a/internal/manager/directive.go b/internal/manager/directive.go
@@ -69,7 +69,7 @@ func (mgr *Manager) RunDirective(ctx context.Context, ds Directive) error {
 			return fmt.Errorf("Failed to template script: %s", err)
 		}
 
-		err = shell.Run(ctx, runID, ds.String(), renderedScript, nil)
+		rc, err := shell.Run(ctx, runID, ds.String(), renderedScript, nil)
 		mgr.executedDirectives[ds.String()] = struct{}{} // mark directive as executed
 
 		// update metrics regardless of error, so do them before handling error
@@ -80,7 +80,12 @@ func (mgr *Manager) RunDirective(ctx context.Context, ds Directive) error {
 
 		if err != nil {
 			metricManagerDirectiveRunFailedTotal.With(labels).Inc()
-			return fmt.Errorf("Failed to apply directive: %v", err)
+			return fmt.Errorf("Failed to apply directive, error: %v", err)
+		}
+
+		if rc != 0 {
+			metricManagerDirectiveRunFailedTotal.With(labels).Inc()
+			return fmt.Errorf("Failed to apply directive, non-zero exit code returned: %d", rc)
 		}
 	}
 

diff --git a/internal/manager/module.go b/internal/manager/module.go
@@ -142,11 +142,27 @@ func (mgr *Manager) RunModule(ctx context.Context, logger *slog.Logger, mod Modu
 			return fmt.Errorf("Failed to template script: %s", err)
 		}
 
-		if err := shell.Run(ctx, runID, mod.m.Test, renderedTest, allVars); err != nil {
+		rc, err := shell.Run(ctx, runID, mod.m.Test, renderedTest, allVars)
+		switch {
+		case err != nil:
 			// if test script for a module fails, log a warning for user and continue with apply
 			metricManagerModuleRunFailedTotal.With(labels).Inc()
-			logger.WarnContext(ctx, "Failed module test, running apply to get system to desired state")
-		} else {
+			logger.LogAttrs(
+				ctx,
+				slog.LevelWarn,
+				"Failed to run module test",
+				slog.String("err", err.Error()),
+			)
+		case rc != 0:
+			// if test script for a module fails, log a warning for user and continue with apply
+			metricManagerModuleRunFailedTotal.With(labels).Inc()
+			logger.LogAttrs(
+				ctx,
+				slog.LevelWarn,
+				"Failed to run module test, received non-zero exit code",
+				slog.Any("exit_code", rc),
+			)
+		default:
 			metricManagerModuleRunTotal.With(labels).Inc()
 			metricManagerModuleRunSuccessTimestamp.With(labels).Set(float64(testStart.Unix()))
 		}
@@ -164,7 +180,7 @@ func (mgr *Manager) RunModule(ctx context.Context, logger *slog.Logger, mod Modu
 		return fmt.Errorf("Failed to template script: %s", err)
 	}
 
-	err = shell.Run(ctx, runID, mod.m.Apply, renderedApply, allVars)
+	rc, err := shell.Run(ctx, runID, mod.m.Apply, renderedApply, allVars)
 
 	// update metrics regardless of error, so do them before handling error
 	applyEnd := time.Since(applyStart)
@@ -174,7 +190,12 @@ func (mgr *Manager) RunModule(ctx context.Context, logger *slog.Logger, mod Modu
 
 	if err != nil {
 		metricManagerModuleRunFailedTotal.With(labels).Inc()
-		return fmt.Errorf("Failed to apply module: %v", err)
+		return fmt.Errorf("Failed to run module apply: %v", err)
+	}
+
+	if rc != 0 {
+		metricManagerModuleRunFailedTotal.With(labels).Inc()
+		return fmt.Errorf("Failed to run module apply, non-zero exit code returned: %d", rc)
 	}
 
 	return nil

diff --git a/internal/shell/shell.go b/internal/shell/shell.go
@@ -204,9 +204,10 @@ func MergeVariables(maps ...VariableMap) VariableSlice {
 //   - string containing the contents of the templated script
 //   - a slice of strings in `key=value` pair containing the merged variables to
 //     be provided to the script as environment variables
-func Run(ctx context.Context, runID ulid.ULID, path, content string, allVars []string) error {
+//
+func Run(ctx context.Context, runID ulid.ULID, path, content string, allVars []string) (uint8, error) {
 	if content == "" {
-		return fmt.Errorf("No script data provided")
+		return 1, fmt.Errorf("No script data provided")
 	}
 
 	// setup log files for script output
@@ -216,39 +217,39 @@ func Run(ctx context.Context, runID ulid.ULID, path, content string, allVars []s
 	//	/var/log/mango/manager/run/01GZF2QSPGTCKHFSECPBQ6H8FQ/test/mockup/inventory/modules/test-env-vars/apply/stdout
 	logDir := filepath.Join(viper.GetString("mango.log-dir"), "manager/run", runID.String(), path)
 	if err := os.MkdirAll(logDir, 0750); err != nil && !os.IsExist(err) {
-		return fmt.Errorf("Failed to create directory for script logs: %v", err)
+		return 1, fmt.Errorf("Failed to create directory for script logs: %v", err)
 	}
 
 	// log stdout from script
 	stdoutLog, err := os.OpenFile(filepath.Join(logDir, "stdout"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
 	if err != nil {
-		return fmt.Errorf("Failed to open script log for stdout: %v", err)
+		return 1, fmt.Errorf("Failed to open script log for stdout: %v", err)
 	}
 	defer stdoutLog.Close()
 
 	// log stderr from script
 	stderrLog, err := os.OpenFile(filepath.Join(logDir, "stderr"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
 	if err != nil {
-		return fmt.Errorf("Failed to open script log for stderr: %v", err)
+		return 1, fmt.Errorf("Failed to open script log for stderr: %v", err)
 	}
 	defer stderrLog.Close()
 
 	// log exit status from script
 	exitStatusLog, err := os.OpenFile(filepath.Join(logDir, "exit_status"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
 	if err != nil {
-		return fmt.Errorf("Failed to open script log for exit status: %v", err)
+		return 1, fmt.Errorf("Failed to open script log for exit status: %v", err)
 	}
 	defer exitStatusLog.Close()
 
 	// log script content itself for testing template rendering
 	if err := os.WriteFile(filepath.Join(logDir, "script.mango-rendered"), []byte(content), 0644); err != nil {
-		return fmt.Errorf("Failed to write rendered script to log file: %v", err)
+		return 1, fmt.Errorf("Failed to write rendered script to log file: %v", err)
 	}
 
 	// runtime dir prep
 	workDir := filepath.Join(viper.GetString("mango.temp-dir"), runID.String())
 	if err := os.MkdirAll(workDir, 0750); err != nil && !os.IsExist(err) {
-		return fmt.Errorf("Failed to create working directory for script: %v", err)
+		return 1, fmt.Errorf("Failed to create working directory for script: %v", err)
 	}
 
 	// create shell interpreter
@@ -258,13 +259,13 @@ func Run(ctx context.Context, runID ulid.ULID, path, content string, allVars []s
 		interp.Dir(workDir),
 	)
 	if err != nil {
-		return fmt.Errorf("Failed to create shell interpreter: %s", err)
+		return 1, fmt.Errorf("Failed to create shell interpreter: %s", err)
 	}
 
 	// create shell parser based on rendered template script
 	file, err := syntax.NewParser().Parse(strings.NewReader(content), path)
 	if err != nil {
-		return fmt.Errorf("Failed to parse: %v", err)
+		return 1, fmt.Errorf("Failed to parse: %v", err)
 	}
 
 	// run it!
@@ -274,15 +275,15 @@ func Run(ctx context.Context, runID ulid.ULID, path, content string, allVars []s
 		status, ok := interp.IsExitStatus(err)
 		if !ok {
 			// Not an exit code, something else went wrong
-			return fmt.Errorf("Failed to run script %s: %v", path, err)
+			return 1, fmt.Errorf("Failed to run script %s: %v", path, err)
 		}
 
 		exitStatus = status
 	}
 
 	if _, err := exitStatusLog.WriteString(fmt.Sprintf("%d\n", exitStatus)); err != nil {
-		return fmt.Errorf("Failed to write exit status log for status code '%d': %v", exitStatus, err)
+		return 1, fmt.Errorf("Failed to write exit status log for status code '%d': %v", exitStatus, err)
 	}
 
-	return nil
+	return exitStatus, nil
 }