From 4471a45591ef6884f9aed1e973b2b88e85294975 Mon Sep 17 00:00:00 2001
From: Nikolaus Schuetz <nikolauspschuetz@gmail.com>
Date: Sat, 1 Aug 2026 21:21:02 -0700
Subject: [PATCH] Surface invalid-regex errors to the call site with workload
 context

Per review: instead of logging the skipped pattern inside ShouldReload
(which lacks workload identity), collect the compile errors on
ReloadCheckResult.Errors and log them at the upgrade call site, where the
resource name, type, and namespace are known. Add a test for the
multi-value case where one comma-separated pattern is malformed and a
valid one still matches.
---
 internal/pkg/handler/upgrade.go |  4 ++++
 pkg/common/common.go            |  9 ++++++++-
 pkg/common/common_test.go       | 29 +++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/internal/pkg/handler/upgrade.go b/internal/pkg/handler/upgrade.go
index a4870403..5570ee2d 100644
--- a/internal/pkg/handler/upgrade.go
+++ b/internal/pkg/handler/upgrade.go
@@ -310,6 +310,10 @@ func upgradeResource(clients kube.Clients, config common.Config, upgradeFuncs ca
 	podAnnotations := upgradeFuncs.PodAnnotationsFunc(resource)
 	result := common.ShouldReload(config, upgradeFuncs.ResourceType, annotations, podAnnotations, common.GetCommandLineOptions())
 
+	for _, reloadErr := range result.Errors {
+		logrus.Errorf("Skipping invalid reload annotation on %s '%s' in namespace '%s': %v", upgradeFuncs.ResourceType, resourceName, config.Namespace, reloadErr)
+	}
+
 	if !result.ShouldReload {
 		logrus.Debugf("No changes detected in '%s' of type '%s' in namespace '%s'", config.ResourceName, config.Type, config.Namespace)
 		return false, nil
diff --git a/pkg/common/common.go b/pkg/common/common.go
index 45de1f19..25017e3f 100644
--- a/pkg/common/common.go
+++ b/pkg/common/common.go
@@ -2,6 +2,7 @@ package common
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"regexp"
 	"strconv"
@@ -22,6 +23,7 @@ type Map map[string]string
 type ReloadCheckResult struct {
 	ShouldReload bool
 	AutoReload   bool
+	Errors       []error
 }
 
 // ReloaderOptions contains all configurable options for the Reloader controller.
@@ -273,18 +275,20 @@ func ShouldReload(config Config, resourceType string, annotations Map, podAnnota
 		}
 	}
 
+	var regexErrors []error
 	values := strings.Split(annotationValue, ",")
 	for _, value := range values {
 		value = strings.TrimSpace(value)
 		re, err := regexp.Compile("^" + value + "$")
 		if err != nil {
-			logrus.Errorf("Invalid regex %q in reload annotation %q on resource '%s' of type '%s'; skipping this pattern: %v", value, config.Annotation, config.ResourceName, config.Type, err)
+			regexErrors = append(regexErrors, fmt.Errorf("invalid regex %q in reload annotation %q: %w", value, config.Annotation, err))
 			continue
 		}
 		if re.Match([]byte(config.ResourceName)) {
 			return ReloadCheckResult{
 				ShouldReload: true,
 				AutoReload:   false,
+				Errors:       regexErrors,
 			}
 		}
 	}
@@ -295,6 +299,7 @@ func ShouldReload(config Config, resourceType string, annotations Map, podAnnota
 			return ReloadCheckResult{
 				ShouldReload: true,
 				AutoReload:   true,
+				Errors:       regexErrors,
 			}
 		}
 	}
@@ -305,11 +310,13 @@ func ShouldReload(config Config, resourceType string, annotations Map, podAnnota
 		return ReloadCheckResult{
 			ShouldReload: true,
 			AutoReload:   true,
+			Errors:       regexErrors,
 		}
 	}
 
 	return ReloadCheckResult{
 		ShouldReload: false,
+		Errors:       regexErrors,
 	}
 }
 
diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go
index 0bbdab54..ba251ce3 100644
--- a/pkg/common/common_test.go
+++ b/pkg/common/common_test.go
@@ -245,4 +245,33 @@ func TestShouldReload_InvalidRegexAnnotation_DoesNotPanic(t *testing.T) {
 	if result.ShouldReload {
 		t.Errorf("Expected ShouldReload=false for an invalid regex pattern, got=%v", result.ShouldReload)
 	}
+	if len(result.Errors) != 1 {
+		t.Errorf("Expected 1 surfaced regex error, got=%d: %v", len(result.Errors), result.Errors)
+	}
+}
+
+// When a named reload annotation holds several comma-separated patterns, a
+// single malformed one is skipped while a valid one still matches, and the
+// skipped pattern's error is surfaced on the result.
+func TestShouldReload_InvalidRegexAnnotation_SkipsMalformedPattern(t *testing.T) {
+	config := Config{
+		ResourceName: "app-config",
+		Annotation:   "secret.reloader.stakater.com/reload",
+	}
+	annotations := Map{
+		// first pattern is invalid (unbalanced bracket), second matches
+		"secret.reloader.stakater.com/reload": "bad[,app-config",
+	}
+	opts := &ReloaderOptions{
+		ReloaderAutoAnnotation: "reloader.stakater.com/auto",
+	}
+
+	result := ShouldReload(config, "Deployment", annotations, Map{}, opts)
+
+	if !result.ShouldReload {
+		t.Errorf("Expected ShouldReload=true from the valid pattern, got=%v", result.ShouldReload)
+	}
+	if len(result.Errors) != 1 {
+		t.Errorf("Expected the malformed pattern to surface 1 error, got=%d: %v", len(result.Errors), result.Errors)
+	}
 }