pem: direct-query gRPC endpoint — stub + TDD contract#49
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds a PEM direct-query ExecuteScript gRPC endpoint with HS256 JWT bearer-token verification, server-streaming query execution, comprehensive in-process tests, PEM runtime wiring, Bazel visibility and dependency updates, and minor utility/CI tweaks. ChangesPEM Direct-Query Feature
Sequence DiagramsequenceDiagram
participant Client
participant DirectQueryServer
participant JWTVerifier
participant Carnot
participant ResultSink
Client->>DirectQueryServer: ExecuteScriptRequest with Bearer token
DirectQueryServer->>JWTVerifier: AuthenticateRequest(authorization header)
JWTVerifier->>JWTVerifier: Extract and base64url decode JWT parts
JWTVerifier->>JWTVerifier: Verify HS256 HMAC-SHA256 signature
JWTVerifier->>JWTVerifier: Validate claims (aud, iss, exp, Scopes)
alt Auth failure
JWTVerifier-->>DirectQueryServer: UNAUTHENTICATED
DirectQueryServer-->>Client: UNAUTHENTICATED
else Auth success
JWTVerifier-->>DirectQueryServer: OK
DirectQueryServer->>Carnot: Compile PxL query
alt Compile failure
Carnot-->>DirectQueryServer: error
DirectQueryServer-->>Client: INVALID_ARGUMENT
else Compile success
Carnot-->>DirectQueryServer: compiled plan with sinks
DirectQueryServer->>ResultSink: ResetQueryResults()
DirectQueryServer->>Carnot: ExecuteQuery()
loop Drain results
ResultSink-->>DirectQueryServer: ExecuteScriptResponse (schema/rows/stats)
DirectQueryServer-->>Client: stream response
end
DirectQueryServer-->>Client: OK (end stream)
end
end
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/agent/pem/direct_query_server_test.cc`:
- Around line 58-67: The test ValidToken_Mutation_Unimplemented is unreachable
because MakeBearerToken returns placeholder non-JWT strings and
AuthenticateRequest always returns UNAUTHENTICATED; change the test surface so a
"valid" token is actually recognized: either make MakeBearerToken produce a
token format that AuthenticateRequest will accept as valid (e.g., generate a
simple signed JWT or a recognized test token when TokenKind::kValid) or adjust
AuthenticateRequest (in direct_query_server.cc's AuthenticateRequest) to accept
the test placeholder for TokenKind::kValid; update MakeBearerToken's TokenKind
cases (kValid, kWrongKey, kExpired) to return distinct tokens that map to
AuthenticateRequest's logic so the ValidToken_Mutation_Unimplemented path can
observe UNIMPLEMENTED as intended.
🪄 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: 649f9e15-2555-4317-9973-b200df724e3d
📒 Files selected for processing (5)
src/vizier/services/agent/pem/BUILD.bazelsrc/vizier/services/agent/pem/DIRECT_QUERY_CONTRACT.mdsrc/vizier/services/agent/pem/direct_query_server.ccsrc/vizier/services/agent/pem/direct_query_server.hsrc/vizier/services/agent/pem/direct_query_server_test.cc
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/vizier/services/agent/pem/direct_query_server.cc (1)
175-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStill missing the cluster-service claim checks.
This verifier still accepts any HS256 token signed with the shared key and
aud=="vizier". That is broader than the stated cluster-service JWT contract, becauseiss,sub,Scopes, andServiceIDare never enforced here.Suggested tightening
if (!aud_ok) { return ::grpc::Status(::grpc::StatusCode::UNAUTHENTICATED, "direct-query: wrong audience (expected vizier)"); } + if (!payload.HasMember("iss") || !payload["iss"].IsString() || + std::strcmp(payload["iss"].GetString(), "PL") != 0 || + !payload.HasMember("sub") || !payload["sub"].IsString() || + std::strcmp(payload["sub"].GetString(), "service") != 0 || + !payload.HasMember("Scopes") || !payload["Scopes"].IsString() || + std::strcmp(payload["Scopes"].GetString(), "service") != 0 || + !payload.HasMember("ServiceID") || !payload["ServiceID"].IsString() || + payload["ServiceID"].GetStringLength() == 0) { + return ::grpc::Status(::grpc::StatusCode::UNAUTHENTICATED, + "direct-query: not a cluster service JWT"); + } if (!payload.HasMember("exp")) { return ::grpc::Status(::grpc::StatusCode::UNAUTHENTICATED, "direct-query: missing exp claim"); }🤖 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/agent/pem/direct_query_server.cc` around lines 175 - 218, The verifier currently only checks aud and exp but must enforce the cluster-service JWT contract: after the aud check and before exp validation, add explicit checks on payload for "iss" (must be string == kExpectedIssuer), "sub" (must be string == kExpectedSubject), "Scopes" (must contain the cluster-service scope, e.g., check string or array includes kClusterServiceScope), and "ServiceID" (must exist and match the expected service/cluster id, e.g., kExpectedServiceID or the runtime cluster id). For each missing or mismatched claim return ::grpc::Status(::grpc::StatusCode::UNAUTHENTICATED, "<contextual message>") similar to the existing messages; use the existing payload variable and keep the new constants kExpectedIssuer, kExpectedSubject, kClusterServiceScope, kExpectedServiceID (or existing equivalents) to locate where to enforce these checks.
🤖 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/agent/pem/direct_query_server_test.cc`:
- Around line 196-236: The test uses an empty http_events table so it never
exercises the drainSinkAndStream() column-copy path; seed at least one row into
the table (e.g., in MakeHTTPEventsTable() or in
TEST_F(DirectQueryServerExecTest, ValidToken_TrivialQuery_StreamsRows) before
calling stub_->ExecuteScript) using the Table API for "http_events" (populate
fields time_, upid, remote_addr, remote_port, trace_role), then when streaming
responses from stub_->ExecuteScript assert that you observe a streamed batch
with resp.has_table_id() == true (or matches the expected table id) and
resp.num_rows() > 0 to validate the non-empty row-batch path is exercised.
---
Duplicate comments:
In `@src/vizier/services/agent/pem/direct_query_server.cc`:
- Around line 175-218: The verifier currently only checks aud and exp but must
enforce the cluster-service JWT contract: after the aud check and before exp
validation, add explicit checks on payload for "iss" (must be string ==
kExpectedIssuer), "sub" (must be string == kExpectedSubject), "Scopes" (must
contain the cluster-service scope, e.g., check string or array includes
kClusterServiceScope), and "ServiceID" (must exist and match the expected
service/cluster id, e.g., kExpectedServiceID or the runtime cluster id). For
each missing or mismatched claim return
::grpc::Status(::grpc::StatusCode::UNAUTHENTICATED, "<contextual message>")
similar to the existing messages; use the existing payload variable and keep the
new constants kExpectedIssuer, kExpectedSubject, kClusterServiceScope,
kExpectedServiceID (or existing equivalents) to locate where to enforce these
checks.
🪄 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: 325769b9-0ed7-4ca0-8704-52d0312dd1e0
📒 Files selected for processing (10)
.github/workflows/vizier_release.yamlsrc/carnot/BUILD.bazelsrc/carnot/exec/BUILD.bazelsrc/carnot/udf/BUILD.bazelsrc/vizier/services/agent/pem/BUILD.bazelsrc/vizier/services/agent/pem/direct_query_server.ccsrc/vizier/services/agent/pem/direct_query_server_test.ccsrc/vizier/services/agent/pem/pem_main.ccsrc/vizier/services/agent/pem/pem_manager.ccsrc/vizier/services/agent/pem/pem_manager.h
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 4, 2026
dx-agent observed the stock fork 0.14.17 PEM in CrashLoopBackOff (23 restarts over hours) with: libc++abi: terminating due to uncaught exception of type jwt::SigningError: key not provided Root cause: src/vizier/services/agent/shared/manager/manager.cc:434 calls `obj.secret(FLAGS_jwt_signing_key); obj.signature();` in GenerateServiceToken. cpp_jwt's signature() throws SigningError when the secret is empty. The throw lands inside the first outgoing AddServiceTokenToClientContext call — typically the PEM's first query execution against Kelvin — and there is no surrounding catch, so the process aborts mid-stream with libc++abi terminate. Fix: fail fast in Manager::Init when FLAGS_jwt_signing_key is empty, returning a clean InvalidArgument Status with a precise message. The agent now refuses to start instead of running for an indeterminate period and then crashing on the first query. Lives in the shared base so it covers Kelvin + PEM both. Kelvin always has the key wired via pl-cluster-secrets, so this changes no production behavior; it just turns a delayed uncaught throw into a fast clean exit if a deployment ever omits the key (as the live PEM's pre-#29 daemonset apparently did on some clusters). Reviewed under direct-query soak (PR #49 / entlein/dx#29) where the direct-query path's verify uses FLAGS_direct_query_jwt_signing_key, not FLAGS_jwt_signing_key — same env var (PL_JWT_SIGNING_KEY) feeds both, so a single secret continues to cover both auth directions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vizier/services/agent/pem/direct_query_server.cc (1)
411-422:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSerialize sink reset/execute/drain to prevent cross-request result corruption.
result_server_is shared state, and this method resets plus drains global accumulated chunks. ConcurrentExecuteScriptcalls can clobber each other and stream mixed results.Suggested minimal guard (serialize direct-query execution path)
+#include "absl/synchronization/mutex.h" ... namespace { +absl::Mutex g_direct_query_exec_mu; } // namespace ... ::grpc::Status DirectQueryServer::ExecuteScript( ::grpc::ServerContext* context, const ::px::api::vizierpb::ExecuteScriptRequest* request, ::grpc::ServerWriter<::px::api::vizierpb::ExecuteScriptResponse>* writer) { + absl::MutexLock lk(&g_direct_query_exec_mu); if (auto s = AuthenticateRequest(context, jwt_signing_key_); !s.ok()) { return s; } ... result_server_->ResetQueryResults(); auto exec_s = carnot_->ExecuteQuery(request->query_str(), query_id, ::px::CurrentTimeNS());🤖 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/agent/pem/direct_query_server.cc` around lines 411 - 422, This RPC resets and reads the shared result sink (result_server_) around carnot_->ExecuteQuery and drainSinkAndStream, so concurrent ExecuteScript/ExecuteQuery calls can interleave and corrupt results; serialize the sequence by introducing a mutex (e.g., a class member direct_query_mu_) and acquire a std::lock_guard (or std::unique_lock) at the start of the method that surrounds result_server_->ResetQueryResults(), the call to carnot_->ExecuteQuery(...), and drainSinkAndStream(result_server_, query_id_str, writer) so the reset/execute/drain happens atomically for a single request.
🤖 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/agent/pem/pem_manager.cc`:
- Around line 39-42: The code currently uses FLAGS_direct_query_jwt_signing_key
separately from FLAGS_jwt_signing_key causing split-brain; change the logic in
pem_manager.cc to treat FLAGS_direct_query_jwt_signing_key as optional and fall
back to FLAGS_jwt_signing_key when empty (i.e., wherever
FLAGS_direct_query_jwt_signing_key is read—references around the DEFINE_string
and the usages near the blocks you noted at lines ~121-125 and ~161-163—use a
single effective key variable like effective_direct_query_key =
FLAGS_direct_query_jwt_signing_key.empty() ? FLAGS_jwt_signing_key :
FLAGS_direct_query_jwt_signing_key and use that variable for direct-query
auth/minting).
---
Outside diff comments:
In `@src/vizier/services/agent/pem/direct_query_server.cc`:
- Around line 411-422: This RPC resets and reads the shared result sink
(result_server_) around carnot_->ExecuteQuery and drainSinkAndStream, so
concurrent ExecuteScript/ExecuteQuery calls can interleave and corrupt results;
serialize the sequence by introducing a mutex (e.g., a class member
direct_query_mu_) and acquire a std::lock_guard (or std::unique_lock) at the
start of the method that surrounds result_server_->ResetQueryResults(), the call
to carnot_->ExecuteQuery(...), and drainSinkAndStream(result_server_,
query_id_str, writer) so the reset/execute/drain happens atomically for a single
request.
🪄 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: 271756a5-b762-4b8d-9811-f5b171d98116
📒 Files selected for processing (4)
src/vizier/services/agent/pem/direct_query_server.ccsrc/vizier/services/agent/pem/pem_main.ccsrc/vizier/services/agent/pem/pem_manager.ccsrc/vizier/services/agent/shared/manager/manager.cc
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 5, 2026
dx-agent caught aeprod1 (run 26982157827) sitting `queued` 5h with `runner_name:""` — no runner carries `oracle-16cpu-64gb-x86-64` or `oracle-8cpu-32gb-x86-64` on this fork. The active fleet uses the `-vm-` form: `oracle-vm-16cpu-64gb-x86-64`. PR #49 (PEM branch) already shipped this fix; the AE branch missed it because of when it forked from main. Aligns both jobs: `build-release` (was `oracle-16cpu`) and `update-gh-artifacts-manifest` (was `oracle-8cpu`). Cancelled run was confirmed dead by dx-agent.
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 5, 2026
Three PR-checks were failing: 1. run-container-lint (cfmt) — pem_manager.cc had a two-line LOG that clang-format wants on one line. `arc lint --apply-patches` autofixed the step 6/6 LOG(INFO) wrap. No behavioral change. 2. run-genfiles — same buildifier reorder of src/stirling/source_connectors/socket_tracer/testing/container_images/BUILD.bazel that PR #47 had earlier (`make go-setup` named-arg alphabetization inside go_container_libraries calls). Triggered by the same shared genfile that flips between branches; identical fix to PR #47's a9ef878. 3. lint-pr-description — handled separately by editing the PR body to the Summary:/Test Plan:/Type of change: literal-key format the linter (tools/linters/pr_description_linter.sh) requires (was markdown `## Summary` headers, which the script's `^Summary: .+` regex doesn't match). No commit needed for that one.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vizier/services/agent/pem/direct_query_server.cc (1)
301-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop work when stream writes fail.
In
src/vizier/services/agent/pem/direct_query_server.cc, theServerWriter::Write(...)return values are ignored atwriter->Write(schema_resp);(around lines 301) andwriter->Write(resp);(around line 373). If the client disconnects, the server can keep doing schema/result processing instead of aborting early.Handle
Write(...)failures by propagating them up and returning aCANCELLED/early-abort status to stop further work/draining.🤖 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/agent/pem/direct_query_server.cc` around lines 301 - 302, The server currently ignores the boolean return from writer->Write(...) calls (notably the calls with schema_resp and resp in direct_query_server.cc), so if the client disconnects the server continues processing; modify the enclosing methods (the RPC handler or helper functions around the writer->Write(schema_resp) and writer->Write(resp) sites) to check the Write(...) return value and, on failure, immediately stop further work and return a gRPC CANCELLED status (or propagate a failure status) up to the caller so processing/draining aborts; ensure any callers of those helpers propagate that Status instead of ignoring it.
🤖 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/agent/pem/direct_query_server.cc`:
- Around line 329-374: This code leaks shared mutable sink state across
concurrent ExecuteScript calls because ResetQueryResults(), ExecuteQuery(), and
draining raw_query_results() access the same sink; protect the critical section
by serializing per-request access (e.g., use a mutex or request-scoped lock)
around the sequence that calls ResetQueryResults(), ExecuteQuery(), and the loop
over result_server->raw_query_results() so chunks cannot be cleared or
interleaved by another request; apply the same protection to the analogous block
referenced at the other location (the 414-421 section) and ensure the lock is
held from before ResetQueryResults() until after the writer->Write(resp) loop
completes.
---
Outside diff comments:
In `@src/vizier/services/agent/pem/direct_query_server.cc`:
- Around line 301-302: The server currently ignores the boolean return from
writer->Write(...) calls (notably the calls with schema_resp and resp in
direct_query_server.cc), so if the client disconnects the server continues
processing; modify the enclosing methods (the RPC handler or helper functions
around the writer->Write(schema_resp) and writer->Write(resp) sites) to check
the Write(...) return value and, on failure, immediately stop further work and
return a gRPC CANCELLED status (or propagate a failure status) up to the caller
so processing/draining aborts; ensure any callers of those helpers propagate
that Status instead of ignoring it.
🪄 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: cfc25291-1a5a-4a5a-8032-7380f3e88e1b
📒 Files selected for processing (4)
src/vizier/services/agent/pem/direct_query_server.ccsrc/vizier/services/agent/pem/pem_main.ccsrc/vizier/services/agent/pem/pem_manager.ccsrc/vizier/services/agent/shared/manager/manager.cc
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 5, 2026
…ntlein/dx#29) User asks on PR #49: 1. CodeRabbit r3359029109: avoid split-brain between FLAGS_direct_query_jwt_signing_key and FLAGS_jwt_signing_key. 2. Extend direct_query_server_test.cc with broader query coverage + robustness. 3. Full README on the signing-key security contract + explicit tampering scenarios with tests. 4. Name the bidirectional fail-soft contract between direct-query and broker paths. Address (1) — pem_manager.cc:39, :115: - Reword the DEFINE_string doc on FLAGS_direct_query_jwt_signing_key so it's explicitly optional; falls back to FLAGS_jwt_signing_key. - DECLARE_string(jwt_signing_key) at the top of pem_manager.cc (the DEFINE_string lives in shared/manager/manager.cc). - In MaybeStartDirectQueryServer, compute effective_signing_key as FLAGS_direct_query_jwt_signing_key.empty() ? FLAGS_jwt_signing_key : FLAGS_direct_query_jwt_signing_key and pass that to the DirectQueryServer ctor. Empty-effective-key still fails soft with LOG(ERROR) and Status::OK(). - Manager::Init's existing guard (refuse to start with empty FLAGS_jwt_signing_key) means the fallback is a no-op in production (both come from the same PL_JWT_SIGNING_KEY env), but it closes the CLI-override-of-one-flag-only hole CodeRabbit flagged. Address (2) + (3) — direct_query_server_test.cc: ~25 new TEST_F cases organised in four blocks: JWT robustness (8): GarbageBearer, AlgNoneToken, ValidToken_ AudAsString_Authenticated, WrongAud, MissingAud, MissingExp, BearerEmptyToken, ValidToken_LowercaseBearerPrefix_Authenticated, WrongAuthScheme. Tampering (6): TamperedSignatureByte, TamperedPayloadByte, TamperedHeaderByte, TruncatedToken, ConcatenatedTokens, AlgConfusion_HS384. Routine queries (4 on exec fixture + dns_events): ColumnProjection, MultiTableDisplay, Mutation_Unimplemented (with real Carnot). PxL robustness (3 on exec): EmptyPxL_Errors, MalformedPxL_Errors, NonexistentTable_Errors. Concurrency / reuse (2): ConcurrentQueries_AllSucceed, SequentialQueries_AllSucceed. Fail-soft contract documentation (2): DirectQueryDecoupledFromBroker (PASS — proves the local code path has no broker dep), BrokerFailureToleratedByDirectQuery (RED, SKIP — names the bidirectional contract gap in code). New helpers FlipNthChar / SegmentIndex enable byte-level tampering without segment-boundary realignment. TokenKind enum extended with kAudAsString / kMissingAud / kWrongAud / kMissingExp / kAlgNone for named token shapes; comment block on the enum lists the verifier's checks so reviewers can see which claims are NOT inspected (iss, nbf, sub) and why no tests are minted for those. Address (3) — new DIRECT_QUERY_SECURITY.md: - Single source of truth for the signing-key contract. - Key-flow ASCII diagram showing the four cluster consumers of pl-cluster-secrets/jwt-signing-key. - Threat-model table: what the key protects (7 rows: unauth call, wrong key, expired, alg:none, wrong aud, tampered, wrong scheme) and what it doesn't (6 rows: key compromise, replay within window, channel confidentiality, PxL-level authz, multi-tenant isolation, NetworkPolicy). - Tampering-scenarios table cross-references each unit test by name. - Rotation contract (no overlap window today; tracked as a follow-up). - Logging discipline: signing key MUST NEVER hit stderr. - Cross-references to all the code anchors (manager.cc:60/:140/:423, pem_manager.cc:39/:115, direct_query_server.cc:133, pem_daemonset.yaml). Address (4) — direct_query_server_test.cc: - Multi-paragraph header comment block above the FailSoft_* tests states the contract: each side OPTIONAL with respect to the other. - Direction (local → broker fails) is implemented + tested via the fixture's broker-free construction. - Direction (broker → local fails) is RED today and explicitly tracked in the SKIP message + DIRECT_QUERY_SECURITY.md follow-up note. Surfacing it needs either a MaybeStartDirectQueryServer hoist before Stirling startup, or a broker-optional Manager mode flag. Both are out of scope for #29; the placeholder ensures any future refactor has a target to flip from SKIP to PASS. All tests green (1 binary, ~30 cases): bazel test //src/vizier/services/agent/pem:direct_query_server_test arc lint --output summary clean on all three changed files.
Author
|
Review for claude-build-agent:
Kind regard, your human user (I am not the pixie-agent) |
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 5, 2026
…x#29) User review on PR #49 — 7 items, addressing the security-emphasized ones in this commit; benchmark is filed as a follow-up SKIP in test code. 1. Compile-time disable (highest priority). - New bazel config_setting :direct_query_disabled in pem/BUILD.bazel selecting `defines = ["PX_PEM_DIRECT_QUERY_DISABLED"]` for cc_library when invoked with `--define=PX_PEM_DIRECT_QUERY=disabled`. - direct_query_server.cc wraps its entire feature-bearing body (JWT verifier, Carnot driver, drain loop) in `#ifndef PX_PEM_DIRECT_QUERY_DISABLED`. The `#else` block provides stub `AuthenticateRequest` / `DirectQueryServer::ExecuteScript` definitions that return UNAUTHENTICATED / UNIMPLEMENTED so the class still resolves at link time but no feature code lives in the binary. Stdlib + boringssl + rapidjson + absl includes stay OUTSIDE the #ifndef so cpplint's IWYU scan (which doesn't follow preprocessor branches) doesn't false-flag every type as missing an include. - pem_manager.cc wraps the three flag DEFINEs (direct_query_enabled, direct_query_port, direct_query_jwt_signing_key) + the DECLARE_string(jwt_signing_key) in the same `#ifndef`, and MaybeStartDirectQueryServer early-returns Status::OK with a log line when disabled. The runtime flags do not exist in this build's gflags registry — passing them on the CLI errors with "unknown flag". 2. Feature-toggle 100%-effective tests. New TEST_F cases under PX_PEM_DIRECT_QUERY_DISABLED guard: CompiledOut_ValidToken_StillUnauthenticated — even a freshly signed-by-the-cluster JWT cannot re-enable the feature in a disabled build. CompiledOut_NoToken_Unauthenticated — same for no token. Plus the default-build documentary book-end ToggleContract_DocumentBothLevels. 3. Auth README sections — DIRECT_QUERY_SECURITY.md. "Client authentication — how to integrate" — 4-step contract for any consumer (canonical client is dx_daemon's pxbroker.go): mint with pl-cluster-secrets/jwt-signing-key via the cluster mint helpers, claim shape, gRPC metadata, per-call mint when fan-out > 30s. "Discouraged practices" — 8-row table with WHY for each: long-lived JWTs, hard-coding the key, non-Secret key sources, logging tokens, sharing tokens, leaving test-only key paths in production, cloud-to-direct-query routing, raw header values. "Disabling the feature" — full runtime vs compile-time matrix, each step's effect on the binary footprint, the cleanup semantics for an in-flight rolling update. "Failure modes — what each auth failure looks like to a client" — 8-row gRPC-status table for operators. 4. Apples-to-apples benchmark — RED SKIP placeholder Benchmark_PemDirect_Vs_BrokerPath_RedPlaceholder names the follow-up in code so the gap is greppable. Soak data on pemdq5 measured pemdirect ~43.5s/q vs broker ~27s/q (dominant factor: second Carnot exec). Proper bench needs a live cluster + per- call latency histogram + auth/compile/exec/drain breakdown — not a gtest. Tracked in DIRECT_QUERY_SECURITY.md follow-ups. Verification: - bazel test //src/vizier/services/agent/pem:direct_query_server_test (default build) — green. - bazel build //src/vizier/services/agent/pem:cc_library --define=PX_PEM_DIRECT_QUERY=disabled (compile-out build) — green; proves direct_query_server.cc + pem_manager.cc compile cleanly with the feature bytes excluded. - arc lint clean on all 5 changed files.
Author
|
please address the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/agent/pem/DIRECT_QUERY_SECURITY.md`:
- Around line 1-332: The doc's cross-reference to the DEFINE location is out of
date: update the mention of pem_manager.cc:39 to the current line
(pem_manager.cc:47) where FLAGS_direct_query_jwt_signing_key's DEFINE_string now
lives; search for the text "pem_manager.cc:39" in DIRECT_QUERY_SECURITY.md and
replace it with "pem_manager.cc:47" and run a quick grep to ensure no other
stale numeric references to pem_manager.cc remain, keeping the reference text
around the symbol FLAGS_direct_query_jwt_signing_key unchanged.
🪄 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: 4486db87-17b2-44c9-9f62-57db22524ab2
📒 Files selected for processing (6)
src/vizier/services/agent/pem/BUILD.bazelsrc/vizier/services/agent/pem/DIRECT_QUERY_SECURITY.mdsrc/vizier/services/agent/pem/direct_query_server.ccsrc/vizier/services/agent/pem/direct_query_server.hsrc/vizier/services/agent/pem/direct_query_server_test.ccsrc/vizier/services/agent/pem/pem_manager.cc
ConstanzeTU
pushed a commit
that referenced
this pull request
Jun 7, 2026
Advances entlein/dx pointer ee97e40 -> d5dcf67 so the submodule carries the 20 BUILD.bazel files the vizier_release pipeline needs to build //src/vizier/services/dx:dx_daemon_image. Built and verified end-to-end on this VM: bazel build //src/vizier/services/dx:dx_daemon_image --config=clang -> bazel-bin/src/vizier/services/dx/dx_daemon_image-layer.tar GREEN Combined with the pixie-side wiring already on this branch: - k8s/vizier/BUILD.bazel VIZIER_IMAGE_TO_LABEL entry (4408de8) - skaffold/skaffold_vizier.yaml artifact (4408de8) - pxapi.WithDirectTLSSkipVerify (06522e0, cherry-pick from #49) …an annotated release/vizier/v0.14.19-<suffix> tag now publishes ghcr.io/k8sstormcenter/vizier-dx_daemon_image:0.14.19-<suffix>-x86_64 in the same 9-image vizier bundle as kelvin/metadata/PEM/AE.
entlein
changed the title
dx-agent's kickoff flagged likely include/dep nits — confirmed two plus a -Wunused-private-field nit that surfaced from -Werror, plus a clang-format / IWYU sweep on the three stub files. BUILD.bazel 1. //src/common/testing:cc_library — duplicate label (pl_cc_test auto-injects gtest/gmock). Removed; mirrors tracepoint_manager_test. 2. //src/carnot:cc_library — not visible to PEM (default_visibility is //src/carnot:__subpackages__ + //src/experimental:__subpackages__, which is how standalone_pem reaches it but not us). Switched to //src/carnot:carnot — the public header target, explicitly opened to //src/vizier/services/agent:__subpackages__. Sub-deps for the real exec path (engine_state, planner/compiler) land in Step 2. direct_query_server.cc 3. -Wunused-private-field on carnot_ + engine_state_ — stub holds the pointers for the Step 2 wiring but doesn't touch them yet, and clang-15 + -Werror rejects. Added (void) casts inside the UNIMPLEMENTED body; same pattern as the existing (void)writer. direct_query_server.h 4. <utility> added for std::move (build/include_what_you_use warning). Plus auto-applied: - clang-format on .cc/.h/_test.cc (broke long status strings). - Trailing-whitespace strip on DIRECT_QUERY_CONTRACT.md L84. RED state captured: bazel test //src/vizier/services/agent/pem:direct_query_server_test 3 PASS — NoToken / WrongKey / Expired → UNAUTHENTICATED (fail-closed stub already gets these for the right reason). 1 FAIL — ValidToken_Mutation_Unimplemented: placeholder MakeBearerToken fails auth before the mutation branch fires. Step 1's real JWT mint + verify unlocks this. 2 SKIP — ValidToken_TrivialQuery_StreamsRows (Step 2) and PerPodFilter_MetadataConnected (Step 3). Next: Step 1 — port manager.cc:423 jwt::jwt_object HS256 mint pattern into both MakeBearerToken (test) and AuthenticateRequest (server), using jwt::decode against jwt_signing_key.
Server-side (AuthenticateRequest)
- Extracts the "authorization" header from ServerContext metadata; gRPC
lowercases keys but not values, and manager.cc:440 mints with a
lowercase "bearer " prefix while RFC 6750 calls for "Bearer " — we
accept both.
- Manually parses <header>.<payload>.<signature>:
* verifies "alg":"HS256" in the decoded header (refuses an "alg":"none"
forgery at the door),
* recomputes HMAC-SHA256 over <header>.<payload> with the signing key
using BoringSSL's HMAC(EVP_sha256(), …) and constant-time-compares
against the base64url-decoded signature,
* validates aud == "vizier" and exp > now.
- All failure paths collapse to UNAUTHENTICATED on the wire (no claim-
level detail leaked to peers); VLOG(1) keeps the diagnostic.
Why not jwt::decode for verify
Cpp_jwt's HMACSign<>::verify calls BIO_f_base64() out of BoringSSL's
src/decrepit/bio/base64_bio.c — that file isn't in @boringssl//:crypto
on this fork, and decrepit/ isn't exposed as its own bazel package.
Two unblock options: (a) patch boringssl.patch to add a :decrepit
target — fork-level + invasive, or (b) inline the verify ourselves
with native BoringSSL HMAC — ~150 LoC, no patch, what's done here.
Mint side still uses cpp_jwt (one-line jwt::jwt_object); the mint
path never touches BIO_f_base64.
Test-side mint (MakeBearerToken)
Mirrors GenerateServiceToken in src/vizier/services/agent/shared/manager
/manager.cc:423-440 — HS256, iss=PL, aud=vizier, iat/nbf/exp,
sub=service. kValid: signed with `signing_key`, exp +60s; kWrongKey:
caller passes the wrong key so the HMAC's against the wrong secret;
kExpired: signed with `signing_key`, exp -60s.
BUILD.bazel
- + @boringssl//:crypto (BoringSSL HMAC + EVP_sha256)
- + @com_github_tencent_rapidjson//:rapidjson (claim parsing)
- cpp_jwt now only on the test target (for MakeBearerToken).
Result
bazel test //src/vizier/services/agent/pem:direct_query_server_test
→ 4 PASS for the right reason:
NoToken / WrongKey / Expired → UNAUTHENTICATED (verifier really
rejects rather than the stub failing-closed),
ValidToken_Mutation_Unimplemented → auth passes, mutation guard fires.
→ 2 SKIP: ValidToken_TrivialQuery_StreamsRows (Step 2),
PerPodFilter_MetadataConnected (Step 3).
…#29) Structural scaffolding for the ExecuteScript port from standalone_pem/vizier_server.h. The dx-agent's contract says reuse the PEM's already-running Carnot + EngineState — that's the production wiring landing in Step 4. For the unit test, we'll build a CarnotTest-style fixture (table_store + http_events seed + Carnot configured with a LocalGRPCResultSinkServer) in Step 2b. This commit just adds the missing 4th ctor parameter — the LocalGRPCResultSinkServer the server reads results from after Carnot::ExecuteQuery returns. Forward-declared in the header (test target doesn't need to pull the impl include yet); the auth-only tests pass nullptr. Mutation/exec paths still UNIMPLEMENTED — Step 2b ports the real compile + execute + drain + stream sequence. Test stays at 4 PASS + 2 SKIP (no behavior change).
Real port of standalone_pem/vizier_server.h:60-181 against the
DirectQueryServer ctor's live Carnot + EngineState + LocalGRPCResultSinkServer.
ExecuteScript impl (direct_query_server.cc)
- After auth + mutation guard: compile via
engine_state_->CreateLocalExecutionCompilerState(0) → Compiler().Compile.
- Walk the plan once and write one meta_data-only ExecuteScriptResponse
per GRPC_SINK_OPERATOR sink so the client sees column types up front
(same shape standalone_pem produces, so dx's pxapi consumer reads it).
- Reset the sink → carnot_->ExecuteQuery(query, query_id, CurrentTimeNS)
(synchronous; matches carnot_test.cc:110 and standalone_pem:176) →
drain result_server_->raw_query_results() into ExecuteScriptResponse.
- Per-chunk: copy table_id/num_rows/eow/eos; column data marshal is a
TODO documented for Step 4's live e2e (carnotpb RowBatchData ↔ vizierpb
RowBatchData column variants is per-type translation that the schema
responses above already cover for client consumers that only read meta).
- carnot/engine/sink null at ExecuteScript time → FAILED_PRECONDITION
rather than crash. Auth tests still pass nullptr; the exec tests
build the real fixture.
Test (direct_query_server_test.cc)
- DirectQueryServerExecTest fixture builds a CarnotTest-style stack:
TableStore + LocalGRPCResultSinkServer + udf::Registry +
funcs::RegisterFuncsOrDie + Carnot::Create with the sink stub
generator wired through ClientsConfig. http_events table seeded
inline (same 5-column subset as CarnotTestUtils::HTTPEventsTable —
empty rows are fine; the trivial query just enumerates the schema).
- ValidToken_TrivialQuery_StreamsRows flipped from GTEST_SKIP to a
real assertion: ExecuteScript returns OK and streams ≥1 response.
Visibility opened on three carnot subtargets for the PEM test fixture
(same pattern //src/experimental/standalone_pem already uses for the
broader set):
- //src/carnot:cc_library
- //src/carnot/exec:cc_library (LocalGRPCResultSinkServer header
promoted from globbed-impl-only to hdrs)
- //src/carnot/exec:test_utils
- //src/carnot/udf default_visibility
all add //src/vizier/services/agent/pem:__pkg__.
Result
bazel test //src/vizier/services/agent/pem:direct_query_server_test
→ 5 PASS:
NoToken / WrongKey / Expired → UNAUTHENTICATED
ValidToken_Mutation → UNIMPLEMENTED
ValidToken_TrivialQuery_StreamsRows → OK + ≥1 streamed response (new)
→ 1 SKIP: PerPodFilter_MetadataConnected (Step 3)
Three gflags for the direct-query endpoint, each environ-fallback so operators can opt in via either flag or env var (matching the rest of the PEM's flag style): --direct_query_enabled / PL_PEM_DIRECT_QUERY_ENABLED (default: false) --direct_query_port / PL_PEM_DIRECT_QUERY_PORT (default: 50305) --direct_query_jwt_signing_key / PL_JWT_SIGNING_KEY (default: "") PL_JWT_SIGNING_KEY intentionally shares the existing env name with manager.cc's outgoing mint path (DEFINE_string(jwt_signing_key)) — one secret covers both directions, no new ConfigMap/Secret bind required. Default false → flag off → existing PEM deployments byte-identical. The pem_manager-side construction (which has access to the live Carnot + EngineState) lands in the next commit; this commit is the flag surface + DIRECT_QUERY_CONTRACT.md's documented env names landing in the binary.
Upstream's vizier_release.yaml uses oracle-16cpu-64gb-x86-64 and oracle-8cpu-32gb-x86-64 runs-on labels — neither exists on this k8sstormcenter/pixie fork's self-hosted pool, so tag-triggered release builds would queue forever (which is exactly what the closed PR #48 flagged + the user explicitly approved fixing in its closing comment: "Nice catch on the runner label, though!"). Same single substitution PR #48 used: both labels → oracle-vm-16cpu-64gb-x86-64 (the fork's actual VM label, already used by perf_clickhouse.yaml and perf_soc_attack.yaml). Lands on this branch as Step 6 prep — without it, the release/vizier/v... tag that builds + pushes vizier-pem_image (including the direct-query endpoint) never gets a runner.
…hal (#29) Two live-e2e-blockers dx-agent caught reviewing my Step 1+2b post-mortem: 1. aud is a JSON ARRAY, not a string. Pixie's go mint (src/shared/services/utils/jwt.go:46) builds Audience([]string{...}) → lestrrat-go/jwx serializes as "aud":["vizier"]. My verifier's literal string compare would have UNAUTHENTICATED every live call while the unit tests stayed green (they minted a string-form aud). Verifier now accepts both forms per RFC 7519 §4.1.3; the test mint is switched to the array form so the unit guards the regression. 2. Per-row column data is required, not a TODO. dx's HandleRecord reads r.Data per Column to build rows; schema-only responses → empty rowset → no verdict. Wired now via a wire-format round-trip: carnotpb::RowBatchData and vizierpb::RowBatchData share field numbers 1-4 (cols/num_rows/eow/eos) AND the embedded Column message has identical oneof layout (boolean/int64/uint128/time64ns/ float64/string with matching field numbers). So we SerializeToString the carnot RowBatchData, ParseFromString into the vizier RowBatchData, then set vizier-only table_id (field 5) explicitly from query_result().table_name(). Tested locally: same unit test goes green; per-cell data marshaling lands as a byproduct. Fallback path emits the metadata-only frame if the roundtrip ever fails on a malformed payload. Test: bazel test //src/vizier/services/agent/pem:direct_query_server_test → still 5 PASS + 1 SKIP, now exercising aud-array mint + per-row marshal. Next: re-tag release/vizier/v0.14.19-pemdq2 once the live image with these fixes is what dx-agent should point DX_BENCH=pemdirect at.
…29) dx-agent ran the source tree on the pemdq2 image and called the correct shot: flags + DirectQueryServer class were present + unit- tested green, but nothing was actually constructing the gRPC server + binding the listener, so :50305 stayed dark even with the flag on. This wires PEMManager to do both. PostRegisterHookImpl, when FLAGS_direct_query_enabled=true: - LocalGRPCResultSinkServer for node-local result chunks - dedicated carnot::Carnot sharing table_store (no duplicate data plane) and registering mds_manager()'s CurrentAgentMetadataState callback (so per-pod filters resolve the same way the live Carnot does) - DirectQueryServer constructed with both + the live engine_state - grpc::ServerBuilder, InsecureServerCredentials (dx confirmed pxapi sends the bearer JWT as plain metadata; no TLS required — matches kelvin/standalone_pem deploy), AddListeningPort on 0.0.0.0:FLAGS_direct_query_port (50305 default), BuildAndStart. - Returns FAILED_PRECONDITION if signing key is empty or BuildAndStart returns null. StopImpl: Shutdown the gRPC server, reset all four owners. Contract deviation The contract said "reuse the live Carnot — don't stand up a second engine." This commit stands up a second Carnot but shares table_store and the agent metadata callback. The live PEM Carnot binds its ResultSinkStubGenerator to Kelvin's address at construction time; redirecting that per-call would touch core/manager.cc. A second Carnot that shares the heavy data plane (table_store) + metadata (via the callback) is the smallest delta that gives the direct-query path a node-local sink. The engine itself is small; the duplicate is just the planner/exec state, not the rows. Will reflect this in the contract md when dx-agent confirms the live e2e works. BUILD.bazel - + //src/carnot/funcs:cc_library (RegisterFuncsOrDie) - + //src/carnot/udf:cc_library (udf::Registry) Test - Local: cc_library + pem_image both build clean. - Flag-off path: all four members stay nullptr from the early-return, byte-identical PEM behavior (verified by reading the new code path — no allocation, no listener). - Flag-on path: ttl.sh/vizier-pem-dq29-pemdq3:24h, digest sha256:95de8a575054d67502cb2cb83013f63a0e58a0c073095c6589bcbca6b5abe0b8 pushed for dx-agent's live e2e validation. Next: cut release/vizier/v0.14.19-pemdq3 for the canonical multi-arch ghcr publish to follow once dx confirms the live path.
dx-agent caught on pemdq3 that every query failed mid-stream with "unimplemented type : internal error". Root cause: pxapi/results.go:142-143 returns ErrInternalUnImplementedType when an ExecuteScriptResponse has neither meta_data, data.batch, data.encrypted_batch, nor data.execution_stats set; my drainSinkAndStream was writing query_id-only frames for any TransferResultChunkRequest that wasn't query_result/execution_error (carnot's sink also emits initiate_conn + execution_and_timing_info). Fix: - Track has_payload across the three branches and `continue` past chunks with nothing to send (e.g. initiate_conn). - Map execution_and_timing_info.execution_stats → QueryData.execution_stats via wire-format roundtrip (carnotpb and vizierpb QueryExecutionStats share field numbers 1 timing / 2 bytes_processed / 3 records_processed; QueryTimingInfo shares 1 execution_time_ns / 2 compilation_time_ns). Collateral: move direct_query_* flag DEFINEs from pem_main.cc into pem_manager.cc. The flags are consumed by pem_manager.cc inside cc_library; defining them in the binary-only translation unit left the test binary (which links cc_library but not pem_main.cc) with undefined gflags symbols. The pem binary still picks them up transitively via cc_library.
pemdq4 (9ce6fbd) crashloop'd the live PEM with exit=1 and `:50305` never bound; --previous logs were lost to the rollback so the exact line is unknown. Make MaybeStartDirectQueryServer **fail-soft** so any future init failure cannot take the data plane down: - Every error path logs and returns Status::OK(); PostRegisterHookImpl no longer propagates a direct-query failure to the base manager PX_CHECK_OK. dx_daemon sees a harmless "connection refused" on :50305. - try/catch around the whole setup catches std::exception + any throw. - LOG(INFO) breadcrumb at each step (1/6 sink → 6/6 BuildAndStart). A future crashloop's stderr will name the exact failing step. Direct-query is OPTIONAL on the PEM (default-OFF flag); a setup failure must not be a data-plane outage. This is the safety net dx-agent asked for after pemdq4 degraded the broker path.
dx-agent observed the stock fork 0.14.17 PEM in CrashLoopBackOff (23 restarts over hours) with: libc++abi: terminating due to uncaught exception of type jwt::SigningError: key not provided Root cause: src/vizier/services/agent/shared/manager/manager.cc:434 calls `obj.secret(FLAGS_jwt_signing_key); obj.signature();` in GenerateServiceToken. cpp_jwt's signature() throws SigningError when the secret is empty. The throw lands inside the first outgoing AddServiceTokenToClientContext call — typically the PEM's first query execution against Kelvin — and there is no surrounding catch, so the process aborts mid-stream with libc++abi terminate. Fix: fail fast in Manager::Init when FLAGS_jwt_signing_key is empty, returning a clean InvalidArgument Status with a precise message. The agent now refuses to start instead of running for an indeterminate period and then crashing on the first query. Lives in the shared base so it covers Kelvin + PEM both. Kelvin always has the key wired via pl-cluster-secrets, so this changes no production behavior; it just turns a delayed uncaught throw into a fast clean exit if a deployment ever omits the key (as the live PEM's pre-#29 daemonset apparently did on some clusters). Reviewed under direct-query soak (PR #49 / entlein/dx#29) where the direct-query path's verify uses FLAGS_direct_query_jwt_signing_key, not FLAGS_jwt_signing_key — same env var (PL_JWT_SIGNING_KEY) feeds both, so a single secret continues to cover both auth directions.
Three PR-checks were failing: 1. run-container-lint (cfmt) — pem_manager.cc had a two-line LOG that clang-format wants on one line. `arc lint --apply-patches` autofixed the step 6/6 LOG(INFO) wrap. No behavioral change. 2. run-genfiles — same buildifier reorder of src/stirling/source_connectors/socket_tracer/testing/container_images/BUILD.bazel that PR #47 had earlier (`make go-setup` named-arg alphabetization inside go_container_libraries calls). Triggered by the same shared genfile that flips between branches; identical fix to PR #47's a9ef878. 3. lint-pr-description — handled separately by editing the PR body to the Summary:/Test Plan:/Type of change: literal-key format the linter (tools/linters/pr_description_linter.sh) requires (was markdown `## Summary` headers, which the script's `^Summary: .+` regex doesn't match). No commit needed for that one.
…ntlein/dx#29) User asks on PR #49: 1. CodeRabbit r3359029109: avoid split-brain between FLAGS_direct_query_jwt_signing_key and FLAGS_jwt_signing_key. 2. Extend direct_query_server_test.cc with broader query coverage + robustness. 3. Full README on the signing-key security contract + explicit tampering scenarios with tests. 4. Name the bidirectional fail-soft contract between direct-query and broker paths. Address (1) — pem_manager.cc:39, :115: - Reword the DEFINE_string doc on FLAGS_direct_query_jwt_signing_key so it's explicitly optional; falls back to FLAGS_jwt_signing_key. - DECLARE_string(jwt_signing_key) at the top of pem_manager.cc (the DEFINE_string lives in shared/manager/manager.cc). - In MaybeStartDirectQueryServer, compute effective_signing_key as FLAGS_direct_query_jwt_signing_key.empty() ? FLAGS_jwt_signing_key : FLAGS_direct_query_jwt_signing_key and pass that to the DirectQueryServer ctor. Empty-effective-key still fails soft with LOG(ERROR) and Status::OK(). - Manager::Init's existing guard (refuse to start with empty FLAGS_jwt_signing_key) means the fallback is a no-op in production (both come from the same PL_JWT_SIGNING_KEY env), but it closes the CLI-override-of-one-flag-only hole CodeRabbit flagged. Address (2) + (3) — direct_query_server_test.cc: ~25 new TEST_F cases organised in four blocks: JWT robustness (8): GarbageBearer, AlgNoneToken, ValidToken_ AudAsString_Authenticated, WrongAud, MissingAud, MissingExp, BearerEmptyToken, ValidToken_LowercaseBearerPrefix_Authenticated, WrongAuthScheme. Tampering (6): TamperedSignatureByte, TamperedPayloadByte, TamperedHeaderByte, TruncatedToken, ConcatenatedTokens, AlgConfusion_HS384. Routine queries (4 on exec fixture + dns_events): ColumnProjection, MultiTableDisplay, Mutation_Unimplemented (with real Carnot). PxL robustness (3 on exec): EmptyPxL_Errors, MalformedPxL_Errors, NonexistentTable_Errors. Concurrency / reuse (2): ConcurrentQueries_AllSucceed, SequentialQueries_AllSucceed. Fail-soft contract documentation (2): DirectQueryDecoupledFromBroker (PASS — proves the local code path has no broker dep), BrokerFailureToleratedByDirectQuery (RED, SKIP — names the bidirectional contract gap in code). New helpers FlipNthChar / SegmentIndex enable byte-level tampering without segment-boundary realignment. TokenKind enum extended with kAudAsString / kMissingAud / kWrongAud / kMissingExp / kAlgNone for named token shapes; comment block on the enum lists the verifier's checks so reviewers can see which claims are NOT inspected (iss, nbf, sub) and why no tests are minted for those. Address (3) — new DIRECT_QUERY_SECURITY.md: - Single source of truth for the signing-key contract. - Key-flow ASCII diagram showing the four cluster consumers of pl-cluster-secrets/jwt-signing-key. - Threat-model table: what the key protects (7 rows: unauth call, wrong key, expired, alg:none, wrong aud, tampered, wrong scheme) and what it doesn't (6 rows: key compromise, replay within window, channel confidentiality, PxL-level authz, multi-tenant isolation, NetworkPolicy). - Tampering-scenarios table cross-references each unit test by name. - Rotation contract (no overlap window today; tracked as a follow-up). - Logging discipline: signing key MUST NEVER hit stderr. - Cross-references to all the code anchors (manager.cc:60/:140/:423, pem_manager.cc:39/:115, direct_query_server.cc:133, pem_daemonset.yaml). Address (4) — direct_query_server_test.cc: - Multi-paragraph header comment block above the FailSoft_* tests states the contract: each side OPTIONAL with respect to the other. - Direction (local → broker fails) is implemented + tested via the fixture's broker-free construction. - Direction (broker → local fails) is RED today and explicitly tracked in the SKIP message + DIRECT_QUERY_SECURITY.md follow-up note. Surfacing it needs either a MaybeStartDirectQueryServer hoist before Stirling startup, or a broker-optional Manager mode flag. Both are out of scope for #29; the placeholder ensures any future refactor has a target to flip from SKIP to PASS. All tests green (1 binary, ~30 cases): bazel test //src/vizier/services/agent/pem:direct_query_server_test arc lint --output summary clean on all three changed files.
…x#29) User review on PR #49 — 7 items, addressing the security-emphasized ones in this commit; benchmark is filed as a follow-up SKIP in test code. 1. Compile-time disable (highest priority). - New bazel config_setting :direct_query_disabled in pem/BUILD.bazel selecting `defines = ["PX_PEM_DIRECT_QUERY_DISABLED"]` for cc_library when invoked with `--define=PX_PEM_DIRECT_QUERY=disabled`. - direct_query_server.cc wraps its entire feature-bearing body (JWT verifier, Carnot driver, drain loop) in `#ifndef PX_PEM_DIRECT_QUERY_DISABLED`. The `#else` block provides stub `AuthenticateRequest` / `DirectQueryServer::ExecuteScript` definitions that return UNAUTHENTICATED / UNIMPLEMENTED so the class still resolves at link time but no feature code lives in the binary. Stdlib + boringssl + rapidjson + absl includes stay OUTSIDE the #ifndef so cpplint's IWYU scan (which doesn't follow preprocessor branches) doesn't false-flag every type as missing an include. - pem_manager.cc wraps the three flag DEFINEs (direct_query_enabled, direct_query_port, direct_query_jwt_signing_key) + the DECLARE_string(jwt_signing_key) in the same `#ifndef`, and MaybeStartDirectQueryServer early-returns Status::OK with a log line when disabled. The runtime flags do not exist in this build's gflags registry — passing them on the CLI errors with "unknown flag". 2. Feature-toggle 100%-effective tests. New TEST_F cases under PX_PEM_DIRECT_QUERY_DISABLED guard: CompiledOut_ValidToken_StillUnauthenticated — even a freshly signed-by-the-cluster JWT cannot re-enable the feature in a disabled build. CompiledOut_NoToken_Unauthenticated — same for no token. Plus the default-build documentary book-end ToggleContract_DocumentBothLevels. 3. Auth README sections — DIRECT_QUERY_SECURITY.md. "Client authentication — how to integrate" — 4-step contract for any consumer (canonical client is dx_daemon's pxbroker.go): mint with pl-cluster-secrets/jwt-signing-key via the cluster mint helpers, claim shape, gRPC metadata, per-call mint when fan-out > 30s. "Discouraged practices" — 8-row table with WHY for each: long-lived JWTs, hard-coding the key, non-Secret key sources, logging tokens, sharing tokens, leaving test-only key paths in production, cloud-to-direct-query routing, raw header values. "Disabling the feature" — full runtime vs compile-time matrix, each step's effect on the binary footprint, the cleanup semantics for an in-flight rolling update. "Failure modes — what each auth failure looks like to a client" — 8-row gRPC-status table for operators. 4. Apples-to-apples benchmark — RED SKIP placeholder Benchmark_PemDirect_Vs_BrokerPath_RedPlaceholder names the follow-up in code so the gap is greppable. Soak data on pemdq5 measured pemdirect ~43.5s/q vs broker ~27s/q (dominant factor: second Carnot exec). Proper bench needs a live cluster + per- call latency histogram + auth/compile/exec/drain breakdown — not a gtest. Tracked in DIRECT_QUERY_SECURITY.md follow-ups. Verification: - bazel test //src/vizier/services/agent/pem:direct_query_server_test (default build) — green. - bazel build //src/vizier/services/agent/pem:cc_library --define=PX_PEM_DIRECT_QUERY=disabled (compile-out build) — green; proves direct_query_server.cc + pem_manager.cc compile cleanly with the feature bytes excluded. - arc lint clean on all 5 changed files.
Concurrent ExecuteScript calls share the LocalGRPCResultSinkServer's
accumulator (ResetQueryResults / ExecuteQuery / raw_query_results all
operate on the same mutable state). Without serialization, one caller's
ResetQueryResults could wipe another caller's chunks mid-drain, or two
callers' chunks could interleave in a single sink — the previous
ConcurrentQueries_AllSucceed test passed only because the scheduling
happened not to hit the race in practice.
Add a per-instance absl::Mutex `exec_mu_` on DirectQueryServer; hold
from before ResetQueryResults until after drainSinkAndStream returns.
Per-instance (not file-scope) so distinct DirectQueryServer instances
in tests don't over-serialize against each other. Standalone_pem
makes the same single-threaded assumption; dx_daemon doesn't fan out
per-PEM today, so contention is expected to be low. The
ConcurrentQueries_AllSucceed test continues to verify N parallel
callers all succeed under the lock.
direct_query_server.h: + absl::synchronization::mutex.h include +
mutable absl::Mutex exec_mu_ member.
direct_query_server.cc: + absl::MutexLock lk(&exec_mu_) before
ResetQueryResults; lock guards the full reset/execute/drain
critical section.
Both build modes still green:
bazel test //src/vizier/services/agent/pem:direct_query_server_test
bazel build //src/vizier/services/agent/pem:cc_library
--define=PX_PEM_DIRECT_QUERY=disabled
dx-agent flagged the insecure-credentials gap as blocking. The
direct-query listener was binding :50305 with
::grpc::InsecureServerCredentials(), so the JWT bearer + the PxL
body crossed the pod network in the clear. Any pod with network
reach to the PEM could capture a token and replay it within its
60-second exp window.
Fix: swap both Insecure* creds in MaybeStartDirectQueryServer to
SSL::DefaultGRPCServerCreds() (from src/vizier/services/agent/shared/
manager/ssl.h). That helper reuses the PEM's already-mounted
cluster TLS pair (PL_TLS_CA_CERT + PL_CLIENT_TLS_CERT +
PL_CLIENT_TLS_KEY in pem_daemonset.yaml — same env kelvin / metadata
/ broker use). Plaintext fallback only when an operator sets
PL_DISABLE_SSL=1, which is the cluster-wide dev/soak escape hatch
already documented for the other components — not a silent default.
Two call sites updated:
- server_config->grpc_server_creds — Carnot's internal sink server
config; not strictly needed (LocalGRPCResultSinkServer uses
InProcessChannel) but matches the cluster's TLS policy in case
a future caller swaps to a TCP channel.
- builder.AddListeningPort — the EXTERNAL :50305 listener; this
is the actual blocker fix.
DIRECT_QUERY_SECURITY.md: add a "Transport" section documenting
the TLS posture and the s_client/grpcurl validations to run on
the next soak; update the threat-model row on channel
confidentiality to reflect TLS-by-default.
Both build modes still green:
bazel test //src/vizier/services/agent/pem:direct_query_server_test
bazel build //src/vizier/services/agent/pem:cc_library
--define=PX_PEM_DIRECT_QUERY=disabled
dx-agent's pxbroker.go pemdirect path dials the PEM at the node's HOST_IP:50305. With direct-query now serving TLS (pem_manager.cc swap to SSL::DefaultGRPCServerCreds in 847409f), the bearer JWT rides an encrypted channel — but the PEM's TLS cert is the cluster service cert whose SAN is the DNS name (vizier-pem-svc.pl.svc.…), NOT the node IP. Chain+hostname verification therefore fails on the node-IP dial. Add WithDirectTLSSkipVerify() — sets disableTLSVerification=true so the existing Client.init() builds the TLS dial config with InsecureSkipVerify:true. The channel is encrypted; the cert is just not chain/hostname-verified. Same posture the broker path uses for in-cluster service-cert dials. Strictly more secure than WithDirectCredsInsecure (which builds a plaintext channel via insecure.NewCredentials) — JWTs no longer travel in the clear on the pod network. Full CA+hostname verify is future hardening (needs node-IP SANs on the PEM cert, or a CA-pool+skip-hostname verifier); tracked as a follow-up. Verified: bazel build //src/api/go/pxapi:pxapi green. arc lint clean. dx-agent will bump dx's go.mod to this commit + ship the pxbroker.go swap from WithDirectCredsInsecure to WithDirectTLSSkipVerify. Patch text was authored by dx-agent on the soak VM (cmd/dx-daemon go module wasn't available there); committing on their behalf so the dx side can pull it.
CodeRabbit r3357199175 — verifier previously only checked aud+exp, so any HS256 token signed with PL_JWT_SIGNING_KEY and aud=vizier authenticated (e.g., a kelvin-targeted token). Adds two claim checks matching what manager.cc::GenerateServiceToken emits: iss must equal "PL" sub must equal "service" Wrong-value and missing-claim paths each get a TEST_F. Existing positive fixtures already mint these claims so they stay green (verified locally: 37 tests, 34 pass + 3 pre-existing skips). CodeRabbit r3364977606 — DIRECT_QUERY_SECURITY.md still cited stale line numbers from earlier iterations. Updated: pem_manager.cc:39 -> :47 (FLAGS_direct_query_jwt_signing_key) pem_manager.cc:115 -> :132 (MaybeStartDirectQueryServer) direct_query_server.cc:133 -> :151 (verifyHs256Jwt) manager.cc:423 -> :440 (GenerateServiceToken)
verifyHs256Jwt required sub=="service", but pixie service tokens (GenerateJWTForService, claims.go) set sub=<serviceID> (e.g. "dx") and carry "service" in the Scopes claim. Every real in-cluster caller (dx-daemon) was thus rejected UNAUTHENTICATED "invalid bearer token" (live: pemdq9 + dx rc13; the broker accepted the same token). The unit test masked it by minting sub="service". Fix: require the "service" scope (Scopes claim, comma-joined); stop asserting the subject — matching canonical pixie verify (jwt.go ParseToken: signature+audience). Test mints realistic tokens (sub=serviceID, Scopes="service") with kWrongScope/kMissingScope negatives.
entlein
force-pushed
the
entlein/pem-direct-query
branch
from
June 21, 2026 17:14
e075700 to
c357699
Compare
entlein
added a commit
that referenced
this pull request
Jun 21, 2026
…bit items User review 4536971862: #2 passthrough/reconcile_test.go — strengthened with two new tests that exercise the FULL chain (loop → real sink.ClickHouseHTTP → httptest CH endpoint → reconcile recorder). TestTick_ReconcileCatchesCHSilentDrop mimics CH's X-ClickHouse-Summary silent-drop shape (200 OK, written_rows=0) and asserts the loop records WroteCount=0 with a silent-drop attribution — the exact R6 (sink-layer loss) regression the instrument exists to detect. TestTick_ReconcileAttributesCHFailureCorrectly covers the 500-response branch. The pre-existing in-process-fake test stays as the wiring check. #4 pixie/pixie.go — added a long comment justifying the API-key auth choice. pixie.Client targets the Pixie CLOUD (cloudpb's PluginService), whose auth interceptor accepts pixie-api-key and rejects JWT service tokens (those are for INSIDE-cluster vizier services). The pixieapi.Adapter that talks to vizier directly already uses JWT via jwtutils.GenerateJWTForService — same pattern as cloud_connector/vizhealth/checker.go:111. So the split is intentional; flipping pixie.go to JWT would break cloud auth, not improve it. #6 pxl/queryfor — hardened escapePxL so a raw \n, \r, \t or NUL byte in Target.Pod/Target.Namespace can't terminate the PxL string literal and inject a new statement. Added TestQueryFor_RejectsInjectionInTargetFields driving QueryFor with 7 adversarial pod/namespace shapes (newline, single-quote-only, CR, backslash-escape-of-escape, NUL, tab) plus the regex_match fallback path, asserting the output line count and every statement's leading token. Extends existing TestEscapePxL_TableDriven with the new escape mappings. CodeRabbit oversights (verified each against current code): CR r3379377432 (main.go) — wrapped the control-surface listener in http.Server with Read/ReadHeader/Write/Idle timeouts so a slow client can't pin a goroutine indefinitely. CR r3379377607 (pixieapi.go) — switched direct-mode dial from pxapi.WithDisableTLSVerification (env + addr-gated, brittle) to pxapi.WithDirectTLSSkipVerify (added in PR #49 b523ce3 for the same node-IP-dial scenario). Removed the cluster.local + PX_DISABLE_TLS precondition in NewDirect; the always-skip semantics match the AE deployment shape. Refactored the obsolete env-gate test. CR r3379377645 (streaming/filter.go) — the deltaCh-close path returned without calling disarm(), leaking the timer's goroutine on shutdown. Now calls disarm() before return on chan-close. CR r3426923299 (sink/clickhouse.go Record) — capped Record at a 2s per-call context timeout. The 30s chhttp default was too long for the scanner/passthrough/controller hot paths that call Record inline; reconcile is best-effort by contract, so a stalled CH must not pin the pull loop. All 14 AE packages pass go test. arc lint: 0 Errors on PR-53 file scope. Signed-off-by: entlein <einentlein@gmail.com>
entlein
added a commit
that referenced
this pull request
Jun 21, 2026
) * adaptive_export: production AE — streaming export + write-integrity Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: ADAPTIVE_PASSTHROUGH firehose loop New env-gated background loop that runs the same PxL shape AE's anomaly-gated path uses, but with an empty Target (no ns/pod predicate) and over a configurable rolling window. Writes via the existing sink so the byte-shape of forensic_db rows is comparable between the PASSTHROUGH=1 phase (EVERYTHING) and the PASSTHROUGH=0 phase (AE-FILTER). One-shot A/B that yields the per-table capture fraction of the adaptive write path. - internal/passthrough/passthrough.go — Loop + Config; defaults to 30s window / 30s refresh / clickhouse.PixieTables() table list. - internal/passthrough/passthrough_test.go — 6 tests; the load-bearing one is TestLoop_EmitsEmptyTargetPxL (asserts neither df.namespace nor df.pod predicates appear in the emitted PxL). - cmd/main.go: ADAPTIVE_PASSTHROUGH + _WINDOW_SEC + _REFRESH_SEC env knobs. Adapter is constructed unconditionally when passthrough is on (joins the existing PushPixie / streaming construction path so the same pxapi grpc stream is reused). Loop is registered with the shutdown WaitGroup so SIGTERM waits for the in-flight tick. - cmd/BUILD.bazel: drop @px// load (other AE BUILD.bazel files use //bazel — sticking out as the only one with @px is a leftover from a prior gazelle run; align). Add passthrough dep. Signed-off-by: entlein <einentlein@gmail.com> * ci: dx-image workflow — build + publish dx-daemon to ghcr Stand-alone workflow that builds entlein/dx (private Active-Diagnosis Framework) into ghcr.io/k8sstormcenter/dx-daemon. Separates the dx image publish from the bazel-based vizier_release pipeline; the dx repo ships its own Dockerfile.dxd (Go cross-compile + distroless final stage) so it doesn't need to live as a submodule inside src/vizier/services/dx. Triggers: - tag push 'release/dx/v*' on this repo cuts a release build, image tag derived from the tag suffix (release/dx/v0.1.0 -> image tag 0.1.0). - workflow_dispatch lets us build any dx ref on demand with a custom tag (default: short sha of the resolved dx commit). Pulls dx via DX_ENTLEIN_PAT (already configured on the repo). Multi-arch build (linux/amd64, linux/arm64); Dockerfile.dxd cross-compiles in the native BUILDPLATFORM stage and the final stage is COPY-only, so target emulation isn't required. Signed-off-by: entlein <einentlein@gmail.com> * Revert ci: dx-image workflow — wrong repo The dx image build pipeline lives in entlein/dx itself (PR #53, branch feat/bazel-release): bazel-based with @px external pin to pixie's ae-prod tip, pushes to docker.io/entlein/dx-daemon on a release/dx/v* tag in the dx repo. The pixie-side buildx workflow this reverts duplicated that intent in the wrong repo + the wrong build system (docker buildx instead of bazel + pl_go_image macros) + the wrong registry (ghcr.io/k8sstormcenter instead of docker.io/entlein). Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: unit-normalize trigger watermark cursor + load-test affordances Fixes the silent-halt bug: the trigger gated on a RAW event_time high-water-mark, so a single anomaly in a larger unit (ms/ns) drove the watermark past all real seconds rows and AE stopped processing forever (data still on Pixie). Normalize event_time to canonical nanoseconds in the poll SELECT filter+order and in the in-memory/persisted cursor, boundary-dedup, and maxSeen (normalizeEventTimeNanos + chNormEventTimeNanos). Validated at the data layer: vs a poisoned watermark the raw filter returns 0 rows, the normalized filter recovers all 60. Also adds ADAPTIVE_PUSH_REFRESH_SEC (negative = single-shot pull) for the reproducible load-test harness, an in-package trigger unit test, and an e2e hermetic load test (mock PixieQuerier, exact rows+bytes). Signed-off-by: entlein <einentlein@gmail.com> * e2e_test/adaptive_export_loadtest: AE fixture-isolation load-test harness Consolidate the adaptive_export load-test harness under src/e2e_test/ (matching vzconn_loadtest / px_cluster conventions). Control-plane experiments (E1-E4, E6, E8 sustained) are proven exactly-reproducible on a live rig; the data-plane experiments (E5, E8 data-mode) are authored and pending live validation on a vizier-registered rig (status documented in README + FINDINGS_AND_BACKLOG). - harness/: shell + python (inject, exp_control, exp_e8, stats, ...) + lib helpers - fixtures/EXPERIMENTS.md: curated kubescape_logs data-set catalog + expected outputs - k8s/: isolated sinks + per-rep generator pod (no probes) - tools/loadgen/: cleanloadgen + httpsink (docker-built test tool; .bazelignore'd pending a bazel target — lib/pq is already vendored in the module) - FINDINGS_AND_BACKLOG.md: F8 watermark-poison bug + the fix + AE backlog The AE Go unit/e2e tests live with the service (internal/{trigger,e2e}). Signed-off-by: entlein <einentlein@gmail.com> * e2e_test/adaptive_export_loadtest: document AE implied contracts (C1-C14) + diagrams Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export_loadtest: C15 write-duration contract + DX-steering diagram; gen sustained-DNS mode C15 = AE must keep re-pulling+writing an active pod until t_end or DX stop (the contract DX steers on; last week's 'wrote then stopped' is its violation). Add DX-steering sequence diagram. Generator gains SUSTAIN_SEC (distinct-DNS trickle, a Pixie-traced protocol) + configurable SETTLE_PRE_MS warm-up for fresh-pod capture; harness wires GEN_SUSTAIN_SEC/GEN_SETTLE_MS. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export/trigger: update test SQL substrings for multiIf normalisation The 700821d trigger unit-normalisation wrapped event_time in a multiIf(...) inside both the WHERE filter and the ORDER BY. Three existing tests in watermark_test.go + one in clickhouse_test.go pinned the raw 'event_time >= N' substring and broke at HEAD. Update each test's expected substring to match the new normalized form (') >= <ns-scaled N>' — the closing paren of multiIf, then the value in canonical nanoseconds). Per-test ns-scaling: watermark_test.go:94 1744000000000000000 already ns -> unchanged watermark_test.go:125 InitialWatermark=42 < 1e10 sec -> * 1e9 watermark_test.go:156 InitialWatermark=7 < 1e10 sec -> * 1e9 watermark_test.go:297 event_time='5000' < 1e10 sec -> * 1e9 clickhouse_test.go:82 ref.T=1744..303e9 ns already ns -> unchanged go test ./src/vizier/services/adaptive_export/... all green. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export_loadtest: exp_control uses real now_s event_time (no future-stamp watermark poison) Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: ADAPTIVE_RECONCILE per-pull write-fidelity instrument Records one forensic_db.ae_reconcile row per data-plane pull (read_count vs wrote_count, window, ns/pod) across ALL three write paths — controller fan-out (filter), passthrough firehose, and streaming scanner — so a reconcile run localizes loss to query (R5: read<PEM) vs sink (R6: wrote<read) and quantifies re-pull dup (C8). Counts alone (write >= read) were proven insufficient. - new internal/reconcile leaf package (Row, Recorder, Nop) — no import cycle - sink.Record: CH-backed recorder (INSERT INTO forensic_db.ae_reconcile) - ae_reconcile table: schema.sql + KnownTables + OperatorOwnedTables (synced); not a pixie table (absent from PixieTables, so VerifyPixieSchema ignores it) - wired: passthrough.tick, controller.pushPixieRows (deferred, all return paths), streaming.scanner.Run; gated by ADAPTIVE_RECONCILE=true, else Nop - unit test proves read/wrote capture incl. the sink-drop read>wrote shape - fixed apply_test trailing-tables guard for the new operator table - harness: exp_row_reconcile.sh (row-level PEM<->CH), ae_vs_all.sh, exp_datavolume_extreme.sh Signed-off-by: entlein <einentlein@gmail.com> * harness: exp_pipeline_reconcile — skip empty-key rows (0 rows != LOSS 1) px -o json empty result previously printed one blank line → counted as a phantom LOSS=1. Guard: empty set → 0-byte keys file; drop all-empty-field keys. Confirmed against the controlled log4j run (backend http 14/14 exact, conn 66>=12, no loss). Signed-off-by: entlein <einentlein@gmail.com> * harness: log4shell_fire.sh — reliably fire + restart the log4j-chain log4shell Reliable BY CONSTRUCTION against bob#140 (stateful/unreliable exploit on re-fire): fresh-JVM backend (delete pod) + attacker-before-backend + the WORKING resolvable FQDN attacker.<ns>.svc.cluster.local:1389 (NOT the bare attacker-ns.svc which NXDOMAINs and gets dropped), then VERIFY the actual backend->:1389 LDAP egress in forensic_db.conn_stats and RETRY until confirmed (the validity gate). Never assumes the exploit fired. Node-side. Signed-off-by: entlein <einentlein@gmail.com> * harness: log4shell_fire.sh — detection-signal framing (Cyber Verification) Reword from offensive 'exploit' to detection-signal-generation language: this validates the kubescape->DX->AE detection chain. No logic change. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export(passthrough): precompiled + concurrent firehose, drop http2 - pxl.CompilePassthrough/Render: precompile per-table PxL once (fixed window => constant relative start_time), only the two time_ bounds are stamped per tick. Rendered output is byte-identical to QueryFor with an empty Target (TestCompilePassthrough_MatchesQueryFor), so this is a structural change, not a capture change. upid->pod/ns stays in PxL. - passthrough: tickConcurrent fans every table out at once (was a serial loop); shared pull() helper. Sink/recorder are pool/HTTP-backed and already called concurrently elsewhere. - drop http2_messages.beta from the firehose set (not materialised on every cluster => ""Table not found"" every tick); shared PixieTables/DDL lists untouched. - toggle ADAPTIVE_PASSTHROUGH_COMPILED (default on; =false reverts to the legacy serial QueryFor path). Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: bazel BUILD deps for internal/reconcile + pxl compile.go Fixes "missing strict dependencies: import of .../internal/reconcile" that broke the AE image build for passthrough, sink, streaming, controller, cmd (pre-existing since the ADAPTIVE_RECONCILE commit added the package + imports without bazel deps; never CI-built). Also wires the new pxl/compile.go srcs + passthrough/pxl test srcs (compiled_test.go, reconcile_test.go, compile_test.go). - new internal/reconcile/BUILD.bazel (go_library, stdlib-only) - +//internal/reconcile dep: passthrough, sink, streaming, controller, cmd - pxl go_library +compile.go; pxl_test +compile_test.go; passthrough_test +compiled_test.go,reconcile_test.go (+reconcile dep) Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export(pxl): raise Pixie 10k result cap via #px:set query flag F1 RCA: Pixie caps every px.display at max_output_rows_per_table (default 10000, query_flags.go) — the planner add_limit_to_batch_result_sink_rule silently truncates wide firehose windows / busy pods at the READ (write path is clean: ae_reconcile shows read==wrote). Fix uses Pixie own native knob — prepend `#px:set max_output_rows_per_table=1000000` to every generated PxL (QueryFor + CompilePassthrough) so all pull paths are uncapped. Validated on rig: a 14208-row window returned 10000 (capped) vs 14298 (with flag). No pagination loop, no extra round-trips. See memory project-ae-passthrough-10k-cap. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export/sink: content_type silent-drop contract suite Consolidates the recurring content_type silent-drop incident class into one default-suite test gate (6 tests, ~15ms): I1 TestContract_ContentTypeIsInt64InSchema I2 TestContract_FastEncodeContentTypeAsInt I3 TestContract_SilentDropDetected I3.b TestContract_SilentDropNotTriggeredOnSuccess I3.c TestContract_SilentDropToleratesMissingSummaryHeader I4 TestContract_HTTPEventsRoundTrip Top-of-file docstring chronicles the incident timeline so future operators can grep their way to the contract. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export_loadtest: DX-steered-vs-ALL datavolume reduction harness Measures the AdaptiveExport value prop: datavolume REDUCTION of DX-steered AE (rev-3 streaming, AE writes only DX-steered activeSet pods over the control surface) vs saving ALL data (passthrough firehose). Two arms, same fixed load, forensic_db active-part deltas (rows+bytes) per table; reduction = 1 - DX/ALL. Uses the canonical resolvable JNDI FQDN (attacker.attacker-ns.svc.cluster.local) so the chain fires + DX can classify (a malformed host → NXDOMAIN → no steer). Successor to ae_vs_all.sh, whose AE arm used the rev-2 controller gate + stale JNDI. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export_loadtest: deep AE NFR benchmark harness Measures all AE non-functional requirements under steady load on the rig: throughput (rows+bytes/sec), capture completeness (AE read vs broker count = F1 cap proof), write fidelity (read==wrote + write-error count), end-to-end freshness latency (now - max(time_) in CH), resource footprint (AE pod cpu/mem idle vs loaded), per-cycle cadence. Emits a structured report; companion to exp_dx_steering_reduction.sh. Real-data only. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export_loadtest: fix DX-reduction dead-arm (clear stale steering + live-pod guard) Run-1 reported false 100% reduction because stale adaptive_attribution windows rehydrated DEAD pods (deleted loaders) into the activeSet → AE streamed dead pods → 0 rows. Clear adaptive_attribution before the DX arm so the activeSet only gets freshly-steered LIVE pods; add a guard that prints the steered pods + marshalsec fire count + live log4j-poc pods so a dead-arm result is caught, not reported as a reduction. Signed-off-by: entlein <einentlein@gmail.com> * nfr harness: fix lag (dateDiff) + drop racy broker-pct completeness lag query used now()-DateTime64 (type error -> na); use dateDiff(second,...). Capture-completeness vs broker was window-misaligned (623% artifact) -> drop it; report tot_read vs tot_wrote (read==wrote) + errs instead. The 10k-cap/completeness proof is the dedicated F1 test (max_read>10000 vs broker for the SAME window). Signed-off-by: entlein <einentlein@gmail.com> * dx-reduction harness: report ROWS reduction (primary) + bytes (secondary) Run-2 byte-delta reduction came out negative because system.parts byte delta is compaction-noisy (merges land mid-window). Report rows reduction as primary (actual captured-row count, noise-free); keep bytes as secondary context. Signed-off-by: entlein <einentlein@gmail.com> * ae deployment: add memory limit (1Gi) + raise cpu limit to 1 core Eviction-RCA finding (PR #63 NFR campaign): AE had NO memory limit (only cpu 300m) and was CPU-pinned at 300m under concurrent passthrough. AE measured tiny (16-38Mi steady), but the raised 1M-row passthrough cap can spike, so cap at 1Gi so AE can never memory-pressure a node; raise cpu limit 300m->1 core (was throttling). NOTE node evictions were NOT AE/OOM — node-01 went NotReady (network/heartbeat); the memory consumer is PEM (1365Mi, OOMs at the 2Gi default). Signed-off-by: entlein <einentlein@gmail.com> * ae bootstrap: separate the secret from the re-applied infra bundle Root cause of the recurring "AE unauthenticated / writes 0 / crashloop" reverts: kustomization.yaml bundled adaptive_export_secrets.yaml (placeholder pixie-api-key) with the role+deployment, so EVERY infra re-apply (make log4j) clobbered the real key that ae-auth had written. Separation of concerns: remove the secret from the kustomization — infra (role+deployment) stays re-appliable; the secret holds real creds and is owned solely by `make ae-auth`, created once, never touched by infra re-applies. Secret manifest kept as a hand-applied seed-only template (documented). Signed-off-by: entlein <einentlein@gmail.com> * dx-reduction harness: fire BOTH attack stages so DX steers the backend The DX-steered arm was failing because the harness only fired stage-1 (JNDI/LDAP). That generates ldap-egress but NO kubescape R0001 → backend never flagged → DX no case → indeterminate → AE steers wrong/no pods. R0001 comes from stage-2 (post- exploitation exec). fire() now does stage-1 (JNDI) + stage-2 (whoami/shadow/token/ getent in the backend) → kubescape R0001+R0006 → DX rules backend MALIGNANT → backend enters AE activeSet → reduction is measurable. Verified live: DX evidence unexpected-spawn+sensitive-file-read → verdict ruled_in generic=MALIGNANT. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: rename whitelist→allowlist across streaming path + add DX-steering diagnostics Standing terminology rule: allowlist/blocklist, never whitelist/blacklist. Pure rename (no behavior change) of the rev-3 streaming filter: FilterModeWhitelist → FilterModeAllowlist MaxWhitelistSize → MaxAllowlistSize ADAPTIVE_STREAM_MAX_WHITELIST → ADAPTIVE_STREAM_MAX_ALLOWLIST (env) mode=whitelist log string → mode=allowlist plus all comments/identifiers/tests in streaming, activeset, cmd/main. DX-steering diagnostics (the reason DX-arm-writes-0 has been hard to RCA — we could not tell "empty ActiveSet" from "broker returned 0 rows"): - scanner: log the empty-allowlist short-circuit (was silent) so an empty ActiveSet is visible in logs, distinct from "query completed rows=0". - FilterUpdater: emitted-filter log Debug→Info so the steered pod count per ActiveSet change is visible without debug logging. NOTE: ADAPTIVE_STREAM_MAX_WHITELIST env renamed → tooling that sets the old name must switch to ADAPTIVE_STREAM_MAX_ALLOWLIST. Signed-off-by: entlein <einentlein@gmail.com> * ae(clickhouse): create forensic_db.dx_attack_graph at boot The Pixie dx_evidence_graph UI reads dx_attack_graph via px.DataFrame clickhouse_dsn, whose query template hardcodes event_time + hostname and ORDER BY event_time. A table without those columns fails 'Unknown identifier event_time'; a table created by hand (local, not via the operator) isn't globally registered. Fix: make AE own it like the other forensic tables. - schema.sql: dx_attack_graph DDL with event_time(UInt64 nanos) + hostname, edge columns, fromUnixTimestamp64Nano partition/TTL (nanos-correct). - KnownTables + OperatorOwnedTables: register it so Apply creates it at boot. - apply_test: assert last-applied DDL == last OperatorOwnedTables entry (robust to appended operator tables) instead of hardcoding trigger_watermark. go test ./.../clickhouse green. Signed-off-by: entlein <einentlein@gmail.com> * ae(clickhouse): dx_attack_graph numeric cols Int64/Float64 (px-readable) Pixie's clickhouse_dsn type mapper reads UInt8 as BOOLEAN and does not handle UInt16/UInt32/Float32 -> px fails with 'Column[N] given incorrect type' rendering the dx_evidence_graph. weight/max_severity/num_findings -> Int64, confidence -> Float64 (map cleanly to INT64/FLOAT64). Verified live: px run returns all 6 edges with every column. event_time stays UInt64 (matches kubescape_logs, which px reads). Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export(streaming): add #px:set max_output_rows cap flag to scanner buildPxL The DX/streaming arm silently capped each per-table pull at Pixie's default 10000-row limit while the passthrough/ALL arm (pxl.CompilePassthrough / QueryFor) already raises it to 1,000,000 via the broker's #px:set query flag. Validated live on 6a33dac0: a single streaming http_events pull returned exactly rows=10000 (the cap). Left unfixed this UNDER-counts the DX arm and OVERSTATES the DX-vs-ALL volume reduction. Prepend the same #px:set directive to the streaming scanner's PxL so both arms are uncapped and comparable. Signed-off-by: entlein <einentlein@gmail.com> * ae(clickhouse): create dx_attack_graph_malicious view at boot Adds the rule-ins-only view (condition != '') to the canonical schema.sql, registers it in KnownTables + OperatorOwnedTables (after dx_attack_graph), and teaches DDL() to extract CREATE VIEW headers. AE now creates it on boot so the dx_evidence_graph UI's default malicious-only read is standard, not a per-rig manual step. Tests updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: entlein <einentlein@gmail.com> * ae(control): /dx/attack_graph ingest endpoint -> ClickHouse dx POSTs a JSON array of edges to /dx/attack_graph; AE writes them to forensic_db.dx_attack_graph via JSONEachRow (Applier.WriteAttackGraph). Wired in main.go when CONTROL_ADDR is set; 501 if the sink is unset. This is the AE half of the dx->AE->CH attack-graph write path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: convention pass — consolidate CH HTTP, drop dead code, fix stale tests PR-53 review follow-ups (see review summary in conversation): 1. License headers added to 17 src/e2e_test/adaptive_export_loadtest/ harness/*.{sh,py} scripts. Matches the convention every other src/e2e_test/*/sh in pixie already follows. 2. Dead code removed (was unreachable per golang.org/x/tools/cmd/deadcode): - internal/script/script.go: IsClickHouseScript, IsScriptForCluster, GetActions, getScriptName, getInterval, templateScript, plus the ScriptConfig and ScriptActions types. The cron-script sync flow they served was replaced by the streaming model; only the Script and ScriptDefinition types remain. - internal/pixie/pixie.go: Client.GetPresetScripts (replaced by builtinPresetScripts() inline in cmd/main.go). - internal/streaming/{supervisor,writer}.go: SupervisorStats, TableStats, Stats types + Supervisor.Stats and BatchWriter.Stats methods (no production reader). Atomic counters dropped; the existing flush log preserves the per-flush summary. 3. Stale passthrough tests fixed. TestLoop_DefaultsTablesToPixieTables and TestNew_AppliesDefaults asserted len(clickhouse.PixieTables()) == 13 but passthrough.New strips excludedTables (http2_messages.beta), yielding 12. Tests now compare against filterExcluded(clickhouse.PixieTables()) and add an extra assertion that excluded tables were not written. 4. ClickHouse HTTP client consolidation: new internal/chhttp/ package collapses three near-identical HTTP CH clients (clickhouse.Applier, sink.ClickHouseHTTP, trigger.ClickHouseWatermarkStore) into one. Centralises endpoint validation, basic-auth header, 30s default timeout, fail-loud INSERT settings (4 CH input_format knobs), and the X-ClickHouse-Summary read path. The 4 INSERT call sites in the three callers all route through chhttp.Client.Insert now; SELECT through Query or QueryStream (the latter preserves the QueryActive streaming behaviour). Net code: -200 LOC across the three callers plus a 200-LOC chhttp package with its own tests. 5. Pixie service scaffold wired into cmd/main.go: services.SetupService("adaptive-export", 50900) + services.SetupSSLClientFlags() + services.PostFlagSetupAndParse() + services.SetupServiceLogging(). Matches the pattern every other pixie Go service uses. CheckServiceFlags() is deliberately skipped (AE does not run a TLS gRPC server). Existing AE env-var reads (ADAPTIVE_*) are untouched and still authoritative for tuning knobs. All 14 adaptive_export internal packages pass go test (1 new chhttp, 13 unchanged), including the 3 differential oracle tests in pxl/compile_test.go. arc lint OKAY for everything except 3 pre-existing findings unrelated to this commit (loadgen has its own go.mod; one QF1001 De Morgan's-law nit in reconcile_test.go from c9f19b6). Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: restore executable bits on harness scripts (post-header) Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: lint pass + restore @px load prefix in cmd/BUILD.bazel User flagged on review 4536971862: - cmd/BUILD.bazel:18 dropped the '@px' external-workspace prefix on pl_build_system.bzl load. Restored — the standalone AE build pulls pixie as @px and needs the qualifier. Lint cleanup (PR-53 scope, no production code touched outside renames): - chhttp/chhttp_test.go: errcheck on two w.Write calls + gofumpt on the gotSettings declaration. - passthrough/reconcile_test.go: staticcheck QF1001 (De Morgan's law) on the conn_stats sink-drop assertion. - script/script.go: rename ScriptId -> ScriptID (ST1003). Propagated to pixie/pixie.go and cmd/main.go callsites. - internal/e2e/BUILD.bazel and internal/trigger/BUILD.bazel: gazelle drift — adding loadtest_test.go and clickhouse_internal_test.go to srcs. - k8s/00-sinks.yaml + gen-pod.tmpl.yaml: yamllint compliance — document-start marker, dedent sequence items per .yamllint indent-sequences=false, tighten flow-mapping spaces, collapse multi-space after commas. YAML semantics unchanged. - harness/stats.py: flake8 E501 — split a long line into two. Final arc lint state on PR-53 file scope: 0 Errors, 14 Warnings (SHELLCHECK SC2155/SC2086 in pre-existing harness scripts from ae7b86f, not introduced or modified by this commit). Pre-existing loadgen typecheck failures (separate go.mod) are unaffected. Signed-off-by: entlein <einentlein@gmail.com> * adaptive_export: address user review #2 #4 #6 + 4 outstanding CodeRabbit items User review 4536971862: #2 passthrough/reconcile_test.go — strengthened with two new tests that exercise the FULL chain (loop → real sink.ClickHouseHTTP → httptest CH endpoint → reconcile recorder). TestTick_ReconcileCatchesCHSilentDrop mimics CH's X-ClickHouse-Summary silent-drop shape (200 OK, written_rows=0) and asserts the loop records WroteCount=0 with a silent-drop attribution — the exact R6 (sink-layer loss) regression the instrument exists to detect. TestTick_ReconcileAttributesCHFailureCorrectly covers the 500-response branch. The pre-existing in-process-fake test stays as the wiring check. #4 pixie/pixie.go — added a long comment justifying the API-key auth choice. pixie.Client targets the Pixie CLOUD (cloudpb's PluginService), whose auth interceptor accepts pixie-api-key and rejects JWT service tokens (those are for INSIDE-cluster vizier services). The pixieapi.Adapter that talks to vizier directly already uses JWT via jwtutils.GenerateJWTForService — same pattern as cloud_connector/vizhealth/checker.go:111. So the split is intentional; flipping pixie.go to JWT would break cloud auth, not improve it. #6 pxl/queryfor — hardened escapePxL so a raw \n, \r, \t or NUL byte in Target.Pod/Target.Namespace can't terminate the PxL string literal and inject a new statement. Added TestQueryFor_RejectsInjectionInTargetFields driving QueryFor with 7 adversarial pod/namespace shapes (newline, single-quote-only, CR, backslash-escape-of-escape, NUL, tab) plus the regex_match fallback path, asserting the output line count and every statement's leading token. Extends existing TestEscapePxL_TableDriven with the new escape mappings. CodeRabbit oversights (verified each against current code): CR r3379377432 (main.go) — wrapped the control-surface listener in http.Server with Read/ReadHeader/Write/Idle timeouts so a slow client can't pin a goroutine indefinitely. CR r3379377607 (pixieapi.go) — switched direct-mode dial from pxapi.WithDisableTLSVerification (env + addr-gated, brittle) to pxapi.WithDirectTLSSkipVerify (added in PR #49 b523ce3 for the same node-IP-dial scenario). Removed the cluster.local + PX_DISABLE_TLS precondition in NewDirect; the always-skip semantics match the AE deployment shape. Refactored the obsolete env-gate test. CR r3379377645 (streaming/filter.go) — the deltaCh-close path returned without calling disarm(), leaking the timer's goroutine on shutdown. Now calls disarm() before return on chan-close. CR r3426923299 (sink/clickhouse.go Record) — capped Record at a 2s per-call context timeout. The 30s chhttp default was too long for the scanner/passthrough/controller hot paths that call Record inline; reconcile is best-effort by contract, so a stalled CH must not pin the pull loop. All 14 AE packages pass go test. arc lint: 0 Errors on PR-53 file scope. Signed-off-by: entlein <einentlein@gmail.com> * test(harness): consolidate to one run-picture + e2e CI workflow Finish the harness consolidation #53 started: adopt exp_matrix.sh (canonical ALL-vs-DX reduction matrix) + nfr.sh (throughput/mem/verdict-latency) as the single runners; cut the overlapping variants (ae_vs_all, exp_datavolume_extreme, exp_dx_steering_reduction, exp_ae_nfr_benchmark, exp_pipeline_reconcile) and the superseded standalone setup (deploy_ae, build_gen_image). README rewritten as the single 'how to run' source: two families — fixture-isolation (run.sh + E-series) and live-attack e2e (log4shell_fire → exp_matrix → nfr → exp_row_reconcile). Add e2e_log4shell_soc.yaml: k3s + full SOC stack (Pixie/kubescape/ClickHouse/AE/dx via k8sstormcenter/soc) on the oracle runner; asserts every canonical harness script runs + dx rules in; profiles dx in real life (pprof CPU/heap + verdict latency, uploaded). Uses existing repo secrets. Signed-off-by: entlein <einentlein@gmail.com> * revert(bazel): drop stray buildifier attribute-reorder in stirling container_images BUILD This file is unrelated to adaptive_export — the only change was buildifier alphabetizing container_type/bazel_sdk_versions (no functional change). Restore main's version to keep #53's diff to real AE changes. Signed-off-by: entlein <einentlein@gmail.com> * ci: fix run-genfiles + run-container-lint on PR 53 run-genfiles: the PR had reordered the kwargs in stirling/.../container_images/ BUILD.bazel alphabetically (bazel_sdk_versions before container_type) — a local buildifier or gazelle drift from when the file was first committed. CI gazelle wants the original order (container_type first), so the diff loop fails. Restored to origin/main's ordering. Local gazelle disagrees with CI's expected output (tooling-version drift); CI is authoritative. run-container-lint: two findings. 1. staticcheck QF1001 (De Morgan's law) in pxl/queryfor_test.go:318. Rewrote the !(a || b || c || d) negated disjunction as the equivalent !a && !b && !c && !d. Test behaviour unchanged; verified locally with TestQueryFor_RejectsInjection. 2. golangci-lint typechecking failed on src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/{cleanloadgen, httpsink}/main.go because that subtree carries its own go.mod and does not resolve as a package under the root px.dev/pixie module. Added the loadgen subtree to .arclint's exclude list. Same fix the existing entries for other-module subtrees apply. Local 'arc lint' on the PR-53 file scope: 0 Errors, 14 SHELLCHECK Warnings + 26 SHELLCHECK Advice (all pre-existing in ae7b86f harness scripts; not introduced by this PR). Signed-off-by: entlein <einentlein@gmail.com> * ci: apply gazelle's actual kwarg order to stirling container_images run-genfiles CI re-failed after f244ffc reverted this file to main's ordering — turns out gazelle on this repo IS alphabetizing the kwargs (bazel_sdk_versions before container_type), and CI runs 'gazelle fix' then 'git diff' to catch any drift. So main's ordering is no longer gazelle-stable; the file has to match gazelle's preference, not main's. Verified locally with 'bazel run //:gazelle -- fix'; the file is now idempotent (a second gazelle run produces no diff). Signed-off-by: entlein <einentlein@gmail.com> * ci: fix container-lint Errors on e2e workflow + new harness scripts run-container-lint re-failed after the run-genfiles fix because two files added on this branch (a03aa15) had unfixed lint errors: .github/workflows/e2e_log4shell_soc.yaml — 5 yamllint Errors: - 1 indentation: list items under steps: must be parent-aligned per the repo's .yamllint config (indent-sequences: false), not 2-indented. Dedented every step item + its run: block by 2 spaces. - 4 line-length (>120 chars): split the long kubectl-set-image, the long grep-detection gate, the curl pprof URL, and the verdict-latency grep across continuation lines. Semantics unchanged. src/e2e_test/adaptive_export_loadtest/harness/{exp_matrix.sh, nfr.sh} — missing Apache headers. Applied via arc lint --apply-patches; exec bits restored. Local arc lint on PR-53 file scope: 0 Errors, 14 SHELLCHECK Warnings + 26 Advice (all pre-existing in harness scripts, unchanged). Signed-off-by: entlein <einentlein@gmail.com> * ci: silence 6 SHELLCHECK Warnings to clear container-lint exit code arc lint --apply-patches exits non-zero on Warning level too. Resolved each: replaced unused 'for i/t in ...' loop vars with '_', split a SC2155 declare-and-assign, dropped two never-referenced hip/pip assignments. Signed-off-by: entlein <einentlein@gmail.com> --------- Signed-off-by: entlein <einentlein@gmail.com> Co-authored-by: Entlein <eineintlein@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: pixie-agent <noreply@local>
Author
|
@CodeRabbit : rereview the entire PR now, it seems ready to merge |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== direct_query_server.h ==="
cat src/vizier/services/agent/pem/direct_query_server.h
echo ""
echo "=== direct_query_server.cc ==="
cat src/vizier/services/agent/pem/direct_query_server.cc
echo ""
echo "=== direct_query_server_test.cc (first 300 lines) ==="
head -300 src/vizier/services/agent/pem/direct_query_server_test.ccLength of output: 41934 🏁 Script executed: #!/bin/bash
echo "=== direct_query_server_test.cc (lines 300-600) ==="
sed -n '300,600p' src/vizier/services/agent/pem/direct_query_server_test.cc
echo ""
echo "=== direct_query_server_test.cc (lines 600-895) ==="
sed -n '600,895p' src/vizier/services/agent/pem/direct_query_server_test.ccLength of output: 30332 🏁 Script executed: #!/bin/bash
echo "=== pem_manager.cc (new additions only - last 200 lines) ==="
tail -250 src/vizier/services/agent/pem/pem_manager.cc
echo ""
echo "=== pem_manager.h ==="
cat src/vizier/services/agent/pem/pem_manager.h
echo ""
echo "=== DIRECT_QUERY_SECURITY.md ==="
cat src/vizier/services/agent/pem/DIRECT_QUERY_SECURITY.md
echo ""
echo "=== DIRECT_QUERY_CONTRACT.md ==="
cat src/vizier/services/agent/pem/DIRECT_QUERY_CONTRACT.mdLength of output: 44645 🏁 Script executed: #!/bin/bash
echo "=== BUILD.bazel (pem) ==="
cat src/vizier/services/agent/pem/BUILD.bazel
echo ""
echo "=== manager.cc changes ==="
cat src/vizier/services/agent/shared/manager/manager.cc | grep -A 20 "jwt_signing_key"
echo ""
echo "=== opts.go ==="
cat src/api/go/pxapi/opts.go
echo ""
echo "=== delete.go ==="
cat src/utils/shared/k8s/delete.goLength of output: 25624
Overall: Functionally merge-ready ✅All seven original review requirements and both blockers (TLS + auth bug) are resolved. The implementation is solid. I found three minor issues below — none block merge, but one has a misleading comment worth correcting. ✅ What is now correct
🟡 Minor issues (non-blocking)1. Misleading comment in
But the code returns immediately: bool constantTimeEquals(absl::string_view a, absl::string_view b) {
if (a.size() != b.size()) return false; // ← returns immediately, doesn't walk
...
}In practice this is not a security concern here since 2.
} else if (payload["exp"].IsInt()) {
exp_secs = static_cast<int64_t>(payload["exp"].GetInt());
}3. Line 20 of Remaining skipped tests (acceptable)
VerdictThe implementation is complete. The critical auth bug and TLS blocker that were merge-blockers are both resolved and live-verified on pemdq10. The three items above are cosmetic/defensive and can be addressed in a follow-up. This PR is ready to merge. |
10 tasks
entlein
added a commit
that referenced
this pull request
Jul 17, 2026
* adaptive_export: production AE — streaming export + write-integrity
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: ADAPTIVE_PASSTHROUGH firehose loop
New env-gated background loop that runs the same PxL shape AE's
anomaly-gated path uses, but with an empty Target (no ns/pod predicate)
and over a configurable rolling window. Writes via the existing sink so
the byte-shape of forensic_db rows is comparable between the
PASSTHROUGH=1 phase (EVERYTHING) and the PASSTHROUGH=0 phase
(AE-FILTER). One-shot A/B that yields the per-table capture fraction of
the adaptive write path.
- internal/passthrough/passthrough.go — Loop + Config; defaults to
30s window / 30s refresh / clickhouse.PixieTables() table list.
- internal/passthrough/passthrough_test.go — 6 tests; the load-bearing
one is TestLoop_EmitsEmptyTargetPxL (asserts neither df.namespace nor
df.pod predicates appear in the emitted PxL).
- cmd/main.go: ADAPTIVE_PASSTHROUGH + _WINDOW_SEC + _REFRESH_SEC env
knobs. Adapter is constructed unconditionally when passthrough is on
(joins the existing PushPixie / streaming construction path so the
same pxapi grpc stream is reused). Loop is registered with the
shutdown WaitGroup so SIGTERM waits for the in-flight tick.
- cmd/BUILD.bazel: drop @px// load (other AE BUILD.bazel files use
//bazel — sticking out as the only one with @px is a leftover from a
prior gazelle run; align). Add passthrough dep.
Signed-off-by: entlein <einentlein@gmail.com>
* ci: dx-image workflow — build + publish dx-daemon to ghcr
Stand-alone workflow that builds entlein/dx (private Active-Diagnosis
Framework) into ghcr.io/k8sstormcenter/dx-daemon. Separates the dx image
publish from the bazel-based vizier_release pipeline; the dx repo ships
its own Dockerfile.dxd (Go cross-compile + distroless final stage) so it
doesn't need to live as a submodule inside src/vizier/services/dx.
Triggers:
- tag push 'release/dx/v*' on this repo cuts a release build, image tag
derived from the tag suffix (release/dx/v0.1.0 -> image tag 0.1.0).
- workflow_dispatch lets us build any dx ref on demand with a custom tag
(default: short sha of the resolved dx commit).
Pulls dx via DX_ENTLEIN_PAT (already configured on the repo). Multi-arch
build (linux/amd64, linux/arm64); Dockerfile.dxd cross-compiles in the
native BUILDPLATFORM stage and the final stage is COPY-only, so target
emulation isn't required.
Signed-off-by: entlein <einentlein@gmail.com>
* Revert ci: dx-image workflow — wrong repo
The dx image build pipeline lives in entlein/dx itself (PR #53,
branch feat/bazel-release): bazel-based with @px external pin to
pixie's ae-prod tip, pushes to docker.io/entlein/dx-daemon on a
release/dx/v* tag in the dx repo. The pixie-side buildx workflow
this reverts duplicated that intent in the wrong repo + the wrong
build system (docker buildx instead of bazel + pl_go_image macros)
+ the wrong registry (ghcr.io/k8sstormcenter instead of
docker.io/entlein).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: unit-normalize trigger watermark cursor + load-test affordances
Fixes the silent-halt bug: the trigger gated on a RAW event_time high-water-mark,
so a single anomaly in a larger unit (ms/ns) drove the watermark past all real
seconds rows and AE stopped processing forever (data still on Pixie). Normalize
event_time to canonical nanoseconds in the poll SELECT filter+order and in the
in-memory/persisted cursor, boundary-dedup, and maxSeen (normalizeEventTimeNanos
+ chNormEventTimeNanos). Validated at the data layer: vs a poisoned watermark the
raw filter returns 0 rows, the normalized filter recovers all 60.
Also adds ADAPTIVE_PUSH_REFRESH_SEC (negative = single-shot pull) for the
reproducible load-test harness, an in-package trigger unit test, and an e2e
hermetic load test (mock PixieQuerier, exact rows+bytes).
Signed-off-by: entlein <einentlein@gmail.com>
* e2e_test/adaptive_export_loadtest: AE fixture-isolation load-test harness
Consolidate the adaptive_export load-test harness under src/e2e_test/ (matching
vzconn_loadtest / px_cluster conventions). Control-plane experiments (E1-E4, E6,
E8 sustained) are proven exactly-reproducible on a live rig; the data-plane
experiments (E5, E8 data-mode) are authored and pending live validation on a
vizier-registered rig (status documented in README + FINDINGS_AND_BACKLOG).
- harness/: shell + python (inject, exp_control, exp_e8, stats, ...) + lib helpers
- fixtures/EXPERIMENTS.md: curated kubescape_logs data-set catalog + expected outputs
- k8s/: isolated sinks + per-rep generator pod (no probes)
- tools/loadgen/: cleanloadgen + httpsink (docker-built test tool; .bazelignore'd
pending a bazel target — lib/pq is already vendored in the module)
- FINDINGS_AND_BACKLOG.md: F8 watermark-poison bug + the fix + AE backlog
The AE Go unit/e2e tests live with the service (internal/{trigger,e2e}).
Signed-off-by: entlein <einentlein@gmail.com>
* e2e_test/adaptive_export_loadtest: document AE implied contracts (C1-C14) + diagrams
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: C15 write-duration contract + DX-steering diagram; gen sustained-DNS mode
C15 = AE must keep re-pulling+writing an active pod until t_end or DX stop (the
contract DX steers on; last week's 'wrote then stopped' is its violation). Add
DX-steering sequence diagram. Generator gains SUSTAIN_SEC (distinct-DNS trickle,
a Pixie-traced protocol) + configurable SETTLE_PRE_MS warm-up for fresh-pod
capture; harness wires GEN_SUSTAIN_SEC/GEN_SETTLE_MS.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export/trigger: update test SQL substrings for multiIf normalisation
The 700821d3b trigger unit-normalisation wrapped event_time in a
multiIf(...) inside both the WHERE filter and the ORDER BY. Three
existing tests in watermark_test.go + one in clickhouse_test.go pinned
the raw 'event_time >= N' substring and broke at HEAD.
Update each test's expected substring to match the new normalized form
(') >= <ns-scaled N>' — the closing paren of multiIf, then the value
in canonical nanoseconds). Per-test ns-scaling:
watermark_test.go:94 1744000000000000000 already ns -> unchanged
watermark_test.go:125 InitialWatermark=42 < 1e10 sec -> * 1e9
watermark_test.go:156 InitialWatermark=7 < 1e10 sec -> * 1e9
watermark_test.go:297 event_time='5000' < 1e10 sec -> * 1e9
clickhouse_test.go:82 ref.T=1744..303e9 ns already ns -> unchanged
go test ./src/vizier/services/adaptive_export/... all green.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: exp_control uses real now_s event_time (no future-stamp watermark poison)
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: ADAPTIVE_RECONCILE per-pull write-fidelity instrument
Records one forensic_db.ae_reconcile row per data-plane pull (read_count vs
wrote_count, window, ns/pod) across ALL three write paths — controller fan-out
(filter), passthrough firehose, and streaming scanner — so a reconcile run
localizes loss to query (R5: read<PEM) vs sink (R6: wrote<read) and quantifies
re-pull dup (C8). Counts alone (write >= read) were proven insufficient.
- new internal/reconcile leaf package (Row, Recorder, Nop) — no import cycle
- sink.Record: CH-backed recorder (INSERT INTO forensic_db.ae_reconcile)
- ae_reconcile table: schema.sql + KnownTables + OperatorOwnedTables (synced);
not a pixie table (absent from PixieTables, so VerifyPixieSchema ignores it)
- wired: passthrough.tick, controller.pushPixieRows (deferred, all return
paths), streaming.scanner.Run; gated by ADAPTIVE_RECONCILE=true, else Nop
- unit test proves read/wrote capture incl. the sink-drop read>wrote shape
- fixed apply_test trailing-tables guard for the new operator table
- harness: exp_row_reconcile.sh (row-level PEM<->CH), ae_vs_all.sh, exp_datavolume_extreme.sh
Signed-off-by: entlein <einentlein@gmail.com>
* harness: exp_pipeline_reconcile — skip empty-key rows (0 rows != LOSS 1)
px -o json empty result previously printed one blank line → counted as a phantom
LOSS=1. Guard: empty set → 0-byte keys file; drop all-empty-field keys. Confirmed
against the controlled log4j run (backend http 14/14 exact, conn 66>=12, no loss).
Signed-off-by: entlein <einentlein@gmail.com>
* harness: log4shell_fire.sh — reliably fire + restart the log4j-chain log4shell
Reliable BY CONSTRUCTION against bob#140 (stateful/unreliable exploit on re-fire):
fresh-JVM backend (delete pod) + attacker-before-backend + the WORKING resolvable FQDN
attacker.<ns>.svc.cluster.local:1389 (NOT the bare attacker-ns.svc which NXDOMAINs and
gets dropped), then VERIFY the actual backend->:1389 LDAP egress in forensic_db.conn_stats
and RETRY until confirmed (the validity gate). Never assumes the exploit fired. Node-side.
Signed-off-by: entlein <einentlein@gmail.com>
* harness: log4shell_fire.sh — detection-signal framing (Cyber Verification)
Reword from offensive 'exploit' to detection-signal-generation language: this validates
the kubescape->DX->AE detection chain. No logic change.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export(passthrough): precompiled + concurrent firehose, drop http2
- pxl.CompilePassthrough/Render: precompile per-table PxL once (fixed
window => constant relative start_time), only the two time_ bounds are
stamped per tick. Rendered output is byte-identical to QueryFor with an
empty Target (TestCompilePassthrough_MatchesQueryFor), so this is a
structural change, not a capture change. upid->pod/ns stays in PxL.
- passthrough: tickConcurrent fans every table out at once (was a serial
loop); shared pull() helper. Sink/recorder are pool/HTTP-backed and
already called concurrently elsewhere.
- drop http2_messages.beta from the firehose set (not materialised on
every cluster => ""Table not found"" every tick); shared PixieTables/DDL
lists untouched.
- toggle ADAPTIVE_PASSTHROUGH_COMPILED (default on; =false reverts to the
legacy serial QueryFor path).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: bazel BUILD deps for internal/reconcile + pxl compile.go
Fixes "missing strict dependencies: import of .../internal/reconcile" that
broke the AE image build for passthrough, sink, streaming, controller, cmd
(pre-existing since the ADAPTIVE_RECONCILE commit added the package + imports
without bazel deps; never CI-built). Also wires the new pxl/compile.go srcs
+ passthrough/pxl test srcs (compiled_test.go, reconcile_test.go, compile_test.go).
- new internal/reconcile/BUILD.bazel (go_library, stdlib-only)
- +//internal/reconcile dep: passthrough, sink, streaming, controller, cmd
- pxl go_library +compile.go; pxl_test +compile_test.go; passthrough_test +compiled_test.go,reconcile_test.go (+reconcile dep)
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export(pxl): raise Pixie 10k result cap via #px:set query flag
F1 RCA: Pixie caps every px.display at max_output_rows_per_table (default
10000, query_flags.go) — the planner add_limit_to_batch_result_sink_rule
silently truncates wide firehose windows / busy pods at the READ (write path
is clean: ae_reconcile shows read==wrote). Fix uses Pixie own native knob —
prepend `#px:set max_output_rows_per_table=1000000` to every generated PxL
(QueryFor + CompilePassthrough) so all pull paths are uncapped. Validated on
rig: a 14208-row window returned 10000 (capped) vs 14298 (with flag). No
pagination loop, no extra round-trips. See memory project-ae-passthrough-10k-cap.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export/sink: content_type silent-drop contract suite
Consolidates the recurring content_type silent-drop incident class
into one default-suite test gate (6 tests, ~15ms):
I1 TestContract_ContentTypeIsInt64InSchema
I2 TestContract_FastEncodeContentTypeAsInt
I3 TestContract_SilentDropDetected
I3.b TestContract_SilentDropNotTriggeredOnSuccess
I3.c TestContract_SilentDropToleratesMissingSummaryHeader
I4 TestContract_HTTPEventsRoundTrip
Top-of-file docstring chronicles the incident timeline so future
operators can grep their way to the contract.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: DX-steered-vs-ALL datavolume reduction harness
Measures the AdaptiveExport value prop: datavolume REDUCTION of DX-steered AE
(rev-3 streaming, AE writes only DX-steered activeSet pods over the control
surface) vs saving ALL data (passthrough firehose). Two arms, same fixed load,
forensic_db active-part deltas (rows+bytes) per table; reduction = 1 - DX/ALL.
Uses the canonical resolvable JNDI FQDN (attacker.attacker-ns.svc.cluster.local)
so the chain fires + DX can classify (a malformed host → NXDOMAIN → no steer).
Successor to ae_vs_all.sh, whose AE arm used the rev-2 controller gate + stale JNDI.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: deep AE NFR benchmark harness
Measures all AE non-functional requirements under steady load on the rig:
throughput (rows+bytes/sec), capture completeness (AE read vs broker count =
F1 cap proof), write fidelity (read==wrote + write-error count), end-to-end
freshness latency (now - max(time_) in CH), resource footprint (AE pod cpu/mem
idle vs loaded), per-cycle cadence. Emits a structured report; companion to
exp_dx_steering_reduction.sh. Real-data only.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export_loadtest: fix DX-reduction dead-arm (clear stale steering + live-pod guard)
Run-1 reported false 100% reduction because stale adaptive_attribution windows
rehydrated DEAD pods (deleted loaders) into the activeSet → AE streamed dead pods
→ 0 rows. Clear adaptive_attribution before the DX arm so the activeSet only gets
freshly-steered LIVE pods; add a guard that prints the steered pods + marshalsec
fire count + live log4j-poc pods so a dead-arm result is caught, not reported as
a reduction.
Signed-off-by: entlein <einentlein@gmail.com>
* nfr harness: fix lag (dateDiff) + drop racy broker-pct completeness
lag query used now()-DateTime64 (type error -> na); use dateDiff(second,...).
Capture-completeness vs broker was window-misaligned (623% artifact) -> drop it;
report tot_read vs tot_wrote (read==wrote) + errs instead. The 10k-cap/completeness
proof is the dedicated F1 test (max_read>10000 vs broker for the SAME window).
Signed-off-by: entlein <einentlein@gmail.com>
* dx-reduction harness: report ROWS reduction (primary) + bytes (secondary)
Run-2 byte-delta reduction came out negative because system.parts byte delta is
compaction-noisy (merges land mid-window). Report rows reduction as primary
(actual captured-row count, noise-free); keep bytes as secondary context.
Signed-off-by: entlein <einentlein@gmail.com>
* ae deployment: add memory limit (1Gi) + raise cpu limit to 1 core
Eviction-RCA finding (PR #63 NFR campaign): AE had NO memory limit (only cpu
300m) and was CPU-pinned at 300m under concurrent passthrough. AE measured tiny
(16-38Mi steady), but the raised 1M-row passthrough cap can spike, so cap at 1Gi
so AE can never memory-pressure a node; raise cpu limit 300m->1 core (was
throttling). NOTE node evictions were NOT AE/OOM — node-01 went NotReady
(network/heartbeat); the memory consumer is PEM (1365Mi, OOMs at the 2Gi default).
Signed-off-by: entlein <einentlein@gmail.com>
* ae bootstrap: separate the secret from the re-applied infra bundle
Root cause of the recurring "AE unauthenticated / writes 0 / crashloop" reverts:
kustomization.yaml bundled adaptive_export_secrets.yaml (placeholder pixie-api-key)
with the role+deployment, so EVERY infra re-apply (make log4j) clobbered the real
key that ae-auth had written. Separation of concerns: remove the secret from the
kustomization — infra (role+deployment) stays re-appliable; the secret holds real
creds and is owned solely by `make ae-auth`, created once, never touched by infra
re-applies. Secret manifest kept as a hand-applied seed-only template (documented).
Signed-off-by: entlein <einentlein@gmail.com>
* dx-reduction harness: fire BOTH attack stages so DX steers the backend
The DX-steered arm was failing because the harness only fired stage-1 (JNDI/LDAP).
That generates ldap-egress but NO kubescape R0001 → backend never flagged → DX no
case → indeterminate → AE steers wrong/no pods. R0001 comes from stage-2 (post-
exploitation exec). fire() now does stage-1 (JNDI) + stage-2 (whoami/shadow/token/
getent in the backend) → kubescape R0001+R0006 → DX rules backend MALIGNANT →
backend enters AE activeSet → reduction is measurable. Verified live: DX evidence
unexpected-spawn+sensitive-file-read → verdict ruled_in generic=MALIGNANT.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: rename whitelist→allowlist across streaming path + add DX-steering diagnostics
Standing terminology rule: allowlist/blocklist, never whitelist/blacklist.
Pure rename (no behavior change) of the rev-3 streaming filter:
FilterModeWhitelist → FilterModeAllowlist
MaxWhitelistSize → MaxAllowlistSize
ADAPTIVE_STREAM_MAX_WHITELIST → ADAPTIVE_STREAM_MAX_ALLOWLIST (env)
mode=whitelist log string → mode=allowlist
plus all comments/identifiers/tests in streaming, activeset, cmd/main.
DX-steering diagnostics (the reason DX-arm-writes-0 has been hard to RCA —
we could not tell "empty ActiveSet" from "broker returned 0 rows"):
- scanner: log the empty-allowlist short-circuit (was silent) so an
empty ActiveSet is visible in logs, distinct from "query completed rows=0".
- FilterUpdater: emitted-filter log Debug→Info so the steered pod count
per ActiveSet change is visible without debug logging.
NOTE: ADAPTIVE_STREAM_MAX_WHITELIST env renamed → tooling that sets the old
name must switch to ADAPTIVE_STREAM_MAX_ALLOWLIST.
Signed-off-by: entlein <einentlein@gmail.com>
* ae(clickhouse): create forensic_db.dx_attack_graph at boot
The Pixie dx_evidence_graph UI reads dx_attack_graph via px.DataFrame
clickhouse_dsn, whose query template hardcodes event_time + hostname and
ORDER BY event_time. A table without those columns fails 'Unknown
identifier event_time'; a table created by hand (local, not via the
operator) isn't globally registered. Fix: make AE own it like the other
forensic tables.
- schema.sql: dx_attack_graph DDL with event_time(UInt64 nanos) + hostname,
edge columns, fromUnixTimestamp64Nano partition/TTL (nanos-correct).
- KnownTables + OperatorOwnedTables: register it so Apply creates it at boot.
- apply_test: assert last-applied DDL == last OperatorOwnedTables entry
(robust to appended operator tables) instead of hardcoding trigger_watermark.
go test ./.../clickhouse green.
Signed-off-by: entlein <einentlein@gmail.com>
* ae(clickhouse): dx_attack_graph numeric cols Int64/Float64 (px-readable)
Pixie's clickhouse_dsn type mapper reads UInt8 as BOOLEAN and does not
handle UInt16/UInt32/Float32 -> px fails with 'Column[N] given incorrect
type' rendering the dx_evidence_graph. weight/max_severity/num_findings
-> Int64, confidence -> Float64 (map cleanly to INT64/FLOAT64). Verified
live: px run returns all 6 edges with every column. event_time stays
UInt64 (matches kubescape_logs, which px reads).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export(streaming): add #px:set max_output_rows cap flag to scanner buildPxL
The DX/streaming arm silently capped each per-table pull at Pixie's default
10000-row limit while the passthrough/ALL arm (pxl.CompilePassthrough /
QueryFor) already raises it to 1,000,000 via the broker's #px:set query flag.
Validated live on 6a33dac0: a single streaming http_events pull returned
exactly rows=10000 (the cap). Left unfixed this UNDER-counts the DX arm and
OVERSTATES the DX-vs-ALL volume reduction. Prepend the same #px:set directive
to the streaming scanner's PxL so both arms are uncapped and comparable.
Signed-off-by: entlein <einentlein@gmail.com>
* ae(clickhouse): create dx_attack_graph_malicious view at boot
Adds the rule-ins-only view (condition != '') to the canonical schema.sql,
registers it in KnownTables + OperatorOwnedTables (after dx_attack_graph), and
teaches DDL() to extract CREATE VIEW headers. AE now creates it on boot so the
dx_evidence_graph UI's default malicious-only read is standard, not a per-rig
manual step. Tests updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: entlein <einentlein@gmail.com>
* ae(control): /dx/attack_graph ingest endpoint -> ClickHouse
dx POSTs a JSON array of edges to /dx/attack_graph; AE writes them to
forensic_db.dx_attack_graph via JSONEachRow (Applier.WriteAttackGraph). Wired in
main.go when CONTROL_ADDR is set; 501 if the sink is unset. This is the AE half
of the dx->AE->CH attack-graph write path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: convention pass — consolidate CH HTTP, drop dead code, fix stale tests
PR-53 review follow-ups (see review summary in conversation):
1. License headers added to 17 src/e2e_test/adaptive_export_loadtest/
harness/*.{sh,py} scripts. Matches the convention every other
src/e2e_test/*/sh in pixie already follows.
2. Dead code removed (was unreachable per golang.org/x/tools/cmd/deadcode):
- internal/script/script.go: IsClickHouseScript, IsScriptForCluster,
GetActions, getScriptName, getInterval, templateScript, plus the
ScriptConfig and ScriptActions types. The cron-script sync flow they
served was replaced by the streaming model; only the Script and
ScriptDefinition types remain.
- internal/pixie/pixie.go: Client.GetPresetScripts (replaced by
builtinPresetScripts() inline in cmd/main.go).
- internal/streaming/{supervisor,writer}.go: SupervisorStats,
TableStats, Stats types + Supervisor.Stats and BatchWriter.Stats
methods (no production reader). Atomic counters dropped; the
existing flush log preserves the per-flush summary.
3. Stale passthrough tests fixed.
TestLoop_DefaultsTablesToPixieTables and TestNew_AppliesDefaults
asserted len(clickhouse.PixieTables()) == 13 but passthrough.New
strips excludedTables (http2_messages.beta), yielding 12. Tests now
compare against filterExcluded(clickhouse.PixieTables()) and add an
extra assertion that excluded tables were not written.
4. ClickHouse HTTP client consolidation: new internal/chhttp/ package
collapses three near-identical HTTP CH clients
(clickhouse.Applier, sink.ClickHouseHTTP, trigger.ClickHouseWatermarkStore)
into one. Centralises endpoint validation, basic-auth header,
30s default timeout, fail-loud INSERT settings (4 CH input_format
knobs), and the X-ClickHouse-Summary read path. The 4 INSERT
call sites in the three callers all route through chhttp.Client.Insert
now; SELECT through Query or QueryStream (the latter preserves the
QueryActive streaming behaviour). Net code: -200 LOC across the
three callers plus a 200-LOC chhttp package with its own tests.
5. Pixie service scaffold wired into cmd/main.go:
services.SetupService("adaptive-export", 50900) +
services.SetupSSLClientFlags() + services.PostFlagSetupAndParse() +
services.SetupServiceLogging(). Matches the pattern every other
pixie Go service uses. CheckServiceFlags() is deliberately skipped
(AE does not run a TLS gRPC server). Existing AE env-var reads
(ADAPTIVE_*) are untouched and still authoritative for tuning knobs.
All 14 adaptive_export internal packages pass go test (1 new chhttp,
13 unchanged), including the 3 differential oracle tests in
pxl/compile_test.go. arc lint OKAY for everything except 3 pre-existing
findings unrelated to this commit (loadgen has its own go.mod; one
QF1001 De Morgan's-law nit in reconcile_test.go from c9f19b6b1).
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: restore executable bits on harness scripts (post-header)
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: lint pass + restore @px load prefix in cmd/BUILD.bazel
User flagged on review 4536971862:
- cmd/BUILD.bazel:18 dropped the '@px' external-workspace prefix on
pl_build_system.bzl load. Restored — the standalone AE build pulls
pixie as @px and needs the qualifier.
Lint cleanup (PR-53 scope, no production code touched outside renames):
- chhttp/chhttp_test.go: errcheck on two w.Write calls + gofumpt on
the gotSettings declaration.
- passthrough/reconcile_test.go: staticcheck QF1001 (De Morgan's law)
on the conn_stats sink-drop assertion.
- script/script.go: rename ScriptId -> ScriptID (ST1003). Propagated
to pixie/pixie.go and cmd/main.go callsites.
- internal/e2e/BUILD.bazel and internal/trigger/BUILD.bazel: gazelle
drift — adding loadtest_test.go and clickhouse_internal_test.go to
srcs.
- k8s/00-sinks.yaml + gen-pod.tmpl.yaml: yamllint compliance —
document-start marker, dedent sequence items per .yamllint
indent-sequences=false, tighten flow-mapping spaces, collapse
multi-space after commas. YAML semantics unchanged.
- harness/stats.py: flake8 E501 — split a long line into two.
Final arc lint state on PR-53 file scope: 0 Errors, 14 Warnings
(SHELLCHECK SC2155/SC2086 in pre-existing harness scripts from
ae7b86f64, not introduced or modified by this commit). Pre-existing
loadgen typecheck failures (separate go.mod) are unaffected.
Signed-off-by: entlein <einentlein@gmail.com>
* adaptive_export: address user review #2 #4 #6 + 4 outstanding CodeRabbit items
User review 4536971862:
#2 passthrough/reconcile_test.go — strengthened with two new tests that
exercise the FULL chain (loop → real sink.ClickHouseHTTP → httptest
CH endpoint → reconcile recorder). TestTick_ReconcileCatchesCHSilentDrop
mimics CH's X-ClickHouse-Summary silent-drop shape (200 OK,
written_rows=0) and asserts the loop records WroteCount=0 with a
silent-drop attribution — the exact R6 (sink-layer loss) regression
the instrument exists to detect. TestTick_ReconcileAttributesCHFailureCorrectly
covers the 500-response branch. The pre-existing in-process-fake test
stays as the wiring check.
#4 pixie/pixie.go — added a long comment justifying the API-key auth
choice. pixie.Client targets the Pixie CLOUD (cloudpb's PluginService),
whose auth interceptor accepts pixie-api-key and rejects JWT service
tokens (those are for INSIDE-cluster vizier services). The
pixieapi.Adapter that talks to vizier directly already uses JWT via
jwtutils.GenerateJWTForService — same pattern as
cloud_connector/vizhealth/checker.go:111. So the split is intentional;
flipping pixie.go to JWT would break cloud auth, not improve it.
#6 pxl/queryfor — hardened escapePxL so a raw \n, \r, \t or NUL byte
in Target.Pod/Target.Namespace can't terminate the PxL string literal
and inject a new statement. Added TestQueryFor_RejectsInjectionInTargetFields
driving QueryFor with 7 adversarial pod/namespace shapes (newline,
single-quote-only, CR, backslash-escape-of-escape, NUL, tab) plus
the regex_match fallback path, asserting the output line count and
every statement's leading token. Extends existing
TestEscapePxL_TableDriven with the new escape mappings.
CodeRabbit oversights (verified each against current code):
CR r3379377432 (main.go) — wrapped the control-surface listener in
http.Server with Read/ReadHeader/Write/Idle timeouts so a slow
client can't pin a goroutine indefinitely.
CR r3379377607 (pixieapi.go) — switched direct-mode dial from
pxapi.WithDisableTLSVerification (env + addr-gated, brittle) to
pxapi.WithDirectTLSSkipVerify (added in PR #49 b523ce362 for the
same node-IP-dial scenario). Removed the cluster.local + PX_DISABLE_TLS
precondition in NewDirect; the always-skip semantics match the AE
deployment shape. Refactored the obsolete env-gate test.
CR r3379377645 (streaming/filter.go) — the deltaCh-close path returned
without calling disarm(), leaking the timer's goroutine on shutdown.
Now calls disarm() before return on chan-close.
CR r3426923299 (sink/clickhouse.go Record) — capped Record at a 2s
per-call context timeout. The 30s chhttp default was too long for
the scanner/passthrough/controller hot paths that call Record inline;
reconcile is best-effort by contract, so a stalled CH must not pin
the pull loop.
All 14 AE packages pass go test. arc lint: 0 Errors on PR-53 file
scope.
Signed-off-by: entlein <einentlein@gmail.com>
* test(harness): consolidate to one run-picture + e2e CI workflow
Finish the harness consolidation #53 started: adopt exp_matrix.sh (canonical
ALL-vs-DX reduction matrix) + nfr.sh (throughput/mem/verdict-latency) as the
single runners; cut the overlapping variants (ae_vs_all, exp_datavolume_extreme,
exp_dx_steering_reduction, exp_ae_nfr_benchmark, exp_pipeline_reconcile) and the
superseded standalone setup (deploy_ae, build_gen_image). README rewritten as the
single 'how to run' source: two families — fixture-isolation (run.sh + E-series)
and live-attack e2e (log4shell_fire → exp_matrix → nfr → exp_row_reconcile).
Add e2e_log4shell_soc.yaml: k3s + full SOC stack (Pixie/kubescape/ClickHouse/AE/dx
via k8sstormcenter/soc) on the oracle runner; asserts every canonical harness
script runs + dx rules in; profiles dx in real life (pprof CPU/heap + verdict
latency, uploaded). Uses existing repo secrets.
Signed-off-by: entlein <einentlein@gmail.com>
* revert(bazel): drop stray buildifier attribute-reorder in stirling container_images BUILD
This file is unrelated to adaptive_export — the only change was buildifier
alphabetizing container_type/bazel_sdk_versions (no functional change). Restore
main's version to keep #53's diff to real AE changes.
Signed-off-by: entlein <einentlein@gmail.com>
* ci: fix run-genfiles + run-container-lint on PR 53
run-genfiles: the PR had reordered the kwargs in stirling/.../container_images/
BUILD.bazel alphabetically (bazel_sdk_versions before container_type) — a
local buildifier or gazelle drift from when the file was first committed.
CI gazelle wants the original order (container_type first), so the diff
loop fails. Restored to origin/main's ordering. Local gazelle disagrees
with CI's expected output (tooling-version drift); CI is authoritative.
run-container-lint: two findings.
1. staticcheck QF1001 (De Morgan's law) in
pxl/queryfor_test.go:318. Rewrote the !(a || b || c || d) negated
disjunction as the equivalent !a && !b && !c && !d. Test behaviour
unchanged; verified locally with TestQueryFor_RejectsInjection.
2. golangci-lint typechecking failed on
src/e2e_test/adaptive_export_loadtest/tools/loadgen/cmd/{cleanloadgen,
httpsink}/main.go because that subtree carries its own go.mod and
does not resolve as a package under the root px.dev/pixie module.
Added the loadgen subtree to .arclint's exclude list. Same fix the
existing entries for other-module subtrees apply.
Local 'arc lint' on the PR-53 file scope: 0 Errors, 14 SHELLCHECK
Warnings + 26 SHELLCHECK Advice (all pre-existing in ae7b86f64
harness scripts; not introduced by this PR).
Signed-off-by: entlein <einentlein@gmail.com>
* ci: apply gazelle's actual kwarg order to stirling container_images
run-genfiles CI re-failed after f244ffc6c reverted this file to main's
ordering — turns out gazelle on this repo IS alphabetizing the kwargs
(bazel_sdk_versions before container_type), and CI runs 'gazelle fix'
then 'git diff' to catch any drift. So main's ordering is no longer
gazelle-stable; the file has to match gazelle's preference, not main's.
Verified locally with 'bazel run //:gazelle -- fix'; the file is now
idempotent (a second gazelle run produces no diff).
Signed-off-by: entlein <einentlein@gmail.com>
* ci: fix container-lint Errors on e2e workflow + new harness scripts
run-container-lint re-failed after the run-genfiles fix because two
files added on this branch (a03aa1570) had unfixed lint errors:
.github/workflows/e2e_log4shell_soc.yaml — 5 yamllint Errors:
- 1 indentation: list items under steps: must be parent-aligned per
the repo's .yamllint config (indent-sequences: false), not 2-indented.
Dedented every step item + its run: block by 2 spaces.
- 4 line-length (>120 chars): split the long kubectl-set-image, the
long grep-detection gate, the curl pprof URL, and the verdict-latency
grep across continuation lines. Semantics unchanged.
src/e2e_test/adaptive_export_loadtest/harness/{exp_matrix.sh, nfr.sh}
— missing Apache headers. Applied via arc lint --apply-patches; exec
bits restored.
Local arc lint on PR-53 file scope: 0 Errors, 14 SHELLCHECK Warnings
+ 26 Advice (all pre-existing in harness scripts, unchanged).
Signed-off-by: entlein <einentlein@gmail.com>
* ci: silence 6 SHELLCHECK Warnings to clear container-lint exit code
arc lint --apply-patches exits non-zero on Warning level too. Resolved
each: replaced unused 'for i/t in ...' loop vars with '_', split a
SC2155 declare-and-assign, dropped two never-referenced hip/pip
assignments.
Signed-off-by: entlein <einentlein@gmail.com>
* feat(ae-control): bearer-JWT auth + input validation (CodeRabbit followup)
AE control endpoints had no auth. Add it using the SAME shared lib the vizier
broker/PEM use (px.dev/pixie/src/shared/services/utils): SetAuth verifies a
bearer JWT via jwtutils.ParseToken (signature + audience), middleware on
Handler() with /healthz exempt. dx already mints this exact service JWT
(GenerateJWTForService, PL_JWT_SIGNING_KEY) — it just attaches it. No new
secret/crypto. Flag-gated (CONTROL_REQUIRE_AUTH + PL_JWT_SIGNING_KEY), default
OFF so it merges before dx sends the bearer; flip on after dx is updated.
Also: reject invalid t_end (<=0) and query windows (start>=end, non-positive).
Test mints a real JWT via the shared lib; asserts 401 on missing/bad token,
pass on valid, /healthz open.
* fix(ae): remaining CodeRabbit Majors (#53 followup)
- controller: don't fan out Pixie rows when attribution Sink.Write fails
(avoids orphaned rows; release in-flight slot, non-fatal). (controller.go:347)
- controller.Rehydrate: re-arm rev-1 pushPixieRows for restored windows so a
restart doesn't silently miss post-restart Pixie data. (controller.go:255)
- passthrough.pull: per-table timeout context so a hung dependency can't stall
the sweep / delay shutdown (covers serial+concurrent ticks). (passthrough.go:147)
- schema: TTL (30d) on ae_reconcile to cap append-only growth. (schema.sql:485)
Verified no-change-needed: adaptive_attribution ORDER BY (hostname, anomaly_hash)
is safe — anomaly_hash already encodes namespace+pod (anomaly/hash.go), so rows
never collapse across ns/pod. (schema.sql:430)
Already on ae-prod (no-op): filter timer leak, stats.py EXACT guard, sink async,
watermark paging, pixieapi WithDirectTLSSkipVerify, http.Server timeouts.
* fix(ae-trigger): escape a PollLimit-saturated watermark boundary (from #67)
ae-prod's boundary handling accumulates the seen-fingerprint set on a no-progress
tick, but if >PollLimit rows share one normalized event_time the SQL
(>= watermark ORDER BY time LIMIT N, no secondary key) returns the same N boundary
rows every poll → rows beyond N at that timestamp are never emitted (infinite
boundary). Detect all-skipped-at-capacity and advance the watermark by 1ns to make
forward progress (fingerprint dedup already tolerates the 1ns overlap).
Cherry-picked the trigger fix + its test from the stale CodeRabbit-chat PR #67
(687851d8c); dropped that PR's unrelated gen-pod.tmpl.yaml churn. #67 itself is
NOT mergeable (77 commits behind ae-prod, re-adds deleted terraform).
* feat(ae-control): TLS on the control surface (CONTROL_TLS) (#71)
Auth without TLS is half a control: tcpdump on dx→AE :9100 captured 720
cleartext `Authorization: Bearer` JWTs in 70s — the #68 token crosses the
CNI in plaintext. CONTROL_TLS=true now serves TLS with server.crt/key from
the service-tls-certs secret (broker/PEM already use it; dx skip-verifies).
Default-OFF for incremental rollout, symmetric to CONTROL_REQUIRE_AUTH.
Stacks on #68 (ae-followup-auth). dx client half: entlein/dx#88.
Co-authored-by: Entlein <eineintlein@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(ae-trigger): differential oracle vs naive reference (incremental → 73.7% cov)
The trigger's incremental kubescape-events pump has 3 moving parts
that must agree: watermark advancement, boundary fingerprint dedup,
and PollLimit-saturated draining (PR #67 fix). Existing tests cover
each in isolation. This new file pins them TOGETHER against the
simplest possible reference ("drain everything in event_time order,
dedupe by fingerprint, advance the cursor to max(event_time)"). If
the iterative trigger and the reference disagree on the set of
emitted rows for ANY poll sequence, one of the three parts is wrong.
Four oracle tests:
TestOracle_TriggerEmitsNaiveSet_StaggeredCorpus — 50 rows scattered
across distinct event_times, PollLimit=10 forces ≥5 polls; trigger
must emit exactly the 50 rows the naive reference computes.
TestOracle_PollLimitSaturation_AtCapacity — regression guard for PR
#67 (dfdc465a9): when EXACTLY PollLimit rows share a boundary
event_time, every one of them must emit, and the cursor must clear
the boundary for the next-event_time row that follows.
TestOracle_PollLimitOverflow_DocumentsLossBound — pins PR #67's
intentional trade-off: when >PollLimit rows share a boundary
event_time, the first PollLimit emit, then the 1ns escape advances
the cursor past the surplus. Lock-in test: if a future fix recovers
the surplus (good!) this test fails loudly so both can update in
lock-step instead of regressing silently.
TestOracle_BoundaryDedup_NoDuplicates — when CH returns the same
boundary row in two consecutive polls (real production case: a new
row lands at the same event_time after a previous poll's cursor
advanced past it), the seenAtBoundary map must filter the duplicate.
Coverage: trigger 71.6% → 73.7% (statement-level). The oracle's value
isn't in statement count — it's in property-level coverage on
invariants the per-feature tests can't enforce together.
Build.bazel: adds oracle_test.go to the existing trigger_test target.
* ci: fix vizier-release Build Release runner label (oracle-16cpu → oracle-vm-16cpu)
Merge from main re-introduced the deprecated 'oracle-16cpu-64gb-x86-64'
label on the Build Release job. That label doesn't resolve in the
fork's runner pool, so aeprod22's release workflow sat queued
indefinitely. update-gh-artifacts-manifest at L143 already had the
'-vm-' variant; align Build Release at L18 to match. Same fix the
fork applied earlier in 21d536e3d for the standalone vizier_release
workflow — main keeps regressing it.
* ci: fix merge-conflict regressions from main pickup (runner labels, ui.bzl, fork cockpit)
The merge of main into ae-followup-auth (0c657514a) silently took the
upstream version on a swath of files where the fork had previously
deviated. This commit reverts the merge's regressions back to the
fork-correct state (origin/ae-prod) AND completes the runner-label
sweep so every release/mirror/perf workflow uses the same -vm-16cpu
label the fork's runner pool actually has.
1. Runner labels — main re-introduced 'oracle-16cpu-64gb-x86-64' and
'oracle-8cpu-32gb-x86-64' on 7 workflows. Fork's pool only resolves
'oracle-vm-16cpu-64gb-x86-64'; both stale labels sit queued forever.
Fixed: cli_release.yaml (L18+L212), cloud_release.yaml (L18),
mirror_demos.yaml (L12), mirror_deps.yaml (L12),
mirror_releases.yaml (L13), operator_release.yaml (L18+L143),
perf_common.yaml (L37+L60). vizier_release.yaml L18 was fixed
already in 0fd9c3f21.
2. bazel/ui.bzl — main reverted PR #64's webpack-build fixes that
broke release/cloud/v0.0.10 with 'export: `18': not a valid
identifier'. Restored: 'set -x' for action-shell tracing, PATH
that puts /opt/px_dev/tools/node/bin FIRST, 'hash -r', the
STABLE_BUILD_TAG|BUILD_TIMESTAMP allowlist sed (vs the wildcard
that word-splits FORMATTED_DATE), and use_default_shell_env=True
so --incompatible_strict_action_env doesn't strip yarn from PATH.
3. 28 fork-cloud config files — main's PR #2391 (cert-manager
migration) deleted private/cockpit/*,
terraform/kubernetes/auth0/*, terraform/kubernetes/cloud_deps/*,
.sops.yaml, private/skaffold_cloud.yaml. These are still load-
bearing for the AOCC pixie-cloud deployment; the fork hasn't
migrated to cert-manager-compatible secrets yet (PR #2391's
monitor.go fallback path is in place, so adoption is the
follow-up, not a blocker). Restored all 28 from origin/ae-prod.
Genuine main pickups that were CORRECT to keep (no fix needed): the
src/utils/shared/k8s/{apply,delete}.go import-order +
sets.New[string] generics migration,
src/operator/controllers/monitor.go's cert-manager secret fallback,
and the src/carnot/BUILD.bazel + src/carnot/exec/BUILD.bazel
additions.
* fix(ae): address 13 CodeRabbit Go-code findings on PR 68
11 TRUE + 2 PARTIAL CodeRabbit comments verified against current code;
all valid. This commit lands every one of them as a discrete change,
keeps the existing tests green, adds new tests where the contract
itself changed.
🔴 Real bugs:
controller/controller.go: handle() now SNAPSHOTS c.active[hash]
before mutation and ROLLS BACK on sink.Write failure. Without
this, a failed persist left c.active extended; an already-running
pushPixieRows would re-snapshot and fan out data based on an
attribution row that never landed in CH. Updated
TestController_SinkErrorNonFatal to pin the new rollback contract
(active==0 after sink error) and added a writeAttempts() observer
on fakeSink so the test doesn't race.
sink/fastencode.go: appendJSONValue now rejects NaN/+Inf/-Inf
floats with errFastEncodeUnsupported (triggers the encoding/json
fallback). Previously strconv.AppendFloat emitted invalid JSON
tokens that made CH reject the whole batch.
streaming/writer.go: flush() keeps the row buffer on write failure
(so the next attempt retries instead of silently dropping), and
the shutdown branch uses context.Background() for the final flush
so it isn't fast-failed by the already-cancelled parent ctx.
control/server.go: decode() now wraps the body in
http.MaxBytesReader(w, body, 4 MiB) so an oversized JSON payload
on the public(-ish) control endpoint can't OOM the operator. The
JWT auth still gates access — this hardens what a holding-an-
acceptable-JWT attacker can do.
🟠 Lower-impact:
cmd/main.go: isOperatorManagedScript matches the EXACT builtin
names (no "ch-" prefix), so a user-authored script named
"ch-something-custom" can't get deleted under
INSTALL_PRESET_SCRIPTS=true.
chhttp/chhttp.go: QueryStream now uses a separate http.Client with
no Timeout. Go's http.Client.Timeout covers body reads, so
reusing the 30s default would silently truncate a multi-MB
active-set rehydrate. Stream callers must bound via ctx.Deadline.
passthrough/passthrough.go: tickConcurrent's precompile-skip branch
now calls l.rec(...) so the compiled and legacy paths produce
identical reconcile-row counts. Previously the compiled path
silently dropped a table → invisible divergence.
trigger/oracle_test.go: COLLECT: labelled break stops the busy-spin
on deadline expiry. Bare "break" only exits the select, leaving
the for-loop to busy-spin until the count condition is reached.
🟡 Cosmetic / cleanup:
control/server_test.go: TestBadInputRejected now pins t_end<=0 on
/export/start AND inverted/zero window on /query — 4 new
assertions for validators that existed but were untested.
pixieapi/pixieapi.go: TODO comment marking the bounded pxapi.Client
leak for follow-up when direct-mode throughput crosses ~1 q/s.
sink/clickhouse.go: "sink: pixie write completed" demoted Info
→ Debug. One log per Pixie batch in fan-out paths was avoidable
log-volume pressure; the silent-drop guard below still fires
loud on the actual failure mode.
sink/integration_test.go: per-table 15s ctx (was a shared 60s for
the entire loop) so a slow early table can't starve later ones.
trigger/clickhouse.go: PollLimit docstring now matches the
0→10000 rewrite in New(); "unlimited" is no longer a documented
option.
Local gates: build exit 0, go test 14/14 packages pass,
tools/linters/lint_like_ci.sh OKAY (0 Errors, 0 Warnings).
* ci: carve out private/cockpit + terraform/credentials/cockpit from filename-linter
The fork's filename-linter (inherited from upstream pixie-io) rejects
any PR diff that touches a path containing 'private'. The restored
fork-cloud config from the main-merge fix (cb81ecd72) added 12
private/cockpit/* + 1 terraform/credentials/cockpit/* files, which
upstream-main had deleted via PR #2391 — the linter then correctly
saw them as 'new private/* additions' and failed.
These specific paths are LEGITIMATE fork-cloud deployment config that
pre-dates the upstream linter; the original 'no private/* leaks'
intent stays in place for everything else via two negative-glob
exceptions.
* Revert "ci: carve out private/cockpit + terraform/credentials/cockpit from filename-linter"
This reverts commit 7f43c514b7f007f30bf176cb774763e69140f411.
* ci: rename private/{cockpit,skaffold_cloud.yaml} so filename-linter accepts the fork-cloud config upstream
Honest fix for the merge-regression: the fork's filename-linter rejects
any PR diff touching a path containing 'private' (an upstream-pixie-io
guard against secrets leaking into OSS PRs). The fork is a CONTRIBUTOR
that upstreams, so the guard applies; the prior 'restore from ae-prod'
fix legitimately re-added private/cockpit/* (which upstream-main #2391
deleted via cert-manager migration) but tripped the linter.
Reverts the lazy carve-out fix (c579e48c4 reverted 7f43c514b) and does
the structural rename instead.
Moves:
private/cockpit/ -> cockpit/
private/skaffold_cloud.yaml -> skaffold/skaffold_cloud.yaml
(collapsed with the upstream-
resurrected skaffold/skaffold_cloud.yaml
— the fork's private/ version was
the maintained superset)
Reference updates:
cockpit/kustomization.yaml ../../k8s/cloud/... -> ../k8s/cloud/...
(depth-1 after the rename)
skaffold/skaffold_cloud.yaml:84 - private/cockpit -> - cockpit
Linter intent preserved: any future PR that touches a real 'private/*'
path still fails — the guard's correct semantics are restored.
* terraform: drop fork IaC from PR 68 — belongs on main, not this PR
cb81ecd72 re-added the terraform/ tree (16 files, 9817 lines) while
fixing regressions from the origin/main pickup, because the main tip
53d09cc8d had deleted it. That restoration is unrelated to the
adaptive_export auth work in this PR; it is now carried on its own
branch (restore-terraform-main, off origin/main) for a separate PR
into main. Removing it here keeps PR 68 scoped to the AE feature.
* fork-infra: drop cockpit, .sops.yaml, e2e workflow from PR 68
These were re-added on this branch while fixing the origin/main pickup
regression (cb81ecd72 + the d94624a1c cockpit rename), but they are fork
infra deleted from main by 53d09cc8d, not part of the adaptive_export
auth work. Carried on restore-fork-infra-main for a separate PR into
main. skaffold/skaffold_cloud.yaml is left in place — it is an upstream
file, not a clean fork-only add.
* adaptive_export: drop out-of-scope bazel/ui.bzl (fork UI-build fix belongs in UI PR #62)
* adaptive_export_loadtest: replace shell harness with a Go TDD suite (fixtures + KPIs)
Collapse the 12-script shell harness (+ stats.py) into a table-driven Go test
suite under suite/: fixtures (control-plane experiments), KPI asserts via
testify/require (reproducibility/reconcile/reduction), and one runner driving a
deployed AE image on a rig (live-gated by AELOAD_LIVE). Removes all CVE names and
adversarial verbs from the harness; kubescape/k8s wire tokens stay literal.
* adaptive_export_loadtest: drop the arbitrary reduction-threshold assert
Volume reduction is a measured outcome to report, not an invariant — asserting
'reduction >= minPct' gates on a made-up threshold. Removed RequireReductionAtLeast;
the reduction test now measures + logs the firehose->steered delta without a pass/fail gate.
* adaptive_export: standardize event_time on nanoseconds (schema + loadtest)
kubescape_logs.event_time is unix NANOSECONDS (Vector kubescape_enrich emits ns).
The DDL read it as seconds via toDateTime() -> ns overflow -> wrong partitions +
rows born already-TTL-expired (the F1/F8 unit bug). Fix, matching soc clickhouse-lab:
PARTITION BY toYYYYMM(fromUnixTimestamp64Nano(event_time))
TTL toDateTime(fromUnixTimestamp64Nano(event_time)) + INTERVAL 30 DAY
Also raise the protocol tables' derived event_time DateTime64(3) (ms) -> DateTime64(9)
(ns), matching time_. The loadtest suite now injects UnixNano and documents C1 as ns.
* adaptive_export_loadtest: add evidence.sh (stack integration + nanos + loadtest) + AELOAD_REPS knob
evidence.sh captures, on a live rig: pipeline components Running (kubescape→vector→
node-agent→ClickHouse→AE), every forensic_db table created, event_time NANOSECONDS
end-to-end (kubescape_logs UInt64/fromUnixTimestamp64Nano + protocol DateTime64(9)),
the pipeline flowing, and the AE control-plane reproducibility loadtest. Sandbox-safe
(no shell sleep; ClickHouse via kubectl exec). suite_test.go gains AELOAD_REPS for
fast runs. Verified live on rig 6a58ec21: PASS=29 FAIL=0.
* adaptive_export_loadtest/suite: commit go.sum (reproducible testify build)
* adaptive_export_loadtest: test EVERY forensic_db timestamp is nanoseconds
schema_test.go: per-(table,column) fixtures asserting every timestamp column is
nanosecond-precision — UInt64 unix-ns (kubescape_logs.event_time, watermark,
dx_attack_graph.event_time) or DateTime64(9) (all protocol time_/event_time,
adaptive_attribution t_start/t_end/last_seen, ae_reconcile, alerts). Plus
TestNoCoarserTimestamps: dynamic backstop that fails on ANY DateTime coarser than
(9) in forensic_db. evidence.sh runs both. Verified live: 37/37 columns PASS.
* adaptive_export_loadtest: drop evidence.sh (the Go schema + reproducibility tests are the suite)
* adaptive_export_loadtest: java-poc disease calibration (real e2e)
TestJavaPocCalibration assumes the full stack (kubescape+vector+ClickHouse+
adaptive_export+dx+java-poc) and induces the java-poc listeriosis disease once,
asserting each pipeline stage lights up as a before->after delta:
1 kubescape detects (R0001 spawn) 2 vector->ClickHouse carries it (fresh ns)
3 adaptive_export captures forensics (backend->pathogen:1389 LDAP egress)
4 dx rules in (non-blind verdict).
Pathogen vocabulary (pathogen/disease/Specimen/listeriosis); external wire literal.
Live+e2e gated (AELOAD_LIVE=1 AELOAD_E2E=1); sandbox-safe 3s polls, no long sleep.
Nothing mocked — unlike the control-plane suite, the signal originates from a real
workload and flows through every component.
* test(e2e): single pixie-native command to deploy the non-Pixie stack
The calibration/e2e suite assumed a hand-installed stack (kubescape + vector +
ClickHouse + the java-poc apps), which made runs non-reproducible. Add ONE
command that stands the whole non-Pixie environment up, from each component's
golden-source repo (one source of truth per component):
- skaffold.yaml (module e2e-nonpixie): requires the soc stack Skaffold
(ClickHouse -> kubescape -> vector) then the bob java-poc apps Skaffold
(SBoBs -> workloads). Pixie itself (pl) is overlaid separately.
- suite/deploy.go: EnsureE2EStack(t) invokes it (AELOAD_DEPLOY=1; AELOAD_CLEAN=1
for a from-scratch redeploy keeping pl + dx). TestJavaPocCalibration calls it,
so the test deploys its own world when asked and otherwise assumes a live rig.
Proven on a k3s rig: `skaffold deploy -m java-poc-apps -p k3s` applies
namespaces -> SBoBs -> 5 workloads and waits them Ready.
Refs k8sstormcenter/soc#231, k8sstormcenter/bob#152.
* test(e2e): confusion matrix + all-pod KPIs; robust stage4 + CH retry
- confusion_test.go (TestStackEvidence): extracts per-workload KPIs (R0001/R0010,
attribution, egress) across the whole java-poc stack and scores dx verdicts as a
confusion matrix vs ground truth (only backend is malignant, only under log4shell).
Reports always; asserts under AELOAD_MATRIX=1. Catches cross-scenario FPs — e.g.
backend ruled_in [argocd-malicious-render] (a dx-repo issue) → FP=1, precision=0.50.
- calibration_test.go stage4: poll the CURRENT backend pod's ruled_in (150s) instead
of a saturated cumulative count that missed dx's workup lag; all-dx-pod aware.
waitUntil now returns bool.
- harness.go chReq: retry transient transport errors + 5xx (a kubectl port-forward
EOF was flaking dedup-extend).
Verified live on k3s: full suite green (schema + reproducibility std=0 + calibration);
matrix report renders TP=1 FP=1 FN=0 TN=4.
---------
Signed-off-by: entlein <einentlein@gmail.com>
Co-authored-by: Entlein <eineintlein@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: pixie-agent <noreply@local>
Co-authored-by: pixie-agent <croedig@sba-research.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary: PEM direct-query gRPC endpoint (entlein/dx#29). Make the metadata-connected vizier-pem serve api.vizierpb.VizierService.ExecuteScript directly over gRPC, JWT-authenticated, so dx queries the node-local PEM with no broker hop. Ports the standalone_pem capability with two upgrades: metadata-connected reuse of the live PEM Carnot + agent metadata (closes #15), and HS256 service-token auth via the cluster jwt-signing-key (no longer insecure). Drain fix (execution_and_timing_info -> QueryData.execution_stats wire-roundtrip + skip payload-less responses) closes the unimplemented type stream error caught on live PEM soak. Fail-soft direct-query startup (try/catch + step 1/6 to 6/6 breadcrumbs) so init failure can never crashloop the data plane. Manager::Init refuses to start with empty PL_JWT_SIGNING_KEY (catches the jwt::SigningError that crashloop'd the stock 0.14.17 PEM).
Relevant Issues: entlein/dx#29 (PEM direct-query)
Type of change: /kind feature
Test Plan: bazel test //src/vizier/services/agent/pem:direct_query_server_test (auth-negative + ValidToken_TrivialQuery_StreamsRows green); bazel test //src/vizier/services/agent/shared/manager/... (5/5 pass, JWT guard wired through Manager::Init); vizier-release CI builds pemdq6 image (commit 50dffb0 / tag release/vizier/v0.14.19-pemdq6); live PG soak