[contrib] add orchestrion integration configuration#3074
Conversation
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
60deffb to
ed1218e
Compare
Datadog ReportBranch report: ✅ 0 Failed, 5201 Passed, 72 Skipped, 2m 47.81s Total Time |
f06d0d3 to
8591beb
Compare
8591beb to
6d487dd
Compare
|
|
||
| # ddapm-test-agent is used to observe side effects from the tracer during integration tests. | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 |
There was a problem hiding this comment.
🟠 Code Vulnerability
Workflow depends on a GitHub actions pinned by tag (...read more)
BenchmarksBenchmark execution time: 2025-01-20 15:18:12 Comparing candidate commit 6e09796 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 1 unstable metrics. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| func (l testLogger) Log(msg string) { | ||
| l.TB.Log(msg) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI over 1 year ago
To fix the problem, we need to ensure that sensitive information is not logged in clear text. In this case, we should avoid logging the home directory path directly. Instead, we can log a generic message indicating that the home directory was retrieved without including the actual path. This way, we maintain the functionality of logging without exposing sensitive information.
We will make changes to the getHomeDir function in internal/civisibility/utils/home.go to log a generic message instead of the actual home directory path. Additionally, we will ensure that the logging function in internal/orchestrion/_integration/internal/agent/logger.go does not log sensitive information.
| @@ -14,3 +14,8 @@ | ||
| func (l testLogger) Log(msg string) { | ||
| l.TB.Log(msg) | ||
| // Ensure no sensitive information is logged | ||
| if strings.Contains(msg, "home directory") { | ||
| l.TB.Log("Sensitive information redacted") | ||
| } else { | ||
| l.TB.Log(msg) | ||
| } | ||
| } |
| @@ -61,3 +61,3 @@ | ||
| defer func() { | ||
| log.Debug("civisibility: home directory: %v", homeDir) | ||
| log.Debug("civisibility: home directory retrieved successfully") | ||
| }() |
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write(b) |
Check warning
Code scanning / CodeQL
Reflected cross-site scripting
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI over 1 year ago
To fix the reflected cross-site scripting vulnerability, we need to sanitize the user-provided data before writing it to the response. In Go, we can use the html.EscapeString function from the html package to escape special characters in the user input, making it safe to include in the HTML response.
- We will import the
htmlpackage. - We will use
html.EscapeStringto sanitize the data read fromr.Bodybefore writing it to the response.
| @@ -19,2 +19,3 @@ | ||
| "github.com/stretchr/testify/require" | ||
| "html" | ||
| ) | ||
| @@ -153,3 +154,3 @@ | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write(b) | ||
| _, _ = w.Write([]byte(html.EscapeString(string(b)))) | ||
| } |
9386875 to
d0e5e17
Compare
rarguelloF
left a comment
rarguelloF
left a comment
There was a problem hiding this comment.
LGTM overall! left some comments / questions 🙇
| // Log as desired using the context-aware logger | ||
| cLog.Info("Completed some work!") | ||
| // Output: | ||
| // You should see: |
There was a problem hiding this comment.
I'm curious the motivation of the change, was the testable example failing? I'm wondering if we actually run these in CI 🤔
There was a problem hiding this comment.
They run as part of go test normal behaviour. It didn't work because the hook is not adding the fields for trace ID and span ID.
I checked the hook's code and it wasn't changed. I'm not sure why it triggered here, but it did.
| // short-hand request functions [http.Get], [http.Head], [http.Post], and | ||
| // [http.PostForm]. Using these functions allows for better control over the | ||
| // trace context propagation. | ||
| package client |
There was a problem hiding this comment.
could this package be made internal? it doesn't seem to make a lot of sense outside of Orchestrion right?
There was a problem hiding this comment.
it's intended to allow for users to be able to manually do what orchestrion automatically does, and provides context-aware versions of short-hand functions in the net/http package which otherwise allow no context.
This package actually has very little use to orchestrion itself (as it enables GLS-based context propagation, making the context.Context passing possibly un-necessary).
There was a problem hiding this comment.
what I mean is this package doesn't provide any tracing specific funcionality, these are just convenience helpers over http.NewRequestWithContext + http.DefaultClient.Do that IMO shouldn't be part of the exposed api of dd-trace-go.
| @@ -0,0 +1,65 @@ | |||
| ## Orchestrion Integration Tests | |||
There was a problem hiding this comment.
why does the folder name start with _
There was a problem hiding this comment.
The go toolchain (alledgedly) ignores folders starting with _ so it makes it clear that this is a module delineation that isn't meant for users to directly use.
888d119 to
afde3ea
Compare
d770999 to
6e09796
Compare
7 participants
What does this PR do?
Moves orchestrion integrations configuration from orchestrion itself to dd-trace-go.
Motivation
This improves maintainability of automatic injection configuration, and allows orchestrion customers more fine-grained control of what gets injected automatically.
Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!