[APMON-406] Re-define Single Step Instrumentation logic for existing …

…configuration options (#21673) * [APM Onboarding] Refactor APM Instrumentation configurations * Change config options * Fix unit-tests after redefining SSI configuration options * Fix unit test that depends on the dd resoucre namespace * Simplify namespace filtering logic * Update pkg/clusteragent/admission/mutate/common.go Co-authored-by: Cedric Lamoriniere <[email protected]> --------- Co-authored-by: Cedric Lamoriniere <[email protected]>
DataDog · Jan 5, 2024 · 30c3524 · 30c3524
1 parent b8ceee3
commit 30c3524
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 29 deletions.
diff --git a/pkg/clusteragent/admission/mutate/auto_instrumentation_test.go b/pkg/clusteragent/admission/mutate/auto_instrumentation_test.go
@@ -1574,7 +1574,7 @@ func TestInjectAutoInstrumentation(t *testing.T) {
 			),
 			wantErr: false,
 			setupConfig: func() {
-				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", false)
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
 				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled_namespaces", []string{"ns"})
 			},
 		},

diff --git a/pkg/clusteragent/admission/mutate/common.go b/pkg/clusteragent/admission/mutate/common.go
@@ -12,13 +12,15 @@ import (
 	"fmt"
 
 	"github.com/wI2L/jsondiff"
+	"golang.org/x/exp/slices"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 
 	admCommon "github.com/DataDog/datadog-agent/pkg/clusteragent/admission/common"
 	"github.com/DataDog/datadog-agent/pkg/config"
+	apiServerCommon "github.com/DataDog/datadog-agent/pkg/util/kubernetes/apiserver/common"
 	"github.com/DataDog/datadog-agent/pkg/util/log"
 )
 
@@ -224,49 +226,42 @@ func shouldInject(pod *corev1.Pod) bool {
 }
 
 // isApmInstrumentationEnabled indicates if Single Step Instrumentation is enabled for the namespace in the cluster
