[system-probe] Start of dynamic instrumentation system probe module #16034

grantseltzer · 2023-03-09T16:17:37Z

What does this PR do?

This introduces the scaffolding for the new dynamic instrumentation system-probe module. This will be a module used by the dynamic-instrumentation product for system level tracing of user applications. This belongs in the system-probe as it will soon need higher level permissions (CAP_SYS_ADMIN, CAP_BPF) for loading and attaching bpf programs, and inspecting inside container filesystems.

Motivation

This PR simply creates the module in code, and gives it the scaffolding for reading a static configuration file. The motivation for this is to break up the large amount of code that will need to be placed here into smaller PRs.

Additional Notes

This also is specified via go build tags to only build on Linux which is required for the bpf features it will consume.
This is opened after realizing [system-probe] Start of dynamic instrumentation system probe module #16022 was from a branch on my fork of the repo.

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Build the system probe: inv system-probe.build
Run the system-probe sudo bin/system-probe/system-probe

You should see it's disabled:

2023-03-08 13:37:33 PST | SYS-PROBE | INFO | (cmd/system-probe/api/module/loader.go:50 in Register) | module dynamic_instrumentation disabled
2023-03-08 13:37:33 PST | SYS-PROBE | INFO | (cmd/system-probe/app/run.go:123 in run) | system probe successfully started

You can run it enabled by setting DD_DYNAMIC_INSTRUMENTATION_ENABLED=1 or adding the following section to your system-probe config:

####################################
## Dynamic Instrumentation Config ##
###################################

dynamic_instrumentation:
  ## @param enabled - boolean - optional - default: false
  ## @env DD_DYNAMIC_INSTRUMENTATION_ENABLED - boolean -optional - default: false
  ## Set to true to enable the User Tracer Module of the System Probe
  #
  enabled: true

Reviewer's Checklist

sgnn7

Few nits. I'll preemptively approve since they're easy fixes.

sgnn7 · 2023-03-09T16:20:31Z

.github/CODEOWNERS

@@ -247,6 +247,7 @@
 /pkg/config/remote/service/meta/        @DataDog/remote-config @DataDog/software-integrity-and-trust
 /pkg/diagnose/                          @Datadog/container-integrations
 /pkg/diagnose/connectivity/             @DataDog/agent-shared-components
+/pkg/dynamicinstrumentation/           @DataDog/debugger


Offset in column 2

sgnn7 · 2023-03-09T16:22:41Z

pkg/dynamicinstrumentation/module.go

+
+type Module struct{}
+
+func NewModule(config *Config) (*Module, error) {


Linter errors are right - you have an exclusion flag in the other file but no build flag here so for non-Linux platforms both files will be built, causing name collision. I would probably suggest that you maybe name this file module_linux just to make it clear from that standpoint that this only works on that platform.

sgnn7 · 2023-03-09T16:23:07Z

pkg/dynamicinstrumentation/module_unsupported.go

+import (
+	"github.com/DataDog/datadog-agent/cmd/system-probe/api/module"
+	"github.com/DataDog/datadog-agent/pkg/ebpf"
+	"github.com/DataDog/datadog-agent/pkg/security/config"


Linter is correct here - you're not using "github.com/DataDog/datadog-agent/pkg/security/config" package in windows impl

guyarb · 2023-03-09T17:08:02Z

pkg/config/system_probe.go

@@ -131,6 +133,9 @@ func InitSystemProbeConfig(cfg Config) {
 	cfg.BindEnvAndSetDefault(join(spNS, "zypper_repos_dir"), suffixHostEtc(defaultZypperReposDirSuffix), "DD_ZYPPER_REPOS_DIR")
 	cfg.BindEnvAndSetDefault(join(spNS, "attach_kprobes_with_kprobe_events_abi"), false, "DD_ATTACH_KPROBES_WITH_KPROBE_EVENTS_ABI")

+	// User Tracer
+	cfg.BindEnvAndSetDefault(join(diNS, "enabled"), false, "DD_DYNAMIC_INSTRUMENTATION_ENABLED")


it is recommended to add the feature to:

pkg/metadata/inventories/README.md

pkg/metadata/inventories/inventories.go

and to add UTs for the configuration:

pkg/network/config/config_test.go

adding an example PR from my team #14620

The metadata/inventories package seems to have specific features rather than having the whole module enabled. For example USM just has different network protocols it supports, rather than USM as a whole. Are you sure it makes sense to have something like feature_dynamic_instrumentation_enabled? I don't know what inventories is so I really don't know. I can certainly add it and then add specific features as they get added.

Similarly, dynamic instrumentation might not belong under pkg/network/config since it's not a network feature.

usm is not the best example (unforutnately, as we started as a feature in npm and evolved)
but take a look in npm https://github.com/DataDog/datadog-agent/blob/main/pkg/metadata/inventories/inventories.go#L418
or CWS https://github.com/DataDog/datadog-agent/blob/main/pkg/metadata/inventories/inventories.go#L415

regarding the configuration test, you don't need to add them in pkg/network/config/config_test.go but a similar to the correct config_test.go file
another example in other package https://github.com/DataDog/datadog-agent/blob/main/pkg/config/config_test.go

Sounds good - I've added to the metadata/inventories package and added a test to cmd/system-probe/config/config_linux_test.go

cmd/system-probe/modules/dynamic_instrumentation.go

pkg/dynamicinstrumentation/config.go

pkg/dynamicinstrumentation/module_unsupported.go

brycekahle

Minor suggestions, but nothing blocking.

grantseltzer · 2023-03-28T14:01:14Z

@brycekahle Is there anything else I can do to help this get merged?

github-actions · 2023-03-28T17:24:26Z

⚠️🚨 Warning, this pull request increases the binary size of serverless extension by 4096 bytes. Each MB of binary size increase means about 10ms of additional cold start time, so this pull request would increase cold start time by 0ms.

New dependencies added

We suggest you consider adding the !serverless build tag to remove any new dependencies not needed in the serverless extension.

If you have questions, we are happy to help, come visit us in the #serverless slack channel and provide a link to this comment.

guyarb · 2023-03-29T06:38:09Z

@grantseltzer seems like you never logged in into CircleCI, therefore the relevant jobs didn't run, and those jobs are required for merging

pkg/dynamicinstrumentation/config.go

Signed-off-by: grantseltzer <[email protected]>

…on other platforms Signed-off-by: grantseltzer <[email protected]>

Signed-off-by: grantseltzer <[email protected]>

…r now) Signed-off-by: grantseltzer <[email protected]>

Signed-off-by: grantseltzer <[email protected]>

grantseltzer requested review from a team as code owners March 9, 2023 16:17

grantseltzer mentioned this pull request Mar 9, 2023

[system-probe] Start of dynamic instrumentation system probe module #16022

Closed

10 tasks

sgnn7 approved these changes Mar 9, 2023

View reviewed changes

guyarb reviewed Mar 9, 2023

View reviewed changes

brycekahle reviewed Mar 9, 2023

View reviewed changes

cmd/system-probe/modules/dynamic_instrumentation.go Outdated Show resolved Hide resolved

brycekahle reviewed Mar 9, 2023

View reviewed changes

cmd/system-probe/modules/dynamic_instrumentation.go Outdated Show resolved Hide resolved

brycekahle reviewed Mar 9, 2023

View reviewed changes

pkg/dynamicinstrumentation/config.go Outdated Show resolved Hide resolved

brycekahle reviewed Mar 9, 2023

View reviewed changes

pkg/dynamicinstrumentation/module_unsupported.go Outdated Show resolved Hide resolved

brycekahle approved these changes Mar 9, 2023

View reviewed changes

grantseltzer added [deprecated] qa/skip-qa - use other qa/ labels [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card changelog/no-changelog labels Mar 9, 2023

grantseltzer added this to the 7.45.0 milestone Mar 9, 2023

grantseltzer force-pushed the grantseltzer/user-tracer-scaffolding branch from dc3668d to d8adf12 Compare March 28, 2023 17:20

grantseltzer force-pushed the grantseltzer/user-tracer-scaffolding branch from d8adf12 to 838ec17 Compare March 29, 2023 14:24

guyarb reviewed Mar 29, 2023

View reviewed changes

pkg/dynamicinstrumentation/config.go Show resolved Hide resolved

grantseltzer added the team/dynamic-instrumentation Dynamic Instrumentation label Mar 29, 2023

grantseltzer force-pushed the grantseltzer/user-tracer-scaffolding branch 3 times, most recently from 60d736f to e889753 Compare April 7, 2023 13:40

grantseltzer added 7 commits April 11, 2023 07:55

Start of dynamic instrumentation system probe module

41eb42c

Signed-off-by: grantseltzer <[email protected]>

Update CODEOWNERS to assign debugger team to pkg/dynamic-instrumentation

029b714

Signed-off-by: grantseltzer <[email protected]>

Rename pkg/dynamic-instrumentation to pkg/dynamicinstrumentation

4a45276

Signed-off-by: grantseltzer <[email protected]>

Disable the dynamic instrumentation module by default

c860795

Signed-off-by: grantseltzer <[email protected]>

Fix buildtag issue where the linux module file wasn't being excluded …

d339680

…on other platforms Signed-off-by: grantseltzer <[email protected]>

Remove unneeded file, fix build tags, update error strings/logs

0cf43c9

Signed-off-by: grantseltzer <[email protected]>

Add a test for dynamic instrumentation config fields (just enabled fo…

06df7c1

…r now) Signed-off-by: grantseltzer <[email protected]>

grantseltzer added 2 commits April 11, 2023 07:55

Add copyright statement to config.go

efd4b98

Signed-off-by: grantseltzer <[email protected]>

Fix codeowners spelling

fd236e4

Signed-off-by: grantseltzer <[email protected]>

grantseltzer force-pushed the grantseltzer/user-tracer-scaffolding branch from e889753 to fd236e4 Compare April 11, 2023 14:55

grantseltzer merged commit fd191ab into main Apr 14, 2023

grantseltzer deleted the grantseltzer/user-tracer-scaffolding branch April 14, 2023 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[system-probe] Start of dynamic instrumentation system probe module #16034

[system-probe] Start of dynamic instrumentation system probe module #16034

grantseltzer commented Mar 9, 2023

sgnn7 left a comment

sgnn7 Mar 9, 2023

sgnn7 Mar 9, 2023

sgnn7 Mar 9, 2023

guyarb Mar 9, 2023

grantseltzer Mar 9, 2023 •

edited

Loading

guyarb Mar 10, 2023

guyarb Mar 10, 2023

grantseltzer Mar 10, 2023

brycekahle left a comment

grantseltzer commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

guyarb commented Mar 29, 2023


		type Module struct{}

		func NewModule(config Config) (Module, error) {

[system-probe] Start of dynamic instrumentation system probe module #16034

[system-probe] Start of dynamic instrumentation system probe module #16034

Conversation

grantseltzer commented Mar 9, 2023

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

sgnn7 left a comment

Choose a reason for hiding this comment

sgnn7 Mar 9, 2023

Choose a reason for hiding this comment

sgnn7 Mar 9, 2023

Choose a reason for hiding this comment

sgnn7 Mar 9, 2023

Choose a reason for hiding this comment

guyarb Mar 9, 2023

Choose a reason for hiding this comment

grantseltzer Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

guyarb Mar 10, 2023

Choose a reason for hiding this comment

guyarb Mar 10, 2023

Choose a reason for hiding this comment

grantseltzer Mar 10, 2023

Choose a reason for hiding this comment

brycekahle left a comment

Choose a reason for hiding this comment

grantseltzer commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

guyarb commented Mar 29, 2023

grantseltzer Mar 9, 2023 •

edited

Loading