diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/auto_instrumentation.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/auto_instrumentation.go index bba9166d1bab..f1d64fa9a51f 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/auto_instrumentation.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/auto_instrumentation.go @@ -27,7 +27,7 @@ import ( // NewAutoInstrumentation is a helper function to create a fully initialized webhook for SSI. Our webhook is made up of // several components, but consumers of this webhook should not need to care about how the webhook is wired together. // When on-demand instrumentation is enabled and rcClient is non-nil, the mutator also subscribes to remote-config SSI -// policies (APM_POLICIES), which are layered on top of the configuration baseline at runtime. +// policies (APM_POLICIES), evaluated after static targets with last-TRUE-wins among RC policies. func NewAutoInstrumentation(datadogConfig config.Component, wmeta workloadmeta.Component, serverVersion *version.Info, csiDriverWatcher libraryinjection.CSIDriverWatcher, rcClient *rcclient.Client) (*Webhook, error) { config, err := NewConfig(datadogConfig) if err != nil { diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher.go index b2b452c458f4..c4fad3d01869 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher.go @@ -16,8 +16,8 @@ import ( ) // policyMatcher evaluates SSI policies against pods using the pure Go policy -// engine. It holds the effective ordered policy set (configuration policies, -// optionally augmented with remote-config ones) and resolves the first match. +// engine. The last policy that evaluates to TRUE wins. FALSE and ABSTAIN do +// not match. type policyMatcher struct { policies []policies.Policy wmeta workloadmeta.Component @@ -31,8 +31,7 @@ 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. +// Match returns the outcome of the last policy that matches the pod. func (m *policyMatcher) Match(pod *corev1.Pod) (policies.Outcome, bool) { idx := m.matchIndex(pod) if idx < 0 { @@ -41,13 +40,13 @@ func (m *policyMatcher) Match(pod *corev1.Pod) (policies.Outcome, bool) { return m.policies[idx].Outcome, true } -// matchIndex returns the index of the first policy that matches the pod, or -1 -// if none match. Policies are evaluated in order (first match wins). +// matchIndex returns the index of the last policy that evaluates to TRUE, or +// -1 if none match. // // Namespace labels are fetched lazily when the first policy that needs them is // reached. If they cannot be resolved, policies that need namespace labels are // skipped while policies using the available pod and namespace-name facts keep -// their relative first-match ordering. +// their relative last-match ordering. func (m *policyMatcher) matchIndex(pod *corev1.Pod) int { if m == nil || pod == nil { return -1 @@ -59,6 +58,7 @@ func (m *policyMatcher) matchIndex(pod *corev1.Pod) int { } namespaceLabelsLoaded := false namespaceLabelsUnavailable := false + matched := -1 for i := range m.policies { if nodeUsesNamespaceLabels(m.policies[i].Rules) && !namespaceLabelsLoaded { @@ -78,10 +78,10 @@ func (m *policyMatcher) matchIndex(pod *corev1.Pod) int { } if policies.Evaluate(m.policies[i].Rules, ctx) == policies.ResultTrue { - return i + matched = i } } - return -1 + return matched } func nodeUsesNamespaceLabels(n *policies.Node) bool { diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher_test.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher_test.go index 60e24da73a8a..dbf422a917c6 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher_test.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/policy_matcher_test.go @@ -44,8 +44,8 @@ func TestPolicyMatcherPodLabels(t *testing.T) { } out, ok := m.Match(podWith("any", map[string]string{"app": "db"})) - if !ok || out.TracerVersions["java"] != "latest" { - t.Fatalf("db pod: got %+v ok=%v", out, ok) + if !ok || out.TracerVersions["php"] != "latest" { + t.Fatalf("db pod should hit last TRUE (catch-all): got %+v ok=%v", out, ok) } out, ok = m.Match(podWith("any", map[string]string{"app": "web"})) diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies.go index 83c9e9133ea2..6bb5efd85f72 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies.go @@ -56,11 +56,11 @@ func remotePolicyPathOrder(path string) int { } // subscribeRemoteConfig wires the remote-config client to the mutator so that -// SSI policies delivered over remote config are layered on top of the -// configuration baseline. It is a no-op when remote config is not available, -// in which case the mutator keeps matching against its configuration baseline -// only. The wire format is the dd-wls policies document; targets do not appear -// on this path. +// SSI policies delivered over remote config are evaluated after static targets. +// RC policies are last-TRUE-wins on the wire order (default first, exceptions +// after). It is a no-op when remote config is not available, in which case the +// mutator keeps matching against its configuration baseline only. The wire +// format is the dd-wls policies document; targets do not appear on this path. func (m *TargetMutator) subscribeRemoteConfig(client *rcclient.Client) { if client == nil { return diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies_test.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies_test.go index 65e148a14781..00408dccccc7 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies_test.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/rc_policies_test.go @@ -24,6 +24,12 @@ apm_config: enabled: false ` +const rcSSIOnNoTargets = ` +apm_config: + instrumentation: + enabled: true +` + const rcCatchAllCfg = ` apm_config: instrumentation: @@ -78,74 +84,51 @@ func TestRemotePolicies_AppliedOnEmptyBaseline(t *testing.T) { require.Nil(t, m.getMatchingTarget(rcPod("ns", map[string]string{"app": "other"}))) } -// TestRemotePolicies_PrecedenceOverConfig verifies that remote policies are -// evaluated before the configuration baseline (first match wins), while -// non-matching pods still fall through to the configuration. -func TestRemotePolicies_PrecedenceOverConfig(t *testing.T) { +// TestRemotePolicies_HelmCatchAllWinsOverRemote verifies that an explicit static +// catch-all matches in the static phase, so remote policies never apply. +func TestRemotePolicies_HelmCatchAllWinsOverRemote(t *testing.T) { wmeta := newMatchTestWmeta(t) m := newMatchMutator(t, rcCatchAllCfg, wmeta) - // Baseline: the catch-all config target matches everything. - name, fromPolicy := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) - require.Equal(t, "config-default", name) - require.False(t, fromPolicy) - require.NoError(t, m.SetRemotePolicies([]policies.Policy{ podLabelPolicy("remote", "app", "db", true, map[string]string{"python": "default"}), + podLabelPolicy("remote-deny", "app", "legacy", false, nil), })) - // Remote wins for the matching pod... - name, fromPolicy = matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) - require.Equal(t, "remote", name) - require.True(t, fromPolicy) - - // ...but unrelated pods still fall through to the config baseline. - name, fromPolicy = matchedTarget(t, m, rcPod("ns", map[string]string{"app": "other"})) + name, fromPolicy := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) require.Equal(t, "config-default", name) require.False(t, fromPolicy) -} -// TestRemotePolicies_DenyStopsInjection verifies that a matched deny policy -// prevents injection even when a later policy (or the config baseline) would -// otherwise match. -func TestRemotePolicies_DenyStopsInjection(t *testing.T) { - wmeta := newMatchTestWmeta(t) - m := newMatchMutator(t, rcCatchAllCfg, wmeta) - - require.NoError(t, m.SetRemotePolicies([]policies.Policy{ - podLabelPolicy("remote-deny", "app", "legacy", false, nil), - })) - - // The deny policy matches first, so no target is returned even though the - // config catch-all would otherwise apply. - require.Nil(t, m.getMatchingTarget(rcPod("ns", map[string]string{"app": "legacy"}))) - - // A non-matching pod still hits the config baseline. - name, fromPolicy := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "ok"})) + name, fromPolicy = matchedTarget(t, m, rcPod("ns", map[string]string{"app": "legacy"})) require.Equal(t, "config-default", name) require.False(t, fromPolicy) } -// TestRemotePolicies_ClearRevertsToBaseline verifies that clearing the remote -// policies reverts the mutator to its configuration baseline. +// TestRemotePolicies_ClearRevertsToBaseline verifies that clearing remote +// policies restores the synthetic inject-all default. func TestRemotePolicies_ClearRevertsToBaseline(t *testing.T) { wmeta := newMatchTestWmeta(t) - m := newMatchMutator(t, rcCatchAllCfg, wmeta) + m := newMatchMutator(t, rcSSIOnNoTargets, wmeta) + + name, fromPolicy := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) + require.Equal(t, "default", name) + require.False(t, fromPolicy) require.NoError(t, m.SetRemotePolicies([]policies.Policy{ podLabelPolicy("remote", "app", "db", true, map[string]string{"python": "default"}), })) - name, _ := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) + name, fromPolicy = matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) require.Equal(t, "remote", name) + require.True(t, fromPolicy) m.ClearRemotePolicies() - name, fromPolicy := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) - require.Equal(t, "config-default", name) + name, fromPolicy = matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) + require.Equal(t, "default", name) require.False(t, fromPolicy) } -// TestRemotePolicies_FirstMatchWins verifies the ordering among remote policies. -func TestRemotePolicies_FirstMatchWins(t *testing.T) { +// TestRemotePolicies_LastMatchWins verifies last-TRUE-wins among remote policies. +func TestRemotePolicies_LastMatchWins(t *testing.T) { wmeta := newMatchTestWmeta(t) m := newMatchMutator(t, rcDisabledCfg, wmeta) @@ -155,15 +138,15 @@ func TestRemotePolicies_FirstMatchWins(t *testing.T) { })) name, _ := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) - require.Equal(t, "first", name) + require.Equal(t, "second", name) } // TestOnRemoteConfigUpdate_ParsesAndApplies exercises the remote-config callback // end to end with a dd-wls policies document, then verifies that an empty update -// reverts the mutator to the configuration baseline. +// clears remote policies. func TestOnRemoteConfigUpdate_ParsesAndApplies(t *testing.T) { wmeta := newMatchTestWmeta(t) - m := newMatchMutator(t, rcCatchAllCfg, wmeta) + m := newMatchMutator(t, rcDisabledCfg, wmeta) const raw = `{ "policies": [{ @@ -196,11 +179,9 @@ func TestOnRemoteConfigUpdate_ParsesAndApplies(t *testing.T) { require.Equal(t, "java for db-user", name) require.True(t, fromPolicy) - // An empty update reverts to the config baseline. + // An empty update clears remote policies (SSI off → nothing). m.onRemoteConfigUpdate(map[string]state.RawConfig{}, apply) - name, fromPolicy = matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db-user"})) - require.Equal(t, "config-default", name) - require.False(t, fromPolicy) + require.Nil(t, m.getMatchingTarget(rcPod("ns", map[string]string{"app": "db-user"}))) } func TestOnRemoteConfigUpdate_OrdersPolicyIDsByNumericPrefix(t *testing.T) { @@ -242,14 +223,15 @@ func TestOnRemoteConfigUpdate_OrdersPolicyIDsByNumericPrefix(t *testing.T) { "datadog/2/APM_POLICIES/2.allow/config": {Config: []byte(allow)}, }, func(string, state.ApplyStatus) {}) - set := m.activeSet() - require.Len(t, set.matcher.policies, 2) - require.Equal(t, "allow", set.matcher.policies[0].Name) - require.Equal(t, "deny", set.matcher.policies[1].Name) + remotePolicies := m.remotePolicies.Load() + require.NotNil(t, remotePolicies) + require.Len(t, remotePolicies.matcher.policies, 2) + // Numeric prefix sorts 2.allow before 10.deny; last-TRUE-wins then picks deny. + require.Equal(t, "allow", remotePolicies.matcher.policies[0].Name) + require.Equal(t, "deny", remotePolicies.matcher.policies[1].Name) - name, fromPolicy := matchedTarget(t, m, rcPod("ns", map[string]string{"app": "db"})) - require.Equal(t, "allow", name) - require.True(t, fromPolicy) + // Last-TRUE-wins: deny is after allow, both match app=db. + require.Nil(t, m.getMatchingTarget(rcPod("ns", map[string]string{"app": "db"}))) } // TestOnRemoteConfigUpdate_InvalidPayloadKeepsBaseline verifies that one malformed diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/target_matching_test.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/target_matching_test.go index eb381daadea1..ffc8609210fb 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/target_matching_test.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/target_matching_test.go @@ -7,13 +7,11 @@ package autoinstrumentation -// This file is a behavioral characterization (golden master) of the target -// matcher: which target a pod resolves to for every supported selector shape -// and for the "first match wins" ordering rule. It exercises the matcher -// exclusively through the public TargetMutator API (NewTargetMutator + -// getMatchingTarget), so the exact same suite runs against the legacy -// label-selector implementation and the policy-engine implementation and proves -// they agree. +// This file is a behavioral characterization of the target matcher: which +// target a pod resolves to for every supported selector shape, configuration +// first-wins (targets reversed at construction so the last-TRUE-wins matcher +// preserves config order), and static vs remote-config source precedence. It +// exercises matching through NewTargetMutator + getMatchingTarget. import ( "testing" @@ -31,6 +29,7 @@ import ( workloadmetamock "github.com/DataDog/datadog-agent/comp/core/workloadmeta/mock" configmock "github.com/DataDog/datadog-agent/pkg/config/mock" "github.com/DataDog/datadog-agent/pkg/util/fxutil" + "github.com/DataDog/dd-policy-engine/go/policies" ) // matchCase is a single (pod -> matched target name) expectation. @@ -84,8 +83,8 @@ func runMatchCases(t *testing.T, yamlCfg string, cases []matchCase, namespaces . } } -// TestMatching_Precedence verifies the "first match wins" ordering rule when a -// pod satisfies more than one target. +// TestMatching_Precedence verifies configuration first-wins when a pod satisfies +// more than one target. func TestMatching_Precedence(t *testing.T) { const cfg = ` apm_config: @@ -118,6 +117,139 @@ apm_config: }) } +// TestMatching_EvaluationSources pins which source wins: static targets +// (including enabledNamespaces), remote-config policies (wire last-TRUE-wins), +// then the SSI inject-all default when both are absent. +// +// SSI | static targets | RC | Decision +// off | — | none | nothing +// off | — | policies | last matching policy, else nothing +// on | none | none | everything +// on | none | policies | last matching policy, else nothing +// on | present | none | first matching target, else nothing +// on | present | policies | first matching target, else last matching policy, else nothing +func TestMatching_EvaluationSources(t *testing.T) { + const ssiOff = ` +apm_config: + instrumentation: + enabled: false +` + const ssiOnNoTargets = ` +apm_config: + instrumentation: + enabled: true +` + const ssiOnEnabledNamespaces = ` +apm_config: + instrumentation: + enabled: true + enabled_namespaces: + - app-ns +` + const ssiOnTargets = ` +apm_config: + instrumentation: + enabled: true + targets: + - name: "helm-python" + podSelector: + matchLabels: + language: "python" + ddTraceVersions: + python: "default" + - name: "helm-python-also" + podSelector: + matchLabels: + language: "python" + ddTraceVersions: + python: "v1" +` + + rcPolicies := []policies.Policy{ + { + Name: "rc-default", + Rules: policies.AlwaysTrue(), + Outcome: policies.Outcome{Inject: true, InjectSet: true, TracerVersions: map[string]string{"java": "default"}}, + }, + podLabelPolicy("rc-db", "app", "db", true, map[string]string{"java": "v1"}), + podLabelPolicy("rc-legacy-deny", "app", "legacy", false, nil), + } + + type want struct { + name string + fromPolicy bool + } + nothing := want{} + helm := func(name string) want { return want{name: name} } + rc := func(name string) want { return want{name: name, fromPolicy: true} } + + assertMatch := func(t *testing.T, m *TargetMutator, ns string, labels map[string]string, w want) { + t.Helper() + name, fromPolicy := matchedTarget(t, m, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: ns, Labels: labels}}) + require.Equal(t, w.name, name) + require.Equal(t, w.fromPolicy, fromPolicy) + } + + t.Run("ssi off / no RC / nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOff, newMatchTestWmeta(t)) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, nothing) + assertMatch(t, m, "ns", map[string]string{"app": "other"}, nothing) + }) + + t.Run("ssi off / RC / last matching policy, else nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOff, newMatchTestWmeta(t)) + require.NoError(t, m.SetRemotePolicies(rcPolicies)) + assertMatch(t, m, "ns", map[string]string{"app": "legacy"}, nothing) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, rc("rc-db")) + assertMatch(t, m, "ns", map[string]string{"app": "other"}, rc("rc-default")) + }) + + t.Run("ssi on / no targets / no RC / everything", func(t *testing.T) { + m := newMatchMutator(t, ssiOnNoTargets, newMatchTestWmeta(t)) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, helm("default")) + assertMatch(t, m, "ns", map[string]string{"app": "other"}, helm("default")) + }) + + t.Run("ssi on / no targets / RC / last matching policy, else nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOnNoTargets, newMatchTestWmeta(t)) + require.NoError(t, m.SetRemotePolicies(rcPolicies)) + assertMatch(t, m, "ns", map[string]string{"app": "legacy"}, nothing) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, rc("rc-db")) + assertMatch(t, m, "ns", map[string]string{"app": "other"}, rc("rc-default")) + }) + + t.Run("ssi on / enabledNamespaces / no RC / first matching target, else nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOnEnabledNamespaces, newMatchTestWmeta(t)) + assertMatch(t, m, "app-ns", map[string]string{"app": "db"}, helm("default")) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, nothing) + }) + + t.Run("ssi on / enabledNamespaces / RC / first matching target, else last matching policy, else nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOnEnabledNamespaces, newMatchTestWmeta(t)) + require.NoError(t, m.SetRemotePolicies(rcPolicies)) + assertMatch(t, m, "app-ns", map[string]string{"app": "legacy"}, helm("default")) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, rc("rc-db")) + assertMatch(t, m, "ns", map[string]string{"app": "legacy"}, nothing) + assertMatch(t, m, "ns", map[string]string{"app": "other"}, rc("rc-default")) + }) + + t.Run("ssi on / targets / no RC / first matching target, else nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOnTargets, newMatchTestWmeta(t)) + assertMatch(t, m, "ns", map[string]string{"language": "python"}, helm("helm-python")) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, nothing) + }) + + t.Run("ssi on / targets / RC / first matching target, else last matching policy, else nothing", func(t *testing.T) { + m := newMatchMutator(t, ssiOnTargets, newMatchTestWmeta(t)) + require.NoError(t, m.SetRemotePolicies(rcPolicies)) + assertMatch(t, m, "ns", map[string]string{"language": "python"}, helm("helm-python")) + assertMatch(t, m, "ns", map[string]string{"language": "python", "app": "db"}, helm("helm-python")) + assertMatch(t, m, "ns", map[string]string{"app": "db"}, rc("rc-db")) + assertMatch(t, m, "ns", map[string]string{"app": "legacy"}, nothing) + assertMatch(t, m, "ns", map[string]string{"app": "other"}, rc("rc-default")) + }) +} + // TestMatching_PodMatchLabels verifies that pod matchLabels are ANDed and that // extra labels on the pod do not prevent a match. func TestMatching_PodMatchLabels(t *testing.T) { diff --git a/pkg/clusteragent/admission/mutate/autoinstrumentation/target_mutator.go b/pkg/clusteragent/admission/mutate/autoinstrumentation/target_mutator.go index d87ac2c3f90e..c8cf7c43f7a6 100644 --- a/pkg/clusteragent/admission/mutate/autoinstrumentation/target_mutator.go +++ b/pkg/clusteragent/admission/mutate/autoinstrumentation/target_mutator.go @@ -11,6 +11,7 @@ import ( "encoding/json" "errors" "fmt" + "slices" "strings" "go.uber.org/atomic" @@ -37,11 +38,9 @@ const ( AppliedPolicyEnvVar = "DD_INSTRUMENTATION_APPLIED_POLICY" ) -// policySet is an immutable, atomically swappable view of the policies a -// TargetMutator matches against. matcher.policies and targets are aligned by -// index, so a match resolves directly to its injection config. A disabled -// mutator is simply represented by an empty set (no targets, no policies), -// which naturally matches nothing. +// policySet is matcher.policies aligned with injection targets by index, so a +// match resolves directly to its injection config. An empty set (no targets, no +// policies) matches nothing. type policySet struct { targets []targetInternal matcher *policyMatcher @@ -56,20 +55,21 @@ type TargetMutator struct { containerRegistry string mutateUnlabelled bool defaultLibVersions []libInfo - - // base is the policy set derived from the agent configuration file. It is - // the baseline that remote-config policies are layered on top of. - base policySet - // active is the effective policy set: base when no remote policies are - // present, or remote policies (taking precedence) layered on top of base - // otherwise. It is swapped atomically on remote-config updates. - active atomic.Pointer[policySet] + ssiEnabled bool + + // staticPolicies is local targeting: explicit targets, or enabledNamespaces + // as a namespace target (Helm, Operator, or datadog.yaml). Empty when SSI + // is off or when SSI is on with no targeting. + staticPolicies policySet + // injectAll is the SSI-on fallback when there is no static targeting and no RC. + injectAll *targetInternal + // remotePolicies is the current RC policy set. Nil when none are installed. + remotePolicies atomic.Pointer[policySet] } // NewTargetMutator creates a new mutator for target based workload selection. We convert the targets to a more // efficient internal format for quick lookups. When on-demand instrumentation is enabled and rcClient is non-nil, the -// mutator also subscribes to remote-config SSI policies, which are layered on top of the configuration baseline at -// runtime. +// mutator also subscribes to remote-config SSI policies, which are evaluated after static targets. func NewTargetMutator(config *Config, wmeta workloadmeta.Component, imageResolver imageresolver.Resolver, csiDriverWatcher libraryinjection.CSIDriverWatcher, rcClient *rcclient.Client) (*TargetMutator, error) { // Create a map of user-configured disabled namespaces for quick lookups. // Default namespaces (kube-system, datadog agent namespace) are excluded at @@ -82,27 +82,20 @@ func NewTargetMutator(config *Config, wmeta workloadmeta.Component, imageResolve // Fetch the default lib versions to use if there are no user defined versions. defaultLibVersions := getAllLatestDefaultLibraries(config.containerRegistry) - // If there are no targets, we should fall back to enabledNamespace/libVersions. If those are also not defined, the - // expected behavior is to inject all pods into all namespaces. A disabled mutator keeps an empty baseline so it - // matches nothing unless remote-config policies are layered on. + ssiEnabled := config.Instrumentation.Enabled var targets []Target - if config.Instrumentation.Enabled { + if ssiEnabled { targets = config.Instrumentation.Targets - if len(targets) == 0 { + if len(targets) == 0 && len(config.Instrumentation.EnabledNamespaces) > 0 { targets = append(targets, createDefaultTarget(config.Instrumentation.EnabledNamespaces, config.Instrumentation.LibVersions)) } } - internalTargets, err := buildInternalTargets(config, targets, defaultLibVersions) + staticPolicies, err := newPolicySet(config, targets, defaultLibVersions, wmeta) if err != nil { return nil, err } - // Lower the configuration targets into policies once, at the config - // boundary. Everything past this point matches on policies only, aligned - // by index with the internal targets above. - configPolicies := policiesFromTargets(targets) - m := &TargetMutator{ disabledNamespaces: disabledNamespacesMap, securityClientLibraryMutator: config.securityClientLibraryMutator, @@ -110,12 +103,17 @@ func NewTargetMutator(config *Config, wmeta workloadmeta.Component, imageResolve containerRegistry: config.containerRegistry, mutateUnlabelled: config.mutateUnlabelled, defaultLibVersions: defaultLibVersions, - base: policySet{ - targets: internalTargets, - matcher: newPolicyMatcher(configPolicies, wmeta), - }, + ssiEnabled: ssiEnabled, + staticPolicies: staticPolicies, + } + // SSI on and no static targeting: prepare inject-all. Applied only when RC is also absent. + if ssiEnabled && len(targets) == 0 { + fallback, err := buildInternalTargets(config, []Target{createDefaultTarget(nil, config.Instrumentation.LibVersions)}, defaultLibVersions) + if err != nil { + return nil, err + } + m.injectAll = &fallback[0] } - m.active.Store(&m.base) core := newMutatorCore(config, wmeta, imageResolver, csiDriverWatcher) m.core = core @@ -130,6 +128,22 @@ func NewTargetMutator(config *Config, wmeta workloadmeta.Component, imageResolve return m, nil } +func newPolicySet(config *Config, targets []Target, defaultLibVersions []libInfo, wmeta workloadmeta.Component) (policySet, error) { + // Configuration targets are first-wins. Reverse so the last-TRUE-wins matcher + // preserves that order. RC is already last-wins on the wire and is not reversed. + targets = slices.Clone(targets) + slices.Reverse(targets) + + internalTargets, err := buildInternalTargets(config, targets, defaultLibVersions) + if err != nil { + return policySet{}, err + } + return policySet{ + targets: internalTargets, + matcher: newPolicyMatcher(policiesFromTargets(targets), wmeta), + }, nil +} + // buildInternalTargets converts configuration targets into the internal format used for injection. Matching is not // part of it: the selectors are lowered into policies by policiesFromTargets and evaluated by the policy engine. func buildInternalTargets(config *Config, targets []Target, defaultLibVersions []libInfo) ([]targetInternal, error) { @@ -184,40 +198,31 @@ func buildInternalTargets(config *Config, targets []Target, defaultLibVersions [ return internalTargets, nil } -// activeSet returns the effective policy set. It is never nil after the -// mutator has been constructed. -func (m *TargetMutator) activeSet() *policySet { - return m.active.Load() -} - -// SetRemotePolicies layers remote-config policies on top of the configuration -// baseline and swaps in the result atomically. Remote policies are evaluated -// first (they take precedence), then the configuration policies, preserving -// first-match-wins semantics. +// SetRemotePolicies installs remote-config policies as a second last-TRUE-wins +// phase after static targets. The wire order is already last-TRUE-wins (default +// first, exceptions after) and is stored as-is. func (m *TargetMutator) SetRemotePolicies(ps []policies.Policy) error { + if len(ps) == 0 { + m.ClearRemotePolicies() + return nil + } + remoteTargets, err := buildInternalTargetsFromPolicies(m.core.config, ps, m.defaultLibVersions) if err != nil { return err } - combinedPolicies := make([]policies.Policy, 0, len(ps)+len(m.base.matcher.policies)) - combinedPolicies = append(combinedPolicies, ps...) - combinedPolicies = append(combinedPolicies, m.base.matcher.policies...) - - combinedTargets := make([]targetInternal, 0, len(remoteTargets)+len(m.base.targets)) - combinedTargets = append(combinedTargets, remoteTargets...) - combinedTargets = append(combinedTargets, m.base.targets...) - - m.active.Store(&policySet{ - targets: combinedTargets, - matcher: newPolicyMatcher(combinedPolicies, m.core.wmeta), + m.remotePolicies.Store(&policySet{ + targets: remoteTargets, + matcher: newPolicyMatcher(ps, m.core.wmeta), }) return nil } -// ClearRemotePolicies reverts the mutator to the configuration baseline. +// ClearRemotePolicies drops remote-config policies. Matching falls back to +// static targets, then the SSI inject-all default if there is no static targeting. func (m *TargetMutator) ClearRemotePolicies() { - m.active.Store(&m.base) + m.remotePolicies.Store(nil) } // buildInternalTargetsFromPolicies resolves each policy's outcome (tracer @@ -305,10 +310,7 @@ func (m *TargetMutator) MutatePod(pod *corev1.Pod, ns string, _ dynamic.Interfac // Library selection still short-circuits on annotations (unchanged GA // precedence). SSI mode is decided separately from whether a target/policy // matched the pod — not from a namespace-level eligibility approximation. - // Load the policy set once so target selection and SSI mode cannot diverge - // if remote config swaps active between two match evaluations. - set := m.activeSet() - target, ssi := m.resolveTargetAndSSI(set, pod) + target, ssi := m.resolveTargetAndSSI(pod) if target == nil { return false, nil } @@ -411,14 +413,13 @@ type targetInternal struct { // getTarget determines which target to use for a given a pod, which includes the set of tracing libraries to inject. // Library annotations still short-circuit matching (GA precedence unchanged in this change). func (m *TargetMutator) getTarget(pod *corev1.Pod) *targetInternal { - target, _ := m.resolveTargetAndSSI(m.activeSet(), pod) + target, _ := m.resolveTargetAndSSI(pod) return target } -// resolveTargetAndSSI selects what to inject and whether the pod is in SSI mode -// from a single loaded policy set snapshot. -func (m *TargetMutator) resolveTargetAndSSI(set *policySet, pod *corev1.Pod) (*targetInternal, bool) { - matched := m.matchingTargetFromSet(set, pod) +// resolveTargetAndSSI selects what to inject and whether the pod is in SSI mode. +func (m *TargetMutator) resolveTargetAndSSI(pod *corev1.Pod) (*targetInternal, bool) { + matched := m.getMatchingTarget(pod) result := m.getTargetFromAnnotation(pod) if !result.shouldContinue { return result.target, matched != nil @@ -478,43 +479,52 @@ func (m *TargetMutator) getTargetFromAnnotation(pod *corev1.Pod) *annotationResu } } -// getMatchingTarget filters a pod based on the targets. It returns the target to inject. -// -// Matching is delegated to the native policy engine: each target is compiled -// into an equivalent policy (namespace and pod selectors ANDed together) and -// the first policy that evaluates to true wins, preserving the previous -// first-match semantics without relying on CGO or k8s label selectors. +// getMatchingTarget: static targets first, then RC, then SSI inject-all if both +// are absent. A matched deny returns nil and does not fall through. func (m *TargetMutator) getMatchingTarget(pod *corev1.Pod) *targetInternal { - return m.matchingTargetFromSet(m.activeSet(), pod) -} - -func (m *TargetMutator) matchingTargetFromSet(set *policySet, pod *corev1.Pod) *targetInternal { - // If the namespace is disabled, we don't need to check the targets. if _, ok := m.disabledNamespaces[pod.Namespace]; ok { return nil } - // The matcher and targets are aligned by index, so the first matching - // policy resolves directly to its injection config (first match wins). + if t, matched := applyMatch(&m.staticPolicies, pod); matched { + return t + } + remotePolicies := m.remotePolicies.Load() + if t, matched := applyMatch(remotePolicies, pod); matched { + return t + } + if m.ssiEnabled && !hasTargets(&m.staticPolicies) && remotePolicies == nil { + return m.injectAll + } + return nil +} + +func hasTargets(set *policySet) bool { + return set != nil && len(set.targets) > 0 +} + +// applyMatch returns the injection target for a policy set. matched is true +// when a policy evaluated to TRUE (even if that policy denies injection). +func applyMatch(set *policySet, pod *corev1.Pod) (*targetInternal, bool) { + if set == nil || set.matcher == nil { + return nil, false + } + idx := set.matcher.matchIndex(pod) if idx < 0 || idx >= len(set.targets) { - return nil + return nil, false } - // A matched policy may explicitly deny injection (first match wins). if !set.matcher.policies[idx].Outcome.Inject { log.Debugf("Pod %q matched policy %q which denies injection", mutatecommon.PodString(pod), set.targets[idx].name) - return nil + return nil, true } log.Debugf("Pod %q matched target %q", mutatecommon.PodString(pod), set.targets[idx].name) - return &set.targets[idx] + return &set.targets[idx], true } -// createDefaultTarget is used when there are no targets. If a user configures enabledNamespaces and libVersions, which -// are mutually exclusive with a list of targets, then we need to translate those configuration options into a target. -// Additionally, if there are no targets and enabledNamespaces/libVersions are not set, the expected behavior is that -// we would inject all SDKs to all pods. This target encompasses both of those cases. +// createDefaultTarget translates enabledNamespaces/libVersions into a target. func createDefaultTarget(namespaces []string, pinnedLibVersions map[string]string) Target { // Create a default target. target := Target{ diff --git a/releasenotes-dca/notes/ssi-eval-last-wins-dd7097f68bf689a1.yaml b/releasenotes-dca/notes/ssi-eval-last-wins-dd7097f68bf689a1.yaml new file mode 100644 index 000000000000..f5736f840e81 --- /dev/null +++ b/releasenotes-dca/notes/ssi-eval-last-wins-dd7097f68bf689a1.yaml @@ -0,0 +1,16 @@ +# Each section from every release note are combined when the +# CHANGELOG-DCA.rst is rendered. So the text needs to be worded so that +# it does not depend on any information only available in another +# section. This may mean repeating some details, but each section +# must be readable independently of the other. +# +# Each section note must be formatted as reStructuredText. +--- +enhancements: + - | + Single Step Instrumentation now evaluates local targeting (Helm, Operator, + or ``datadog.yaml``) before Remote Config policies. Local targets keep + first-match-wins order. Remote Config policies use last-match-wins: the + last matching policy applies, so a catch-all can be listed first and + exceptions after. A workload that matches a local target is not overridden + by a remote deny. diff --git a/test/new-e2e/tests/ssi/BUILD.bazel b/test/new-e2e/tests/ssi/BUILD.bazel index d29410618640..5a36a7c11ccc 100644 --- a/test/new-e2e/tests/ssi/BUILD.bazel +++ b/test/new-e2e/tests/ssi/BUILD.bazel @@ -48,6 +48,7 @@ dd_agent_go_test( "testdata/namespace_selection.yaml", "testdata/rc_deny_targeted_namespace_policy.json", "testdata/rc_host_linux_only_policy.json", + "testdata/rc_last_wins_other_policy.json", "testdata/rc_namespace_other_policy.json", "testdata/rc_policies.yaml", "testdata/registry_allow_list.yaml", @@ -61,6 +62,7 @@ dd_agent_go_test( "testdata/namespace_selection.yaml", "testdata/rc_deny_targeted_namespace_policy.json", "testdata/rc_host_linux_only_policy.json", + "testdata/rc_last_wins_other_policy.json", "testdata/rc_namespace_other_policy.json", "testdata/rc_policies.yaml", "testdata/registry_allow_list.yaml", diff --git a/test/new-e2e/tests/ssi/ssi_test.go b/test/new-e2e/tests/ssi/ssi_test.go index 45832d831cbe..4c77c9330676 100644 --- a/test/new-e2e/tests/ssi/ssi_test.go +++ b/test/new-e2e/tests/ssi/ssi_test.go @@ -64,6 +64,9 @@ var rcNamespaceOtherPolicyJSON []byte //go:embed testdata/rc_deny_targeted_namespace_policy.json var rcDenyTargetedNamespacePolicyJSON []byte +//go:embed testdata/rc_last_wins_other_policy.json +var rcLastWinsOtherPolicyJSON []byte + const ( apmPoliciesRCProduct = "APM_POLICIES" rcHostLinuxOnlyConfigID = "1.host-linux-only" @@ -72,6 +75,8 @@ const ( rcNamespaceOtherConfigName = "config" rcDenyTargetedNamespaceConfigID = "1.deny-targeted-namespace" rcDenyTargetedNamespaceConfigName = "config" + rcLastWinsOtherConfigID = "1.last-wins-other" + rcLastWinsOtherConfigName = "config" rcFakeIntakeDefaultOrgID = "42" rcHelmTargetNamespace = "targeted-namespace" rcHelmTargetApp = "rc-target-python" @@ -701,21 +706,21 @@ func (v *ssiSuite) TestRemoteConfig() { v.requireHelmTargetStillSSI(k8s) }) - // Remote policies are evaluated before local helm targets. A deny matching - // targeted-namespace blocks SSI on the helm workload; namespace "other" is unchanged. - v.Run("NamespacePolicyOverridesHelmTarget", func() { + // Static targeting is evaluated before RC. A remote deny matching targeted-namespace + // must not block the helm python workload; RC only applies when no helm target matches. + // An allow matching "other" is published first so a pod leaves the helm baseline; + // replacing that same document with the deny must restore lib-injection in "other" + // (the deny does not match that namespace) while helm targeting stays SSI. + v.Run("HelmTargetWinsOverRemoteDeny", func() { k8s := v.Env().KubernetesCluster.Client() - cleanup := v.pushAPMPolicy(fi, rcDenyTargetedNamespaceConfigID, rcDenyTargetedNamespaceConfigName, rcDenyTargetedNamespacePolicyJSON) + cleanup := v.pushAPMPolicy(fi, rcDenyTargetedNamespaceConfigID, rcDenyTargetedNamespaceConfigName, rcNamespaceOtherPolicyJSON) defer cleanup() - helmPod := RestartUntil(v.T(), k8s, rcHelmTargetNamespace, rcHelmTargetApp, noInjection(rcHelmTargetApp)) - helmValidator := testutils.NewPodValidator(helmPod, testutils.InjectionModeAuto) - helmValidator.RequireNoInjection(v.T()) - helmValidator.RequireMissingAnnotations(v.T(), []string{testutils.AppliedTargetAnnotation, testutils.AppliedPolicyAnnotation}) + RestartUntil(v.T(), k8s, rcOtherNamespace, rcAnnotatedPodApp, hasInstallType(rcAnnotatedPodApp, "k8s_single_step")) - RestartPod(v.T(), k8s, rcOtherNamespace, rcAnnotatedPodApp) - annotated := WaitForMutatedPodInNamespace(v.T(), k8s, rcOtherNamespace, rcAnnotatedPodApp) + _ = v.pushAPMPolicy(fi, rcDenyTargetedNamespaceConfigID, rcDenyTargetedNamespaceConfigName, rcDenyTargetedNamespacePolicyJSON) + annotated := RestartUntil(v.T(), k8s, rcOtherNamespace, rcAnnotatedPodApp, hasInstallType(rcAnnotatedPodApp, "k8s_lib_injection")) annotatedValidator := testutils.NewPodValidator(annotated, testutils.InjectionModeAuto) annotatedValidator.RequireInjection(v.T(), []string{rcAnnotatedPodApp}) annotatedValidator.RequireInstallType(v.T(), "k8s_lib_injection", []string{rcAnnotatedPodApp}) @@ -727,14 +732,45 @@ func (v *ssiSuite) TestRemoteConfig() { unannotatedValidator := testutils.NewPodValidator(unannotated, testutils.InjectionModeAuto) unannotatedValidator.RequireNoInjection(v.T()) unannotatedValidator.RequireMissingAnnotations(v.T(), []string{testutils.AppliedTargetAnnotation, testutils.AppliedPolicyAnnotation}) + + v.requireHelmTargetStillSSI(k8s) + }) + + // Two RC policies both match namespace "other": allow then deny. Last TRUE wins, + // so the unannotated pod is uninjected. An allow-only document is published first + // so the pod leaves the helm baseline; replacing it with allow+deny must restore + // no-injection. Delete-then-add would restore the baseline as soon as the allow is + // gone, before last-wins is applied. First-wins would keep SSI after the replace. + v.Run("LastMatchingRemotePolicyWins", func() { + k8s := v.Env().KubernetesCluster.Client() + + cleanup := v.pushAPMPolicy(fi, rcLastWinsOtherConfigID, rcLastWinsOtherConfigName, rcNamespaceOtherPolicyJSON) + defer cleanup() + + RestartUntil(v.T(), k8s, rcOtherNamespace, rcUnannotatedPodApp, hasInstallType(rcUnannotatedPodApp, "k8s_single_step")) + + _ = v.pushAPMPolicy(fi, rcLastWinsOtherConfigID, rcLastWinsOtherConfigName, rcLastWinsOtherPolicyJSON) + unannotated := RestartUntil(v.T(), k8s, rcOtherNamespace, rcUnannotatedPodApp, noInjection(rcUnannotatedPodApp)) + unannotatedValidator := testutils.NewPodValidator(unannotated, testutils.InjectionModeAuto) + unannotatedValidator.RequireNoInjection(v.T()) + unannotatedValidator.RequireMissingAnnotations(v.T(), []string{testutils.AppliedTargetAnnotation, testutils.AppliedPolicyAnnotation}) + + RestartPod(v.T(), k8s, rcOtherNamespace, rcAnnotatedPodApp) + annotated := WaitForMutatedPodInNamespace(v.T(), k8s, rcOtherNamespace, rcAnnotatedPodApp) + annotatedValidator := testutils.NewPodValidator(annotated, testutils.InjectionModeAuto) + annotatedValidator.RequireInjection(v.T(), []string{rcAnnotatedPodApp}) + annotatedValidator.RequireInstallType(v.T(), "k8s_lib_injection", []string{rcAnnotatedPodApp}) + annotatedValidator.RequireMissingEnvs(v.T(), []string{"DD_TRACE_ENABLED"}, []string{rcAnnotatedPodApp}) + annotatedValidator.RequireMissingAnnotations(v.T(), []string{testutils.AppliedTargetAnnotation, testutils.AppliedPolicyAnnotation}) + + v.requireHelmTargetStillSSI(k8s) }) } func (v *ssiSuite) requireHelmTargetStillSSI(k8s kubeClient.Interface) { v.T().Helper() - RestartPod(v.T(), k8s, rcHelmTargetNamespace, rcHelmTargetApp) - pod := WaitForMutatedPodInNamespace(v.T(), k8s, rcHelmTargetNamespace, rcHelmTargetApp) + pod := RestartUntil(v.T(), k8s, rcHelmTargetNamespace, rcHelmTargetApp, hasInstallType(rcHelmTargetApp, "k8s_single_step")) podValidator := testutils.NewPodValidator(pod, testutils.InjectionModeAuto) podValidator.RequireInjection(v.T(), []string{rcHelmTargetApp}) podValidator.RequireInstallType(v.T(), "k8s_single_step", []string{rcHelmTargetApp}) diff --git a/test/new-e2e/tests/ssi/testdata/rc_deny_targeted_namespace_policy.json b/test/new-e2e/tests/ssi/testdata/rc_deny_targeted_namespace_policy.json index cd03788c460b..884c46a96b7b 100644 --- a/test/new-e2e/tests/ssi/testdata/rc_deny_targeted_namespace_policy.json +++ b/test/new-e2e/tests/ssi/testdata/rc_deny_targeted_namespace_policy.json @@ -1,7 +1,7 @@ { "policies": [ { - "description": "deny SSI in targeted-namespace (overrides helm target)", + "description": "deny SSI in targeted-namespace (does not override helm target)", "id": {"hi": 10, "lo": 3}, "version": 1, "rules": { diff --git a/test/new-e2e/tests/ssi/testdata/rc_last_wins_other_policy.json b/test/new-e2e/tests/ssi/testdata/rc_last_wins_other_policy.json new file mode 100644 index 000000000000..8fdd14d61469 --- /dev/null +++ b/test/new-e2e/tests/ssi/testdata/rc_last_wins_other_policy.json @@ -0,0 +1,48 @@ +{ + "policies": [ + { + "description": "allow SSI in namespace other", + "id": {"hi": 10, "lo": 4}, + "version": 1, + "rules": { + "node_type": "EvaluatorNode", + "node": { + "description": "namespace name other", + "eval_type": "StrEvaluator", + "eval": { + "id": "NAMESPACE_NAME", + "cmp": "CMP_EXACT", + "value": "other" + } + } + }, + "actions": [ + {"action": "INJECT_ALLOW"}, + { + "action": "ENABLE_SDK", + "values": ["python=default"] + } + ] + }, + { + "description": "deny SSI in namespace other (last TRUE wins)", + "id": {"hi": 10, "lo": 5}, + "version": 1, + "rules": { + "node_type": "EvaluatorNode", + "node": { + "description": "namespace name other", + "eval_type": "StrEvaluator", + "eval": { + "id": "NAMESPACE_NAME", + "cmp": "CMP_EXACT", + "value": "other" + } + } + }, + "actions": [ + {"action": "INJECT_DENY"} + ] + } + ] +}