Skip to content

fix(k8s): handle DeletedFinalStateUnknown in informer delete handlers - #329

Merged
mayankpande88 merged 1 commit into
mainfrom
fix/informer-delete-tombstone-panic
Sep 10, 2026
Merged

fix(k8s): handle DeletedFinalStateUnknown in informer delete handlers#329
mayankpande88 merged 1 commit into
mainfrom
fix/informer-delete-tombstone-panic

Conversation

@mayankpande88

Copy link
Copy Markdown
Contributor

Problem

Every DeleteFunc in common/ip_resolver.go asserted the informer payload straight to its concrete type:

DeleteFunc: func(obj interface{}) {
    pod := obj.(*v1.Pod)     // panics on a tombstone
    ...
}

client-go does not always deliver the object on a delete. When a watch is interrupted and the final delete event is missed, the informer resyncs and delivers a cache.DeletedFinalStateUnknown tombstone wrapping the last known state. The assertion panics on it.

These handlers run on the shared informer's goroutine, so the panic reaches apimachinery's runtime handler — which is fatal. The process exits and the pod restarts:

panic: interface conversion: interface {} is cache.DeletedFinalStateUnknown, not *v1.Pod
  common.(*K8sIPResolver).addPodHandlers.func3
level=fatal msg="Panic in Kubernetes runtime handler" subsys=k8s

Reported from a customer cluster where this accounted for 14 of 15 node-agent restarts in 12 hours across two nodes (Terminated / Error / exit 1).

This is not load- or memory-related. One missed delete event is enough, so it fires on any cluster where the watch connection is disrupted — more likely on clusters with flaky control-plane connectivity or high object churn.

Scope

All nine delete handlers had the same defect, not just the reported pod one:

ReplicaSet · DaemonSet · StatefulSet · Job · CronJob · Service · Deployment · Pod · Node

Fixing only the reported handler would leave eight identical landmines, each capable of the same fatal panic.

Fix

func deletedObject[T any](obj interface{}) (T, bool)

Unwraps the tombstone, and returns ok=false for anything it cannot interpret — wrong type, nil payload, unrelated value — so the delete is skipped rather than killing the agent.

Skipping is safe: these maps are keyed by UID and the object is already gone. A missed delete leaves a stale entry that the next resync reconciles, which is strictly better than terminating the process.

Testing

go build ./...        ok  (whole repo)
go vet ./common/      clean
--- PASS: TestDeletedObjectUnwrapsTombstone      (plain object, tombstone)
--- PASS: TestDeletedObjectRejectsBadPayloads    (nil, wrong type, tombstone-wrapping-wrong-type,
                                                  tombstone-wrapping-nil, unrelated value)
--- PASS: TestDeletedObjectOtherTypes            (Service, Node)

The bad-payload cases assert no panic specifically, since panicking is the failure mode being fixed.

./common is not in CI's test exclusion (only ./containers is, for NVML reasons), so go test $(go list ./... | grep -v '/containers$') will run these and gate regressions.

Note on the same incident

The customer also reported 1 OOM out of those 15 restarts. That is a separate matter and is not addressed here. Heap profile from the affected pod shows 120MB total with a healthy distribution — no runaway allocation — so the OOM looks like a distinct, much rarer event rather than the crash-loop cause.

@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 introduces a generic helper function deletedObject in common/ip_resolver.go to safely extract deleted Kubernetes objects from informer DeleteFunc payloads. This prevents application panics and crash loops when client-go delivers a cache.DeletedFinalStateUnknown tombstone instead of the actual object. Direct type assertions in the delete handlers for various Kubernetes resources (such as Pods, Services, and ReplicaSets) have been updated to use this helper. Additionally, comprehensive unit tests have been added in common/ip_resolver_delete_test.go to verify correct behavior under different scenarios. There are no review comments, and I have no additional feedback to provide.

Every DeleteFunc in the IP resolver asserted the informer payload directly
to its concrete type. client-go does not always deliver the object on a
delete: when a watch is interrupted and the final delete event is missed,
the informer resyncs and delivers a cache.DeletedFinalStateUnknown
tombstone wrapping the last known state. The assertion panics on that.

Because these handlers run on the shared informer's goroutine, the panic
reaches apimachinery's runtime handler, which is fatal — the process exits
and the pod restarts:

  panic: interface conversion: interface {} is cache.DeletedFinalStateUnknown, not *v1.Pod
  common.(*K8sIPResolver).addPodHandlers.func3
  level=fatal msg="Panic in Kubernetes runtime handler" subsys=k8s

Reported from a customer cluster where this accounted for 14 of 15
node-agent restarts in 12 hours across two nodes. It is not load or memory
related — a single missed delete event is enough, so it fires on any
cluster where the watch connection is disrupted.

All nine delete handlers had the same defect, not just pods: ReplicaSet,
DaemonSet, StatefulSet, Job, CronJob, Service, Deployment, Pod and Node.
Fixing only the reported one would leave eight identical landmines.

deletedObject[T] unwraps the tombstone and returns ok=false for anything
it cannot interpret — wrong type, nil payload, unrelated value — so a
delete is skipped rather than killing the agent. Skipping is safe: the
entry is keyed by UID and the object is already gone.

./common is not excluded from CI tests, so the new cases gate this.
@mayankpande88
mayankpande88 force-pushed the fix/informer-delete-tombstone-panic branch from c7ddd1d to 25bfb73 Compare September 10, 2026 13:19
@mayankpande88
mayankpande88 merged commit 25d3562 into main Sep 10, 2026
7 checks passed
@mayankpande88
mayankpande88 deleted the fix/informer-delete-tombstone-panic branch September 10, 2026 13:25
mayankpande88 added a commit that referenced this pull request Sep 10, 2026
…#332)

#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.
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