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) {