Skip to content
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

Add DD_AGENT_IPC_* env vars #1604

Merged
merged 3 commits into from
Feb 6, 2025
Merged

Add DD_AGENT_IPC_* env vars #1604

merged 3 commits into from
Feb 6, 2025

Conversation

mackjmr
Copy link
Member

@mackjmr mackjmr commented Jan 7, 2025

What does this PR do?

This PR adds DD_AGENT_IPC_PORT and DD_AGENT_IPC_CONFIG_REFRESH_INTERVAL env vars in otel agent and core agent. It does not override them if they are added by the user. This is necessary for the otel-agent to pull the api key from core config in the case where backend secrets are used.

Motivation

OTEL-2323

Additional Notes

I have an open question about test, see PR comment.

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: vX.Y.Z
  • Cluster Agent: vX.Y.Z

Describe your test plan

Run the otel-agent without API Key in otel collector config, and by providing api key in core agent via backend secret.

Setup 1:

kubectl create secret generic datadog-secret-deux --from-literal api_key=xxx

operator config:

  global:
    secretBackend:
      command: /readsecret_multiple_providers.sh
      enableGlobalPermissions: true
    kubelet:
      tlsVerify: false
    credentials:
      apiKey: ENC[k8s_secret@system/datadog-secret-deux/api_key]

Validate config can be fetched by the otel agent by checking otel-agent logs contain:

2025-01-07 12:57:02 UTC | OTELCOL | INFO | (/usr/local/go/src/reflect/value.go:581 in call) | configsync enabled (agent_ipc 'localhost:5009' | agent_ipc.config_refresh_interval: 60)
2025-01-07 12:57:02 UTC | OTELCOL | INFO | (comp/core/configsync/configsyncimpl/module.go:68 in newComponent) | triggering configsync on init (will retry for 30s)
2025-01-07 12:57:04 UTC | OTELCOL | INFO | (comp/core/configsync/configsyncimpl/module.go:100 in newConfigSync) | Succeeded to fetch config from core agent

Setup 2:

Same as above but user provided env vars, make sure these are not overriden.

Operator config:

spec:
  override:
    nodeAgent:
      containers: 
        agent:
          env:
            - name: DD_AGENT_IPC_PORT
              value: "3333"
            - name: DD_AGENT_IPC_CONFIG_REFRESH_INTERVAL
              value: "21"
        otel-agent:
          env:
            - name: DD_AGENT_IPC_PORT
              value: "3333"
            - name: DD_AGENT_IPC_CONFIG_REFRESH_INTERVAL
              value: "21"

Validate config can be fetched by the otel agent by checking otel-agent logs contain:

2025-01-07 12:57:02 UTC | OTELCOL | INFO | (/usr/local/go/src/reflect/value.go:581 in call) | configsync enabled (agent_ipc 'localhost:3333' | agent_ipc.config_refresh_interval: 21)
2025-01-07 12:57:02 UTC | OTELCOL | INFO | (comp/core/configsync/configsyncimpl/module.go:68 in newComponent) | triggering configsync on init (will retry for 30s)
2025-01-07 12:57:04 UTC | OTELCOL | INFO | (comp/core/configsync/configsyncimpl/module.go:100 in newConfigSync) | Succeeded to fetch config from core agent

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

This PR adds DD_AGENT_IPC_PORT and DD_AGENT_IPC_CONFIG_REFRESH_INTERVAL env vars in otel agent and core agent. It does not override them if they are added by the user. THis is necessary for the otel-agent to pull the api key from core config in the case where backend secrets are used.
@mackjmr mackjmr added the enhancement New feature or request label Jan 7, 2025
Value: "60",
}
// don't set env var if it was already set by user.
mergeFunc := func(current, newEnv *corev1.EnvVar) (*corev1.EnvVar, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be tested by the current test framework, as the env vars added via WithEnvVars helper don't make it into container env vars.

e.g.

		{
			Name: "otel agent user provided ipc port",
			DDA: testutils.NewDatadogAgentBuilder().
				WithOTelCollectorEnabled(true).
				WithEnvVars([]corev1.EnvVar{
					{
						Name:  v2alpha1.DDAgentIpcPort,
						Value: "3333",
					},
				}).
				Build(),
			WantConfigure:        true,
			WantDependenciesFunc: testExpectedDepsCreatedCM,
			Agent: testExpectedAgent(apicommon.OtelAgent, defaultExpectedPorts, defaultLocalObjectReferenceName,
				expectedEnvVars{
					agent_ipc_port: expectedEnvVar{
						present: true,
						value:   "3333",
					},
					agent_ipc_refresh: expectedEnvVar{
						present: true,
						value:   "60",
					},
				},
			),
		},

will fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithEnvVars adds env variables to global handled in

if config.Env != nil {
for _, envVar := range config.Env {
manager.EnvVar().AddEnvVar(&envVar)
}
}
which isn't executed by feature unit tests.

controlelr_v2_test.go could be used for this if you want to test outcome of whole reconcile.

@mackjmr mackjmr marked this pull request as ready for review January 7, 2025 13:26
@mackjmr mackjmr requested a review from a team as a code owner January 7, 2025 13:26
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 49.29%. Comparing base (8b68405) to head (23979c3).

Files with missing lines Patch % Lines
...ller/datadogagent/feature/otelcollector/feature.go 81.25% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1604      +/-   ##
==========================================
+ Coverage   49.26%   49.29%   +0.02%     
==========================================
  Files         218      218              
  Lines       21073    21089      +16     
==========================================
+ Hits        10382    10395      +13     
- Misses      10150    10152       +2     
- Partials      541      542       +1     
Flag Coverage Δ
unittests 49.29% <81.25%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ller/datadogagent/feature/otelcollector/feature.go 86.03% <81.25%> (-0.47%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b68405...23979c3. Read the comment docs.

@mackjmr mackjmr added this to the v1.12.0 milestone Jan 7, 2025
@tbavelier tbavelier modified the milestones: v1.12.0, v1.13.0 Jan 8, 2025
}
// don't set env var if it was already set by user.
mergeFunc := func(current, newEnv *corev1.EnvVar) (*corev1.EnvVar, error) {
return current, nil
Copy link
Contributor

@levan-m levan-m Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to add the two env vars if current doesn't contain them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding based on code is that the mergeFunc is only run if the container already contains the env var we are trying to add. In which case, I want to prioritize the one already set, which is why I return current.

If the env var is not present in the container, then the merge func is not run, and the env vars are added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's right. Thanks for clarifying.

@mackjmr mackjmr merged commit 1b3bf05 into main Feb 6, 2025
19 checks passed
@mackjmr mackjmr deleted the mackjmr/add-ipc-env-vars branch February 6, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants