From 9df73e5facabc218328f04116dd0e2196e60ba8e Mon Sep 17 00:00:00 2001 From: Bryce Kahle Date: Fri, 15 Nov 2024 13:14:23 -0800 Subject: [PATCH] Use config for windows named pipe path --- .github/CODEOWNERS | 1 - .../subcommands/flare/command_other_test.go | 2 ++ cmd/agent/subcommands/flare/command_test.go | 20 ++++++++-------- .../subcommands/flare/command_windows_test.go | 14 ++--------- cmd/system-probe/api/client/client_windows.go | 11 +++------ cmd/system-probe/config/adjust.go | 3 ++- cmd/system-probe/config/config_unix.go | 3 --- cmd/system-probe/config/config_unsupported.go | 9 ++++---- cmd/system-probe/config/config_windows.go | 9 +++----- .../wincrashdetect_windows_test.go | 12 ++-------- pkg/config/setup/config_darwin.go | 4 ++-- pkg/config/setup/config_nix.go | 4 ++-- pkg/config/setup/config_windows.go | 4 ++-- pkg/config/setup/system_probe.go | 2 +- pkg/process/net/windows_pipe.go | 12 ++++------ pkg/process/net/windows_pipe_testutil.go | 23 ------------------- 16 files changed, 38 insertions(+), 95 deletions(-) delete mode 100644 pkg/process/net/windows_pipe_testutil.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index a33ea3f5c3cc26..0a1f5269185011 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -485,7 +485,6 @@ /pkg/process/net/ @DataDog/universal-service-monitoring @DataDog/Networks /pkg/process/net/common_windows.go @DataDog/windows-agent /pkg/process/net/windows_pipe.go @DataDog/windows-kernel-integrations -/pkg/process/net/windows_pipe_testutil.go @DataDog/windows-kernel-integrations /pkg/proto/datadog/remoteconfig/ @DataDog/remote-config /pkg/proto/pbgo/ # do not notify anyone /pkg/proto/pbgo/trace @DataDog/agent-apm diff --git a/cmd/agent/subcommands/flare/command_other_test.go b/cmd/agent/subcommands/flare/command_other_test.go index 6730af25dcecee..bfb643b52e04da 100644 --- a/cmd/agent/subcommands/flare/command_other_test.go +++ b/cmd/agent/subcommands/flare/command_other_test.go @@ -17,6 +17,8 @@ import ( "github.com/DataDog/datadog-agent/pkg/config/model" ) +const systemProbeTestPipeName = "" + // NewSystemProbeTestServer starts a new mock server to handle System Probe requests. func NewSystemProbeTestServer(_ http.Handler) (*httptest.Server, error) { // Linux still uses a port-based system-probe, it does not need a dedicated system probe server diff --git a/cmd/agent/subcommands/flare/command_test.go b/cmd/agent/subcommands/flare/command_test.go index b555c751f58b59..1765fff7033be5 100644 --- a/cmd/agent/subcommands/flare/command_test.go +++ b/cmd/agent/subcommands/flare/command_test.go @@ -36,7 +36,11 @@ type commandTestSuite struct { func (c *commandTestSuite) SetupSuite() { t := c.T() - c.sysprobeSocketPath = path.Join(t.TempDir(), "sysprobe.sock") + if runtime.GOOS == "windows" { + c.sysprobeSocketPath = systemProbeTestPipeName + } else { + c.sysprobeSocketPath = path.Join(t.TempDir(), "sysprobe.sock") + } } // startTestServers starts test servers from a clean state to ensure no cache responses are used. @@ -125,11 +129,9 @@ func (c *commandTestSuite) TestReadProfileData() { mockConfig.SetWithoutSource("process_config.expvar_port", port) mockConfig.SetWithoutSource("security_agent.expvar_port", port) - mockSysProbeConfig := configmock.NewSystemProbe(t) - mockSysProbeConfig.SetWithoutSource("system_probe_config.enabled", true) - if runtime.GOOS == "windows" { - mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", u.Host) - } else { + if runtime.GOOS != "darwin" { + mockSysProbeConfig := configmock.NewSystemProbe(t) + mockSysProbeConfig.SetWithoutSource("system_probe_config.enabled", true) mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", c.sysprobeSocketPath) } @@ -197,11 +199,7 @@ func (c *commandTestSuite) TestReadProfileDataNoTraceAgent() { mockSysProbeConfig := configmock.NewSystemProbe(t) mockSysProbeConfig.SetWithoutSource("system_probe_config.enabled", true) - if runtime.GOOS == "windows" { - mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", u.Host) - } else { - mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", c.sysprobeSocketPath) - } + mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", c.sysprobeSocketPath) data, err := readProfileData(10) require.Error(t, err) diff --git a/cmd/agent/subcommands/flare/command_windows_test.go b/cmd/agent/subcommands/flare/command_windows_test.go index 9780e664740331..60b2c5b95709b8 100644 --- a/cmd/agent/subcommands/flare/command_windows_test.go +++ b/cmd/agent/subcommands/flare/command_windows_test.go @@ -21,21 +21,13 @@ import ( const ( // SystemProbeTestPipeName is the test named pipe for system-probe systemProbeTestPipeName = `\\.\pipe\dd_system_probe_flare_test` - - // systemProbeTestPipeSecurityDescriptor has a DACL that allows Everyone access for these tests. - systemProbeTestPipeSecurityDescriptor = "D:PAI(A;;FA;;;WD)" ) // NewSystemProbeTestServer starts a new mock server to handle System Probe requests. func NewSystemProbeTestServer(handler http.Handler) (*httptest.Server, error) { server := httptest.NewUnstartedServer(handler) - // Override the named pipe path for tests to avoid conflicts with the locally installed Datadog agent. - processNet.OverrideSystemProbeNamedPipeConfig( - systemProbeTestPipeName, - systemProbeTestPipeSecurityDescriptor) - - conn, err := processNet.NewSystemProbeListener("") + conn, err := processNet.NewSystemProbeListener(systemProbeTestPipeName) if err != nil { return nil, err } @@ -52,9 +44,7 @@ func InjectConnectionFailures(mockSysProbeConfig model.Config, mockConfig model. // Exercise a connection failure for a Windows system probe named pipe client by // making them use a bad path. // The system probe http server must be setup before this override. - processNet.OverrideSystemProbeNamedPipeConfig( - `\\.\pipe\dd_system_probe_test_bad`, - systemProbeTestPipeSecurityDescriptor) + mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", `\\.\pipe\dd_system_probe_test_bad`) // The security-agent connection is expected to fail too in this test, but // by enabling system probe, a port will be provided to it (security agent). diff --git a/cmd/system-probe/api/client/client_windows.go b/cmd/system-probe/api/client/client_windows.go index 1e8d667af08791..2d6dde51d57473 100644 --- a/cmd/system-probe/api/client/client_windows.go +++ b/cmd/system-probe/api/client/client_windows.go @@ -22,25 +22,20 @@ const ( idleConnTimeout = 5 * time.Second ) -var ( - // SystemProbePipeName is the production named pipe for system probe - SystemProbePipeName = `\\.\pipe\dd_system_probe` -) - // DialContextFunc returns a function to be used in http.Transport.DialContext for connecting to system-probe. // The result will be OS-specific. -func DialContextFunc(_ string) func(context.Context, string, string) (net.Conn, error) { +func DialContextFunc(namedPipePath string) func(context.Context, string, string) (net.Conn, error) { return func(_ context.Context, _, _ string) (net.Conn, error) { // Go clients do not immediately close (named pipe) connections when done, // they keep connections idle for a while. Make sure the idle time // is not too high and the timeout is generous enough for pending connections. var timeout = 30 * time.Second - namedPipe, err := winio.DialPipe(SystemProbePipeName, &timeout) + namedPipe, err := winio.DialPipe(namedPipePath, &timeout) if err != nil { // This important error may not get reported upstream, making connection failures // very difficult to diagnose. Explicitly log the error here too for diagnostics. - var namedPipeErr = fmt.Errorf("error connecting to named pipe %q: %s", SystemProbePipeName, err) + var namedPipeErr = fmt.Errorf("error connecting to named pipe %q: %s", namedPipePath, err) log.Error(namedPipeErr.Error()) return nil, namedPipeErr } diff --git a/cmd/system-probe/config/adjust.go b/cmd/system-probe/config/adjust.go index f1b6eb2205ce5c..6e133ff92a96e7 100644 --- a/cmd/system-probe/config/adjust.go +++ b/cmd/system-probe/config/adjust.go @@ -11,6 +11,7 @@ import ( "sync" "github.com/DataDog/datadog-agent/pkg/config/model" + "github.com/DataDog/datadog-agent/pkg/config/setup" "github.com/DataDog/datadog-agent/pkg/util/log" ) @@ -40,7 +41,7 @@ func Adjust(cfg model.Config) { cfg.Set(netNS("enabled"), true, model.SourceAgentRuntime) } - validateString(cfg, spNS("sysprobe_socket"), defaultSystemProbeAddress, ValidateSocketAddress) + validateString(cfg, spNS("sysprobe_socket"), setup.DefaultSystemProbeAddress, ValidateSocketAddress) deprecateBool(cfg, spNS("allow_precompiled_fallback"), spNS("allow_prebuilt_fallback")) allowPrebuiltEbpfFallback(cfg) diff --git a/cmd/system-probe/config/config_unix.go b/cmd/system-probe/config/config_unix.go index eaa3c04e14de1f..5e054a267041ee 100644 --- a/cmd/system-probe/config/config_unix.go +++ b/cmd/system-probe/config/config_unix.go @@ -13,9 +13,6 @@ import ( ) const ( - // defaultSystemProbeAddress is the default unix socket path to be used for connecting to the system probe - defaultSystemProbeAddress = "/opt/datadog-agent/run/sysprobe.sock" - defaultConfigDir = "/etc/datadog-agent" ) diff --git a/cmd/system-probe/config/config_unsupported.go b/cmd/system-probe/config/config_unsupported.go index 164e25b7472249..d871055afb6252 100644 --- a/cmd/system-probe/config/config_unsupported.go +++ b/cmd/system-probe/config/config_unsupported.go @@ -8,19 +8,18 @@ package config import ( - "fmt" + "errors" "github.com/DataDog/datadog-agent/pkg/config/model" ) const ( - defaultConfigDir = "" - defaultSystemProbeAddress = "" + defaultConfigDir = "" ) -// ValidateSocketAddress validates that the sysprobe socket config option is of the correct format. +// ValidateSocketAddress is not supported on this platform func ValidateSocketAddress(sockPath string) error { - return fmt.Errorf("system-probe unsupported") + return errors.New("system-probe unsupported") } // ProcessEventDataStreamSupported returns true if process event data stream is supported diff --git a/cmd/system-probe/config/config_windows.go b/cmd/system-probe/config/config_windows.go index 54074b4042ab50..224fbe184b6005 100644 --- a/cmd/system-probe/config/config_windows.go +++ b/cmd/system-probe/config/config_windows.go @@ -9,16 +9,13 @@ package config import ( "fmt" - "net" + "strings" "github.com/DataDog/datadog-agent/pkg/config/model" "github.com/DataDog/datadog-agent/pkg/util/winutil" ) const ( - // defaultSystemProbeAddress is the default address to be used for connecting to the system probe - defaultSystemProbeAddress = "localhost:3333" - // ServiceName is the service name used for the system-probe ServiceName = "datadog-system-probe" ) @@ -36,8 +33,8 @@ func init() { // ValidateSocketAddress validates that the sysprobe socket config option is of the correct format. func ValidateSocketAddress(sockAddress string) error { - if _, _, err := net.SplitHostPort(sockAddress); err != nil { - return fmt.Errorf("socket address must be of the form 'host:port'") + if !strings.HasPrefix(sockAddress, `\\.\pipe\`) { + return fmt.Errorf(`named pipe must be of the form '\\.\pipe\'`) } return nil } diff --git a/pkg/collector/corechecks/system/wincrashdetect/wincrashdetect_windows_test.go b/pkg/collector/corechecks/system/wincrashdetect/wincrashdetect_windows_test.go index ff1cdb0ced1c83..54adc802083922 100644 --- a/pkg/collector/corechecks/system/wincrashdetect/wincrashdetect_windows_test.go +++ b/pkg/collector/corechecks/system/wincrashdetect/wincrashdetect_windows_test.go @@ -28,20 +28,12 @@ import ( ) const ( - // SystemProbeTestPipeName is the test named pipe for system-probe + // systemProbeTestPipeName is the test named pipe for system-probe systemProbeTestPipeName = `\\.\pipe\dd_system_probe_wincrash_test` - - // systemProbeTestPipeSecurityDescriptor has a DACL that allows Everyone access for these tests. - systemProbeTestPipeSecurityDescriptor = "D:PAI(A;;FA;;;WD)" ) func createSystemProbeListener() (l net.Listener, close func()) { - process_net.OverrideSystemProbeNamedPipeConfig( - systemProbeTestPipeName, - systemProbeTestPipeSecurityDescriptor) - - // No socket address. Windows uses a fixed name pipe - conn, err := process_net.NewSystemProbeListener("") + conn, err := process_net.NewSystemProbeListener(systemProbeTestPipeName) if err != nil { panic(err) } diff --git a/pkg/config/setup/config_darwin.go b/pkg/config/setup/config_darwin.go index f1c5aa880a384d..4bd2cc4b437bdc 100644 --- a/pkg/config/setup/config_darwin.go +++ b/pkg/config/setup/config_darwin.go @@ -18,8 +18,8 @@ const ( DefaultProcessAgentLogFile = "/opt/datadog-agent/logs/process-agent.log" // DefaultOTelAgentLogFile is the default otel-agent log file DefaultOTelAgentLogFile = "/opt/datadog-agent/logs/otel-agent.log" - // defaultSystemProbeAddress is the default unix socket path to be used for connecting to the system probe - defaultSystemProbeAddress = "/opt/datadog-agent/run/sysprobe.sock" + // DefaultSystemProbeAddress is the default unix socket path to be used for connecting to the system probe + DefaultSystemProbeAddress = "/opt/datadog-agent/run/sysprobe.sock" // defaultEventMonitorAddress is the default unix socket path to be used for connecting to the event monitor defaultEventMonitorAddress = "/opt/datadog-agent/run/event-monitor.sock" defaultSystemProbeLogFilePath = "/opt/datadog-agent/logs/system-probe.log" diff --git a/pkg/config/setup/config_nix.go b/pkg/config/setup/config_nix.go index 1d27458abe8d9b..d57173501678dd 100644 --- a/pkg/config/setup/config_nix.go +++ b/pkg/config/setup/config_nix.go @@ -21,8 +21,8 @@ var ( ) var ( - // defaultSystemProbeAddress is the default unix socket path to be used for connecting to the system probe - defaultSystemProbeAddress = filepath.Join(InstallPath, "run/sysprobe.sock") + // DefaultSystemProbeAddress is the default unix socket path to be used for connecting to the system probe + DefaultSystemProbeAddress = filepath.Join(InstallPath, "run/sysprobe.sock") // defaultEventMonitorAddress is the default unix socket path to be used for connecting to the event monitor defaultEventMonitorAddress = filepath.Join(InstallPath, "run/event-monitor.sock") // DefaultDDAgentBin the process agent's binary diff --git a/pkg/config/setup/config_windows.go b/pkg/config/setup/config_windows.go index c8f237798ece67..88afb8621714dc 100644 --- a/pkg/config/setup/config_windows.go +++ b/pkg/config/setup/config_windows.go @@ -26,8 +26,8 @@ var ( DefaultProcessAgentLogFile = "C:\\ProgramData\\Datadog\\logs\\process-agent.log" // DefaultOTelAgentLogFile is the default otel-agent log file DefaultOTelAgentLogFile = "C:\\ProgramData\\Datadog\\logs\\otel-agent.log" - // defaultSystemProbeAddress is the default address to be used for connecting to the system probe - defaultSystemProbeAddress = "localhost:3333" + // DefaultSystemProbeAddress is the default address to be used for connecting to the system probe + DefaultSystemProbeAddress = `\\.\pipe\dd_system_probe` // defaultEventMonitorAddress is the default address to be used for connecting to the event monitor defaultEventMonitorAddress = "localhost:3335" defaultSystemProbeLogFilePath = "c:\\programdata\\datadog\\logs\\system-probe.log" diff --git a/pkg/config/setup/system_probe.go b/pkg/config/setup/system_probe.go index 3308d3a07a39c6..464620f3942be2 100644 --- a/pkg/config/setup/system_probe.go +++ b/pkg/config/setup/system_probe.go @@ -118,7 +118,7 @@ func InitSystemProbeConfig(cfg pkgconfigmodel.Config) { cfg.BindEnvAndSetDefault(join(spNS, "external"), false, "DD_SYSTEM_PROBE_EXTERNAL") cfg.SetKnown(join(spNS, "adjusted")) - cfg.BindEnvAndSetDefault(join(spNS, "sysprobe_socket"), defaultSystemProbeAddress, "DD_SYSPROBE_SOCKET") + cfg.BindEnvAndSetDefault(join(spNS, "sysprobe_socket"), DefaultSystemProbeAddress, "DD_SYSPROBE_SOCKET") cfg.BindEnvAndSetDefault(join(spNS, "max_conns_per_message"), defaultConnsMessageBatchSize) cfg.BindEnvAndSetDefault(join(spNS, "debug_port"), 0) diff --git a/pkg/process/net/windows_pipe.go b/pkg/process/net/windows_pipe.go index 3b5b63d8cbd9c4..2475511e9a5903 100644 --- a/pkg/process/net/windows_pipe.go +++ b/pkg/process/net/windows_pipe.go @@ -12,8 +12,6 @@ import ( "net" "github.com/Microsoft/go-winio" - - "github.com/DataDog/datadog-agent/cmd/system-probe/api/client" ) const ( @@ -54,15 +52,13 @@ func newPipeListener(namedPipeName string) (net.Listener, error) { } // NewSystemProbeListener sets up a named pipe listener for the system probe service. -func NewSystemProbeListener(_ string) (*WindowsPipeListener, error) { - // socketAddr not used - - namedPipe, err := newPipeListener(client.SystemProbePipeName) +func NewSystemProbeListener(namedPipePath string) (*WindowsPipeListener, error) { + namedPipe, err := newPipeListener(namedPipePath) if err != nil { - return nil, fmt.Errorf("error named pipe %s : %s", client.SystemProbePipeName, err) + return nil, fmt.Errorf("error named pipe %s : %s", namedPipePath, err) } - return &WindowsPipeListener{namedPipe, client.SystemProbePipeName}, nil + return &WindowsPipeListener{namedPipe, namedPipePath}, nil } // GetListener will return underlying Listener's conn diff --git a/pkg/process/net/windows_pipe_testutil.go b/pkg/process/net/windows_pipe_testutil.go deleted file mode 100644 index d3e1a9413db23d..00000000000000 --- a/pkg/process/net/windows_pipe_testutil.go +++ /dev/null @@ -1,23 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2024-present Datadog, Inc. - -//go:build test && windows - -package net - -import "github.com/DataDog/datadog-agent/cmd/system-probe/api/client" - -// OverrideSystemProbeNamedPipeConfig sets the active named pipe path and its DACL for -// System Probe connections. -// This is used by tests only to avoid conflicts with an existing locally installed Datadog agent. -func OverrideSystemProbeNamedPipeConfig(path string, securityDescriptor string) { - if path != "" { - client.SystemProbePipeName = path - } - - if securityDescriptor != "" { - systemProbePipSecurityDescriptor = securityDescriptor - } -}