Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/carnot/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ pl_cc_library(
"carnot_executable.cc",
],
),
visibility = [
"//src/carnot:__subpackages__",
"//src/experimental:__subpackages__",
"//src/vizier/services/agent:__subpackages__",
],
deps = [
"//src/carnot/exec:cc_library",
"//src/carnot/exec/ml:cc_library",
Expand Down
5 changes: 4 additions & 1 deletion src/carnot/exec/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

load("//bazel:pl_build_system.bzl", "pl_cc_binary", "pl_cc_library", "pl_cc_test", "pl_cc_test_library")

package(default_visibility = ["//src/carnot:__subpackages__"])
package(default_visibility = [
"//src/carnot:__subpackages__",
"//src/vizier/services/agent:__subpackages__",
])
Comment on lines +19 to +22

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be best to avoid making this package broadly open to all //src/vizier/services/agent subpackages. I need to think about how to scope this more tightly, but we do have a similar pattern in here.

I think we need to consider this for the other package visibility changes as well. I'll have to think about this more before I have any proposals on how to structure it better.


pl_cc_library(
name = "cc_library",
Expand Down
1 change: 1 addition & 0 deletions src/carnot/udf/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ load("//bazel:pl_build_system.bzl", "pl_cc_binary", "pl_cc_library", "pl_cc_test
package(default_visibility = [
"//src/carnot:__subpackages__",
"//src/vizier/funcs:__subpackages__",
"//src/vizier/services/agent:__subpackages__",
])

pl_cc_library(
Expand Down
52 changes: 52 additions & 0 deletions src/vizier/services/agent/pem/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ load("//bazel:pl_build_system.bzl", "pl_cc_binary", "pl_cc_library", "pl_cc_test

package(default_visibility = ["//src/vizier:__subpackages__"])

# Compile-time kill switch for the direct-query endpoint.
# Operators who do not want the feature available even as a disabled-by-default
# option build with `bazel build … --//src/vizier/services/agent/pem:direct_query=disabled`
# (or `--define=PX_PEM_DIRECT_QUERY=disabled`). This propagates
# `-DPX_PEM_DIRECT_QUERY_DISABLED` into cc_library; the entire feature body
# in direct_query_server.cc and pem_manager.cc is `#ifndef`'d out and
# stub methods return UNIMPLEMENTED. See DIRECT_QUERY_SECURITY.md.
config_setting(
name = "direct_query_disabled",
define_values = {"PX_PEM_DIRECT_QUERY": "disabled"},
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try the bazel build … --//src/vizier/services/agent/pem:direct_query=disabled build and verify it works? I believe you need a bool_flag associated with this. I think it would be better as a bool_flag so it can work with --//src/vizier/services/agent/pem:direct_query=false/true


pl_cc_library(
name = "cc_library",
srcs = glob(
Expand All @@ -30,7 +42,27 @@ pl_cc_library(
],
),
hdrs = glob(["*.h"]),
defines = select({
":direct_query_disabled": ["PX_PEM_DIRECT_QUERY_DISABLED"],
"//conditions:default": [],
}),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use local_defines otherwise it will propagate to every transitive dependency.

deps = [
# direct-query server deps (direct_query_server.{h,cc}):
"//src/api/proto/vizierpb:vizier_pl_cc_proto",
"//src/carnot",
"//src/carnot:cc_library",
"//src/carnot/carnotpb:carnot_pl_cc_proto",
"//src/carnot/exec:cc_library",
"//src/carnot/funcs:cc_library",
"//src/carnot/planner/compiler:cc_library",
"//src/carnot/planpb:plan_pl_cc_proto",
"//src/carnot/udf:cc_library",
# HS256 verify uses BoringSSL HMAC directly and parses claims via rapidjson.
"@boringssl//:crypto",
"@com_github_grpc_grpc//:grpc++",
"@com_github_rlyeh_sole//:sole",
"@com_github_tencent_rapidjson//:rapidjson",
# existing PEM deps:
"//src/carnot/planner/dynamic_tracing/ir/logicalpb:logical_pl_cc_proto",
"//src/integrations/grpc_clocksync:cc_library",
"//src/shared/tracepoint_translation:cc_library",
Expand All @@ -51,6 +83,26 @@ pl_cc_test(
],
)

# TDD contract for the PEM direct-query endpoint.
# See DIRECT_QUERY_CONTRACT.md for the behavioral spec.
pl_cc_test(
name = "direct_query_server_test",
srcs = ["direct_query_server_test.cc"],
deps = [
":cc_library",
"//src/api/proto/vizierpb:vizier_pl_cc_proto",
"//src/carnot",
"//src/carnot:cc_library",
"//src/carnot/exec:cc_library",
"//src/carnot/funcs:cc_library",
"//src/carnot/udf:cc_library",
"//src/table_store:cc_library",
"@com_github_arun11299_cpp_jwt//:cpp_jwt",
"@com_github_grpc_grpc//:grpc++",
"@com_github_rlyeh_sole//:sole",
],
)

pl_cc_binary(
name = "pem",
srcs = ["pem_main.cc"],
Expand Down
89 changes: 89 additions & 0 deletions src/vizier/services/agent/pem/DIRECT_QUERY_CONTRACT.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# PEM direct-query gRPC endpoint — contract

## Why

Today in-cluster services read PEM data by querying the **vizier-query-broker**
(the standard `ExecuteScript` path). That works and is the primary path. This
feature adds the *node-local* alternative: make the normal `vizier-pem` itself
serve `ExecuteScript` directly over gRPC, so an on-node client can query its
node-local PEM with no broker hop and no cloud dependency.

This capability was already proved in the experimental `standalone_pem`
(`src/experimental/standalone_pem/vizier_server.h` — `px::vizier::agent::VizierServer`
implementing `api::vizierpb::VizierService::ExecuteScript` against a local Carnot).
The two differences for the real PEM:

1. **Metadata-connected.** The normal PEM has the metadata service, so per-pod PxL
filters (`df[df.ctx['pod'] == ...]`) resolve — the gap that made standalone_pem
return empty per-pod results. Reuse the PEM's existing Carnot +
table_store + metadata state; do **not** stand up a second Carnot.
2. **Authenticated.** standalone_pem was insecure (`WithDirectCredsInsecure`). The
real PEM is in `pl` and must require a **valid cluster service JWT** (the same
`jwt-signing-key` used by kelvin, query-broker, and metadata-server).

## The endpoint

- Service: `px.api.vizierpb.VizierService` (the generated gRPC service).
- Method implemented: **`ExecuteScript`** (server-streaming). Mutations/tracepoints
are **out of scope** — return `UNIMPLEMENTED` for `req.mutation()==true`.
(standalone_pem handles mutations; the read path never mutates.)
- Transport: gRPC over TLS using the in-cluster self-signed CA
(`SSL::DefaultGRPCServerCreds()`). Insecure fallback only when `PL_DISABLE_SSL=1`
is explicitly set for a dev/soak cluster.

## Config (flags / env) — gated OFF by default

| flag / env | default | meaning |
|-------------------------------------|---------|----------------------------------------------------|
| `--direct_query_enabled` / `PL_PEM_DIRECT_QUERY_ENABLED` | `false` | master switch; when false the port is never opened |
| `--direct_query_port` / `PL_PEM_DIRECT_QUERY_PORT` | `50305` | gRPC listen port for the direct-query service |
| `--direct_query_jwt_signing_key` / `PL_JWT_SIGNING_KEY` | `""` | HMAC key the bearer JWT must verify against |

Default-off so existing PEM deployments are byte-for-byte unchanged until opted in.
Opt out at runtime with `PL_PEM_DIRECT_QUERY_ENABLED=false`, or compile it out with
`--//src/vizier/services/agent/pem:direct_query=disabled`.

## Auth contract

Every `ExecuteScript` call MUST present `authorization: Bearer <jwt>` metadata.
The JWT is verified with `PL_JWT_SIGNING_KEY` and must:
- have a valid signature (HS256) against the signing key,
- be unexpired (`exp` in the future),
- carry a service/audience claim acceptable to vizier
(minted via `GenerateJWTForService` in `src/shared/services/utils`).

Missing/invalid/expired token → `grpc::StatusCode::UNAUTHENTICATED`. No token must
ever fall through to query execution.

## Behavioral contract (the executable spec → `direct_query_server_test.cc`)

1. **flag-off → no listener.** With `direct_query_enabled=false`, nothing listens on
the port; the PEM starts exactly as today.
2. **flag-on → serves ExecuteScript.** With it enabled + a signing key set, a gRPC
client with a valid bearer JWT gets a streamed response (status OK) for a trivial
PxL (e.g. `import px; px.display(px.DataFrame('http_events'))`).
3. **auth required.** Same call with (a) no token, (b) a token signed by the wrong
key, (c) an expired token → each `UNAUTHENTICATED`, no rows.
4. **metadata-connected filter.** A PxL with a per-pod filter returns only that pod's
rows (proves the metadata gap from standalone_pem is closed on the real PEM). May
be an integration test tagged `requires_metadata` if a unit Carnot fixture can't
supply pod context.
5. **mutation rejected.** `req.mutation()==true` → `UNIMPLEMENTED` (scope guard).
6. **no regression.** The existing PEM agent registration / Carnot / Stirling path is
unchanged when the flag is off (assert via the existing PEM smoke/unit tests).

## Client integration (informational)

The existing in-cluster clients already have the pieces:
- **JWT mint**: `GenerateJWTForService` in `src/shared/services/utils/jwt.go`; mount
the `pl-cluster-secrets/jwt-signing-key` via `secretKeyRef`.
- **gRPC metadata**: attach `authorization: Bearer <jwt>` to each call.
- **Address**: `<HOST_IP>:50305` (HOST_IP via downward API, or the pod's node IP).
- **TLS**: use `WithDisableTLSVerification` against the cluster's self-signed CA,
matching the broker path.

## Done =

`direct_query_server_test.cc` green under `bazel test`, PEM image builds via the
vizier-release workflow, and a live cluster shows an on-node client ruling in an
e2e scenario off the node-local PEM with the same verdict it gets via the broker.
Loading
Loading