From 366ed3a5c89d399276bc12fa7eab83822597c350 Mon Sep 17 00:00:00 2001 From: mayankpande88 Date: Tue, 28 Jul 2026 17:27:18 +0530 Subject: [PATCH] fix(metrics): bound the owner-chain climb against circular ownerReferences OwnerReferences are client-settable, so a cycle is reachable via a buggy controller or a hand-edited ref. The climb had no termination condition other than reaching the top of the chain, so a cycle would spin forever and wedge the resolver goroutine, stalling every metric it feeds. This was true of the previous implementation too. Bound the walk at maxOwnerChainDepth (10). Genuine chains are at most two hops (Pod -> ReplicaSet -> Deployment, Pod -> Job -> CronJob), so the headroom covers nested custom controllers while still terminating. Exhausting the bound is treated as terminal rather than transient: a cycle cannot be fixed by retrying, and leaving the descriptor uncached would re-walk ten levels on every scrape. The identity resolved so far is kept and a warning is logged. Adds a regression test that runs the resolve on a goroutine and fails on a 5s timeout, so removing the bound reintroduces a detectable hang rather than an invisible one. --- common/ip_resolver.go | 22 +++++++++- common/ip_resolver_workload_identity_test.go | 42 ++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/common/ip_resolver.go b/common/ip_resolver.go index 19701ef..d078134 100644 --- a/common/ip_resolver.go +++ b/common/ip_resolver.go @@ -807,6 +807,12 @@ func getControllerOwnerRef(refs []metav1.OwnerReference) *metav1.OwnerReference return nil } +// maxOwnerChainDepth bounds the Pod -> ... -> top-level-controller climb. +// Genuine Kubernetes chains are at most two hops (Pod -> ReplicaSet -> +// Deployment, Pod -> Job -> CronJob); the headroom covers nested custom +// controllers while still terminating on a circular ownerReference. +const maxOwnerChainDepth = 10 + // errOwnerNotCached means the owner object exists in the cluster but is not in // our informer snapshot yet (informers still syncing), or was garbage-collected // while its pods linger. This is TRANSIENT: the same lookup may succeed later, @@ -929,7 +935,21 @@ func (resolver *K8sIPResolver) resolvePodDescriptor(pod *MinimalPod) Workload { // statement, which shadowed the outer error and left it permanently // nil, so a failed climb was still treated as a success and cached. current := ownerRef.DeepCopy() - for { + // The climb is depth-bounded. OwnerReferences are client-settable, so + // a circular chain (buggy controller, hand-edited ownerRef) would + // otherwise loop forever and wedge the collector goroutine, stalling + // every metric this resolver feeds. Real chains are at most two hops + // (Pod -> ReplicaSet -> Deployment, Pod -> Job -> CronJob). + for depth := 0; ; depth++ { + if depth >= maxOwnerChainDepth { + // Exhausting the bound means the chain is malformed. Treat it + // as terminal rather than transient: re-walking it on every + // scrape would burn CPU forever, and retrying cannot fix a + // cycle. Keep the identity resolved so far. + klog.Warningf("owner chain for pod %s/%s exceeded max depth %d at %s/%s — possible circular ownerReferences", + namespace, pod.Name, maxOwnerChainDepth, kind, name) + break + } next, err := resolver.getControllerOfOwner(current) if err != nil { if errors.Is(err, errOwnerNotCached) { diff --git a/common/ip_resolver_workload_identity_test.go b/common/ip_resolver_workload_identity_test.go index 09c618c..0a4de7c 100644 --- a/common/ip_resolver_workload_identity_test.go +++ b/common/ip_resolver_workload_identity_test.go @@ -2,6 +2,7 @@ package common import ( "testing" + "time" "github.com/coroot/coroot-node-agent/flags" "github.com/google/uuid" @@ -175,6 +176,47 @@ func TestResolvePodDescriptor_UnsupportedOwnerKindIsCached(t *testing.T) { assert.True(t, cached, "terminal (unsupported kind) resolution should be memoized") } +// OwnerReferences are client-settable, so a circular chain is possible via a +// buggy controller or a hand-edited ref. Without a depth bound the climb loops +// forever and wedges the collector goroutine. This test deadlocks on timeout if +// the bound is ever removed. +func TestResolvePodDescriptor_CircularOwnerChainTerminates(t *testing.T) { + disableEphemeralAggregation(t) + + aUID := types.UID(uuid.NewString()) + bUID := types.UID(uuid.NewString()) + podUID := types.UID(uuid.NewString()) + + r := &K8sIPResolver{} + // A is owned by B, B is owned by A. + r.snapshot.ReplicaSets.Store(aUID, MinimalOwnerInfo{ + OwnerReferences: []metav1.OwnerReference{controllerRef("b", bUID, "Deployment")}, + }) + r.snapshot.Deployments.Store(bUID, MinimalOwnerInfo{ + OwnerReferences: []metav1.OwnerReference{controllerRef("a", aUID, "ReplicaSet")}, + }) + + pod := &MinimalPod{ + UID: podUID, + Name: "cyclic-pod", + Namespace: "nudgebee", + OwnerReferences: []metav1.OwnerReference{controllerRef("a", aUID, "ReplicaSet")}, + } + + done := make(chan Workload, 1) + go func() { done <- r.resolvePodDescriptor(pod) }() + + select { + case got := <-done: + // Terminates with whichever identity the bounded climb reached. + assert.Contains(t, []string{"a", "b"}, got.Name) + _, cached := r.snapshot.PodDescriptors.Load(podUID) + assert.True(t, cached, "a malformed chain is terminal and should be cached, not re-walked every scrape") + case <-time.After(5 * time.Second): + t.Fatal("resolvePodDescriptor did not terminate on a circular owner chain") + } +} + // A bare ReplicaSet with no pod-template-hash must keep its own name; inventing // a Deployment for it would be wrong. func TestResolvePodDescriptor_BareReplicaSetKeepsItsName(t *testing.T) {