[ABLD-395] Framework to create toolchains for OS provided tools - #54177
Conversation
WHAT - Creates a tiny framework for finding an OS provided tool and telling Bazel it is a toolchain. - Use it for Macos pkgbuild and codesign. MOTIV This is an important part of our supply chain security posture. We want to account for every binary we use to build the product. Ideally that would include even basic low level things, like bash and cat. In practice we just want to declare the things that might vary across OSes or from OS release to release. Eventually, we will expand this the framework to include package metadata (https://github.com/bazel-contrib/supply-chain/tree/main/metadata) that can be easily gathered into a workspace BOM. The more immediate need is to create toolchains for pkgbuild and codesign to create rules to make macos packages. #54115
Files inventory check summaryFile checks results against ancestor 9569d8dc: Results for datadog-agent_7.83.0~devel.git.536.d9d76a4.pipeline.128686930-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 29a80d4 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +1.04 | [+0.17, +1.91] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.73 | [+0.48, +0.97] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.14 | [+0.09, +0.18] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.09 | [+0.04, +0.14] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.05 | [-0.04, +0.14] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.09 | [-0.12, -0.05] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.17 | [-0.21, -0.13] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.29 | [-0.41, -0.17] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 148.31MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 731.79KiB ≤ 819.20KiB | 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 | 492.20MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.12MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 186.67MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.34MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 363.65 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 414.19MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.93GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.14MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 27.62 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 301.85MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 60.29 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 280.39MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 20.42 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 283.60MiB ≤ 320MiB | 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".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_metrics_logs, bounds check cpu_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 missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 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 total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6e763d290
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
alopezz
left a comment
There was a problem hiding this comment.
This is probably mostly fine, but I need a bit more time to make sure I understand enough of how the toolchain missing case is being handled. In the meantime I do have a suggestion of two.
| exec_compatible_with = rctx.attr.exec_compatible_with, | ||
| ) | ||
|
|
||
| def make_repo_builder(name, tool_name, impl = _default_repo_builder_impl): |
There was a problem hiding this comment.
This name seems to describe the function poorly. Probably something like local_toolchain_repository would be more apt.
There was a problem hiding this comment.
It's not that either, because it doesn't make the repository. It returns a rule, that you have to call to make the thing you want. So it's really a builder pattern. Think of the expansion in MODULE.bazel. It would be
# This next line is the effect of load(..., "find_macos_pkgbuild")
find_macos_pkgbuild = make_repo_builder(name = NAME, tool_name = "pkgbuild")
# This calls the function to make the repository.
find_macos_pkgbuild(
name = "macos_pkgbuild",
exec_compatible_with = ["@platforms//os:macos"],
)
So, maybe we rename find_macos_pkgbuild. Maybe "os_provided_macos_pkgbuild".
That's wordy, but more accurate.
There was a problem hiding this comment.
That's fair, but the name is still a bit too generic, it could be something like make_local_toolchain_repository_rule or make_os_provided_tool_repo_rule or similar, at least something that points at this being intended to wrap local tools would be desirable in my opinion.
There was a problem hiding this comment.
Changed to make_local_toolchain_repository_rule
alopezz
left a comment
There was a problem hiding this comment.
A couple more questions.
|
|
||
| load("@@//bazel/toolchains:toolchain_info.bzl", "ToolInfo") | ||
|
|
||
| def _{TOOL_NAME}_toolchain_impl(ctx): |
There was a problem hiding this comment.
And another question that I have is whether we actually need to be templated with the TOOL_NAME baked in or whether a single common rule can be used to define all these toolchains instead.
There was a problem hiding this comment.
For just the two I have done, yes. But that gets in the way of future expansion, where I want different things to customize the template. pkgbuild and codesign, do not have version commands, but for tools that do, I want the repo rule to extract the version (which has to be tool specific) and put that in as metadata so we can have the SBOM say things like otool: cctools-1030.6.3
I have that ready in another PR, but it's not needed yet. Essentially the repo_builder call will use a private impl function instead of the default, and your private impl will call do the lookup and then call write_toolchain_repo to emit the repo.
chouquette
left a comment
There was a problem hiding this comment.
LGTM but codex seems to have a good point
| # {TOOL_NAME}_missing_toolchain provides a fallback toolchain so that toolchain | ||
| # resolution can succeed even on platforms that do not have that tool. | ||
| # If this toolchain is selected, the constraint ":have_{TOOL_NAME}" will not be satistifed, | ||
| # so that can be used with with exec_compatible_with clauses. |
There was a problem hiding this comment.
Did you check whether it can actually be used in exec_compatible_with clauses?
According to https://bazel.build/reference/be/platforms-and-toolchains#toolchain_args, only constraint_setting's can be passed here, and the attribute is nonconfigurable.
There was a problem hiding this comment.
You're right it can't be used there. I got ahead of myself. We were going to unify constraint_value and config_setting but never got around to it. A rule using it would have to examine the valid field.
I fixed the comment.
What does this PR do?
The important feature of the way this is implemented is that the toolchain always safely resolves on all platforms, so you can
bazel cqueryacross platforms, even though you will fail if you try tobuild. Combined withexec_compatible_withyou can make targets that only build if the required tools are available. This is important when there are different tools available as OSes evolve (or are simply different by vendor, like debian and redhat) and we need to create paths that can gracefully use different tools depending on what is available.There is also a capability to use a bazel target as the tool implementation instead of looking it up in $PATH. That is useful for building alternate implementations (let's say a windows tool on linux). That will come into use in full remote execution mode, when the target and exec hosts can be different architectures.
Examples:
Motivation
This is an important part of our supply chain security posture. We want to account for every binary we use to build the product. Ideally that would include even basic low level things, like bash and cat. In practice we can make the tradeoff to stop at declaring the things that might vary across OSes or from OS release to release.
Eventually, we will expand this the framework to include package metadata (https://github.com/bazel-contrib/supply-chain/tree/main/metadata) that can be easily gathered into a workspace BOM.
The more immediate need is to create toolchains for pkgbuild and codesign to create rules to make macos packages. #54115
Describe how you validated your changes
With the BUILD file
macos:
linux:
Additional
This is an alternate version of #54154. I think I like it better.
[ABLD-386]: https://datadoghq.atlassian.net/browse/ABLD-386