chore: switch from hectane/go-acl to DataDog/go-acl fork#49932
chore: switch from hectane/go-acl to DataDog/go-acl fork#49932
Conversation
Go Package Import DifferencesBaseline: baebb27
|
Files inventory check summaryFile checks results against ancestor 9cdd8c6e: Results for datadog-agent_7.80.0~devel.git.354.9b25671.pipeline.110522586-1_amd64.deb:No change detected |
This comment has been minimized.
This comment has been minimized.
Static quality checks✅ Please find below the results from static quality gates 31 successful checks with minimal change (< 2 KiB)
On-wire sizes (compressed)
|
chouetz
added
changelog/no-changelog
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c91e5390e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect | ||
| github.com/ebitengine/purego v0.10.0 // indirect | ||
| github.com/go-ole/go-ole v1.3.0 // indirect | ||
| github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb // indirect |
There was a problem hiding this comment.
This leaf module has no OTel collector dependencies, yet tidy still adds github.com/hectane/go-acl immediately after switching its only ACL import to github.com/DataDog/go-acl. That means the new fork is still pulling upstream's github.com/hectane/go-acl/api internally, so Windows builds of this package continue to depend on the untagged upstream module rather than only the tagged DataDog fork; the fork should rewrite its internal api import and be retagged before this dependency swap achieves its stated supply-chain goal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for catching this — I verified the chain and confirmed the issue. The fork's source files (apply.go and util.go) still imported github.com/hectane/go-acl/api, so consumers of the fork were transitively pulling upstream regardless of the module-path rename.
Fixed in the fork via DataDog/go-acl#3 — internal api imports rewritten to github.com/DataDog/go-acl/api. v1.0.0 retagged at the new merge commit, and go.sum refreshed here in commit 3adc641 to pick up the new zip hash (h1:FL7NsVyHOfZl2b1jTrmJCVhMatG/0zYKgWS+5FbUiSA=).
Confirmed self-contained on the fork side: go list -deps github.com/DataDog/go-acl after a build cache flush no longer references github.com/hectane/go-acl/api.
The hectane/go-acl // indirect entries that remain in some workspace go.mod files are no longer caused by the fork's internals — they come solely from opentelemetry-collector-contrib packages pinning older datadog-agent submodule versions that still required hectane. Those will clear automatically once OTel bumps its datadog-agent pin past this PR.
chouquette
left a comment
chouquette
left a comment
There was a problem hiding this comment.
LGTM for the only agent-build owned file
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 3698031 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | -0.68 | [-3.57, +2.21] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +1.53 | [+0.55, +2.51] | 1 | Logs bounds checks dashboard |
| ➖ | otlp_ingest_logs | memory utilization | +0.55 | [+0.44, +0.67] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.49 | [+0.45, +0.53] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.37 | [+0.12, +0.62] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | +0.15 | [+0.10, +0.20] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | +0.09 | [-0.35, +0.53] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.06 | [-0.45, +0.57] | 1 | Logs |
| ➖ | docker_containers_memory | memory utilization | +0.05 | [-0.05, +0.15] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | +0.04 | [-0.16, +0.24] | 1 | Logs |
| ➖ | ddot_metrics_sum_delta | memory utilization | +0.04 | [-0.15, +0.22] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.03 | [-0.06, +0.12] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.02 | [-0.37, +0.42] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.01 | [-0.19, +0.20] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_v3 | ingress throughput | -0.00 | [-0.20, +0.20] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | -0.01 | [-0.13, +0.11] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | -0.03 | [-0.27, +0.21] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.08 | [-0.12, -0.03] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | -0.27 | [-0.32, -0.22] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | -0.42 | [-0.57, -0.26] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | -0.56 | [-0.72, -0.40] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | -0.67 | [-0.74, -0.59] | 1 | Logs |
| ➖ | docker_containers_cpu | % cpu utilization | -0.68 | [-3.57, +2.21] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -1.79 | [-1.98, -1.60] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | 740 ≥ 26 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | 241.98MiB ≤ 370MiB | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | 681 ≥ 26 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | 0.16GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_0ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | 0.20GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_1000ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | 0.17GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_100ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | 0.18GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_500ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 141.58MiB ≤ 147MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 468.27MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 173.44MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 347.34 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 394.04MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
chouetz
force-pushed
the
nschweitzer/switch-to-datadog-go-acl-fork
branch
2 times, most recently
from
d03c83a to
df9ea66
Compare
Replaces the direct dependency on github.com/hectane/go-acl (v0.0.0-20230225031251-cdfc9e3acf94, the head of upstream's unmerged PR #19) with github.com/DataDog/go-acl v1.0.0, a tagged release of a DataDog-owned fork that contains the same code (upstream master HEAD plus the golang.org/x/sys 0.1.0 bump from PR #19). Why: - Upstream hectane/go-acl is inactive, has no semver tags, and the commit we depended on lives on an unmerged PR branch — fragile ground for Renovate, which fell back to digest updates that produced malformed go.mod entries and time-regressing "updates" (see #49574). - Owning a tagged fork lets Renovate resolve real semver versions and guarantees the source we depend on cannot vanish or be force-pushed. Scope: - Two Go imports rewritten (pkg/util/filesystem/permission_windows.go and pkg/security/probe/probe_auditing_windows_test.go). - All affected go.mod/go.sum updated via dda inv tidy. - Bazel manifest updated (deps/go.MODULE.bazel, pkg/util/filesystem/BUILD.bazel). - LICENSE-3rdparty.csv regenerated. The hectane/go-acl // indirect entries that remain come from old datadog-agent submodule versions pinned by opentelemetry-collector- contrib. They will disappear once OTel bumps its datadog-agent pin past this PR.
The fork's v1.0.0 tag was moved to include a fix that rewrites the fork's internal api-subpackage imports from github.com/hectane/go-acl/ api to github.com/DataDog/go-acl/api. This commit updates go.sum to reflect the retagged v1.0.0 zip hash. Source code change (in the fork): apply.go and util.go now import the api subpackage from github.com/DataDog/go-acl/api instead of upstream's path, so consumers depending solely on the fork no longer transitively fetch upstream hectane/go-acl. The remaining hectane/go-acl // indirect entries in workspace go.mod files come from the OTel collector contrib chain (it pins older datadog-agent submodule versions) and will clear once OTel bumps past this PR.
CI failed on the v1.0.0 retag because the GOPROXY had cached the zip from the first retag and refused to serve the post-internal-imports- fix content for the same tag. This is exactly why Go modules treat tags as immutable. Cut a fresh DataDog/go-acl v1.0.1 at the same commit (ff63db7) — no content change, just a new immutable tag with a clean proxy cache. As a bonus: with the fork now fully self-contained (its internal api imports point to DataDog/go-acl/api) AND a fresh tag avoiding cache issues, dda inv tidy now removes every hectane/go-acl // indirect entry from the workspace go.mod files. The supply-chain goal is fully achieved: no upstream hectane code in the build.
chouetz
force-pushed
the
nschweitzer/switch-to-datadog-go-acl-fork
branch
from
9b25671 to
782d4d6
Compare
5 participants
Summary
Replaces the direct dependency on
github.com/hectane/go-acl(v0.0.0-20230225031251-cdfc9e3acf94, which is the head of upstream's unmerged PR #19) withgithub.com/DataDog/go-acl v1.0.0, a tagged release of a DataDog-owned fork containing the same code.Why
Upstream
hectane/go-aclis effectively inactive (last commit early 2023) and has no semver tags. The commit we currently pin lives on an unmerged PR branch, which made it fragile in two ways:go.modentries and time-regressing "updates" — see Update github.com/hectane/go-acl digest to ca0b05c - autoclosed #49574, where Renovate proposed bumping the dep from a Feb 2023 commit to a Jan 2023 one (older).Owning a tagged fork lets Renovate resolve real semver versions and guarantees the source we depend on is immutable.
The companion guardrail PR #49861 disables digest updates for the
gomodRenovate manager — together they prevent this class of issue across all tag-less Go deps.What's in the fork
DataDog/go-acl@v1.0.0is upstream master HEAD (ca0b05c) plus thegolang.org/x/sys 0.1.0bump from upstream PR #19 (cherry-picked, dependabot authorship preserved), with themoduledirective set togithub.com/DataDog/go-acl.Scope
pkg/util/filesystem/permission_windows.gopkg/security/probe/probe_auditing_windows_test.gogo.modandgo.sumupdates across the workspace viadda inv tidy.deps/go.MODULE.bazel,pkg/util/filesystem/BUILD.bazel).LICENSE-3rdparty.csvregenerated.About the remaining
hectane/go-acl // indirectentriesAfter this PR,
hectane/go-aclstill appears as// indirectin some submodulego.modfiles. Those come fromopentelemetry-collector-contribpackages that pin olderdatadog-agentsubmodule versions which still required hectane. They will disappear automatically once OTel bumps itsdatadog-agentpin past this PR — no action needed on our side.The pseudo-version on those indirect entries (
v0.0.0-20230122075934-ca0b05cb1adb) is the upstream master HEAD as resolved by Go's MVS — the previous Feb 2023 pin (cdfc9e3) was a manual reference to the unmerged PR branch and was never present in the published versions of our submodules.Test plan
dda inv tidyis idempotent.pkg/util/filesystemsucceeds on Windows.dda inv lint-licensespasses.