diff --git a/applicationset/webhook/webhook.go b/applicationset/webhook/webhook.go index 2bbd055adf..acb9fa85aa 100644 --- a/applicationset/webhook/webhook.go +++ b/applicationset/webhook/webhook.go @@ -26,10 +26,14 @@ import ( "github.com/go-playground/webhooks/v6/github" "github.com/go-playground/webhooks/v6/gitlab" log "github.com/sirupsen/logrus" + + "github.com/argoproj/argo-cd/v3/util/guard" ) const payloadQueueSize = 50000 +const panicMsgAppSet = "panic while processing applicationset-controller webhook event" + type WebhookHandler struct { sync.WaitGroup // for testing github *github.Webhook @@ -102,6 +106,7 @@ func NewWebhookHandler(webhookParallelism int, argocdSettingsMgr *argosettings.S } func (h *WebhookHandler) startWorkerPool(webhookParallelism int) { + compLog := log.WithField("component", "applicationset-webhook") for i := 0; i < webhookParallelism; i++ { h.Add(1) go func() { @@ -111,7 +116,7 @@ func (h *WebhookHandler) startWorkerPool(webhookParallelism int) { if !ok { return } - h.HandleEvent(payload) + guard.RecoverAndLog(func() { h.HandleEvent(payload) }, compLog, panicMsgAppSet) } }() } diff --git a/util/guard/guard.go b/util/guard/guard.go new file mode 100644 index 0000000000..59121b2660 --- /dev/null +++ b/util/guard/guard.go @@ -0,0 +1,20 @@ +package guard + +import ( + "runtime/debug" +) + +// Minimal logger contract; avoids depending on any specific logging package. +type Logger interface{ Errorf(string, ...any) } + +// Run executes fn and recovers a panic, logging a component-specific message. +func RecoverAndLog(fn func(), log Logger, msg string) { + defer func() { + if r := recover(); r != nil { + if log != nil { + log.Errorf("%s: %v %s", msg, r, debug.Stack()) + } + } + }() + fn() +} diff --git a/util/guard/guard_test.go b/util/guard/guard_test.go new file mode 100644 index 0000000000..19f5365775 --- /dev/null +++ b/util/guard/guard_test.go @@ -0,0 +1,92 @@ +package guard + +import ( + "fmt" + "strings" + "sync" + "testing" +) + +type nop struct{} + +func (nop) Errorf(string, ...any) {} + +// recorder is a thread-safe logger that captures formatted messages. +type recorder struct { + mu sync.Mutex + calls int + msgs []string +} + +func (r *recorder) Errorf(format string, args ...any) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls++ + r.msgs = append(r.msgs, fmt.Sprintf(format, args...)) +} + +func TestRun_Recovers(_ *testing.T) { + RecoverAndLog(func() { panic("boom") }, nop{}, "msg") // fails if panic escapes +} + +func TestRun_AllowsNextCall(t *testing.T) { + ran := false + RecoverAndLog(func() { panic("boom") }, nop{}, "msg") + RecoverAndLog(func() { ran = true }, nop{}, "msg") + if !ran { + t.Fatal("expected second callback to run") + } +} + +func TestRun_LogsMessageAndStack(t *testing.T) { + r := &recorder{} + RecoverAndLog(func() { panic("boom") }, r, "msg") + if r.calls != 1 { + t.Fatalf("expected 1 log call, got %d", r.calls) + } + got := strings.Join(r.msgs, "\n") + if !strings.Contains(got, "msg") { + t.Errorf("expected log to contain message %q; got %q", "msg", got) + } + if !strings.Contains(got, "boom") { + t.Errorf("expected log to contain panic value %q; got %q", "boom", got) + } + // Heuristic check that a stack trace was included. + if !strings.Contains(got, "guard.go") && !strings.Contains(got, "runtime/panic.go") && !strings.Contains(got, "goroutine") { + t.Errorf("expected log to contain a stack trace; got %q", got) + } +} + +func TestRun_NilLoggerDoesNotPanic(_ *testing.T) { + var l Logger // nil + RecoverAndLog(func() { panic("boom") }, l, "ignored") +} + +func TestRun_NoPanicDoesNotLog(t *testing.T) { + r := &recorder{} + ran := false + RecoverAndLog(func() { ran = true }, r, "msg") + if !ran { + t.Fatal("expected fn to run") + } + if r.calls != 0 { + t.Fatalf("expected 0 log calls, got %d", r.calls) + } +} + +func TestRun_ConcurrentPanicsLogged(t *testing.T) { + r := &recorder{} + const n = 10 + var wg sync.WaitGroup + wg.Add(n) + for i := 0; i < n; i++ { + go func(i int) { + defer wg.Done() + RecoverAndLog(func() { panic(fmt.Sprintf("boom-%d", i)) }, r, "msg") + }(i) + } + wg.Wait() + if r.calls != n { + t.Fatalf("expected %d log calls, got %d", n, r.calls) + } +} diff --git a/util/webhook/webhook.go b/util/webhook/webhook.go index 8e4ebc567b..1c4f4380fc 100644 --- a/util/webhook/webhook.go +++ b/util/webhook/webhook.go @@ -37,6 +37,7 @@ import ( "github.com/argoproj/argo-cd/v3/util/db" "github.com/argoproj/argo-cd/v3/util/git" "github.com/argoproj/argo-cd/v3/util/glob" + "github.com/argoproj/argo-cd/v3/util/guard" "github.com/argoproj/argo-cd/v3/util/settings" ) @@ -52,6 +53,8 @@ const usernameRegex = `[\w\.][\w\.-]{0,30}[\w\.\$-]?` const payloadQueueSize = 50000 +const panicMsgServer = "panic while processing api-server webhook event" + var _ settingsSource = &settings.SettingsManager{} type ArgoCDWebhookHandler struct { @@ -127,6 +130,7 @@ func NewHandler(namespace string, applicationNamespaces []string, webhookParalle } func (a *ArgoCDWebhookHandler) startWorkerPool(webhookParallelism int) { + compLog := log.WithField("component", "api-server-webhook") for i := 0; i < webhookParallelism; i++ { a.Add(1) go func() { @@ -136,7 +140,7 @@ func (a *ArgoCDWebhookHandler) startWorkerPool(webhookParallelism int) { if !ok { return } - a.HandleEvent(payload) + guard.RecoverAndLog(func() { a.HandleEvent(payload) }, compLog, panicMsgServer) } }() }