-// Single Step Instrumentation is enabled if:
-//   - cluster-wide configuration `apm_config.instrumentation.enabled` is true and the namespace is not excluded via `apm_config.instrumentation.disabled_namespaces`
-//   - cluster-wide configuration `apm_config.instrumentation.enabled` is false and the namespace is included via `apm_config.instrumentation.enabled_namespaces`
 func isApmInstrumentationEnabled(namespace string) bool {
 	apmInstrumentationEnabled := config.Datadog.GetBool("apm_config.instrumentation.enabled")
-	isNsEnabled := apmInstrumentationEnabled
 
-	apmEnabledNamespaces := config.Datadog.GetStringSlice("apm_config.instrumentation.enabled_namespaces")
-	apmDisabledNamespaces := config.Datadog.GetStringSlice("apm_config.instrumentation.disabled_namespaces")
-
-	// If Single Step Instrumentation is enabled in the cluster, enabling it in the specific namespaces is redundant
-	if apmInstrumentationEnabled && len(apmEnabledNamespaces) > 0 {
-		log.Warnf("Redundant configuration option `apm_config.instrumentation.enabled_namespaces:%v`", apmEnabledNamespaces)
+	if !apmInstrumentationEnabled {
+		log.Debugf("APM Instrumentation is disabled")
+		return false
 	}
 
-	// If Single Step Instrumentation is disabled in the cluster, disabling it in the specific namespaces is redundant
-	if !apmInstrumentationEnabled && len(apmDisabledNamespaces) > 0 {
-		log.Warnf("Redundant configuration option `apm_config.instrumentation.disabled_namespaces:%v`", apmDisabledNamespaces)
-	}
+	return filterNamespace(namespace)
+}
+
+// filterNamespace returns a bool indicating if Single Step Instrumentation on the namespace
+func filterNamespace(ns string) bool {
+	apmEnabledNamespaces := config.Datadog.GetStringSlice("apm_config.instrumentation.enabled_namespaces")
+	apmDisabledNamespaces := config.Datadog.GetStringSlice("apm_config.instrumentation.disabled_namespaces")
 
 	// apm.instrumentation.enabled_namespaces and apm.instrumentation.disabled_namespaces configuration cannot be set at the same time
 	if len(apmEnabledNamespaces) > 0 && len(apmDisabledNamespaces) > 0 {
 		log.Errorf("apm.instrumentation.enabled_namespaces and apm.instrumentation.disabled_namespaces configuration cannot be set together")
 		return false
 	}
 
-	// Single Step Instrumentation for the namespace can override cluster-wide configuration
-	for _, ns := range apmEnabledNamespaces {
-		if ns == namespace {
-			return true
-		}
+	// Always disable Single Step Instrumentation in kube-system and Datadog namespaces
+	if (ns == namespaceWithAlwaysDisabledInjections) || (ns == apiServerCommon.GetResourcesNamespace()) {
+		return false
 	}
 
-	// Unless kube-system ns is explicitly enabled, always disable Single Step Instrumentation
-	if namespace == namespaceWithAlwaysDisabledInjections {
-		return false
+	// If apm_config.instrumentation.enabled_namespaces option set, enable Single Step Instrumentation only in listed namespaces
+	if len(apmEnabledNamespaces) > 0 {
+		return slices.Contains[[]string, string](apmEnabledNamespaces, ns)
 	}
 
-	for _, ns := range apmDisabledNamespaces {
-		if ns == namespace {
-			return false
-		}
+	// Disable Single Step Instrumentation in all excluded namespaces
+	if slices.Contains[[]string, string](apmDisabledNamespaces, ns) {
+		return false
 	}
 
-	return isNsEnabled
+	return true
 }
diff --git a/pkg/clusteragent/admission/mutate/common_test.go b/pkg/clusteragent/admission/mutate/common_test.go
@@ -217,6 +217,24 @@ func Test_shouldInject(t *testing.T) {
 		},
 		{
 			name: "instrumentation on with disabled namespace, no label",
+			pod:  fakePodWithNamespaceAndLabel("ns", "", ""),
+			setupConfig: func() {
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
+				mockConfig.SetWithoutSource("apm_config.instrumentation.disabled_namespaces", []string{"ns"})
+			},
+			want: false,
+		},
+		{
+			name: "instrumentation on with disabled namespace, no label",
+			pod:  fakePodWithNamespaceAndLabel("ns", "", ""),
+			setupConfig: func() {
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
+				mockConfig.SetWithoutSource("apm_config.instrumentation.disabled_namespaces", []string{"ns2"})
+			},
+			want: true,
+		},
+		{
+			name: "instrumentation on with disabled namespace, disabled label",
 			pod:  fakePodWithNamespaceAndLabel("ns", "admission.datadoghq.com/enabled", "false"),
 			setupConfig: func() {
 				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
@@ -249,7 +267,7 @@ func Test_shouldInject(t *testing.T) {
 			name: "instrumentation off with enabled namespace, label enabled",
 			pod:  fakePodWithNamespaceAndLabel("ns", "admission.datadoghq.com/enabled", "true"),
 			setupConfig: func() {
-				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", false)
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
 				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled_namespaces", []string{"ns"})
 			},
 			want: true,
@@ -270,8 +288,43 @@ func Test_shouldInject(t *testing.T) {
 				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", false)
 				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled_namespaces", []string{"ns"})
 			},
+			want: false,
+		},
+		{
+			name: "instrumentation on with enabled namespace, no label",
+			pod:  fakePodWithNamespaceAndLabel("ns", "", ""),
+			setupConfig: func() {
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled_namespaces", []string{"ns"})
+			},
 			want: true,
 		},
+		{
+			name: "instrumentation on with enabled other namespace, no label",
+			pod:  fakePodWithNamespaceAndLabel("ns", "", ""),
+			setupConfig: func() {
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled_namespaces", []string{"n2s"})
+			},
+			want: false,
+		},
+		{
+			name: "instrumentation on in kube-system namespace, no label",
+			pod:  fakePodWithNamespaceAndLabel("kube-system", "", ""),
+			setupConfig: func() {
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
+			},
+			want: false,
+		},
+		{
+			name: "instrumentation on in default (datadog) namespace, no label",
+			pod:  fakePodWithNamespaceAndLabel("default", "", ""),
+			setupConfig: func() {
+				mockConfig.SetWithoutSource("apm_config.instrumentation.enabled", true)
+				mockConfig.SetWithoutSource("kube_resources_namespace", "default")
+			},
+			want: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {