[procmgr] add Windows LogonUser spawn when supervisor token does not match - #55834
[procmgr] add Windows LogonUser spawn when supervisor token does not match#55834jose-manuel-almaza wants to merge 12 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 6179283 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +0.65 | [-0.20, +1.50] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | +0.61 | [+0.38, +0.85] | 1 | Logs |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.56 | [+0.51, +0.60] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.06 | [-0.29, +0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.12 | [-0.16, -0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.18 | [-0.31, -0.06] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.22 | [-0.25, -0.19] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.33 | [-0.40, -0.25] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.37 | [-0.41, -0.34] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | -1.48 | [-1.69, -1.27] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 ≤ 5 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 172.46MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 747.75KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 ≤ 5 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 522.04MiB ≤ 537MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 212.98MiB ≤ 228MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.43MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 364.30 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 433.10MiB ≤ 455MiB | 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.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 72.98MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 31.40 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 325.35MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 62.02 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 306.12MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 22.81 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 307.63MiB ≤ 345MiB | 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 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 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 memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, 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_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 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_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 missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_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_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
Files inventory check summaryFile checks results against ancestor 61792831: Results for datadog-agent_7.85.0~devel.git.163.36a715f.pipeline.136848129-1_amd64.deb:No change detected Results for datadog-iot-agent_7.85.0~devel.git.163.36a715f.pipeline.136848129-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
25 successful checks with minimal change (< 2 KiB)
|
b868ff7 to
a7295e2
Compare
a7295e2 to
2e58f19
Compare
pgimalac
left a comment
There was a problem hiding this comment.
Any chance this could be split into several smaller PRs ?
4000 lines is not really reviewable tbh...
@pgimalac |
|
@jose-manuel-almaza yeah would be nice if possible, currently it's a lot of changes at once and I don't have context on most of that so it's not easy to review |
faec628 to
9e1930e
Compare
|
@pgimalac Split is done.
The |
90103cb to
d68f3ff
Compare
### What does this PR do? Extracts spawn behind `platform::spawn_child_handle()` and introduces `ProcessHandle` and `SpawnRequest`. Unix and Windows still use `Command::spawn`. Successful `ManagedProcess::spawn` starts the exit watcher itself, so the manager no longer calls `spawn` then `spawn_watcher` as two steps. Spawn behavior is unchanged. ### Motivation Reviewer-requested split of #55834. This lands the cross-platform spawn plumbing first. Windows LogonUser agent-profile spawn stays in #55834. ### Describe how you validated your changes - `cargo test -p dd-procmgrd` - `cargo clippy -p dd-procmgrd --all-targets -- -D warnings` - `cargo test -p dd-procmgrd --test e2e --features test-helpers reload_` - `cargo test -p dd-procmgrd --test e2e --features test-helpers list` - `cargo test -p dd-procmgrd --test e2e --features test-helpers describe` - `dda inv test --targets=./pkg/procmgr/coat/...` Bazel `//pkg/procmgr/rust:dd-procmgrd_test` is linux/windows only (skipped locally). ### Additional Notes Mechanical refactor only. No LogonUser, no password read, no change to privileged spawn. Keep #55834 open. After this PR merges, rebase #55834 onto `main` so that diff is LogonUser-only. Co-authored-by: josemanuel.almaza <josemanuel.almaza@datadoghq.com>
### What does this PR do? Splits the monolithic `platform/windows.rs` (~684 lines) into focused modules: `child_env`, `console`, `job_object`, `process`, `runtime_user`, and `wide`. Public API and spawn behavior are unchanged. Windows still uses `Command::spawn` via `spawn_child_handle(name, config)`. ### Motivation #55834 was too large to review in one pass. #55944 extracted the cross-platform spawn plumbing first. This PR reorganizes the Windows platform code only, so follow-up work on identity resolution and LogonUser spawn can land in smaller, reviewable PRs. ### Describe how you validated your changes - `cargo clippy -p dd-procmgrd --all-targets -- -D warnings` - `cargo test -p dd-procmgrd` Bazel `//pkg/procmgr/rust:dd-procmgrd_test` on Windows amd64 (CI). ### Additional Notes Mechanical refactor only. No LogonUser, no credential types, no identity modules, no change to the `ManagedProcess` spawn path. **Review map:** 1. `platform/windows/mod.rs`: re-exports and install/registry helpers (same surface as old `windows.rs`) 2. `child_env.rs`, `console.rs`, `job_object.rs`, `runtime_user.rs`, `wide.rs`: pure moves from #55944 3. `process.rs`: `terminate_process_by_pid` (used by `send_force_kill`) 4. `platform/windows/spawn.rs`: unchanged `Command::spawn` path Stacks on #55944. After that PR merges, retarget base to `main`. Co-authored-by: josemanuel.almaza <josemanuel.almaza@datadoghq.com>
…#56015) ## Summary Adds Windows spawn identity resolution (`SpawnCredential`, installed-user registry/SID lookup) and agent-profile `CreateProcessW` when dd-procmgrd already runs as the installed agent account (supervisor-token inherit). `Privileged` profile keeps `Command::spawn`. Agent profile fails when the supervisor token does not match the installed account, with no wrong-identity fallback. Expands `ProcessHandle` on Windows for supervised Win32 children (suspended spawn, job assignment, restricted stdio inheritance). Preserves registry install-account display in list/describe (`registry_domain` vs `logon_domain`). ## Motivation #55834 was too large to review in one pass. #55944 and #56010 landed spawn plumbing and platform structure. This PR adds identity and the supervisor-inherit spawn path so the LogonUser work in #55834 can focus on the remaining credential cases. ## Describe how you validated your changes - `cargo test -p dd-procmgrd` - `cargo clippy -p dd-procmgrd --all-targets -- -D warnings` - reload / list / describe e2e (test-helpers) - pending Windows CI - COAT tests - pending Windows CI Bazel Windows CI. ## Additional Notes **Review map:** 1. `local_agent_account.rs`, `token_identity.rs`, `sid.rs`: identity resolution 2. `spawn/managed.rs`: inherit vs explicit error when supervisor token does not match installed account 3. `spawn/credential.rs`: supervisor-only token duplication 4. `handle.rs` + `spawn/suspended.rs`: supervised child lifecycle 5. `process.rs`: `ManagedProcess` credential field + unified `spawn_child_handle(self)` Follows #56010 (merged). No LogonUser, no installer password read. Co-authored-by: josemanuel.almaza <josemanuel.almaza@datadoghq.com>
Wire agent-profile LogonUser spawn on top of CreateProcessW inherit. Privileged and same-account agent children share that inherit path, with the job attached at CreateProcess time. LogonUser remains for agent children when the supervisor token does not match. Default MSI still inherits as the installed agent account.
0968953 to
52195ed
Compare
Compact SupervisorAccount and PasswordLogon destructuring so Bazel rustfmt checks pass in CI.
Attach JOB_LIST before HANDLE_LIST in STARTUPINFOEX. Use CREATE_NO_WINDOW only for hidden children. Remove dead from_tokio_child.
7414868 to
36ac90a
Compare
GitLab CI runs dd-procmgrd inside a job object, so create-time JOB_LIST needs CREATE_BREAKAWAY_FROM_JOB. On ERROR_INVALID_PARAMETER, fall back to post-create nested job assignment and fix the Clippy items-after-test-module lint in win32.rs.
Apply rustfmt to job_object.rs and inherit_supervisor.rs so the dd-procmgrd-lib and dd-procmgrd_test rustfmt checks pass in CI.
When create-time JOB_LIST is rejected (parent in a foreign job), retry with CREATE_NEW_CONSOLE so spawn works in no-console CI runners.
Detect foreign job parents with IsProcessInJob and use the pre-JOB_LIST suspended create path (assign + ResumeThread) instead of create-time JOB_LIST. Replaces the ineffective CREATE_NEW_CONSOLE-only fallback.
Apply rustfmt import ordering across the Windows job-object spawn files and silence clippy on the post-assign spawn helpers so Bazel rustfmt and clippy checks pass in CI.
Use sleep_test_config with a short stop_timeout and a 10s Windows sleep so ping-based test children force-kill quickly instead of hanging past the 60s per-test limit.
Use wait_for_stop after request_stop in process tests so ping-based sleep children escalate to force-kill instead of waiting for natural exit.
PowerShell is not always on PATH in the Windows Bazel CI container, so test_shutdown_all_sigkill_on_timeout could fail immediately on spawn. Use ping for stubborn-child helpers and shorten Windows TEST_SLEEP_SECS.
Restore a longer Windows sleep child duration so ping does not exit before request_stop under CI scheduling jitter, which left processes in Exited instead of Stopped.
What does this PR do?
Rebases Windows agent-profile LogonUser spawn onto
mainafter #56015 (supervisor-token inherit).Windows spawn router:
datadog-agent-process) and agent inherit (supervisor already runs as the installed agent account):CreateProcessWin the supervisor's security context. Job attached at create viaPROC_THREAD_ATTRIBUTE_JOB_LIST.LogonUser+CreateProcessAsUserW, with the user profile kept loaded until the child exits. Job attached the same way.Windows no longer uses Tokio
Command::spawn.CreateProcessAsUserWis not used for inherit: that API needsSeIncreaseQuotaPrivilege, which the installed agent account does not hold.Default MSI (process manager as ddagentuser) still inherits. No behavior change for those children. LogonUser is for a future LocalSystem supervisor (after A1 + #55921).
Unix behavior is unchanged.
Motivation
If
dd-procmgrdruns as LocalSystem, agent-profile children must log on as the installed agent account. This must land before the MSI switches the process manager service account.Review follow-up from the previous stack: both privilege levels use
CreateProcess*and pass the job at create time, instead of Tokio spawn plus post-assign.Describe how you validated your changes
cargo test -p dd-procmgrd --features test-helpers --libcargo test -p dd-procmgrd --test e2e --features test-helpers reload_cfg(windows)spawn paths need CI (not compiled on macOS)Additional Notes
mainafter [procmgr] resolve Windows spawn identity and inherit supervisor token #56015. Previous tip remains recoverable on GitHub.