diff --git a/charts/nebari-llm-serving/crds/llmmodel-crd.yaml b/charts/nebari-llm-serving/crds/llmmodel-crd.yaml index 2575745..66f2fe6 100644 --- a/charts/nebari-llm-serving/crds/llmmodel-crd.yaml +++ b/charts/nebari-llm-serving/crds/llmmodel-crd.yaml @@ -1336,6 +1336,17 @@ spec: serving: description: serving configures the vLLM serving layer properties: + command: + description: |- + command overrides the container command for the vLLM container. + Defaults to the standard vLLM OpenAI-server entrypoint + (python3 -m vllm.entrypoints.openai.api_server), which the default + serving image requires because its entrypoint is the NVIDIA CUDA + wrapper with no default CMD. Set this when a custom serving image + needs a different launcher; vllmArgs are appended as arguments. + items: + type: string + type: array dataParallelism: default: 1 description: dataParallelism controls data parallelism diff --git a/charts/nebari-llm-serving/templates/operator-clusterrole.yaml b/charts/nebari-llm-serving/templates/operator-clusterrole.yaml index 086529b..6bc8112 100644 --- a/charts/nebari-llm-serving/templates/operator-clusterrole.yaml +++ b/charts/nebari-llm-serving/templates/operator-clusterrole.yaml @@ -41,6 +41,13 @@ rules: - apiGroups: ["inference.networking.k8s.io"] resources: ["inferencepools"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] + # The operator creates a per-model EPP Role granting these + # inference.networking.x-k8s.io permissions (see reconcilers/inferencepool.go). + # RBAC privilege-escalation prevention requires the operator to already hold + # any permission it grants, so it must carry them here too. + - apiGroups: ["inference.networking.x-k8s.io"] + resources: ["inferenceobjectives", "inferencemodelrewrites", "inferencepools", "inferencepoolimports"] + verbs: ["get", "list", "watch"] - apiGroups: ["aigateway.envoyproxy.io"] resources: ["aigatewayroutes", "aiservicebackends", "backendsecuritypolicies"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] diff --git a/dev/manifests/operator.yaml b/dev/manifests/operator.yaml index 3f3bc3c..75dce76 100644 --- a/dev/manifests/operator.yaml +++ b/dev/manifests/operator.yaml @@ -56,6 +56,14 @@ rules: - apiGroups: ["inference.networking.k8s.io"] resources: ["inferencepools"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] + # The operator creates a per-model EPP Role granting these + # inference.networking.x-k8s.io permissions (see reconcilers/inferencepool.go). + # RBAC privilege-escalation prevention requires the operator to already hold + # any permission it grants, so it must carry them here too (mirrors the Helm + # chart ClusterRole; #121). + - apiGroups: ["inference.networking.x-k8s.io"] + resources: ["inferenceobjectives", "inferencemodelrewrites", "inferencepools", "inferencepoolimports"] + verbs: ["get", "list", "watch"] - apiGroups: ["aigateway.envoyproxy.io"] resources: ["aigatewayroutes", "aiservicebackends", "backendsecuritypolicies"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] diff --git a/dev/manifests/passthrough-test-model.yaml b/dev/manifests/passthrough-test-model.yaml index 42e59c0..73828d8 100644 --- a/dev/manifests/passthrough-test-model.yaml +++ b/dev/manifests/passthrough-test-model.yaml @@ -5,7 +5,9 @@ # # Once reconciled (kubectl -n llm-operator-system get passthroughmodel), reach # it through the gateway. The external endpoint uses API-key auth, so inject a -# client key into the api-keys Secret to skip the key-manager: +# client key into the api-keys Secret to skip the key-manager. A curl right +# after the patch can 403 until the operator re-renders the SecurityPolicy +# allow-list from the Secret (a few seconds) - retry on 403: # kubectl -n llm-operator-system patch secret openrouter-api-keys --type merge \ # -p '{"stringData":{"localtester":"sk-localtest-abc123"}}' # kubectl -n envoy-gateway-system port-forward svc/envoy-...-nebari-gateway 8443:443 & diff --git a/dev/manifests/test-model.yaml b/dev/manifests/test-model.yaml index 1c4be39..ca9e433 100644 --- a/dev/manifests/test-model.yaml +++ b/dev/manifests/test-model.yaml @@ -23,6 +23,10 @@ spec: memory: "256Mi" serving: image: mock-vllm:dev + # The operator defaults the container command to the real vLLM entrypoint + # (required by the llm-d-cuda serving image); the mock image runs a plain + # Python HTTP server instead, so override it here. + command: ["python", "/server.py"] replicas: 1 monitoring: enabled: false diff --git a/dev/mock-vllm/server.py b/dev/mock-vllm/server.py index 2e36475..0fc2dc6 100644 --- a/dev/mock-vllm/server.py +++ b/dev/mock-vllm/server.py @@ -1,4 +1,4 @@ -"""Mock vLLM server that responds to health check and model list endpoints.""" +"""Mock vLLM server that responds to health check, model list, and chat endpoints.""" import json from http.server import HTTPServer, BaseHTTPRequestHandler @@ -19,6 +19,32 @@ def do_GET(self): self.send_response(404) self.end_headers() + def do_POST(self): + # Minimal OpenAI-compatible chat-completion response so the dev stack can + # exercise the full request path (gateway auth/routing -> backend -> 200) + # without a real model. + if self.path in ("/v1/chat/completions", "/v1/completions"): + length = int(self.headers.get("Content-Length", 0) or 0) + if length: + self.rfile.read(length) + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.end_headers() + body = json.dumps({ + "id": "chatcmpl-mock", + "object": "chat.completion", + "model": "test/tiny-model", + "choices": [{ + "index": 0, + "message": {"role": "assistant", "content": "ok"}, + "finish_reason": "stop", + }], + }) + self.wfile.write(body.encode()) + else: + self.send_response(404) + self.end_headers() + def log_message(self, format, *args): pass # suppress logs diff --git a/docs/src/content/docs/architecture.md b/docs/src/content/docs/architecture.md index 2ddef5e..03062ce 100644 --- a/docs/src/content/docs/architecture.md +++ b/docs/src/content/docs/architecture.md @@ -69,7 +69,7 @@ All resources live in the `LLMModel`'s own namespace, which (per the validating | InferencePool + EPP | Intelligent inference scheduling | | AIGatewayRoute (external) | External endpoint with token counting, rate limiting | | AIGatewayRoute (internal) | Internal endpoint with token counting, rate limiting | -| SecurityPolicy (external) | apiKeyAuth referencing the per-model Secret in the same namespace | +| SecurityPolicy (external) | apiKeyAuth pooling every model's api-keys Secret (all same-namespace) plus a deny-by-default authorization allow-list of this model's own client IDs | | SecurityPolicy (internal) | JWT validation with group claim matching against `access.groups` | | Secret (API keys) | Per-model API key store (created by operator, data managed by key manager) | | ConfigMap (key metadata) | Per-model metadata for API keys (creator, timestamp, description) | @@ -153,7 +153,7 @@ Most operator work is per-`LLMModel` and flows through `LLMModelReconciler`. A s These are reconciled by `ClusterTLSSingleton`, a `manager.Runnable` (not a controller-runtime Reconciler) that runs under leader election with a 5-minute resync after an initial reconcile on leader acquisition. It sets no `OwnerReferences` on its targets: the Certificate is cluster-scoped in spirit even though it lives in a namespace, and the Gateways are owned out-of-band by the platform - the operator only mutates their `.spec.listeners` slice in place, matched by listener name. On operator uninstall, the Certificate and listeners stay behind so in-flight traffic continues to terminate TLS while a new pack version rolls. -Use this pattern for any future cluster-wide concern; do not co-locate cluster-wide state inside `LLMModelReconciler`. The split keeps per-model reconciles fast and cluster-singleton reconciles rare and idempotent. +Use this pattern for any future cluster-wide concern; do not co-locate cluster-wide state inside `LLMModelReconciler`. The split keeps per-model reconciles fast and cluster-singleton reconciles rare and idempotent. One deliberate exception: the pooled `apiKeyAuth.credentialRefs` on each external SecurityPolicy are fleet-wide input to a per-model resource - see the external endpoint section for why that pooling cannot be hoisted to a gateway-level policy. ### Shared TLS and Gateway listeners @@ -169,7 +169,7 @@ Escape hatch: set `platform.gateway.manageSharedListeners: false`. The operator Every model on the cluster shares a single hostname pair: `llm.` for the external endpoint and `llm-internal.` for the internal endpoint. One TLS certificate covering both names serves every model. -Per-model routing happens via the `x-ai-eg-model` request header. The Envoy AI Gateway controller automatically extracts the `model` field from the JSON request body and surfaces it as that header. Each `LLMModel` produces an `AIGatewayRoute` whose single rule matches both the shared `Host` header and `x-ai-eg-model: ` exactly. Clients set `model` in the request body, the same way they would against api.openai.com - no per-model URL is required. +Per-model routing happens via the `x-ai-eg-model` request header. The Envoy AI Gateway controller automatically extracts the `model` field from the JSON request body and surfaces it as that header. Each `LLMModel` produces an `AIGatewayRoute` whose single rule matches `x-ai-eg-model: ` exactly; `sectionName` on the parentRef scopes the route to its listener, whose own hostname pins the FQDN. (The rule previously also matched the shared `Host` header, but the AI Gateway v0.5 controller does not register a model whose match rule carries any header beyond `x-ai-eg-model`, so the Host matcher was removed; see [#116](https://github.com/nebari-dev/llm-serving-pack/issues/116).) Clients set `model` in the request body, the same way they would against api.openai.com - no per-model URL is required. The `endpoints.external.subdomain` field on `LLMModel` is currently unused at the routing layer. It is retained on the CRD for a future DNS-01 / wildcard cert path. See [Configuration](/configuration/) for field details. @@ -177,7 +177,7 @@ The `endpoints.external.subdomain` field on `LLMModel` is currently unused at th ## Dual endpoint auth -Each `LLMModel` gets two endpoints with different auth mechanisms. A NetworkPolicy on model pods ensures all traffic flows through the Gateway. +Each `LLMModel` gets two endpoints with different auth mechanisms. Access control is enforced at the gateway level by SecurityPolicies bound per-route: Keycloak JWT plus group authorization on the internal endpoint, and API-key authentication plus a per-model client-ID authorization allow-list on the external endpoint. (Models share one hostname and are dispatched by the `x-ai-eg-model` header, so isolation comes from the per-route policies, not from per-model hostnames.) A NetworkPolicy on model pods ensures all traffic flows through the Gateway. ### External endpoint @@ -190,7 +190,33 @@ Client -> Authorization: Bearer sk-... -> Envoy AI Gateway -> apiKeyAuth Securit - Hostname: `llm.` (shared; per-model dispatch by `x-ai-eg-model` header) - `AIGatewayRoute` for token counting, rate limiting, protocol normalization - `SecurityPolicy` with `apiKeyAuth` attached to the generated `HTTPRoute` (same name as the `AIGatewayRoute`), per model -- Planned (not yet enabled): `sanitize: true` to strip the API key before forwarding to vLLM, and `forwardClientIDHeader: X-Client-ID` to pass the authenticated client ID downstream for logging and flow control. These `apiKeyAuth` fields require Envoy Gateway v1.7+ and are not present in the v1.3 `SecurityPolicy` CRD the pack currently targets, so today the validated `Authorization` header is forwarded to vLLM unchanged. +- `sanitize: true` strips the API key before forwarding to vLLM +- Per-model scoping is enforced by an `authorization` block on the external + SecurityPolicy, not by authentication. API-key authentication is pooled across + the shared listener (any valid key authenticates), so the SecurityPolicy adds + `apiKeyAuth.forwardClientIDHeader: x-llm-client-id` and a deny-by-default + `authorization` rule that allows only the client IDs present in that model's + own api-keys Secret. A key minted for one model therefore returns 403 against + any other model on the listener. The operator re-renders the allow-list when + the key-manager mints or revokes a key, so a newly minted key activates within + about a minute. The forwarded `x-llm-client-id` header also reaches the + backend, where it serves request attribution and GIE flow control. +- Authentication is pooled by design, and it lives on each per-route policy + rather than on a single gateway-level one. Envoy Gateway's `api_key_auth` + filter runs before the AI Gateway ext_proc resolves the model, so + authentication is evaluated before the request reaches the model's own + route; each external SecurityPolicy therefore pools every model's api-keys + Secret into its `credentialRefs`, and any valid key authenticates on the + shared listener. Hoisting that shared credential pool to one gateway-level + SecurityPolicy does not work: each route needs its own SecurityPolicy for + the per-model authorization allow-list anyway, and a route-level policy + takes precedence over a gateway-level one rather than merging with it (per + Envoy Gateway policy-attachment semantics), which would leave routes + without the pooled credentials. The consequence is that every per-model + reconcile reads the full api-keys Secret set and any Secret change + re-reconciles all models - a deliberate exception to the + cluster-singleton rule, since the rendered SecurityPolicy is genuinely + per-model and only its inputs are fleet-wide. - API key Secret referenced from the SecurityPolicy without crossing namespace boundaries (Envoy Gateway's `apiKeyAuth` does not honor cross-namespace `credentialRefs`) ### Internal endpoint @@ -252,7 +278,7 @@ spec: schemaVersion: api/v1 # upstream path prefix (default v1) credentialSecretName: openrouter-api-key # same-namespace Secret, key "apiKey" models: - catchAll: true # any model id not claimed elsewhere + catchAll: true # currently inert on AI Gateway v0.5; see below declared: # explicit routes, advertised by /v1/models - openai/gpt-5.2 - anthropic/claude-opus-4.6 @@ -278,12 +304,13 @@ Generated resources (all in the CR's namespace, no serving stack): Semantics worth knowing: - **Two auth layers.** Users authenticate to the cluster gateway (their own API key externally, Keycloak JWT internally); the gateway injects the platform's provider credential upstream. Users never see the provider key. -- **Served models win.** Per-LLMModel routes match Host AND `x-ai-eg-model` (two headers) while the catch-all matches Host only, so Gateway API precedence keeps any locally served model id local. +- **Served models win (validated live on EG v1.6.7 / AI Gateway v0.5).** The mechanism changed with the Host-matcher removal ([#116](https://github.com/nebari-dev/llm-serving-pack/issues/116)): dispatch is decided by the AI Gateway ext_proc's model registry, not by Gateway API header-count precedence. A request whose `model` is a served or declared id is routed to that model's own rule via `x-ai-eg-model`, regardless of route age - verified with a `catchAll: true` PassthroughModel whose routes were older than the served model's. +- **`catchAll: true` is currently inert on AI Gateway v0.5.** The ext_proc returns 404 ("model not configured in the Gateway") for any model id not registered by some route rule, before HTTPRoute matching runs, so the catch-all rule (a Host-only match) never receives traffic - undeclared ids cannot reach the provider through it. Declared models are unaffected. The field is retained for a future AI Gateway version where unregistered ids fall through to route matching. - **`modelsOwnedBy` is set on the external route only.** The gateway's `/v1/models` endpoint aggregates declared models across every route on the Gateway; declaring on both endpoints lists each model twice. - **Catch-all conflicts are not validated.** Two catch-all PassthroughModels on the same gateway race for unclaimed model ids the same way two identical HTTPRoutes would; the webhook validates per-CR only, consistent with the LLMModel webhook dropping cross-CR collision checks. Document one catch-all per cluster as the supported shape. - **Names are unique across both kinds.** Both an LLMModel and a PassthroughModel derive their api-keys Secret name as `-api-keys`, so sharing a name would make the two controllers fight over one Secret. Both webhooks reject a CR whose name is already taken by the other kind in the same namespace. The key-manager cache also keeps kind-prefixed entries as defense in depth. - **Status.** Phase is `Ready`/`Error` with conditions `BackendConfigured`, `ExternalEndpointReady`, `InternalEndpointReady`; missing AI Gateway CRDs surface as `ApplyFailed` conditions with a one-minute requeue rather than failing reconciliation outright. This surface-and-requeue handling is the intended operator-wide convention for a degraded gateway-apply. The older LLMModel reconciler instead logs-and-continues for the same case; converging it onto this convention is tracked as a follow-up. -- **Generated gateway resources are reconciled on-change-only.** Because the AI Gateway kinds (`Backend`, `BackendTLSPolicy`, `AIServiceBackend`, `BackendSecurityPolicy`, `AIGatewayRoute`, `SecurityPolicy`) are applied as unstructured objects, `SetupWithManager` registers only `For(&PassthroughModel{})` and does not `Owns(...)` them. If a generated resource is deleted out-of-band, it is not recreated until the next edit to the PassthroughModel CR. +- **Generated gateway resources are reconciled on-change-only.** Because the AI Gateway kinds (`Backend`, `BackendTLSPolicy`, `AIServiceBackend`, `BackendSecurityPolicy`, `AIGatewayRoute`, `SecurityPolicy`) are applied as unstructured objects, `SetupWithManager` registers `For(&PassthroughModel{})` plus a watch on operator-managed api-keys Secrets, and does not `Owns(...)` them. If a generated resource is deleted out-of-band, it is not recreated until the next PassthroughModel edit or the next api-keys Secret change in the namespace (any key mint or revoke re-reconciles every model). --- @@ -298,9 +325,9 @@ A small web application behind `NebariApp` that lets authenticated users generat 3. Key manager watches all `LLMModel` CRs, filters to models where `access.groups` overlaps with the user's OIDC groups (or `access.public: true`) 4. User sees only models they can access 5. User creates a key for a model; key manager generates `sk-`, writes the client ID and key value to that model's `-api-keys` Secret in the operator namespace, and writes metadata to the corresponding ConfigMap -6. Envoy Gateway's `apiKeyAuth` picks up the new Secret entry immediately +6. Envoy Gateway's pooled `apiKeyAuth` picks up the new Secret entry as soon as it syncs (authentication); separately, the operator - watching api-keys Secrets - re-renders the model's SecurityPolicy authorization allow-list to include the new client ID (authorization). A fresh key works once both land - typically seconds, within about a minute -Revocation: remove the entry from the Secret and its corresponding metadata from the ConfigMap. Effect is immediate. +Revocation: remove the entry from the Secret and its corresponding metadata from the ConfigMap. Authentication fails as soon as Envoy Gateway syncs the Secret (immediate in practice); the operator also drops the client ID from the authorization allow-list. ### Known limitation - group-change revocation not yet implemented @@ -312,7 +339,7 @@ Automatic revocation on group change is **planned but not implemented in v0.1**. No database. State is split across two Kubernetes resources per model, both in the operator namespace: -- **Secret** (`-api-keys`): contains only the data Envoy Gateway needs. Each entry: key = client ID (e.g., `user-chuck-1`), value = the raw API key. This Secret is the source of truth for authentication. Individual Secrets are limited to 1 MiB, which supports roughly a few thousand keys per model (known scaling limit for v0.1). +- **Secret** (`-api-keys`): contains only the data Envoy Gateway needs. Each entry: key = client ID (e.g., `user-chuck-phi4-mini-adae6d48-1`: user, model, an FNV-1a hash binding the (user, model) pair, sequence number), value = the raw API key. Client IDs must be globally unique across every model's Secret - the operator pools all api-keys Secrets into each external SecurityPolicy's `credentialRefs`, and a duplicated client ID in that pooled set both breaks authentication and leaks authorization across models; the pair hash keeps hyphenated usernames and model names from composing to the same ID. This Secret is the source of truth for authentication. Individual Secrets are limited to 1 MiB, which supports roughly a few thousand keys per model (known scaling limit for v0.1). - **ConfigMap** (`-api-key-metadata`): contains a JSON blob per client ID with management metadata (creator username, creation timestamp, description). Separated from the Secret so the key manager can read and display metadata without exposing actual key values. Also limited to 1 MiB. @@ -324,8 +351,8 @@ Envoy Gateway's `apiKeyAuth` expects Secret data entries where each key is the c ```yaml data: - user-chuck-1: c2stYWJjMTIzZGVmNDU2Nzg5MGFiY2RlZjEyMzQ1Njc4 # base64 of "sk-abc123def..." - user-alice-1: c2stZGVmNDU2Nzg5MGFiY2RlZjEyMzQ1Njc4OTBhYmNk + user-chuck-phi4-mini-adae6d48-1: c2stYWJjMTIzZGVmNDU2Nzg5MGFiY2RlZjEyMzQ1Njc4 # base64 of "sk-abc123def..." + user-alice-phi4-mini-11079538-1: c2stZGVmNDU2Nzg5MGFiY2RlZjEyMzQ1Njc4OTBhYmNk ``` ### Key manager RBAC @@ -406,4 +433,4 @@ This restriction exists because Envoy Gateway's `SecurityPolicy.spec.apiKeyAuth. **Secret isolation**: API key Secrets live in the operator namespace with namespace-scoped RBAC. The key manager and the operator are the only components with access - the operator creates them, the key manager reads and updates them. SecurityPolicies in the same namespace reference them via `apiKeyAuth.credentialRefs` without crossing namespace boundaries. -**Gateway as security boundary**: all model access (external and internal) flows through Envoy Gateway, where auth is enforced via SecurityPolicy. The external endpoint uses `apiKeyAuth` to authenticate requests before they reach vLLM. Key stripping (`sanitize: true`) is planned but not yet enabled - it requires Envoy Gateway v1.7+, so on the currently targeted v1.3 the validated `Authorization` header still reaches vLLM. The internal endpoint uses JWT validation against the OIDC issuer's JWKS endpoint. +**Gateway as security boundary**: all model access (external and internal) flows through Envoy Gateway, where auth is enforced via SecurityPolicy. The external endpoint uses `apiKeyAuth` with `sanitize: true` (API keys are stripped before reaching vLLM) plus a per-model authorization allow-list, so a key is accepted only by the model it was minted for. The internal endpoint uses JWT validation against the OIDC issuer's JWKS endpoint. diff --git a/docs/src/content/docs/configuration.md b/docs/src/content/docs/configuration.md index 9fe3057..4612d0f 100644 --- a/docs/src/content/docs/configuration.md +++ b/docs/src/content/docs/configuration.md @@ -149,6 +149,7 @@ Configures the vLLM serving layer. All fields optional. |-------|------|---------|-------------| | `spec.serving.replicas` | integer | `1` | Number of serving replicas. | | `spec.serving.image` | string | chart default | Container image for the vLLM server. Overrides `defaults.serving.image`. | +| `spec.serving.command` | []string | `python3 -m vllm.entrypoints.openai.api_server` | Container command for the vLLM container. The default is required by the default serving image, whose entrypoint is the NVIDIA CUDA wrapper with no default CMD. Set this when a custom `serving.image` needs a different launcher; `serving.vllmArgs` are appended as arguments. | | `spec.serving.updateStrategy` | string | `Recreate` | Deployment rollout strategy: `Recreate` (default) or `RollingUpdate`. `Recreate` is the default because model pods hold exclusive GPUs and a ReadWriteOnce PVC; on clusters without spare GPU capacity a rolling update deadlocks until the old pod is removed. Set to `RollingUpdate` only when the cluster has enough free GPUs to run old and new pods simultaneously. | | `spec.serving.tensorParallelism` | integer | `gpu.count` | Tensor parallelism degree. Defaults to the GPU count when not set. | | `spec.serving.dataParallelism` | integer | `1` | Data parallelism degree. | diff --git a/docs/src/content/docs/installation.md b/docs/src/content/docs/installation.md index 050af33..06138a1 100644 --- a/docs/src/content/docs/installation.md +++ b/docs/src/content/docs/installation.md @@ -1645,12 +1645,27 @@ config. Re-check section 6. ### `401` on external endpoint with a valid API key +Authentication is pooled across every model's api-keys Secret, so a 401 +means the key is not present in any model's Secret (revoked, mistyped, or +never minted). + - Verify the API key Secret exists: `kubectl get secret -n nebari-llm-serving-system -api-keys` -- Verify the key is for the right model (keys are per-model) - Verify the `model` field in your request body matches the HuggingFace model ID (e.g. `Qwen/Qwen3.5-35B-A3B-GPTQ-Int4`), not the LLMModel CR name +### `403` on external endpoint with a valid API key + +Keys authenticate anywhere on the shared listener but are authorized only +for the model they were minted for, so a 403 means the key is valid but +not on this model's allow-list. + +- Verify the key was minted for the model you are calling (a key for + model A returns 403 against model B by design) +- A freshly minted key can 403 until the operator re-renders the model's + SecurityPolicy allow-list - typically seconds, within about a minute; + retry before digging deeper + ### Key-manager UI shows "No models available" for a user who should have access - Confirm the user is in the correct Keycloak group (`llm` or diff --git a/docs/src/content/docs/local-development.md b/docs/src/content/docs/local-development.md index 5ff535a..3ddf6ed 100644 --- a/docs/src/content/docs/local-development.md +++ b/docs/src/content/docs/local-development.md @@ -202,6 +202,11 @@ curl -k https://llm.local:8443/v1/chat/completions \ -d '{"model":"openai/gpt-4o-mini","messages":[{"role":"user","content":"hi"}]}' ``` +A curl issued immediately after the Secret patch can return 403: the key +authenticates as soon as Envoy Gateway syncs the Secret, but authorization +waits for the operator to re-render the model's SecurityPolicy allow-list +(typically a few seconds). Retry on a 403 before digging deeper. + The internal endpoint (`llm-internal.local`) always requires a real Keycloak JWT, even when access is public, so it is not reachable on a bare kind cluster. diff --git a/key-manager/internal/api/handler_test.go b/key-manager/internal/api/handler_test.go index 1e1ef69..43201c7 100644 --- a/key-manager/internal/api/handler_test.go +++ b/key-manager/internal/api/handler_test.go @@ -31,11 +31,11 @@ func (m *mockModelLister) FilterModelsForUser(groups []string) []models.ModelInf } type mockKeyManager struct { - keys []secrets.KeyInfo - createResult *secrets.CreateKeyResult - createErr error - revokeErr error - listUserErr error + keys []secrets.KeyInfo + createResult *secrets.CreateKeyResult + createErr error + revokeErr error + listUserErr error } func (m *mockKeyManager) CreateKey(ctx context.Context, modelName, username, description string) (*secrets.CreateKeyResult, error) { @@ -323,15 +323,15 @@ func TestCreateKey(t *testing.T) { } tests := []struct { - name string - user *UserInfo - body interface{} - accessModels []models.ModelInfo - createResult *secrets.CreateKeyResult - createErr error - wantStatus int - wantClientID string - wantAPIKey string + name string + user *UserInfo + body interface{} + accessModels []models.ModelInfo + createResult *secrets.CreateKeyResult + createErr error + wantStatus int + wantClientID string + wantAPIKey string }{ { name: "creates key and returns 201 with apiKey", @@ -442,17 +442,17 @@ func TestDeleteKey(t *testing.T) { wantStatus int }{ { - name: "returns 204 when key belongs to user", - user: testUser, - path: "/api/keys/default/llama3/user-chuck-1", - keys: testKeys, + name: "returns 204 when key belongs to user", + user: testUser, + path: "/api/keys/default/llama3/user-chuck-1", + keys: testKeys, wantStatus: http.StatusNoContent, }, { - name: "returns 403 when user doesn't own the key", - user: testUser, - path: "/api/keys/default/llama3/user-alice-1", - keys: testKeys, + name: "returns 403 when user doesn't own the key", + user: testUser, + path: "/api/keys/default/llama3/user-alice-1", + keys: testKeys, wantStatus: http.StatusForbidden, }, { diff --git a/key-manager/internal/secrets/manager.go b/key-manager/internal/secrets/manager.go index 862825e..dd86a07 100644 --- a/key-manager/internal/secrets/manager.go +++ b/key-manager/internal/secrets/manager.go @@ -27,7 +27,7 @@ type KeyInfo struct { // CreateKeyResult is returned when a new key is created. type CreateKeyResult struct { - ClientID string // e.g., "user-chuck-1" + ClientID string // e.g., "user-chuck-phi4-mini-adae6d48-1" (user, model, pair hash, sequence) APIKey string // e.g., "sk-abc123..." - only returned once at creation time } @@ -187,20 +187,49 @@ func (m *Manager) createKeyOnce(ctx context.Context, modelName, username, descri return nil, err } - // Count existing keys for this user to determine the sequence number. - // The sanitized username is used both for the data-key prefix and for the - // composed clientID so the count and the new clientID stay in sync. - // KeyInfo.Creator below preserves the raw username for ownership checks. + // The clientID is scoped by BOTH username and model: the operator pools + // every model's api-keys Secret into each model's SecurityPolicy + // credentialRefs (model-scoped auth), and forwards the matched data key as + // the x-llm-client-id used for per-model authorization. A clientID that + // repeats across models - e.g. "user--1" for the first key of every + // model - collides in that pooled set, so one model's key both + // authenticates and authorizes for another, and the user's other keys fail + // to authenticate at all (only one value survives per duplicated key). + // + // A hyphen-joined "user--" is still ambiguous because both + // parts legitimately contain hyphens: ("mary", "jane-chat") and + // ("mary-jane", "chat") compose identically. An FNV-1a hash of the raw + // (username, model) pair is appended so distinct pairs always mint + // distinct client IDs. KeyInfo.Creator below preserves the raw username + // for ownership checks. safeUsername := sanitizeUsernameForKey(username) - userPrefix := "user-" + safeUsername + "-" + safeModel := sanitizeUsernameForKey(modelName) + pair := fnv.New32a() + _, _ = pair.Write([]byte(username)) + _, _ = pair.Write([]byte{0}) + _, _ = pair.Write([]byte(modelName)) + userPrefix := fmt.Sprintf("user-%s-%s-%08x-", safeUsername, safeModel, pair.Sum32()) count := 0 for k := range secret.Data { if strings.HasPrefix(k, userPrefix) { count++ } } + // Probe upward from count+1 until the clientID is unused. Revocation + // leaves gaps in the sequence, so a bare count can land on a still-live + // key (keys {1,2,3}, revoke #2, count=2 -> "-3") and would silently + // overwrite that credential in both the Secret and the ConfigMap. sequence := count + 1 - clientID := fmt.Sprintf("user-%s-%d", safeUsername, sequence) + clientID := fmt.Sprintf("%s%d", userPrefix, sequence) + for { + _, inSecret := secret.Data[clientID] + _, inCM := cm.Data[clientID] + if !inSecret && !inCM { + break + } + sequence++ + clientID = fmt.Sprintf("%s%d", userPrefix, sequence) + } apiKey, err := generateAPIKey() if err != nil { diff --git a/key-manager/internal/secrets/manager_test.go b/key-manager/internal/secrets/manager_test.go index b5b69d5..fa883af 100644 --- a/key-manager/internal/secrets/manager_test.go +++ b/key-manager/internal/secrets/manager_test.go @@ -55,6 +55,18 @@ func sanitizedPrefix(raw string) string { return fmt.Sprintf("%s-%08x", string(out), h.Sum32()) } +// pairSuffix mirrors the production (username, model) pair hash appended to +// every clientID so black-box tests can assemble the exact clientID the +// manager will produce. Kept as an independent implementation so a drift +// between this and production fails the tests below. +func pairSuffix(rawUser, rawModel string) string { + h := fnv.New32a() + _, _ = h.Write([]byte(rawUser)) + _, _ = h.Write([]byte{0}) + _, _ = h.Write([]byte(rawModel)) + return fmt.Sprintf("%08x", h.Sum32()) +} + const testNamespace = "llm-api-keys" func buildScheme(t *testing.T) *runtime.Scheme { @@ -173,19 +185,22 @@ func TestCreateKey_ClientIDFormat(t *testing.T) { name: "first key for user is -1", username: "chuck", existingKeys: []string{}, - wantClientID: "user-chuck-1", + wantClientID: "user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-1", }, { name: "second key for user is -2", username: "chuck", - existingKeys: []string{"user-chuck-1"}, - wantClientID: "user-chuck-2", + existingKeys: []string{"user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-1"}, + wantClientID: "user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-2", }, { - name: "third key for user is -3", - username: "chuck", - existingKeys: []string{"user-chuck-1", "user-chuck-2"}, - wantClientID: "user-chuck-3", + name: "third key for user is -3", + username: "chuck", + existingKeys: []string{ + "user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-1", + "user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-2", + }, + wantClientID: "user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-3", }, } @@ -225,6 +240,135 @@ func TestCreateKey_ClientIDFormat(t *testing.T) { } } +// TestCreateKey_ClientIDUniqueAcrossModels is a regression test for the +// per-model API-key scoping bug: a single user minting one key for each of two +// models must get DISTINCT client IDs. Before the fix the sequence was counted +// per-model, so the first key of every model was "user--1"; once the +// operator pooled every model's api-keys Secret into each model's +// SecurityPolicy credentialRefs, those duplicate client IDs collided - one +// model's key authenticated/authorized for another, and the user's other keys +// failed to authenticate at all. +func TestCreateKey_ClientIDUniqueAcrossModels(t *testing.T) { + scheme := buildScheme(t) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects( + makeSecret("model-a", testNamespace), + makeConfigMap("model-a", testNamespace), + makeSecret("model-b", testNamespace), + makeConfigMap("model-b", testNamespace), + ). + Build() + + mgr := secrets.NewManager(fakeClient, testNamespace) + a, err := mgr.CreateKey(context.Background(), "model-a", "chuck", "test") + if err != nil { + t.Fatalf("CreateKey model-a: %v", err) + } + b, err := mgr.CreateKey(context.Background(), "model-b", "chuck", "test") + if err != nil { + t.Fatalf("CreateKey model-b: %v", err) + } + if a.ClientID == b.ClientID { + t.Errorf("client IDs collide across models: model-a=%q model-b=%q (must be unique)", a.ClientID, b.ClientID) + } + wantA := "user-chuck-model-a-" + pairSuffix("chuck", "model-a") + "-1" + if a.ClientID != wantA { + t.Errorf("model-a clientID = %q, want %q", a.ClientID, wantA) + } + wantB := "user-chuck-model-b-" + pairSuffix("chuck", "model-b") + "-1" + if b.ClientID != wantB { + t.Errorf("model-b clientID = %q, want %q", b.ClientID, wantB) + } +} + +// TestCreateKey_ClientIDUnambiguousBoundary: usernames and model names both +// legitimately contain hyphens, so composing "user---" with a +// hyphen separator is ambiguous: ("mary", "jane-chat") and ("mary-jane", +// "chat") would compose to the same clientID. Duplicate client IDs across two +// models' Secrets collide in the pooled credentialRefs set (the #122 failure +// mode: cross-model authorization plus one key failing to authenticate), so +// distinct (user, model) pairs must always mint distinct client IDs. +func TestCreateKey_ClientIDUnambiguousBoundary(t *testing.T) { + scheme := buildScheme(t) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects( + makeSecret("jane-chat", testNamespace), + makeConfigMap("jane-chat", testNamespace), + makeSecret("chat", testNamespace), + makeConfigMap("chat", testNamespace), + ). + Build() + + mgr := secrets.NewManager(fakeClient, testNamespace) + a, err := mgr.CreateKey(context.Background(), "jane-chat", "mary", "test") + if err != nil { + t.Fatalf("CreateKey mary/jane-chat: %v", err) + } + b, err := mgr.CreateKey(context.Background(), "chat", "mary-jane", "test") + if err != nil { + t.Fatalf("CreateKey mary-jane/chat: %v", err) + } + if a.ClientID == b.ClientID { + t.Errorf("distinct (user, model) pairs minted the same clientID %q; pooled credentialRefs require globally unique client IDs", a.ClientID) + } +} + +// TestCreateKey_RevokeThenMintDoesNotTouchLiveKeys: the sequence number must +// not be derived from a bare count of existing keys. With keys {1,2,3}, +// revoking #2 leaves a count of 2, so a count-based mint would reuse sequence +// 3 and silently overwrite the still-live key #3 (Secret value and ConfigMap +// metadata), killing a credential the user believes is active. +func TestCreateKey_RevokeThenMintDoesNotTouchLiveKeys(t *testing.T) { + scheme := buildScheme(t) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects( + makeSecret("model-a", testNamespace), + makeConfigMap("model-a", testNamespace), + ). + Build() + + ctx := context.Background() + mgr := secrets.NewManager(fakeClient, testNamespace) + minted := make([]*secrets.CreateKeyResult, 0, 3) + for range 3 { + r, err := mgr.CreateKey(ctx, "model-a", "chuck", "test") + if err != nil { + t.Fatalf("CreateKey: %v", err) + } + minted = append(minted, r) + } + + if err := mgr.RevokeKey(ctx, "model-a", minted[1].ClientID); err != nil { + t.Fatalf("RevokeKey %s: %v", minted[1].ClientID, err) + } + live := []*secrets.CreateKeyResult{minted[0], minted[2]} + + fresh, err := mgr.CreateKey(ctx, "model-a", "chuck", "test") + if err != nil { + t.Fatalf("CreateKey after revoke: %v", err) + } + + secret := &corev1.Secret{} + key := types.NamespacedName{Namespace: testNamespace, Name: "model-a-api-keys"} + if err := fakeClient.Get(ctx, key, secret); err != nil { + t.Fatalf("Get Secret: %v", err) + } + for _, l := range live { + if fresh.ClientID == l.ClientID { + t.Errorf("mint after revoke reused live clientID %q", l.ClientID) + } + if got := string(secret.Data[l.ClientID]); got != l.APIKey { + t.Errorf("live key %q value changed after mint: got %q, want %q", l.ClientID, got, l.APIKey) + } + } + if _, ok := secret.Data[fresh.ClientID]; !ok { + t.Errorf("freshly minted clientID %q missing from Secret", fresh.ClientID) + } +} + func TestCreateKey_EmptyUsername(t *testing.T) { scheme := buildScheme(t) fakeClient := fake.NewClientBuilder(). @@ -472,26 +616,26 @@ func TestCreateKey_SanitizesEmailUsername(t *testing.T) { { name: "email username gets @ replaced with -at- and a hash suffix", username: "alice@example.com", - wantClientID: "user-" + sanitizedPrefix("alice@example.com") + "-1", + wantClientID: "user-" + sanitizedPrefix("alice@example.com") + "-my-model-" + pairSuffix("alice@example.com", "my-model") + "-1", wantCreatorRaw: "alice@example.com", }, { name: "plain alphanumeric username is unchanged", username: "chuck", - wantClientID: "user-chuck-1", + wantClientID: "user-chuck-my-model-" + pairSuffix("chuck", "my-model") + "-1", wantCreatorRaw: "chuck", }, { name: "sequence increments using sanitized prefix", username: "alice@example.com", - wantClientID: "user-" + sanitizedPrefix("alice@example.com") + "-2", + wantClientID: "user-" + sanitizedPrefix("alice@example.com") + "-my-model-" + pairSuffix("alice@example.com", "my-model") + "-2", wantCreatorRaw: "alice@example.com", - existingDataKeys: []string{"user-" + sanitizedPrefix("alice@example.com") + "-1"}, + existingDataKeys: []string{"user-" + sanitizedPrefix("alice@example.com") + "-my-model-" + pairSuffix("alice@example.com", "my-model") + "-1"}, }, { name: "raw alice-at-example.com does NOT collide with alice@example.com", username: "alice-at-example.com", - wantClientID: "user-alice-at-example.com-1", // already valid -> no hash suffix + wantClientID: "user-alice-at-example.com-my-model-" + pairSuffix("alice-at-example.com", "my-model") + "-1", // already-valid username -> no sanitizer hash suffix wantCreatorRaw: "alice-at-example.com", }, } diff --git a/operator/api/v1alpha1/llmmodel_types.go b/operator/api/v1alpha1/llmmodel_types.go index a49392b..9ebf51e 100644 --- a/operator/api/v1alpha1/llmmodel_types.go +++ b/operator/api/v1alpha1/llmmodel_types.go @@ -117,6 +117,14 @@ type ServingSpec struct { // image overrides the default vLLM container image // +optional Image string `json:"image,omitempty"` + // command overrides the container command for the vLLM container. + // Defaults to the standard vLLM OpenAI-server entrypoint + // (python3 -m vllm.entrypoints.openai.api_server), which the default + // serving image requires because its entrypoint is the NVIDIA CUDA + // wrapper with no default CMD. Set this when a custom serving image + // needs a different launcher; vllmArgs are appended as arguments. + // +optional + Command []string `json:"command,omitempty"` // replicas is the number of serving replicas // +optional // +kubebuilder:default=1 diff --git a/operator/api/v1alpha1/zz_generated.deepcopy.go b/operator/api/v1alpha1/zz_generated.deepcopy.go index f974995..fcb7912 100644 --- a/operator/api/v1alpha1/zz_generated.deepcopy.go +++ b/operator/api/v1alpha1/zz_generated.deepcopy.go @@ -541,6 +541,11 @@ func (in *ResourceSpec) DeepCopy() *ResourceSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServingSpec) DeepCopyInto(out *ServingSpec) { *out = *in + if in.Command != nil { + in, out := &in.Command, &out.Command + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Replicas != nil { in, out := &in.Replicas, &out.Replicas *out = new(int32) diff --git a/operator/config/crd/bases/llm.nebari.dev_llmmodels.yaml b/operator/config/crd/bases/llm.nebari.dev_llmmodels.yaml index 2575745..66f2fe6 100644 --- a/operator/config/crd/bases/llm.nebari.dev_llmmodels.yaml +++ b/operator/config/crd/bases/llm.nebari.dev_llmmodels.yaml @@ -1336,6 +1336,17 @@ spec: serving: description: serving configures the vLLM serving layer properties: + command: + description: |- + command overrides the container command for the vLLM container. + Defaults to the standard vLLM OpenAI-server entrypoint + (python3 -m vllm.entrypoints.openai.api_server), which the default + serving image requires because its entrypoint is the NVIDIA CUDA + wrapper with no default CMD. Set this when a custom serving image + needs a different launcher; vllmArgs are appended as arguments. + items: + type: string + type: array dataParallelism: default: 1 description: dataParallelism controls data parallelism diff --git a/operator/internal/controller/apikeys.go b/operator/internal/controller/apikeys.go new file mode 100644 index 0000000..b8dda78 --- /dev/null +++ b/operator/internal/controller/apikeys.go @@ -0,0 +1,125 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "sort" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/nebari-dev/nebari-llm-serving-pack/operator/internal/controller/reconcilers" +) + +// This file holds the api-keys Secret plumbing shared by the LLMModel and +// PassthroughModel controllers. Both render external SecurityPolicies from +// the same fleet-wide inputs (pooled credentialRefs across every model's +// api-keys Secret, a per-model client-ID allow-list), so both register the +// same Secret watch and gather the same inputs during Reconcile. + +// managedByOperatorPredicate filters watch events to objects the operator +// itself manages, keeping the Secret watch from firing on unrelated Secrets. +func managedByOperatorPredicate() predicate.Predicate { + return predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetLabels()["app.kubernetes.io/managed-by"] == "nebari-llm-operator" + }) +} + +// apiKeyClientIDs reads a model's api-keys Secret and returns its client IDs +// (data-key names). A missing Secret (first reconcile, before it is created) +// yields no client IDs, which renders a deny-all external authorization until +// a key is minted. +func apiKeyClientIDs(ctx context.Context, c client.Reader, modelName, namespace string) ([]string, error) { + secret := &corev1.Secret{} + key := types.NamespacedName{Name: reconcilers.APIKeySecretName(modelName), Namespace: namespace} + if err := c.Get(ctx, key, secret); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, err + } + return reconcilers.ClientIDsFromSecret(secret), nil +} + +// apiKeySecretNamesWith returns the names of all operator-managed api-keys +// Secrets in the namespace (every model's Secret), guaranteeing ownName is +// present, sorted for deterministic rendering. These pool into each external +// SecurityPolicy's apiKeyAuth.credentialRefs so any valid key authenticates on +// the shared listener; the per-model authorization block then scopes it (#116). +func apiKeySecretNamesWith(ctx context.Context, c client.Reader, namespace, ownName string) ([]string, error) { + var list corev1.SecretList + if err := c.List(ctx, &list, client.InNamespace(namespace), + client.MatchingLabels{"app.kubernetes.io/managed-by": "nebari-llm-operator"}); err != nil { + return nil, err + } + set := map[string]struct{}{ownName: {}} + for i := range list.Items { + if strings.HasSuffix(list.Items[i].Name, reconcilers.APIKeySecretSuffix) { + set[list.Items[i].Name] = struct{}{} + } + } + names := make([]string, 0, len(set)) + for name := range set { + names = append(names, name) + } + sort.Strings(names) + return names, nil +} + +// enqueueAllModelsForAPIKeySecret enqueues every item of list (an +// LLMModelList or PassthroughModelList) in the Secret's namespace when an +// operator-managed api-keys Secret changes. Both the per-model authorization +// allow-list (a model's own keys) and the pooled credentialRefs (every +// model's Secret) depend on the full set of api-keys Secrets, so a key +// mint/revoke or a model being added/removed must re-reconcile every model. +// Churn is bounded by model count; scoping key-mint updates to the owning +// model is a possible follow-up. +func enqueueAllModelsForAPIKeySecret(ctx context.Context, c client.Reader, obj client.Object, list client.ObjectList) []reconcile.Request { + if !strings.HasSuffix(obj.GetName(), reconcilers.APIKeySecretSuffix) { + return nil + } + if err := c.List(ctx, list, client.InNamespace(obj.GetNamespace())); err != nil { + logf.FromContext(ctx).Error(err, "listing models for api-keys Secret change; re-render skipped until the next event", + "secret", obj.GetName(), "namespace", obj.GetNamespace()) + return nil + } + items, err := meta.ExtractList(list) + if err != nil { + logf.FromContext(ctx).Error(err, "extracting model list for api-keys Secret change", + "secret", obj.GetName(), "namespace", obj.GetNamespace()) + return nil + } + reqs := make([]reconcile.Request, 0, len(items)) + for _, item := range items { + model, ok := item.(client.Object) + if !ok { + continue + } + reqs = append(reqs, reconcile.Request{NamespacedName: types.NamespacedName{ + Name: model.GetName(), Namespace: model.GetNamespace(), + }}) + } + return reqs +} diff --git a/operator/internal/controller/apikeys_test.go b/operator/internal/controller/apikeys_test.go new file mode 100644 index 0000000..3578a4b --- /dev/null +++ b/operator/internal/controller/apikeys_test.go @@ -0,0 +1,119 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + llmv1alpha1 "github.com/nebari-dev/nebari-llm-serving-pack/operator/api/v1alpha1" +) + +func apikeysTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := llmv1alpha1.AddToScheme(s); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + return s +} + +func namedSecret(name, namespace string) *corev1.Secret { + return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}} +} + +func llmModel(name, namespace string) *llmv1alpha1.LLMModel { + return &llmv1alpha1.LLMModel{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}} +} + +func passthroughModel(name, namespace string) *llmv1alpha1.PassthroughModel { + return &llmv1alpha1.PassthroughModel{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}} +} + +func TestEnqueueAllModelsForAPIKeySecret(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + list client.ObjectList + objects []client.Object + wantNames []string + }{ + { + name: "api-keys Secret enqueues every LLMModel in its namespace", + secret: namedSecret("phi-api-keys", "llm"), + list: &llmv1alpha1.LLMModelList{}, + objects: []client.Object{ + llmModel("phi", "llm"), + llmModel("mistral", "llm"), + llmModel("elsewhere", "other-ns"), + }, + wantNames: []string{"phi", "mistral"}, + }, + { + name: "non api-keys Secret enqueues nothing", + secret: namedSecret("phi-tls", "llm"), + list: &llmv1alpha1.LLMModelList{}, + objects: []client.Object{ + llmModel("phi", "llm"), + }, + wantNames: nil, + }, + { + name: "api-keys Secret enqueues every PassthroughModel in its namespace", + secret: namedSecret("claude-api-keys", "llm"), + list: &llmv1alpha1.PassthroughModelList{}, + objects: []client.Object{ + passthroughModel("claude", "llm"), + passthroughModel("gemini", "llm"), + }, + wantNames: []string{"claude", "gemini"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := fake.NewClientBuilder(). + WithScheme(apikeysTestScheme(t)). + WithObjects(tt.objects...). + Build() + + reqs := enqueueAllModelsForAPIKeySecret(context.Background(), c, tt.secret, tt.list) + + got := map[string]bool{} + for _, r := range reqs { + if r.Namespace != tt.secret.Namespace { + t.Errorf("request %q enqueued in namespace %q, want %q", r.Name, r.Namespace, tt.secret.Namespace) + } + got[r.Name] = true + } + if len(reqs) != len(tt.wantNames) { + t.Fatalf("got %d requests (%v), want %d (%v)", len(reqs), got, len(tt.wantNames), tt.wantNames) + } + for _, want := range tt.wantNames { + if !got[want] { + t.Errorf("missing request for %q (got %v)", want, got) + } + } + }) + } +} diff --git a/operator/internal/controller/llmmodel_controller.go b/operator/internal/controller/llmmodel_controller.go index a6331bc..00442b8 100644 --- a/operator/internal/controller/llmmodel_controller.go +++ b/operator/internal/controller/llmmodel_controller.go @@ -32,9 +32,12 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" llmv1alpha1 "github.com/nebari-dev/nebari-llm-serving-pack/operator/api/v1alpha1" "github.com/nebari-dev/nebari-llm-serving-pack/operator/internal/config" @@ -124,7 +127,15 @@ func (r *LLMModelReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // 6. Reconcile auth resources (Secret + ConfigMap, both in the model's namespace) var authResources *reconcilers.AuthResources if r.Config != nil { - authResources, err = reconcilers.BuildAuthResources(model, r.Config) + clientIDs, err := apiKeyClientIDs(ctx, r.Client, model.Name, model.Namespace) + if err != nil { + return ctrl.Result{}, fmt.Errorf("reading api key client ids: %w", err) + } + credentialSecretNames, err := apiKeySecretNamesWith(ctx, r.Client, model.Namespace, reconcilers.APIKeySecretName(model.Name)) + if err != nil { + return ctrl.Result{}, fmt.Errorf("listing api key secrets: %w", err) + } + authResources, err = reconcilers.BuildAuthResources(model, r.Config, clientIDs, credentialSecretNames) if err != nil { return ctrl.Result{}, fmt.Errorf("building auth resources: %w", err) } @@ -744,6 +755,13 @@ func (r *LLMModelReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&appsv1.Deployment{}). Owns(&corev1.Service{}). Owns(&corev1.PersistentVolumeClaim{}). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { + return enqueueAllModelsForAPIKeySecret(ctx, r.Client, obj, &llmv1alpha1.LLMModelList{}) + }), + builder.WithPredicates(managedByOperatorPredicate()), + ). Named("llmmodel"). Complete(r) } diff --git a/operator/internal/controller/passthroughmodel_controller.go b/operator/internal/controller/passthroughmodel_controller.go index 9d311e2..f9c9e0a 100644 --- a/operator/internal/controller/passthroughmodel_controller.go +++ b/operator/internal/controller/passthroughmodel_controller.go @@ -29,9 +29,12 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" llmv1alpha1 "github.com/nebari-dev/nebari-llm-serving-pack/operator/api/v1alpha1" "github.com/nebari-dev/nebari-llm-serving-pack/operator/internal/config" @@ -89,7 +92,15 @@ func (r *PassthroughModelReconciler) Reconcile(ctx context.Context, req ctrl.Req } } - resources, err := reconcilers.BuildPassthroughResources(pm, r.Config) + clientIDs, err := apiKeyClientIDs(ctx, r.Client, pm.Name, pm.Namespace) + if err != nil { + return ctrl.Result{}, fmt.Errorf("reading api key client ids: %w", err) + } + credentialSecretNames, err := apiKeySecretNamesWith(ctx, r.Client, pm.Namespace, reconcilers.APIKeySecretName(pm.Name)) + if err != nil { + return ctrl.Result{}, fmt.Errorf("listing api key secrets: %w", err) + } + resources, err := reconcilers.BuildPassthroughResources(pm, r.Config, clientIDs, credentialSecretNames) if err != nil { // Surface the build failure as a condition so `kubectl describe` // explains the Error phase rather than only the reconcile log. @@ -339,6 +350,13 @@ func boolOrDefaultStatus(b *bool) bool { func (r *PassthroughModelReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&llmv1alpha1.PassthroughModel{}). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { + return enqueueAllModelsForAPIKeySecret(ctx, r.Client, obj, &llmv1alpha1.PassthroughModelList{}) + }), + builder.WithPredicates(managedByOperatorPredicate()), + ). Named("passthroughmodel"). Complete(r) } diff --git a/operator/internal/controller/reconcilers/auth.go b/operator/internal/controller/reconcilers/auth.go index 1e11758..8af6467 100644 --- a/operator/internal/controller/reconcilers/auth.go +++ b/operator/internal/controller/reconcilers/auth.go @@ -1,6 +1,8 @@ package reconcilers import ( + "sort" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -9,6 +11,23 @@ import ( "github.com/nebari-dev/nebari-llm-serving-pack/operator/internal/config" ) +// apiKeyClientIDHeader is the request header Envoy Gateway populates with the +// authenticated API key's client ID (the api-keys Secret data-key name). The +// per-route authorization rule matches this header against the model's own +// client IDs so a key minted for one model cannot call another. The api_key_auth +// filter (order 6) runs before the RBAC/authorization filter (order 301), so the +// header is present when authorization evaluates. +const apiKeyClientIDHeader = "x-llm-client-id" + +// authzActionAllow and authzActionDeny are the Envoy Gateway SecurityPolicy +// authorization action values. Both the external (API-key) and internal (JWT) +// policies default to Deny and add a single Allow rule scoped to the model's +// own principals. +const ( + authzActionAllow = "Allow" + authzActionDeny = "Deny" +) + // AuthResources holds all auth-related resources for an LLMModel. All // resources live in the LLMModel's own namespace - the operator no longer // uses a separate api-keys namespace because Envoy Gateway's @@ -27,7 +46,7 @@ type AuthResources struct { // BuildAuthResources is a pure function that computes all auth-related Kubernetes resources // for the given LLMModel: API key Secret, metadata ConfigMap, and SecurityPolicies. // All resources are placed in the LLMModel's own namespace. -func BuildAuthResources(model *llmv1alpha1.LLMModel, cfg *config.OperatorConfig) (*AuthResources, error) { +func BuildAuthResources(model *llmv1alpha1.LLMModel, cfg *config.OperatorConfig, clientIDs []string, credentialSecretNames []string) (*AuthResources, error) { result := &AuthResources{} labels := authResourceLabels(model) @@ -36,7 +55,7 @@ func BuildAuthResources(model *llmv1alpha1.LLMModel, cfg *config.OperatorConfig) result.APIKeyMetadataCM = buildAPIKeyMetadataConfigMap(model, labels) if boolOrDefault(model.Spec.Endpoints.External.Enabled, true) { - result.ExternalSecurityPolicy = buildExternalSecurityPolicy(model) + result.ExternalSecurityPolicy = buildExternalSecurityPolicy(model, clientIDs, credentialSecretNames) } if boolOrDefault(model.Spec.Endpoints.Internal.Enabled, true) { @@ -46,6 +65,23 @@ func BuildAuthResources(model *llmv1alpha1.LLMModel, cfg *config.OperatorConfig) return result, nil } +// ClientIDsFromSecret returns the sorted data-key names of an api-keys Secret. +// Each data-key name is an Envoy Gateway client ID (one per minted API key; +// the key-manager writes secret.Data[clientID] = apiKey). The list is sorted so +// SecurityPolicy rendering is deterministic across reconciles. A nil or +// dataless Secret yields an empty slice. +func ClientIDsFromSecret(secret *corev1.Secret) []string { + if secret == nil { + return []string{} + } + ids := make([]string, 0, len(secret.Data)) + for k := range secret.Data { + ids = append(ids, k) + } + sort.Strings(ids) + return ids +} + // authResourceLabels returns the labels applied to the API-key Secret and // metadata ConfigMap. The `llm.nebari.dev/model` label (from StandardLabels) // is the documented selector for `kubectl get secrets -l llm.nebari.dev/model` @@ -82,13 +118,14 @@ func buildAPIKeyMetadataConfigMap(model *llmv1alpha1.LLMModel, labels map[string } } -func buildExternalSecurityPolicy(model *llmv1alpha1.LLMModel) *unstructured.Unstructured { +func buildExternalSecurityPolicy(model *llmv1alpha1.LLMModel, clientIDs []string, credentialSecretNames []string) *unstructured.Unstructured { return buildAPIKeyAuthSecurityPolicy( model.Name+"-external-auth", model.Namespace, labelsToInterface(StandardLabels(model)), model.Name+"-external", - APIKeySecretName(model.Name), + credentialSecretNames, + clientIDs, ) } @@ -96,7 +133,21 @@ func buildExternalSecurityPolicy(model *llmv1alpha1.LLMModel) *unstructured.Unst // external HTTPRoute behind per-user API keys. Shared between the LLMModel // and PassthroughModel reconcilers so both endpoint types present the same // client UX (Authorization header carrying a key from the api-keys Secret). -func buildAPIKeyAuthSecurityPolicy(name, namespace string, labels map[string]interface{}, routeName, secretName string) *unstructured.Unstructured { +func buildAPIKeyAuthSecurityPolicy(name, namespace string, labels map[string]interface{}, routeName string, credentialSecretNames []string, clientIDs []string) *unstructured.Unstructured { + // credentialRefs pools EVERY model's api-keys Secret on the shared + // listener, not just this model's. EG's api_key_auth filter runs before + // the AI Gateway ext_proc resolves the model, so authentication happens on + // whichever catch-all route wins; pooling lets any valid key authenticate + // there, and the per-route authorization block below scopes it to this + // model. All Secrets are same-namespace (#59). See #116. + credRefs := make([]interface{}, 0, len(credentialSecretNames)) + for _, secretName := range credentialSecretNames { + credRefs = append(credRefs, map[string]interface{}{ + "group": "", + "kind": "Secret", + "name": secretName, + }) + } return &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "gateway.envoyproxy.io/v1alpha1", @@ -119,13 +170,7 @@ func buildAPIKeyAuthSecurityPolicy(name, namespace string, labels map[string]int // any cross-namespace credentialRef (and does not honor // ReferenceGrant for this field). The Secret lives in the // same namespace as this SecurityPolicy by design. - "credentialRefs": []interface{}{ - map[string]interface{}{ - "group": "", - "kind": "Secret", - "name": secretName, - }, - }, + "credentialRefs": credRefs, "extractFrom": []interface{}{ map[string]interface{}{ "headers": []interface{}{ @@ -133,13 +178,53 @@ func buildAPIKeyAuthSecurityPolicy(name, namespace string, labels map[string]int }, }, }, - // TODO: Add sanitize:true and forwardClientIDHeader when minimum - // Envoy Gateway version is bumped to v1.7+ (these fields are not - // in v1.3 SecurityPolicy CRD) + // forwardClientIDHeader surfaces the matched credential's + // client ID to the authorization filter (and upstream). + // sanitize strips the API key before the backend sees it. + "forwardClientIDHeader": apiKeyClientIDHeader, + "sanitize": true, + }, + // Authentication (apiKeyAuth) is pooled per listener: any valid + // key on the shared listener authenticates. Authorization is + // per-route and is what scopes a key to its model: deny by + // default, allow only this model's client IDs. + "authorization": buildAPIKeyAuthorization(clientIDs), + }, + }, + } +} + +// buildAPIKeyAuthorization returns a deny-by-default authorization block that +// allows only the given client IDs (matched on the forwarded client-ID header). +// With no client IDs it returns deny-all (no Allow rule), which is correct for a +// model that has no keys minted yet: every request, including one bearing a key +// valid for a different model on the shared listener, is denied. +func buildAPIKeyAuthorization(clientIDs []string) map[string]interface{} { + authz := map[string]interface{}{ + "defaultAction": authzActionDeny, + } + if len(clientIDs) == 0 { + return authz + } + values := make([]interface{}, 0, len(clientIDs)) + for _, id := range clientIDs { + values = append(values, id) + } + authz["rules"] = []interface{}{ + map[string]interface{}{ + "name": "allow-model-clients", + "action": authzActionAllow, + "principal": map[string]interface{}{ + "headers": []interface{}{ + map[string]interface{}{ + "name": apiKeyClientIDHeader, + "values": values, + }, }, }, }, } + return authz } func buildInternalSecurityPolicy(model *llmv1alpha1.LLMModel, cfg *config.OperatorConfig) *unstructured.Unstructured { @@ -235,11 +320,11 @@ func buildGroupAuthorization(groups []string, groupsClaim string) map[string]int values = append(values, g) } return map[string]interface{}{ - "defaultAction": "Deny", + "defaultAction": authzActionDeny, "rules": []interface{}{ map[string]interface{}{ "name": "allow-groups", - "action": "Allow", + "action": authzActionAllow, "principal": map[string]interface{}{ "jwt": map[string]interface{}{ "provider": "oidc", diff --git a/operator/internal/controller/reconcilers/auth_test.go b/operator/internal/controller/reconcilers/auth_test.go index d0ca98b..4739125 100644 --- a/operator/internal/controller/reconcilers/auth_test.go +++ b/operator/internal/controller/reconcilers/auth_test.go @@ -3,6 +3,7 @@ package reconcilers import ( "testing" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" llmv1alpha1 "github.com/nebari-dev/nebari-llm-serving-pack/operator/api/v1alpha1" @@ -54,14 +55,55 @@ func defaultAuthConfig() *config.OperatorConfig { } } +func TestClientIDsFromSecret(t *testing.T) { + t.Parallel() + tests := []struct { + name string + secret *corev1.Secret + want []string + }{ + {name: "nil secret", secret: nil, want: []string{}}, + { + name: "empty data", + secret: &corev1.Secret{Data: map[string][]byte{}}, + want: []string{}, + }, + { + name: "keys returned sorted", + secret: &corev1.Secret{Data: map[string][]byte{ + "user-chuck-2": []byte("k2"), + "user-alice-1": []byte("k1"), + "user-chuck-1": []byte("k0"), + }}, + want: []string{"user-alice-1", "user-chuck-1", "user-chuck-2"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ClientIDsFromSecret(tt.secret) + if len(got) != len(tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + for i := range tt.want { + if got[i] != tt.want[i] { + t.Errorf("index %d: got %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} + func TestBuildAuthResources(t *testing.T) { //nolint:gocyclo // table-driven test t.Parallel() tests := []struct { - name string - model *llmv1alpha1.LLMModel - cfg *config.OperatorConfig - check func(t *testing.T, result *AuthResources, err error) + name string + model *llmv1alpha1.LLMModel + cfg *config.OperatorConfig + clientIDs []string + credentialSecretNames []string + check func(t *testing.T, result *AuthResources, err error) }{ { name: "API key Secret: correct name and namespace", @@ -214,8 +256,86 @@ func TestBuildAuthResources(t *testing.T) { //nolint:gocyclo // table-driven tes } }, }, - // NOTE: sanitize and forwardClientIDHeader are Envoy Gateway v1.7+ features. - // Tests for these fields will be added when minimum EG version is bumped. + { + name: "External SecurityPolicy: sets forwardClientIDHeader and sanitize", + model: defaultAuthModel(), + cfg: defaultAuthConfig(), + clientIDs: []string{"user-chuck-1"}, + check: func(t *testing.T, result *AuthResources, err error) { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + spec := result.ExternalSecurityPolicy.Object["spec"].(map[string]interface{}) + apiKeyAuth := spec["apiKeyAuth"].(map[string]interface{}) + if apiKeyAuth["forwardClientIDHeader"] != apiKeyClientIDHeader { + t.Errorf("expected forwardClientIDHeader=x-llm-client-id, got %v", apiKeyAuth["forwardClientIDHeader"]) + } + if apiKeyAuth["sanitize"] != true { + t.Errorf("expected sanitize=true, got %v", apiKeyAuth["sanitize"]) + } + }, + }, + { + name: "External SecurityPolicy: authorization allow-list is exactly this model's client IDs", + model: defaultAuthModel(), + cfg: defaultAuthConfig(), + clientIDs: []string{"user-alice-1", "user-chuck-2"}, + check: func(t *testing.T, result *AuthResources, err error) { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + spec := result.ExternalSecurityPolicy.Object["spec"].(map[string]interface{}) + authz, ok := spec["authorization"].(map[string]interface{}) + if !ok { + t.Fatal("expected authorization block on external SecurityPolicy") + } + if authz["defaultAction"] != authzActionDeny { + t.Errorf("expected defaultAction=Deny, got %v", authz["defaultAction"]) + } + rules := authz["rules"].([]interface{}) + if len(rules) != 1 { + t.Fatalf("expected 1 rule, got %d", len(rules)) + } + principal := rules[0].(map[string]interface{})["principal"].(map[string]interface{}) + headers := principal["headers"].([]interface{}) + hm := headers[0].(map[string]interface{}) + if hm["name"] != apiKeyClientIDHeader { + t.Errorf("expected header name x-llm-client-id, got %v", hm["name"]) + } + values := hm["values"].([]interface{}) + if len(values) != 2 || values[0] != "user-alice-1" || values[1] != "user-chuck-2" { + t.Errorf("expected values=[user-alice-1 user-chuck-2], got %v", values) + } + // A foreign client ID must NOT be in the allow-list. + for _, v := range values { + if v == "user-bob-9" { + t.Errorf("foreign client id leaked into allow-list: %v", values) + } + } + }, + }, + { + name: "External SecurityPolicy: zero client IDs renders deny-all with no allow rule", + model: defaultAuthModel(), + cfg: defaultAuthConfig(), + clientIDs: nil, + check: func(t *testing.T, result *AuthResources, err error) { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + spec := result.ExternalSecurityPolicy.Object["spec"].(map[string]interface{}) + authz, ok := spec["authorization"].(map[string]interface{}) + if !ok { + t.Fatal("expected authorization block even with zero client IDs") + } + if authz["defaultAction"] != authzActionDeny { + t.Errorf("expected defaultAction=Deny, got %v", authz["defaultAction"]) + } + if _, present := authz["rules"]; present { + t.Errorf("expected no rules key for zero client IDs, got %v", authz["rules"]) + } + }, + }, { name: "External SecurityPolicy: extractFrom headers includes Authorization", model: defaultAuthModel(), @@ -418,7 +538,7 @@ func TestBuildAuthResources(t *testing.T) { //nolint:gocyclo // table-driven tes if !ok { t.Fatal("expected authorization block on internal SecurityPolicy") } - if authz["defaultAction"] != "Deny" { + if authz["defaultAction"] != authzActionDeny { t.Errorf("expected defaultAction=Deny, got %v", authz["defaultAction"]) } rules := authz["rules"].([]interface{}) @@ -426,7 +546,7 @@ func TestBuildAuthResources(t *testing.T) { //nolint:gocyclo // table-driven tes t.Fatalf("expected 1 rule, got %d", len(rules)) } rule := rules[0].(map[string]interface{}) - if rule["action"] != "Allow" { + if rule["action"] != authzActionAllow { t.Errorf("expected action=Allow, got %v", rule["action"]) } principal := rule["principal"].(map[string]interface{}) @@ -573,12 +693,49 @@ func TestBuildAuthResources(t *testing.T) { //nolint:gocyclo // table-driven tes } }, }, + { + name: "External SecurityPolicy: credentialRefs pools all Secrets; authz scopes to own client IDs", + model: defaultAuthModel(), + cfg: defaultAuthConfig(), + clientIDs: []string{"user-alice-1"}, + credentialSecretNames: []string{"a-api-keys", "b-api-keys", "my-model-api-keys"}, + check: func(t *testing.T, result *AuthResources, err error) { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + spec := result.ExternalSecurityPolicy.Object["spec"].(map[string]interface{}) + apiKeyAuth := spec["apiKeyAuth"].(map[string]interface{}) + refs := apiKeyAuth["credentialRefs"].([]interface{}) + if len(refs) != 3 { + t.Fatalf("expected 3 pooled credentialRefs, got %d", len(refs)) + } + gotNames := map[string]bool{} + for _, r := range refs { + gotNames[r.(map[string]interface{})["name"].(string)] = true + } + for _, want := range []string{"a-api-keys", "b-api-keys", "my-model-api-keys"} { + if !gotNames[want] { + t.Errorf("credentialRefs missing %q (have %v)", want, gotNames) + } + } + // Pooled authn, but authorization still scopes to THIS model's own client IDs. + authz := spec["authorization"].(map[string]interface{}) + vals := authz["rules"].([]interface{})[0].(map[string]interface{})["principal"].(map[string]interface{})["headers"].([]interface{})[0].(map[string]interface{})["values"].([]interface{}) + if len(vals) != 1 || vals[0] != "user-alice-1" { + t.Errorf("authz allow-list should be own client IDs [user-alice-1], got %v", vals) + } + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - result, err := BuildAuthResources(tt.model, tt.cfg) + creds := tt.credentialSecretNames + if creds == nil { + creds = []string{APIKeySecretName(tt.model.Name)} + } + result, err := BuildAuthResources(tt.model, tt.cfg, tt.clientIDs, creds) tt.check(t, result, err) }) } diff --git a/operator/internal/controller/reconcilers/common.go b/operator/internal/controller/reconcilers/common.go index 65e0686..f423410 100644 --- a/operator/internal/controller/reconcilers/common.go +++ b/operator/internal/controller/reconcilers/common.go @@ -55,9 +55,14 @@ func SetOwnerReference(model *llmv1alpha1.LLMModel, obj metav1.Object, scheme *r return controllerutil.SetControllerReference(model, obj, scheme) } +// APIKeySecretSuffix is the naming suffix shared by every api-keys Secret. +// APIKeySecretName appends it, and the controllers' Secret watches filter on +// it to recognize api-keys Secrets among operator-managed Secrets. +const APIKeySecretSuffix = "-api-keys" + // APIKeySecretName returns the name of the Secret holding API keys for the given model. func APIKeySecretName(modelName string) string { - return modelName + "-api-keys" + return modelName + APIKeySecretSuffix } // APIKeyMetadataConfigMapName returns the name of the ConfigMap holding API key metadata for the given model. diff --git a/operator/internal/controller/reconcilers/inferencepool.go b/operator/internal/controller/reconcilers/inferencepool.go index 39bba48..6cf2f11 100644 --- a/operator/internal/controller/reconcilers/inferencepool.go +++ b/operator/internal/controller/reconcilers/inferencepool.go @@ -314,6 +314,16 @@ func buildEPPRole(eppName string, labels map[string]string) *rbacv1.Role { Resources: []string{"inferencepools"}, Verbs: []string{"get", "list", "watch"}, }, + { + // The llm-d inference scheduler (EPP) v0.8.x watches these GIE + // resources; without RBAC the informers never sync and the EPP + // crash-loops ("failed waiting for *v1alpha2.InferenceObjective + // Informer to sync"), so the served model never gets a healthy + // upstream. + APIGroups: []string{"inference.networking.x-k8s.io"}, + Resources: []string{"inferenceobjectives", "inferencemodelrewrites", "inferencepools", "inferencepoolimports"}, + Verbs: []string{"get", "list", "watch"}, + }, { APIGroups: []string{""}, Resources: []string{"pods"}, diff --git a/operator/internal/controller/reconcilers/modelservice.go b/operator/internal/controller/reconcilers/modelservice.go index e6d7c89..ce7c410 100644 --- a/operator/internal/controller/reconcilers/modelservice.go +++ b/operator/internal/controller/reconcilers/modelservice.go @@ -194,10 +194,24 @@ func buildVLLMContainer( port8000 := intstr.FromString("http") + // The default serving image (llm-d-cuda) sets its entrypoint to the NVIDIA + // CUDA wrapper and ships no default CMD, so the vLLM command must be + // explicit. Without it the wrapper exec's the vLLM flags as if they were + // the command ("exec: --: invalid option") and the container crash-loops. + // This is the standard vLLM OpenAI-server entrypoint; the image's venv + // python3 is first on PATH. (Pack-specific divergence from the upstream + // llm-d-modelservice chart, which leaves the command to the image.) + // spec.serving.command overrides it for images with a different launcher. + command := []string{"python3", "-m", "vllm.entrypoints.openai.api_server"} + if len(model.Spec.Serving.Command) > 0 { + command = model.Spec.Serving.Command + } + container := corev1.Container{ - Name: "vllm", - Image: image, - Args: args, + Name: "vllm", + Image: image, + Command: command, + Args: args, Ports: []corev1.ContainerPort{ {Name: "http", ContainerPort: 8000, Protocol: corev1.ProtocolTCP}, }, diff --git a/operator/internal/controller/reconcilers/modelservice_test.go b/operator/internal/controller/reconcilers/modelservice_test.go index ac828a5..a8e2e4f 100644 --- a/operator/internal/controller/reconcilers/modelservice_test.go +++ b/operator/internal/controller/reconcilers/modelservice_test.go @@ -232,6 +232,52 @@ func TestBuildModelServiceResources(t *testing.T) { //nolint:gocyclo // table-dr assertArgValue(t, args, "--served-model-name", "mistralai/Mistral-7B-v0.1") }, }, + { + name: "vllm container sets an explicit command (serving image entrypoint is the CUDA wrapper, no default CMD)", + model: defaultModel(), + storage: defaultStorage(), + cfg: defaultConfig(), + check: func(t *testing.T, result *ModelServiceResources, err error) { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cmd := result.Deployment.Spec.Template.Spec.Containers[0].Command + want := []string{"python3", "-m", "vllm.entrypoints.openai.api_server"} + if len(cmd) != len(want) { + t.Fatalf("expected vllm command %v, got %v", want, cmd) + } + for i := range want { + if cmd[i] != want[i] { + t.Errorf("vllm command[%d] = %q, want %q", i, cmd[i], want[i]) + } + } + }, + }, + { + name: "spec.serving.command overrides the default vllm command", + model: func() *llmv1alpha1.LLMModel { + m := defaultModel() + m.Spec.Serving.Command = []string{"vllm", "serve"} + return m + }(), + storage: defaultStorage(), + cfg: defaultConfig(), + check: func(t *testing.T, result *ModelServiceResources, err error) { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + cmd := result.Deployment.Spec.Template.Spec.Containers[0].Command + want := []string{"vllm", "serve"} + if len(cmd) != len(want) { + t.Fatalf("expected vllm command %v, got %v", want, cmd) + } + for i := range want { + if cmd[i] != want[i] { + t.Errorf("vllm command[%d] = %q, want %q", i, cmd[i], want[i]) + } + } + }, + }, { name: "tensorParallelism explicit value used", model: func() *llmv1alpha1.LLMModel { diff --git a/operator/internal/controller/reconcilers/passthrough.go b/operator/internal/controller/reconcilers/passthrough.go index b6acf2c..bf0d00d 100644 --- a/operator/internal/controller/reconcilers/passthrough.go +++ b/operator/internal/controller/reconcilers/passthrough.go @@ -45,7 +45,7 @@ func PassthroughStandardLabels(pm *llmv1alpha1.PassthroughModel) map[string]stri // BuildPassthroughResources is a pure function that computes every resource // for the given PassthroughModel. No serving, storage, or scheduling // resources are involved; the provider serves the models, we route to it. -func BuildPassthroughResources(pm *llmv1alpha1.PassthroughModel, cfg *config.OperatorConfig) (*PassthroughResources, error) { +func BuildPassthroughResources(pm *llmv1alpha1.PassthroughModel, cfg *config.OperatorConfig, clientIDs []string, credentialSecretNames []string) (*PassthroughResources, error) { labels := PassthroughStandardLabels(pm) authLabels := map[string]string{} for k, v := range labels { @@ -97,7 +97,8 @@ func BuildPassthroughResources(pm *llmv1alpha1.PassthroughModel, cfg *config.Ope pm.Namespace, labelsToInterface(labels), pm.Name+"-external", - APIKeySecretName(pm.Name), + credentialSecretNames, + clientIDs, ) } @@ -259,9 +260,15 @@ func buildProviderBackendSecurityPolicy(pm *llmv1alpha1.PassthroughModel, labels } // buildPassthroughRoute renders the AIGatewayRoute for one endpoint of a -// PassthroughModel. Rule order matters only for readability: Gateway API -// precedence (more header matches wins) is what keeps served LLMModels, -// whose routes match Host AND x-ai-eg-model, ahead of the catch-all rule. +// PassthroughModel. Rule order within this route matters only for +// readability. NOTE: since served-model rules lost their Host matcher +// (AI Gateway v0.5 model registration; #116), header-count precedence no +// longer orders a served rule (x-ai-eg-model) against the opt-in catch-all +// rule (Host). Live-validated on EG v1.6.7 / AI Gateway v0.5: dispatch is +// decided by the ext_proc's model registry, so served/declared ids always +// reach their own rule regardless of route age, and unregistered ids 404 at +// the ext_proc before route matching - which leaves the catch-all rule +// currently inert (see the comment in the CatchAll block below). // // sectionName scoping is load-bearing for the same reason as in // buildAIGatewayRoute: the AI Gateway controller appends a catch-all @@ -290,7 +297,6 @@ func buildPassthroughRoute( for _, id := range pm.Spec.Models.Declared { matches = append(matches, map[string]interface{}{ "headers": []interface{}{ - hostHeader, map[string]interface{}{ "type": "Exact", "name": "x-ai-eg-model", @@ -311,6 +317,14 @@ func buildPassthroughRoute( } if pm.Spec.Models.CatchAll { + // The catch-all rule (opt-in; catchAll defaults false) keeps the Host + // matcher. It carries no x-ai-eg-model header, so it is not a + // model-registration rule and is not affected by the AI Gateway v0.5 + // issue fixed for the declared-model rules (#116). Live-tested on + // EG v1.6.7 / AI Gateway v0.5: the ext_proc 404s any model id not + // registered by some rule before route matching runs, so this rule + // currently receives no traffic. Kept for a future AI Gateway version + // where unregistered ids fall through to route matching. rules = append(rules, map[string]interface{}{ "matches": []interface{}{ map[string]interface{}{ diff --git a/operator/internal/controller/reconcilers/passthrough_test.go b/operator/internal/controller/reconcilers/passthrough_test.go index 45a8f27..e09227e 100644 --- a/operator/internal/controller/reconcilers/passthrough_test.go +++ b/operator/internal/controller/reconcilers/passthrough_test.go @@ -85,7 +85,7 @@ func routeRules(t *testing.T, route *unstructured.Unstructured) []interface{} { func TestBuildPassthroughResourcesProviderPlumbing(t *testing.T) { pm := testPassthroughModel() - res, err := BuildPassthroughResources(pm, testPassthroughConfig()) + res, err := BuildPassthroughResources(pm, testPassthroughConfig(), nil, []string{APIKeySecretName(pm.Name)}) if err != nil { t.Fatalf("BuildPassthroughResources returned error: %v", err) } @@ -173,7 +173,7 @@ func TestBuildPassthroughResourcesProviderPlumbing(t *testing.T) { func TestBuildPassthroughResourcesKeySecretAndConfigMap(t *testing.T) { pm := testPassthroughModel() - res, err := BuildPassthroughResources(pm, testPassthroughConfig()) + res, err := BuildPassthroughResources(pm, testPassthroughConfig(), nil, []string{APIKeySecretName(pm.Name)}) if err != nil { t.Fatalf("BuildPassthroughResources returned error: %v", err) } @@ -255,7 +255,7 @@ func TestBuildPassthroughResourcesRoutes(t *testing.T) { t.Run(tt.name, func(t *testing.T) { pm := testPassthroughModel() tt.mutate(pm) - res, err := BuildPassthroughResources(pm, testPassthroughConfig()) + res, err := BuildPassthroughResources(pm, testPassthroughConfig(), nil, []string{APIKeySecretName(pm.Name)}) if err != nil { t.Fatalf("BuildPassthroughResources returned error: %v", err) } @@ -288,7 +288,7 @@ func TestBuildPassthroughResourcesRoutes(t *testing.T) { func TestBuildPassthroughRouteDetails(t *testing.T) { pm := testPassthroughModel() cfg := testPassthroughConfig() - res, err := BuildPassthroughResources(pm, cfg) + res, err := BuildPassthroughResources(pm, cfg, nil, []string{APIKeySecretName(pm.Name)}) if err != nil { t.Fatalf("BuildPassthroughResources returned error: %v", err) } @@ -314,25 +314,20 @@ func TestBuildPassthroughRouteDetails(t *testing.T) { if len(matches) != 2 { t.Fatalf("declared matches = %d, want 2", len(matches)) } - // Every declared match carries Host AND x-ai-eg-model exact headers. + // Every declared match carries ONLY the x-ai-eg-model exact header - no + // Host matcher (removed for AI Gateway v0.5 model registration; #116). seen := map[string]bool{} for _, m := range matches { headers, _ := m.(map[string]interface{})["headers"].([]interface{}) - var hostOK bool for _, h := range headers { hm, _ := h.(map[string]interface{}) - switch hm["name"] { - case ptHostHeader: - if hm["value"] == ptExternalHost && hm["type"] == "Exact" { - hostOK = true - } - case ptModelHeader: + if hm["name"] == ptHostHeader { + t.Errorf("declared match must not carry a Host header: %v", m) + } + if hm["name"] == ptModelHeader { seen[hm["value"].(string)] = true } } - if !hostOK { - t.Errorf("declared match missing exact Host header: %v", m) - } } if !seen["openai/gpt-5.2"] || !seen["anthropic/claude-opus-4.6"] { t.Errorf("declared model ids = %v", seen) @@ -381,7 +376,7 @@ func TestBuildPassthroughRouteDetails(t *testing.T) { func TestBuildPassthroughResourcesSecurityPolicies(t *testing.T) { pm := testPassthroughModel() cfg := testPassthroughConfig() - res, err := BuildPassthroughResources(pm, cfg) + res, err := BuildPassthroughResources(pm, cfg, nil, []string{APIKeySecretName(pm.Name)}) if err != nil { t.Fatalf("BuildPassthroughResources returned error: %v", err) } @@ -424,7 +419,7 @@ func TestBuildPassthroughResourcesSecurityPolicies(t *testing.T) { if !ok { t.Fatal("expected authorization block for non-public access") } - if authz["defaultAction"] != "Deny" { + if authz["defaultAction"] != authzActionDeny { t.Errorf("defaultAction = %v", authz["defaultAction"]) } }) @@ -432,7 +427,7 @@ func TestBuildPassthroughResourcesSecurityPolicies(t *testing.T) { t.Run("public access drops authorization", func(t *testing.T) { pub := testPassthroughModel() pub.Spec.Access = llmv1alpha1.AccessSpec{Public: ptr.To(true)} - pubRes, err := BuildPassthroughResources(pub, cfg) + pubRes, err := BuildPassthroughResources(pub, cfg, nil, []string{APIKeySecretName(pub.Name)}) if err != nil { t.Fatalf("BuildPassthroughResources returned error: %v", err) } @@ -441,3 +436,37 @@ func TestBuildPassthroughResourcesSecurityPolicies(t *testing.T) { } }) } + +func TestBuildPassthroughExternalAuthorization(t *testing.T) { + t.Parallel() + pm := testPassthroughModel() + cfg := testPassthroughConfig() + + withKeys, err := BuildPassthroughResources(pm, cfg, []string{"user-chuck-1"}, []string{APIKeySecretName(pm.Name)}) + if err != nil { + t.Fatalf("BuildPassthroughResources returned error: %v", err) + } + spec := withKeys.ExternalSecurityPolicy.Object["spec"].(map[string]interface{}) + apiKeyAuth := spec["apiKeyAuth"].(map[string]interface{}) + if apiKeyAuth["forwardClientIDHeader"] != apiKeyClientIDHeader || apiKeyAuth["sanitize"] != true { + t.Errorf("expected forwardClientIDHeader/sanitize set, got %v", apiKeyAuth) + } + authz := spec["authorization"].(map[string]interface{}) + if authz["defaultAction"] != authzActionDeny { + t.Errorf("expected defaultAction=Deny, got %v", authz["defaultAction"]) + } + rules := authz["rules"].([]interface{}) + values := rules[0].(map[string]interface{})["principal"].(map[string]interface{})["headers"].([]interface{})[0].(map[string]interface{})["values"].([]interface{}) + if len(values) != 1 || values[0] != "user-chuck-1" { + t.Errorf("expected values=[user-chuck-1], got %v", values) + } + + noKeys, err := BuildPassthroughResources(pm, cfg, nil, []string{APIKeySecretName(pm.Name)}) + if err != nil { + t.Fatalf("BuildPassthroughResources returned error: %v", err) + } + authzNo := noKeys.ExternalSecurityPolicy.Object["spec"].(map[string]interface{})["authorization"].(map[string]interface{}) + if _, present := authzNo["rules"]; present { + t.Errorf("expected deny-all (no rules) for zero client IDs, got %v", authzNo["rules"]) + } +} diff --git a/operator/internal/controller/reconcilers/routing.go b/operator/internal/controller/reconcilers/routing.go index 80cdf61..10133df 100644 --- a/operator/internal/controller/reconcilers/routing.go +++ b/operator/internal/controller/reconcilers/routing.go @@ -54,7 +54,6 @@ func BuildRoutingResources(model *llmv1alpha1.LLMModel, cfg *config.OperatorConf ExternalHTTPSListenerName, model.Name, model.Spec.Model.Name, - SharedExternalHostname(cfg.BaseDomain), ) } @@ -68,7 +67,6 @@ func BuildRoutingResources(model *llmv1alpha1.LLMModel, cfg *config.OperatorConf InternalHTTPSListenerName, model.Name, model.Spec.Model.Name, - SharedInternalHostname(cfg.BaseDomain), ) } @@ -82,7 +80,6 @@ func buildAIGatewayRoute( listenerSectionName string, poolName string, modelName string, - hostname string, ) *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -103,8 +100,8 @@ func buildAIGatewayRoute( // SecurityPolicy bound to this HTTPRoute catches traffic // bound for unrelated hostnames (key-manager UI, argocd, // keycloak, base domain). Regression seen in alpha.3/alpha.4; - // the Host header matcher on the first rule narrowed that - // rule but had no effect on the catch-all. + // sectionName is the sole mechanism that contains this (model + // rules match only x-ai-eg-model, no Host matcher). "parentRefs": []interface{}{ map[string]interface{}{ "name": gatewayName, @@ -114,27 +111,20 @@ func buildAIGatewayRoute( }, "rules": []interface{}{ map[string]interface{}{ - // Match on BOTH the shared hostname (Host header) AND the - // `x-ai-eg-model` header. With sectionName scoping - // on parentRefs above, the Host match is defence in - // depth: it guarantees the rule only fires for the - // intended FQDN even if a future refactor loosens - // parent attachment. + // Match on the `x-ai-eg-model` header for per-model dispatch on the + // shared listener. The Envoy AI Gateway extproc derives this value from + // the request body's `model` field before HTTPRoute matching runs. // - // The x-ai-eg-model match gives us per-model dispatch - // once a request lands on a shared listener. The - // Envoy AI Gateway extproc filter derives the value - // from the request body's `model` field before - // HTTPRoute matching runs. Both matches are ANDed - // (per Gateway API spec). + // We match ONLY x-ai-eg-model, not Host: sectionName on parentRefs + // already scopes this route to the llm-https listener (whose own + // hostname is the FQDN), so a Host matcher was redundant. Critically, + // the Envoy AI Gateway v0.5 controller does not register a model whose + // match rule carries any header beyond x-ai-eg-model - the extra Host + // matcher made every request 404 "model not configured in the Gateway" + // (nebari-dev/llm-serving-pack#116). "matches": []interface{}{ map[string]interface{}{ "headers": []interface{}{ - map[string]interface{}{ - "type": "Exact", - "name": "Host", - "value": hostname, - }, map[string]interface{}{ "type": "Exact", "name": "x-ai-eg-model", diff --git a/operator/internal/controller/reconcilers/routing_test.go b/operator/internal/controller/reconcilers/routing_test.go index 376dc7d..bb6e67c 100644 --- a/operator/internal/controller/reconcilers/routing_test.go +++ b/operator/internal/controller/reconcilers/routing_test.go @@ -67,9 +67,11 @@ func modelHeaderMatchValue(t *testing.T, route *unstructured.Unstructured) strin return v } -// hostHeaderMatchValue returns the value of the Host header matcher on the -// first rule of the given AIGatewayRoute, or fails the test. -func hostHeaderMatchValue(t *testing.T, route *unstructured.Unstructured) string { +// hostMatcherPresent reports whether the first rule of the given AIGatewayRoute +// has a Host header matcher. Routes must NOT carry one: the Envoy AI Gateway +// v0.5 controller will not register a model whose match rule has any header +// beyond x-ai-eg-model (see buildAIGatewayRoute; nebari-dev/llm-serving-pack#116). +func hostMatcherPresent(t *testing.T, route *unstructured.Unstructured) bool { t.Helper() if route == nil { t.Fatal("expected route to be non-nil") @@ -83,15 +85,10 @@ func hostHeaderMatchValue(t *testing.T, route *unstructured.Unstructured) string for _, h := range headers { header, _ := h.(map[string]interface{}) if header["name"] == "Host" { - v, ok := header["value"].(string) - if !ok { - t.Fatalf("expected Host header value to be a string, got %T", header["value"]) - } - return v + return true } } - t.Fatalf("did not find a Host header matcher in %v", headers) - return "" + return false } func defaultRoutingModel() *llmv1alpha1.LLMModel { @@ -302,32 +299,28 @@ func TestBuildRoutingResources(t *testing.T) { //nolint:gocyclo // table-driven }, }, { - name: "External: rule has Host matcher for shared external hostname (scopes route to shared listener)", + name: "External: rule has NO Host matcher (removed for AI Gateway v0.5 model registration; #116)", model: defaultRoutingModel(), cfg: defaultRoutingConfig(), check: func(t *testing.T, result *RoutingResources, err error) { if err != nil { t.Fatalf("unexpected error: %v", err) } - got := hostHeaderMatchValue(t, result.ExternalRoute) - want := "llm.example.com" - if got != want { - t.Errorf("expected external Host match %q, got %q", want, got) + if hostMatcherPresent(t, result.ExternalRoute) { + t.Error("external route must not carry a Host matcher; sectionName scopes it to the llm-https listener and a Host matcher breaks AI Gateway v0.5 model registration") } }, }, { - name: "Internal: rule has Host matcher for shared internal hostname", + name: "Internal: rule has NO Host matcher", model: defaultRoutingModel(), cfg: defaultRoutingConfig(), check: func(t *testing.T, result *RoutingResources, err error) { if err != nil { t.Fatalf("unexpected error: %v", err) } - got := hostHeaderMatchValue(t, result.InternalRoute) - want := "llm-internal.example.com" - if got != want { - t.Errorf("expected internal Host match %q, got %q", want, got) + if hostMatcherPresent(t, result.InternalRoute) { + t.Error("internal route must not carry a Host matcher") } }, }, diff --git a/scripts/check-content-parity.mjs b/scripts/check-content-parity.mjs index 9242fdd..3d6edee 100644 --- a/scripts/check-content-parity.mjs +++ b/scripts/check-content-parity.mjs @@ -10,7 +10,7 @@ const REF = process.env.HUGO_REF || 'origin/docs-site'; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const docsDir = path.join(repoRoot, 'docs/src/content/docs'); -// Three files are intentionally excluded from this parity check: +// Five files are intentionally excluded from this parity check: // - cicd-and-releasing.md documents the Astro docs workflow (not byte-identical to // the Hugo source by design, user-approved). // - installation.md has its screenshot references rewritten from /public absolute @@ -19,6 +19,12 @@ const docsDir = path.join(repoRoot, 'docs/src/content/docs'); // - local-development.md was updated by PR #115 to document the AI Gateway v0.5 // local dev flow (real ext_proc, passthrough inference, key-manager UI dev mode), // so its body intentionally diverges from the frozen Hugo source. +// - architecture.md was updated by PR #117 (#116) to document the now-enabled +// per-model API-key authorization on the external endpoint, so its body +// intentionally diverges from the frozen Hugo source. +// - configuration.md was updated by PR #117 to document the new +// spec.serving.command override, so its body intentionally diverges from +// the frozen Hugo source. // migrated file (in src/content/docs) -> hugo source basename (in docs/site/content) const MAP = { @@ -26,8 +32,6 @@ const MAP = { 'quickstart.md': 'quickstart.md', 'shared-storage.md': 'shared-storage.md', 'troubleshooting.md': 'troubleshooting.md', - 'configuration.md': 'configuration.md', - 'architecture.md': 'architecture.md', }; // Strip a leading TOML (+++) or YAML (---) frontmatter block; return the body.