diff --git a/.github/workflows/test-e2e.yml b/.github/workflows/test-e2e.yml index 5db15dc616..10ea2aedde 100644 --- a/.github/workflows/test-e2e.yml +++ b/.github/workflows/test-e2e.yml @@ -254,6 +254,29 @@ jobs: PULSE_MULTI_TENANT_ENABLED: "true" run: node scripts/pretest.mjs + # One shared fixture fault previously consumed 47 minutes of repeated + # setup/login failures before probation hit the job limit. Admit the + # same authenticated inventory and history once, before either tier. + - name: Admit the default mock runtime + id: fixture + timeout-minutes: 8 + working-directory: tests/integration + env: + PULSE_E2E_BOOTSTRAP_TOKEN: 0123456789abcdef0123456789abcdef0123456789abcdef + PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY: "true" + PULSE_MULTI_TENANT_ENABLED: "true" + run: npx playwright test --config=fixture-readiness.config.ts + + - name: Upload failed fixture admission + if: ${{ !cancelled() && steps.fixture.outcome == 'failure' }} + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: fixture-readiness-shard-${{ matrix.shard }} + path: | + tests/integration/playwright-report-readiness/ + tests/integration/test-results-readiness/ + retention-days: 7 + # Two-tier run (see PROBATION_SPECS in tests/integration/e2e-tiering.mjs # for the tier mechanism and the promotion/demotion rule). The stable tier # is the gate; the probation tier reuses the same containers but reports @@ -281,6 +304,26 @@ jobs: working-directory: tests/integration run: node scripts/report-stable-e2e-failures.mjs test-results/junit.xml + # Preserve the completed stable failure before observational work can + # reach the job deadline. Probation must not take its receipts with it. + - name: Upload Playwright report + if: ${{ !cancelled() && steps.stable.outcome == 'failure' }} + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: playwright-report-shard-${{ matrix.shard }} + path: tests/integration/playwright-report/ + retention-days: 3 + + # The HTML report already contains screenshots, videos and traces. + # Keep only the separate machine-readable result here. + - name: Upload JUnit results + if: ${{ !cancelled() && steps.stable.outcome == 'failure' }} + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: junit-results-shard-${{ matrix.shard }} + path: tests/integration/test-results/junit.xml + retention-days: 3 + # Runs even when the stable tier failed (its data still counts toward # promotion), but not when the stable tier was skipped — that means the # environment never came up and every probation spec would fail on it. @@ -332,24 +375,6 @@ jobs: echo "=== Mock GitHub server logs ===" docker logs pulse-mock-github 2>&1 || echo "No pulse-mock-github container" - - name: Upload Playwright report - if: failure() - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: playwright-report-shard-${{ matrix.shard }} - path: tests/integration/playwright-report/ - retention-days: 3 - - # The HTML report already contains screenshots, videos and traces. - # Keep only the separate machine-readable result here. - - name: Upload JUnit results - if: failure() - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: junit-results-shard-${{ matrix.shard }} - path: tests/integration/test-results/junit.xml - retention-days: 3 - # continue-on-error keeps the job green on a probation failure, so # failure() never fires for these; key off the step outcome instead. - name: Upload probation Playwright report diff --git a/docs/API.md b/docs/API.md index 12ee36475f..1b5a6e56d8 100644 --- a/docs/API.md +++ b/docs/API.md @@ -634,39 +634,122 @@ Version fields are returned as plain semantic versions (no leading `v`). Returns a small public config payload (update channel, auto-update enabled). ### List Nodes -`GET /api/config/nodes` +`GET /api/config/nodes` (admin, `settings:read`) + +This administrative inventory covers saved **PVE, PBS and PMG connections**, +not individual VMs or agents. Keep its response private. Use the returned +connection `id` for updates, deletion and tests; a display name, VMID or another +installation's node name is not that ID. Re-read the inventory before a change +rather than guessing an ID from a list position. ### Add Node -`POST /api/config/nodes` +`POST /api/config/nodes` (admin, `settings:write`) + +Adding a connection saves credentials and enables collection; it is not a +passive validation step. Use an existing dedicated, least-privilege platform +token. The **Pulse API token** in the private header file authorises this +configuration request; the **Proxmox token** in the JSON body authorises reads +from the monitored platform. They are different credentials. Do not use a root +password or widen a department-scoped token to make denied peer reads succeed; +follow the [scoped-access checks](TROUBLESHOOTING.md#check-permissions-proxmox). +For PBS and PMG, use their [PBS](PBS.md#method-1-api-only-connection-recommended) +and [PMG](MAIL_GATEWAY.md) token guidance. PBS password-based onboarding can +also try to create a token and ACLs on the PBS server; it is not read-only. + +Prepare the request in a private editor, not a credential-bearing shell +assignment, here-document or command argument. Use the same Bash session as +the [private-response helper](#api-token-recommended): + +```bash +node_request_dir=$(umask 077; mktemp -d "${TMPDIR:-/tmp}/pulse-node.XXXXXX") && +node_request_file="$node_request_dir/node.json" && +(umask 077; set -C; : > "$node_request_file") && +vi "$node_request_file" +``` + +If preparation or the editor fails, stop. In that editor, enter the following +JSON, replacing the example hostname, connection name, full platform token ID +(`user@realm!token`) and token-secret placeholder with your intended values: + ```json { "type": "pve", - "name": "Proxmox 1", - "host": "https://198.51.100.10:8006", - "user": "root@pam", - "password": "password" + "name": "PVE connection A", + "host": "https://pve.example.net:8006", + "tokenName": "pulse-monitor@pve!pulse-readonly", + "tokenValue": "", + "verifySSL": true } ``` -### Test Connection -`POST /api/config/nodes/test-connection` -Validate credentials before saving. +Set `verifySSL` explicitly to `true`: omission on these new-connection and +test request paths does **not** enable certificate verification. The example +requires a certificate trusted by the Pulse server for that hostname. For a +self-signed/private certificate, also supply `fingerprint` with the SHA-256 +certificate fingerprint verified through an independent trusted channel. +A fingerprint merely fetched from the endpoint is not established trust. +Do not clear an existing pin, set `verifySSL` to `false`, or change to HTTP to +work around a certificate failure. -### Test Node Config (Validation Only) -`POST /api/config/nodes/test-config` -Validates node config without saving. +After reviewing this one intended configuration change, send the file: + +```bash +pulse_api POST /api/config/nodes < "$node_request_file" +``` + +The helper retains the response privately and makes one bounded request, +without retries. A lost response can follow a successful save: re-read the +saved inventory before deciding whether another request is needed. HTTP success +does not establish fresh readings for every node, guest, backup or History view. +Keep both request and response files out of reports and shared repositories; +remove the temporary request under your credential policy when no longer needed. + +### Test Connection +`POST /api/config/nodes/test-connection` (admin, `settings:write`) + +Tests supplied credentials with live platform requests without saving the +connection. Use the same private JSON-file transport and explicit TLS settings, +only when a live connection test is appropriate; it is not a required precursor +to filing a report. Success covers the reads performed by that test, not all +collection permissions, guest recovery or sustained monitoring. + +### Test Node Config (No Save, Live Request) +`POST /api/config/nodes/test-config` (admin, `settings:write`) + +This also contacts the platform using the supplied credentials; **no save** +does not mean offline schema validation. Do not run either test merely to +collect new evidence during a freeze/thaw, backup or unresponsive-host incident. +Use existing observations and the [safe diagnostic guidance](TROUBLESHOOTING.md#collect-diagnostics-safely). ### Update Node -`PUT /api/config/nodes/{id}` +`PUT /api/config/nodes/{id}` (admin, `settings:write`) + +Review the current connection and submit only the intended supported changes +from a private JSON file. Do not copy masked credentials from a GET response +back as new secrets. Credential rotation and trust changes are separate +decisions; preserve the existing credential and fingerprint when not changing +them. A response failure does not prove the update was unapplied. ### Delete Node -`DELETE /api/config/nodes/{id}` +`DELETE /api/config/nodes/{id}` (admin, `settings:write`) + +Removes the Pulse connection, not the platform workloads or the upstream token. +Collection and related alert coverage can stop. Check the exact connection and +arrange any needed independent coverage before an intentional removal; do not +delete and re-add a connection to diagnose missing readings or identity mixing. ### Test Node (Legacy) -`POST /api/config/nodes/{id}/test` +`POST /api/config/nodes/{id}/test` (admin, `settings:write`) + +Uses the saved connection for a live test. It is not a passive read of its +last result or a guarantee that ordinary collection recovered. ### Refresh Cluster Nodes -`POST /api/config/nodes/{id}/refresh-cluster` +`POST /api/config/nodes/{id}/refresh-cluster` (admin, `settings:write`) + +Actively rediscovers a saved PVE connection's cluster membership. It does not +select authorised nodes, grant missing permissions or repair an unrelated +standalone installation. Keep intentional token restrictions intact. ### Export Configuration `POST /api/config/export` (instance admin for the default organization, tenant diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b7de212224..5c8e8f591c 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -936,32 +936,111 @@ an agent assignment remain available in every edition. ### ICMP probe privileges -ICMP probes run the system `ping` binary, which needs the `CAP_NET_RAW` -capability. The systemd unit written by the installer hardens the service -with `NoNewPrivileges=true`, which strips ping's setuid bit and file -capabilities, so the unit also grants the capability directly with -`AmbientCapabilities=CAP_NET_RAW`. Units written by older versions of the -installer lack that line, and ICMP probes fail with -`icmp probe failed: ping: socktype: SOCK_RAW ... missing cap_net_raw+p capability`. +ICMP checks execute the system `ping` binary **on the selected observation +host**. A local permission error (`Operation not permitted`, `cap_net_raw` or +`icmp probe blocked`) is not evidence that the target is down, nor proof that +one particular service setting caused it. First distinguish **This Pulse +server** from an external agent observation. Changing the server's unit cannot +repair an agent's permissions on another host. -To fix an existing install, either re-run the install script (it rewrites -the unit) or add the capability as an override: +For Linux systemd servers, the current installer grants both +`AmbientCapabilities=CAP_NET_RAW` and `CapabilityBoundingSet=CAP_NET_RAW`, while +keeping `NoNewPrivileges=true`. This allows the child `ping` to inherit the +capability without gaining it through setuid or file capabilities. Older units +can lack the ambient grant. **Updates preserve an existing service unit**; +rerunning an installer is not a reliable unit repair, even if an older error +message recommends it. Do not reinstall over a working data directory. + +#### Inspect the actual systemd server first + +On the machine running the Pulse server, read only these effective properties. +For a Pulse LXC, run this **inside the Pulse LXC**, not on the Proxmox host or a +monitored guest. The example uses `pulse.service`; substitute your actual server +unit throughout, such as `pulse-backend.service` on some older installs, +**not pulse-agent.service**: ```bash -systemctl edit pulse # pulse-backend on ProxmoxVE community-script installs +timeout --signal=TERM --kill-after=1s 8s systemctl show pulse.service \ + --property=LoadState,ActiveState,User,NoNewPrivileges,AmbientCapabilities,CapabilityBoundingSet ``` +Interpret the result only after a successful read with `LoadState=loaded`. +A missing unit, failed or timed-out read is **unknown**, not an empty capability +set or a stopped server. This command does not print environment values, +credentials or full unit contents; keep those private. + +| Existing observation | What follows | +| --- | --- | +| `NoNewPrivileges=yes`, ceiling includes `cap_net_raw`, ambient grant missing | An older non-root server unit may need the narrow override below. | +| Both capability properties already include `cap_net_raw` | Do not add the grant again. Check the selected observation host, installed ping and its OS/container restrictions through normal administration. | +| Ceiling excludes `cap_net_raw`, or the deployment deliberately forbids it | An ambient line alone cannot overcome the ceiling. Preserve that restriction; do not replace the capability set with a permissive one. | +| `User=` is empty/root, or the service is custom | This is not the standard non-root server recipe. Review its existing privilege and data-access requirements using [manual-service guidance](INSTALL.md#2-bare-metal--systemd). Do not change the user or grant broader privileges just to clear the error. | + +#### Narrow override for an older non-root server + +Use this only for an **active** non-root server whose capability ceiling already +permits `cap_net_raw`, whose ambient grant is missing, and where granting it is +consistent with the deployment's intended policy. Keep deliberately inactive +services inactive. Record the existing drop-in locally before editing; it may +contain private configuration. Preserve every unrelated setting: + +```bash +sudo systemctl edit pulse.service +``` + +Add just this service setting to the drop-in: + ```ini [Service] AmbientCapabilities=CAP_NET_RAW ``` -Then `systemctl daemon-reload && systemctl restart pulse`. +Keep `NoNewPrivileges` enabled; do not run Pulse as root, set file capabilities +on the system-wide ping binary or remove sandbox settings. Do not reset or widen +`CapabilityBoundingSet` to make this example apply to a different profile. -Docker installs are unaffected: Docker's default capability set includes -`NET_RAW`. If you run the container with `--cap-drop=ALL`, add -`--cap-add=NET_RAW` to keep ICMP probes working. TCP and HTTP/HTTPS probes -need no special privileges. +```bash +sudo systemctl daemon-reload +``` + +If editing or reloading fails, stop. Repeat the read-only property check above +to confirm the intended ambient grant and preserved ceiling/hardening. Reload +changes systemd's configuration, **not the running process's capabilities**. + +Schedule the restart as maintenance, outside backups or other guest operations. +**Pulse monitoring and alerts are unavailable during the restart**; arrange +independent outage coverage. Only after the checks pass, for the server that +was active beforehand: + +```bash +sudo systemctl restart pulse.service && \ + timeout --signal=TERM --kill-after=1s 8s systemctl is-active pulse.service +``` + +`active` proves service startup, not ICMP recovery or notification delivery. +Check the next normally scheduled observation from the **same location**; +do not induce a workload failure or another notification to prove recovery. +A failed/unknown restart needs normal recovery, not repeated restarts. To undo +this change, edit only the added line, preserve other drop-in content and +reload; do not use `systemctl revert`, which can remove unrelated overrides. +Apply any further restart only under the same maintenance and prior-active +conditions. Preserve the data directory and encryption key throughout. + +#### Containers and external agents + +Docker's default capability set includes `NET_RAW`, but that does not prove ICMP +works in every rootless, non-root or restricted container. For an intentionally +restricted deployment, review only the selected observation host's ping/socket +permissions and saved deployment. Do not use `--privileged`, `--cap-add=ALL`, +disable user namespaces or recreate a container as a diagnostic. A deliberate +capability change must use the deployment's normal maintenance route while +preserving data and configuration; the systemd override above does not apply. + +External agents have their own OS/service requirements; see +[Agent Security](AGENT_SECURITY.md). Do not grant command execution or widen +agent privileges merely because the server's ICMP check failed. TCP and +HTTP/HTTPS checks need no raw-socket capability, but measure different behaviour; +switching protocol or observation location is not proof that ICMP recovered. --- diff --git a/docs/UNIFIED_AGENT.md b/docs/UNIFIED_AGENT.md index 201e12ef91..717e743b9e 100644 --- a/docs/UNIFIED_AGENT.md +++ b/docs/UNIFIED_AGENT.md @@ -979,6 +979,50 @@ private, and report evidence as unavailable if safe collection is not possible. QNAP and Unraid log locations are described in the [space troubleshooting section](#installer-fails-with-not-enough-free-disk-space). +### Agent lookup returns HTTP 401 or 403 + +At `/api/agents/agent/lookup`, Pulse authenticates the request **before** +searching for the host. **HTTP 401** is an authentication rejection; +**HTTP 403** with `missing_scope` means the authenticated token lacks +`agent:report`. An authenticated lookup can return **HTTP 404** when no matching +host exists. A 401 alone does not identify a mistyped, expired or revoked token, +the wrong Pulse instance, or an upstream authentication layer. + +The silent token prompt only keeps the secret out of the copied command and +shell history; it does not change reporting permissions. The normal +**Settings → Infrastructure → Install on a host** flow issues a monitoring +token. It does not use `--enroll`; the separate cluster deployment flow exchanges +a bootstrap token for a runtime token. Do not substitute a bootstrap token or +assume an enrolment-specific correction applies to a normal host installation. + +Use the [bounded agent log reader](#collect-agent-logs-safely) on the affected +agent host, or inspect an already retained excerpt privately. Distinguish: + +- `remote_config_client` reporting a lookup failure: this concerns configuration + retrieval and does not establish whether ordinary host reports were accepted. +- **Pulse rejected this agent's API token** from host reporting: keep its HTTP + status and redacted error, separately from the lookup failure. + +A running local service or successful-looking installer output does not prove +that Pulse accepted a fresh report. No host-report error in the selected log +window is inconclusive, not proof of successful reporting. Do not restart, +re-enrol, delete identity or enable debug logging to manufacture that evidence. + +In your signed-in administrator session on the **same Pulse instance** the +agent targets, check the intended record under **Settings → Security → API +Tokens** without revealing or copying its secret. Its presence, expiry and +reporting scope help distinguish server-side rejection, but do not prove that +the running service is using that record. Keep service environments, token +files and connection files private; do not paste a credential into a command, +URL or issue. Do not grant administrator scope, change Proxmox permissions or +disable authentication/TLS to test a guess. + +If reporting this problem, say whether the existing error is lookup-only or +also affects host reporting, and share only the failure stage, HTTP status and +manually redacted error line. Keep the installation and saved credentials +unchanged until the failing stage is understood; a 401 is not itself a reason +to reinstall or rotate every token. + ### pfSense service disabled after a major upgrade Pulse installs two service files on pfSense: the FreeBSD rc.d service at diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index ebc34e3afc..725dcd7dfe 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -9360,6 +9360,15 @@ registration, enrolment, install, update, removal, report identity and continuity are unchanged, and an agent's own declared node link still ignores exclusions. +A report-merge that names sources now picks links by the member that carries +the source instead of by the sources a link's folded side took in with it +(`internal/api/resourceapi/resources.go`, `ReportedManualLinkFolds`). An agent +that holds a Docker host and sits in a VM leaves the VM only when the agent's +own source is named, and a report naming only Docker leaves the agent in the +VM. A report naming every source or none still splits every link. Agent +registration, enrolment, install, update, removal, report identity and +continuity are unchanged. + ### Windows installer acceptance engine and absence (8 October 2026) Native installer parsing, service-removal failure controls and actual lifecycle @@ -9395,3 +9404,13 @@ for report-merge. No synthetic telemetry, identity key, enrollment or service link is added. `TestOperatorSplitOverridesProxmoxNodeAgentLink` checks that the monitor and its re-ingested API seed retain the fold across all split shapes, then survive pin persistence and a repeated split. + +### Mock-mode metrics-target lookups ride the fixture structure revision + +`internal/monitoring/monitor.go` changed only so the mock-mode unified view +records the fixture structure revision it was built at and +`MetricsTargetForResource` resolves mock-mode targets from that view while the +revision and link list hold (monitoring contract, "Mock-mode metrics-target +lookups ride the fixture structure revision"). Agent registration, enrolment, +install, update, removal, report identity and continuity are unchanged, and +real-mode resolution still goes through the live registry. diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 44d19f48f2..a9718d4d31 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1155,6 +1155,37 @@ inspectability, or convert missing/stale evidence into health. ## Current State +### Webhook drafts survive unconfirmed persistence + +The existing Notifications editor awaits an explicit successful acknowledgement +from its add/update owner before clearing a draft. The owner returns success +only after the API responds and the returned destination replaces or joins the +displayed inventory. A rejected or interrupted save keeps the same inputs, +masked fields, template, mention and tag/severity policy. Fixed inline guidance +does not claim the write was rejected: an interrupted response can follow a +committed create, so the operator checks saved destinations in another tab +before explicitly retrying. There is no editor-triggered automatic replay. + +One pending editor save locks the draft, Cancel and the conflicting local list +controls, and subsequent Save calls do not invoke another mutation. Success +closes the editor; failure unlocks it for correction or deliberate cancellation. +The API, encrypted persistence/publication ordering, saved-value masking, Test +payload parity, delivery policy, SSRF/TLS and existing shared API-client retry +rules are unchanged. This is draft recovery, not exactly-once creation or proof +that ordinary notifications arrive. + +`WebhookConfig.persistence.test.tsx` exercises the actual editor and mutation +owner through deferred create/update responses, failure, explicit retry, +canonical accepted response, cancellation and fixed callback-error guidance. +The deliberate-retry control defers its response separately: clearing the old +warning at retry start is not acknowledgement, and the draft stays locked until +the accepted response incorporates the destination and closes the editor. +`WebhookConfig.test.tsx` now acknowledges successful saves before expecting +editor dismissal. The local `webhook-save-acknowledgement` browser fixture +checks the same components with synthetic API transport, including desktop and +phone keyboard/touch journeys and another-tab reconciliation after a lost +response. It sends no real notification and establishes no installed outcome. + ### Active Docker update pending age survives restart A positive Docker image-update report reuses the matching active occurrence's @@ -4298,6 +4329,27 @@ Formatting-only follow-up retains this warning-level contract. The production-co browser matrix was rerun after formatting at desktop and narrow widths, including all/critical/warning save/reload, cancel, and warning webhook creation. +### SMTP rate-limit preference persistence + +The existing SMTP editor's rate limit is included by +`alertDestinationsModel.buildEmailConfigPayload` on every destination save. +Unrelated email edits and switching email off must preserve the loaded limit; +an explicitly edited limit must reach the existing Notifications API unchanged. +Zero remains the backend's default-limit sentinel, not an omitted or unlimited +value. Missing saved limits still use the existing UI default of 60 per minute. +SMTP masks, TLS settings, recipients, tags and severity preferences are unchanged. +This repairs a missing UI-to-API field, not the schema or transport policy. +Revealing switched-off email settings permits pointer and keyboard editing without +enabling email. The status toggle remains off and Send test email stays disabled. + +`emailRateLimit.persistence.test.tsx` connects the actual SMTP editor, destination +mutation owner, payload builder and email API to synthetic wire assertions. +The two payload-shape suites retain strict key checks including `rateLimit`. +`browser-tests/email-rate-limit.cjs` checks save and readback on desktop and phone, +including unrelated edits, disabled email and legacy zero. These fixtures do not +send notifications or establish native SMTP throughput, release availability or +the cause of an existing delivery failure. + ### Pulse Mobile push destination without an upsell The Alerts destinations page renders the mobile push panel only when the diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 7473475be7..ddf92e11b7 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -12033,3 +12033,15 @@ pairs with the identity-match exclusions. `400 Resource is not merged` and so two linked agents, which share their only source, can be reported. `TestContract_ResourceReportMergeReplacesOperatorLink` pins the response shape and the link's removal for a VM and the agent linked into it. + +A request that names `sources` undoes the links that join a member carrying +one of those sources to the rest of the merged resource, chosen by member +(`ReportedManualLinkFolds`) rather than by the sources a link's folded side +took in along with its own members. Naming the source of a leaf in a chain of +links no longer also splits the leaf's holder from the resource, and where +links form a cycle the named member is cut from every link that would rejoin +it. The request, status codes, response shape and the meaning of an empty +`sources` (every link) are unchanged, and no new endpoint, field or stored +decision is added: `exclusions` still counts one link pair each. +`TestResourceReportMergeSourceFilterDetachesChainMembers` reports a chain +through the handler and the REST listing. diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index 86b5afdf31..51278df518 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -2433,6 +2433,23 @@ artifact-selection behaviour. the deterministic dev user/hash. `tests/integration/scripts/run-playwright.mjs` owns the run-scoped `HOT_DEV_VERIFY_LOCK_FILE` handoff so overlapping browser proof cannot reuse stale first-run credentials. + Its npm entrypoint must attempt the existing posttest teardown once whenever + pretest was launched, including a nonzero setup exit or a failed test launch. + Failed setup skips Playwright and retains its original exit; teardown failure + may fail success but cannot turn an earlier failure into success. The handoff + stays held through teardown and is removed only if this invocation wrote it + and its complete contents still match. A failed lock preparation starts + neither setup nor teardown, and a non-hot-dev invocation never removes a + pre-existing verification lock. This is an ownership observation, not a new + exclusive lock or protection against an independent writer racing unlink. + `tests/integration/scripts/run-playwright.test.mjs` executes the actual + entrypoint with controlled pretest, Playwright and posttest child processes, + retaining exit precedence, identical run/state handoffs, partial-runtime + cleanup and foreign/replaced-lock preservation. Script smoke discovers it + through `scripts/tests/test-e2e-runner-cleanup.sh`. These command-level + controls do not establish real managed-runtime teardown, descendant reaping, + catchable-signal recovery, Windows execution or browser acceptance; the + existing owned shell supervisor and native/browser proofs remain separate. The managed and foreground hot-dev entrypoints must share one network-default contract: local dev binds frontend and backend traffic to loopback by default so installed LAN agents cannot accidentally treat a developer laptop as the @@ -5712,6 +5729,23 @@ helper must also prove that storage-pool and physical-disk history cover the suite's deepest seven-day chart window. The compose harness must seed that same seven-day window by default instead of forcing every parallel shard to build unrelated 90-day preview history. +Core E2E must admit that same authenticated default fixture once, through +`tests/integration/fixture-readiness.config.ts` and its dedicated spec, after +container startup and before either test tier. An absent opt-in, incomplete +inventory or insufficient pool/disk history fails the shard before suite work; +it must not become repeated per-test setup failures. The policy lives in +`tests/integration/scripts/default-mock-readiness.mjs`, consumed by the existing +authenticated helper, with unchanged 120/180-second phase waits and history +depth. Admission has no retry or ignored failure and a separate bounded report; +it does not replace later fixture checks or any product/browser assertion. +Completed stable-tier failure reports and JUnit must be uploaded before the +observational probation pass, so a later probation cancellation cannot suppress +those completed receipts. The job deadline, stable/probation selection and verdict +gate stay unchanged. `scripts/tests/test-e2e-fixture-admission.sh` covers every +required platform identity, both history scopes and depth, actual admission-spec +error/cleanup behavior, the workflow's fail-fast command and receipt ordering. +These controls prove fixture/gate orchestration, not the cause of a hosted +runtime fault, provider transience or native monitoring recovery. Managed runtime recovery and browser bootstrap proofs therefore need to keep helper coverage that demonstrates browser-shell request tracking remains trustworthy when the same test also performs direct health or security-status diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 58f47f21be..6178dec032 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -3530,6 +3530,73 @@ and `TestUnifiedResourceSnapshotWithLinksLeavesTheSharedSnapshotUnlinked` in `TestMockUnifiedStateViewUsesCanonicalMockFixtureGraph` in `internal/monitoring/canonical_guardrails_test.go` pins the call. +### Mock-mode metrics-target lookups ride the fixture structure revision + +`Monitor.MetricsTargetForResource` is the lookup behind every windowed alert +metric: an alert pass resolves a target once per resource and metric +(`metricWindowPoints`). In mock mode it answers from the `metricsTargets` the +cached mock view captured when it was built (`mockMetricsTarget` in +`internal/monitoring/metrics_target_resolution.go`), and that view serves +lookups for as long as `mock.FixtureStructureRevision` and the operator link +list it was built with are unchanged. A metric tick advances +`mock.FixtureDataVersion` but moves neither the revision nor any input of a +target (`BuildMetricsTarget` reads a resource's type, its source mappings and +the technology, Docker container ID and disk identity of its facets, with the +resource's own `MetricsTarget` as fallback), so a warm cache no longer sends +the next lookup off to build an estate. Mock mode being toggled, the estate +being reconfigured or a link change rebuilds the view on the next lookup; as +before, two lookups that miss together can each build one, and a cold or +invalidated cache builds even for an ID the estate does not list. Once the +view is current an unknown ID resolves to nothing and falls to the raw +resource store. The view stays keyed on the data version for every other +consumer (broadcast, `/api/state`, `GetUnifiedReadStateOrSnapshot`), which +still rebuild it on a data-version miss. + +The revision is read before the data version when a view is cached, and the +fixture advances its data version before its revision +(`advanceFixtureStructure` in `internal/mock/integration.go`). A reader that +sees a new revision therefore also sees the new data version and cannot cache +the previous estate's memoized snapshot under it. The interleaving cannot be +forced from outside, so `TestFixtureStructureRevisionOrderingContract` pins the +order of the writes and of the reads in source. + +Keyed on the data version, a lookup after a tick built the view (about 0.5 s +of CPU for the default 1,776-resource estate on an idle core), and a build +slower than the 2-second tick published an already-stale view, so the next +lookup built again. Startup runs the first alert pass before the listener +opens, so on a starved host it never finished: on 2026-10-08 the startup +watchdog's dump on pulse-dev parked the main goroutine in +`evaluateMetricWindow` -> `MetricsTargetForResource` -> `currentUnifiedStateView` +and `/api/health` timed out. The same binary pinned to one core behind four +busy loops missed the view cache on 675 of 929 requests at a 16-second average +build and had not served by 606 s; with this change it served at 148 s (13 s +on the core alone). This was not a regression: builds from 1a094646d7 and +d2ac0c5112 stall identically under equal contention and both start in 14 to +16 seconds at normal load. The remaining time on a starved host is the other +consumers of the shared view, which rebuild it whenever they find it stale (on +the starved run, 42 view requests in a start of 194 s were all misses, from the +Docker-alert prune, the connection snapshots and the host-agent evaluation), +and the monitor's repeated full resource-store passes at startup. Loops that +still reach the data-version view once per item are unchanged: the mock storage +chart history (`resolveMockStorageTotal` per uncached pool), the demo patrol's +finding adds (`patrolResourceOperatorStateProvider`) and, with +`PULSE_MOCK_KEEP_REAL_POLLING`, the per-node polling lookups +(`linkedHostForNode`, `getHostAgentTemperatureForNode`). + +`TestMockMetricsTargetLookupsDoNotRebuildTheViewAfterFixtureTicks` and +`TestMockMetricsTargetForUnknownIDDoesNotRebuildTheViewAfterFixtureTicks` fail on +the data-version key; `TestMockMetricsTargetFollowsAStructuralFixtureChange`, +`TestMockMetricsTargetFollowsAnOperatorLink`, +`TestMockMetricsTargetRebuildsWhenTheLinkListMoves` and +`TestMockMetricsTargetMatchesTheRegistryResolution` pin that the reused view is +replaced when the structure or the links change and answers as the registry +does. All six are in `internal/monitoring/monitor_host_agents_test.go`, with +their helpers in `internal/monitoring/metrics_target_resolution_test.go`. +`TestMetricTicksNeverChangeTheFixtureStructure` in +`internal/mock/platform_fixtures_test.go` pins the assumption itself: after each +tick of two cohort rotations the revision, the resource set and every +resolved target are unchanged. + ### Saved quiet-hours policy before queue activation `Monitor.New` binds `alerts.Manager.QuietHoursNotificationPolicy` to the @@ -6777,3 +6844,28 @@ generation younger than two seconds. The decision takes effect in the next loaded generation, not retroactively in the listing already served. The fixture seed keeps default thresholds while the configured live adapter retains its four-minute Proxmox threshold. + +### Incomplete guest filesystem responses do not establish recovery + +`GetVMFSInfo` preserves valid peer rows from a malformed array but returns an +error with them, through both single and cluster clients. The surviving rows +cannot establish total guest capacity, usage, or removal of a rejected volume. +A completed response with invalid counters is not a command-completion failure: +serial admission, same-target lock checks and single-attempt guest requests +remain unchanged, and there is no failover/replay or new cooldown. Alternate +clients' contradictory or out-of-range counters enforce the same completeness +boundary before publishing a current aggregate. + +Monitoring retains only the prior complete disk tuple, mount inventory and +original observation time within the existing age bound; otherwise disk usage +is unavailable. Partial responses do not create disk History, resolve genuine +filesystem alerts, or remove unknown volumes. Independent current CPU/memory +continues. A later complete ordinary inventory can still remove a volume and +publish an explicitly measured zero, append History and recover alerts. + +`pkg/proxmox/guest_filesystem_completeness_test.go` and +`internal/monitoring/guest_filesystem_completeness_test.go` cover valid peers plus +malformed records, both client/collector paths, canonical and served state, +original age/expiry, History, breach continuity and complete removal/recovery. +These are synthetic source controls, not native thaw, the cause of #2619/#2439, +or installed/released acceptance. diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index 83b984974a..84c77d9849 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -3724,6 +3724,35 @@ nonempty machine and agent IDs and canonical identity across fixture rebuilds. This changes fixture generation only, without relaxing production identity resolution or adding work to the recurring update path. +### Alert passes resolve metrics targets without materializing the estate + +An alert pass makes one `Monitor.MetricsTargetForResource` call per resource +and windowed metric, so that call has to be a lookup, not a view build. In mock +mode it reads the targets the cached view captured and keeps reading them across +metric ticks (see "Mock-mode metrics-target lookups ride the fixture structure +revision" in the monitoring contract). On pulse-dev, +`BenchmarkMockMetricsTargetResolutionAfterTick` measured a pass of 4,731 +lookups against the default estate, after a tick, at 1.8 ms and 151 KB. The +data-version key made the first lookup after every tick build the estate, and +made every lookup of a pass build it once a build outlasted the 2-second tick, +which stalled startup under CPU starvation. Real mode did not show this cost in +steady state: the store is itself the read state, and a one-off measurement +with a temporary counter (not asserted by a test) found a full +`updateResourceStore` pass over 1,694 resources (populate, metric syncs and +alert sync) taking 108 ms with 121 registry resolutions that fell back to the +full-mapping scan, because the registry's typed views are built before the +alert loop runs. `BenchmarkLiveRegistryMetricsTargetResolution` records only +the lookup cost of a registry whose views are unbuilt against one whose views +are built (136 ms against 5 ms per 5,079 lookups); it controls the warming +itself, so it cannot detect production failing to warm the views before the +alert loop, which is what the one-off measurement above showed. Run both +benchmarks with `-benchtime=3x`, since the registry case rebuilds its ingest +outside the timer. `TestMockMetricsTargetLookupsDoNotRebuildTheViewAfterFixtureTicks` +is the guard that fails on a rebuild per lookup. The remaining startup cost +under starvation is the monitor's repeated full resource-store passes (one per +supplemental provider registration) and the other consumers of the shared view, +which rebuild it whenever they find it stale; neither is changed here. + ### Update evidence reuses the bounded node observation Package evidence adds only one bounded status, one bounded reason, and the diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 702a5ae45d..89d68a05ec 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6758,3 +6758,12 @@ links' pairs, which deletes the pair's link row in the resource store as an unlink does. Nothing new is persisted: the record is rebuilt with each registry generation, and no schema, backup, retention, migration or recovery path is added or moved. + +Each fold also records its two sides' own sources (`HolderOwn`, `FoldedOwn`), +in the same unexported in-memory field, and `ReportedManualLinkFolds` in +`internal/unifiedresources/manual_link_folds.go` uses them so a +source-filtered report-merge in `internal/api/resourceapi/resources.go` +excludes the links that join a member carrying the named source to the rest +of the merged resource. That changes which pairs the report excludes, through +the same exclusion write as before; no schema, backup, retention, migration or recovery path is added or +moved. diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index e2efb80393..b3f485af41 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -7643,14 +7643,37 @@ fold already recorded (the holder still carries what the earlier fold brought). The record is unexported and rides in-memory clones, so it reaches the resources API's registry, which seeds from the monitor's already-linked listing and never holds the folded row. -Report-merge excludes the pair of every link whose folded side brought a -reported source category (`ResourceRegistry.ManualLinkFolds`), and the -store's one-decision-per-pair rule deletes that link row, so the pair splits -on every surface from the monitor's next rebuild, as with unlink. The drawer -reports every merged source. Categories are not attributed to individual -contributors: naming one that some folded side carried undoes that link even -where the holder carries it too, and a source that arrived along a chain of -links undoes every link on its way to the merged resource. +A fold's `Sources` is its folded side's whole subtree, so it cannot say +which member of a chain a source belongs to. Selecting links by it undid every +link on the way to a reported source (a Docker host linked into an agent +linked into a VM: naming Docker split the agent from the VM too), and where +links form a cycle it left the reported member joined through the link it did +not name. Each fold therefore also records what its two sides are on their own +(`HolderOwn`, `FoldedOwn`): the sources the side's row listed before any link +folded something into it, which a side that already took members in reads from +the record its first fold made. `recordManualLinkFold` runs before the other +side is merged for that reason, and a repeated pair unions the own sources as +it does `Sources`. + +Report-merge picks links by member (`ReportedManualLinkFolds`, fed by +`ResourceRegistry.ManualLinkFolds`). A member whose own sources include a +reported source is reported, whatever it took in; the resource being reported +on never is, since it is what stays. A link is undone when it joins a reported +member to a member that stays, one the resource still reaches without passing +through a reported member, or to another reported member. A member that hangs +below a reported one and carries no reported source goes with it, and the link +holding it there stays: the report names a source that is not theirs to undo. +Where links form a cycle, a reported member linked both to the resource and to +a member that stays is cut from both, so the surviving link cannot fold it +back in. The store's one-decision-per-pair rule deletes each undone link row, +so the pair splits on every surface from the monitor's next rebuild, as with +unlink. The drawer reports every merged source, which undoes every link, and +so does a report naming none. A fold recorded without its sides' own sources +falls back to its folded subtree, and a member several folds name takes the +union of what they record, since a refold unions into its own pair's fold +only. A resource the records no longer name (re-keyed after the links folded) +is replaced by the one holder no link folded; when no single holder qualifies, +which the recorder does not produce, the folds' subtrees decide, as before. A link joins only the pair its row names. Canonical-ID succession re-keys link endpoints with `UPDATE OR IGNORE` but moves `primary_id` @@ -7678,8 +7701,18 @@ monitor's registry and on one seeded from its listing, and `TestManualLinkFoldsRecordEachPairOnceAcrossRecordIngests` folds a TrueNAS system into a VM across repeated record ingests, and `TestManualLinkFoldRepeatedPairKeepsEarlierSources` refolds a side with fewer -sources than it first brought. `TestResourceAPIReportMergeExcludesRegistryLinkFolds` -keeps the handler on the registry's fold record. +sources than it first brought. `TestManualLinkFoldOwnSourcesComeFromTheFirstRecord` +keeps a holder's own sources from growing with what it takes in. +`TestReportedManualLinkFoldsDetachMembersCarryingTheSource` selects links for a +leaf, a holder, both and a cycle (a TrueNAS VM taking in a Proxmox storage and +a vSphere VM that is also linked to the storage), and +`TestReportedManualLinkFoldsLeaveNoReportedMemberJoined` checks that no +reported member stays joined to the resource through the links left, which the +subtree rule fails on that cycle. `TestResourceReportMergeSourceFilterDetachesChainMembers` +reports a Docker host linked into an agent linked into a VM through the API, +for the leaf, the holder, both, every source and no filter, on the monitor and +the REST surface. `TestResourceAPIReportMergeExcludesRegistryLinkFolds` keeps +the handler on the registry's fold record. ### Reconciled node-agent relink fold bookkeeping diff --git a/frontend-modern/browser-tests/email-rate-limit.cjs b/frontend-modern/browser-tests/email-rate-limit.cjs new file mode 100644 index 0000000000..53545ccdf2 --- /dev/null +++ b/frontend-modern/browser-tests/email-rate-limit.cjs @@ -0,0 +1,258 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const crypto = require('node:crypto'); +const { chromium, webkit } = require('playwright'); + +const root = '/workspace/frontend-modern'; +const output = '/workspace/tmp/email-rate-limit-proof'; +const runtimePath = 'frontend-modern/src/features/alerts/alertDestinationsModel.ts'; +const runtimePaths = [ + runtimePath, + 'frontend-modern/src/features/alerts/AlertEmailDestinationsSection.tsx', +]; +const baseEmail = { + enabled: true, + provider: '', + server: 'smtp.example.test', + port: 587, + username: 'ops@example.test', + password: '***REDACTED***', + from: 'pulse@example.test', + to: ['alerts@example.test'], + tls: false, + startTLS: true, + rateLimit: 17, + tagFilter: ['production'], + tagFilterMode: 'any', + minimumSeverity: 'warning', +}; + +(async () => { + fs.mkdirSync(output, { recursive: true }); + const playwrightVersion = require('playwright/package.json').version; + assert.equal( + playwrightVersion, + JSON.parse(fs.readFileSync('/workspace/tests/integration/package-lock.json')).packages[ + 'node_modules/@playwright/test' + ].version, + ); + const digests = Object.fromEntries( + runtimePaths.map((file) => [ + file, + crypto + .createHash('sha256') + .update(fs.readFileSync(path.join('/workspace', file))) + .digest('hex'), + ]), + ); + const { createServer } = await import(path.join(root, 'node_modules/vite/dist/node/index.js')); + const server = await createServer({ + root, + configFile: path.join(root, 'vite.config.ts'), + cacheDir: path.join(output, 'vite-cache'), + server: { host: '127.0.0.1', port: 5294, strictPort: true }, + }); + const results = []; + let browser; + let serverClosed = false; + try { + await server.listen(); + for (const [name, engine, width, height, dark] of [ + ['desktop-light', chromium, 1365, 900, false], + ['desktop-dark', chromium, 1365, 900, true], + ['phone-dark', webkit, 390, 844, true], + ['narrow-light', webkit, 320, 844, false], + ]) { + browser = await engine.launch( + engine === chromium + ? { headless: true, channel: 'chromium', args: ['--no-sandbox'] } + : { headless: true }, + ); + const browserVersion = browser.version(); + const page = await browser.newPage({ + viewport: { width, height }, + isMobile: width < 500, + hasTouch: width < 500, + locale: 'en-GB', + timezoneId: 'UTC', + colorScheme: dark ? 'dark' : 'light', + }); + page.setDefaultTimeout(15000); + let email = { ...baseEmail }; + const writes = []; + const unexpected = []; + const errors = []; + const checks = []; + const captures = []; + page.on('pageerror', (error) => errors.push(error.message)); + await page.addInitScript((isDark) => { + if (!isDark) return; + const apply = () => document.documentElement.classList.add('dark'); + if (document.documentElement) apply(); + else document.addEventListener('DOMContentLoaded', apply, { once: true }); + }, dark); + await page.route('**/*', async (route) => { + const request = route.request(); + const url = new URL(request.url()); + if (url.origin !== 'http://127.0.0.1:5294') { + unexpected.push('off-origin request'); + return route.abort(); + } + if (!url.pathname.startsWith('/api/')) return route.continue(); + const key = `${request.method()} ${url.pathname}`; + let body; + if (key === 'GET /api/notifications/email') body = email; + else if (key === 'GET /api/notifications/email-providers') body = []; + else if (key === 'GET /api/notifications/webhooks') body = []; + else if (key === 'GET /api/notifications/apprise') body = { enabled: false, targets: [] }; + else if (key === 'GET /api/alerts/deadman/config') + body = { pingUrl: '', configured: false }; + else if (key === 'GET /api/health') body = { csrfToken: 'fixture-only' }; + else if (key === 'PUT /api/notifications/email') { + const submitted = request.postDataJSON(); + assert.equal(submitted.password, baseEmail.password); + assert.equal(submitted.tls, false); + assert.equal(submitted.startTLS, true); + assert.deepEqual(submitted.tagFilter, ['production']); + assert.equal(submitted.tagFilterMode, 'any'); + assert.equal(submitted.minimumSeverity, 'warning'); + writes.push({ rateLimit: submitted.rateLimit, enabled: submitted.enabled }); + // Model the existing default only in the fixture, not in the client. + email = { ...submitted, rateLimit: submitted.rateLimit ?? 60 }; + body = { success: true }; + } else if (key === 'PUT /api/notifications/apprise') { + body = request.postDataJSON(); + } else if (key === 'PUT /api/alerts/deadman/config') { + body = { success: true, configured: false }; + } else { + unexpected.push(key); + return route.abort(); + } + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(body), + }); + }); + const open = async () => { + await page.goto('http://127.0.0.1:5294/browser-tests/email-rate-limit.html'); + await page.getByRole('button', { name: 'Save changes' }).waitFor(); + assert.equal( + await page.evaluate(() => document.documentElement.classList.contains('dark')), + dark, + ); + }; + const advanced = async () => { + await page.getByRole('button', { name: 'Show advanced options' }).click(); + return page.getByRole('spinbutton', { name: 'Rate limit' }); + }; + const save = async (expectedLimit) => { + const before = writes.length; + await page.getByRole('button', { name: 'Save changes' }).click(); + await page + .getByRole('status') + .filter({ hasText: /^Saved$/ }) + .waitFor(); + assert.equal(writes.length, before + 1); + assert.equal(writes.at(-1).rateLimit, expectedLimit); + assert.equal(email.rateLimit, expectedLimit); + }; + const check = async (label, run) => { + await run(); + checks.push(label); + }; + const capture = async (state) => { + assert.ok( + await page.evaluate(() => document.documentElement.scrollWidth <= innerWidth + 1), + 'no horizontal overflow', + ); + const file = `${name}-${state}.png`; + await page.screenshot({ path: path.join(output, file), fullPage: true }); + captures.push(file); + }; + await check('loaded non-default limit is visible', async () => { + await open(); + assert.equal(await (await advanced()).inputValue(), '17'); + }); + await capture('loaded'); + await check('unrelated edit preserves limit, mask, TLS and routing on the wire', async () => { + await page + .getByRole('textbox', { name: 'From address', exact: true }) + .fill('new@example.test'); + await save(17); + }); + await check('unrelated edit survives a reload with the same limit', async () => { + await open(); + assert.equal(await (await advanced()).inputValue(), '17'); + assert.equal( + await page.getByRole('textbox', { name: 'From address', exact: true }).inputValue(), + 'new@example.test', + ); + }); + await check('edited limit is sent exactly once', async () => { + await page.getByRole('spinbutton', { name: 'Rate limit' }).fill('12'); + await save(12); + }); + await capture('saved'); + await check('edited limit is read back after reload', async () => { + await open(); + assert.equal(await (await advanced()).inputValue(), '12'); + }); + await check('switching email off preserves its limit', async () => { + await page.getByRole('button', { name: 'Email notifications Enabled' }).click(); + await save(12); + assert.equal(email.enabled, false); + }); + await check('switched-off settings read back the same limit', async () => { + await open(); + await page.getByRole('button', { name: 'Show settings', exact: true }).click(); + assert.equal(await (await advanced()).inputValue(), '12'); + assert.equal( + await page.getByRole('button', { name: 'Send test email' }).isDisabled(), + true, + ); + await page.getByRole('spinbutton', { name: 'Rate limit' }).fill('9'); + await save(9); + assert.equal(email.enabled, false); + }); + await capture('off'); + await check('legacy default sentinel zero remains zero in the saved payload', async () => { + email = { ...baseEmail, rateLimit: 0 }; + await open(); + await save(0); + }); + await check('no test, retry, dismissal, off-origin request or browser error', async () => { + assert.deepEqual(unexpected, []); + assert.deepEqual(errors, []); + assert.equal(writes.length, 5); + }); + results.push({ name, browserVersion, width, height, dark, checks, writes, captures }); + await browser.close(); + browser = undefined; + } + } finally { + if (browser) await browser.close(); + await server.close(); + serverClosed = true; + fs.writeFileSync( + path.join(output, 'result.json'), + JSON.stringify( + { + result: results.length === 4 ? 'passed' : 'incomplete', + playwrightVersion, + content_sha256: digests, + browserClosed: true, + serverClosed, + results, + }, + null, + 2, + ) + '\n', + ); + } + console.log(JSON.stringify({ result: 'passed', groups: 36, captures: 12, serverClosed })); +})().catch((error) => { + console.error(error); + process.exitCode = 1; +}); diff --git a/frontend-modern/browser-tests/email-rate-limit.html b/frontend-modern/browser-tests/email-rate-limit.html new file mode 100644 index 0000000000..5c3b3dd9e9 --- /dev/null +++ b/frontend-modern/browser-tests/email-rate-limit.html @@ -0,0 +1,12 @@ + + + + + + SMTP rate-limit persistence verification + + +
+ + + diff --git a/frontend-modern/browser-tests/email-rate-limit.tsx b/frontend-modern/browser-tests/email-rate-limit.tsx new file mode 100644 index 0000000000..f0aa26a35c --- /dev/null +++ b/frontend-modern/browser-tests/email-rate-limit.tsx @@ -0,0 +1,54 @@ +// Existing SMTP editor and mutation owner with fixture-only persisted responses. +// No notification, retry, dismissal or licensed/native operation is performed. +import { Show, createSignal, onMount } from 'solid-js'; +import { render } from 'solid-js/web'; +import { AlertEmailDestinationsSection } from '../src/features/alerts/AlertEmailDestinationsSection'; +import { useAlertDestinationsState } from '../src/features/alerts/useAlertDestinationsState'; +import '../src/index.css'; + +function Fixture() { + const state = useAlertDestinationsState({ activeTab: () => 'destinations' }); + const [ready, setReady] = createSignal(false); + const [saving, setSaving] = createSignal(false); + const [status, setStatus] = createSignal(''); + onMount(async () => { + await state.loadDestinations(); + setReady(true); + }); + return ( +
+

