Read and propagate helm config for security products #23575

robertpi · 2024-03-08T12:43:24Z

What does this PR do?

This PR is indent to allow ASM, IAST and SCA to be enabled (or disabled) by Helm Charts config on the controller pod. The cluster-agent, running on the controller pod, will read some environment variables and use this to mutate the configuration of other pod to set the environment variables that will activate products within the client libraries.

Motivation

This will make it easier for k8s clients (at least those using Helm Charts) to activate our products. Simplified installation is a common request.

Additional Notes

The PR will is designed to work with this PR: DataDog/helm-charts#1337

Each product will have env var with the following format:

DD_product_ENABLED_PROPAGATE - overall enable disable switch
DD_product_ENABLED_PROPAGATE_NAMESPACES - filter for namespaces where the products should enabled
DD_product_DISABLED_PROPAGATE_NAMESPACES - filter for namespaces where the products shouldn't enabled

It will result in the a variable with the following format be propagated to all pods (or those conforming to the filters):

DD_product_ENABLED

Possible Drawbacks / Trade-offs

Adds more complexity to our config handling.

Describe how to test/QA your changes

Unit tests have been added
Manual end to end testing has been preformed

Intended to work with SSI, but could be useful on it's own. Allows users specify that the security products should be activated and the controller pod will propagate the necessary env var to the other pods.

bits-bot · 2024-03-08T12:43:29Z

All committers have signed the CLA.

adel121 · 2024-03-08T13:50:49Z

pkg/config/setup/apm.go

+	config.BindEnv("sca_config.enabled", "DD_SCA_ENABLED_PROPAGATE")
+	config.BindEnvAndSetDefault("sca_config.enabled_namespaces", []string{}, "DD_SCA_ENABLED_PROPAGATE_NAMESPACES")
+	config.BindEnvAndSetDefault("sca_config.disabled_namespaces", []string{}, "DD_SCA_DISABLED_PROPAGATE_NAMESPACES")
+


❓

I wonder why you placed these configurations here undeapm.go
AFAIK, these three features are not related to APM, should these configs be places under pkg/config/setup/config.go ?

adel121 · 2024-03-08T13:52:33Z

pkg/config/setup/apm.go

+	config.BindEnv("sca_config.enabled", "DD_SCA_ENABLED_PROPAGATE")
+	config.BindEnvAndSetDefault("sca_config.enabled_namespaces", []string{}, "DD_SCA_ENABLED_PROPAGATE_NAMESPACES")
+	config.BindEnvAndSetDefault("sca_config.disabled_namespaces", []string{}, "DD_SCA_DISABLED_PROPAGATE_NAMESPACES")
+


I don't think you need to use asm_config.*, asm.* should be enough. (same for all the others)

adel121 · 2024-03-08T13:56:27Z

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

@@ -141,17 +141,39 @@ type Webhook struct {
 	endpoint          string
 	resources         []string
 	operations        []admiv1.OperationType
-	filter            *containers.Filter
+	filters           map[string]*containers.Filter


I don't think this is a good idea.

This leads to coupling within the same webhook multiple features that are not actually related.

We should probably have one webhook per feature (or a webhook for a set of features only if it makes sense to, which I assume is a very rare situation)

adel121 · 2024-03-08T14:04:12Z

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

+	}
+	filters["apm_config.instrumentation.enabled"] = ssiFilter;
+
+	asmFilter, err := apmProductNamespaceFilter("asm_config.enabled_namespaces", "asm_config.disabled_namespaces");


ASM is not related to APM afaik (correct me if I am wrong).

Given this, I don't think the naming is good here (i.e. apmProductNamespaceFilter(...) is returning an asm filter)

Same applies to the following lines.

adel121 · 2024-03-08T14:04:20Z

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

 	if err != nil {
 		return nil, err
 	}
+	filters["asm_config.enabled"] = asmFilter;
+
+	iastFilter, err := apmProductNamespaceFilter("iast_config.enabled_namespaces", "iast_config.disabled_namespaces");


same comment as above

adel121 · 2024-03-08T14:04:30Z

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

+	}
+	filters["iast_config.enabled"] = iastFilter;
+
+	scaFilter, err := apmProductNamespaceFilter("sca_config.enabled_namespaces", "sca_config.disabled_namespaces");


same comment as above

adel121 · 2024-03-08T14:07:36Z

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

+	if err != nil {
+		return nil, err
+	}
+	filters["iast_config.enabled"] = iastFilter;


I don't think it is good to have the absolute config name as a key in the filters map. (same applies to the rest).

Using filters["iast"] should be enough, and would be better to create some constant like const iast = "iast" and use it wherever needed; this helps avoid runtime errors in case of typo in the key when used as a plain string.

But despite this:

It is not good to have all the features in the same webhook in the first place

pr-commenter · 2024-03-08T14:26:22Z

Bloop Bleep... Dogbot Here

Regression Detector Results

Run ID: 238dac63-6905-4342-84d5-7b4298dfac47
Baseline: c7b1b20
Comparison: 2244379

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

Experiments with missing or malformed data

basic_py_check

Usually, this warning means that there is no usable optimization goal data for that experiment, which could be a result of misconfiguration.

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	-0.66	[-7.18, +5.87]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+2.01	[+0.57, +3.46]
➖	file_tree	memory utilization	+1.75	[+1.65, +1.84]
➖	process_agent_real_time_mode	memory utilization	+0.69	[+0.64, +0.73]
➖	process_agent_standard_check	memory utilization	+0.60	[+0.56, +0.64]
➖	process_agent_standard_check_with_stats	memory utilization	+0.36	[+0.32, +0.40]
➖	idle	memory utilization	+0.04	[-0.00, +0.09]
➖	trace_agent_json	ingress throughput	+0.04	[+0.01, +0.07]
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.00, +0.00]
➖	uds_dogstatsd_to_api	ingress throughput	+0.00	[-0.00, +0.00]
➖	trace_agent_msgpack	ingress throughput	-0.01	[-0.02, -0.00]
➖	tcp_syslog_to_blackhole	ingress throughput	-0.42	[-0.47, -0.36]
➖	file_to_blackhole	% cpu utilization	-0.66	[-7.18, +5.87]
➖	otel_to_otel_logs	ingress throughput	-0.67	[-1.33, -0.01]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

eliottness

Couple of go nitpicks but thanks @robertpi 💯

eliottness · 2024-03-08T15:22:46Z

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

+	ssiFilter, err := apmSSINamespaceFilter();
+	if err != nil {
+		return nil, err
+	}
+	filters["apm_config.instrumentation.enabled"] = ssiFilter;
+
+	asmFilter, err := apmProductNamespaceFilter("asm_config.enabled_namespaces", "asm_config.disabled_namespaces");
 	if err != nil {
 		return nil, err
 	}
+	filters["asm_config.enabled"] = asmFilter;
+
+	iastFilter, err := apmProductNamespaceFilter("iast_config.enabled_namespaces", "iast_config.disabled_namespaces");
+	if err != nil {
+		return nil, err
+	}
+	filters["iast_config.enabled"] = iastFilter;
+
+	scaFilter, err := apmProductNamespaceFilter("sca_config.enabled_namespaces", "sca_config.disabled_namespaces");
+	if err != nil {
+		return nil, err
+	}
+	filters["sca_config.enabled"] = scaFilter;


Suggested change

ssiFilter, err := apmSSINamespaceFilter();

if err != nil {

return nil, err

}

filters["apm_config.instrumentation.enabled"] = ssiFilter;

asmFilter, err := apmProductNamespaceFilter("asm_config.enabled_namespaces", "asm_config.disabled_namespaces");

if err != nil {

return nil, err

}

filters["asm_config.enabled"] = asmFilter;

iastFilter, err := apmProductNamespaceFilter("iast_config.enabled_namespaces", "iast_config.disabled_namespaces");

if err != nil {

return nil, err

}

filters["iast_config.enabled"] = iastFilter;

scaFilter, err := apmProductNamespaceFilter("sca_config.enabled_namespaces", "sca_config.disabled_namespaces");

if err != nil {

return nil, err

}

filters["sca_config.enabled"] = scaFilter;

var err error

filters["apm_config.instrumentation.enabled"], err = apmSSINamespaceFilter()

if err != nil {

return nil, err

}

filters["asm_config.enabled"], err = apmProductNamespaceFilter("asm_config.enabled_namespaces", "asm_config.disabled_namespaces")

if err != nil {

return nil, err

}

filters["iast_config.enabled"], err = apmProductNamespaceFilter("iast_config.enabled_namespaces", "iast_config.disabled_namespaces")

if err != nil {

return nil, err

}

filters["sca_config.enabled"], err = apmProductNamespaceFilter("sca_config.enabled_namespaces", "sca_config.disabled_namespaces")

if err != nil {

return nil, err

}

eliottness · 2024-03-08T15:23:56Z

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

+func (w *Webhook) isEnabledInNamespace(enabledConfig string, namespace string) bool {
+	enabledValue := config.Datadog.GetString(enabledConfig)
+
+	if len(enabledValue) == 0 {


Suggested change

if len(enabledValue) == 0 {

if enabledValue == "" {

eliottness · 2024-03-08T15:24:47Z

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

+	if (w.isEnabledInNamespace(enableKey, pod.Namespace)) {
+		enabledValue := config.Datadog.GetBool(enableKey)
+		_ = mutatecommon.InjectEnv(pod, corev1.EnvVar{
+				Name:	envVarKey,
+				Value: 	strconv.FormatBool(enabledValue),
+			});
+	}
+	return nil;


Suggested change

if (w.isEnabledInNamespace(enableKey, pod.Namespace)) {

enabledValue := config.Datadog.GetBool(enableKey)

_ = mutatecommon.InjectEnv(pod, corev1.EnvVar{

Name: envVarKey,

Value: strconv.FormatBool(enabledValue),

});

}

return nil;

if w.isEnabledInNamespace(enableKey, pod.Namespace) {

enabledValue := config.Datadog.GetBool(enableKey)

mutatecommon.InjectEnv(pod, corev1.EnvVar{

Name: envVarKey,

Value: strconv.FormatBool(enabledValue),

})

}

return nil

eliottness · 2024-03-08T15:25:03Z

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

@@ -229,6 +252,11 @@ func apmSSINamespaceFilter() (*containers.Filter, error) {
 	return containers.NewFilter(containers.GlobalFilter, apmEnabledNamespacesWithPrefix, filterExcludeList)
 }

+// apmSSINamespaceFilter returns the filter used by APM SSI to filter namespaces.
+func apmSSINamespaceFilter() (*containers.Filter, error) {
+	return apmProductNamespaceFilter("apm_config.instrumentation.enabled_namespaces", "apm_config.instrumentation.disabled_namespaces");


Suggested change

return apmProductNamespaceFilter("apm_config.instrumentation.enabled_namespaces", "apm_config.instrumentation.disabled_namespaces");

return apmProductNamespaceFilter("apm_config.instrumentation.enabled_namespaces", "apm_config.instrumentation.disabled_namespaces")

eliottness

Woops, missclicked on the approve button

robertpi · 2024-03-11T12:56:17Z

closing in favour of the simpler #23618

Read and propagrate helm config for security products

2244379

Intended to work with SSI, but could be useful on it's own. Allows users specify that the security products should be activated and the controller pod will propagate the necessary env var to the other pods.

adel121 reviewed Mar 8, 2024

View reviewed changes

eliottness approved these changes Mar 8, 2024

View reviewed changes

eliottness reviewed Mar 8, 2024

View reviewed changes

eliottness self-requested a review March 8, 2024 15:29

eliottness removed their request for review March 13, 2024 09:31

dd-devflow bot closed this Sep 7, 2024

dd-devflow bot deleted the robert/helm-chart-integration branch September 7, 2024 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read and propagate helm config for security products #23575

Read and propagate helm config for security products #23575

robertpi commented Mar 8, 2024 •

edited

Loading

bits-bot commented Mar 8, 2024 •

edited

Loading

adel121 Mar 8, 2024

adel121 Mar 8, 2024

adel121 Mar 8, 2024

adel121 Mar 8, 2024

adel121 Mar 8, 2024

adel121 Mar 8, 2024

adel121 Mar 8, 2024

pr-commenter bot commented Mar 8, 2024

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

eliottness left a comment •

edited

Loading

eliottness Mar 8, 2024

eliottness Mar 8, 2024

eliottness Mar 8, 2024

eliottness Mar 8, 2024

eliottness left a comment •

edited

Loading

robertpi commented Mar 11, 2024

	return apmProductNamespaceFilter("apm_config.instrumentation.enabled_namespaces", "apm_config.instrumentation.disabled_namespaces");
	return apmProductNamespaceFilter("apm_config.instrumentation.enabled_namespaces", "apm_config.instrumentation.disabled_namespaces")

Read and propagate helm config for security products #23575

Read and propagate helm config for security products #23575

Conversation

robertpi commented Mar 8, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

bits-bot commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr-commenter bot commented Mar 8, 2024

Bloop Bleep... Dogbot Here

Regression Detector Results

Experiments with missing or malformed data

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

eliottness left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliottness left a comment • edited Loading

Choose a reason for hiding this comment

robertpi commented Mar 11, 2024

robertpi commented Mar 8, 2024 •

edited

Loading

bits-bot commented Mar 8, 2024 •

edited

Loading

eliottness left a comment •

edited

Loading

eliottness left a comment •

edited

Loading