From 70f062c9b4971e60aaa474d472da7d2fca75d579 Mon Sep 17 00:00:00 2001 From: mayankpande88 Date: Thu, 10 Sep 2026 20:00:02 +0530 Subject: [PATCH] fix: make the agent panic-safe on unexpected types, and gate it in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. --- .github/workflows/ci.yml | 10 +++ .golangci.yml | 31 +++++++++ common/ip_resolver.go | 140 ++++++++++++++++++++++++++++++++------- containers/cilium.go | 16 ++++- containers/container.go | 14 +++- ebpftracer/tracer.go | 8 ++- pinger/pinger.go | 6 +- 7 files changed, 197 insertions(+), 28 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index acd1cb2f..8ed9f124 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,6 +29,16 @@ jobs: go install golang.org/x/tools/cmd/goimports@latest files=$(goimports -l .); if [[ -n "$files" ]]; then echo "$files"; exit 1; fi - run: go vet ./... + # go vet has no unchecked-type-assertion check. See .golangci.yml: an + # unchecked assertion in an informer handler is fatal, not recoverable. + # Must be built with Go >= the go directive in go.mod, otherwise it + # refuses to load the config ("the Go language version used to build + # golangci-lint is lower than the targeted Go version"). + - name: golangci-lint (forcetypeassert) + uses: golangci/golangci-lint-action@v8 + with: + version: v2.13.2 + args: --timeout=8m # /containers transitively imports github.com/NVIDIA/go-nvml. The # bindings register NVML symbols (including some only present in # very recent libnvidia-ml.so versions, e.g. diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..c2d1e2ac --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,31 @@ +version: "2" + +# Deliberately narrow. This config exists to gate one class of defect, not to +# impose a style regime on an existing codebase — a linter that fails on +# hundreds of pre-existing findings gets disabled instead of fixed. +# +# forcetypeassert: an unchecked type assertion panics on an unexpected type. +# In this agent that is not a recoverable error: informer handlers run on +# client-go's shared goroutine, where a panic reaches apimachinery's runtime +# handler and terminates the process. A customer cluster crashlooped node-agent +# 14 times in 12 hours on exactly one such assertion — a cache.DeletedFinalState +# Unknown tombstone asserted straight to *v1.Pod. go vet does not catch this. +linters: + default: none + enable: + - forcetypeassert + + exclusions: + generated: lax + rules: + # Test code asserting on values it just constructed is not a production + # crash risk, and failing a test is already a visible outcome. + - path: _test\.go$ + linters: + - forcetypeassert + +issues: + # Every finding is a potential process-killing panic; do not collapse + # repeats of the same one. + max-issues-per-linter: 0 + max-same-issues: 0 diff --git a/common/ip_resolver.go b/common/ip_resolver.go index 60f1db6c..ee71bf07 100644 --- a/common/ip_resolver.go +++ b/common/ip_resolver.go @@ -376,13 +376,19 @@ func (resolver *K8sIPResolver) StartWatching() error { func (resolve *K8sIPResolver) addReplicaSetHandlers(replicaSetInformer cache.SharedIndexInformer) { replicaSetInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - rs := obj.(*appsv1.ReplicaSet) + rs, ok := objectAs[*appsv1.ReplicaSet](obj) + if !ok { + return + } resolve.snapshot.ReplicaSets.Store(rs.UID, MinimalOwnerInfo{ OwnerReferences: rs.OwnerReferences, }) }, UpdateFunc: func(oldObj, newObj interface{}) { - rs := newObj.(*appsv1.ReplicaSet) + rs, ok := objectAs[*appsv1.ReplicaSet](newObj) + if !ok { + return + } resolve.snapshot.ReplicaSets.Store(rs.UID, MinimalOwnerInfo{ OwnerReferences: rs.OwnerReferences, }) @@ -427,16 +433,43 @@ func deletedObject[T any](obj interface{}) (T, bool) { return zero, false } +// objectAs asserts an add/update informer payload to T without panicking. +// +// Add and update never carry a tombstone, so unlike deletedObject there is +// nothing to unwrap — but the assertion is still on the shared informer's +// goroutine, where a panic reaches apimachinery's runtime handler and kills the +// process. The informers here also register transform functions (stripPod, +// stripNode, stripService) that return the object unchanged when their own +// assertion fails, so an unexpected type can reach a handler rather than being +// filtered out. +// +// Skipping the event is the right failure mode: the resolver loses one object +// until the next resync, instead of taking the agent down. +func objectAs[T any](obj interface{}) (T, bool) { + if o, ok := obj.(T); ok { + return o, true + } + var zero T + klog.V(2).Infof("ignoring event with unexpected payload %T", obj) + return zero, false +} + func (resolve *K8sIPResolver) addDaemonSetHandlers(daemonSetInformer cache.SharedIndexInformer) { daemonSetInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - ds := obj.(*appsv1.DaemonSet) + ds, ok := objectAs[*appsv1.DaemonSet](obj) + if !ok { + return + } resolve.snapshot.DaemonSets.Store(ds.UID, MinimalOwnerInfo{ OwnerReferences: ds.OwnerReferences, }) }, UpdateFunc: func(oldObj, newObj interface{}) { - ds := newObj.(*appsv1.DaemonSet) + ds, ok := objectAs[*appsv1.DaemonSet](newObj) + if !ok { + return + } resolve.snapshot.DaemonSets.Store(ds.UID, MinimalOwnerInfo{ OwnerReferences: ds.OwnerReferences, }) @@ -454,13 +487,19 @@ func (resolve *K8sIPResolver) addDaemonSetHandlers(daemonSetInformer cache.Share func (resolve *K8sIPResolver) addStatefulSetHandlers(statefulSetInformer cache.SharedIndexInformer) { statefulSetInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - ss := obj.(*appsv1.StatefulSet) + ss, ok := objectAs[*appsv1.StatefulSet](obj) + if !ok { + return + } resolve.snapshot.StatefulSets.Store(ss.UID, MinimalOwnerInfo{ OwnerReferences: ss.OwnerReferences, }) }, UpdateFunc: func(oldObj, newObj interface{}) { - ss := newObj.(*appsv1.StatefulSet) + ss, ok := objectAs[*appsv1.StatefulSet](newObj) + if !ok { + return + } resolve.snapshot.StatefulSets.Store(ss.UID, MinimalOwnerInfo{ OwnerReferences: ss.OwnerReferences, }) @@ -478,13 +517,19 @@ func (resolve *K8sIPResolver) addStatefulSetHandlers(statefulSetInformer cache.S func (resolve *K8sIPResolver) addJobHandlers(jobInformer cache.SharedIndexInformer) { jobInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - job := obj.(*batchv1.Job) + job, ok := objectAs[*batchv1.Job](obj) + if !ok { + return + } resolve.snapshot.Jobs.Store(job.UID, MinimalOwnerInfo{ OwnerReferences: job.OwnerReferences, }) }, UpdateFunc: func(oldObj, newObj interface{}) { - job := newObj.(*batchv1.Job) + job, ok := objectAs[*batchv1.Job](newObj) + if !ok { + return + } resolve.snapshot.Jobs.Store(job.UID, MinimalOwnerInfo{ OwnerReferences: job.OwnerReferences, }) @@ -502,13 +547,19 @@ func (resolve *K8sIPResolver) addJobHandlers(jobInformer cache.SharedIndexInform func (resolve *K8sIPResolver) addCronJobHandlers(cronJobInformer cache.SharedIndexInformer) { cronJobInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - cronJob := obj.(*batchv1.CronJob) + cronJob, ok := objectAs[*batchv1.CronJob](obj) + if !ok { + return + } resolve.snapshot.CronJobs.Store(cronJob.UID, MinimalOwnerInfo{ OwnerReferences: cronJob.OwnerReferences, }) }, UpdateFunc: func(oldObj, newObj interface{}) { - cronJob := newObj.(*batchv1.CronJob) + cronJob, ok := objectAs[*batchv1.CronJob](newObj) + if !ok { + return + } resolve.snapshot.CronJobs.Store(cronJob.UID, MinimalOwnerInfo{ OwnerReferences: cronJob.OwnerReferences, }) @@ -526,7 +577,10 @@ func (resolve *K8sIPResolver) addCronJobHandlers(cronJobInformer cache.SharedInd func (resolve *K8sIPResolver) addServiceHandlers(serviceInformer cache.SharedIndexInformer) { serviceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - service := obj.(*v1.Service) + service, ok := objectAs[*v1.Service](obj) + if !ok { + return + } minSvc := MinimalService{ Name: service.Name, Namespace: service.Namespace, @@ -542,8 +596,14 @@ func (resolve *K8sIPResolver) addServiceHandlers(serviceInformer cache.SharedInd } }, UpdateFunc: func(oldObj, newObj interface{}) { - oldService := oldObj.(*v1.Service) - service := newObj.(*v1.Service) + oldService, ok := objectAs[*v1.Service](oldObj) + if !ok { + return + } + service, ok := objectAs[*v1.Service](newObj) + if !ok { + return + } minSvc := MinimalService{ Name: service.Name, Namespace: service.Namespace, @@ -586,13 +646,19 @@ func (resolve *K8sIPResolver) addServiceHandlers(serviceInformer cache.SharedInd func (resolve *K8sIPResolver) addDeploymentHandlers(deploymentInformer cache.SharedIndexInformer) { deploymentInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - deployment := obj.(*appsv1.Deployment) + deployment, ok := objectAs[*appsv1.Deployment](obj) + if !ok { + return + } resolve.snapshot.Deployments.Store(deployment.UID, MinimalOwnerInfo{ OwnerReferences: deployment.OwnerReferences, }) }, UpdateFunc: func(oldObj, newObj interface{}) { - deployment := newObj.(*appsv1.Deployment) + deployment, ok := objectAs[*appsv1.Deployment](newObj) + if !ok { + return + } resolve.snapshot.Deployments.Store(deployment.UID, MinimalOwnerInfo{ OwnerReferences: deployment.OwnerReferences, }) @@ -610,12 +676,21 @@ func (resolve *K8sIPResolver) addDeploymentHandlers(deploymentInformer cache.Sha func (resolver *K8sIPResolver) addPodHandlers(podInformer cache.SharedIndexInformer) { podInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - pod := obj.(*v1.Pod) + pod, ok := objectAs[*v1.Pod](obj) + if !ok { + return + } resolver.handlePodAdd(pod) }, UpdateFunc: func(oldObj, newObj interface{}) { - oldPod := oldObj.(*v1.Pod) - newPod := newObj.(*v1.Pod) + oldPod, ok := objectAs[*v1.Pod](oldObj) + if !ok { + return + } + newPod, ok := objectAs[*v1.Pod](newObj) + if !ok { + return + } // Clean old IPs that are no longer present newIPs := make(map[string]bool, len(newPod.Status.PodIPs)) for _, ip := range newPod.Status.PodIPs { @@ -686,14 +761,20 @@ func (resolver *K8sIPResolver) handlePodAdd(pod *v1.Pod) bool { func (resolver *K8sIPResolver) addNodeHandlers(nodeInformer cache.SharedIndexInformer) { nodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - node := obj.(*v1.Node) + node, ok := objectAs[*v1.Node](obj) + if !ok { + return + } shouldReturn := resolver.handleNodeEvent(node) if shouldReturn { return } }, UpdateFunc: func(oldObj, newObj interface{}) { - node := newObj.(*v1.Node) + node, ok := objectAs[*v1.Node](newObj) + if !ok { + return + } shouldReturn := resolver.handleNodeEvent(node) if shouldReturn { return @@ -935,7 +1016,11 @@ func (resolver *K8sIPResolver) getControllerOfOwner(owner *metav1.OwnerReference if !ok { return nil, fmt.Errorf("%w: %s %s", errOwnerNotCached, owner.Kind, owner.UID) } - info := val.(MinimalOwnerInfo) + info, ok := val.(MinimalOwnerInfo) + if !ok { + klog.V(5).Infof("type confusion in owner cache for %s %s", owner.Kind, owner.UID) + return nil, fmt.Errorf("%w: %s %s", errOwnerNotCached, owner.Kind, owner.UID) + } return getControllerOwnerRef(info.OwnerReferences), nil } @@ -1113,8 +1198,17 @@ func (resolver *K8sIPResolver) resolvePodDescriptor(pod *MinimalPod) Workload { func (resolver *K8sIPResolver) ResolvePodOwner(podName string, podNamespace string) Workload { if uidVal, ok := resolver.snapshot.PodNameIndex.Load(podNamespace + "/" + podName); ok { - if podVal, ok := resolver.snapshot.Pods.Load(uidVal.(types.UID)); ok { - pod := podVal.(MinimalPod) + uid, ok := uidVal.(types.UID) + if !ok { + klog.V(5).Infof("type confusion in PodNameIndex for %s/%s", podNamespace, podName) + return Workload{} + } + if podVal, ok := resolver.snapshot.Pods.Load(uid); ok { + pod, ok := podVal.(MinimalPod) + if !ok { + klog.V(5).Infof("type confusion in Pods cache for %s/%s", podNamespace, podName) + return Workload{} + } return resolver.resolvePodDescriptor(&pod) } } diff --git a/containers/cilium.go b/containers/cilium.go index 2494ff9a..6fafb200 100644 --- a/containers/cilium.go +++ b/containers/cilium.go @@ -110,7 +110,13 @@ func lookupCilium4(src, dst netaddr.IPPort) *netaddr.IPPort { if err != nil || v == nil { return nil } - e := v.(*ctmap.CtEntry) + // Cilium BPF structs are decoded from bpffs, so a Cilium version whose + // layout differs from the one we build against can yield an unexpected + // type here. Degrade to "unresolved" rather than panicking the agent. + e, ok := v.(*ctmap.CtEntry) + if !ok { + return nil + } backendKey := lbmap.NewBackend4KeyV3(loadbalancer.BackendID(e.BackendID)) b, err := backends4Map.Lookup(backendKey) @@ -151,7 +157,13 @@ func lookupCilium6(src, dst netaddr.IPPort) *netaddr.IPPort { if err != nil || v == nil { return nil } - e := v.(*ctmap.CtEntry) + // Cilium BPF structs are decoded from bpffs, so a Cilium version whose + // layout differs from the one we build against can yield an unexpected + // type here. Degrade to "unresolved" rather than panicking the agent. + e, ok := v.(*ctmap.CtEntry) + if !ok { + return nil + } backendKey := lbmap.NewBackend6KeyV3(loadbalancer.BackendID(e.BackendID)) b, err := backends6Map.Lookup(backendKey) if err != nil || b == nil { diff --git a/containers/container.go b/containers/container.go index 994a95a3..7bb2ce62 100644 --- a/containers/container.go +++ b/containers/container.go @@ -416,7 +416,7 @@ func (c *Container) Collect(ch chan<- prometheus.Metric) { for _, ctr := range p.parser.GetCounters() { if ctr.Level == logparser.LevelCritical || ctr.Level == logparser.LevelError { sample, _ := c.logSamples.LoadOrStore(ctr.Hash, common.TruncateUtf8(ctr.Sample, *flags.MaxLabelLength)) - ch <- c.counter(metrics.LogMessages, float64(ctr.Messages), source, ctr.Level.String(), ctr.Hash, sample.(string)) + ch <- c.counter(metrics.LogMessages, float64(ctr.Messages), source, ctr.Level.String(), ctr.Hash, sampleString(sample)) } } for _, sc := range p.parser.GetSensitiveCounters() { @@ -2306,3 +2306,15 @@ func (c *Container) gauge(desc *prometheus.Desc, value float64, labelValues ...s allLabels = append(allLabels, labelValues...) return prometheus.MustNewConstMetric(desc, prometheus.GaugeValue, value, allLabels...) } + +// sampleValue renders a log sample stored in c.logSamples as a metric label. +// +// The map only ever holds strings, but this is a metric-collection path that +// runs on every scrape: a type confusion here should degrade the label, not +// take the agent down. +func sampleString(v interface{}) string { + if s, ok := v.(string); ok { + return s + } + return "" +} diff --git a/ebpftracer/tracer.go b/ebpftracer/tracer.go index 71370c7b..9e03d4ea 100644 --- a/ebpftracer/tracer.go +++ b/ebpftracer/tracer.go @@ -448,7 +448,13 @@ func getLostSamplesTracker(name string) *lostSamplesTracker { if !ok { tracker, _ = lostSamplesTrackers.LoadOrStore(name, &lostSamplesTracker{interval: 10}) } - return tracker.(*lostSamplesTracker) + t, ok := tracker.(*lostSamplesTracker) + if !ok { + // Only this function ever writes the map, so this is unreachable; return a + // throwaway rather than panicking in a metrics path. + return &lostSamplesTracker{interval: 10} + } + return t } // safeDuration converts a uint64 nanosecond value from eBPF to time.Duration. diff --git a/pinger/pinger.go b/pinger/pinger.go index 16c9520e..d7d8a1e5 100644 --- a/pinger/pinger.go +++ b/pinger/pinger.go @@ -214,7 +214,11 @@ func openConn() (*net.IPConn, error) { if err != nil { return nil, err } - ipconn := conn.(*net.IPConn) + ipconn, ok := conn.(*net.IPConn) + if !ok { + conn.Close() + return nil, fmt.Errorf("unexpected connection type %T for ip4:icmp", conn) + } f, err := ipconn.File() if err != nil { return nil, err