Notifications

+ + setStatus('Unsaved changes')} + onTest={() => { + throw new Error('Sending a test notification is outside this fixture'); + }} + testing={false} + /> + +

{status()}

+ +
+ ); +} + +render(() => , document.getElementById('root')!); diff --git a/frontend-modern/browser-tests/webhook-composed.tsx b/frontend-modern/browser-tests/webhook-composed.tsx index 031b934b28..43471431bc 100644 --- a/frontend-modern/browser-tests/webhook-composed.tsx +++ b/frontend-modern/browser-tests/webhook-composed.tsx @@ -7,7 +7,10 @@ import '../src/index.css'; function Fixture() { const [operations, setOperations] = createSignal(0); - const unexpectedOperation = () => setOperations((count) => count + 1); + const unexpectedOperation = () => { + setOperations((count) => count + 1); + return false; + }; return (

Webhook setup

diff --git a/frontend-modern/browser-tests/webhook-save-acknowledgement.cjs b/frontend-modern/browser-tests/webhook-save-acknowledgement.cjs new file mode 100644 index 0000000000..58b1d817e5 --- /dev/null +++ b/frontend-modern/browser-tests/webhook-save-acknowledgement.cjs @@ -0,0 +1,382 @@ +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const crypto = require('node:crypto'); +const { chromium, webkit } = require('playwright'); +const root = '/workspace/frontend-modern'; +const output = '/workspace/tmp/webhook-save-proof/browser'; +const hash = (file) => crypto.createHash('sha256').update(fs.readFileSync(file)).digest('hex'); + +(async () => { + const binding = JSON.parse( + fs.readFileSync('/workspace/tmp/webhook-save-proof/browser-binding.json'), + ); + for (const [file, digest] of Object.entries(binding.content_sha256)) { + assert.equal(hash(path.join('/workspace', file)), digest); + } + const playwrightVersion = require('playwright/package.json').version; + assert.equal( + playwrightVersion, + JSON.parse(fs.readFileSync('/workspace/tests/integration/package-lock.json')).packages[ + 'node_modules/@playwright/test' + ].version, + ); + fs.mkdirSync(output, { recursive: true }); + process.chdir(root); + const { createServer } = await import(path.join(root, 'node_modules/vite/dist/node/index.js')); + const server = await createServer({ + root, + configFile: path.join(root, 'vite.config.ts'), + cacheDir: path.join(output, 'vite-cache'), + server: { host: '127.0.0.1', port: 5310, strictPort: true, watch: null }, + }); + const origin = 'http://127.0.0.1:5310'; + const results = [], + screenshots = [], + browserVersions = []; + let browser, + phase = 'server', + browserClosed = false, + serverClosed = false, + passed = false; + try { + await server.listen(); + for (const [name, engine, width, height, dark] of [ + ['desktop-light', chromium, 1365, 900, false], + ['desktop-dark', chromium, 1365, 900, true], + ['phone-dark', webkit, 390, 844, true], + ['narrow-light', webkit, 320, 740, false], + ]) { + browserClosed = false; + browser = await engine.launch( + engine === chromium + ? { headless: true, channel: 'chromium', args: ['--no-sandbox'] } + : { headless: true }, + ); + browserVersions.push({ + name, + engine: engine === chromium ? 'chromium' : 'webkit', + version: browser.version(), + }); + const context = await browser.newContext({ + viewport: { width, height }, + isMobile: width < 500, + hasTouch: width < 500, + locale: 'en-GB', + }); + await context.addInitScript((dark) => { + const apply = () => document.documentElement.classList.toggle('dark', dark); + if (document.documentElement) apply(); + else document.addEventListener('DOMContentLoaded', apply, { once: true }); + }, dark); + const saved = { + id: 'saved-hook', + name: 'Existing destination', + url: 'https://example.test/receiver', + method: 'POST', + enabled: true, + service: 'generic', + headers: { Authorization: '********' }, + customFields: { team: 'ops' }, + template: '{"message":"{{message}}"}', + mention: 'operators', + tagFilter: ['department-a'], + tagFilterMode: 'any', + minimumSeverity: 'warning', + }; + let inventory = [saved], + mode = 'pending', + release; + const writes = [], + unexpected = [], + errors = [], + checks = []; + await context.routeWebSocket(/.*/, (socket) => { + const url = new URL(socket.url()); + if (url.origin === 'ws://127.0.0.1:5310' && url.pathname === '/') socket.connectToServer(); + else { + unexpected.push({ type: 'websocket' }); + socket.close(); + } + }); + await context.route('**/*', async (route) => { + const req = route.request(), + url = new URL(req.url()); + if (url.origin !== origin) { + unexpected.push({ type: 'off-origin' }); + return route.abort(); + } + if (!url.pathname.startsWith('/api/')) return route.continue(); + if (req.method() === 'GET') { + if (url.pathname === '/api/health') return route.fulfill({ json: { status: 'healthy' } }); + if (url.pathname === '/api/notifications/webhooks') + return route.fulfill({ json: inventory }); + if (url.pathname === '/api/notifications/webhook-templates') + return route.fulfill({ json: [] }); + } + if ( + (req.method() === 'POST' && url.pathname === '/api/notifications/webhooks') || + (req.method() === 'PUT' && url.pathname === '/api/notifications/webhooks/saved-hook') + ) { + const write = { method: req.method(), path: url.pathname, payload: req.postDataJSON() }; + writes.push(write); + if (mode === 'pending') + await new Promise((resolve) => { + release = resolve; + }); + if (mode === 'failure') + return route.fulfill({ + status: 500, + json: { error: 'Synthetic configuration save unavailable' }, + }); + const accepted = { + ...write.payload, + id: req.method() === 'POST' ? `new-hook-${writes.length}` : 'saved-hook', + headers: { Authorization: '********' }, + }; + inventory = + req.method() === 'POST' + ? [...inventory, accepted] + : inventory.map((hook) => (hook.id === accepted.id ? accepted : hook)); + if (mode === 'lost-response') return route.abort('failed'); + return route.fulfill({ json: accepted }); + } + unexpected.push({ method: req.method(), path: url.pathname }); + return route.fulfill({ + status: 403, + json: { error: 'Fixture refuses Test, deletion and unrelated operations' }, + }); + }); + results.push({ name, checks, writes, unexpected, errors }); + const page = await context.newPage(); + page.on('pageerror', (error) => errors.push(error.message)); + page.setDefaultTimeout(8000); + page.setDefaultNavigationTimeout(45000); + const check = async (label, action) => { + phase = `${name}: ${label}`; + await action(); + checks.push(label); + }; + const click = async (locator) => { + if (width < 500) await locator.tap(); + else { + await locator.focus(); + await page.keyboard.press('Enter'); + } + }; + const capture = async (state) => { + const file = `${name}-${state}.png`; + await page.screenshot({ path: path.join(output, file) }); + screenshots.push({ + file, + state, + sha256: hash(path.join(output, file)), + viewport: { width, height }, + }); + }; + const noOverflow = async () => + assert.ok( + await page.evaluate(() => document.documentElement.scrollWidth <= innerWidth + 1), + ); + phase = `${name}: load actual editor`; + await page.goto(`${origin}/browser-tests/webhook-save-acknowledgement.html`); + await page.getByText('Existing destination', { exact: true }).first().waitFor(); + await click(page.getByRole('button', { name: '+ Add Webhook', exact: true })); + await page.getByLabel('Name', { exact: true }).fill('New destination'); + await page.getByLabel('Webhook URL', { exact: true }).fill('https://example.test/new'); + await page + .getByLabel('Custom header 1 value', { exact: true }) + .fill('application/custom+json'); + await page.getByLabel('Minimum alert severity', { exact: true }).selectOption('warning'); + await click(page.getByRole('button', { name: 'Add Webhook', exact: true })); + await check( + 'pending save keeps draft, locks conflicting actions and admits one write', + async () => { + await page.getByRole('button', { name: 'Saving…', exact: true }).waitFor(); + assert.equal( + await page.getByLabel('Name', { exact: true }).inputValue(), + 'New destination', + ); + assert.equal(await page.getByLabel('Name', { exact: true }).isEnabled(), false); + for (const label of ['Saving…', 'Cancel', 'Edit', 'Delete', 'Enabled', 'Disable All']) + assert.equal( + await page.getByRole('button', { name: label, exact: true }).isEnabled(), + false, + ); + await page.getByRole('button', { name: 'Saving…' }).evaluate((button) => { + button.click(); + button.click(); + }); + await page.waitForFunction( + () => document.querySelector('fieldset')?.getAttribute('aria-busy') === 'true', + ); + assert.equal(writes.length, 1); + await noOverflow(); + }, + ); + await capture('pending'); + mode = 'failure'; + release(); + await check( + 'failed create retains values and announces fixed uncertainty, without a replay', + async () => { + await page.getByRole('alert').waitFor(); + assert.ok((await page.getByRole('alert').innerText()).includes('another tab')); + assert.equal( + await page.getByLabel('Custom header 1 value').inputValue(), + 'application/custom+json', + ); + assert.equal( + await page.getByLabel('Minimum alert severity', { exact: true }).inputValue(), + 'warning', + ); + assert.equal(await page.getByLabel('Name', { exact: true }).isEnabled(), true); + assert.equal(writes.length, 1); + await noOverflow(); + }, + ); + await page.getByRole('alert').scrollIntoViewIfNeeded(); + await capture('failed-create'); + mode = 'success'; + await click(page.getByRole('button', { name: 'Add Webhook', exact: true })); + await check('explicit create retry closes only after accepted response', async () => { + await page.getByRole('button', { name: '+ Add Webhook', exact: true }).waitFor(); + assert.equal( + await page.getByLabel('Saved destination names').innerText(), + 'Existing destination, New destination', + ); + assert.equal(writes.length, 2); + assert.deepEqual(writes[0].payload, writes[1].payload); + assert.equal(await page.getByRole('alert').count(), 0); + }); + await click(page.getByRole('button', { name: 'Edit', exact: true }).first()); + await page.getByLabel('Name', { exact: true }).fill('Edited destination'); + mode = 'failure'; + await click(page.getByRole('button', { name: 'Update Webhook', exact: true })); + await check('failed edit keeps masking, template, tag and severity policy', async () => { + await page.getByRole('alert').waitFor(); + assert.equal(await page.getByLabel('Custom header 1 value').inputValue(), '********'); + assert.equal( + await page.getByLabel('Name', { exact: true }).inputValue(), + 'Edited destination', + ); + assert.equal( + await page.getByLabel('Minimum alert severity', { exact: true }).inputValue(), + 'warning', + ); + const payload = writes[2].payload; + for (const key of [ + 'headers', + 'customFields', + 'template', + 'mention', + 'tagFilter', + 'tagFilterMode', + 'minimumSeverity', + ]) + assert.deepEqual(payload[key], saved[key]); + assert.equal( + await page.getByLabel('Saved destination names').innerText(), + 'Existing destination, New destination', + ); + await noOverflow(); + }); + await page.getByRole('alert').scrollIntoViewIfNeeded(); + await capture('failed-edit'); + mode = 'success'; + await click(page.getByRole('button', { name: 'Update Webhook', exact: true })); + await check('accepted edit replaces inventory with acknowledged response', async () => { + await page.getByRole('button', { name: '+ Add Webhook', exact: true }).waitFor(); + assert.equal( + await page.getByLabel('Saved destination names').innerText(), + 'Edited destination, New destination', + ); + assert.deepEqual(writes[2].payload, writes[3].payload); + }); + await click(page.getByRole('button', { name: '+ Add Webhook', exact: true })); + await page.getByLabel('Name', { exact: true }).fill('Unconfirmed destination'); + await page + .getByLabel('Webhook URL', { exact: true }) + .fill('https://example.test/unconfirmed'); + mode = 'lost-response'; + await click(page.getByRole('button', { name: 'Add Webhook', exact: true })); + await check('lost response leaves the draft and makes no unrequested duplicate', async () => { + await page.getByRole('alert').waitFor(); + assert.equal( + await page.getByLabel('Name', { exact: true }).inputValue(), + 'Unconfirmed destination', + ); + assert.equal(writes.length, 5); + assert.ok(!(await page.getByRole('alert').innerText()).includes('not saved')); + const other = await context.newPage(); + await other.goto(`${origin}/browser-tests/webhook-save-acknowledgement.html`); + await other.getByText('Unconfirmed destination', { exact: true }).first().waitFor(); + assert.ok( + (await other.getByLabel('Saved destination names').innerText()).includes( + 'Unconfirmed destination', + ), + ); + await other.close(); + assert.equal(writes.length, 5); + assert.equal(new Set(inventory.map((hook) => hook.id)).size, inventory.length); + await noOverflow(); + }); + await page.getByRole('alert').scrollIntoViewIfNeeded(); + await capture('lost-response'); + await click(page.getByRole('button', { name: 'Cancel', exact: true })); + await click(page.getByRole('button', { name: '+ Add Webhook', exact: true })); + await check( + 'deliberate Cancel clears old draft and error without another write', + async () => { + assert.equal(await page.getByLabel('Name', { exact: true }).inputValue(), ''); + assert.equal(await page.getByRole('alert').count(), 0); + assert.equal(writes.length, 5); + assert.deepEqual(unexpected, []); + assert.deepEqual(errors, []); + await noOverflow(); + }, + ); + + await browser.close(); + browserClosed = true; + } + passed = true; + } catch (error) { + fs.writeFileSync( + path.join(output, 'failure.json'), + JSON.stringify({ phase, error: String(error) }, null, 2), + ); + throw error; + } finally { + if (browser && !browserClosed) { + await browser.close(); + browserClosed = true; + } + await server.close(); + serverClosed = true; + fs.writeFileSync( + path.join(output, 'result.json'), + JSON.stringify( + { + result: passed ? 'passed' : 'failed', + phase, + base_sha: binding.base_sha, + content_sha256: binding.content_sha256, + playwrightVersion, + browserVersions, + results, + screenshots, + browserClosed, + serverClosed, + scope: + 'Actual existing Webhook section/editor/mutation owner and API client with synthetic transport. No provider/notification, native persistence, deployment or release acceptance.', + }, + null, + 2, + ), + ); + } +})().catch((error) => { + process.stderr.write(`${error.stack || error}\n`); + process.exitCode = 1; +}); diff --git a/frontend-modern/browser-tests/webhook-save-acknowledgement.html b/frontend-modern/browser-tests/webhook-save-acknowledgement.html new file mode 100644 index 0000000000..760e19d8d0 --- /dev/null +++ b/frontend-modern/browser-tests/webhook-save-acknowledgement.html @@ -0,0 +1,12 @@ + + + + + + Webhook save acknowledgement verification + + +
+ + + diff --git a/frontend-modern/browser-tests/webhook-save-acknowledgement.tsx b/frontend-modern/browser-tests/webhook-save-acknowledgement.tsx new file mode 100644 index 0000000000..e485040ba5 --- /dev/null +++ b/frontend-modern/browser-tests/webhook-save-acknowledgement.tsx @@ -0,0 +1,32 @@ +// Actual destination section, editor, API client and mutation owner. The runner +// supplies synthetic local responses; no notification, provider or saved host. +import { render } from 'solid-js/web'; +import { AlertWebhookDestinationsSection } from '../src/features/alerts/AlertWebhookDestinationsSection'; +import { useAlertWebhookDestinationsState } from '../src/features/alerts/useAlertWebhookDestinationsState'; +import '../src/index.css'; + +function Fixture() { + const state = useAlertWebhookDestinationsState(); + return ( +
+

Notification destinations

+

Offline save-recovery check. No notification is sent.

+ + + {state + .webhooks() + .map((hook) => hook.name) + .join(', ')} + +
+ ); +} + +render(() => , document.getElementById('root')!); diff --git a/frontend-modern/browser-verification/226c5fee156bd820.json b/frontend-modern/browser-verification/226c5fee156bd820.json new file mode 100644 index 0000000000..4db0b243cc --- /dev/null +++ b/frontend-modern/browser-verification/226c5fee156bd820.json @@ -0,0 +1,114 @@ +{ + "version": 1, + "base_sha": "27576b90ff416cd16d1ef744cd299b83af4a2f36", + "verified_at": "2026-10-10T03:40:27.414721Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/alerts/AlertEmailDestinationsSection.tsx", + "frontend-modern/src/features/alerts/alertDestinationsModel.ts" + ], + "content_sha256": { + "frontend-modern/src/features/alerts/alertDestinationsModel.ts": "1af3cc1bfcc6202d1033f56f8982a1f785a65dc93b2fd1734e1cfe124748e18c", + "frontend-modern/src/features/alerts/AlertEmailDestinationsSection.tsx": "131354cd4a7c835973173ea391a01d912e58cad0969f520c2dfb805a984bcb47" + }, + "routes": [ + "Existing Alerts Notifications SMTP section via browser-tests/email-rate-limit.html (fixture only)" + ], + "viewports": [ + { + "width": 1365, + "height": 900 + }, + { + "width": 1365, + "height": 900 + }, + { + "width": 390, + "height": 844 + }, + { + "width": 320, + "height": 844 + } + ], + "states": [ + "Loaded non-default limit, saved unrelated edit, edited limit with reload, switched-off email with inspectable/editable settings, explicit legacy zero sentinel. Desktop light/dark and WebKit phone/narrow light." + ], + "interactions": [ + "Actual Email section, editor, destination mutation owner, payload builder and API client against synthetic persisted GET/PUT responses.", + "Each viewport preserves loaded 17 through an unrelated edit, saves 12 and reloads it, switches email off then inspects/edits/saves 9 without enabling email or the Test button, and preserves zero on the wire.", + "Thirty-six assertion groups and twelve individually inspected full-page captures. Masks, STARTTLS, tag routing and warning severity remain unchanged. No Test, retry, dismissal, Relay/native operation or external/provider request." + ], + "screenshots": [ + { + "file": "desktop-light-loaded.png", + "sha256": "bec915910540fd8f0936531b5e5a47723baf2eeeac0be774b839a3b5738da4da" + }, + { + "file": "desktop-light-saved.png", + "sha256": "555902f2581fff7237f187fe9d34908ad5c1c1dc06f11ce3e856012f3c974455" + }, + { + "file": "desktop-light-off.png", + "sha256": "54a29af969607ac81f63dee0c1120b7a508023bdba154cf790b1d31cb5153811" + }, + { + "file": "desktop-dark-loaded.png", + "sha256": "f9ca2e82f774dbeee06f8d310da6832bbb612d501bed7e72fd7ac7c1da0d15ef" + }, + { + "file": "desktop-dark-saved.png", + "sha256": "47575ae1a3fe600054e2dbb5a42f93b7d13049495b7b656ba1c9a8b915110a24" + }, + { + "file": "desktop-dark-off.png", + "sha256": "2f0186f278b17996175e564b98c42670fbc01f393b4b7d0a7941ccc2324086c2" + }, + { + "file": "phone-dark-loaded.png", + "sha256": "51bfbe5bda8b7a0a434a8c7de684c76b4995c6ab265fc3ffda66a3836c7482da" + }, + { + "file": "phone-dark-saved.png", + "sha256": "9f70f6c89242953b9e301b70f094c01b3cf3dae4fc2b5be831f39eb3106eb89d" + }, + { + "file": "phone-dark-off.png", + "sha256": "689c31341391f6102646d60a43e58089754ba11251d1725378fa2fd25e3ea4ba" + }, + { + "file": "narrow-light-loaded.png", + "sha256": "43bbf97b82cd562d151f80c04e8cc75d3b74003c5330c3533a923ca0a8a1aea1" + }, + { + "file": "narrow-light-saved.png", + "sha256": "f48237f0ebe5a6365cae3c9b52c7a86efe71660cb733bc4301818f2c5cd9fa24" + }, + { + "file": "narrow-light-off.png", + "sha256": "902b2cb9af2dba6429c1a64824a4b9dd9bc9962c271dc4b7bf4314eddf5f3d38" + } + ], + "tools": { + "playwright": "1.56.1", + "browser_versions": [ + { + "name": "desktop-light", + "version": "141.0.7390.37" + }, + { + "name": "desktop-dark", + "version": "141.0.7390.37" + }, + { + "name": "phone-dark", + "version": "26.0" + }, + { + "name": "narrow-light", + "version": "26.0" + } + ] + } +} diff --git a/frontend-modern/browser-verification/ca7dfc37d900399f.json b/frontend-modern/browser-verification/ca7dfc37d900399f.json new file mode 100644 index 0000000000..803cadf46f --- /dev/null +++ b/frontend-modern/browser-verification/ca7dfc37d900399f.json @@ -0,0 +1,52 @@ +{ + "version": 1, + "base_sha": "fcaaabe25bbd8766c2eea6f72fbac62b92ea26a0", + "result": "passed", + "verified_at": "2026-10-10T02:54:43Z", + "changed_paths": [ + "frontend-modern/src/components/Alerts/WebhookConfig.tsx", + "frontend-modern/src/components/Alerts/WebhookConfigForm.tsx", + "frontend-modern/src/components/Alerts/WebhookConfigList.tsx", + "frontend-modern/src/components/Alerts/useWebhookConfigState.ts", + "frontend-modern/src/features/alerts/AlertWebhookDestinationsSection.tsx", + "frontend-modern/src/features/alerts/useAlertWebhookDestinationsState.ts" + ], + "content_sha256": { + "frontend-modern/src/components/Alerts/WebhookConfig.tsx": "bdc26fd7e7ff60c4e5dee07e65a684cd730e1b840c0a4074c40a3d3127f8734f", + "frontend-modern/src/components/Alerts/WebhookConfigForm.tsx": "97ca12ed9c7e7c0fe93a6d5c9eb0daed596ce859fd884f2f56316ab2d91512d3", + "frontend-modern/src/components/Alerts/WebhookConfigList.tsx": "3caf78928a5b28947eaba68a335aa7404262d0cafba3a37cefb8095fafc9b545", + "frontend-modern/src/components/Alerts/useWebhookConfigState.ts": "b3f3888bc6a87c2894ccf40dd936a4076a7b910982743037deb57730f294996f", + "frontend-modern/src/features/alerts/AlertWebhookDestinationsSection.tsx": "002609ca384cec8e6275392ce1c0617088ad64c9f06e621edf93bfe11818ad5b", + "frontend-modern/src/features/alerts/useAlertWebhookDestinationsState.ts": "50e7843784251c2d47cdb20886df9f94b1a066f2dac788234a129bd4ae1a7184" + }, + "routes": [ + "Existing Notifications webhook section in local /browser-tests/webhook-save-acknowledgement.html", + "Existing Notifications editor and real mutation owner/API client; no shipped new route" + ], + "states": [ + "Pending create, disabled editor/conflicting list controls, one request", + "Failed create/update preserves the same draft and masks/template/routing", + "Accepted create/update incorporates canonical response before clearing draft", + "Accepted synthetic create followed by lost response: draft retained, another tab confirms stored fixture without duplicate", + "Explicit retry and deliberate cancellation; desktop light/dark and phone dark/narrow light" + ], + "interactions": [ + "28 assertion groups: keyboard Enter and phone touch saves/cancel, request counts/payloads, failed-save recovery, masked update, another-tab reconciliation", + "16 inspected screenshots; fixed inline warning readable, fieldsets/controls remain within 320px viewport; lower phone footer reached by scrolling before each interaction" + ], + "viewports": [ + { + "width": 1365, + "height": 900 + }, + { + "width": 390, + "height": 844 + }, + { + "width": 320, + "height": 740 + } + ], + "notes": "Playwright 1.56.1; Chromium 141.0.7390.37 and WebKit 26.0; browser/server closed. Actual existing components with synthetic local API responses. No notification/native persistence/provider/delivery/release acceptance. Earlier label/CSRF-setup fixture failures retained separately." +} diff --git a/frontend-modern/public/docs/API.md b/frontend-modern/public/docs/API.md index 12ee36475f..1b5a6e56d8 100644 --- a/frontend-modern/public/docs/API.md +++ b/frontend-modern/public/docs/API.md @@ -634,39 +634,122 @@ Version fields are returned as plain semantic versions (no leading `v`). Returns a small public config payload (update channel, auto-update enabled). ### List Nodes -`GET /api/config/nodes` +`GET /api/config/nodes` (admin, `settings:read`) + +This administrative inventory covers saved **PVE, PBS and PMG connections**, +not individual VMs or agents. Keep its response private. Use the returned +connection `id` for updates, deletion and tests; a display name, VMID or another +installation's node name is not that ID. Re-read the inventory before a change +rather than guessing an ID from a list position. ### Add Node -`POST /api/config/nodes` +`POST /api/config/nodes` (admin, `settings:write`) + +Adding a connection saves credentials and enables collection; it is not a +passive validation step. Use an existing dedicated, least-privilege platform +token. The **Pulse API token** in the private header file authorises this +configuration request; the **Proxmox token** in the JSON body authorises reads +from the monitored platform. They are different credentials. Do not use a root +password or widen a department-scoped token to make denied peer reads succeed; +follow the [scoped-access checks](TROUBLESHOOTING.md#check-permissions-proxmox). +For PBS and PMG, use their [PBS](PBS.md#method-1-api-only-connection-recommended) +and [PMG](MAIL_GATEWAY.md) token guidance. PBS password-based onboarding can +also try to create a token and ACLs on the PBS server; it is not read-only. + +Prepare the request in a private editor, not a credential-bearing shell +assignment, here-document or command argument. Use the same Bash session as +the [private-response helper](#api-token-recommended): + +```bash +node_request_dir=$(umask 077; mktemp -d "${TMPDIR:-/tmp}/pulse-node.XXXXXX") && +node_request_file="$node_request_dir/node.json" && +(umask 077; set -C; : > "$node_request_file") && +vi "$node_request_file" +``` + +If preparation or the editor fails, stop. In that editor, enter the following +JSON, replacing the example hostname, connection name, full platform token ID +(`user@realm!token`) and token-secret placeholder with your intended values: + ```json { "type": "pve", - "name": "Proxmox 1", - "host": "https://198.51.100.10:8006", - "user": "root@pam", - "password": "password" + "name": "PVE connection A", + "host": "https://pve.example.net:8006", + "tokenName": "pulse-monitor@pve!pulse-readonly", + "tokenValue": "", + "verifySSL": true } ``` -### Test Connection -`POST /api/config/nodes/test-connection` -Validate credentials before saving. +Set `verifySSL` explicitly to `true`: omission on these new-connection and +test request paths does **not** enable certificate verification. The example +requires a certificate trusted by the Pulse server for that hostname. For a +self-signed/private certificate, also supply `fingerprint` with the SHA-256 +certificate fingerprint verified through an independent trusted channel. +A fingerprint merely fetched from the endpoint is not established trust. +Do not clear an existing pin, set `verifySSL` to `false`, or change to HTTP to +work around a certificate failure. -### Test Node Config (Validation Only) -`POST /api/config/nodes/test-config` -Validates node config without saving. +After reviewing this one intended configuration change, send the file: + +```bash +pulse_api POST /api/config/nodes < "$node_request_file" +``` + +The helper retains the response privately and makes one bounded request, +without retries. A lost response can follow a successful save: re-read the +saved inventory before deciding whether another request is needed. HTTP success +does not establish fresh readings for every node, guest, backup or History view. +Keep both request and response files out of reports and shared repositories; +remove the temporary request under your credential policy when no longer needed. + +### Test Connection +`POST /api/config/nodes/test-connection` (admin, `settings:write`) + +Tests supplied credentials with live platform requests without saving the +connection. Use the same private JSON-file transport and explicit TLS settings, +only when a live connection test is appropriate; it is not a required precursor +to filing a report. Success covers the reads performed by that test, not all +collection permissions, guest recovery or sustained monitoring. + +### Test Node Config (No Save, Live Request) +`POST /api/config/nodes/test-config` (admin, `settings:write`) + +This also contacts the platform using the supplied credentials; **no save** +does not mean offline schema validation. Do not run either test merely to +collect new evidence during a freeze/thaw, backup or unresponsive-host incident. +Use existing observations and the [safe diagnostic guidance](TROUBLESHOOTING.md#collect-diagnostics-safely). ### Update Node -`PUT /api/config/nodes/{id}` +`PUT /api/config/nodes/{id}` (admin, `settings:write`) + +Review the current connection and submit only the intended supported changes +from a private JSON file. Do not copy masked credentials from a GET response +back as new secrets. Credential rotation and trust changes are separate +decisions; preserve the existing credential and fingerprint when not changing +them. A response failure does not prove the update was unapplied. ### Delete Node -`DELETE /api/config/nodes/{id}` +`DELETE /api/config/nodes/{id}` (admin, `settings:write`) + +Removes the Pulse connection, not the platform workloads or the upstream token. +Collection and related alert coverage can stop. Check the exact connection and +arrange any needed independent coverage before an intentional removal; do not +delete and re-add a connection to diagnose missing readings or identity mixing. ### Test Node (Legacy) -`POST /api/config/nodes/{id}/test` +`POST /api/config/nodes/{id}/test` (admin, `settings:write`) + +Uses the saved connection for a live test. It is not a passive read of its +last result or a guarantee that ordinary collection recovered. ### Refresh Cluster Nodes -`POST /api/config/nodes/{id}/refresh-cluster` +`POST /api/config/nodes/{id}/refresh-cluster` (admin, `settings:write`) + +Actively rediscovers a saved PVE connection's cluster membership. It does not +select authorised nodes, grant missing permissions or repair an unrelated +standalone installation. Keep intentional token restrictions intact. ### Export Configuration `POST /api/config/export` (instance admin for the default organization, tenant diff --git a/frontend-modern/public/docs/CONFIGURATION.md b/frontend-modern/public/docs/CONFIGURATION.md index b7de212224..5c8e8f591c 100644 --- a/frontend-modern/public/docs/CONFIGURATION.md +++ b/frontend-modern/public/docs/CONFIGURATION.md @@ -936,32 +936,111 @@ an agent assignment remain available in every edition. ### ICMP probe privileges -ICMP probes run the system `ping` binary, which needs the `CAP_NET_RAW` -capability. The systemd unit written by the installer hardens the service -with `NoNewPrivileges=true`, which strips ping's setuid bit and file -capabilities, so the unit also grants the capability directly with -`AmbientCapabilities=CAP_NET_RAW`. Units written by older versions of the -installer lack that line, and ICMP probes fail with -`icmp probe failed: ping: socktype: SOCK_RAW ... missing cap_net_raw+p capability`. +ICMP checks execute the system `ping` binary **on the selected observation +host**. A local permission error (`Operation not permitted`, `cap_net_raw` or +`icmp probe blocked`) is not evidence that the target is down, nor proof that +one particular service setting caused it. First distinguish **This Pulse +server** from an external agent observation. Changing the server's unit cannot +repair an agent's permissions on another host. -To fix an existing install, either re-run the install script (it rewrites -the unit) or add the capability as an override: +For Linux systemd servers, the current installer grants both +`AmbientCapabilities=CAP_NET_RAW` and `CapabilityBoundingSet=CAP_NET_RAW`, while +keeping `NoNewPrivileges=true`. This allows the child `ping` to inherit the +capability without gaining it through setuid or file capabilities. Older units +can lack the ambient grant. **Updates preserve an existing service unit**; +rerunning an installer is not a reliable unit repair, even if an older error +message recommends it. Do not reinstall over a working data directory. + +#### Inspect the actual systemd server first + +On the machine running the Pulse server, read only these effective properties. +For a Pulse LXC, run this **inside the Pulse LXC**, not on the Proxmox host or a +monitored guest. The example uses `pulse.service`; substitute your actual server +unit throughout, such as `pulse-backend.service` on some older installs, +**not pulse-agent.service**: ```bash -systemctl edit pulse # pulse-backend on ProxmoxVE community-script installs +timeout --signal=TERM --kill-after=1s 8s systemctl show pulse.service \ + --property=LoadState,ActiveState,User,NoNewPrivileges,AmbientCapabilities,CapabilityBoundingSet ``` +Interpret the result only after a successful read with `LoadState=loaded`. +A missing unit, failed or timed-out read is **unknown**, not an empty capability +set or a stopped server. This command does not print environment values, +credentials or full unit contents; keep those private. + +| Existing observation | What follows | +| --- | --- | +| `NoNewPrivileges=yes`, ceiling includes `cap_net_raw`, ambient grant missing | An older non-root server unit may need the narrow override below. | +| Both capability properties already include `cap_net_raw` | Do not add the grant again. Check the selected observation host, installed ping and its OS/container restrictions through normal administration. | +| Ceiling excludes `cap_net_raw`, or the deployment deliberately forbids it | An ambient line alone cannot overcome the ceiling. Preserve that restriction; do not replace the capability set with a permissive one. | +| `User=` is empty/root, or the service is custom | This is not the standard non-root server recipe. Review its existing privilege and data-access requirements using [manual-service guidance](INSTALL.md#2-bare-metal--systemd). Do not change the user or grant broader privileges just to clear the error. | + +#### Narrow override for an older non-root server + +Use this only for an **active** non-root server whose capability ceiling already +permits `cap_net_raw`, whose ambient grant is missing, and where granting it is +consistent with the deployment's intended policy. Keep deliberately inactive +services inactive. Record the existing drop-in locally before editing; it may +contain private configuration. Preserve every unrelated setting: + +```bash +sudo systemctl edit pulse.service +``` + +Add just this service setting to the drop-in: + ```ini [Service] AmbientCapabilities=CAP_NET_RAW ``` -Then `systemctl daemon-reload && systemctl restart pulse`. +Keep `NoNewPrivileges` enabled; do not run Pulse as root, set file capabilities +on the system-wide ping binary or remove sandbox settings. Do not reset or widen +`CapabilityBoundingSet` to make this example apply to a different profile. -Docker installs are unaffected: Docker's default capability set includes -`NET_RAW`. If you run the container with `--cap-drop=ALL`, add -`--cap-add=NET_RAW` to keep ICMP probes working. TCP and HTTP/HTTPS probes -need no special privileges. +```bash +sudo systemctl daemon-reload +``` + +If editing or reloading fails, stop. Repeat the read-only property check above +to confirm the intended ambient grant and preserved ceiling/hardening. Reload +changes systemd's configuration, **not the running process's capabilities**. + +Schedule the restart as maintenance, outside backups or other guest operations. +**Pulse monitoring and alerts are unavailable during the restart**; arrange +independent outage coverage. Only after the checks pass, for the server that +was active beforehand: + +```bash +sudo systemctl restart pulse.service && \ + timeout --signal=TERM --kill-after=1s 8s systemctl is-active pulse.service +``` + +`active` proves service startup, not ICMP recovery or notification delivery. +Check the next normally scheduled observation from the **same location**; +do not induce a workload failure or another notification to prove recovery. +A failed/unknown restart needs normal recovery, not repeated restarts. To undo +this change, edit only the added line, preserve other drop-in content and +reload; do not use `systemctl revert`, which can remove unrelated overrides. +Apply any further restart only under the same maintenance and prior-active +conditions. Preserve the data directory and encryption key throughout. + +#### Containers and external agents + +Docker's default capability set includes `NET_RAW`, but that does not prove ICMP +works in every rootless, non-root or restricted container. For an intentionally +restricted deployment, review only the selected observation host's ping/socket +permissions and saved deployment. Do not use `--privileged`, `--cap-add=ALL`, +disable user namespaces or recreate a container as a diagnostic. A deliberate +capability change must use the deployment's normal maintenance route while +preserving data and configuration; the systemd override above does not apply. + +External agents have their own OS/service requirements; see +[Agent Security](AGENT_SECURITY.md). Do not grant command execution or widen +agent privileges merely because the server's ICMP check failed. TCP and +HTTP/HTTPS checks need no raw-socket capability, but measure different behaviour; +switching protocol or observation location is not proof that ICMP recovered. --- diff --git a/frontend-modern/public/docs/UNIFIED_AGENT.md b/frontend-modern/public/docs/UNIFIED_AGENT.md index 201e12ef91..717e743b9e 100644 --- a/frontend-modern/public/docs/UNIFIED_AGENT.md +++ b/frontend-modern/public/docs/UNIFIED_AGENT.md @@ -979,6 +979,50 @@ private, and report evidence as unavailable if safe collection is not possible. QNAP and Unraid log locations are described in the [space troubleshooting section](#installer-fails-with-not-enough-free-disk-space). +### Agent lookup returns HTTP 401 or 403 + +At `/api/agents/agent/lookup`, Pulse authenticates the request **before** +searching for the host. **HTTP 401** is an authentication rejection; +**HTTP 403** with `missing_scope` means the authenticated token lacks +`agent:report`. An authenticated lookup can return **HTTP 404** when no matching +host exists. A 401 alone does not identify a mistyped, expired or revoked token, +the wrong Pulse instance, or an upstream authentication layer. + +The silent token prompt only keeps the secret out of the copied command and +shell history; it does not change reporting permissions. The normal +**Settings → Infrastructure → Install on a host** flow issues a monitoring +token. It does not use `--enroll`; the separate cluster deployment flow exchanges +a bootstrap token for a runtime token. Do not substitute a bootstrap token or +assume an enrolment-specific correction applies to a normal host installation. + +Use the [bounded agent log reader](#collect-agent-logs-safely) on the affected +agent host, or inspect an already retained excerpt privately. Distinguish: + +- `remote_config_client` reporting a lookup failure: this concerns configuration + retrieval and does not establish whether ordinary host reports were accepted. +- **Pulse rejected this agent's API token** from host reporting: keep its HTTP + status and redacted error, separately from the lookup failure. + +A running local service or successful-looking installer output does not prove +that Pulse accepted a fresh report. No host-report error in the selected log +window is inconclusive, not proof of successful reporting. Do not restart, +re-enrol, delete identity or enable debug logging to manufacture that evidence. + +In your signed-in administrator session on the **same Pulse instance** the +agent targets, check the intended record under **Settings → Security → API +Tokens** without revealing or copying its secret. Its presence, expiry and +reporting scope help distinguish server-side rejection, but do not prove that +the running service is using that record. Keep service environments, token +files and connection files private; do not paste a credential into a command, +URL or issue. Do not grant administrator scope, change Proxmox permissions or +disable authentication/TLS to test a guess. + +If reporting this problem, say whether the existing error is lookup-only or +also affects host reporting, and share only the failure stage, HTTP status and +manually redacted error line. Keep the installation and saved credentials +unchanged until the failing stage is understood; a 401 is not itself a reason +to reinstall or rotate every token. + ### pfSense service disabled after a major upgrade Pulse installs two service files on pfSense: the FreeBSD rc.d service at diff --git a/frontend-modern/src/components/Alerts/WebhookConfig.persistence.test.tsx b/frontend-modern/src/components/Alerts/WebhookConfig.persistence.test.tsx new file mode 100644 index 0000000000..0e162351d9 --- /dev/null +++ b/frontend-modern/src/components/Alerts/WebhookConfig.persistence.test.tsx @@ -0,0 +1,227 @@ +import { createSignal } from 'solid-js'; +import { cleanup, fireEvent, render, renderHook, screen, waitFor } from '@solidjs/testing-library'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { NotificationsAPI, type Webhook } from '@/api/notifications'; +import { notificationStore } from '@/stores/notifications'; +import { useAlertWebhookDestinationsState } from '@/features/alerts/useAlertWebhookDestinationsState'; +import { WebhookConfig } from './WebhookConfig'; + +vi.mock('@/api/notifications', () => ({ + NotificationsAPI: { + getWebhookTemplates: vi.fn(), + createWebhook: vi.fn(), + updateWebhook: vi.fn(), + }, +})); +vi.mock('@/stores/notifications', () => ({ + notificationStore: { success: vi.fn(), error: vi.fn() }, +})); +vi.mock('@/utils/logger', () => ({ logger: { error: vi.fn() } })); + +const savedWebhook: Webhook = { + id: 'saved-hook', + name: 'Existing destination', + url: 'https://example.test/receiver', + method: 'POST', + enabled: true, + service: 'generic', + headers: { Authorization: '********' }, + customFields: { team: 'ops' }, + template: '{"message":"{{message}}"}', + mention: 'operators', + tagFilter: ['department-a'], + tagFilterMode: 'any', + minimumSeverity: 'warning', +}; + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason: Error) => void; + const promise = new Promise((yes, no) => { + resolve = yes; + reject = no; + }); + return { promise, resolve, reject }; +} + +function mount(initial: Webhook[] = []) { + const { result: owner } = renderHook(() => { + const [webhooks, setWebhooks] = createSignal(initial); + return useAlertWebhookDestinationsState({ webhooks, setWebhooks, autoLoad: false }); + }); + render(() => ( + + )); + return owner; +} + +function openNewDraft() { + fireEvent.click(screen.getByRole('button', { name: '+ Add Webhook' })); + fireEvent.input(screen.getByLabelText('Name'), { target: { value: 'New destination' } }); + fireEvent.input(screen.getByLabelText('Webhook URL'), { + target: { value: 'https://example.test/new' }, + }); +} + +describe('webhook persistence acknowledgement', () => { + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(NotificationsAPI.getWebhookTemplates).mockResolvedValue([]); + }); + afterEach(cleanup); + + it('keeps the draft and locks conflicting controls until a single create is acknowledged', async () => { + const pending = deferred(); + vi.mocked(NotificationsAPI.createWebhook).mockReturnValue(pending.promise); + const owner = mount([savedWebhook]); + openNewDraft(); + const save = screen.getByRole('button', { name: 'Add Webhook' }); + fireEvent.click(save); + fireEvent.click(save); + + expect(NotificationsAPI.createWebhook).toHaveBeenCalledTimes(1); + expect(screen.getByLabelText('Name')).toHaveValue('New destination'); + expect(screen.getByLabelText('Name')).toBeDisabled(); + expect(screen.getByRole('button', { name: 'Saving…' })).toBeDisabled(); + for (const name of ['Cancel', 'Edit', 'Delete', 'Disable All', 'Enabled']) { + expect(screen.getByRole('button', { name })).toBeDisabled(); + } + expect(owner.webhooks()).toEqual([savedWebhook]); + + pending.resolve({ ...savedWebhook, id: 'new-hook', name: 'New destination' }); + await waitFor(() => expect(screen.queryByLabelText('Name')).not.toBeInTheDocument()); + expect(owner.webhooks().map((hook) => hook.id)).toEqual(['saved-hook', 'new-hook']); + expect(notificationStore.success).toHaveBeenCalledTimes(1); + }); + + it('retains a failed create draft and requires an explicit retry before clearing it', async () => { + vi.mocked(NotificationsAPI.createWebhook).mockRejectedValueOnce(new Error('Save unavailable')); + const owner = mount(); + openNewDraft(); + fireEvent.input(screen.getByLabelText('Custom header 1 value'), { + target: { value: 'application/custom+json' }, + }); + fireEvent.click(screen.getByRole('button', { name: 'Add Webhook' })); + + await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('Could not confirm')); + expect(screen.getByLabelText('Name')).toHaveValue('New destination'); + expect(screen.getByLabelText('Webhook URL')).toHaveValue('https://example.test/new'); + expect(screen.getByLabelText('Custom header 1 value')).toHaveValue('application/custom+json'); + expect(screen.getByLabelText('Name')).toBeEnabled(); + expect(owner.webhooks()).toEqual([]); + expect(NotificationsAPI.createWebhook).toHaveBeenCalledTimes(1); + expect(notificationStore.success).not.toHaveBeenCalled(); + + const retry = deferred(); + vi.mocked(NotificationsAPI.createWebhook).mockReturnValueOnce(retry.promise); + fireEvent.click(screen.getByRole('button', { name: 'Add Webhook' })); + // Starting a deliberate retry clears the old warning, not the draft. + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(screen.getByLabelText('Name')).toHaveValue('New destination'); + expect(screen.getByLabelText('Name')).toBeDisabled(); + expect(screen.getByRole('button', { name: 'Saving…' })).toBeDisabled(); + expect(owner.webhooks()).toEqual([]); + expect(notificationStore.success).not.toHaveBeenCalled(); + expect(NotificationsAPI.createWebhook).toHaveBeenCalledTimes(2); + + retry.resolve({ ...savedWebhook, id: 'new-hook', name: 'Accepted destination' }); + await waitFor(() => expect(screen.queryByLabelText('Name')).not.toBeInTheDocument()); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(notificationStore.success).toHaveBeenCalledTimes(1); + expect(vi.mocked(NotificationsAPI.createWebhook).mock.calls[1][0]).toEqual( + vi.mocked(NotificationsAPI.createWebhook).mock.calls[0][0], + ); + expect(owner.webhooks()[0].name).toBe('Accepted destination'); + }); + + it('retains masked credentials, routing and payload edits on update failure, then accepts the response', async () => { + const pending = deferred(); + vi.mocked(NotificationsAPI.updateWebhook).mockReturnValueOnce(pending.promise); + const owner = mount([savedWebhook]); + fireEvent.click(screen.getByRole('button', { name: 'Edit' })); + fireEvent.input(screen.getByLabelText('Name'), { target: { value: 'Edited destination' } }); + const draft = screen.getByLabelText('Name'); + fireEvent.click(screen.getByRole('button', { name: 'Update Webhook' })); + pending.reject(new Error('Save unavailable')); + + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()); + expect(screen.getByLabelText('Name')).toBe(draft); + expect(draft).toHaveValue('Edited destination'); + expect(screen.getByLabelText('Custom header 1 value')).toHaveValue('********'); + expect(owner.webhooks()).toEqual([savedWebhook]); + const [id, payload] = vi.mocked(NotificationsAPI.updateWebhook).mock.calls[0]; + expect(id).toBe(savedWebhook.id); + expect(payload).toMatchObject({ + ...savedWebhook, + name: 'Edited destination', + }); + + vi.mocked(NotificationsAPI.updateWebhook).mockResolvedValueOnce({ + ...savedWebhook, + name: 'Accepted edit', + enabled: false, + }); + fireEvent.click(screen.getByRole('button', { name: 'Update Webhook' })); + await waitFor(() => expect(screen.queryByLabelText('Name')).not.toBeInTheDocument()); + expect(owner.webhooks()[0]).toEqual({ ...savedWebhook, name: 'Accepted edit', enabled: false }); + expect(vi.mocked(NotificationsAPI.updateWebhook).mock.calls[1]).toEqual([id, payload]); + }); + + it('allows cancellation after a failure without an automatic save or stale error on reopening', async () => { + vi.mocked(NotificationsAPI.createWebhook).mockRejectedValue(new Error('Save unavailable')); + mount(); + openNewDraft(); + fireEvent.click(screen.getByRole('button', { name: 'Add Webhook' })); + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()); + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })); + fireEvent.click(screen.getByRole('button', { name: '+ Add Webhook' })); + expect(screen.getByLabelText('Name')).toHaveValue(''); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(NotificationsAPI.createWebhook).toHaveBeenCalledTimes(1); + }); + + it('handles a rejected callback with fixed guidance, without putting its text in the editor', async () => { + const privateDetail = 'synthetic receiver-controlled detail'; + render(() => ( + { + throw new Error(privateDetail); + }} + onUpdate={async () => false} + onDelete={() => {}} + onTest={() => {}} + /> + )); + openNewDraft(); + fireEvent.click(screen.getByRole('button', { name: 'Add Webhook' })); + await waitFor(() => + expect(screen.getByRole('alert')).toHaveTextContent('Check the configured destinations'), + ); + expect(screen.getByRole('alert')).not.toHaveTextContent(privateDetail); + expect(screen.getByLabelText('Name')).toHaveValue('New destination'); + }); + + it('returns true only after an accepted create or update and false on failure', async () => { + const { result: owner } = renderHook(() => + useAlertWebhookDestinationsState({ autoLoad: false }), + ); + vi.mocked(NotificationsAPI.createWebhook).mockResolvedValue(savedWebhook); + expect(await owner.addWebhook(savedWebhook)).toBe(true); + vi.mocked(NotificationsAPI.updateWebhook).mockRejectedValue(new Error('Save unavailable')); + expect(await owner.updateWebhook(savedWebhook)).toBe(false); + expect(owner.webhooks()).toEqual([savedWebhook]); + vi.mocked(NotificationsAPI.updateWebhook).mockResolvedValue({ + ...savedWebhook, + enabled: false, + }); + expect(await owner.updateWebhook(savedWebhook)).toBe(true); + }); +}); diff --git a/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx b/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx index b58eb327c7..18c8d4a3a5 100644 --- a/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx +++ b/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx @@ -110,8 +110,8 @@ describe('WebhookConfig', () => { let onTestMock: ReturnType any>>; beforeEach(() => { - onAddMock = vi.fn(); - onUpdateMock = vi.fn(); + onAddMock = vi.fn().mockResolvedValue(true); + onUpdateMock = vi.fn().mockResolvedValue(true); onDeleteMock = vi.fn(); onTestMock = vi.fn(); getWebhookTemplatesMock.mockReset(); @@ -630,7 +630,7 @@ describe('WebhookConfig', () => { expect(onAddMock).not.toHaveBeenCalled(); }); - it('resets form and closes panel after adding a webhook', () => { + it('resets form and closes panel after adding a webhook', async () => { render(() => ( { fireEvent.click(screen.getByText('Add Webhook')); - // Form should close, "+ Add Webhook" should reappear - expect(screen.getByText('+ Add Webhook')).toBeInTheDocument(); + // Form closes only after the owner acknowledges the save. + await waitFor(() => expect(screen.getByText('+ Add Webhook')).toBeInTheDocument()); expect(screen.queryByText('Service Type')).not.toBeInTheDocument(); }); diff --git a/frontend-modern/src/components/Alerts/WebhookConfig.tsx b/frontend-modern/src/components/Alerts/WebhookConfig.tsx index a2be7425a9..b7a3cde737 100644 --- a/frontend-modern/src/components/Alerts/WebhookConfig.tsx +++ b/frontend-modern/src/components/Alerts/WebhookConfig.tsx @@ -9,8 +9,8 @@ import { useWebhookConfigState } from './useWebhookConfigState'; export interface WebhookConfigProps { webhooks: Webhook[]; - onAdd: (webhook: Omit) => void; - onUpdate: (webhook: Webhook) => void; + onAdd: (webhook: Omit) => boolean | Promise; + onUpdate: (webhook: Webhook) => boolean | Promise; onDelete: (id: string) => void; onTest: (id: string, webhookData?: Omit) => void; testing?: string | null; @@ -26,6 +26,7 @@ export function WebhookConfig(props: WebhookConfigProps) { webhooks={props.webhooks} templates={state.templates} testing={props.testing} + saving={state.saving()} allEnabled={state.allEnabled} someEnabled={state.someEnabled} toggleAllWebhooks={state.toggleAllWebhooks} @@ -58,12 +59,15 @@ export function WebhookConfig(props: WebhookConfigProps) { testWebhookForm={state.testWebhookForm} saveWebhook={state.saveWebhook} testing={props.testing} + saving={state.saving} + saveError={state.saveError} /> - + ); } diff --git a/frontend-modern/src/components/Alerts/WebhookConfigList.tsx b/frontend-modern/src/components/Alerts/WebhookConfigList.tsx index e910e04ed5..04aa8c4487 100644 --- a/frontend-modern/src/components/Alerts/WebhookConfigList.tsx +++ b/frontend-modern/src/components/Alerts/WebhookConfigList.tsx @@ -17,6 +17,7 @@ interface WebhookConfigListProps { webhooks: Webhook[]; templates: Accessor; testing?: string | null; + saving: boolean; allEnabled: Accessor; someEnabled: Accessor; toggleAllWebhooks: (enabled: boolean) => void; @@ -39,14 +40,14 @@ export function WebhookConfigList(props: WebhookConfigListProps) {
+ Saved + + ); + } + render(() => ); + if (!enabled) { + fireEvent.click(await screen.findByRole('button', { name: 'Show settings' })); + expect(screen.getByRole('button', { name: 'Send test email' })).toBeDisabled(); + } + fireEvent.click(await screen.findByRole('button', { name: 'Show advanced options' })); + const input = screen.getByRole('spinbutton', { name: 'Rate limit' }); + expect(input).toHaveValue(17); + fireEvent.input(input, { target: { value: '12' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save changes' })); + await waitFor(() => expect(screen.getByText('Saved')).toBeInTheDocument()); + + expect(writtenEmailConfig()).toEqual({ ...savedConfig, rateLimit: 12, enabled }); + }, + ); +}); diff --git a/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsState.test.tsx b/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsState.test.tsx index 77faffdf15..ad753891a4 100644 --- a/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsState.test.tsx +++ b/frontend-modern/src/features/alerts/__tests__/useAlertDestinationsState.test.tsx @@ -72,6 +72,7 @@ describe('useAlertDestinationsState', () => { to: ['alerts@example.com'], tls: true, startTLS: true, + rateLimit: 17, minimumSeverity: 'critical', } as any); vi.mocked(NotificationsAPI.getAppriseConfig).mockResolvedValue({ @@ -129,6 +130,7 @@ describe('useAlertDestinationsState', () => { expect(RelayAPI.getConfig).toHaveBeenCalledTimes(1); expect(result.emailConfig().server).toBe('smtp.example.com'); expect(result.emailConfig().minimumSeverity).toBe('critical'); + expect(result.emailConfig().rateLimit).toBe(17); expect(result.appriseConfig().targetsText).toContain('mailto://ops@example.com'); expect(result.appriseConfig().minimumSeverity).toBe('critical'); expect(result.deadManPingUrl()).toBe('***REDACTED***'); @@ -165,7 +167,7 @@ describe('useAlertDestinationsState', () => { await result.saveDestinations(); expect(NotificationsAPI.updateEmailConfig).toHaveBeenCalledWith( - expect.objectContaining({ server: 'smtp.internal' }), + expect.objectContaining({ server: 'smtp.internal', rateLimit: 17 }), ); expect(NotificationsAPI.updateAppriseConfig).toHaveBeenCalledWith( expect.objectContaining({ diff --git a/frontend-modern/src/features/alerts/alertDestinationsModel.ts b/frontend-modern/src/features/alerts/alertDestinationsModel.ts index 6e33c860a8..b29b9865df 100644 --- a/frontend-modern/src/features/alerts/alertDestinationsModel.ts +++ b/frontend-modern/src/features/alerts/alertDestinationsModel.ts @@ -42,6 +42,7 @@ export function buildEmailConfigPayload(config: UIEmailConfig): EmailConfig { to: config.to.map((entry) => entry.trim()).filter((entry) => entry.length > 0), tls: config.tls, startTLS: config.startTLS, + rateLimit: config.rateLimit, }; if (config.tagFilter !== undefined) { payload.tagFilter = config.tagFilter; diff --git a/frontend-modern/src/features/alerts/useAlertWebhookDestinationsState.ts b/frontend-modern/src/features/alerts/useAlertWebhookDestinationsState.ts index 68bbae0707..bf0689e218 100644 --- a/frontend-modern/src/features/alerts/useAlertWebhookDestinationsState.ts +++ b/frontend-modern/src/features/alerts/useAlertWebhookDestinationsState.ts @@ -81,11 +81,13 @@ export function useAlertWebhookDestinationsState( const created = await NotificationsAPI.createWebhook(webhook); setWebhooks((current) => [...current, normalizeWebhook(created)]); notificationStore.success(getAlertWebhookMutationSuccess('add')); + return true; } catch (error) { logger.error('Failed to add webhook:', error); notificationStore.error( error instanceof Error ? error.message : getAlertWebhookMutationFailure('add'), ); + return false; } }; @@ -96,11 +98,13 @@ export function useAlertWebhookDestinationsState( current.map((entry) => (entry.id === webhook.id ? normalizeWebhook(updated) : entry)), ); notificationStore.success(getAlertWebhookMutationSuccess('update')); + return true; } catch (error) { logger.error('Failed to update webhook:', error); notificationStore.error( error instanceof Error ? error.message : getAlertWebhookMutationFailure('update'), ); + return false; } }; diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 9a0309f38e..df67f9edb2 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -15056,6 +15056,90 @@ func TestContract_ResourceReportMergeReplacesOperatorLink(t *testing.T) { } } +// A source-filtered report-merge undoes the links that join the members +// carrying the named source, not every link along the way to it: a Docker +// host linked into an agent linked into a VM leaves the VM and the agent +// merged when only the Docker source is named. +func TestContract_ResourceReportMergeSourceFilterKeepsLinksOfUnreportedMembers(t *testing.T) { + now := time.Now().UTC() + h := newActionTestResourceHandlers(t, &config.Config{DataPath: t.TempDir()}) + h.SetStateProvider(resourceStateProvider{snapshot: models.StateSnapshot{ + LastUpdate: now, + VMs: []models.VM{{ID: "lab:pve1:101", VMID: 101, Name: "web", Node: "pve1", Instance: "lab", Status: "running", Type: "qemu", LastSeen: now}}, + Hosts: []models.Host{{ID: "host-box", Hostname: "box-agent", MachineID: "fedcba9876543210", Status: "online", LastSeen: now}}, + DockerHosts: []models.DockerHost{{ID: "docker-box", Hostname: "dock-box", MachineID: "0011223344556677", Status: "online", LastSeen: now}}, + }}) + list := func() []unifiedresources.Resource { + t.Helper() + rec := httptest.NewRecorder() + h.HandleListResources(rec, httptest.NewRequest(http.MethodGet, "/api/resources", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("/api/resources status = %d, body=%s", rec.Code, rec.Body.String()) + } + var resp ResourcesResponse + if err := json.NewDecoder(rec.Body).Decode(&resp); err != nil { + t.Fatalf("decode /api/resources: %v", err) + } + return resp.Data + } + var vmID, agentID, dockerID string + for _, resource := range list() { + switch { + case resource.Type == unifiedresources.ResourceTypeVM: + vmID = resource.ID + case resource.Docker != nil: + dockerID = resource.ID + case resource.Agent != nil: + agentID = resource.ID + } + } + if vmID == "" || agentID == "" || dockerID == "" { + t.Fatalf("fixture did not list a separate VM, agent and Docker host: %+v", list()) + } + link := func(fromID, targetID string) { + t.Helper() + rec := httptest.NewRecorder() + h.HandleLink(rec, httptest.NewRequest(http.MethodPost, "/api/resources/"+fromID+"/link", strings.NewReader(`{"targetId":"`+targetID+`"}`))) + if rec.Code != http.StatusOK { + t.Fatalf("link status = %d, body=%s", rec.Code, rec.Body.String()) + } + } + // Links apply in store order: the agent takes in the Docker host, then the + // VM takes in the agent. + link(agentID, dockerID) + link(vmID, agentID) + if chained := list(); len(chained) != 1 { + t.Fatalf("links listed %d resources, want the chain folded into one: %+v", len(chained), chained) + } + + rec := httptest.NewRecorder() + h.HandleReportMerge(rec, httptest.NewRequest(http.MethodPost, "/api/resources/"+vmID+"/report-merge", strings.NewReader(`{"sources":["docker"]}`))) + if rec.Code != http.StatusOK { + t.Fatalf("report-merge status = %d, body=%s", rec.Code, rec.Body.String()) + } + store, err := h.getStore("default") + if err != nil { + t.Fatal(err) + } + links, err := store.GetLinks() + if err != nil || len(links) != 1 { + t.Fatalf("report-merge of the Docker source left links %+v (err %v), want only the VM and agent link", links, err) + } + if (links[0].ResourceA != vmID || links[0].ResourceB != agentID) && (links[0].ResourceA != agentID || links[0].ResourceB != vmID) { + t.Fatalf("report-merge kept link %+v, want the VM and agent link", links[0]) + } + split := list() + if len(split) != 2 { + t.Fatalf("report-merge listed %d resources, want the VM with its agent and the Docker host apart: %+v", len(split), split) + } + for _, resource := range split { + docker := slices.Contains(resource.Sources, unifiedresources.SourceDocker) + if docker != (len(resource.Sources) == 1) { + t.Fatalf("report-merge listed %s with sources %v, want the Docker host alone and the VM without Docker", resource.ID, resource.Sources) + } + } +} + func TestContract_ResourceListPolicyMetadata(t *testing.T) { now := time.Date(2026, 3, 17, 10, 0, 0, 0, time.UTC) h := newActionTestResourceHandlers(t, &config.Config{DataPath: t.TempDir()}) diff --git a/internal/api/resourceapi/resources.go b/internal/api/resourceapi/resources.go index b6c2bcb214..45b924df23 100644 --- a/internal/api/resourceapi/resources.go +++ b/internal/api/resourceapi/resources.go @@ -1019,17 +1019,17 @@ func (h *QueryService) HandleReportMerge(w http.ResponseWriter, r *http.Request) reason = "reported_incorrect_merge" } - // An operator link that brought a reported source in is undone by - // excluding the link's own pair, which replaces the link in the store. - // Identity merges are kept apart by excluding the candidate ID each - // source's record takes when no match is allowed; for a linked source that - // candidate names neither side of the link. + // An operator link that joins a member carrying a reported source to the + // part that stays is undone by excluding the link's own pair, which + // replaces the link in the store. A fold's sources span everything its + // folded side took in, so the registry picks links by member instead + // (ReportedManualLinkFolds). Identity merges are kept apart by excluding + // the candidate ID each source's record takes when no match is allowed; + // for a linked source that candidate names neither side of the link. type exclusionPair struct{ a, b string } pairs := make([]exclusionPair, 0, len(linkFolds)+len(sourceTargets)) - for _, fold := range linkFolds { - if selected(fold.Sources...) { - pairs = append(pairs, exclusionPair{fold.HolderID, fold.FoldedID}) - } + for _, fold := range unified.ReportedManualLinkFolds(path, linkFolds, selected) { + pairs = append(pairs, exclusionPair{fold.HolderID, fold.FoldedID}) } for _, target := range sourceTargets { if !selected(target.Source) || target.CandidateID == "" || target.CandidateID == path { diff --git a/internal/api/resourceapi/resources_report_merge_test.go b/internal/api/resourceapi/resources_report_merge_test.go index 0ebea98fad..aefd4aa74d 100644 --- a/internal/api/resourceapi/resources_report_merge_test.go +++ b/internal/api/resourceapi/resources_report_merge_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "slices" + "strings" "testing" "time" @@ -144,6 +146,109 @@ func TestResourceReportMergeSourceFilterSelectsLinks(t *testing.T) { } } +// A source filter detaches the members of a chain that carry the source. A +// Docker host linked into an agent that is linked into a VM leaves two folds, +// and the agent's fold lists the Docker source as well, because its subtree +// holds the Docker host. Naming Docker used to undo the VM-agent link too, and +// the pair was apart on every surface; now only the link that holds the Docker +// host goes. Naming the agent detaches the agent from the VM and keeps the +// Docker host in the agent, which the report does not name. +func TestResourceReportMergeSourceFilterDetachesChainMembers(t *testing.T) { + now := time.Now().UTC() + snapshot := models.StateSnapshot{ + LastUpdate: now, + VMs: []models.VM{{ID: "lab:pve1:101", VMID: 101, Name: "web", Node: "pve1", Instance: "lab", Status: "running", Type: "qemu", LastSeen: now}}, + Hosts: []models.Host{{ID: "host-box", Hostname: "box-agent", MachineID: "fedcba9876543210", Status: "online", LastSeen: now}}, + DockerHosts: []models.DockerHost{{ID: "docker-box", Hostname: "dock-box", MachineID: "0011223344556677", Status: "online", LastSeen: now}}, + } + for _, tc := range []struct { + name string + sources []string + // want lists the sources of each resource left standing, and links + // how many of the chain's two links the report leaves in the store. + want []string + links int + }{ + {"leaf member", []string{"docker"}, []string{"agent+proxmox", "docker"}, 1}, + {"member that took another in", []string{"agent"}, []string{"agent+docker", "proxmox"}, 1}, + {"both members", []string{"agent", "docker"}, []string{"agent", "docker", "proxmox"}, 0}, + {"every source", []string{"proxmox", "agent", "docker"}, []string{"agent", "docker", "proxmox"}, 0}, + {"no filter", nil, []string{"agent", "docker", "proxmox"}, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + h, store, rebuild := newReportMergeHarness(t, snapshot) + var vmID, agentID, dockerID string + for _, resource := range rebuild() { + switch { + case resource.Type == unified.ResourceTypeVM: + vmID = resource.ID + case resource.Agent != nil: + agentID = resource.ID + case resource.Docker != nil: + dockerID = resource.ID + } + } + if vmID == "" || agentID == "" || dockerID == "" { + t.Fatalf("fixture lacks a standalone VM, agent and Docker host: %+v", rebuild()) + } + // Links apply in store order: the agent takes in the Docker host, + // then the VM takes in the agent. + postResourceAction(t, h, agentID, "link", map[string]any{"targetId": dockerID}) + postResourceAction(t, h, vmID, "link", map[string]any{"targetId": agentID}) + chained := rebuild() + if got := resourceSourceGroups(chained); !slices.Equal(got, []string{"agent+docker+proxmox"}) { + t.Fatalf("links left %v, want the chain folded into one resource", got) + } + assertRESTSourceGroups(t, h, []string{"agent+docker+proxmox"}) + + body := map[string]any{} + if tc.sources != nil { + body["sources"] = tc.sources + } + postResourceAction(t, h, chained[0].ID, "report-merge", body) + if got := resourceSourceGroups(rebuild()); !slices.Equal(got, tc.want) { + t.Fatalf("monitor lists %v after reporting %v, want %v", got, tc.sources, tc.want) + } + assertRESTSourceGroups(t, h, tc.want) + if links, err := store.GetLinks(); err != nil || len(links) != tc.links { + t.Fatalf("report-merge left %d links (err %v), want %d: %+v", len(links), err, tc.links, links) + } + }) + } +} + +// resourceSourceGroups lists the sources of each resource, sorted, as +// "a+b" groups in sorted order. +func resourceSourceGroups(resources []unified.Resource) []string { + groups := make([]string, 0, len(resources)) + for _, resource := range resources { + sources := make([]string, 0, len(resource.Sources)) + for _, source := range resource.Sources { + sources = append(sources, string(source)) + } + slices.Sort(sources) + groups = append(groups, strings.Join(sources, "+")) + } + slices.Sort(groups) + return groups +} + +func assertRESTSourceGroups(t *testing.T, h *QueryService, want []string) { + t.Helper() + rec := httptest.NewRecorder() + h.HandleListResources(rec, httptest.NewRequest(http.MethodGet, "/api/resources", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("list status = %d body=%s", rec.Code, rec.Body.String()) + } + var resp ResourcesResponse + if err := json.NewDecoder(rec.Body).Decode(&resp); err != nil { + t.Fatalf("decode list: %v", err) + } + if got := resourceSourceGroups(resp.Data); !slices.Equal(got, want) { + t.Fatalf("REST lists %v, want %v", got, want) + } +} + // Report-merge on a pair identity matching merged keeps it apart through the // monitor's rebuilds, whose identity pins persist on one generation and steer // the next. diff --git a/internal/availabilityprobe/icmp_diagnostic_test.go b/internal/availabilityprobe/icmp_diagnostic_test.go new file mode 100644 index 0000000000..983139c2ca --- /dev/null +++ b/internal/availabilityprobe/icmp_diagnostic_test.go @@ -0,0 +1,155 @@ +package availabilityprobe + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" +) + +func TestICMPProbeDiagnostics(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("synthetic ping fixture requires a POSIX shell; no real ping is used") + } + + runners := []struct { + name string + run func(context.Context, config.AvailabilityTarget) (ProbeResult, error) + }{ + {"Run", func(ctx context.Context, target config.AvailabilityTarget) (ProbeResult, error) { + return ProbeResult{}, Run(ctx, target) + }}, + {"Result", func(ctx context.Context, target config.AvailabilityTarget) (ProbeResult, error) { + outcome, err := Result(ctx, target) + return ProbeResult{Outcome: outcome}, err + }}, + {"DetailedResult", DetailedResult}, + } + const privateMarker = "synthetic-private-ping-details" + cases := []struct { + name string + output string + exit int + blocked bool + wantError string + contextErr error + missing bool + }{ + {name: "operation-not-permitted", output: "ping: socket: Operation not permitted\n" + privateMarker, exit: 1, blocked: true}, + {name: "cap-net-raw", output: "ping: missing cap_net_raw capability\n" + privateMarker, exit: 2, blocked: true}, + {name: "ordinary-error", output: " ping: unknown host\n", exit: 1, wantError: "icmp probe failed: ping: unknown host"}, + {name: "bounded-output", output: strings.Repeat("x", 300), exit: 1, wantError: "icmp probe failed: " + strings.Repeat("x", 240)}, + {name: "empty-output", exit: 17, wantError: "icmp probe failed: exit status 17"}, + {name: "success-with-permission-like-output", output: "Operation not permitted cap_net_raw", exit: 0}, + {name: "cancelled", contextErr: context.Canceled}, + {name: "expired", contextErr: context.DeadlineExceeded}, + {name: "missing-ping", missing: true}, + } + for _, runner := range runners { + t.Run(runner.name, func(t *testing.T) { + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + argsFile := filepath.Join(dir, "args") + if !tc.missing { + // Shell builtins only: this fixture cannot contact a target or + // accidentally delegate to the host's actual ping binary. + script := "#!/bin/sh\nprintf '%s\\n' \"$@\" > \"$PULSE_TEST_PING_ARGS\"\nprintf '%s' \"$PULSE_TEST_PING_OUTPUT\" >&2\nexit \"$PULSE_TEST_PING_EXIT\"\n" + if err := os.WriteFile(filepath.Join(dir, "ping"), []byte(script), 0700); err != nil { + t.Fatal(err) + } + } + t.Setenv("PATH", dir) + t.Setenv("PULSE_TEST_PING_ARGS", argsFile) + t.Setenv("PULSE_TEST_PING_OUTPUT", tc.output) + t.Setenv("PULSE_TEST_PING_EXIT", fmt.Sprint(tc.exit)) + + ctx := context.Background() + if tc.contextErr == context.Canceled { + cancelled, cancel := context.WithCancel(ctx) + cancel() + ctx = cancelled + } else if tc.contextErr == context.DeadlineExceeded { + expired, cancel := context.WithDeadline(ctx, time.Now().Add(-time.Second)) + defer cancel() + ctx = expired + } + target := config.AvailabilityTarget{ + Address: "192.0.2.10", Protocol: config.AvailabilityProbeICMP, Enabled: true, TimeoutMillis: 1375, + } + result, err := runner.run(ctx, target) + if runner.name == "DetailedResult" && (result.TransportOutcome != result.Outcome || result.Application != nil || result.Certificate != nil) { + t.Errorf("ICMP result changed non-diagnostic fields: %+v", result) + } + switch { + case tc.blocked: + if err == nil { + t.Fatal("permission failure was reported as success") + } + message := err.Error() + for _, required := range []string{"icmp probe blocked", "local permissions", "selected observation host", "server or agent", "docs/CONFIGURATION.md#icmp-probe-privileges", "NoNewPrivileges", "intentional", "restrictions"} { + if !strings.Contains(message, required) { + t.Errorf("permission diagnostic %q omits %q", message, required) + } + } + for _, unsafe := range []string{privateMarker, "Pulse service unit", "installer", "AmbientCapabilities=", "CapabilityBoundingSet=", "restart", "setcap", "--privileged", "--cap-add", "as root"} { + if strings.Contains(message, unsafe) { + t.Errorf("permission diagnostic %q contains unsupported advice or raw details %q", message, unsafe) + } + } + if len(message) > 240 { + t.Errorf("diagnostic has %d bytes; agent reports truncate after 240", len(message)) + } + case tc.contextErr != nil: + if !errors.Is(err, tc.contextErr) { + t.Fatalf("error = %v, want %v", err, tc.contextErr) + } + case tc.missing: + if !errors.Is(err, exec.ErrNotFound) || !strings.HasPrefix(err.Error(), "icmp probe failed: ") { + t.Fatalf("error = %v, want wrapped missing executable error", err) + } + case tc.wantError != "": + if err == nil || err.Error() != tc.wantError { + t.Fatalf("error = %v, want %q", err, tc.wantError) + } + if tc.output == "" { + var exitError *exec.ExitError + if !errors.As(err, &exitError) || exitError.ExitCode() != tc.exit { + t.Fatalf("error = %v, want wrapped exit %d", err, tc.exit) + } + } + default: + if err != nil { + t.Fatalf("successful ping returned error: %v", err) + } + } + if runner.name != "Run" { + want := OutcomeReachable + if err != nil { + want = OutcomeUnreachable // Existing classification is unchanged. + } + if result.Outcome != want { + t.Fatalf("outcome = %q, want %q", result.Outcome, want) + } + } + args, argsErr := os.ReadFile(argsFile) + if tc.contextErr != nil || tc.missing { + if !errors.Is(argsErr, os.ErrNotExist) { + t.Fatalf("unstarted ping left arguments: %q, error %v", args, argsErr) + } + } else if want := strings.Join(pingArgs(target.Address, target.TimeoutMillis), "\n") + "\n"; argsErr != nil || string(args) != want { + t.Fatalf("ping arguments = %q, error %v; want %q", args, argsErr, want) + } + }) + } + }) + } +} diff --git a/internal/availabilityprobe/probe.go b/internal/availabilityprobe/probe.go index ac2a132018..55fd587f89 100644 --- a/internal/availabilityprobe/probe.go +++ b/internal/availabilityprobe/probe.go @@ -200,11 +200,11 @@ func probeICMP(ctx context.Context, target config.AvailabilityTarget) error { if details == "" { return fmt.Errorf("icmp probe failed: %w", err) } - // Units written before v6.1.0-rc.1 lack AmbientCapabilities=CAP_NET_RAW and - // in-place updates never rewrite the unit, so ping fails like this on every - // upgraded install (#1554). Point at the unit instead of echoing ping stderr. + // The check runs on the selected server or agent, and a permission failure + // does not identify a particular unit setting. Keep the guidance within the + // agent's 240-byte error limit without recommending privilege escalation. if strings.Contains(details, "Operation not permitted") || strings.Contains(details, "cap_net_raw") { - return fmt.Errorf("icmp probe blocked. The Pulse service unit does not grant CAP_NET_RAW, so ping cannot open a socket. Re-run the Pulse installer to regenerate the unit, or add a systemd override with AmbientCapabilities=CAP_NET_RAW and CapabilityBoundingSet=CAP_NET_RAW, then restart the service") + return fmt.Errorf("icmp probe blocked by local permissions on the selected observation host (server or agent). See docs/CONFIGURATION.md#icmp-probe-privileges. Preserve NoNewPrivileges and intentional capability/container restrictions.") } if len(details) > 240 { details = details[:240] diff --git a/internal/hostagent/availability_icmp_diagnostic_test.go b/internal/hostagent/availability_icmp_diagnostic_test.go new file mode 100644 index 0000000000..4ecc391b78 --- /dev/null +++ b/internal/hostagent/availability_icmp_diagnostic_test.go @@ -0,0 +1,47 @@ +package hostagent + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" +) + +func TestAvailabilityModuleICMPPermissionDiagnosticSurvivesReport(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("synthetic ping fixture requires a POSIX shell; no real ping is used") + } + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "ping"), []byte("#!/bin/sh\nprintf '%s' 'ping: socket: Operation not permitted' >&2\nexit 1\n"), 0700); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir) + module := testProbeModule(t, nil) // Exercise the shared core, not the test callback. + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "remote-icmp", Address: "192.0.2.10", Protocol: config.AvailabilityProbeICMP, Enabled: true, ConfigRevision: 7, + }) + module.check(context.Background(), target) + results := module.snapshotForReport() + if len(results) != 1 { + t.Fatalf("report has %d observations, want one", len(results)) + } + result := results[0] + if result.TargetID != target.ID || result.ConfigRevision != target.ConfigRevision || result.ObservationID == "" || result.CheckedAt.IsZero() { + t.Fatalf("report lost observation identity: %+v", result) + } + if result.Outcome != "unreachable" || result.TransportOutcome != "unreachable" { + t.Fatalf("report changed existing ICMP failure classification: %+v", result) + } + for _, required := range []string{"local permissions", "selected observation host", "server or agent", "docs/CONFIGURATION.md#icmp-probe-privileges", "NoNewPrivileges", "intentional capability/container restrictions."} { + if !strings.Contains(result.Error, required) { + t.Errorf("agent diagnostic %q lost %q", result.Error, required) + } + } + if len(result.Error) > availabilityErrorLimit || strings.Contains(result.Error, "Pulse service unit") || strings.Contains(result.Error, "installer") { + t.Fatalf("agent diagnostic is unbounded or blames the server: %q", result.Error) + } +} diff --git a/internal/mock/integration.go b/internal/mock/integration.go index 219946857c..8ae4c87c58 100644 --- a/internal/mock/integration.go +++ b/internal/mock/integration.go @@ -218,8 +218,7 @@ func enableMockMode(config MockConfig, fromInit bool) { dataMu.Lock() mockConfig = config mockGraph = buildFixtureGraph(config, now) - fixtureRevision.Add(1) - fixtureDataVersion.Add(1) + advanceFixtureStructure() metricCohort.Store(0) enabled.Store(true) dataMu.Unlock() @@ -255,8 +254,7 @@ func disableMockMode() { dataMu.Lock() mockGraph = emptyFixtureGraph() - fixtureRevision.Add(1) - fixtureDataVersion.Add(1) + advanceFixtureStructure() dataMu.Unlock() log.Info().Msg("mock mode disabled") @@ -334,6 +332,33 @@ func FixtureDataVersion() uint64 { return fixtureDataVersion.Load() } +// FixtureStructureRevision returns a token that advances only when the mock +// estate itself changes (mock mode enabled or disabled, or a configuration +// that rebuilds the graph), never on a metric tick. Which resources exist, +// their identities and their source mappings are fixed between two reads of +// the same revision, so a lookup that depends only on those, such as a +// resource's metrics target, may be served from a view built at any data +// version of that revision. +// +// A cache that tags a snapshot with the revision must read the revision +// BEFORE the data version it memoizes the snapshot under. The two counters +// advance under dataMu, data version first (advanceFixtureStructure), so a +// reader that sees a new revision is guaranteed the new data version after +// it and cannot be handed the previous estate's memoized snapshot under the +// new revision; the other interleaving only tags a newer snapshot with an +// older revision, which costs one rebuild. +func FixtureStructureRevision() uint64 { + return fixtureRevision.Load() +} + +// advanceFixtureStructure records a structural change of the fixture graph. +// Callers hold dataMu for writing. The data version moves before the +// structure revision; see FixtureStructureRevision for the ordering contract. +func advanceFixtureStructure() { + fixtureDataVersion.Add(1) + fixtureRevision.Add(1) +} + // GetConfig returns the current mock configuration. func GetConfig() MockConfig { dataMu.RLock() @@ -539,8 +564,7 @@ func SetMockConfig(cfg MockConfig) { setMockUpdateInterval(normalized.UpdateInterval) if configChanged && enabled.Load() { mockGraph = buildFixtureGraph(normalized, time.Now()) - fixtureRevision.Add(1) - fixtureDataVersion.Add(1) + advanceFixtureStructure() } dataMu.Unlock() diff --git a/internal/mock/platform_fixtures_test.go b/internal/mock/platform_fixtures_test.go index 7f25908432..bf5db89b7f 100644 --- a/internal/mock/platform_fixtures_test.go +++ b/internal/mock/platform_fixtures_test.go @@ -4,8 +4,15 @@ import ( "bytes" "encoding/json" "fmt" + "go/ast" + "go/parser" + "go/token" "math" + "os" + "path/filepath" + "reflect" "slices" + "strings" "testing" "time" @@ -1017,3 +1024,280 @@ func TestFixtureGraphMetricCohortProducesSparseUnifiedResourceChanges(t *testing t.Fatalf("expected sparse unified resource churn, changed=%d total=%d", changed, len(after)) } } + +func fixtureMetricsTargets(t *testing.T) map[string]unifiedresources.MetricsTarget { + t.Helper() + resources, _ := UnifiedResourceSnapshot() + if len(resources) == 0 { + t.Fatal("expected mock resources") + } + registry := unifiedresources.NewRegistry(nil) + registry.IngestResources(resources) + _, targets := registry.ListWithMetricsTargets() + if len(targets) == 0 { + t.Fatal("expected the mock estate to resolve metrics targets") + } + return targets +} + +func fixtureResourceIDs(t *testing.T) []string { + t.Helper() + resources, _ := UnifiedResourceSnapshot() + ids := make([]string, 0, len(resources)) + for _, resource := range resources { + ids = append(ids, resource.ID) + } + slices.Sort(ids) + return ids +} + +// smallMockEstateForTest replaces the ambient estate with a fixed small one, +// so a test neither depends on the process's PULSE_MOCK_* settings (a node +// count at the supported maximum clamps back when raised) nor pays for a +// 929-guest snapshot per assertion. The ambient configuration returns when +// the test ends. +func smallMockEstateForTest(t *testing.T, nodes int) MockConfig { + t.Helper() + previous := GetConfig() + t.Cleanup(func() { SetMockConfig(previous) }) + small := previous + small.NodeCount = nodes + SetMockConfig(small) + return small +} + +// Callers cache a resource's metrics target for as long as the structure +// revision holds (see Monitor.MetricsTargetForResource), so a metric tick must +// leave the revision, the resource set and every target exactly as they were. +// Checked after every tick of two full cohort rotations, not only at the end, +// so a change that a later tick reverts is still caught. +func TestMetricTicksNeverChangeTheFixtureStructure(t *testing.T) { + withMockEnabledForTest(t) + stopUpdateLoop() + t.Cleanup(func() { + if IsMockEnabled() { + startUpdateLoop() + } + }) + smallMockEstateForTest(t, 6) + stopUpdateLoop() + + revision := FixtureStructureRevision() + ids := fixtureResourceIDs(t) + targets := fixtureMetricsTargets(t) + version := FixtureDataVersion() + + for tick := 1; tick <= mockMetricCohortCount*2+1; tick++ { + updateMetrics(GetConfig()) + + if got := FixtureDataVersion(); got <= version { + t.Fatalf("tick %d did not advance the data version (%d)", tick, got) + } else { + version = got + } + if got := FixtureStructureRevision(); got != revision { + t.Fatalf("tick %d moved the fixture structure revision from %d to %d", tick, revision, got) + } + if got := fixtureResourceIDs(t); !slices.Equal(got, ids) { + t.Fatalf("tick %d changed the resource set: %d resources before, %d after", tick, len(ids), len(got)) + } + if got := fixtureMetricsTargets(t); !reflect.DeepEqual(got, targets) { + t.Fatalf("tick %d changed the resolved metrics targets", tick) + } + } +} + +func TestFixtureStructureRevisionAdvancesOnStructuralChange(t *testing.T) { + withMockEnabledForTest(t) + stopUpdateLoop() + t.Cleanup(func() { + if IsMockEnabled() { + startUpdateLoop() + } + }) + small := smallMockEstateForTest(t, 3) + stopUpdateLoop() + + revision := FixtureStructureRevision() + grown := small + grown.NodeCount = 4 + SetMockConfig(grown) + if got := FixtureStructureRevision(); got <= revision { + t.Fatalf("reconfiguring the estate left the structure revision at %d (was %d)", got, revision) + } + + revision = FixtureStructureRevision() + if err := SetEnabled(false); err != nil { + t.Fatalf("disable mock mode: %v", err) + } + if got := FixtureStructureRevision(); got <= revision { + t.Fatalf("disabling mock mode left the structure revision at %d (was %d)", got, revision) + } + revision = FixtureStructureRevision() + if err := SetEnabled(true); err != nil { + t.Fatalf("enable mock mode: %v", err) + } + if got := FixtureStructureRevision(); got <= revision { + t.Fatalf("enabling mock mode left the structure revision at %d (was %d)", got, revision) + } +} + +// A cache fill reads the structure revision before the data version, and the +// fixture advances its data version before its revision, so a reader that +// sees a new revision is guaranteed the new data version after it and is +// never handed the previous estate's memoized snapshot under the new +// revision. The interleaving cannot be forced from outside, so the order of +// the two writes and of the two reads is pinned on the syntax tree: inside +// the named functions only, so comments, local names and the position of the +// functions in their package do not matter. +func TestFixtureStructureRevisionOrderingContract(t *testing.T) { + mockDecls := parsePackageFuncs(t, ".") + advance := packageFunc(mockDecls["advanceFixtureStructure"]) + if advance == nil { + t.Fatal("the mock package lost advanceFixtureStructure") + } + version := synchronousCallOffsets(advance, "fixtureDataVersion", "Add") + revision := synchronousCallOffsets(advance, "fixtureRevision", "Add") + if len(version) != 1 || len(revision) != 1 || version[0] > revision[0] { + t.Fatal("advanceFixtureStructure must advance the data version once, before the structure revision once, as plain statements") + } + if got := countCalls(advance, "fixtureRevision", "Add") + countCalls(advance, "fixtureDataVersion", "Add"); got != 2 { + t.Fatalf("advanceFixtureStructure must not defer or spawn its counter writes; found %d writes in all", got) + } + + // Every declaration counts for the bypass scan, including methods and the + // build-tagged duplicates of a function. + for name, decls := range mockDecls { + if name == "advanceFixtureStructure" { + continue + } + for _, decl := range decls { + if got := countCalls(decl, "fixtureRevision", "Add"); got != 0 { + t.Fatalf("%s advances the structure revision directly; it must go through advanceFixtureStructure", name) + } + } + } + for _, name := range []string{"enableMockMode", "disableMockMode", "SetMockConfig"} { + decl := packageFunc(mockDecls[name]) + if decl == nil { + t.Fatalf("the mock package lost %s", name) + } + if got := len(synchronousIdentCallOffsets(decl, "advanceFixtureStructure")); got != 1 { + t.Fatalf("%s must advance the fixture structure through advanceFixtureStructure exactly once, as a plain statement; found %d", name, got) + } + } + + view := packageFunc(parsePackageFuncs(t, "../monitoring")["currentUnifiedStateView"]) + if view == nil { + t.Fatal("the monitoring package lost currentUnifiedStateView") + } + structureRead := synchronousCallOffsets(view, "mock", "FixtureStructureRevision") + versionRead := synchronousCallOffsets(view, "mock", "FixtureDataVersion") + if len(structureRead) != 1 || len(versionRead) != 1 || structureRead[0] > versionRead[0] { + t.Fatal("currentUnifiedStateView must read the structure revision once, before it reads the data version") + } +} + +// parsePackageFuncs returns every non-test function and method declaration of +// the package in dir, keyed by name. Names repeat across build-tagged files +// and between functions and methods, so each key holds all of them. +func parsePackageFuncs(t *testing.T, dir string) map[string][]*ast.FuncDecl { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + funcs := make(map[string][]*ast.FuncDecl) + fset := token.NewFileSet() + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + for _, decl := range file.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok { + funcs[fn.Name.Name] = append(funcs[fn.Name.Name], fn) + } + } + } + return funcs +} + +// packageFunc returns the declaration when it is the only one of its name in +// the package (function or method), and nil otherwise. +func packageFunc(decls []*ast.FuncDecl) *ast.FuncDecl { + if len(decls) != 1 { + return nil + } + return decls[0] +} + +// walkSynchronous visits the nodes of fn that run in the function's own +// control flow: it does not descend into function literals, deferred calls or +// goroutine starts, whose source position says nothing about when they run. +func walkSynchronous(fn *ast.FuncDecl, visit func(ast.Node)) { + ast.Inspect(fn, func(node ast.Node) bool { + switch node.(type) { + case *ast.FuncLit, *ast.DeferStmt, *ast.GoStmt: + return false + } + visit(node) + return true + }) +} + +func isSelectorCall(node ast.Node, receiver, method string) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return false + } + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != method { + return false + } + ident, ok := selector.X.(*ast.Ident) + return ok && ident.Name == receiver +} + +// synchronousCallOffsets lists the positions of receiver.method calls that run +// in fn's own control flow, in source order. +func synchronousCallOffsets(fn *ast.FuncDecl, receiver, method string) []token.Pos { + var offsets []token.Pos + walkSynchronous(fn, func(node ast.Node) { + if isSelectorCall(node, receiver, method) { + offsets = append(offsets, node.Pos()) + } + }) + return offsets +} + +// synchronousIdentCallOffsets lists the positions of calls to the package-level +// function name that run in fn's own control flow. +func synchronousIdentCallOffsets(fn *ast.FuncDecl, name string) []token.Pos { + var offsets []token.Pos + walkSynchronous(fn, func(node ast.Node) { + if call, ok := node.(*ast.CallExpr); ok { + if ident, ok := call.Fun.(*ast.Ident); ok && ident.Name == name { + offsets = append(offsets, call.Pos()) + } + } + }) + return offsets +} + +// countCalls counts every receiver.method call anywhere in fn, including +// deferred calls, goroutines and function literals. +func countCalls(fn *ast.FuncDecl, receiver, method string) int { + count := 0 + ast.Inspect(fn, func(node ast.Node) bool { + if isSelectorCall(node, receiver, method) { + count++ + } + return true + }) + return count +} diff --git a/internal/monitoring/guest_filesystem_completeness_test.go b/internal/monitoring/guest_filesystem_completeness_test.go new file mode 100644 index 0000000000..ee2c6e308f --- /dev/null +++ b/internal/monitoring/guest_filesystem_completeness_test.go @@ -0,0 +1,246 @@ +package monitoring + +import ( + "context" + "encoding/json" + "fmt" + "math" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" + "github.com/rcourtman/pulse-go-rewrite/pkg/proxmox" +) + +type incompleteFilesystemClient struct { + mockPVEClientExtended + filesystems []proxmox.VMFileSystem +} + +func (c *incompleteFilesystemClient) GetVMFSInfo(context.Context, string, int) ([]proxmox.VMFileSystem, error) { + return c.filesystems, nil +} + +func TestVMFilesystemIncompleteAlternateClientCannotPublishRemainder(t *testing.T) { + for name, bad := range map[string]proxmox.VMFileSystem{ + "overused": {TotalBytes: 9000, UsedBytes: 9001}, + "signed-overflow": {TotalBytes: uint64(math.MaxInt64) + 1, UsedBytes: 0}, + } { + t.Run(name, func(t *testing.T) { + bad.Mountpoint, bad.Type, bad.Disk = "/data", "ext4", "/dev/vdb1" + peer := proxmox.VMFileSystem{Mountpoint: "/", Type: "ext4", Disk: "/dev/vda1", TotalBytes: 1000, UsedBytes: 0} + c := &incompleteFilesystemClient{filesystems: []proxmox.VMFileSystem{peer, bad}} + m := &Monitor{} + res := proxmox.ClusterResource{Node: "node", VMID: 105, MaxDisk: 10000} + total, used, free, usage, disks, fresh, reason := m.updateVMDisksFromGuestAgentFSInfo(context.Background(), "partial", res, c, 10000, 0, -1) + if total != 10000 || used != 0 || free != 10000 || usage != -1 || len(disks) != 0 || fresh || reason != "agent-error" { + t.Fatalf("invalid peer became a complete current aggregate: %d/%d/%d usage=%v rows=%+v fresh=%t reason=%q", total, used, free, usage, disks, fresh, reason) + } + // A real subsequent complete inventory may remove the other volume. + c.filesystems = []proxmox.VMFileSystem{peer} + total, used, free, usage, disks, fresh, reason = m.updateVMDisksFromGuestAgentFSInfo(context.Background(), "partial", res, c, 10000, 0, -1) + if total != 1000 || used != 0 || free != 1000 || usage != 0 || len(disks) != 1 || !fresh || reason != "" { + t.Fatal("complete inventory removal/zero was treated as partial") + } + }) + } +} + +// Ordinary QEMU-only collection must not turn a malformed large volume into +// recovery/removal merely because a smaller valid volume now reports zero. +// Exercise both collectors, canonical previous state, served JSON, History and +// genuine filesystem alerts. No guest freeze, command bypass or native probe. +func TestVMFilesystemIncompletePollingPreservesHistoryAndBreach(t *testing.T) { + for _, path := range []string{"cluster", "node"} { + t.Run(path, func(t *testing.T) { + var phase, fsCalls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.HasSuffix(r.URL.Path, "/config"): + fmt.Fprint(w, `{"data":{}}`) + case strings.HasSuffix(r.URL.Path, "/status/current"): + fmt.Fprint(w, `{"data":{"status":"running","agent":1,"maxmem":1000,"mem":400,"meminfo":{"total":1000,"available":600}}}`) + case strings.HasSuffix(r.URL.Path, "/agent/get-fsinfo"): + fsCalls.Add(1) + rows := `{"mountpoint":"/","type":"ext4","disk":[{"dev":"/dev/vda1"}],"total-bytes":1000,"used-bytes":980},{"mountpoint":"/data","type":"ext4","disk":[{"dev":"/dev/vdb1"}],"total-bytes":9000,"used-bytes":8820}` + switch phase.Load() { + case 1: + rows = `{"mountpoint":"/","type":"ext4","disk":[{"dev":"/dev/vda1"}],"total-bytes":1000,"used-bytes":0},{"mountpoint":"/data","type":"ext4","total-bytes":9000}` + case 2: + rows = `{"mountpoint":"/","type":"ext4","disk":[{"dev":"/dev/vda1"}],"total-bytes":1000,"used-bytes":0}` + } + fmt.Fprintf(w, `{"data":{"result":[%s]}}`, rows) + case strings.Contains(r.URL.Path, "/agent/"): + fmt.Fprint(w, `{"data":{"result":[]}}`) + default: + t.Errorf("unexpected request: %s", r.URL.Path) + http.NotFound(w, r) + } + })) + defer server.Close() + client, err := proxmox.NewClient(proxmox.ClientConfig{Host: server.URL, TokenName: "fixture@pve!pulse", TokenValue: "fixture", Timeout: time.Second}) + if err != nil { + t.Fatal(err) + } + m := guestHistoryObservationMonitor(t) + m.alertManager = alerts.NewManagerWithDataDir(t.TempDir(), alerts.WithoutPersistedAlertRestore()) + t.Cleanup(m.alertManager.Stop) + cfg := m.alertManager.GetConfig() + cfg.Enabled = true + cfg.GuestDefaults.Disk = &alerts.HysteresisThreshold{Trigger: 90, Clear: 85} + cfg.MetricTimeThresholds = map[string]map[string]int{"guest": {"disk": 0}} + m.alertManager.UpdateConfig(cfg) + m.config, m.state, m.rateTracker = &config.Config{}, models.NewState(), NewRateTracker() + m.guestMetadataLimiter = make(map[string]time.Time) + m.resourceStore = unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(nil)) + res := proxmox.ClusterResource{Type: "qemu", Node: "node", Name: "guest", VMID: 105, Status: "running", MaxMem: 1000, Mem: 400, MaxDisk: 10000, CPU: .2} + id := makeGuestID("partial-filesystems", "node", 105) + publish := func(vm models.VM) { + m.state.UpdateVMs([]models.VM{vm}) + m.updateResourceStore(m.currentStateWithScope()) + } + build := func() models.VM { + t.Helper() + previous := m.previousGuestContextForInstance("partial-filesystems").vmsByID[id] + var vm models.VM + if path == "node" { + vms, _ := m.pollNodeVMsWithClusterResourceBuilder(context.Background(), "partial-filesystems", "node", []proxmox.VM{{VMID: 105, Name: res.Name, Status: res.Status, MaxMem: res.MaxMem, Mem: res.Mem, MaxDisk: res.MaxDisk, CPU: res.CPU}}, client, map[string]models.VM{id: previous}, nil) + if len(vms) != 1 { + t.Fatal("node collector lost guest") + } + vm = vms[0] + } else { + var ok bool + vm, _, _, _, _, ok = m.buildVMFromClusterResource(context.Background(), "partial-filesystems", res, client, id, nil, &previous) + if !ok { + t.Fatal("cluster builder lost guest") + } + m.checkGuestAlertsForVM("partial-filesystems", vm) + } + publish(vm) + m.recordGuestMetrics([]models.VM{vm}, nil, time.Now().Add(-time.Second)) + view := m.currentModeReadState().VMs()[0] + if view.DiskStatusReason() != vm.DiskStatusReason || view.DiskObservation() != vm.DiskObservation || !reflect.DeepEqual(previousVMFromView(view).Disks, vm.Disks) { + t.Fatalf("canonical projection changed completeness, original age or mount rows: vm=%+v view=%+v", vm, previousVMFromView(view)) + } + if vm.Disk.Usage >= 0 && (view.DiskTotal() != vm.Disk.Total || view.DiskUsed() != vm.Disk.Used || view.DiskPercent() != vm.Disk.Usage) { + t.Fatal("canonical projection changed a known disk tuple") + } + // Allocated capacity without usage is not a canonical metric. + // Check absence on the wire below, not equality with raw allocation. + if vm.Disk.Usage < 0 && (view.DiskTotal() != 0 || view.DiskUsed() != 0 || view.DiskPercent() != 0) { + t.Fatal("canonical projection retained expired disk metrics") + } + front := m.buildBroadcastFrontendStateFromSnapshot(models.StateSnapshot{VMs: []models.VM{vm}}, m.mockModeFence.begin()) + wire, err := json.Marshal(front.Resources) + var served []struct { + Proxmox unifiedresources.ProxmoxData + CPU *models.ResourceMetricFrontend + Memory *models.ResourceMetricFrontend + Disk *models.ResourceMetricFrontend + } + if err != nil || json.Unmarshal(wire, &served) != nil || len(served) != 1 || served[0].Proxmox.DiskStatusReason != vm.DiskStatusReason || strings.Contains(string(wire), "diskObservation") { + t.Fatalf("served completeness differs or private observation leaked: %s / %v", wire, err) + } + if served[0].CPU == nil || served[0].CPU.Current != 20 || served[0].Memory == nil || served[0].Memory.Current != 40 { + t.Fatalf("independent current CPU/memory metrics disappeared: %s", wire) + } + metric := served[0].Disk + if vm.Disk.Usage < 0 { + if metric != nil { + t.Fatal("unavailable disk reading became a measured wire zero") + } + } else if metric == nil || metric.Total == nil || metric.Used == nil || *metric.Total != vm.Disk.Total || *metric.Used != vm.Disk.Used || metric.Current != vm.Disk.Usage { + t.Fatalf("known disk tuple or genuine measured zero lost on the wire: %s", wire) + } + return vm + } + initial := build() + if initial.Disk.Total != 10000 || initial.Disk.Used != 9800 || initial.Disk.Usage != 98 || len(initial.Disks) != 2 || initial.DiskStatusReason != "" || initial.DiskObservation.ObservedAt.IsZero() { + t.Fatalf("initial complete reading missing: %+v", initial) + } + active := m.alertManager.GetActiveAlerts() + if len(active) != 2 { + t.Fatalf("complete high readings did not establish two filesystem breaches: %+v", active) + } + breaches := make(map[string]time.Time) + for _, alert := range active { + breaches[alert.ID] = alert.StartTime + } + originalHistory := guestHistoryStoredPoints(t, m, "vm", id, "disk") + if len(originalHistory) != 1 || originalHistory[0].Value != 98 { + t.Fatal("complete reading did not establish History") + } + phase.Store(1) + for i := 0; i < 3; i++ { + retained := build() + if retained.Disk != initial.Disk || !reflect.DeepEqual(retained.Disks, initial.Disks) || retained.DiskStatusReason != "prev-agent-error" || retained.DiskObservation != initial.DiskObservation { + t.Fatalf("partial poll %d became fresh or removed an unknown volume: %+v", i, retained) + } + } + aged := m.previousGuestContextForInstance("partial-filesystems").vmsByID[id] + aged.DiskObservation.ObservedAt = time.Now().Add(-recentGuestAgentEvidenceMaxAge - time.Second) + publish(aged) + expired := build() + if expired.Disk.Usage != -1 || expired.DiskStatusReason != "agent-error" || len(expired.Disks) != 0 || !expired.DiskObservation.ObservedAt.IsZero() { + t.Fatal("repeated partial responses renewed expired complete evidence") + } + active = m.alertManager.GetActiveAlerts() + if len(active) != 2 { + t.Fatalf("partial/expired evidence cleared a genuine filesystem breach: %+v", active) + } + for _, alert := range active { + if !alert.StartTime.Equal(breaches[alert.ID]) { + t.Fatal("partial reading replaced a breach occurrence") + } + } + if !reflect.DeepEqual(guestHistoryStoredPoints(t, m, "vm", id, "disk"), originalHistory) || len(m.metricsHistory.GetGuestMetrics(id, "disk", time.Hour)) != 1 || len(m.metricsHistory.GetGuestMetrics(id, "cpu", time.Hour)) != 5 || fsCalls.Load() != 5 { + t.Fatal("partial evidence created disk History, stopped independent CPU History or retried a command") + } + // Complete ordinary replacement (the large volume really absent) and + // measured zero must still remove/recover. History has second keys. + time.Sleep(1100 * time.Millisecond) + phase.Store(2) + recovered := build() + if recovered.Disk.Total != 1000 || recovered.Disk.Used != 0 || recovered.Disk.Usage != 0 || len(recovered.Disks) != 1 || recovered.DiskStatusReason != "" || !recovered.DiskObservation.ObservedAt.After(initial.DiskObservation.ObservedAt) || len(m.alertManager.GetActiveAlerts()) != 0 || fsCalls.Load() != 6 { + t.Fatalf("complete ordinary removal/zero failed to recover: %+v", recovered) + } + points := guestHistoryStoredPoints(t, m, "vm", id, "disk") + if len(points) != 2 || points[0] != originalHistory[0] || points[1].Value != 0 { + t.Fatal("History lost high reading or failed to append genuine zero") + } + }) + } +} + +func TestVMFilesystemIncompleteColdPollRemainsUnavailable(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/config"): + fmt.Fprint(w, `{"data":{}}`) + case strings.HasSuffix(r.URL.Path, "/get-fsinfo"): + fmt.Fprint(w, `{"data":{"result":[{"mountpoint":"/","type":"ext4","total-bytes":1000,"used-bytes":0},{"mountpoint":"/data","type":"ext4","total-bytes":9000}]}}`) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + client, err := proxmox.NewClient(proxmox.ClientConfig{Host: server.URL, TokenName: "fixture@pve!pulse", TokenValue: "fixture", Timeout: time.Second}) + if err != nil { + t.Fatal(err) + } + res := proxmox.ClusterResource{Node: "node", VMID: 105, MaxDisk: 10000} + _, _, _, usage, disks, fresh, reason := (&Monitor{}).updateVMDisksFromGuestAgentFSInfo(context.Background(), "cold-partial", res, client, 10000, 0, -1) + if usage != -1 || fresh || len(disks) != 0 || reason != "agent-error" { + t.Fatalf("cold partial result became healthy usage: %v/%+v/%t/%q", usage, disks, fresh, reason) + } +} diff --git a/internal/monitoring/metrics_target_resolution.go b/internal/monitoring/metrics_target_resolution.go new file mode 100644 index 0000000000..6d81f731a3 --- /dev/null +++ b/internal/monitoring/metrics_target_resolution.go @@ -0,0 +1,63 @@ +package monitoring + +import ( + "github.com/rcourtman/pulse-go-rewrite/internal/mock" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// mockMetricsTarget resolves a canonical resource ID to its metrics-store +// target in mock mode. The second result is false when mock mode is off, so +// the caller takes its ordinary path; when it is true the answer is final for +// this structure, a nil target meaning the view captured no metrics target for +// the ID (the estate does not list it, or lists a resource that has none, such +// as a VMware network); the caller still tries the raw resource store. +// +// Alert evaluation resolves a target for every resource and every windowed +// metric (CPU by default) on every pass, through MetricsTargetForResource. +// The mock view that answers it is cached per fixture data version, and that +// version advances on every metric tick (2 s by default). Rebuilding the view +// ingests the whole estate twice (the memoized fixture snapshot, then the +// view's own registry), about half a second of CPU for the default estate +// (1,776 resources) on an idle core. A build that takes longer than one tick publishes +// its view under an already-stale version, so the next lookup rebuilds again: +// on a host with CPU to spare that costs one build per tick, but when the +// process is starved (a loaded dev VM, a small demo host) the cache never +// hits, every lookup in the pass builds an estate, and the monitor never +// finishes its first evaluation. Startup stalled in exactly that state, +// before the listener opened, with the watchdog dump parked in the view build +// beneath evaluateMetricWindow. +// +// A metrics target depends only on identity inputs: a resource's type, its +// source mappings, and the technology, Docker container ID and disk identity +// of its facets (unifiedresources.BuildMetricsTarget), falling back to the +// resource's own MetricsTarget. A metric tick moves values and timestamps, +// never those inputs (TestMetricTicksNeverChangeTheFixtureStructure), so the +// captured targets of a view built at any data version of the current +// structure answer the lookup. A data-version change alone therefore never +// triggers a build here: only a structural change (mock mode toggled, the +// fixture reconfigured) or a link change makes the next lookup rebuild the +// view. Two lookups that miss together can each still build one, and a cold +// cache builds even for an ID the estate does not list. +func (m *Monitor) mockMetricsTarget(resourceID string) (*unifiedresources.MetricsTarget, bool) { + if m == nil || !mock.IsMockEnabled() { + return nil, false + } + + structure := mock.FixtureStructureRevision() + links := m.resourceStoreManualLinks() + + m.mockUnifiedViewMu.Lock() + view := m.mockUnifiedView + reusable := m.mockUnifiedViewValid && + m.mockUnifiedViewStructure == structure && + sameManualLinks(m.mockUnifiedViewLinks, links) + m.mockUnifiedViewMu.Unlock() + + if !reusable { + view = m.currentUnifiedStateView() + } + if view.metricsTargets == nil { + return nil, true + } + return view.metricsTargets.MetricsTargetForResource(resourceID), true +} diff --git a/internal/monitoring/metrics_target_resolution_test.go b/internal/monitoring/metrics_target_resolution_test.go new file mode 100644 index 0000000000..79122e93ee --- /dev/null +++ b/internal/monitoring/metrics_target_resolution_test.go @@ -0,0 +1,158 @@ +package monitoring + +import ( + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/mock" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// useMockEstate enables the default mock estate with the given node count, +// whose fixture ticks every interval, and restores the previous mock mode and +// configuration when the test ends. +func useMockEstate(tb testing.TB, nodes int, interval time.Duration) { + tb.Helper() + previous := mock.IsMockEnabled() + previousConfig := mock.GetConfig() + if previous { + mustSetMockEnabled(tb, false) + } + // Start from the default estate, not the ambient one: PULSE_MOCK_* settings + // can remove the standalone agents and clusters these tests look for. + config := mock.DefaultConfig + config.NodeCount = nodes + config.UpdateInterval = interval + mock.SetMockConfig(config) + mustSetMockEnabled(tb, true) + tb.Cleanup(func() { + mustSetMockEnabled(tb, false) + mock.SetMockConfig(previousConfig) + mustSetMockEnabled(tb, previous) + }) +} + +func newMockEstateMonitor() *Monitor { + return &Monitor{ + state: models.NewState(), + resourceStore: unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(unifiedresources.NewMemoryStore())), + } +} + +// publishedMockView returns the view the monitor currently caches for mock +// mode without building one. +func publishedMockView(m *Monitor) (monitorUnifiedStateView, bool) { + m.mockUnifiedViewMu.Lock() + defer m.mockUnifiedViewMu.Unlock() + return m.mockUnifiedView, m.mockUnifiedViewValid +} + +// sameMetricsTarget reports whether two lookups resolved the same target (or +// both resolved none). +func sameMetricsTarget(a, b *unifiedresources.MetricsTarget) bool { + if a == nil || b == nil { + return a == b + } + return *a == *b +} + +func waitForFixtureTicks(tb testing.TB, ticks uint64) { + tb.Helper() + target := mock.FixtureDataVersion() + ticks + // A starved host ticks slowly; the deadline only catches a ticker that + // is not running at all. + deadline := time.Now().Add(5 * time.Minute) + for mock.FixtureDataVersion() < target { + if time.Now().After(deadline) { + tb.Fatalf("fixture data version stayed below %d (at %d) for five minutes; the mock ticker is not running", target, mock.FixtureDataVersion()) + } + time.Sleep(20 * time.Millisecond) + } +} + +func resourceIDsWithTargets(tb testing.TB, view monitorUnifiedStateView) []string { + tb.Helper() + ids := make([]string, 0, len(view.resources)) + for _, resource := range view.resources { + if view.metricsTargets.MetricsTargetForResource(resource.ID) != nil { + ids = append(ids, resource.ID) + } + } + if len(ids) == 0 { + tb.Fatal("expected the mock estate to list resources with metrics targets") + } + return ids +} + +// BenchmarkMockMetricsTargetResolutionAfterTick measures one alert pass's +// lookups (every resource, three windowed metrics) against a view that was +// built before the fixture last ticked. It must stay a map read per lookup, so +// a pass costs microseconds however slowly the process runs, not an estate +// build per lookup. +func BenchmarkMockMetricsTargetResolutionAfterTick(b *testing.B) { + useMockEstate(b, mock.DefaultConfig.NodeCount, time.Second) + m := newMockEstateMonitor() + ids := resourceIDsWithTargets(b, m.currentUnifiedStateView()) + waitForFixtureTicks(b, 1) + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + for _, id := range ids { + for metric := 0; metric < 3; metric++ { + if m.MetricsTargetForResource(id) == nil { + b.Fatalf("no target for %q", id) + } + } + } + } + b.ReportMetric(float64(len(ids)*3), "lookups/op") +} + +// BenchmarkLiveRegistryMetricsTargetResolution measures the same pass against a +// store-backed registry, as real mode resolves it, right after an ingest (the +// registry's typed views not yet built) and once they are. +func BenchmarkLiveRegistryMetricsTargetResolution(b *testing.B) { + useMockEstate(b, mock.DefaultConfig.NodeCount, 5*time.Minute) + graph := mock.CurrentFixtureGraph() + snapshot := unifiedresources.SnapshotWithoutSources(graph.State, mock.SupplementalOwnedSources()) + mustSetMockEnabled(b, false) + + for _, views := range []string{"after-ingest", "views-built"} { + b.Run(views, func(b *testing.B) { + registry := unifiedresources.NewRegistry(nil) + registry.IngestSnapshot(snapshot) + adapter := unifiedresources.NewMonitorAdapter(registry) + m := &Monitor{state: models.NewState(), resourceStore: adapter} + + listed := registry.List() + ids := make([]string, 0, len(listed)) + for _, resource := range listed { + ids = append(ids, resource.ID) + } + if views == "views-built" { + adapter.VMs() + } + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if views == "after-ingest" { + b.StopTimer() + registry = unifiedresources.NewRegistry(nil) + registry.IngestSnapshot(snapshot) + adapter = unifiedresources.NewMonitorAdapter(registry) + m.resourceStore = adapter + b.StartTimer() + } + for _, id := range ids { + for metric := 0; metric < 3; metric++ { + m.MetricsTargetForResource(id) + } + } + } + b.ReportMetric(float64(len(ids)*3), "lookups/op") + }) + } +} diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 317d4130dc..5fe35958a8 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -1216,6 +1216,7 @@ type Monitor struct { mockUnifiedViewMu sync.Mutex mockUnifiedView monitorUnifiedStateView mockUnifiedViewVersion uint64 + mockUnifiedViewStructure uint64 // fixture structure revision the published view was built at mockUnifiedViewLinks []unifiedresources.ResourceLink mockUnifiedViewFixtureAt time.Time // fixture freshness the published view was built from mockUnifiedViewValid bool @@ -5049,10 +5050,20 @@ func (m *Monitor) MetricsTargetForResource(resourceID string) *unifiedresources. return nil } - if view := m.GetUnifiedReadStateOrSnapshot(); view != nil { - if resolver, ok := view.(MetricsTargetResourceStore); ok { - if target := resolver.MetricsTargetForResource(resourceID); target != nil { - return target + // Mock mode resolves from the cached view's captured targets and never + // asks the view again: this runs once per resource and windowed metric in + // every alert pass, and the fixture's data version moves on every tick + // (see mockMetricsTarget). + target, fromMockView := m.mockMetricsTarget(resourceID) + if target != nil { + return target + } + if !fromMockView { + if view := m.GetUnifiedReadStateOrSnapshot(); view != nil { + if resolver, ok := view.(MetricsTargetResourceStore); ok { + if target := resolver.MetricsTargetForResource(resourceID); target != nil { + return target + } } } } @@ -5210,7 +5221,12 @@ func (m *Monitor) currentUnifiedStateView() monitorUnifiedStateView { // Read the version and links before the snapshot so a tick or // rebuild landing in between caches newer data under an older // token (harmless rebuild next call) rather than ever serving stale - // data under a newer one. + // data under a newer one. The structure revision goes first: the + // fixture advances its data version before its revision, so a + // reader that sees a new revision also reads the new version and + // never memoizes the previous estate under it + // (mock.FixtureStructureRevision). + structure := mock.FixtureStructureRevision() version := mock.FixtureDataVersion() links := m.resourceStoreManualLinks() m.mockUnifiedViewMu.Lock() @@ -5255,6 +5271,7 @@ func (m *Monitor) currentUnifiedStateView() monitorUnifiedStateView { m.mockUnifiedView = view m.mockUnifiedViewFixtureAt = fixtureFreshness m.mockUnifiedViewVersion = version + m.mockUnifiedViewStructure = structure m.mockUnifiedViewLinks = links m.mockUnifiedViewValid = true m.mockUnifiedViewMu.Unlock() diff --git a/internal/monitoring/monitor_host_agents_test.go b/internal/monitoring/monitor_host_agents_test.go index b4127d27bf..46cf04cea2 100644 --- a/internal/monitoring/monitor_host_agents_test.go +++ b/internal/monitoring/monitor_host_agents_test.go @@ -5992,6 +5992,187 @@ func TestMockUnifiedViewAppliesOperatorManualLinks(t *testing.T) { } } +// Alert evaluation resolves a target per resource and windowed metric on every +// pass. A metric tick moves the fixture's data version, not what resources +// exist, so a lookup after a tick must be served from the cached view instead +// of building an estate. Before this was pinned, every lookup after a tick +// rebuilt the view, and a process too starved to finish one build within a tick +// rebuilt it for every lookup of the pass: startup never reached its listener. +func TestMockMetricsTargetLookupsDoNotRebuildTheViewAfterFixtureTicks(t *testing.T) { + useMockEstate(t, 4, time.Second) + m := newMockEstateMonitor() + + warm := m.currentUnifiedStateView() + ids := resourceIDsWithTargets(t, warm) + + waitForFixtureTicks(t, 2) + + for _, id := range ids { + want := warm.metricsTargets.MetricsTargetForResource(id) + got := m.MetricsTargetForResource(id) + if !sameMetricsTarget(got, want) { + t.Fatalf("MetricsTargetForResource(%q) = %+v after fixture ticks, want %+v", id, got, want) + } + } + + after, ok := publishedMockView(m) + if !ok || after.readState != warm.readState { + t.Fatal("target lookups rebuilt the unified view after a metric tick; they must reuse the view built for the current fixture structure") + } +} + +// The reused view answers for the estate it was built from. A structural +// change (here a larger estate) must reach the lookup, not keep serving the +// old resource set. +func TestMockMetricsTargetFollowsAStructuralFixtureChange(t *testing.T) { + useMockEstate(t, 3, 5*time.Minute) + m := newMockEstateMonitor() + + before := m.currentUnifiedStateView() + known := make(map[string]struct{}, len(before.resources)) + for _, resource := range before.resources { + known[resource.ID] = struct{}{} + } + + grown := mock.GetConfig() + grown.NodeCount++ + mock.SetMockConfig(grown) + + // Another monitor's view of the grown estate names the resources the + // first one lacks. The first monitor has not been asked for a view since + // the change, so the lookup itself has to notice the structure moved. + var added string + for _, resource := range newMockEstateMonitor().currentUnifiedStateView().resources { + if _, ok := known[resource.ID]; !ok && resource.Type == unifiedresources.ResourceTypeAgent { + added = resource.ID + break + } + } + if added == "" { + t.Fatal("the larger fixture estate lists no agent the first one did not") + } + if target := m.MetricsTargetForResource(added); target == nil { + t.Fatalf("MetricsTargetForResource(%q) = nil after the fixture grew; the cached view outlived its structure", added) + } +} + +// Reusing a view across ticks must not change an answer: every resource +// resolves to what the view's own registry resolves, and an ID the estate does +// not list resolves to nothing without building a view for it. +func TestMockMetricsTargetMatchesTheRegistryResolution(t *testing.T) { + useMockEstate(t, 4, 5*time.Minute) + m := newMockEstateMonitor() + + view := m.currentUnifiedStateView() + registryResolver, ok := view.readState.(MetricsTargetResourceStore) + if !ok { + t.Fatalf("mock view read state %T does not resolve metrics targets", view.readState) + } + for _, resource := range view.resources { + want := registryResolver.MetricsTargetForResource(resource.ID) + got := m.MetricsTargetForResource(resource.ID) + if !sameMetricsTarget(got, want) { + t.Fatalf("MetricsTargetForResource(%q) = %+v, want the registry's %+v", resource.ID, got, want) + } + if padded := m.MetricsTargetForResource(" " + resource.ID + " "); !sameMetricsTarget(padded, want) { + t.Fatalf("MetricsTargetForResource(%q padded) = %+v, want the canonical ID's %+v", resource.ID, padded, want) + } + } + if got := m.MetricsTargetForResource("not-a-mock-resource"); got != nil { + t.Fatalf("MetricsTargetForResource(unknown) = %+v, want nil", got) + } + if after, _ := publishedMockView(m); after.readState != view.readState { + t.Fatal("resolving an unknown ID rebuilt the cached view") + } +} + +// A lookup for an ID the estate does not list must stay cheap after a tick +// too: the view has no more to say about it than the one it replaces. +func TestMockMetricsTargetForUnknownIDDoesNotRebuildTheViewAfterFixtureTicks(t *testing.T) { + useMockEstate(t, 3, time.Second) + m := newMockEstateMonitor() + + warm := m.currentUnifiedStateView() + waitForFixtureTicks(t, 2) + for i := 0; i < 50; i++ { + if got := m.MetricsTargetForResource(fmt.Sprintf("unknown-%d", i)); got != nil { + t.Fatalf("MetricsTargetForResource(unknown) = %+v, want nil", got) + } + } + if after, _ := publishedMockView(m); after.readState != warm.readState { + t.Fatal("unknown-ID lookups rebuilt the unified view after a metric tick") + } +} + +// An operator link folds one resource into another, which changes what the +// folded ID resolves to. Whatever rebuilds the view first, a lookup after the +// resource store applied the link must not resolve the folded resource. +func TestMockMetricsTargetFollowsAnOperatorLink(t *testing.T) { + useMockEstate(t, 4, 5*time.Minute) + store := unifiedresources.NewMemoryStore() + m := &Monitor{ + state: models.NewState(), + resourceStore: unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(store)), + alertManager: alerts.NewManager(), + } + t.Cleanup(m.alertManager.Stop) + + var vmID, agentID string + for _, resource := range m.currentUnifiedStateView().resources { + switch { + case vmID == "" && resource.Type == unifiedresources.ResourceTypeVM && resource.Agent == nil: + vmID = resource.ID + case agentID == "" && resource.Type == unifiedresources.ResourceTypeAgent && resource.Agent != nil && + len(resource.Sources) == 1 && resource.Sources[0] == unifiedresources.SourceAgent: + agentID = resource.ID + } + } + if vmID == "" || agentID == "" { + t.Fatalf("fixture graph needs an agentless VM and a standalone agent, got vm=%q agent=%q", vmID, agentID) + } + if m.MetricsTargetForResource(agentID) == nil || m.MetricsTargetForResource(vmID) == nil { + t.Fatal("expected the unlinked VM and agent to resolve metrics targets") + } + + if err := store.AddLink(unifiedresources.ResourceLink{ResourceA: vmID, ResourceB: agentID, PrimaryID: vmID}); err != nil { + t.Fatalf("AddLink: %v", err) + } + // Rebuild the resource store so it loads the link, without asking for a + // mock view: the lookup itself has to see the link list change. + m.updateResourceStore(models.StateSnapshot{}, m.mockModeFence.begin()) + + if got := m.MetricsTargetForResource(agentID); got != nil { + t.Fatalf("MetricsTargetForResource(folded agent) = %+v after the operator link, want nothing", got) + } + if m.MetricsTargetForResource(vmID) == nil { + t.Fatal("the linked VM lost its metrics target") + } +} + +// The reused view was built under one link list; once the resource store +// applies a different one, a lookup must rebuild rather than answer from it. +// A rebuild by any other consumer would hide a missing check, so the list the +// view was built under is changed directly. +func TestMockMetricsTargetRebuildsWhenTheLinkListMoves(t *testing.T) { + useMockEstate(t, 3, 5*time.Minute) + m := newMockEstateMonitor() + + warm := m.currentUnifiedStateView() + id := resourceIDsWithTargets(t, warm)[0] + + m.mockUnifiedViewMu.Lock() + m.mockUnifiedViewLinks = []unifiedresources.ResourceLink{{ResourceA: "stale-a", ResourceB: "stale-b", PrimaryID: "stale-a"}} + m.mockUnifiedViewMu.Unlock() + + if m.MetricsTargetForResource(id) == nil { + t.Fatalf("MetricsTargetForResource(%q) = nil", id) + } + after, _ := publishedMockView(m) + if after.readState == warm.readState { + t.Fatal("a lookup answered from a view built under a different operator link list") + } +} + // TestMockModeDiscardsRealHostReports pins the push-side of the mock clean // room. Mock mode already suspends pull-based PVE/PBS/PMG collection, but a // real agent keeps POSTing regardless; ingesting those reports lands a real diff --git a/internal/monitoring/monitor_pve_guest_builders.go b/internal/monitoring/monitor_pve_guest_builders.go index 37899bdbe8..0895e3b761 100644 --- a/internal/monitoring/monitor_pve_guest_builders.go +++ b/internal/monitoring/monitor_pve_guest_builders.go @@ -890,8 +890,10 @@ func (m *Monitor) updateVMDisksFromGuestAgentFSInfo( summary := m.summarizeVMFSInfo(instanceName, res, fsInfo) - // If we got valid data from guest agent, use it - if summary.totalBytes > 0 { + // Valid peer rows are not complete guest usage if any reading was rejected. + // The wire client reports that distinction as an error; alternate clients + // can supply invalid counters directly, so enforce it at this boundary too. + if summary.totalBytes > 0 && !summary.invalidBytes { // Sanity check: if the reported disk is way larger than allocated disk, // we might be getting host disk info somehow allocatedDiskGB := float64(res.MaxDisk) / 1073741824 diff --git a/internal/unifiedresources/clone_test.go b/internal/unifiedresources/clone_test.go index 5e3a88e1b2..a5508f29c6 100644 --- a/internal/unifiedresources/clone_test.go +++ b/internal/unifiedresources/clone_test.go @@ -273,8 +273,11 @@ func TestCloneResource_MutateParentBySource(t *testing.T) { // registry's listing can name each link's pair, and stay detached from them. func TestCloneResource_CarriesLinkFolds(t *testing.T) { original := &Resource{ - ID: "vm-1", - linkFolds: []ManualLinkFold{{HolderID: "vm-1", FoldedID: "agent-1", Sources: []DataSource{SourceAgent}}}, + ID: "vm-1", + linkFolds: []ManualLinkFold{{ + HolderID: "vm-1", FoldedID: "agent-1", Sources: []DataSource{SourceAgent}, + HolderOwn: []DataSource{SourceProxmox}, FoldedOwn: []DataSource{SourceAgent}, + }}, } cloned := cloneResource(original) if len(cloned.linkFolds) != 1 || cloned.linkFolds[0].FoldedID != "agent-1" { @@ -282,8 +285,11 @@ func TestCloneResource_CarriesLinkFolds(t *testing.T) { } cloned.linkFolds[0].Sources[0] = SourceDocker + cloned.linkFolds[0].HolderOwn[0] = SourceDocker + cloned.linkFolds[0].FoldedOwn[0] = SourceDocker cloned.linkFolds[0].FoldedID = "MUTATED" - if original.linkFolds[0].Sources[0] != SourceAgent || original.linkFolds[0].FoldedID != "agent-1" { + if original.linkFolds[0].Sources[0] != SourceAgent || original.linkFolds[0].FoldedID != "agent-1" || + original.linkFolds[0].HolderOwn[0] != SourceProxmox || original.linkFolds[0].FoldedOwn[0] != SourceAgent { t.Errorf("mutating cloned link folds affected the original: %+v", original.linkFolds) } } diff --git a/internal/unifiedresources/code_standards_test.go b/internal/unifiedresources/code_standards_test.go index 8cc0b23cf7..8b911ca2a2 100644 --- a/internal/unifiedresources/code_standards_test.go +++ b/internal/unifiedresources/code_standards_test.go @@ -771,6 +771,7 @@ func TestResourceAPIReportMergeExcludesRegistryLinkFolds(t *testing.T) { for _, snippet := range []string{ "linkFolds := registry.ManualLinkFolds(path)", "if len(resource.Sources) < 2 && len(linkFolds) == 0 {", + "for _, fold := range unified.ReportedManualLinkFolds(path, linkFolds, selected) {", "pairs = append(pairs, exclusionPair{fold.HolderID, fold.FoldedID})", } { if !strings.Contains(source, snippet) { diff --git a/internal/unifiedresources/manual_link_folds.go b/internal/unifiedresources/manual_link_folds.go index 4091d9e66a..916c8883b2 100644 --- a/internal/unifiedresources/manual_link_folds.go +++ b/internal/unifiedresources/manual_link_folds.go @@ -6,11 +6,17 @@ package unifiedresources // (the store keeps one decision per pair). A candidate ID derived from the // merged resource's type cannot: for an agent linked into a VM it is a vm- ID // that names neither side. Sources lists what the folded side brought, -// including anything links had already folded into it. +// including anything links had already folded into it. HolderOwn and +// FoldedOwn list what each side is on its own: the sources its row listed +// before any link folded something into it. A row that took other members in +// lists theirs too, so Sources alone cannot say which member of a chain a +// source belongs to. type ManualLinkFold struct { - HolderID string - FoldedID string - Sources []DataSource + HolderID string + FoldedID string + Sources []DataSource + HolderOwn []DataSource + FoldedOwn []DataSource } // recordManualLinkFold notes on the link's merged resource that it took in @@ -24,13 +30,19 @@ type ManualLinkFold struct { // with fewer sources than it brought before, so a repeated pair adds its // sources to the fold already recorded. The holder still carries what the // earlier fold brought. +// +// Call it before the other side's data is merged into primary: the sources +// primary lists at that point are its own unless an earlier fold on it +// recorded them (linkMemberOwnSources). func recordManualLinkFold(primary *Resource, primaryID string, other *Resource, otherID string) { added := make([]ManualLinkFold, 0, len(other.linkFolds)+1) added = append(added, other.linkFolds...) added = append(added, ManualLinkFold{ - HolderID: primaryID, - FoldedID: otherID, - Sources: cloneDataSourceSlice(other.Sources), + HolderID: primaryID, + FoldedID: otherID, + Sources: cloneDataSourceSlice(other.Sources), + HolderOwn: linkMemberOwnSources(primary, primaryID), + FoldedOwn: linkMemberOwnSources(other, otherID), }) folds := make([]ManualLinkFold, 0, len(primary.linkFolds)+len(added)) recorded := make(map[string]int, len(primary.linkFolds)+len(added)) @@ -38,6 +50,12 @@ func recordManualLinkFold(primary *Resource, primaryID string, other *Resource, key := exclusionKey(fold.HolderID, fold.FoldedID) if i, ok := recorded[key]; ok { folds[i].Sources = addSources(cloneDataSourceSlice(folds[i].Sources), fold.Sources) + holderOwn, foldedOwn := fold.HolderOwn, fold.FoldedOwn + if folds[i].HolderID != fold.HolderID { + holderOwn, foldedOwn = foldedOwn, holderOwn + } + folds[i].HolderOwn = addSources(cloneDataSourceSlice(folds[i].HolderOwn), holderOwn) + folds[i].FoldedOwn = addSources(cloneDataSourceSlice(folds[i].FoldedOwn), foldedOwn) return } recorded[key] = len(folds) @@ -72,7 +90,129 @@ func cloneManualLinkFolds(in []ManualLinkFold) []ManualLinkFold { out := make([]ManualLinkFold, len(in)) for i, fold := range in { fold.Sources = cloneDataSourceSlice(fold.Sources) + fold.HolderOwn = cloneDataSourceSlice(fold.HolderOwn) + fold.FoldedOwn = cloneDataSourceSlice(fold.FoldedOwn) out[i] = fold } return out } + +// linkMemberOwnSources returns what the member id is on its own: the sources +// an earlier fold on row recorded for it, else the sources row lists, which +// are its own while no link has folded anything into it. +func linkMemberOwnSources(row *Resource, id string) []DataSource { + for _, fold := range row.linkFolds { + if fold.HolderID == id && fold.HolderOwn != nil { + return cloneDataSourceSlice(fold.HolderOwn) + } + if fold.FoldedID == id && fold.FoldedOwn != nil { + return cloneDataSourceSlice(fold.FoldedOwn) + } + } + return cloneDataSourceSlice(row.Sources) +} + +// ReportedManualLinkFolds picks, from the folds of the merged resource rootID, +// the links a report-merge undoes. reported says whether any of the sources +// it is given was named by the report; a report naming no source reports +// every source. +// +// A report names sources, and the sources belong to link members: the +// resources the folds' pairs name. A member is reported when its own sources +// include a reported one, whatever it took in; the resource being reported on +// is never one, since it is what stays. Reported members leave the part of +// the chain that stays, so a link is undone when it joins a reported member +// to a member that stays or to another reported member. A member that hangs +// below a reported one and carries no reported source goes with it, and the +// link that holds it there stays: the report names a source that is not +// theirs to undo. A member that stays is one the root still reaches without +// passing through a reported member, which matters where links form a cycle: +// a reported member linked both to the root and to a member that stays is cut +// from both, so it cannot rejoin through the other. +// +// A fold's Sources lists its folded side's whole subtree, so selecting folds +// by them undid every link along the way to a reported source: a source +// brought by a leaf split the leaf's holder from the root as well. +func ReportedManualLinkFolds(rootID string, folds []ManualLinkFold, reported func(...DataSource) bool) []ManualLinkFold { + if len(folds) == 0 { + return nil + } + // A member named by several folds takes the union of what they record: a + // repeated pair unions into its own fold only, so a member whose refold + // gained a source would otherwise read differently from fold to fold. + own := make(map[string][]DataSource, len(folds)+1) + adjacent := make(map[string][]string, len(folds)+1) + folded := make(map[string]bool, len(folds)) + for _, fold := range folds { + if fold.HolderOwn != nil { + own[fold.HolderID] = addSources(own[fold.HolderID], fold.HolderOwn) + } + if fold.FoldedOwn != nil { + own[fold.FoldedID] = addSources(own[fold.FoldedID], fold.FoldedOwn) + } + adjacent[fold.HolderID] = append(adjacent[fold.HolderID], fold.FoldedID) + adjacent[fold.FoldedID] = append(adjacent[fold.FoldedID], fold.HolderID) + folded[fold.FoldedID] = true + } + // A fold recorded without its sides' own sources lists the folded side's + // whole subtree, the most that side can have been. + for _, fold := range folds { + if _, ok := own[fold.FoldedID]; !ok { + own[fold.FoldedID] = fold.Sources + } + } + + root := rootID + if _, ok := adjacent[root]; !ok { + // The root was re-keyed after the links folded: the one holder no + // link folded is the resource the records hang from. + root = "" + for _, fold := range folds { + if folded[fold.HolderID] { + continue + } + if root != "" && root != fold.HolderID { + root = "" + break + } + root = fold.HolderID + } + } + if root == "" { + var selected []ManualLinkFold + for _, fold := range folds { + if reported(fold.Sources...) { + selected = append(selected, fold) + } + } + return selected + } + + away := make(map[string]bool, len(adjacent)) + for id := range adjacent { + if id != root && reported(own[id]...) { + away[id] = true + } + } + stays := map[string]bool{root: true} + for queue := []string{root}; len(queue) > 0; { + current := queue[0] + queue = queue[1:] + for _, next := range adjacent[current] { + if away[next] || stays[next] { + continue + } + stays[next] = true + queue = append(queue, next) + } + } + carried := func(id string) bool { return !away[id] && !stays[id] } + + var selected []ManualLinkFold + for _, fold := range folds { + if (away[fold.HolderID] || away[fold.FoldedID]) && !carried(fold.HolderID) && !carried(fold.FoldedID) { + selected = append(selected, fold) + } + } + return selected +} diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index af3d95811e..1e1aa35333 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -5068,8 +5068,10 @@ func (rr *ResourceRegistry) applyManualLinks(thresholds map[DataSource]time.Dura rr.recordLinkOwnPin(primaryID, primary) rr.recordLinkOwnPin(otherID, other) - rr.mergeResourceData(primary, other, thresholds) + // The fold records what each side is on its own, so it reads the + // primary before the other side's sources join its list. recordManualLinkFold(primary, primaryID, other, otherID) + rr.mergeResourceData(primary, other, thresholds) delete(rr.resources, otherID) // The fold record names every ID along a chain of links, so the chain // resolves to its last primary; the folded row's own index entries diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index d8dbbf061d..d8bb80ed7c 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -1958,9 +1958,17 @@ func TestManualLinkFoldsNameEachLinkPairAlongAChain(t *testing.T) { // The agent linked from its own side still folds into the guest it runs // in, and the fold names the link's pair either way. + // Each side also records what it is on its own: the agent's fold into the VM + // lists the Docker host's source too, but the agent is only the agent. want := []ManualLinkFold{ - {HolderID: agentID, FoldedID: dockerID, Sources: []DataSource{SourceDocker}}, - {HolderID: vmID, FoldedID: agentID, Sources: []DataSource{SourceAgent, SourceDocker}}, + { + HolderID: agentID, FoldedID: dockerID, Sources: []DataSource{SourceDocker}, + HolderOwn: []DataSource{SourceAgent}, FoldedOwn: []DataSource{SourceDocker}, + }, + { + HolderID: vmID, FoldedID: agentID, Sources: []DataSource{SourceAgent, SourceDocker}, + HolderOwn: []DataSource{SourceProxmox}, FoldedOwn: []DataSource{SourceAgent}, + }, } if got := adapter.currentRegistry().ManualLinkFolds(vmID); !reflect.DeepEqual(got, want) { t.Fatalf("monitor folds = %+v, want %+v", got, want) @@ -2023,7 +2031,10 @@ func TestManualLinkFoldsRecordEachPairOnceAcrossRecordIngests(t *testing.T) { if listed := rr.List(); len(listed) != 1 { t.Fatalf("link did not fold the TrueNAS system into the VM: %+v", listed) } - want := []ManualLinkFold{{HolderID: vmID, FoldedID: systemID, Sources: []DataSource{SourceTrueNAS}}} + want := []ManualLinkFold{{ + HolderID: vmID, FoldedID: systemID, Sources: []DataSource{SourceTrueNAS}, + HolderOwn: []DataSource{SourceProxmox}, FoldedOwn: []DataSource{SourceTrueNAS}, + }} if got := rr.ManualLinkFolds(vmID); !reflect.DeepEqual(got, want) { t.Fatalf("folds after repeated record ingests = %+v, want %+v", got, want) } @@ -2037,12 +2048,184 @@ func TestManualLinkFoldRepeatedPairKeepsEarlierSources(t *testing.T) { holder := &Resource{ID: "vm-1"} recordManualLinkFold(holder, "vm-1", &Resource{ID: "agent-1", Sources: []DataSource{SourceAgent, SourceDocker}}, "agent-1") recordManualLinkFold(holder, "vm-1", &Resource{ID: "agent-1", Sources: []DataSource{SourceAgent}}, "agent-1") - want := []ManualLinkFold{{HolderID: "vm-1", FoldedID: "agent-1", Sources: []DataSource{SourceAgent, SourceDocker}}} + want := []ManualLinkFold{{ + HolderID: "vm-1", FoldedID: "agent-1", Sources: []DataSource{SourceAgent, SourceDocker}, + FoldedOwn: []DataSource{SourceAgent, SourceDocker}, + }} if !reflect.DeepEqual(holder.linkFolds, want) { t.Fatalf("folds = %+v, want %+v", holder.linkFolds, want) } } +// A holder that already took a member in lists that member's sources too, so +// its own sources come from the record the first fold made, not from its row. +func TestManualLinkFoldOwnSourcesComeFromTheFirstRecord(t *testing.T) { + holder := &Resource{ID: "vm-1", Sources: []DataSource{SourceProxmox}} + first := &Resource{ID: "agent-1", Sources: []DataSource{SourceAgent}} + recordManualLinkFold(holder, "vm-1", first, "agent-1") + holder.Sources = addSources(holder.Sources, first.Sources) + second := &Resource{ID: "docker-1", Sources: []DataSource{SourceDocker}} + recordManualLinkFold(holder, "vm-1", second, "docker-1") + + if got := holder.linkFolds[1].HolderOwn; !reflect.DeepEqual(got, []DataSource{SourceProxmox}) { + t.Fatalf("second fold lists the holder's own sources as %v, want only %v", got, SourceProxmox) + } + + // The VM then folds into another resource: the member it was keeps its own + // sources, not the ones it took in. + root := &Resource{ID: "node-1", Sources: []DataSource{SourceTrueNAS}} + recordManualLinkFold(root, "node-1", holder, "vm-1") + last := root.linkFolds[len(root.linkFolds)-1] + if last.FoldedID != "vm-1" || !reflect.DeepEqual(last.FoldedOwn, []DataSource{SourceProxmox}) { + t.Fatalf("fold of the holder lists its own sources as %v, want only %v", last.FoldedOwn, SourceProxmox) + } + if !reflect.DeepEqual(last.HolderOwn, []DataSource{SourceTrueNAS}) { + t.Fatalf("fold into the root lists its own sources as %v, want only %v", last.HolderOwn, SourceTrueNAS) + } +} + +// ReportedManualLinkFolds picks the links a source-filtered report undoes by +// member. The members here are the estate that exposed the fold subtree +// rule: a TrueNAS VM V takes in a Proxmox storage S and a vSphere VM W, and +// the operator also linked S into W, so recording every link of the chain +// leaves three pairs with W reachable through two of them. +func TestReportedManualLinkFoldsDetachMembersCarryingTheSource(t *testing.T) { + pair := func(holder, folded string, sources, holderOwn, foldedOwn []DataSource) ManualLinkFold { + return ManualLinkFold{HolderID: holder, FoldedID: folded, Sources: sources, HolderOwn: holderOwn, FoldedOwn: foldedOwn} + } + named := func(names ...DataSource) func(...DataSource) bool { + return func(sources ...DataSource) bool { + if len(names) == 0 { + return true + } + for _, source := range sources { + if slices.Contains(names, source) { + return true + } + } + return false + } + } + pairs := func(folds []ManualLinkFold) []string { + out := make([]string, 0, len(folds)) + for _, fold := range folds { + out = append(out, fold.HolderID+">"+fold.FoldedID) + } + slices.Sort(out) + return out + } + truenas, proxmox, vmware := []DataSource{SourceTrueNAS}, []DataSource{SourceProxmox}, []DataSource{SourceVMware} + cycle := []ManualLinkFold{ + pair("V", "S", proxmox, truenas, proxmox), + pair("V", "W", vmware, truenas, vmware), + pair("W", "S", proxmox, vmware, proxmox), + } + // A leaf below a holder below the root: V takes in A (agent), which took + // in D (docker), so A's fold lists both sources. + agent, docker := []DataSource{SourceAgent}, []DataSource{SourceDocker} + tree := []ManualLinkFold{ + pair("A", "D", docker, agent, docker), + pair("V", "A", []DataSource{SourceAgent, SourceDocker}, proxmox, agent), + } + + for _, tc := range []struct { + name string + root string + folds []ManualLinkFold + named []DataSource + want []string + }{ + {"cycle: member reachable two ways is cut from both", "V", cycle, []DataSource{SourceVMware}, []string{"V>W", "W>S"}}, + {"cycle: the storage leaves, the VM keeps the other link", "V", cycle, []DataSource{SourceProxmox}, []string{"V>S", "W>S"}}, + {"cycle: both reported members leave", "V", cycle, []DataSource{SourceVMware, SourceProxmox}, []string{"V>S", "V>W", "W>S"}}, + {"cycle: the root's own source selects nothing", "V", cycle, []DataSource{SourceTrueNAS}, nil}, + {"cycle: no filter undoes every link", "V", cycle, nil, []string{"V>S", "V>W", "W>S"}}, + {"tree: a leaf's source leaves its holder's link alone", "V", tree, []DataSource{SourceDocker}, []string{"A>D"}}, + {"tree: a holder leaves and keeps what it took in", "V", tree, []DataSource{SourceAgent}, []string{"V>A"}}, + {"tree: both reported, both leave", "V", tree, []DataSource{SourceAgent, SourceDocker}, []string{"A>D", "V>A"}}, + {"tree: the root's own source selects nothing", "V", tree, []DataSource{SourceProxmox}, nil}, + {"tree: no filter undoes every link", "V", tree, nil, []string{"A>D", "V>A"}}, + // A refold of the agent unions a source into the VM's fold of it only, + // so the agent's two records disagree; the member is the union. + {"tree: a member whose refold gained the source is reported by it", "V", []ManualLinkFold{ + pair("A", "D", docker, agent, docker), + pair("V", "A", []DataSource{SourceAgent, SourceDocker}, proxmox, []DataSource{SourceAgent, SourceDocker}), + }, []DataSource{SourceDocker}, []string{"A>D", "V>A"}}, + {"root re-keyed after the links folded", "V-renamed", tree, []DataSource{SourceDocker}, []string{"A>D"}}, + {"ambiguous root falls back to the folded subtree", "elsewhere", []ManualLinkFold{ + pair("X", "Y", docker, nil, nil), pair("P", "Q", agent, nil, nil), + }, []DataSource{SourceDocker}, []string{"X>Y"}}, + {"folds recorded without own sources use the folded subtree", "V", []ManualLinkFold{ + {HolderID: "V", FoldedID: "A", Sources: []DataSource{SourceAgent, SourceDocker}}, + {HolderID: "A", FoldedID: "D", Sources: docker}, + }, []DataSource{SourceDocker}, []string{"A>D", "V>A"}}, + } { + t.Run(tc.name, func(t *testing.T) { + got := pairs(ReportedManualLinkFolds(tc.root, tc.folds, named(tc.named...))) + if !slices.Equal(got, tc.want) { + t.Fatalf("report of %v selected %v, want %v", tc.named, got, tc.want) + } + }) + } +} + +// Whatever links a report undoes, no member carrying a reported source may +// stay joined to the root through the links left. The rule the registry +// replaced selected folds whose subtree sources named the report, and left W +// joined through S in the cycle: its control here fails the same check. +func TestReportedManualLinkFoldsLeaveNoReportedMemberJoined(t *testing.T) { + truenas, proxmox, vmware := []DataSource{SourceTrueNAS}, []DataSource{SourceProxmox}, []DataSource{SourceVMware} + cycle := []ManualLinkFold{ + {HolderID: "V", FoldedID: "S", Sources: proxmox, HolderOwn: truenas, FoldedOwn: proxmox}, + {HolderID: "V", FoldedID: "W", Sources: vmware, HolderOwn: truenas, FoldedOwn: vmware}, + {HolderID: "W", FoldedID: "S", Sources: proxmox, HolderOwn: vmware, FoldedOwn: proxmox}, + } + joinedToRoot := func(folds []ManualLinkFold, left map[string]bool, member string) bool { + reached := map[string]bool{"V": true} + for changed := true; changed; { + changed = false + for _, fold := range folds { + if left[exclusionKey(fold.HolderID, fold.FoldedID)] { + continue + } + if reached[fold.HolderID] != reached[fold.FoldedID] { + reached[fold.HolderID], reached[fold.FoldedID] = true, true + changed = true + } + } + } + return reached[member] + } + undone := func(selected []ManualLinkFold) map[string]bool { + out := make(map[string]bool, len(selected)) + for _, fold := range selected { + out[exclusionKey(fold.HolderID, fold.FoldedID)] = true + } + return out + } + for _, tc := range []struct { + reported DataSource + member string + }{{SourceVMware, "W"}, {SourceProxmox, "S"}} { + named := func(sources ...DataSource) bool { return slices.Contains(sources, tc.reported) } + got := ReportedManualLinkFolds("V", cycle, named) + if joinedToRoot(cycle, undone(got), tc.member) { + t.Errorf("after a report of %s, %s is still joined to V through %v", tc.reported, tc.member, got) + } + + // The control: folds selected by their folded side's subtree. + var bySubtree []ManualLinkFold + for _, fold := range cycle { + if named(fold.Sources...) { + bySubtree = append(bySubtree, fold) + } + } + if tc.reported == SourceVMware && !joinedToRoot(cycle, undone(bySubtree), tc.member) { + t.Errorf("control: selecting by subtree sources detached %s, so the cycle no longer shows the defect", tc.member) + } + } +} + func TestLeaseExpiredKubernetesClusterStaysOffline(t *testing.T) { rr := NewRegistry(nil) rr.IngestSnapshot(models.StateSnapshot{ diff --git a/pkg/proxmox/client.go b/pkg/proxmox/client.go index 9d80472c11..ccca7b3c56 100644 --- a/pkg/proxmox/client.go +++ b/pkg/proxmox/client.go @@ -1706,13 +1706,21 @@ func (fs *VMFileSystem) UnmarshalJSON(data []byte) error { if !valid { return fmt.Errorf("guest filesystem record is ambiguous or malformed") } + recognized := false for name := range fields { + switch name { + case "name", "type", "mountpoint", "total-bytes", "total-bytes-privileged", "used-bytes", "disk": + recognized = true + } for _, canonical := range []string{"total-bytes", "total-bytes-privileged", "used-bytes"} { if name != canonical && strings.EqualFold(name, canonical) { return fmt.Errorf("guest filesystem byte count has a noncanonical field name") } } } + if !recognized { + return fmt.Errorf("guest filesystem record has no recognized fields") + } type rawVMFileSystem struct { Name string `json:"name"` Type string `json:"type"` @@ -2023,7 +2031,9 @@ func coerceString(value interface{}) string { } } -// GetVMFSInfo returns filesystem information from QEMU guest agent +// GetVMFSInfo returns filesystem information from QEMU guest agent. If any +// record is malformed, valid peers are returned with an error: they cannot +// establish complete guest capacity or removal of the rejected filesystem. func (c *Client) GetVMFSInfo(ctx context.Context, node string, vmid int) ([]VMFileSystem, error) { resp, err := c.get(ctx, fmt.Sprintf("/nodes/%s/qemu/%d/agent/get-fsinfo", node, vmid)) if err != nil { @@ -2045,8 +2055,8 @@ func (c *Client) GetVMFSInfo(ctx context.Context, node string, vmid int) ([]VMFi Str("response", string(bodyBytes)). Msg("Raw response from guest agent get-fsinfo") - // Decode array payloads entry-by-entry so one malformed filesystem record - // does not wipe valid guest-agent disk data for the whole VM. + // Decode entry-by-entry to retain valid peers, but never mistake that + // remainder for a complete inventory after dropping a malformed record. var arrayResult struct { Data struct { Result []json.RawMessage `json:"result"` @@ -2054,9 +2064,11 @@ func (c *Client) GetVMFSInfo(ctx context.Context, node string, vmid int) ([]VMFi } if err := json.Unmarshal(bodyBytes, &arrayResult); err == nil && arrayResult.Data.Result != nil { filesystems := make([]VMFileSystem, 0, len(arrayResult.Data.Result)) + invalidEntries := 0 for idx, rawFS := range arrayResult.Data.Result { var fs VMFileSystem if err := json.Unmarshal(rawFS, &fs); err != nil { + invalidEntries++ log.Warn(). Err(err). Str("node", node). @@ -2068,6 +2080,9 @@ func (c *Client) GetVMFSInfo(ctx context.Context, node string, vmid int) ([]VMFi filesystems = append(filesystems, fs) } postProcessVMFilesystems(node, vmid, filesystems) + if invalidEntries > 0 { + return filesystems, fmt.Errorf("guest filesystem inventory is incomplete: %d malformed entries", invalidEntries) + } return filesystems, nil } diff --git a/pkg/proxmox/client_api_more3_test.go b/pkg/proxmox/client_api_more3_test.go index 240dce4cef..db03647575 100644 --- a/pkg/proxmox/client_api_more3_test.go +++ b/pkg/proxmox/client_api_more3_test.go @@ -261,8 +261,8 @@ func TestClientVMFSInfoSkipsMalformedEntries(t *testing.T) { ctx := context.Background() filesystems, err := client.GetVMFSInfo(ctx, "node1", 100) - if err != nil { - t.Fatalf("GetVMFSInfo error: %v", err) + if err == nil { + t.Fatal("malformed peer must prevent a complete filesystem inventory") } if len(filesystems) != 1 { t.Fatalf("expected malformed entry to be skipped, got %+v", filesystems) diff --git a/pkg/proxmox/cluster_client.go b/pkg/proxmox/cluster_client.go index fde1cffbec..b30569170c 100644 --- a/pkg/proxmox/cluster_client.go +++ b/pkg/proxmox/cluster_client.go @@ -1466,11 +1466,10 @@ func (cc *ClusterClient) GetVMFSInfo(ctx context.Context, node string, vmid int) var result []VMFileSystem err := cc.executeGuestAgent(ctx, func(client *Client) error { info, err := client.GetVMFSInfo(ctx, node, vmid) - if err != nil { - return err - } + // Preserve valid peers and the incomplete-inventory error together. + // Neither a partial response nor its error grants another endpoint read. result = info - return nil + return err }) return result, err } diff --git a/pkg/proxmox/guest_filesystem_bytes_test.go b/pkg/proxmox/guest_filesystem_bytes_test.go index c0aa9c4379..82f9b17340 100644 --- a/pkg/proxmox/guest_filesystem_bytes_test.go +++ b/pkg/proxmox/guest_filesystem_bytes_test.go @@ -89,7 +89,7 @@ func TestVMFilesystemBytesWireAdmissionKeepsValidPeersAndObjectPrecision(t *test total, used uint64 wantError bool }{ - "array-peers": {`[{"mountpoint":"/bad","type":"ext4","total-bytes":1000},{"mountpoint":"/negative","type":"ext4","total-bytes":1000,"used-bytes":-1},{"mountpoint":"/overfull","type":"ext4","total-bytes":1000,"used-bytes":1001},{"mountpoint":"/","type":"ext4","total-bytes":1000,"used-bytes":0}]`, 1000, 0, false}, + "array-peers": {`[{"mountpoint":"/bad","type":"ext4","total-bytes":1000},{"mountpoint":"/negative","type":"ext4","total-bytes":1000,"used-bytes":-1},{"mountpoint":"/overfull","type":"ext4","total-bytes":1000,"used-bytes":1001},{"mountpoint":"/","type":"ext4","total-bytes":1000,"used-bytes":0}]`, 1000, 0, true}, "object-precision": {`{"mountpoint":"C:\\","type":"ntfs","total-bytes":9007199254740993,"used-bytes":9007199254740991,"disk":{"bus-type":"scsi","target":2}}`, 9007199254740993, 9007199254740991, false}, "object-duplicate": {`{"mountpoint":"C:\\","type":"ntfs","total-bytes":1000,"used-bytes":1001,"used-bytes":0}`, 0, 0, true}, "object-missing": {`{"mountpoint":"C:\\","type":"ntfs","total-bytes":1000}`, 0, 0, true}, @@ -112,9 +112,16 @@ func TestVMFilesystemBytesWireAdmissionKeepsValidPeersAndObjectPrecision(t *test c := backupTestClient(t, server.URL) readings, err := c.GetVMFSInfo(context.Background(), "node", 105) if tc.wantError { - if err == nil || len(readings) != 0 { + if err == nil { t.Fatalf("invalid object admitted: %+v %v", readings, err) } + if name == "array-peers" { + if len(readings) != 1 || readings[0].TotalBytes != tc.total || readings[0].UsedBytes != tc.used { + t.Fatalf("valid peers lost with incomplete inventory error: %+v %v", readings, err) + } + } else if len(readings) != 0 { + t.Fatalf("invalid object returned readings: %+v", readings) + } } else { if err != nil || len(readings) != 1 { t.Fatalf("valid readings lost: %+v %v", readings, err) diff --git a/pkg/proxmox/guest_filesystem_completeness_test.go b/pkg/proxmox/guest_filesystem_completeness_test.go new file mode 100644 index 0000000000..0226d6a304 --- /dev/null +++ b/pkg/proxmox/guest_filesystem_completeness_test.go @@ -0,0 +1,88 @@ +package proxmox + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +// A malformed row is not an absent volume. Keep the valid peers, but make the +// completeness failure visible through both ordinary client paths. This is a +// completed synthetic QGA reply, not a native freeze/recovery experiment. +func TestVMFilesystemIncompleteInventoryKeepsPeersAndSingleAttempt(t *testing.T) { + for _, path := range []string{"single", "cluster"} { + for name, bad := range map[string]string{ + "missing-used": `{"mountpoint":"/data","type":"ext4","total-bytes":9000}`, + "overused": `{"mountpoint":"/data","type":"ext4","total-bytes":9000,"used-bytes":9001}`, + "duplicate": `{"mountpoint":"/data","type":"ext4","total-bytes":9000,"used-bytes":8800,"used-bytes":0}`, + "null-record": `null`, + "scalar-record": `42`, + "empty-record": `{}`, + "unknown-record": `{"unknown":"value"}`, + } { + t.Run(path+"/"+name, func(t *testing.T) { + var commands, configs atomic.Int32 + var corrected atomic.Bool + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/nodes"): + fmt.Fprint(w, `{"data":[]}`) + case strings.HasSuffix(r.URL.Path, "/config"): + configs.Add(1) + fmt.Fprint(w, `{"data":{}}`) + case strings.Contains(r.URL.Path, "/agent/"): + commands.Add(1) + if strings.HasSuffix(r.URL.Path, "/get-fsinfo") { + peer := `{"mountpoint":"/","type":"ext4","total-bytes":1000,"used-bytes":0}` + if corrected.Load() { + fmt.Fprintf(w, `{"data":{"result":[%s]}}`, peer) + } else { + fmt.Fprintf(w, `{"data":{"result":[%s,%s]}}`, bad, peer) + } + } else { + backupAgentPayload(w, r) + } + default: + t.Errorf("unexpected request: %s", r.URL.Path) + http.NotFound(w, r) + } + }) + a, b := httptest.NewServer(handler), httptest.NewServer(handler) + defer a.Close() + defer b.Close() + var client interface { + GetVMFSInfo(context.Context, string, int) ([]VMFileSystem, error) + GetVMAgentInfo(context.Context, string, int) (map[string]interface{}, error) + } + if path == "cluster" { + client = NewClusterClient("partial-filesystems", ClientConfig{Host: a.URL, TokenName: "fixture@pve!pulse", TokenValue: "fixture", Timeout: time.Second}, []string{a.URL, b.URL}, nil) + } else { + client = backupTestClient(t, a.URL) + } + peers, err := client.GetVMFSInfo(context.Background(), "node", 105) + if err == nil || errors.Is(err, ErrGuestAgentDeferred) || len(peers) != 1 || peers[0].TotalBytes != 1000 || peers[0].UsedBytes != 0 || peers[0].Disk != "root-filesystem" { + t.Fatalf("partial inventory lost failure or valid peer: %+v / %v", peers, err) + } + if commands.Load() != 1 || configs.Load() != 2 { + t.Fatalf("incomplete payload repeated/bypassed guest admission: commands=%d config=%d", commands.Load(), configs.Load()) + } + // Parsing failure is not uncertain command completion. Another + // ordinary metadata read remains eligible, not a forced probe. + if _, err := client.GetVMAgentInfo(context.Background(), "node", 105); err != nil { + t.Fatalf("completed malformed reading started a command cooldown: %v", err) + } + corrected.Store(true) + peers, err = client.GetVMFSInfo(context.Background(), "migrated", 105) + if err != nil || len(peers) != 1 || peers[0].UsedBytes != 0 || commands.Load() != 3 || configs.Load() != 6 { + t.Fatalf("complete ordinary removal/zero did not recover once: %+v / %v / commands=%d config=%d", peers, err, commands.Load(), configs.Load()) + } + }) + } + } +} diff --git a/scripts/installtests/api_guidance_test.go b/scripts/installtests/api_guidance_test.go index c134cd69b0..8a1937dca5 100644 --- a/scripts/installtests/api_guidance_test.go +++ b/scripts/installtests/api_guidance_test.go @@ -21,27 +21,80 @@ import ( // Execute the actual reference recipes against synthetic peers only. In // particular, action examples must not acquire an automatic retry or emit a // private response just because a POST failed or its response was incomplete. -func pulseAPIRecipes(doc string) (preparation, helper string, calls []string, err error) { +type pulseAPIRequestContract struct { + name, firstLine, method, uri, input string +} + +// Match each copied request to its own transport contract, not its position in +// the guide. New examples must gain a fixture rather than disabling every +// existing check with an unexplained recipe-count mismatch. +func pulseAPIRequestContracts() []pulseAPIRequestContract { + return []pulseAPIRequestContract{ + {"summary", "pulse_api GET /api/state/summary", "GET", "/api/state/summary", ""}, + {"connections", "pulse_api GET /api/connections", "GET", "/api/connections", ""}, + {"capabilities", "pulse_api GET /api/agent/resource-capabilities/vm%3A42", "GET", "/api/agent/resource-capabilities/vm%3A42", ""}, + {"plan", "pulse_api POST /api/actions/plan <<'JSON'", "POST", "/api/actions/plan", "heredoc"}, + {"decision", "pulse_api POST /api/actions/act_.../decision <<'JSON'", "POST", "/api/actions/act_.../decision", "heredoc"}, + {"execute", "pulse_api POST /api/actions/act_.../execute <<'JSON'", "POST", "/api/actions/act_.../execute", "heredoc"}, + {"audit", "pulse_api GET '/api/audit/actions?resourceId=vm%3A42&limit=10'", "GET", "/api/audit/actions?resourceId=vm%3A42&limit=10", ""}, + {"events", "pulse_api GET /api/audit/actions/act_.../events", "GET", "/api/audit/actions/act_.../events", ""}, + {"add-node", `pulse_api POST /api/config/nodes < "$node_request_file"`, "POST", "/api/config/nodes", "file"}, + } +} + +type pulseAPIRecipe struct { + contract pulseAPIRequestContract + command string +} + +func pulseAPIRecipes(doc string) (preparation, helper string, calls []pulseAPIRecipe, err error) { + contracts := make(map[string]pulseAPIRequestContract) + for _, contract := range pulseAPIRequestContracts() { + contracts[contract.firstLine] = contract + } + seen := make(map[string]bool) for _, match := range regexp.MustCompile("(?s)```bash\n(.*?)```").FindAllStringSubmatch(doc, -1) { block := match[1] switch { case strings.Contains(block, "api-header") && strings.Contains(block, "vi "): + if preparation != "" { + return "", "", nil, fmt.Errorf("duplicate credential preparation recipe") + } preparation = block case strings.HasPrefix(block, "pulse_api() ("): + if helper != "" { + return "", "", nil, fmt.Errorf("duplicate private-response helper") + } helper = block case strings.HasPrefix(block, "pulse_api "): - calls = append(calls, block) + firstLine, _, _ := strings.Cut(block, "\n") + contract, ok := contracts[firstLine] + if !ok { + return "", "", nil, fmt.Errorf("request example lacks a transport fixture: %s", firstLine) + } + if seen[firstLine] { + return "", "", nil, fmt.Errorf("duplicate request example: %s", contract.name) + } + seen[firstLine] = true + calls = append(calls, pulseAPIRecipe{contract, block}) + case strings.Contains(block, "pulse_api "): + return "", "", nil, fmt.Errorf("request example must start with the private-response helper") case strings.Contains(block, "curl "): return "", "", nil, fmt.Errorf("request bypasses the private-response helper") } } - if preparation == "" || helper == "" || len(calls) != 8 { - err = fmt.Errorf("missing credential/helper recipe or request examples (got %d)", len(calls)) + if preparation == "" || helper == "" { + return "", "", nil, fmt.Errorf("missing credential preparation or private-response helper") + } + for _, contract := range pulseAPIRequestContracts() { + if !seen[contract.firstLine] { + return "", "", nil, fmt.Errorf("missing request example: %s", contract.name) + } } return } -func pulseAPIReference(t *testing.T) (string, string, []string) { +func pulseAPIReferenceDocument(t *testing.T) string { t.Helper() doc, err := os.ReadFile(repoFile("docs", "API.md")) if err != nil { @@ -51,13 +104,65 @@ func pulseAPIReference(t *testing.T) (string, string, []string) { if err != nil || string(doc) != string(mirror) { t.Fatal("shipped API reference differs from the tested recipes") } - prep, helper, calls, err := pulseAPIRecipes(string(doc)) + return string(doc) +} + +func pulseAPIReference(t *testing.T) (string, string, []pulseAPIRecipe) { + t.Helper() + prep, helper, calls, err := pulseAPIRecipes(pulseAPIReferenceDocument(t)) if err != nil { t.Fatal(err) } return prep, helper, calls } +func TestPulseAPIGuidanceRecipeCatalogueIsCompleteAndOrderIndependent(t *testing.T) { + doc := pulseAPIReferenceDocument(t) + prep, helper, calls := pulseAPIReference(t) + fence := func(block string) string { return "```bash\n" + block + "```\n" } + reordered := fence(prep) + fence(helper) + for i := len(calls) - 1; i >= 0; i-- { + reordered += fence(calls[i].command) + } + cases := []struct{ name, doc, wantError string }{ + {"current", doc, ""}, + {"reordered", reordered, ""}, + {"missing preparation", strings.Replace(doc, prep, "", 1), "missing credential preparation"}, + {"missing helper", strings.Replace(doc, helper, "", 1), "missing credential preparation"}, + {"duplicate preparation", doc + fence(prep), "duplicate credential preparation"}, + {"duplicate helper", doc + fence(helper), "duplicate private-response helper"}, + {"unqualified request", doc + fence("pulse_api GET /api/unqualified\n"), "lacks a transport fixture"}, + {"hidden request", doc + fence("# Request\npulse_api GET /api/state/summary\n"), "must start with"}, + {"direct curl", doc + fence("curl --disable http://127.0.0.1:7655/api/state/summary\n"), "bypasses"}, + {"changed method", strings.Replace(doc, "pulse_api GET /api/state/summary\n", "pulse_api PUT /api/state/summary\n", 1), "lacks a transport fixture"}, + {"changed node input", strings.Replace(doc, `pulse_api POST /api/config/nodes < "$node_request_file"`, "pulse_api POST /api/config/nodes", 1), "lacks a transport fixture"}, + } + for _, call := range calls { + cases = append(cases, + struct{ name, doc, wantError string }{"missing " + call.contract.name, strings.Replace(doc, call.command, "", 1), "missing request example"}, + struct{ name, doc, wantError string }{"duplicate " + call.contract.name, doc + fence(call.command), "duplicate request example"}) + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, _, got, err := pulseAPIRecipes(tc.doc) + if tc.wantError != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantError) { + t.Fatalf("recipe catalogue error = %v, want %q", err, tc.wantError) + } + return + } + if err != nil || len(got) != len(pulseAPIRequestContracts()) { + t.Fatalf("complete catalogue rejected: %v", err) + } + for _, call := range got { + if !strings.HasPrefix(call.command, call.contract.firstLine+"\n") { + t.Fatal("request was bound to another example's transport contract") + } + } + }) + } +} + func runPulseAPIRecipe(t *testing.T, script, home, bin string, extraEnv ...string) (string, int) { t.Helper() result, err := observeGuidanceRecipe(t, script, home, bin, guidanceRecipeLimits(20*time.Second), extraEnv...) @@ -248,10 +353,9 @@ type apiRecipeRequest struct{ method, uri, token, contentType, body string } func TestPulseAPIGuidanceCopiedRequestsKeepResponsesPrivateAndOneShot(t *testing.T) { _, helper, calls := pulseAPIReference(t) - paths := []string{"/api/state/summary", "/api/connections", "/api/agent/resource-capabilities/vm%3A42", "/api/actions/plan", "/api/actions/act_.../decision", "/api/actions/act_.../execute", "/api/audit/actions?resourceId=vm%3A42&limit=10", "/api/audit/actions/act_.../events"} - for index, call := range calls { + for _, call := range calls { for _, scenario := range []string{"success", "401", "403", "redirect", "partial", "untrusted HTTPS", "trusted HTTPS"} { - t.Run(fmt.Sprintf("request-%d/%s", index, scenario), func(t *testing.T) { + t.Run(call.contract.name+"/"+scenario, func(t *testing.T) { home, bin := t.TempDir(), t.TempDir() dir := filepath.Join(home, ".config", "pulse") if err := os.MkdirAll(dir, 0755); err != nil { @@ -315,11 +419,40 @@ func TestPulseAPIGuidanceCopiedRequestsKeepResponsesPrivateAndOneShot(t *testing t.Cleanup(server.Close) recipe = strings.Replace(recipe, "http://127.0.0.1:7655", server.URL, 1) extra := []string{"API_ARGV=" + capture, "API_CURL=" + realCurl} + contentType, body := "", "" + var nodeInput string + switch call.contract.input { + case "heredoc": + contentType = "application/json" + _, body, _ = strings.Cut(call.command, "<<'JSON'\n") + body = strings.TrimSuffix(body, "JSON\n") + case "file": + // Fixed, non-credential body bytes test the private-file + // transport. The node guide's schema/trust specimen and + // preparation are independently covered by test_node_api_docs.py. + contentType, body = "application/json", "{\"fixture\":\"node-api-doc-fixture\"}\n" + file, err := os.CreateTemp(home, "node-request-*.json") + if err != nil { + t.Fatal(err) + } + info, err := file.Stat() + if err != nil || info.Mode().Perm() != 0600 { + _ = file.Close() + t.Fatal("node input must be private before writing") + } + _, writeErr := io.WriteString(file, body) + closeErr := file.Close() + if writeErr != nil || closeErr != nil { + t.Fatal("could not prepare the synthetic node input") + } + nodeInput = file.Name() + extra = append(extra, "node_request_file="+nodeInput) + } if !strings.Contains(scenario, "HTTPS") { // The local recipe must not send credentials to an ambient proxy. extra = append(extra, "http_proxy=http://127.0.0.1:1", "ALL_PROXY=http://127.0.0.1:1") } - output, exit := runPulseAPIRecipe(t, recipe+"\n"+call, home, bin, extra...) + output, exit := runPulseAPIRecipe(t, recipe+"\n"+call.command, home, bin, extra...) wantExit := map[string]int{"success": 0, "401": 22, "403": 22, "redirect": 1, "partial": 18, "untrusted HTTPS": 60, "trusted HTTPS": 0}[scenario] if exit != wantExit { t.Fatalf("request exit %d, want %d", exit, wantExit) @@ -328,7 +461,7 @@ func TestPulseAPIGuidanceCopiedRequestsKeepResponsesPrivateAndOneShot(t *testing if err != nil || !strings.HasPrefix(string(argv), "--disable\x00") { t.Fatal("curl did not ignore configuration first") } - for _, sensitive := range []string{token, "private-fixture-name", "private-fixture-event"} { + for _, sensitive := range []string{token, "private-fixture-name", "private-fixture-event", "node-api-doc-fixture"} { if strings.Contains(output, sensitive) || strings.Contains(string(argv), sensitive) { t.Fatal("request printed private credentials or response data") } @@ -349,6 +482,13 @@ func TestPulseAPIGuidanceCopiedRequestsKeepResponsesPrivateAndOneShot(t *testing if data, err := os.ReadFile(retained); err != nil || string(data) != "previous-result" { t.Fatal("request replaced an earlier response") } + if nodeInput != "" { + data, readErr := os.ReadFile(nodeInput) + info, statErr := os.Stat(nodeInput) + if readErr != nil || statErr != nil || string(data) != body || info.Mode().Perm() != 0600 { + t.Fatal("request changed its private node input") + } + } if scenario == "untrusted HTTPS" { if len(requests) != 0 { t.Fatal("untrusted TLS peer received credentials") @@ -358,14 +498,7 @@ func TestPulseAPIGuidanceCopiedRequestsKeepResponsesPrivateAndOneShot(t *testing t.Fatal("request was missing, redirected or retried") } got := <-requests - method := "GET" - contentType, body := "", "" - if index >= 3 && index <= 5 { - method, contentType = "POST", "application/json" - _, body, _ = strings.Cut(call, "<<'JSON'\n") - body = strings.TrimSuffix(body, "JSON\n") - } - if got != (apiRecipeRequest{method, paths[index], token, contentType, body}) { + if got != (apiRecipeRequest{call.contract.method, call.contract.uri, token, contentType, body}) { t.Fatal("request method, endpoint, private header or JSON body differs from the example") } if data, err := os.ReadFile(responses[0]); err != nil || string(data) != response { @@ -377,8 +510,49 @@ func TestPulseAPIGuidanceCopiedRequestsKeepResponsesPrivateAndOneShot(t *testing } } +func TestPulseAPIGuidanceMissingNodeInputStopsBeforeCurl(t *testing.T) { + _, helper, calls := pulseAPIReference(t) + home, bin := t.TempDir(), t.TempDir() + dir := filepath.Join(home, ".config", "pulse") + if err := os.MkdirAll(dir, 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "api-header"), []byte("X-API-Token: synthetic-header\n"), 0600); err != nil { + t.Fatal(err) + } + called := filepath.Join(home, "curl-called") + if err := os.WriteFile(filepath.Join(bin, "curl"), []byte("#!/bin/sh\nprintf started > \"$API_CALLED\"\n"), 0700); err != nil { + t.Fatal(err) + } + for _, call := range calls { + if call.contract.name != "add-node" { + continue + } + _, exit := runPulseAPIRecipe(t, helper+"\n"+call.command, home, bin, + "node_request_file="+filepath.Join(home, "absent.json"), "API_CALLED="+called) + if _, err := os.Stat(called); exit == 0 || !os.IsNotExist(err) { + t.Fatal("missing private node input reached curl or succeeded") + } + responses, err := filepath.Glob(filepath.Join(dir, "api-response.*")) + if err != nil || len(responses) != 0 { + t.Fatal("missing node input entered the request helper") + } + return + } + t.Fatal("node request recipe is missing") +} + func TestPulseAPIGuidanceTimedOutPOSTIsNotRepeated(t *testing.T) { _, helper, calls := pulseAPIReference(t) + var execute string + for _, call := range calls { + if call.contract.name == "execute" { + execute = call.command + } + } + if execute == "" { + t.Fatal("execution request recipe is missing") + } home, bin := t.TempDir(), t.TempDir() dir := filepath.Join(home, ".config", "pulse") if err := os.MkdirAll(dir, 0700); err != nil { @@ -406,7 +580,7 @@ func TestPulseAPIGuidanceTimedOutPOSTIsNotRepeated(t *testing.T) { // A deliberate preparation delay exceeds the old whole-shell comparison // once added to the actual twenty-second request. It must not shorten curl's // limit or be mistaken for request time. - result, err := observeGuidanceRecipe(t, "sleep 12\n"+helper+"\n"+calls[5], home, bin, guidanceRecipeLimits(20*time.Second)) + result, err := observeGuidanceRecipe(t, "sleep 12\n"+helper+"\n"+execute, home, bin, guidanceRecipeLimits(20*time.Second)) if err != nil { t.Fatalf("timed-out POST observation failed: %v; %s", err, result.summary()) } diff --git a/scripts/tests/test-e2e-fixture-admission.sh b/scripts/tests/test-e2e-fixture-admission.sh new file mode 100755 index 0000000000..1aaa237410 --- /dev/null +++ b/scripts/tests/test-e2e-fixture-admission.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -euo pipefail +root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +node --test "${root}/tests/integration/scripts/fixture-readiness.test.mjs" diff --git a/scripts/tests/test-e2e-runner-cleanup.sh b/scripts/tests/test-e2e-runner-cleanup.sh new file mode 100755 index 0000000000..2349b4d726 --- /dev/null +++ b/scripts/tests/test-e2e-runner-cleanup.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -euo pipefail +root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +node --test "${root}/tests/integration/scripts/run-playwright.test.mjs" diff --git a/scripts/tests/test_agent_auth_docs.py b/scripts/tests/test_agent_auth_docs.py new file mode 100644 index 0000000000..be05010683 --- /dev/null +++ b/scripts/tests/test_agent_auth_docs.py @@ -0,0 +1,87 @@ +#!/usr/bin/env python3 +"""Keep passive agent-authentication triage aligned with the real install flow. + +These source/document controls do not authenticate an installed agent or +establish a reporter's cause. No credential, service or target is accessed. +""" + +from pathlib import Path +import re +import unittest + + +ROOT = Path(__file__).resolve().parents[2] +DOC = ROOT / "docs/UNIFIED_AGENT.md" + + +def section(): + text = DOC.read_text(encoding="utf-8") + marker = "### Agent lookup returns HTTP 401 or 403\n" + if marker not in text: + raise AssertionError("missing agent authentication triage") + return re.split(r"\n#{2,3} ", text.split(marker, 1)[1], 1)[0] + + +class AgentAuthDocsTest(unittest.TestCase): + def test_status_explanations_match_authentication_before_scoped_lookup(self): + text = " ".join(section().split()) + routes = (ROOT / "internal/api/router_routes_registration.go").read_text() + self.assertIn( + '"/api/agents/agent/lookup", RequireAuth(r.config, ' + 'RequireScope(config.ScopeAgentReport, r.unifiedAgentHandlers.HandleLookup)', + routes, + ) + auth = (ROOT / "internal/api/auth.go").read_text() + self.assertIn('http.Error(w, "Invalid API token", http.StatusUnauthorized)', auth) + scope = (ROOT / "internal/api/apihttp/scope.go").read_text() + denial = scope.split("func RespondMissingScope(", 1)[1] + self.assertIn("w.WriteHeader(http.StatusForbidden)", denial) + self.assertIn('"error":', denial) + self.assertIn('"missing_scope"', denial) + for fact in ("authenticates the request **before**", "**HTTP 401** is an authentication rejection", + "**HTTP 403** with `missing_scope`", "`agent:report`", "**HTTP 404**", + "upstream authentication layer"): + self.assertIn(fact, text) + + def test_normal_install_is_not_conflated_with_bootstrap_enrolment(self): + text = " ".join(section().split()) + install = (ROOT / "frontend-modern/src/components/Settings/useInfrastructureInstallState.tsx").read_text() + args = install.split("const getInstallerExtraArgs = () => [", 1)[1].split("];", 1)[0] + self.assertNotIn("--enroll", args) + scopes = (ROOT / "internal/api/agenttokens/install.go").read_text() + host = scopes.split("func HostScopes(", 1)[1].split("\n}", 1)[0] + self.assertIn("config.ScopeAgentReport", host) + self.assertIn("config.ScopeAgentConfigRead", host) + self.assertNotIn("ScopeAgentEnroll", host) + prompt = (ROOT / "frontend-modern/src/utils/agentInstallCommand.ts").read_text() + self.assertIn('read -r -s -p', prompt) + self.assertIn('--token-file "$token_file"', prompt) + for fact in ("silent token prompt", "does not change reporting permissions", + "It does not use `--enroll`", "bootstrap token for a runtime token", + "Do not substitute a bootstrap token", "normal host installation"): + self.assertIn(fact, text) + + def test_triage_preserves_unknowns_state_and_private_credentials(self): + text = " ".join(section().split()) + host = (ROOT / "internal/hostagent/agent.go").read_text() + self.assertIn("Pulse rejected this agent's API token", host) + client = (ROOT / "internal/remoteconfig/client.go").read_text() + self.assertIn('Str("component", "remote_config_client")', client) + for fact in ("bounded agent log reader", "affected agent host", "retrieval and does not establish", + "ordinary host reports were accepted", "successful-looking installer output does not prove", + "No host-report error", "inconclusive", "Do not restart, re-enrol, delete identity", + "same Pulse instance", "do not prove that the running service is using that record", + "service environments, token files and connection files private", + "Do not grant administrator scope", "disable authentication/TLS", + "manually redacted error line", "saved credentials unchanged", + "not itself a reason to reinstall or rotate every token"): + self.assertIn(fact, text) + self.assertNotIn("```", section(), "Reuse the bounded reader; do not add live credential probes") + + def test_shipped_help_matches_the_canonical_guide(self): + self.assertEqual(DOC.read_bytes(), + (ROOT / "frontend-modern/public/docs/UNIFIED_AGENT.md").read_bytes()) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/tests/test_api_auth_docs.py b/scripts/tests/test_api_auth_docs.py index 93ace3d22f..c61f713045 100644 --- a/scripts/tests/test_api_auth_docs.py +++ b/scripts/tests/test_api_auth_docs.py @@ -203,14 +203,17 @@ class APIAuthDocsTest(unittest.TestCase): self.assertNotRegex(bash, r"(?:--insecure|--verbose|--trace\S*|--location|\s-k\b)") blocks = re.findall(r"```bash\n(.*?)```", DOC.read_text(), re.DOTALL) requests = [block for block in blocks if block.startswith("pulse_api ")] - self.assertEqual(len(requests), 8) + self.assertEqual(len(requests), 9) self.assertEqual([block for block in blocks if "curl " in block], [request_helper()], "an example must not bypass the private-response helper") self.assertIn("curl --disable --fail-with-body ", request_helper()) for request in requests: if request.startswith("pulse_api POST "): self.assertIn("--data-binary @-", request_helper()) - self.assertIn("<<'JSON'", request) + if request.startswith("pulse_api POST /api/config/nodes "): + self.assertEqual(request.strip(), 'pulse_api POST /api/config/nodes < "$node_request_file"') + else: + self.assertIn("<<'JSON'", request) def test_every_documented_shell_recipe_parses(self): for block in re.findall(r"```bash\n(.*?)```", DOC.read_text(), re.DOTALL): diff --git a/scripts/tests/test_e2e_test_images.py b/scripts/tests/test_e2e_test_images.py index 921157ae93..38f9d2bd55 100644 --- a/scripts/tests/test_e2e_test_images.py +++ b/scripts/tests/test_e2e_test_images.py @@ -255,6 +255,45 @@ class E2EImageBundleTest(unittest.TestCase): class E2EImageWorkflowTest(unittest.TestCase): + def assert_fixture_admission(self, workflow): + consumer = job_block(workflow, "e2e") + image_admission = consumer.index("- name: Admit the source-bound test images") + startup = consumer.index("- name: Start test containers") + fixture_admission = consumer.index("- name: Admit the default mock runtime") + stable = consumer.index("- name: Run stable-tier E2E suite") + probation = consumer.index("- name: Run probation-tier E2E suite") + self.assertLess(image_admission, startup) + self.assertLess(startup, fixture_admission) + self.assertLess(fixture_admission, stable) + end = consumer.index("\n - name:", fixture_admission) + admission = consumer[fixture_admission:end] + self.assertIn("npx playwright test --config=fixture-readiness.config.ts", admission) + self.assertIn('PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY: "true"', admission) + for ignored in ("continue-on-error", "if:", "||"): + self.assertNotIn(ignored, admission) + for receipt in ("Upload Playwright report", "Upload JUnit results"): + start = consumer.index(f"- name: {receipt}\n") + self.assertLess(stable, start) + self.assertLess(start, probation) + end = consumer.index("\n - name:", start) + self.assertIn("!cancelled() && steps.stable.outcome == 'failure'", consumer[start:end]) + + def test_fixture_admission_cannot_skip_source_images_or_lose_completed_receipts(self): + self.assert_fixture_admission(WORKFLOW.read_text()) + + def test_ignored_fixture_and_late_receipt_mutants_are_rejected(self): + source = WORKFLOW.read_text() + for old, new in ( + ("run: npx playwright test --config=fixture-readiness.config.ts", + "run: npx playwright test --config=fixture-readiness.config.ts || true"), + (" id: fixture\n", " id: fixture\n continue-on-error: true\n"), + ('PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY: "true"', 'PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY: "false"'), + ("!cancelled() && steps.stable.outcome == 'failure'", "failure()"), + ): + with self.subTest(mutation=new): + with self.assertRaises(AssertionError): + self.assert_fixture_admission(source.replace(old, new)) + def assert_workflow(self, workflow): producer = job_block(workflow, "test-images") self.assertIn("needs: tier-selection", producer) diff --git a/scripts/tests/test_icmp_privilege_docs.py b/scripts/tests/test_icmp_privilege_docs.py new file mode 100644 index 0000000000..f575f821f1 --- /dev/null +++ b/scripts/tests/test_icmp_privilege_docs.py @@ -0,0 +1,170 @@ +#!/usr/bin/env python3 +"""Exercise copied ICMP-help commands without systemd or privilege changes. + +Synthetic command recorders check disclosure, deadlines and failure sequencing. +They do not establish native ICMP recovery, capability grants or notification +delivery. The guide deliberately requires an operator's policy/state judgment. +""" + +import json +import os +from pathlib import Path +import re +import subprocess +import tempfile +import unittest + + +ROOT = Path(__file__).resolve().parents[2] +HEADING = "### ICMP probe privileges\n" +PROPERTIES = "LoadState,ActiveState,User,NoNewPrivileges,AmbientCapabilities,CapabilityBoundingSet" +PRIVATE = "synthetic-private-environment-value" + + +def section(): + text = (ROOT / "docs/CONFIGURATION.md").read_text(encoding="utf-8") + return text.split(HEADING, 1)[1].split("\n---\n", 1)[0] + + +def recipe(needle): + blocks = [b for b in re.findall(r"```bash\n(.*?)```", section(), re.S) + if needle in b] + if len(blocks) != 1: + raise AssertionError(f"expected one ICMP-help recipe for {needle!r}") + return blocks[0] + + +SYSTEMCTL = '''#!/usr/bin/env python3 +import json, os, sys, time +from pathlib import Path +args = sys.argv[1:] +root = Path(os.environ['RECORD_DIR']) +with (root / 'calls.jsonl').open('a') as f: + f.write(json.dumps(args) + '\\n') +mode = os.environ.get('READ_MODE', 'loaded') +if args[0] == 'show': + if mode == 'denied': + print('Read denied', file=sys.stderr) + sys.exit(1) + if mode == 'hang': + time.sleep(30) + requested = args[-1].removeprefix('--property=').split(',') + values = { + 'LoadState': 'not-found' if mode == 'missing' else 'loaded', + 'ActiveState': 'active', 'User': 'pulse', 'NoNewPrivileges': 'yes', + 'AmbientCapabilities': '', 'CapabilityBoundingSet': 'cap_net_raw', + } + if args[-1] == '--all' or not args[-1].startswith('--property='): + print('Environment=' + os.environ['PRIVATE_VALUE']) + for key in requested: + print(key + '=' + values.get(key, os.environ['PRIVATE_VALUE'])) +elif args[0] == 'restart': + sys.exit(int(os.environ.get('RESTART_EXIT', '0'))) +elif args[0] == 'is-active': + print('active') +else: + sys.exit(65) +''' + + +def execute(command, **overrides): + with tempfile.TemporaryDirectory(prefix="icmp docs ") as directory: + root = Path(directory) + tools = root / "tools" + tools.mkdir() + for name, body in (("systemctl", SYSTEMCTL), ("sudo", '#!/bin/sh\nexec "$@"\n')): + path = tools / name + path.write_text(body, encoding="utf-8") + path.chmod(0o700) + env = {**os.environ, "RECORD_DIR": directory, "PRIVATE_VALUE": PRIVATE, + "PATH": str(tools) + os.pathsep + os.environ["PATH"], **overrides} + result = subprocess.run(["bash", "-c", command], env=env, text=True, + capture_output=True, timeout=12) + record = root / "calls.jsonl" + calls = [json.loads(line) for line in record.read_text().splitlines()] if record.exists() else [] + return result, calls + + +class ICMPPrivilegeDocsTest(unittest.TestCase): + def test_install_and_privilege_claims_match_existing_source(self): + text = " ".join(section().split()) + installer = (ROOT / "install.sh").read_text() + for setting in ("AmbientCapabilities=CAP_NET_RAW", "CapabilityBoundingSet=CAP_NET_RAW", + "NoNewPrivileges=true"): + self.assertIn(setting, text) + self.assertIn(setting, installer) + self.assertIn("Updates keep an existing unit file untouched", installer) + for boundary in ("Updates preserve an existing service unit", "not a reliable unit repair", + "An ambient line alone cannot overcome the ceiling", + "Preserve that restriction", "Do not reset or widen", + "set file capabilities on the system-wide ping binary", + "do not run Pulse as root"): + self.assertIn(boundary, text) + self.assertNotIn("Docker installs are unaffected", text) + self.assertNotIn("it rewrites the unit", text) + + def test_location_state_and_recovery_are_not_inferred(self): + text = " ".join(section().split()) + for boundary in ("selected observation host", "not evidence that the target is down", + "cannot repair an agent's permissions", "inside the Pulse LXC", + "not pulse-agent.service", "failed or timed-out read is **unknown**", + "Keep deliberately inactive services inactive", "not the running process", + "monitoring and alerts are unavailable", "same location", + "not ICMP recovery or notification delivery", "not repeated restarts", + "do not use `systemctl revert`", "preserve other drop-in content", + "preserving data and configuration", "Do not use `--privileged`", + "--cap-add=ALL", "not proof that ICMP recovered"): + self.assertIn(boundary, text) + # The installer-independent probe executes on either server or agent. + probe = (ROOT / "internal/availabilityprobe/probe.go").read_text() + self.assertIn('exec.CommandContext(ctx, "ping", args...)', probe) + + def test_copied_read_is_bounded_allowlisted_and_read_only(self): + command = recipe("systemctl show") + self.assertIn("timeout --signal=TERM --kill-after=1s 8s", command) + self.assertIn("--property=" + PROPERTIES, command) + self.assertNotIn("sudo", command) + for mode in ("loaded", "missing"): + with self.subTest(mode=mode): + result, calls = execute(command, READ_MODE=mode) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(calls, [["show", "pulse.service", "--property=" + PROPERTIES]]) + self.assertNotIn(PRIVATE, result.stdout + result.stderr) + self.assertEqual(set(line.split("=", 1)[0] for line in result.stdout.splitlines()), + set(PROPERTIES.split(","))) + self.assertIn("LoadState=" + ("not-found" if mode == "missing" else "loaded"), + result.stdout) + + def test_failed_or_hung_read_has_no_mutation_or_retry(self): + for mode, code in (("denied", 1), ("hang", 124)): + with self.subTest(mode=mode): + result, calls = execute(recipe("systemctl show"), READ_MODE=mode) + self.assertEqual(result.returncode, code, result.stderr) + self.assertEqual(calls, [["show", "pulse.service", "--property=" + PROPERTIES]]) + self.assertNotIn(PRIVATE, result.stdout + result.stderr) + + def test_copied_restart_failure_stops_before_readback(self): + command = recipe("systemctl restart") + for code in (0, 5): + with self.subTest(restart_exit=code): + result, calls = execute(command, RESTART_EXIT=str(code)) + self.assertEqual(result.returncode, code, result.stderr) + expected = [["restart", "pulse.service"]] + if code == 0: + expected.append(["is-active", "pulse.service"]) + self.assertEqual(calls, expected) + self.assertNotIn(PRIVATE, result.stdout + result.stderr) + self.assertEqual(result.stdout, "active\n" if code == 0 else "") + + def test_copied_commands_parse_and_mirror_matches(self): + blocks = re.findall(r"```bash\n(.*?)```", section(), re.S) + self.assertEqual(len(blocks), 4) + for command in blocks: + subprocess.run(["bash", "-n", "-c", command], check=True, capture_output=True) + self.assertIn("INSTALL.md#2-bare-metal--systemd", section()) + self.assertEqual((ROOT / "docs/CONFIGURATION.md").read_bytes(), + (ROOT / "frontend-modern/public/docs/CONFIGURATION.md").read_bytes()) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/tests/test_node_api_docs.py b/scripts/tests/test_node_api_docs.py new file mode 100644 index 0000000000..3d31a139f0 --- /dev/null +++ b/scripts/tests/test_node_api_docs.py @@ -0,0 +1,192 @@ +#!/usr/bin/env python3 +"""Exercise the existing node API guide with synthetic private credentials. + +All HTTP is guest-local loopback, not a platform or production probe. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +import re +import stat +import subprocess +import tempfile +import unittest + +import test_api_auth_docs as auth + + +ROOT = Path(__file__).resolve().parents[2] +DOC = ROOT / "docs/API.md" +# A fixed, non-credential marker proves body transport and output redaction. +# Never substitute an account token, environment value or generated credential. +PLATFORM_FIXTURE_MARKER = "node-api-doc-fixture-7cd40730" + + +def section() -> str: + return DOC.read_text().split("## 🖥️ Nodes & Config\n", 1)[1].split("\n## ", 1)[0] + + +def preparation() -> str: + blocks = re.findall(r"```bash\n(.*?)```", section(), re.DOTALL) + return next(block for block in blocks if 'vi "$node_request_file"' in block) + + +def sample() -> dict: + add_node = section().split("### Add Node\n", 1)[1].split("\n### ", 1)[0] + blocks = re.findall(r"```json\n(.*?)```", add_node, re.DOTALL) + if len(blocks) != 1: + raise AssertionError("expected one node configuration specimen") + return json.loads(blocks[0]) + + +def request() -> str: + blocks = re.findall(r"```bash\n(.*?)```", section(), re.DOTALL) + return next(block for block in blocks if block.startswith("pulse_api POST ")) + + +class NodeAPIDocsTest(unittest.TestCase): + def test_specimen_uses_scoped_token_and_explicit_tls(self): + payload = sample() + self.assertEqual(payload["type"], "pve") + self.assertTrue(payload["host"].startswith("https://")) + self.assertNotIn("user", payload) + self.assertNotIn("password", payload) + self.assertNotIn("root@pam", json.dumps(payload)) + self.assertEqual(payload["tokenName"], "pulse-monitor@pve!pulse-readonly") + self.assertEqual(payload["tokenValue"], "") + self.assertIs(payload["verifySSL"], True) + schema = (ROOT / "internal/api/configapi/config_handlers.go").read_text() + schema = schema.split("type NodeConfigRequest struct {", 1)[1].split("\n}", 1)[0] + fields = set(re.findall(r'json:"([^",]+)', schema)) + self.assertLessEqual(set(payload), fields, "the copied fields must be accepted by this request type") + + def test_scopes_live_tests_and_connection_identity_are_explicit(self): + text = section() + routes = ("", "/test-connection", "/test-config", "/{id}", "/{id}/test", "/{id}/refresh-cluster") + for route in routes: + method = "PUT" if route == "/{id}" else "POST" + self.assertIn(f"`{method} /api/config/nodes{route}` (admin, `settings:write`)", text) + self.assertIn("`GET /api/config/nodes` (admin, `settings:read`)", text) + self.assertIn("`DELETE /api/config/nodes/{id}` (admin, `settings:write`)", text) + for boundary in ("not individual VMs or agents", "returned\nconnection `id`", + "no save", "live platform requests", "not a passive read", + "freeze/thaw", "not all\ncollection permissions", + "does not\nselect authorised nodes"): + self.assertIn(boundary, text) + self.assertNotIn("Validation Only", text) + + def test_credentials_trust_and_uncertain_changes_are_separate(self): + text = section() + for boundary in ("Pulse API token", "Proxmox token", "different credentials", + "does **not** enable certificate verification", + "independent trusted channel", "not established trust", + "Do not clear an existing pin", "can\nalso try to create a token and ACLs", + "without retries", "lost response", "re-read the\nsaved inventory", + "masked credentials", "not the platform workloads or the upstream token", + "Collection and related alert coverage can stop", + "reports and shared repositories"): + self.assertIn(boundary, text) + self.assertNotRegex("\n".join(re.findall(r"```bash\n(.*?)```", text, re.DOTALL)), + r"root@pam|tokenValue|password|--insecure|--location|--retry") + + def test_copied_preparation_is_private_unique_and_stops_on_failure(self): + with tempfile.TemporaryDirectory() as tmp: + home = Path(tmp) + tools = home / "tools" + tools.mkdir() + calls = home / "editor-calls.jsonl" + editor = tools / "vi" + editor.write_text( + "#!/usr/bin/env python3\nimport json, os, sys\nfrom pathlib import Path\n" + "with Path(os.environ['EDITOR_CALLS']).open('a') as f: f.write(json.dumps(sys.argv[1:]) + '\\n')\n" + "Path(sys.argv[1]).write_text('{\"fixture\": true}')\n" + ) + editor.chmod(0o700) + env = dict(os.environ, HOME=str(home), TMPDIR=str(home), + PATH=f"{tools}:{os.environ['PATH']}", EDITOR_CALLS=str(calls)) + created = [] + for _ in range(2): + result = subprocess.run(["bash", "-eu", "-c", preparation()], env=env, + capture_output=True, timeout=10) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, b"", "preparation must not print credentials") + argv = json.loads(calls.read_text().splitlines()[-1]) + self.assertEqual(len(argv), 1) + path = Path(argv[0]) + self.assertEqual(path.name, "node.json") + self.assertEqual(path.read_text(), '{"fixture": true}') + self.assertEqual(stat.S_IMODE(path.stat().st_mode), 0o600) + self.assertEqual(stat.S_IMODE(path.parent.stat().st_mode), 0o700) + created.append(path) + self.assertNotEqual(*created, "a new preparation must not overwrite the previous request") + before = calls.read_bytes() + failed = subprocess.run(["bash", "-eu", "-c", preparation()], + env=dict(env, TMPDIR=str(home / "missing")), + capture_output=True, timeout=10) + self.assertNotEqual(failed.returncode, 0) + self.assertEqual(calls.read_bytes(), before, "failed preparation must not open an editor") + self.assertEqual(created[0].read_text(), '{"fixture": true}') + + def test_copied_file_dispatch_is_private_bounded_and_never_retried(self): + payload = sample() + payload["tokenValue"] = PLATFORM_FIXTURE_MARKER + # Independent operation expectation: the fixture must not accept a + # different route, method, credential or body just because it was copied. + self.assertEqual(request().strip(), 'pulse_api POST /api/config/nodes < "$node_request_file"') + for status, partial in ((201, False), (401, False), (403, False), + (302, False), (500, False), (200, True)): + with self.subTest(status=status, partial=partial), tempfile.TemporaryDirectory() as tmp: + home = Path(tmp) + # mkstemp creates a unique mode-0600 file before any body is + # written, independent of the process umask. Do not repair + # initially public permissions after writing the fixture. + fd, name = tempfile.mkstemp(prefix="node-", suffix=".json", dir=home) + body = Path(name) + with os.fdopen(fd, "w", encoding="utf-8") as stream: + self.assertEqual(stat.S_IMODE(os.fstat(stream.fileno()).st_mode), 0o600) + stream.write(json.dumps(payload)) + script = f'node_request_file={json.dumps(str(body))}\n' + auth.request_helper() + "\n" + request() + with auth.private_recording_server(status, partial) as (port, calls): + result = auth.exercise_curl(self, home, "X-API-Token: " + auth.TEST_TOKEN, + port, script, private_response=True) + self.assertEqual(len(calls), 1) + path, headers, method, data = calls[0] + self.assertEqual((path, method), ("/api/config/nodes", "POST")) + self.assertEqual(headers["X-API-Token"], auth.TEST_TOKEN) + self.assertEqual(headers["Content-Type"], "application/json") + self.assertEqual(json.loads(data), payload) + self.assertEqual(body.read_text(), json.dumps(payload)) + argv = (home / "argv.json").read_text() + self.assertNotIn(PLATFORM_FIXTURE_MARKER, argv) + self.assertNotIn(PLATFORM_FIXTURE_MARKER.encode(), result.stdout + result.stderr) + self.assertEqual(result.returncode == 0, status == 201 and not partial) + + def test_missing_request_file_sends_nothing(self): + with tempfile.TemporaryDirectory() as tmp: + home = Path(tmp) + with auth.recording_server() as (port, calls): + script = f'node_request_file={json.dumps(str(home / "absent.json"))}\n' + script += auth.request_helper().replace("http://127.0.0.1:7655", f"http://127.0.0.1:{port}") + script += "\n" + request() + result = subprocess.run(["bash", "-eu", "-c", script], capture_output=True, timeout=10) + self.assertNotEqual(result.returncode, 0) + self.assertEqual(calls, [], "missing private input must not fall back to another request body") + + def test_all_documented_node_shell_parses(self): + for block in re.findall(r"```bash\n(.*?)```", section(), re.DOTALL): + subprocess.run(["bash", "-n", "-c", block], check=True, capture_output=True) + + def test_private_response_helper_is_preserved(self): + helper = auth.request_helper() + self.assertIn('curl --disable --fail-with-body --silent --show-error', helper) + self.assertIn('--header "@$auth_file"', helper) + self.assertIn('--data-binary @-', helper) + self.assertIn('--output "$result_file"', helper) + self.assertNotIn('--retry', helper) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/integration/fixture-readiness.config.ts b/tests/integration/fixture-readiness.config.ts new file mode 100644 index 0000000000..769234ac73 --- /dev/null +++ b/tests/integration/fixture-readiness.config.ts @@ -0,0 +1,27 @@ +import { defineConfig, devices } from '@playwright/test'; +import { preferredBrowserBaseURL } from './tests/runtime-defaults'; + +// This is admission to the existing suite, not another tier or a replacement +// for its browser assertions. Use the same authenticated readiness helper. +export default defineConfig({ + testDir: './fixture-readiness', + testMatch: 'default-mock-ready.spec.ts', + outputDir: 'test-results-readiness', + workers: 1, + retries: 0, + forbidOnly: !!process.env.CI, + timeout: 360_000, + globalTimeout: 420_000, + reporter: [ + ['list'], + ['html', { outputFolder: 'playwright-report-readiness', open: 'never' }], + ['junit', { outputFile: 'test-results-readiness/junit.xml' }], + ], + use: { + ...devices['Desktop Chrome'], + baseURL: preferredBrowserBaseURL(), + ignoreHTTPSErrors: ['1', 'true', 'yes', 'on'].includes( + String(process.env.PULSE_E2E_INSECURE_TLS || '').trim().toLowerCase(), + ), + }, +}); diff --git a/tests/integration/fixture-readiness/default-mock-ready.spec.ts b/tests/integration/fixture-readiness/default-mock-ready.spec.ts new file mode 100644 index 0000000000..97361971ea --- /dev/null +++ b/tests/integration/fixture-readiness/default-mock-ready.spec.ts @@ -0,0 +1,23 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from '@playwright/test'; +import { createAuthenticatedStorageState } from '../tests/helpers'; + +test('admit the authenticated default mock inventory and seven-day history', async ({ browser }) => { + if (!['1', 'true', 'yes', 'on'].includes( + String(process.env.PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY || '').trim().toLowerCase(), + )) { + throw new Error('default mock admission requires PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY=true'); + } + + const directory = fs.mkdtempSync(path.join(os.tmpdir(), 'pulse-e2e-readiness-')); + try { + // The existing helper performs first-run authentication and the complete + // inventory/pool/disk-history checks. It also retains the usual shared + // cookie session for subsequent fixtures; do not add another login path. + await createAuthenticatedStorageState(browser, path.join(directory, 'session.json')); + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } +}); diff --git a/tests/integration/scripts/default-mock-readiness.d.mts b/tests/integration/scripts/default-mock-readiness.d.mts new file mode 100644 index 0000000000..cb437cf08b --- /dev/null +++ b/tests/integration/scripts/default-mock-readiness.d.mts @@ -0,0 +1,2 @@ +export function defaultMockInventoryReady(state: unknown): boolean; +export function defaultMockHistoryReady(charts: unknown): boolean; diff --git a/tests/integration/scripts/default-mock-readiness.mjs b/tests/integration/scripts/default-mock-readiness.mjs new file mode 100644 index 0000000000..e71c726664 --- /dev/null +++ b/tests/integration/scripts/default-mock-readiness.mjs @@ -0,0 +1,32 @@ +// The shared fixture's existing admission requirements. Keep browser setup +// and the early CI probe on one policy; readiness is not application acceptance. +const hasSource = (resource, source) => + Array.isArray(resource?.sources) && resource.sources.includes(source); + +export function defaultMockInventoryReady(state) { + const resources = Array.isArray(state?.resources) ? state.resources : []; + const infrastructure = Array.isArray(state?.connectedInfrastructure) + ? state.connectedInfrastructure : []; + return resources.some(r => r?.name === 'nvme-primary' && r.type === 'storage' && hasSource(r, 'vmware')) && + resources.some(r => r?.type === 'k8s-cluster' && hasSource(r, 'kubernetes')) && + resources.some(r => r?.name === 'tank' && hasSource(r, 'truenas')) && + resources.some(r => ['vm', 'system-container'].includes(r?.type) && hasSource(r, 'proxmox')) && + resources.some(r => r?.name === 'esxi-01.lab.local' && r.type === 'agent' && hasSource(r, 'vmware')) && + resources.some(r => r?.type === 'docker-host') && + resources.some(r => r?.type === 'pbs') && + resources.some(r => r?.type === 'pmg') && + infrastructure.some(entry => entry?.name === 'esxi-01.lab.local'); +} + +function hasDeepSeries(points) { + if (!Array.isArray(points)) return false; + const timestamps = points.map(point => Number(point?.timestamp)) + .filter(Number.isFinite).sort((left, right) => left - right); + return timestamps.length >= 2 && + timestamps[timestamps.length - 1] - timestamps[0] > 5 * 24 * 60 * 60 * 1000; +} + +export function defaultMockHistoryReady(charts) { + return Object.values(charts?.pools ?? {}).some(pool => hasDeepSeries(pool?.used)) && + Object.values(charts?.disks ?? {}).some(disk => hasDeepSeries(disk?.temperature)); +} diff --git a/tests/integration/scripts/fixture-readiness.test.mjs b/tests/integration/scripts/fixture-readiness.test.mjs new file mode 100644 index 0000000000..61a4830878 --- /dev/null +++ b/tests/integration/scripts/fixture-readiness.test.mjs @@ -0,0 +1,197 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { runInNewContext } from 'node:vm'; +import test from 'node:test'; +import { defaultMockInventoryReady, defaultMockHistoryReady } from './default-mock-readiness.mjs'; + +const integrationRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const root = path.resolve(integrationRoot, '..', '..'); +const source = file => fs.readFileSync(path.join(integrationRoot, file), 'utf8'); +const workflow = fs.readFileSync(process.env.PULSE_READINESS_TEST_WORKFLOW || + path.join(root, '.github/workflows/test-e2e.yml'), 'utf8'); +const steps = workflow.split('\n e2e:\n')[1].split('\n agent-registration:\n')[0]; +const step = name => steps.split(` - name: ${name}\n`)[1]?.split('\n - name:')[0]; + +const readyInventory = () => ({ + connectedInfrastructure: [{ name: 'esxi-01.lab.local' }], + resources: [ + { name: 'nvme-primary', type: 'storage', sources: ['vmware'] }, + { type: 'k8s-cluster', sources: ['kubernetes'] }, + { name: 'tank', sources: ['truenas'] }, + { type: 'vm', sources: ['proxmox'] }, + { name: 'esxi-01.lab.local', type: 'agent', sources: ['vmware'] }, + { type: 'docker-host' }, { type: 'pbs' }, { type: 'pmg' }, + ], +}); +const series = [{ timestamp: 1 }, { timestamp: 7 * 24 * 60 * 60 * 1000 }]; +const readyHistory = () => ({ pools: { tank: { used: series } }, disks: { sdc: { temperature: series } } }); + +test('complete existing inventory passes, including the container alternative', () => { + const state = readyInventory(); + assert.equal(defaultMockInventoryReady(state), true); + state.resources[3].type = 'system-container'; + assert.equal(defaultMockInventoryReady(state), true); +}); + +test('each required resource and connected-host identity is necessary', () => { + for (let i = 0; i < readyInventory().resources.length; i++) { + const state = readyInventory(); + state.resources.splice(i, 1); + assert.equal(defaultMockInventoryReady(state), false, `missing resource ${i}`); + } + const state = readyInventory(); + state.connectedInfrastructure = [{ name: 'another-host' }]; + assert.equal(defaultMockInventoryReady(state), false); +}); + +test('platform attribution and fixed names cannot borrow another source', () => { + for (let i = 0; i < 5; i++) { + const state = readyInventory(); + state.resources[i].sources = ['unrelated']; + assert.equal(defaultMockInventoryReady(state), false); + } + for (const i of [0, 2, 4]) { + const state = readyInventory(); + state.resources[i].name = 'unrelated'; + assert.equal(defaultMockInventoryReady(state), false); + } +}); + +test('unknown or malformed inventory cannot pass admission', () => { + for (const state of [null, undefined, {}, { resources: {} }, { resources: [null] }]) { + assert.equal(defaultMockInventoryReady(state), false); + } + const state = readyInventory(); + state.resources[0].sources = 'vmware'; + assert.equal(defaultMockInventoryReady(state), false); +}); + +test('both pool and physical-disk history are required at the original depth', () => { + assert.equal(defaultMockHistoryReady(readyHistory()), true); + for (const charts of [null, undefined, {}, { pools: readyHistory().pools }, { disks: readyHistory().disks }]) { + assert.equal(defaultMockHistoryReady(charts), false); + } + for (const points of [[], [{ timestamp: 1 }], [{ timestamp: 'bad' }, { timestamp: 700_000_000 }], + [{ timestamp: 0 }, { timestamp: 5 * 24 * 60 * 60 * 1000 }]]) { + for (const kind of ['pool', 'disk']) { + const charts = readyHistory(); + if (kind === 'pool') charts.pools.tank.used = points; + else charts.disks.sdc.temperature = points; + assert.equal(defaultMockHistoryReady(charts), false, `${kind}: ${JSON.stringify(points)}`); + } + } + const charts = readyHistory(); + charts.pools.tank.used = [...series].reverse(); + assert.equal(defaultMockHistoryReady(charts), true); +}); + +async function invokeAdmission(flag, check) { + let invocation; + // Execute the committed spec body with controlled browser/check callbacks, + // not a copy of its orchestration. Real Playwright discovery is also checked + // by the owning exact-source command. + const body = source('fixture-readiness/default-mock-ready.spec.ts').replace(/^import .*;\n/gm, ''); + runInNewContext(body, { + fs, os, path, Error, + process: { env: { PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY: flag } }, + test: (_name, callback) => { invocation = callback({ browser: 'owned-browser' }); }, + createAuthenticatedStorageState: check, + }); + await invocation; +} + +test('the real admission spec calls shared authentication once and clears temporary state', async () => { + let calls = 0; + let filename; + await invokeAdmission('true', async (browser, statePath) => { + calls++; + filename = statePath; + assert.equal(browser, 'owned-browser'); + fs.writeFileSync(statePath, 'controlled session'); + }); + assert.equal(calls, 1); + assert.equal(fs.existsSync(path.dirname(filename)), false); +}); + +test('original inventory/history failure escapes unchanged and clears temporary state', async () => { + for (const message of ['default mock inventory should be complete before browser fixtures start', + 'default mock history should cover the seven-day Core E2E chart window before browser fixtures start']) { + let calls = 0; + let filename; + const failure = new Error(message); + await assert.rejects(invokeAdmission('true', async (_browser, statePath) => { + calls++; + filename = statePath; + fs.writeFileSync(statePath, 'partial controlled session'); + throw failure; + }), err => err === failure); + assert.equal(calls, 1); + assert.equal(fs.existsSync(path.dirname(filename)), false); + } +}); + +test('missing readiness opt-in fails instead of admitting unchecked fixtures', async () => { + for (const flag of [undefined, '', 'false']) { + let called = false; + await assert.rejects(invokeAdmission(flag, async () => { called = true; }), + /requires PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY=true/); + assert.equal(called, false); + } +}); + +test('dedicated discovery cannot retry, absorb or mix admission with either tier', () => { + const config = source('fixture-readiness.config.ts'); + for (const required of ["testDir: './fixture-readiness'", "testMatch: 'default-mock-ready.spec.ts'", + 'workers: 1', 'retries: 0', 'timeout: 360_000', 'globalTimeout: 420_000', + 'test-results-readiness/junit.xml', 'playwright-report-readiness']) { + assert.ok(config.includes(required), required); + } + assert.ok(!config.includes('PULSE_E2E_TIER')); + const helper = source('tests/helpers.ts'); + assert.match(helper, /defaultMockInventoryReady\(await response\.json\(\)\)/); + assert.match(helper, /defaultMockHistoryReady\(await response\.json\(\)\)/); + assert.match(helper, /timeout: 120_000/); + assert.match(helper, /timeout: 180_000/); +}); + +test('workflow admission precedes both tiers without an ignored failure', () => { + const admission = step('Admit the default mock runtime'); + assert.ok(admission, 'missing fixture admission'); + assert.match(admission, /PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY: "true"/); + assert.match(admission, /timeout-minutes: 8/); + assert.match(admission, /working-directory: tests\/integration/); + assert.doesNotMatch(admission, /continue-on-error|\n if:|\|\|/); + assert.ok(steps.indexOf('- name: Admit the default mock runtime') < steps.indexOf('- name: Run stable-tier E2E suite')); +}); + +test('actual workflow shell admits healthy fixtures and stops broken ones before suite work', () => { + const command = step('Admit the default mock runtime')?.match(/\n run: (.+)/)?.[1] || ''; + const directory = fs.mkdtempSync(path.join(os.tmpdir(), 'pulse-admission-command-')); + try { + fs.writeFileSync(path.join(directory, 'npx'), '#!/bin/sh\n[ "$*" = "playwright test --config=fixture-readiness.config.ts" ] || exit 9\n' + + 'echo "controlled default mock readiness result"\nexit "$ADMISSION_EXIT"\n', { mode: 0o700 }); + for (const status of [0, 1]) { + const marker = path.join(directory, `suite-${status}`); + const result = spawnSync('bash', ['--noprofile', '--norc', '-e', '-o', 'pipefail', '-c', + `${command}\nprintf suite-started > "${marker}"`], { + cwd: integrationRoot, encoding: 'utf8', env: { ...process.env, PATH: `${directory}:${process.env.PATH}`, ADMISSION_EXIT: String(status) }, + }); + assert.equal(result.status, status, result.stdout + result.stderr); + assert.equal(fs.existsSync(marker), status === 0); + } + } finally { fs.rmSync(directory, { recursive: true, force: true }); } +}); + +test('completed stable failure receipts are uploaded before probation can be cancelled', () => { + for (const name of ['Upload Playwright report', 'Upload JUnit results']) { + assert.ok(steps.indexOf(`- name: ${name}`) < steps.indexOf('- name: Run probation-tier E2E suite'), name); + assert.match(step(name), /!cancelled\(\) && steps\.stable\.outcome == 'failure'/); + } + assert.match(step('Upload failed fixture admission'), /!cancelled\(\) && steps\.fixture\.outcome == 'failure'/); + assert.match(step('Run probation-tier E2E suite (non-gating)'), /continue-on-error: true/); + assert.ok(steps.includes('timeout-minutes: 60')); +}); diff --git a/tests/integration/scripts/managed-local-backend.test.mjs b/tests/integration/scripts/managed-local-backend.test.mjs index 14eb00e25b..7a02adbb2f 100644 --- a/tests/integration/scripts/managed-local-backend.test.mjs +++ b/tests/integration/scripts/managed-local-backend.test.mjs @@ -200,8 +200,9 @@ test('shared browser readiness waits for the bounded mock history contract', asy assert.match(compose, /PULSE_MOCK_TRENDS_SEED_DURATION=\$\{PULSE_MOCK_TRENDS_SEED_DURATION:-168h\}/); assert.match(helpers, /"\/api\/storage-charts\?range=10080"/); - assert.match(helpers, /hasDeepSeries\(pool\.used\)/); - assert.match(helpers, /hasDeepSeries\(disk\.temperature\)/); + assert.match(helpers, /defaultMockInventoryReady\(await response\.json\(\)\)/); + assert.match(helpers, /defaultMockHistoryReady\(await response\.json\(\)\)/); + assert.match(helpers, /default-mock-readiness\.mjs/); assert.match(helpers, /default mock history should cover the seven-day Core E2E chart window/); }); diff --git a/tests/integration/scripts/run-playwright.mjs b/tests/integration/scripts/run-playwright.mjs index d53a57b7e0..b4a9c7c706 100644 --- a/tests/integration/scripts/run-playwright.mjs +++ b/tests/integration/scripts/run-playwright.mjs @@ -46,17 +46,23 @@ async function writeManagedVerifyLock(env = process.env) { } const lockPath = managedVerifyLockPath(env); + const contents = `pid=${process.pid}\ncreated_at=${new Date().toISOString()}\nrun_id=${String(env.PULSE_E2E_RUN_ID || '').trim()}\n`; await fs.mkdir(path.dirname(lockPath), { recursive: true }); - await fs.writeFile( - lockPath, - `pid=${process.pid}\ncreated_at=${new Date().toISOString()}\nrun_id=${String(env.PULSE_E2E_RUN_ID || '').trim()}\n`, - 'utf8', - ); + await fs.writeFile(lockPath, contents, 'utf8'); + return { lockPath, contents }; } -async function clearManagedVerifyLock(env = process.env) { - const lockPath = managedVerifyLockPath(env); - await fs.rm(lockPath, { force: true }); +async function clearManagedVerifyLock(lock) { + if (!lock) return; + try { + // Non-hot-dev runs never acquired this handoff. A later owner may also + // have replaced it while teardown ran; do not remove that owner's lock. + if (await fs.readFile(lock.lockPath, 'utf8') === lock.contents) { + await fs.unlink(lock.lockPath); + } + } catch (err) { + if (err.code !== 'ENOENT') throw err; + } } const run = (command, args, options = {}) => @@ -68,21 +74,42 @@ const run = (command, args, options = {}) => let exitCode = 0; const childEnv = buildRunScopedEnv(); +let verifyLock; +let setupStarted = false; + +function reportFailure(phase, err) { + console.error(`[integration] ${phase} failed:`, err?.message || err); + if (exitCode === 0) exitCode = 1; +} try { - await writeManagedVerifyLock(childEnv); - const pretestCode = await run(nodeCmd, ['./scripts/pretest.mjs'], { env: childEnv }); - if (pretestCode !== 0) { - process.exit(pretestCode); + verifyLock = await writeManagedVerifyLock(childEnv); + setupStarted = true; + exitCode = await run(nodeCmd, ['./scripts/pretest.mjs'], { env: childEnv }); + // A failed setup may already have started a managed runtime. Keep its + // failure, skip Playwright, and still reach the existing teardown below. + if (exitCode === 0) { + exitCode = await run(npxCmd, ['playwright', 'test', ...playwrightArgs], { env: childEnv }); } - - exitCode = await run(npxCmd, ['playwright', 'test', ...playwrightArgs], { env: childEnv }); +} catch (err) { + reportFailure('setup or test launch', err); } finally { - const posttestCode = await run(nodeCmd, ['./scripts/posttest.mjs'], { env: childEnv }); - if (exitCode === 0 && posttestCode !== 0) { - exitCode = posttestCode; + try { + // If lock preparation failed, this invocation never started a runtime + // and must not stop a pre-existing one through posttest. + if (setupStarted) { + const posttestCode = await run(nodeCmd, ['./scripts/posttest.mjs'], { env: childEnv }); + if (exitCode === 0) exitCode = posttestCode; + } + } catch (err) { + reportFailure('teardown launch', err); + } finally { + try { + await clearManagedVerifyLock(verifyLock); + } catch (err) { + reportFailure('verification lock cleanup', err); + } } - await clearManagedVerifyLock(childEnv); } process.exit(exitCode); diff --git a/tests/integration/scripts/run-playwright.test.mjs b/tests/integration/scripts/run-playwright.test.mjs new file mode 100644 index 0000000000..b84a1d7a8e --- /dev/null +++ b/tests/integration/scripts/run-playwright.test.mjs @@ -0,0 +1,156 @@ +import assert from 'node:assert/strict'; +import { execFile } from 'node:child_process'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { promisify } from 'node:util'; +import test from 'node:test'; + +const execute = promisify(execFile); +// An exact-parent control can select its saved source without changing the +// production runner or replacing the assertions with source-string checks. +const runnerSource = await fs.readFile( + process.env.PULSE_RUNNER_TEST_SOURCE || new URL('./run-playwright.mjs', import.meta.url), + 'utf8', +); +const phaseScript = ` +import fs from 'node:fs'; +import path from 'node:path'; +const root = process.env.FIXTURE_ROOT; +const phase = process.env.FIXTURE_PHASE; +fs.appendFileSync(path.join(root, 'calls.jsonl'), JSON.stringify({ phase, + runId: process.env.PULSE_E2E_RUN_ID, + state: process.env.PULSE_E2E_RUNTIME_STATE_PATH, + lockPath: process.env.HOT_DEV_VERIFY_LOCK_FILE, + lock: fs.existsSync(process.env.HOT_DEV_VERIFY_LOCK_FILE) + ? (fs.statSync(process.env.HOT_DEV_VERIFY_LOCK_FILE).isFile() + ? fs.readFileSync(process.env.HOT_DEV_VERIFY_LOCK_FILE, 'utf8') : 'not-a-file') : null, + args: process.argv.slice(2), +}) + '\\n'); +`; + +async function fixture(t, options = {}) { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'pulse-npm-cleanup-')); + t.after(() => fs.rm(root, { recursive: true, force: true })); + await fs.mkdir(path.join(root, 'scripts')); + await fs.mkdir(path.join(root, 'bin')); + await fs.writeFile(path.join(root, 'scripts/run-playwright.mjs'), runnerSource); + await fs.writeFile(path.join(root, 'scripts/pretest.mjs'), ` +process.env.FIXTURE_PHASE = 'pretest'; +${phaseScript} +fs.writeFileSync(path.join(root, 'partial-runtime'), 'started'); +process.exit(${options.pretest ?? 0}); +`); + await fs.writeFile(path.join(root, 'scripts/posttest.mjs'), ` +process.env.FIXTURE_PHASE = 'posttest'; +${phaseScript} +fs.rmSync(path.join(root, 'partial-runtime'), { force: true }); +${options.replaceLock ? "fs.writeFileSync(process.env.HOT_DEV_VERIFY_LOCK_FILE, 'later-owner\\n');" : ''} +process.exit(${options.posttest ?? 0}); +`); + if (!options.missingNpx) { + await fs.writeFile(path.join(root, 'bin/npx'), `#!${process.execPath} +process.env.FIXTURE_PHASE = 'playwright'; +${phaseScript} +process.exit(${options.playwright ?? 0}); +`, { mode: 0o755 }); + } + const lockPath = path.join(root, 'verify.lock'); + if (options.foreignLock) await fs.writeFile(lockPath, options.foreignLock); + if (options.lockDirectory) await fs.mkdir(lockPath); + const env = { + ...process.env, + PATH: path.join(root, 'bin'), + FIXTURE_ROOT: root, + PULSE_E2E_USE_HOT_DEV: options.hotDev === false ? 'false' : 'true', + PULSE_E2E_RUN_ID: 'fixture-invocation', + PULSE_E2E_RUNTIME_STATE_PATH: path.join(root, 'runtime-state.json'), + HOT_DEV_VERIFY_LOCK_FILE: lockPath, + }; + let result; + try { + const output = await execute(process.execPath, + ['./scripts/run-playwright.mjs', '--project=chromium'], + { cwd: root, env, timeout: 5000 }); + result = { code: 0, ...output }; + } catch (err) { + // A killed/timed-out fixture is an error, never the expected nonzero exit. + assert.equal(err.killed, false, err.message); + assert.equal(err.signal, null, err.message); + assert.equal(typeof err.code, 'number', err.message); + result = { code: err.code, stdout: err.stdout, stderr: err.stderr }; + } + const calls = await fs.readFile(path.join(root, 'calls.jsonl'), 'utf8') + .then(raw => raw.trim().split('\n').map(line => JSON.parse(line))) + .catch(err => { if (err.code === 'ENOENT') return []; throw err; }); + for (const call of calls) { + assert.equal(call.runId, env.PULSE_E2E_RUN_ID); + assert.equal(call.state, env.PULSE_E2E_RUNTIME_STATE_PATH); + assert.equal(call.lockPath, env.HOT_DEV_VERIFY_LOCK_FILE); + } + return { root, lockPath, calls, result }; +} + +async function assertAbsent(file) { + await assert.rejects(fs.stat(file), { code: 'ENOENT' }); +} + +test('failed setup tears down its partial runtime and lock without launching tests', async t => { + const f = await fixture(t, { pretest: 17 }); + assert.equal(f.result.code, 17); + assert.deepEqual(f.calls.map(c => c.phase), ['pretest', 'posttest']); + assert.match(f.calls[0].lock, /\nrun_id=fixture-invocation\n$/); + assert.equal(f.calls[1].lock, f.calls[0].lock, 'handoff stays held through teardown'); + await assertAbsent(path.join(f.root, 'partial-runtime')); + await assertAbsent(f.lockPath); +}); + +test('failed setup remains failed when teardown also fails', async t => { + const f = await fixture(t, { pretest: 17, posttest: 23 }); + assert.equal(f.result.code, 17); + assert.deepEqual(f.calls.map(c => c.phase), ['pretest', 'posttest']); + await assertAbsent(f.lockPath); +}); + +for (const [playwright, posttest, expected] of [[0, 0, 0], [19, 0, 19], [0, 23, 23], [19, 23, 19]]) { + test(`test exit ${playwright}, teardown exit ${posttest} retain exit ${expected} and cleanup`, async t => { + const f = await fixture(t, { playwright, posttest }); + assert.equal(f.result.code, expected); + assert.deepEqual(f.calls.map(c => c.phase), ['pretest', 'playwright', 'posttest']); + assert.deepEqual(f.calls[1].args, ['playwright', 'test', '--project=chromium']); + for (const call of f.calls) assert.equal(call.lock, f.calls[0].lock); + await assertAbsent(path.join(f.root, 'partial-runtime')); + await assertAbsent(f.lockPath); + }); +} + +test('non-hot-dev invocation preserves a verification lock it never acquired', async t => { + const foreignLock = `pid=${process.pid}\nrun_id=another-live-invocation\n`; + const f = await fixture(t, { hotDev: false, foreignLock }); + assert.equal(f.result.code, 0); + assert.deepEqual(f.calls.map(c => c.phase), ['pretest', 'playwright', 'posttest']); + assert.equal(await fs.readFile(f.lockPath, 'utf8'), foreignLock); +}); + +test('teardown preserves a handoff replaced by a later owner', async t => { + const f = await fixture(t, { replaceLock: true }); + assert.equal(f.result.code, 0); + assert.deepEqual(f.calls.map(c => c.phase), ['pretest', 'playwright', 'posttest']); + assert.equal(await fs.readFile(f.lockPath, 'utf8'), 'later-owner\n'); +}); + +test('failed test launch still tears down setup and the acquired lock', async t => { + const f = await fixture(t, { missingNpx: true }); + assert.equal(f.result.code, 1); + assert.match(f.result.stderr, /ENOENT/); + assert.deepEqual(f.calls.map(c => c.phase), ['pretest', 'posttest']); + await assertAbsent(path.join(f.root, 'partial-runtime')); + await assertAbsent(f.lockPath); +}); + +test('failed lock preparation cannot stop a runtime this invocation never started', async t => { + const f = await fixture(t, { lockDirectory: true }); + assert.equal(f.result.code, 1); + assert.deepEqual(f.calls, []); + assert.equal((await fs.stat(f.lockPath)).isDirectory(), true); +}); diff --git a/tests/integration/tests/helpers.ts b/tests/integration/tests/helpers.ts index 8a0a04f786..a830f5ebd6 100644 --- a/tests/integration/tests/helpers.ts +++ b/tests/integration/tests/helpers.ts @@ -20,6 +20,11 @@ import { runtimeStatePath, } from "./runtime-defaults"; +import { + defaultMockInventoryReady, + defaultMockHistoryReady, +} from "../scripts/default-mock-readiness.mjs"; + const helpersDir = path.dirname(fileURLToPath(import.meta.url)); const runtimePrimaryAPIToken = (): string => { @@ -1140,26 +1145,6 @@ async function storageStateHasLiveSession( } } -type E2ERuntimeStateResource = { - name?: string; - sources?: string[]; - type?: string; -}; - -type E2ERuntimeState = { - connectedInfrastructure?: Array<{ name?: string }>; - resources?: E2ERuntimeStateResource[]; -}; - -type E2EMetricPoint = { - timestamp?: number; -}; - -type E2EStorageCharts = { - pools?: Record; - disks?: Record; -}; - const requiresDefaultMockRuntimeReadiness = (): boolean => ["1", "true", "yes", "on"].includes( String(process.env.PULSE_E2E_REQUIRE_DEFAULT_MOCK_READY || "") @@ -1167,11 +1152,6 @@ const requiresDefaultMockRuntimeReadiness = (): boolean => .toLowerCase(), ); -const resourceHasSource = ( - resource: E2ERuntimeStateResource, - source: string, -): boolean => resource.sources?.includes(source) === true; - export async function waitForDefaultMockRuntimeReady(page: Page): Promise { if (!requiresDefaultMockRuntimeReadiness()) { return; @@ -1185,48 +1165,7 @@ export async function waitForDefaultMockRuntimeReady(page: Page): Promise return false; } - const state = (await response.json()) as E2ERuntimeState; - const resources = Array.isArray(state.resources) ? state.resources : []; - const infrastructure = Array.isArray(state.connectedInfrastructure) - ? state.connectedInfrastructure - : []; - - return ( - resources.some( - (resource) => - resource.name === "nvme-primary" && - resource.type === "storage" && - resourceHasSource(resource, "vmware"), - ) && - resources.some( - (resource) => - resource.type === "k8s-cluster" && - resourceHasSource(resource, "kubernetes"), - ) && - resources.some( - (resource) => - resource.name === "tank" && - resourceHasSource(resource, "truenas"), - ) && - resources.some( - (resource) => - (resource.type === "vm" || - resource.type === "system-container") && - resourceHasSource(resource, "proxmox"), - ) && - resources.some( - (resource) => - resource.name === "esxi-01.lab.local" && - resource.type === "agent" && - resourceHasSource(resource, "vmware"), - ) && - resources.some((resource) => resource.type === "docker-host") && - resources.some((resource) => resource.type === "pbs") && - resources.some((resource) => resource.type === "pmg") && - infrastructure.some( - (entry) => entry.name === "esxi-01.lab.local", - ) - ); + return defaultMockInventoryReady(await response.json()); }, { message: @@ -1237,17 +1176,6 @@ export async function waitForDefaultMockRuntimeReady(page: Page): Promise ) .toBe(true); - const hasDeepSeries = (points: E2EMetricPoint[] | undefined): boolean => { - const timestamps = (points ?? []) - .map((point) => Number(point.timestamp)) - .filter(Number.isFinite) - .sort((left, right) => left - right); - if (timestamps.length < 2) { - return false; - } - return timestamps[timestamps.length - 1] - timestamps[0] > 5 * 24 * 60 * 60 * 1000; - }; - // Inventory becomes available before the monitor has finished building its // historical mock timeline. Do not advertise the shared browser fixture as // ready until the deepest Core E2E chart window is queryable for both pools @@ -1264,15 +1192,7 @@ export async function waitForDefaultMockRuntimeReady(page: Page): Promise return false; } - const charts = (await response.json()) as E2EStorageCharts; - return ( - Object.values(charts.pools ?? {}).some((pool) => - hasDeepSeries(pool.used), - ) && - Object.values(charts.disks ?? {}).some((disk) => - hasDeepSeries(disk.temperature), - ) - ); + return defaultMockHistoryReady(await response.json()); }, { message: