fix(autoinstrumentation): skip unresolvable namespace rules instead of aborting

Evaluate policies strictly in order (first match wins). When a pod's
namespace is absent from the workloadmeta store, a rule that reads
namespace labels cannot be evaluated; ignore only that rule and keep
evaluating the rest, so an unresolvable namespace rule never prevents an
otherwise-matching rule from injecting.

This replaces the legacy "abort all matching" fail-safe, which declined
injection entirely as soon as it reached a rule whose namespace could not
be resolved.

Author: iamluc

pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher.go

@@ -38,50 +38,55 @@ func newPolicyMatcher(ps []policies.Policy, wmeta workloadmeta.Component) *polic
 // Match returns the outcome of the first policy that matches the pod, mirroring
 // the "first match wins" semantics of the target mutator.
 func (m *policyMatcher) Match(pod *corev1.Pod) (policies.Outcome, bool) {
-	if m == nil || pod == nil {
-		return policies.Outcome{}, false
-	}
-	facts, err := m.factsForPod(pod)
-	if err != nil {
-		log.Debugf("policy matcher: could not build facts for pod in %q: %v", pod.Namespace, err)
+	idx := m.matchIndex(pod)
+	if idx < 0 {
 		return policies.Outcome{}, false
 	}
-	return policies.Decide(m.policies, facts)
+	return m.policies[idx].Outcome, true
 }
 
 // matchIndex returns the index of the first policy that matches the pod, or -1
-// if none match. It returns an error when a required fact (e.g. namespace
-// labels) could not be resolved, so the caller can abort matching rather than
-// risk an inaccurate decision.
-func (m *policyMatcher) matchIndex(pod *corev1.Pod) (int, error) {
+// if none match. Policies are evaluated in order (first match wins).
+//
+// A policy that reads namespace labels which could not be resolved (e.g. the
+// pod's namespace is absent from the workloadmeta store) is skipped rather than
+// fatal: it cannot be evaluated, so it is ignored, and the remaining policies
+// are still evaluated in order. This guarantees that an unrelated unresolvable
+// namespace rule never prevents an otherwise-matching rule from injecting.
+func (m *policyMatcher) matchIndex(pod *corev1.Pod) int {
 	if m == nil || pod == nil {
-		return -1, nil
-	}
-	facts, err := m.factsForPod(pod)
-	if err != nil {
-		return -1, err
+		return -1
 	}
+	facts, namespaceLabelsResolved := m.factsForPod(pod)
 	for i := range m.policies {
+		if !namespaceLabelsResolved && nodeUsesNamespaceLabels(m.policies[i].Rules) {
+			// Cannot evaluate this rule without namespace labels; ignore it.
+			continue
+		}
 		if policies.Evaluate(m.policies[i].Rules, facts) == policies.ResultTrue {
-			return i, nil
+			return i
 		}
 	}
-	return -1, nil
+	return -1
 }
 
-func (m *policyMatcher) factsForPod(pod *corev1.Pod) (policies.Facts, error) {
+// factsForPod builds the evaluation facts for a pod. The boolean reports whether
+// namespace labels were resolved: when a policy needs them but they cannot be
+// fetched, it is false and namespace-label rules are skipped by matchIndex.
+func (m *policyMatcher) factsForPod(pod *corev1.Pod) (policies.Facts, bool) {
 	facts := policies.Facts{
 		NamespaceName: pod.Namespace,
 		PodLabels:     pod.Labels,
 	}
 	if m.needsNamespaceLabels && m.wmeta != nil {
 		nsLabels, err := getNamespaceLabels(m.wmeta, pod.Namespace)
 		if err != nil {
-			return facts, err
+			log.Debugf("policy matcher: namespace labels unavailable for namespace %q, namespace-label rules will be skipped: %v", pod.Namespace, err)
+			return facts, false
 		}
 		facts.NamespaceLabels = nsLabels
 	}
-	return facts, nil
+	return facts, true
 }
 
 // usesNamespaceLabels reports whether any policy rule reads a namespace label,

pkg/clusteragent/admission/mutate/autoinstrumentation/target_matching_change_test.go

@@ -9,22 +9,23 @@ package autoinstrumentation
 
 import "testing"
 
-// TestMatching_NamespaceMissingFromStore documents an intentional behavior change
-// introduced by the policy-engine matcher.
+// TestMatching_UnresolvableNamespaceRuleIsSkipped documents an intentional
+// behavior change introduced by the policy-engine matcher.
 //
-// When a pod's namespace is absent from the workloadmeta store and at least one
-// target reads namespace labels, the matcher cannot resolve the facts it needs
-// and declines to inject (fail-safe: no injection on an unresolved namespace).
+// When a pod's namespace is absent from the workloadmeta store, a rule that
+// reads namespace labels cannot be evaluated. The matcher now ignores only that
+// rule and keeps evaluating the remaining rules in order, so an unresolvable
+// namespace rule never blocks an otherwise-matching rule from injecting.
 //
-// The legacy label-selector matcher was order-dependent here: a pod-only target
-// placed before the namespace-label target would match without ever resolving
-// namespace labels, so the same pod would be injected. The policy matcher
-// resolves namespace labels once, up front, which removes that order dependence
-// at the cost of not matching the earlier pod-only target in this case. This is
-// considered an improvement (consistent, order-independent fail-safe) and is
-// pinned here so the behavior is explicit and intentional rather than accidental.
-func TestMatching_NamespaceMissingFromStore(t *testing.T) {
-	const cfg = `
+// The legacy label-selector matcher instead aborted all matching as soon as it
+// reached a rule whose namespace could not be resolved (returning "no match").
+// It only injected in this situation by accident of ordering: if a matching
+// pod-only rule happened to come first, it short-circuited before the
+// unresolvable rule was reached. These cases pin the new, order-independent
+// behavior; the "rule first" case is the one that changes versus the legacy
+// matcher (it would previously have aborted).
+func TestMatching_UnresolvableNamespaceRuleIsSkipped(t *testing.T) {
+	const podRuleFirst = `
 apm_config:
   instrumentation:
     enabled: true
@@ -42,9 +43,30 @@ apm_config:
         ddTraceVersions:
           python: "default"
 `
-	// No namespace is registered in the store, so namespace-label resolution
-	// fails for the "ghost" namespace and matching is aborted.
-	runMatchCases(t, cfg, []matchCase{
-		{name: "unresolved namespace declines injection even for an earlier pod-only target", ns: "ghost", podLabels: map[string]string{"app": "web"}, want: ""},
+	const nsRuleFirst = `
+apm_config:
+  instrumentation:
+    enabled: true
+    targets:
+      - name: "ns-label"
+        namespaceSelector:
+          matchLabels:
+            instrument: "true"
+        ddTraceVersions:
+          python: "default"
+      - name: "pod-only"
+        podSelector:
+          matchLabels:
+            app: "web"
+        ddTraceVersions:
+          java: "default"
+`
+	// No namespace is registered in the store, so the namespace-label rule
+	// cannot be evaluated for the "ghost" namespace and is skipped.
+	runMatchCases(t, podRuleFirst, []matchCase{
+		{name: "pod-only rule before the unresolvable rule still matches", ns: "ghost", podLabels: map[string]string{"app": "web"}, want: "pod-only"},
+	})
+	runMatchCases(t, nsRuleFirst, []matchCase{
+		{name: "unresolvable rule first does not block the pod-only rule", ns: "ghost", podLabels: map[string]string{"app": "web"}, want: "pod-only"},
 	})
 }

pkg/clusteragent/admission/mutate/autoinstrumentation/target_mutator.go

@@ -406,11 +406,7 @@ func (m *TargetMutator) getMatchingTarget(pod *corev1.Pod) *targetInternal {
 
 	// The matcher and targets are aligned by index, so the first matching
 	// policy resolves directly to its injection config (first match wins).
-	idx, err := m.matcher.matchIndex(pod)
-	if err != nil {
-		log.Errorf("error encountered matching targets, aborting all together to avoid inaccurate match: %v", err)
-		return nil
-	}
+	idx := m.matcher.matchIndex(pod)
 	if idx < 0 || idx >= len(m.targets) {
 		return nil
 	}

pkg/clusteragent/admission/mutate/autoinstrumentation/target_mutator_test.go

@@ -664,15 +664,23 @@ func TestGetTargetLibraries(t *testing.T) {
 			},
 			expected: nil,
 		},
-		"missing namespace in store gets no tracers": {
+		// When the namespace is absent from the store, the namespace-label rule
+		// ("Enabled Prod Namespaces") cannot be evaluated and is skipped, so the
+		// pod falls through to the selector-less "Default" target rather than
+		// aborting all matching.
+		"missing namespace in store falls through to the default target": {
 			configPath: "testdata/filter.yaml",
 			in: &corev1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "foo",
 					Labels:    map[string]string{},
 				},
 			},
-			expected: nil,
+			expected: &targetInternal{
+				libVersions: []libInfo{
+					defaultLibInfoWithVersion(js, "v5"),
+				},
+			},
 		},
 		"unset tracer versions applies all tracers": {
 			configPath: "testdata/filter.yaml",