Skip to content

fix: make the agent panic-safe on unexpected types, and gate it in CI - #332

Merged
mayankpande88 merged 1 commit into
mainfrom
fix/panic-safety
Sep 10, 2026
Merged

fix: make the agent panic-safe on unexpected types, and gate it in CI#332
mayankpande88 merged 1 commit into
mainfrom
fix/panic-safety

Conversation

@mayankpande88

Copy link
Copy Markdown
Contributor

Why

#329 fixed the nine informer DeleteFunc handlers that panicked on a cache.DeletedFinalStateUnknown tombstone. That fixed the instances, not the class — 25 unchecked type assertions remained, and nothing stopped another being introduced.

An unchecked assertion isn't an ordinary bug here. Informer handlers run on client-go's shared goroutine, where a panic reaches apimachinery's runtime handler and terminates the process. The customer that reported this crashlooped 14 times in 12 hours off a single one.

What was left, and why each matters

Sites Where Risk
20 informer AddFunc/UpdateFunc Same fatal goroutine. Add/update never carry a tombstone — but the informers register transform functions (stripPod, stripNode, stripService) that return the object unchanged when their own assertion fails, so an unexpected type can still reach a handler.
3 sync.Map reads in ip_resolver.go The same file already guards this pattern in four other places with a "type confusion" log. These were simply inconsistent.
2 cilium.go CtEntry is decoded from bpffs. A Cilium version whose struct layout differs from the one we build against is precisely the case that yields an unexpected type — and it should degrade to unresolved, not kill the agent.
3 tracer.go, pinger.go, container.go Unreachable in practice, but on metrics and connection paths where dying is never the right answer.

Every site now skips the event and continues. For an observability agent a degraded resolver beats a crashlooping DaemonSet, and the next informer resync repairs the gap.

The lint gate is the actual fix

go vet has no unchecked-type-assertion check, and this repo had no linter at all — so nothing would have caught the original bug, and nothing would catch the next one.

forcetypeassert catches exactly this class. Verified by reintroducing the original bug on top of this branch:

common/ip_resolver.go:707:4: type assertion must be checked (forcetypeassert)
1 issues: * forcetypeassert: 1

On the fixed tree: 0 issues.

Config is deliberately narrow — one linter, tests excluded. A linter that fails on hundreds of pre-existing findings gets disabled rather than fixed, and this needs to survive.

Testing

gofmt          clean
go build ./... ok
go vet ./...   clean
go test        ok — cgroup, common, ebpftracer, ebpftracer/l7, logs, node, proc, tracing
golangci-lint  0 issues  (and 1 issue when the bug is reintroduced)

Note for reviewers

golangci-lint must be built with Go >= the go directive in go.mod, or it refuses to load the config:

can't load config: the Go language version (go1.24) used to build golangci-lint
is lower than the targeted Go version (1.25.0)

Hence v2.13.2, not the older v2.1.6 I first tried. CI already installs libsystemd-dev before this step, which the linter's typecheck needs.

Not covered

This makes type assertions safe. It does not make the agent panic-proof generally — nil-map writes, slice bounds, and nil derefs remain possible and are not lintable this way. A broader option is setting utilruntime.ReallyCrash = false so informer handler panics are logged instead of fatal; that's a deliberate policy call with its own downside (masking bugs, inconsistent state), so it's left out here rather than slipped in.

#329 fixed the nine informer DeleteFunc handlers that panicked on a
cache.DeletedFinalStateUnknown tombstone. It fixed the instances, not the
class: 25 unchecked type assertions remained, and nothing stopped another
being added.

An unchecked assertion is not an ordinary bug here. Informer handlers run
on client-go's shared goroutine, where a panic reaches apimachinery's
runtime handler and terminates the process — the customer that reported
this crashlooped 14 times in 12 hours off a single one.

Remaining sites, by why they matter:

  informer AddFunc/UpdateFunc (20) — same fatal goroutine. Add and update
  never carry a tombstone, but the informers register transform functions
  (stripPod, stripNode, stripService) that return the object unchanged when
  their own assertion fails, so an unexpected type can still reach a
  handler.

  sync.Map reads in ip_resolver (3) — the same file already guards this
  pattern in four other places with a "type confusion" log. These were
  simply inconsistent.

  cilium.go (2) — CtEntry is decoded from bpffs. A Cilium version whose
  struct layout differs from the one we build against is exactly the case
  that yields an unexpected type, and it should degrade to unresolved.

  tracer.go, pinger.go, container.go (3) — unreachable in practice, but on
  metrics and connection paths where dying is never the right answer.

Every site now skips the event and continues. For an observability agent a
degraded resolver beats a crashlooping DaemonSet, and the next informer
resync repairs the gap.

The lint gate is the part that closes the class. go vet has no
unchecked-type-assertion check and the repo had no linter at all;
forcetypeassert catches exactly this. Verified by reintroducing the
original bug, which fails with:

  common/ip_resolver.go:707:4: type assertion must be checked (forcetypeassert)

Config is deliberately narrow — one linter, tests excluded. A linter that
fails on hundreds of pre-existing findings gets disabled rather than fixed.

golangci-lint must be built with Go >= the go.mod directive or it refuses
to load the config, so v2.13.2 rather than the older v2.1.6.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request configures the forcetypeassert linter and refactors several type assertions across the codebase to use safe type assertions, preventing potential process-killing panics on unexpected types. In common/ip_resolver.go, the reviewer noted that returning an empty Workload{} early upon type confusion in ResolvePodOwner bypasses the API server fallback; they suggest refactoring the logic to allow the function to fall through to the fallback mechanism instead.

Comment thread common/ip_resolver.go
@mayankpande88
mayankpande88 merged commit 23091b0 into main Sep 10, 2026
7 checks passed
@mayankpande88
mayankpande88 deleted the fix/panic-safety branch September 10, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants