-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APM Onboarding] Disable APM Instrumentation on Datadog namespace #1252
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be sure to document in the public manual instructions when not using the helm chart/operator to exclude the Agent namespace as well. I don't think we have any public docs on the setting yet so this would be future work.
- name: DD_APM_INSTRUMENTATION_ENABLED | ||
value: "true" | ||
{{- end }} | ||
{{- if .Values.datadog.apm.instrumentation.enabledNamespaces }} | ||
- name: DD_APM_INSTRUMENTATION_ENABLED_NAMESPACES | ||
value: {{ .Values.datadog.apm.instrumentation.enabledNamespaces | toJson | quote }} | ||
{{- end }} | ||
{{- if .Values.datadog.apm.instrumentation.disabledNamespaces }} | ||
{{- if ne (include "apmInstrumentation.disabledNamespaces" .) "" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we default apmInstrumentation.disabledNamespaces
to an empty list when not defined to simplify this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apmInstrumentation.disabledNamespaces
is a string; it's "" by default.
https://github.com/DataDog/helm-charts/blob/main/charts/datadog/templates/_helpers.tpl#L618-L631 has similar pattern and usage https://github.com/DataDog/helm-charts/blob/main/charts/datadog/templates/_containers-common-env.yaml#L27-L30
{{- if .Values.datadog.apm.instrumentation.enabled }} | ||
- name: DD_APM_INSTRUMENTATION_ENABLED | ||
value: "true" | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the {{- if .Values.datadog.apm.instrumentation.enabled }}
doesn't inclose the other apmInstrumentation options?
We usually use this patern (have a global setting like .Values.datadog.apm.instrumentation.enabled
to enable or disable a full feature). And I suppose it doesn't make sense to not set DD_APM_INSTRUMENTATION_ENABLED but we still inject the DD_APM_INSTRUMENTATION_DISABLED_NAMESPACES
or DD_APM_INSTRUMENTATION_ENABLED_NAMESPACES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per RFC:
To allow more granular control of the injection process, Single Step Instrumentation introduces a set of new configuration parameters on the Cluster Agent:
datadog.apm.instrumentation.enabled
- boolean indicating if automatic injection should be enabled in the whole cluster. Default value - false.
datadog.apm.instrumentation.enabledNamespaces
- comma-separated list of namespaces where automatic injection is enabled. Default value - empty list. This setting applies only when cluster-level configuration datadog.apm.instrumentation.enabled is set to false.
datadog.apm.instrumentation.disabledNamespaces
- comma-separated list of namespaces where automatic injection needs to be disabled. Applies only when datadog.apm.instrumentation.enabled is set to true. Default value - empty list. Single Step Instrumentation will always skip injections in Kubernetes system namespace kube-system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This question was raised in Operator PR as well here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @liliyadd
I think the behavior should be changed, because all the other features in the agent don't use the same pattern, and IMO the current behavior is error-prone.
What I think can make more sense is:
datadog.apm.instrumentation.enabled
- boolean indicating if automatic injection should be enableddatadog.apm.instrumentation.enabledNamespaces
- comma-separated list of namespaces where automatic injection is enabled. (Default value - empty string). If value is empty, automatic injection is enabled on the whole cluster*. (*exceptkube-systems
, and the "agent install" namespace).datadog.apm.instrumentation.disabledNamespaces
- comma-separated list of namespaces where automatic injection needs to be disabled. (Default value - empty string). IfdisabledNamespaces
automatic injection is active on every namespace except the one present indisabledNamespaces
andkube-systems
, and the "agent install" namespace.
I think this change can still be done, and make more sense for cross agent configuration genericity.
{{/* | ||
Return all namespaces with disabled Single Step Instrumentation | ||
*/}} | ||
{{- define "apmInstrumentation.disabledNamespaces" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General behaviour question what happens if disabledNamespaces
is never empty?
Based on the other PR #1211 and specifically this part of the code https://github.com/DataDog/helm-charts/pull/1211/files#diff-5dec0964fafbdc987c2b122b6f38a16f03c7129501b7cb14fd5bbe37afb2cd0dR168
I think the user will not be able to set the enabledNamespaces
option.
Is it the expected behaviour? should have a precedence between the option? do we have a reason to not allow both options at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason not to allow disabledNamespaces
and enabledNamespaces
set at the same time is to prevent unspecified behavior when a namespace is present in both lists.
disabledNamespaces
shouldn't be empty only when SSI is on for the cluster and we just want disable it in the few namespaces.
If users set enabledNamespaces, generally it means that SSI is disable in the whole cluster.
Test cases:
Result env variables on cluster-agent:
Result env variables on cluster-agent:
Result env variables on cluster-agent:
Result env variables on cluster-agent:
|
Looks good to me. I tried various combinations of |
What this PR does / why we need it:
This PR adds the namespace where the datadog-agent is deployed to the exclude-list of APM Single Step Instrumentation. It's important to exclude cluster-agent deployment and agent daemonset by default to make sure sure that cluster-agent pods creation is not blocked if the admission-controller is not available (if cluster-agent pods are not ready/available).
Special notes for your reviewer:
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]
.github/helm-docs.sh
)CHANGELOG.md
has been updatedREADME.md
make update-test-baselines
)