adaptive_export: /query runner + dx_evidence_graph rename + evidence_manifest (folds #77/#78)#73
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdaptive export replaces attack-graph ingestion with evidence-graph and manifest ClickHouse storage, adds corresponding control endpoints, wires the active controller, and introduces ordered Pixie queries. Repository lint, workflow, Skaffold, Copybara, and Bazel ignore configuration is also updated. ChangesAdaptive export evidence flow
Repository configuration cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ControlServer
participant Controller
participant PixieQuerier
participant Applier
participant ClickHouse
Client->>ControlServer: POST evidence graph or manifest
ControlServer->>Controller: OrderQuery target table window
Controller->>PixieQuerier: Execute generated PxL query
PixieQuerier-->>Controller: Return Pixie rows
Controller->>Applier: WritePixieRows
ControlServer->>Applier: Write evidence JSONEachRow
Applier->>ClickHouse: Insert evidence data
ClickHouse-->>Applier: Return write result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@build-agent — please cut an AE release from this branch ( What changed vs the current aeprod (what to exercise):
Live test I'll run (paired with dx Ping the image tag here when green and I'll swap the rig's AE DS to it. |
fb9ccec to
7fc5561
Compare
|
🛠️ AE image build kicked off — Before building I rebased this branch onto the current PR 68 tip ( Images publish to GHCR on success (~55–60 min). I'll report the result here. |
|
✅ Build succeeded — run 28318532828 completed. AE image: Full vizier image set (pem, kelvin, query_broker, metadata, cloud_connector, cert_provisioner, …) published under the same |
…iew) Restacks the dx graph rename onto the #73 /query-runner tip. Renames across the AE forensic path: - table forensic_db.dx_attack_graph → dx_evidence_graph - view dx_attack_graph_malicious → dx_evidence_graph_malignant (medical terminology: malignant, not malicious) - endpoint /dx/attack_graph → /dx/evidence_graph - Go WriteAttackGraph/handleDXAttackGraph → *EvidenceGraph Done mechanically on #73 so it also covers order_query_test.go (absent from the original rename branch's lineage). Also fixes two pre-existing #73 lint nits surfaced by touching the package: gofumpt blank lines in order_query_test.go and the missing order_query_test.go entry in controller/BUILD.bazel's controller_test srcs (gazelle).
…iew) Restacks the dx graph rename onto the #73 /query-runner tip. Renames across the AE forensic path: - table forensic_db.dx_attack_graph → dx_evidence_graph - view dx_attack_graph_malicious → dx_evidence_graph_malignant (medical terminology: malignant, not malicious) - endpoint /dx/attack_graph → /dx/evidence_graph - Go WriteAttackGraph/handleDXAttackGraph → *EvidenceGraph Done mechanically on #73 so it also covers order_query_test.go (absent from the original rename branch's lineage). Also fixes two pre-existing #73 lint nits surfaced by touching the package: gofumpt blank lines in order_query_test.go and the missing order_query_test.go entry in controller/BUILD.bazel's controller_test srcs (gazelle).
Test: aeprod28-rc1 on a live rig — steering ✅, but the new tables are starved upstream (pixie)Deployed ✅ What works
|
RCA: why dns/pgsql/kernel don't persist on our rigs — two layers, layer 1 fixed liveLayer 1 (FIXED live) — AE was on the cloud, not directAE startup: Fix applied on the rig (aeprod28): set Layer 2 (still 0: kernel + dns) — the per-anomaly-pod query can't reach them
So: layer 1 = config (fixed, make it default); layer 2 = |
…sic capture (dx#93) The control surface shipped with control.New(activeSet, nil) — '/query runner wired later'. So every dx OrderQuery 501'd and the table dx read for a verdict (e.g. the jndi-in-http) never reached forensic_db unless a kubescape-anomaly window happened to push it. The forensic export steered to noise, not to dx's evidence. Adds Controller.OrderQuery(target, table, window, queryID): a single-shot QueryFor → querier.Query → sink.WritePixieRows (the same path pushPixieRows uses, plus globalSem + reconcile accounting), independent of any anomaly window. Wires control.New(activeSet, ctl) in main.go so POST /query executes it. When the operator-side querier is disabled, OrderQuery errors and /query 502s; start/stop + dx_attack_graph are unaffected. Pairs with entlein/dx#96 which orders the full consulted read-set through this runner. Tests: writes-rows / no-querier-errors / empty-no-write / sink-error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iew) Restacks the dx graph rename onto the #73 /query-runner tip. Renames across the AE forensic path: - table forensic_db.dx_attack_graph → dx_evidence_graph - view dx_attack_graph_malicious → dx_evidence_graph_malignant (medical terminology: malignant, not malicious) - endpoint /dx/attack_graph → /dx/evidence_graph - Go WriteAttackGraph/handleDXAttackGraph → *EvidenceGraph Done mechanically on #73 so it also covers order_query_test.go (absent from the original rename branch's lineage). Also fixes two pre-existing #73 lint nits surfaced by touching the package: gofumpt blank lines in order_query_test.go and the missing order_query_test.go entry in controller/BUILD.bazel's controller_test srcs (gazelle).
…manifest
Closes the AE-side wire gap for dx's §9 completeness contract (stacked on
the dx_evidence_graph rename). dx already POSTs manifests via
aeclient.WriteEvidenceManifest → /dx/evidence_manifest; aeprod26 404s that
path, so manifest_rows_exported=0 / graph_write_failures{kind=manifest}>0.
Adds, mirroring the evidence_graph path:
- control: manifestWriter iface + SetManifestWriter + POST /dx/evidence_manifest
handler. Accepts one manifest.Manifest per verdict; scalars map to typed
columns, nested collections (case_window/findings/orders/seeds/chain) are
rendered as JSON text so the JSONEachRow insert is CH-version independent.
- clickhouse: forensic_db.dx_evidence_manifest table (columns = manifest.Manifest
JSON tags; event_time nanos + hostname read-path like dx_evidence_graph),
WriteEvidenceManifest sink, KnownTables + OperatorOwnedTables (created on boot).
- main: wire SetManifestWriter(applier).
- tests: endpoint 501/202/502 + nested-as-text flattening; table-set guard.
|
Superseded by #85 — the three PRs (#73/#77/#78) were a linear one-commit-each stack on the pre-squash |
7fc5561 to
da37ced
Compare
|
@build-agent — AE image (ttl.sh fine) from this branch HEAD ( |
entlein
changed the title
|
✅ TTL AE image ready (fast local build → ttl.sh):
Durable release |
- .arclint: exclude the adaptive_export_loadtest/suite/ nested go module (module 'aeloadsuite') from linters — golangci-lint typecheck failed on it from the root module, same as the already-excluded tools/loadgen/. - skaffold.yaml (loadtest): reindent the requires: block to indent-sequences:false (yamllint: expected 0/4, was 2/6). yamllint clean now. - e2e_calibrate_soc.yaml: strip trailing whitespace (arc autofix). - copy.bara.sky: drop the malformed duplicate line (md_udtfs/** + shared/manager BUILD.bazel concatenated behind a comment; both already listed above it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017k7uYSNUctQvkTAZYJbaB3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vizier/services/adaptive_export/cmd/main.go`:
- Around line 576-582: Update controller initialization around ctrlSrv and the
PixieQuerier setup so the query adapter is created and attached whenever
control-ordered queries are enabled, including when only CONTROL_ADDR is
configured. Remove the dependency on PushPixieTables being non-empty for wiring
/query, while preserving the existing anomaly-driven push behavior.
In `@src/vizier/services/adaptive_export/internal/control/server.go`:
- Around line 194-220: Enforce the evidence contract across all ingestion paths:
in src/vizier/services/adaptive_export/internal/control/server.go lines 194-220,
update the manifest handler around dxManifest decoding and
manifest.WriteEvidenceManifest to reject missing required identity and timestamp
fields and validate verdict and enum fields before returning success; in
src/vizier/services/adaptive_export/internal/clickhouse/apply.go lines 123-124
and 137-138, enable FailLoud for both evidence-graph and manifest inserts.
In `@src/vizier/services/adaptive_export/internal/controller/controller.go`:
- Around line 267-296: Update OrderQuery to accept the HTTP request context and
propagate it through the ordered reconcile flow. Use the request context for
semaphore acquisition and derive both the 180-second query and 60-second write
timeout contexts from it instead of context.Background, preserving cancellation
when the request disconnects or times out.
- Around line 262-266: Update the query construction in the controller flow
around QueryFor to select a table-specific query strategy based on the target
table’s contract. Avoid using QueryFor for kernel tables that lack upid, and
apply connection-scoped filtering for server-side PostgreSQL evidence; preserve
the existing ordered-capture behavior for tables compatible with QueryFor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6828aec8-cdd3-4c43-9002-2500261e8cda
📒 Files selected for processing (14)
.arclint.github/workflows/e2e_calibrate_soc.yamlsrc/e2e_test/adaptive_export_loadtest/skaffold.yamlsrc/vizier/services/adaptive_export/cmd/main.gosrc/vizier/services/adaptive_export/internal/clickhouse/apply.gosrc/vizier/services/adaptive_export/internal/clickhouse/apply_test.gosrc/vizier/services/adaptive_export/internal/clickhouse/ddl.gosrc/vizier/services/adaptive_export/internal/clickhouse/schema.sqlsrc/vizier/services/adaptive_export/internal/control/server.gosrc/vizier/services/adaptive_export/internal/control/server_test.gosrc/vizier/services/adaptive_export/internal/controller/BUILD.bazelsrc/vizier/services/adaptive_export/internal/controller/controller.gosrc/vizier/services/adaptive_export/internal/controller/order_query_test.gotools/private/copybara/copy.bara.sky
💤 Files with no reviewable changes (1)
- tools/private/copybara/copy.bara.sky
| // Wire the controller as the /query runner: dx OrderQuery → one-shot pixie | ||
| // capture written to forensic_db (write⊇read; entlein/dx#93). When the | ||
| // operator-side querier is disabled (no PushPixieTables), OrderQuery returns | ||
| // an error and /query 502s — start/stop + dx_evidence_graph still work. | ||
| ctrlSrv := control.New(activeSet, ctl) | ||
| ctrlSrv.SetGraphWriter(applier) // dx_evidence_graph ingest → ClickHouse | ||
| ctrlSrv.SetManifestWriter(applier) // dx_evidence_manifest ingest → ClickHouse |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not gate /query on anomaly-driven push mode.
The controller only receives a PixieQuerier when PushPixieTables is non-empty, so enabling CONTROL_ADDR alone exposes a /query endpoint that always returns 502. Create and attach the adapter whenever control-ordered queries are enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/cmd/main.go` around lines 576 - 582,
Update controller initialization around ctrlSrv and the PixieQuerier setup so
the query adapter is created and attached whenever control-ordered queries are
enabled, including when only CONTROL_ADDR is configured. Remove the dependency
on PushPixieTables being non-empty for wiring /query, while preserving the
existing anomaly-driven push behavior.
| var m dxManifest | ||
| if !decode(w, r, &m) { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| row := map[string]any{ | ||
| "investigation_id": m.InvestigationID, | ||
| "event_time": m.EventTime, | ||
| "hostname": m.Hostname, | ||
| "condition": m.Condition, | ||
| "verdict": m.Verdict, | ||
| "confidence": m.Confidence, | ||
| "posterior": m.Posterior, | ||
| "catalog_version": m.CatalogVersion, | ||
| "case_window": jsonText(m.CaseWindow), | ||
| "findings": jsonText(m.Findings), | ||
| "orders": jsonText(m.Orders), | ||
| "seeds": jsonText(m.Seeds), | ||
| "chain": jsonText(m.Chain), | ||
| "evidence_hash": m.EvidenceHash, | ||
| } | ||
| line, err := json.Marshal(row) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| return | ||
| } | ||
| if err := s.manifest.WriteEvidenceManifest(r.Context(), append(line, '\n')); err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the evidence contract before returning 202.
Both ingestion paths currently permit incomplete or schema-mismatched evidence to be stored with default values.
src/vizier/services/adaptive_export/internal/control/server.go#L194-L220: validate required manifest identity, timestamp, verdict, and enum fields.src/vizier/services/adaptive_export/internal/clickhouse/apply.go#L123-L124: enableFailLoudfor evidence-graph inserts.src/vizier/services/adaptive_export/internal/clickhouse/apply.go#L137-L138: enableFailLoudfor manifest inserts.
📍 Affects 2 files
src/vizier/services/adaptive_export/internal/control/server.go#L194-L220(this comment)src/vizier/services/adaptive_export/internal/clickhouse/apply.go#L123-L124src/vizier/services/adaptive_export/internal/clickhouse/apply.go#L137-L138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/control/server.go` around lines
194 - 220, Enforce the evidence contract across all ingestion paths: in
src/vizier/services/adaptive_export/internal/control/server.go lines 194-220,
update the manifest handler around dxManifest decoding and
manifest.WriteEvidenceManifest to reject missing required identity and timestamp
fields and validate verdict and enum fields before returning success; in
src/vizier/services/adaptive_export/internal/clickhouse/apply.go lines 123-124
and 137-138, enable FailLoud for both evidence-graph and manifest inserts.
| now := c.clock.Now() | ||
| q, err := pxl.QueryFor(table, target, start, end, now) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use table-specific filtering for ordered captures.
QueryFor requires a built-in table and always enriches/filters through upid. The identified kernel tables lack upid, while server-side PostgreSQL evidence needs connection-scoped filtering, so these orders fail or return incomplete results. Select a query strategy based on the table contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/controller/controller.go` around
lines 262 - 266, Update the query construction in the controller flow around
QueryFor to select a table-specific query strategy based on the target table’s
contract. Avoid using QueryFor for kernel tables that lack upid, and apply
connection-scoped filtering for server-side PostgreSQL evidence; preserve the
existing ordered-capture behavior for tables compatible with QueryFor.
| // Background ctx with per-op timeouts mirroring pushPixieRows: a control-ordered | ||
| // capture must complete independently of any anomaly window's lifecycle. | ||
| var readCount, wroteCount int | ||
| var recErr string | ||
| defer func() { | ||
| c.cfg.Rec.Record(context.Background(), reconcile.Row{ | ||
| TS: now, Mode: "ordered", Table: table, | ||
| Namespace: target.Namespace, Pod: target.Pod, | ||
| WinStart: start, WinEnd: end, | ||
| ReadCount: int64(readCount), WroteCount: int64(wroteCount), | ||
| WriteErr: recErr, Hostname: c.cfg.Hostname, | ||
| }) | ||
| }() | ||
| if c.globalSem != nil { | ||
| c.globalSem <- struct{}{} | ||
| defer func() { <-c.globalSem }() | ||
| } | ||
| qctx, cancel := context.WithTimeout(context.Background(), 180*time.Second) | ||
| rows, qerr := c.querier.Query(qctx, q) | ||
| cancel() | ||
| if qerr != nil { | ||
| recErr = qerr.Error() | ||
| return qerr | ||
| } | ||
| readCount = len(rows) | ||
| if len(rows) == 0 { | ||
| return nil // nothing to persist; the read/0-wrote reconcile row still records it | ||
| } | ||
| wctx, wcancel := context.WithTimeout(context.Background(), 60*time.Second) | ||
| werr := c.sink.WritePixieRows(wctx, table, rows) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate the HTTP request context through the ordered query.
The semaphore wait and 180s/60s operations use context.Background, while the control server has a 30s write timeout. A disconnected or timed-out request therefore continues and may be retried, producing duplicate writes and unbounded queued handlers. Add context.Context to OrderQuery, use it for semaphore acquisition, and derive both operation timeouts from it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vizier/services/adaptive_export/internal/controller/controller.go` around
lines 267 - 296, Update OrderQuery to accept the HTTP request context and
propagate it through the ordered reconcile flow. Use the request context for
semaphore acquisition and derive both the 180-second query and 60-second write
timeout contexts from it instead of context.Background, preserving cancellation
when the request disconnects or times out.
|
🔄 Refreshed TTL AE image at the current #73 HEAD (
Durable release |
|
✅ Durable release image published (same
(The |
run-genfiles failed because gazelle generated src/e2e_test/adaptive_export_loadtest/
suite/BUILD.bazel (untracked → dirty tree). suite/ is a separate go module
('aeloadsuite'), not part of the root px.dev/pixie build — gazelle's generated
importpath/deps for it are wrong. Ignore it like the sibling tools/loadgen module
(already in .bazelignore) so gazelle skips it. Verified: gazelle fix leaves the
tree clean afterward.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017k7uYSNUctQvkTAZYJbaB3
|
Both red checks were real — root-caused + fixed: 1. 2. Note: the AE image always published fine on the earlier runs; only this post-publish manifest step was red. |
|
✅ Both checks now green on CI:
Durable release image ( Binary-identical to the earlier ttl.sh / aeprod33 images (the two fixes are |
2 participants
This PR now carries the combined AE stack, rebased onto main — the discussion above (incl. the "pgsql events not reaching ClickHouse / AE only exports http/dns/conn_stats" investigation) stays here where it belongs.
#68 was squash-merged to main, so the old stack (#73→#77→#78, one commit each) was cherry-picked cleanly onto current
main(no conflicts) and force-pushed here:feat(adaptive_export): wire the /query runner— dxOrderQuery→ forensic capture (dx#93): AE runs a dx-ordered query and captures the result toforensic_db— the mechanism by which dx orders capture of a workload's protocol tables (inclpgsql_events).refactor(ae): rename dx_attack_graph → dx_evidence_graph(+_malicious→_malignantview).feat(ae): /dx/evidence_manifest ingest → forensic_db.dx_evidence_manifest.Delta over main: 10 files, +442/−35. Folds in #77 + #78; #81 (DNS ppid) handled separately. Supersedes the transient #85 (closed).
To be tested by replacing the AE image on the existing rig (fresh/empty ClickHouse) alongside dx
release/dx/v0.3.0-nsprec1.