diff --git a/ddtrace/tracer/option.go b/ddtrace/tracer/option.go index db46fcd5ba..16b057d064 100644 --- a/ddtrace/tracer/option.go +++ b/ddtrace/tracer/option.go @@ -100,10 +100,6 @@ var contribIntegrations = map[string]struct { } var ( - // defaultSocketAPM specifies the socket path to use for connecting to the trace-agent. - // Replaced in tests - defaultSocketAPM = "/var/run/datadog/apm.socket" - // defaultSocketDSD specifies the socket path to use for connecting to the statsd server. // Replaced in tests defaultSocketDSD = "/var/run/datadog/dsd.socket" @@ -437,10 +433,7 @@ func newConfig(opts ...StartOption) *config { fn(c) } if c.agentURL == nil { - c.agentURL = resolveAgentAddr() - if url := internal.AgentURLFromEnv(); url != nil { - c.agentURL = url - } + c.agentURL = internal.AgentURLFromEnv() } if c.agentURL.Scheme == "unix" { // If we're connecting over UDS we can just rely on the agent to provide the hostname diff --git a/ddtrace/tracer/option_test.go b/ddtrace/tracer/option_test.go index a14c3e6805..8dc1049197 100644 --- a/ddtrace/tracer/option_test.go +++ b/ddtrace/tracer/option_test.go @@ -27,6 +27,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" + "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" "gopkg.in/DataDog/dd-trace-go.v1/internal/namingschema" "gopkg.in/DataDog/dd-trace-go.v1/internal/telemetry" @@ -827,9 +828,9 @@ func TestTracerOptionsDefaults(t *testing.T) { func TestDefaultHTTPClient(t *testing.T) { defTracerClient := func(timeout int) *http.Client { - if _, err := os.Stat(defaultSocketAPM); err == nil { + if _, err := os.Stat(internal.DefaultTraceAgentUDSPath); err == nil { // we have the UDS socket file, use it - return udsClient(defaultSocketAPM, 0) + return udsClient(internal.DefaultTraceAgentUDSPath, 0) } return defaultHTTPClient(time.Second * time.Duration(timeout)) } @@ -853,8 +854,8 @@ func TestDefaultHTTPClient(t *testing.T) { t.Fatal(err) } defer os.RemoveAll(f.Name()) - defer func(old string) { defaultSocketAPM = old }(defaultSocketAPM) - defaultSocketAPM = f.Name() + defer func(old string) { internal.DefaultTraceAgentUDSPath = old }(internal.DefaultTraceAgentUDSPath) + internal.DefaultTraceAgentUDSPath = f.Name() x := *defTracerClient(2) y := *defaultHTTPClient(2) compareHTTPClients(t, x, y) diff --git a/ddtrace/tracer/transport.go b/ddtrace/tracer/transport.go index 939eebe438..c6a900c055 100644 --- a/ddtrace/tracer/transport.go +++ b/ddtrace/tracer/transport.go @@ -11,8 +11,6 @@ import ( "io" "net" "net/http" - "net/url" - "os" "runtime" "strconv" "strings" @@ -188,32 +186,3 @@ func (t *httpTransport) send(p *payload) (body io.ReadCloser, err error) { func (t *httpTransport) endpoint() string { return t.traceURL } - -// resolveAgentAddr resolves the given agent address and fills in any missing host -// and port using the defaults. Some environment variable settings will -// take precedence over configuration. -func resolveAgentAddr() *url.URL { - var host, port string - if v := os.Getenv("DD_AGENT_HOST"); v != "" { - host = v - } - if v := os.Getenv("DD_TRACE_AGENT_PORT"); v != "" { - port = v - } - if _, err := os.Stat(defaultSocketAPM); host == "" && port == "" && err == nil { - return &url.URL{ - Scheme: "unix", - Path: defaultSocketAPM, - } - } - if host == "" { - host = defaultHostname - } - if port == "" { - port = defaultPort - } - return &url.URL{ - Scheme: "http", - Host: fmt.Sprintf("%s:%s", host, port), - } -} diff --git a/ddtrace/tracer/transport_test.go b/ddtrace/tracer/transport_test.go index b6246feefc..aaaddbbacf 100644 --- a/ddtrace/tracer/transport_test.go +++ b/ddtrace/tracer/transport_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/DataDog/dd-trace-go.v1/internal" ) // getTestSpan returns a Span with different fields set @@ -102,7 +103,7 @@ func TestResolveAgentAddr(t *testing.T) { if tt.envPort != "" { t.Setenv("DD_TRACE_AGENT_PORT", tt.envPort) } - c.agentURL = resolveAgentAddr() + c.agentURL = internal.AgentURLFromEnv() if tt.inOpt != nil { tt.inOpt(c) } @@ -111,12 +112,12 @@ func TestResolveAgentAddr(t *testing.T) { } t.Run("UDS", func(t *testing.T) { - old := defaultSocketAPM + old := internal.DefaultTraceAgentUDSPath d, err := os.Getwd() require.NoError(t, err) - defaultSocketAPM = d // Choose a file we know will exist - defer func() { defaultSocketAPM = old }() - c.agentURL = resolveAgentAddr() + internal.DefaultTraceAgentUDSPath = d // Choose a file we know will exist + defer func() { internal.DefaultTraceAgentUDSPath = old }() + c.agentURL = internal.AgentURLFromEnv() assert.Equal(t, &url.URL{Scheme: "unix", Path: d}, c.agentURL) }) } diff --git a/internal/agent.go b/internal/agent.go index c8f835166c..f4bcdce8b0 100644 --- a/internal/agent.go +++ b/internal/agent.go @@ -6,31 +6,70 @@ package internal import ( + "net" "net/url" "os" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) -// AgentURLFromEnv determines the trace agent URL from environment variable -// DD_TRACE_AGENT_URL. If the determined value is valid and the scheme is -// supported (unix, http or https), it will return an *url.URL. Otherwise, -// it returns nil. +const ( + DefaultAgentHostname = "localhost" + DefaultTraceAgentPort = "8126" +) + +// This is a variable rather than a constant so it can be replaced in unit tests +var DefaultTraceAgentUDSPath = "/var/run/datadog/apm.socket" + +// AgentURLFromEnv resolves the URL for the trace agent based on +// the default host/port and UDS path, and via standard environment variables. +// AgentURLFromEnv has the following priority order: +// - First, DD_TRACE_AGENT_URL if it is set +// - Then, if either of DD_AGENT_HOST and DD_TRACE_AGENT_PORT are set, +// use http://DD_AGENT_HOST:DD_TRACE_AGENT_PORT, +// defaulting to localhost and 8126, respectively +// - Then, DefaultTraceAgentUDSPath, if the path exists +// - Finally, localhost:8126 func AgentURLFromEnv() *url.URL { - agentURL := os.Getenv("DD_TRACE_AGENT_URL") - if agentURL == "" { - return nil + if agentURL := os.Getenv("DD_TRACE_AGENT_URL"); agentURL != "" { + u, err := url.Parse(agentURL) + if err != nil { + log.Warn("Failed to parse DD_TRACE_AGENT_URL: %v", err) + } else { + switch u.Scheme { + case "unix", "http", "https": + return u + default: + log.Warn("Unsupported protocol %q in Agent URL %q. Must be one of: http, https, unix.", u.Scheme, agentURL) + } + } + } + + host, providedHost := os.LookupEnv("DD_AGENT_HOST") + port, providedPort := os.LookupEnv("DD_TRACE_AGENT_PORT") + if host == "" { + // We treat set but empty the same as unset + providedHost = false + host = DefaultAgentHostname + } + if port == "" { + // We treat set but empty the same as unset + providedPort = false + port = DefaultTraceAgentPort } - u, err := url.Parse(agentURL) - if err != nil { - log.Warn("Failed to parse DD_TRACE_AGENT_URL: %v", err) - return nil + httpURL := &url.URL{ + Scheme: "http", + Host: net.JoinHostPort(host, port), } - switch u.Scheme { - case "unix", "http", "https": - return u - default: - log.Warn("Unsupported protocol %q in Agent URL %q. Must be one of: http, https, unix.", u.Scheme, agentURL) - return nil + if providedHost || providedPort { + return httpURL + } + + if _, err := os.Stat(DefaultTraceAgentUDSPath); err == nil { + return &url.URL{ + Scheme: "unix", + Path: DefaultTraceAgentUDSPath, + } } + return httpURL } diff --git a/internal/agent_test.go b/internal/agent_test.go index b6d3e7437f..a7be3f22b5 100644 --- a/internal/agent_test.go +++ b/internal/agent_test.go @@ -11,14 +11,15 @@ import ( "github.com/stretchr/testify/assert" ) -func TestAgentURLFromEnv2(t *testing.T) { +func TestAgentURLFromEnv(t *testing.T) { for name, tc := range map[string]struct { input string want string }{ - "empty": {input: "", want: ""}, - "protocol": {input: "bad://custom:1234", want: ""}, - "invalid": {input: "http://localhost%+o:8126", want: ""}, + "empty": {input: "", want: "http://localhost:8126"}, + // The next two are invalid, in which case we should fall back to the defaults + "protocol": {input: "bad://custom:1234", want: "http://localhost:8126"}, + "invalid": {input: "http://localhost%+o:8126", want: "http://localhost:8126"}, "http": {input: "http://custom:1234", want: "http://custom:1234"}, "https": {input: "https://custom:1234", want: "https://custom:1234"}, "unix": {input: "unix:///path/to/custom.socket", want: "unix:///path/to/custom.socket"}, @@ -26,11 +27,68 @@ func TestAgentURLFromEnv2(t *testing.T) { t.Run(name, func(t *testing.T) { t.Setenv("DD_TRACE_AGENT_URL", tc.input) url := AgentURLFromEnv() - if tc.want == "" { - assert.Nil(t, url) - } else { - assert.Equal(t, tc.want, url.String()) - } + assert.Equal(t, tc.want, url.String()) }) } } + +func TestAgentURLPriorityOrder(t *testing.T) { + makeTestUDS := func(t *testing.T) string { + // NB: We don't try to connect to this, we just check that a + // path exists + s := t.TempDir() + old := DefaultTraceAgentUDSPath + DefaultTraceAgentUDSPath = s + t.Cleanup(func() { DefaultTraceAgentUDSPath = old }) + return s + } + + t.Run("DD_TRACE_AGENT_URL", func(t *testing.T) { + t.Setenv("DD_TRACE_AGENT_URL", "https://foo:1234") + t.Setenv("DD_AGENT_HOST", "bar") + t.Setenv("DD_TRACE_AGENT_PORT", "5678") + _ = makeTestUDS(t) + url := AgentURLFromEnv() + assert.Equal(t, "https", url.Scheme) + assert.Equal(t, "foo:1234", url.Host) + }) + + t.Run("DD_AGENT_HOST-and-DD_TRACE_AGENT_PORT", func(t *testing.T) { + t.Setenv("DD_AGENT_HOST", "bar") + t.Setenv("DD_TRACE_AGENT_PORT", "5678") + _ = makeTestUDS(t) + url := AgentURLFromEnv() + assert.Equal(t, "http", url.Scheme) + assert.Equal(t, "bar:5678", url.Host) + }) + + t.Run("DD_AGENT_HOST", func(t *testing.T) { + t.Setenv("DD_AGENT_HOST", "bar") + _ = makeTestUDS(t) + url := AgentURLFromEnv() + assert.Equal(t, "http", url.Scheme) + assert.Equal(t, "bar:8126", url.Host) + }) + + t.Run("DD_TRACE_AGENT_PORT", func(t *testing.T) { + t.Setenv("DD_TRACE_AGENT_PORT", "5678") + _ = makeTestUDS(t) + url := AgentURLFromEnv() + assert.Equal(t, "http", url.Scheme) + assert.Equal(t, "localhost:5678", url.Host) + }) + + t.Run("UDS", func(t *testing.T) { + uds := makeTestUDS(t) + url := AgentURLFromEnv() + assert.Equal(t, "unix", url.Scheme) + assert.Equal(t, "", url.Host) + assert.Equal(t, uds, url.Path) + }) + + t.Run("nothing", func(t *testing.T) { + url := AgentURLFromEnv() + assert.Equal(t, "http", url.Scheme) + assert.Equal(t, "localhost:8126", url.Host) + }) +} diff --git a/profiler/options.go b/profiler/options.go index 5d0e94f14d..36e715c156 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -202,20 +202,11 @@ func defaultConfig() (*config, error) { c.addProfileType(t) } - agentHost, agentPort := defaultAgentHost, defaultAgentPort - if v := os.Getenv("DD_AGENT_HOST"); v != "" { - agentHost = v - } - if v := os.Getenv("DD_TRACE_AGENT_PORT"); v != "" { - agentPort = v - } - WithAgentAddr(net.JoinHostPort(agentHost, agentPort))(&c) - if url := internal.AgentURLFromEnv(); url != nil { - if url.Scheme == "unix" { - WithUDS(url.Path)(&c) - } else { - c.agentURL = url.String() + "/profiling/v1/input" - } + url := internal.AgentURLFromEnv() + if url.Scheme == "unix" { + WithUDS(url.Path)(&c) + } else { + c.agentURL = url.String() + "/profiling/v1/input" } if v := os.Getenv("DD_PROFILING_UPLOAD_TIMEOUT"); v != "" { d, err := time.ParseDuration(v) @@ -475,13 +466,23 @@ func WithHTTPClient(client *http.Client) Option { // WithUDS configures the HTTP client to dial the Datadog Agent via the specified Unix Domain Socket path. func WithUDS(socketPath string) Option { - return WithHTTPClient(&http.Client{ - Transport: &http.Transport{ - DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { - return net.Dial("unix", socketPath) + return func(c *config) { + // The HTTP client needs a valid URL. The host portion of the + // url in particular can't just be the socket path, or else that + // will be interpreted as part of the request path and the + // request will fail. Clean up the path here so we get + // something resembling the desired path in any profiler logs. + // TODO: copied from ddtrace/tracer, but is this correct? + cleanPath := fmt.Sprintf("UDS_%s", strings.NewReplacer(":", "_", "/", "_", `\`, "_").Replace(socketPath)) + c.agentURL = "http://" + cleanPath + "/profiling/v1/input" + WithHTTPClient(&http.Client{ + Transport: &http.Transport{ + DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { + return net.Dial("unix", socketPath) + }, }, - }, - }) + })(c) + } } // withOutputDir writes a copy of all uploaded profiles to the given diff --git a/profiler/profiler_test.go b/profiler/profiler_test.go index 09d2d2a1cb..844078948b 100644 --- a/profiler/profiler_test.go +++ b/profiler/profiler_test.go @@ -17,6 +17,7 @@ import ( "net/http" "net/http/httptest" "os" + "path" "runtime" "runtime/trace" "strconv" @@ -25,6 +26,7 @@ import ( "time" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" + "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" "gopkg.in/DataDog/dd-trace-go.v1/internal/httpmem" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" @@ -712,3 +714,37 @@ func TestVersionResolution(t *testing.T) { assert.NotContains(t, data.tags, "version:7.8.9") }) } + +func TestUDSDefault(t *testing.T) { + dir := t.TempDir() + socket := path.Join(dir, "agent.socket") + + orig := internal.DefaultTraceAgentUDSPath + defer func() { + internal.DefaultTraceAgentUDSPath = orig + }() + internal.DefaultTraceAgentUDSPath = socket + + profiles := make(chan profileMeta, 1) + backend := &mockBackend{t: t, profiles: profiles} + mux := http.NewServeMux() + // Specifically set up a handler for /profiling/v1/input to test that we + // don't use the filesystem path to the Unix domain socket in the HTTP + // request path. + mux.Handle("/profiling/v1/input", backend) + server := httptest.NewUnstartedServer(mux) + l, err := net.Listen("unix", socket) + if err != nil { + t.Fatal(err) + } + defer l.Close() + server.Listener = l + server.Start() + defer server.Close() + + err = Start(WithProfileTypes(), WithPeriod(10*time.Millisecond)) + require.NoError(t, err) + defer Stop() + + <-profiles +}