feat(layer-5): Kubernetes operator + Helm chart (MCPContract CRD) - #5
Conversation
…stack - Metric tool labels are clamped to the baseline set (else "unknown"): a client-supplied tool name must not mint unbounded Prometheus timeseries (label-cardinality DoS). Regression test: three attacker names collapse into one "unknown" series. - Compose binds Prometheus and Grafana to 127.0.0.1 and drops anonymous Grafana from Admin to Viewer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- MCPContract CRD (covenant.dev/v1alpha1): spec = server URL + baseline ConfigMap ref + intervalSeconds (+ optional proxyRefreshUrl); status shows RESULT / BREAKING / LAST CHECK as kubectl printer columns. - kopf operator: a 30s timer due-gated per CR to its own intervalSeconds runs the existing Layer 0/3 check (schema diff + baselined probes) in-process and patches the verdict into status. Unreachable server or bad baseline becomes result: error - the operator never crash-loops on one bad contract. On drift it nudges the proxy's /covenant/refresh (best-effort) so quarantine follows. - Purity split: reconcile.py has zero kopf/kubernetes imports and unit-tests cluster-free (8 new tests); handlers.py is thin glue. - Helm chart: CRD + proxy Deployment/Service + operator Deployment with least-privilege RBAC (mcpcontracts+status, configmaps get, events create). One Dockerfile serves both roles. Example CR in examples/mcpcontract.yaml. - parse_baseline extracted from read_baseline so a ConfigMap value parses the same as a file. kopf/kubernetes ride a new [operator] extra. In-operator checks instead of probes-as-Jobs - named decision in the Layer 5 spec (status feedback from Job pods costs heavy machinery for a millisecond check). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds operator reconciliation, proxy baseline reload and metrics label clamping, store quarantine-only updates, Helm/runtime packaging, local exposure hardening, and documentation/test coverage. ChangesCovenant operator, proxy refresh, and supporting updates
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant KopfTimer as Kopf timer
participant ConfigMap as Kubernetes ConfigMap
participant Reconcile as covenant.operator.reconcile
participant Proxy as proxy refresh URL
KopfTimer->>ConfigMap: read baselineConfigMap key
ConfigMap-->>KopfTimer: baseline text
KopfTimer->>Reconcile: due(...)
KopfTimer->>Reconcile: check_contract(...)
Reconcile-->>KopfTimer: status patch
KopfTimer->>Proxy: POST refresh (if enabled and not error)
sequenceDiagram
participant Client as client
participant ProxyApp as covenant.proxy.server
participant Disk as baseline file
participant Upstream as upstream server
Client->>ProxyApp: POST /covenant/refresh
ProxyApp->>Disk: read_baseline(baseline_path)
Disk-->>ProxyApp: updated baseline text
ProxyApp->>Upstream: tools/list
Upstream-->>ProxyApp: live tools
ProxyApp-->>Client: refresh result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Correctness (found by an 8-angle adversarial review, each verified): - reconcile.check_contract catches all exceptions, not just CovenantError: a valid-JSON-but-wrong-shape baseline now lands in status.result: error instead of escaping into a kopf retry loop (parse_baseline also rejects non-object roots for a clean CLI exit 2) - /covenant/refresh re-reads the baseline from disk before diffing, so a re-snapshot / updated ConfigMap can't split-brain the operator (clean) against the proxy (false quarantine); metric label set follows - error statuses zero the tier counts: kopf merge-patches status, so omitted counts left a previous check's numbers next to result: error (shared reconcile.error_status used by both error paths) - due() treats a naive lastCheckTime as unreadable instead of raising TypeError (aware minus naive) out of the timer forever - missing ConfigMap key is status.result: error, not kopf.PermanentError: kopf timers re-fire regardless and the CR showed no status at all - kopf sync-handler executor pinned to 32 workers so a few hung MCP servers can't starve every other CR's timer - Helm labels/selectors carry the release name; two releases in one namespace no longer cross-select each other's pods - Dockerfile drops ENTRYPOINT so both documented invocations run verbatim (Helm proxy args gain command: [covenant]) - compose Postgres bound to 127.0.0.1 like the rest of the dev stack Cleanup (clean-code + ponytail passes): - report.summarize is the one severity ladder (reconcile + render reuse it; unknown tiers now raise instead of being silently spread into status) - CRD schema defaults deleted: code defaults are the single source of truth (config.DEFAULT_BASELINE, reconcile.DEFAULT_INTERVAL_S) - dev extra self-references [proxy,store,judge,operator] pins Docs: layer 5 spec decisions updated, README refresh bullet, new DEVELOPER-GUIDE.md (per-layer map, demo script, honest limitations, audience framings). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bf8fa6a to
57c44cc
Compare
Ran the full Layer 5 lifecycle on a real cluster (Docker Desktop K8s): helm install -> operator reconciles -> clean -> injected drift -> RESULT=breaking BREAKING=2 (schema + probe rows) -> proxy quarantines get_account via the refresh nudge -> server restored -> clean again, quarantine released. Two bugs no unit test could see: - operator RBAC was below kopf's floor: kopf lists CRDs and (under --all-namespaces) namespaces at startup and 403-looped before serving a single contract; ClusterRole now grants both (list/watch/get) - the MCP SDK's DNS-rebinding guard rejects host.docker.internal with 421, so a cluster could never reach a host-run demo server; the example server now extends the allowlist via COVENANT_ALLOWED_HOSTS (default stays localhost-only) Docs: spec RBAC decision updated, DEVELOPER-GUIDE verification section now records the cluster-verified lifecycle (Grafana remains the one unexercised surface), README test count corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
covenant/contract.py (1)
90-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTOCTOU in
read_baseline:exists()check then separateread_text().If the file is removed/replaced between the
p.exists()check andparse_baseline(p.read_text(...)), the read raisesFileNotFoundError/OSErrorinstead ofBaselineError. This escapes the narrowexcept CovenantErrorincovenant/proxy/server.py's/covenant/refresh(lines 243-247) and incovenant/cli.py'sproxycommand (line 172-175), surfacing as an unhandled 500/traceback instead of the intended clean error message.🛡️ Proposed fix: collapse the check-then-read into a single try/except
def read_baseline(path: str | Path) -> tuple[str, list[JsonDict], list[JsonDict]]: """Read a baseline file; return (server, wire-shape tool dicts, probe records).""" p = Path(path) - if not p.exists(): - raise BaselineError(f"baseline not found: {p} (run `covenant snapshot` first)") - return parse_baseline(p.read_text(encoding="utf-8"), source=str(p)) + try: + text = p.read_text(encoding="utf-8") + except OSError as e: + raise BaselineError(f"baseline not found: {p} (run `covenant snapshot` first)") from e + return parse_baseline(text, source=str(p))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@covenant/contract.py` around lines 90 - 105, `read_baseline` has a TOCTOU gap because it checks `Path.exists()` before calling `read_text()`, so a file removed or replaced in between can raise `FileNotFoundError`/`OSError` instead of `BaselineError`. Collapse the existence check and read into one `try/except` around `p.read_text(...)`, and translate any file-access failure into `BaselineError` so callers like `covenant/proxy/server.py` and `covenant/cli.py` continue to handle refresh/proxy errors cleanly.
🧹 Nitpick comments (5)
deploy/helm/covenant/templates/operator.yaml (2)
56-60: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a restrictive
securityContext.Combined with the Dockerfile running as root, this Deployment has no
securityContextconstraining privilege escalation. For a workload holding a cluster-scoped ServiceAccount token, this is worth locking down.🔒 Proposed hardening
containers: - name: operator image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" imagePullPolicy: {{ .Values.image.pullPolicy }} command: [kopf, run, -m, covenant.operator.handlers, --all-namespaces] + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: [ALL]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/covenant/templates/operator.yaml` around lines 56 - 60, Add a restrictive securityContext to the operator container in the operator Deployment to harden the workload. Update the container spec in the operator manifest to explicitly drop privilege escalation and set a non-root execution posture, using the existing container definition around the operator name and command. If appropriate for this image, also set read-only filesystem and a non-root user/group so the cluster-scoped ServiceAccount token is not running in an unconstrained root container.
56-60: 🚀 Performance & Scalability | 🔵 TrivialConsider setting resource requests/limits.
No CPU/memory requests or limits are set for the operator container, which can lead to noisy-neighbor issues or unbounded memory growth going unnoticed on shared nodes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/covenant/templates/operator.yaml` around lines 56 - 60, The operator container in the Helm template is missing resource requests and limits, so add a resources block for the operator container in the template that defines appropriate CPU and memory requests/limits using the existing operator container stanza. Update the Kubernetes manifest generation around the operator container definition so the deployment sets bounded defaults via the chart values, and ensure the new settings are wired through the corresponding values schema if needed.Dockerfile (1)
4-6: 📐 Maintainability & Code Quality | 🔵 TrivialConsider unbuffered stdout for timely log visibility.
Without
PYTHONUNBUFFERED=1, log output from the operator/proxy may buffer, delaying visibility inkubectl logsduring incident debugging.♻️ Proposed addition
FROM python:3.12-slim +ENV PYTHONUNBUFFERED=1 WORKDIR /app🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 4 - 6, The container image setup is missing unbuffered Python output, which can delay logs from the operator/proxy. Update the Dockerfile near the existing python:3.12-slim base image and WORKDIR setup to set PYTHONUNBUFFERED=1 in the image environment so stdout/stderr are flushed promptly for kubectl logs visibility.deploy/helm/covenant/templates/proxy.yaml (1)
14-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the proxy pod spec: probes, resources, and a locked-down security context.
The proxy Deployment has no
livenessProbe/readinessProbe, noresources, and inherits the default ServiceAccount token even though the proxy itself never talks to the K8s API (only the operator needs RBAC per the design docs). Worth adding before this chart is used beyond a demo.♻️ Suggested hardening
spec: + automountServiceAccountToken: false + securityContext: + runAsNonRoot: true containers: - name: proxy image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" imagePullPolicy: {{ .Values.image.pullPolicy }} command: [covenant] args: - proxy - --upstream={{ required "proxy.upstream is required" .Values.proxy.upstream }} - --baseline=/covenant/{{ .Values.proxy.baselineKey }} - --host=0.0.0.0 - --port={{ .Values.proxy.port }} ports: - containerPort: {{ .Values.proxy.port }} + readinessProbe: + httpGet: { path: /covenant/status, port: {{ .Values.proxy.port }} } + livenessProbe: + httpGet: { path: /covenant/status, port: {{ .Values.proxy.port }} } + resources: + requests: { cpu: 50m, memory: 64Mi } + limits: { memory: 256Mi } volumeMounts: - name: baseline mountPath: /covenant readOnly: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/covenant/templates/proxy.yaml` around lines 14 - 35, The proxy container spec in the proxy template is missing basic hardening. Update the proxy container definition in the template that renders the proxy Deployment to add liveness/readiness probes, explicit resource requests/limits, and a restricted securityContext. Also disable automatic ServiceAccount token mounting for this pod since the proxy does not use the Kubernetes API, and keep the changes localized around the proxy container, volumes, and pod spec fields.covenant/proxy/server.py (1)
240-249: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSynchronous file read blocks the event loop on the proxy's hot path.
read_baseline(app.state.baseline_path)performs blocking file I/O + JSON parsing directly inside anasync defroute handler, with noawait. Since this proxy sits in front of live MCP traffic on a single event loop, this call stalls all concurrently in-flight/mcpproxying for its duration (worse if the ConfigMap/lock file is on a slow or network-backed mount).⚡ Proposed fix: offload to a thread
if app.state.baseline_path: try: - _, base_tools, _ = read_baseline(app.state.baseline_path) + _, base_tools, _ = await asyncio.to_thread(read_baseline, app.state.baseline_path) except CovenantError as e: raise HTTPException(status_code=500, detail=f"baseline reload failed: {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@covenant/proxy/server.py` around lines 240 - 249, The baseline reload in the async proxy handler is doing blocking file I/O and JSON parsing on the event loop, which can stall concurrent `/mcp` traffic. Update the `read_baseline` call inside the `app.state.baseline_path` branch of `server.py` to run off-thread (for example via the existing async/thread offload pattern used elsewhere), then assign `app.state.baseline` and `app.state.baseline_names` from the returned tools as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@covenant/operator/handlers.py`:
- Around line 61-70: `operator/handlers.py` in the reconcile path still lets
`spec["server"]` raise a `KeyError` outside the existing guarded error handling,
so missing server values bypass status reporting. Move the server extraction
into the same protected block used by `_baseline_text(...)`, and on failure use
`reconcile.error_status(...)` to update `patch.status` before returning. Use the
existing `reconcile.check_contract(...)` and `patch.status.update(...)` flow so
malformed specs still land in status instead of crashing the handler.
In `@covenant/proxy/server.py`:
- Around line 243-247: The baseline reload path in server startup only handles
CovenantError, so unexpected I/O or decode failures from read_baseline() can
bypass the error translation. Update the reload logic around
app.state.baseline_path to catch OSError and UnicodeDecodeError as well as
CovenantError, and re-raise them as HTTPException with the same baseline reload
failed: ... detail so all reload failures are handled consistently.
In `@Dockerfile`:
- Around line 4-11: The container image is missing a non-root runtime user, so
the default process in the Dockerfile runs as root. Update the Dockerfile to
create and switch to a dedicated non-root user after the install steps, and
ensure the final CMD runs under that user; use the Dockerfile’s build/setup
sequence and the CMD instruction as the key places to fix this.
In `@examples/mcp_server.py`:
- Around line 29-38: Transport security is only enabled when _extra_hosts is
set, which leaves FastMCP without protection when COVENANT_ALLOWED_HOSTS is
unset. Update the FastMCP setup in mcp_server.py so transport_security always
receives a TransportSecuritySettings instance, and make the allowed_hosts list
conditional inside that config instead of returning None. Use the existing
_extra_hosts variable and the FastMCP/TransportSecuritySettings initialization
to keep localhost defaults while extending the allowlist only when extra hosts
are present.
---
Outside diff comments:
In `@covenant/contract.py`:
- Around line 90-105: `read_baseline` has a TOCTOU gap because it checks
`Path.exists()` before calling `read_text()`, so a file removed or replaced in
between can raise `FileNotFoundError`/`OSError` instead of `BaselineError`.
Collapse the existence check and read into one `try/except` around
`p.read_text(...)`, and translate any file-access failure into `BaselineError`
so callers like `covenant/proxy/server.py` and `covenant/cli.py` continue to
handle refresh/proxy errors cleanly.
---
Nitpick comments:
In `@covenant/proxy/server.py`:
- Around line 240-249: The baseline reload in the async proxy handler is doing
blocking file I/O and JSON parsing on the event loop, which can stall concurrent
`/mcp` traffic. Update the `read_baseline` call inside the
`app.state.baseline_path` branch of `server.py` to run off-thread (for example
via the existing async/thread offload pattern used elsewhere), then assign
`app.state.baseline` and `app.state.baseline_names` from the returned tools as
before.
In `@deploy/helm/covenant/templates/operator.yaml`:
- Around line 56-60: Add a restrictive securityContext to the operator container
in the operator Deployment to harden the workload. Update the container spec in
the operator manifest to explicitly drop privilege escalation and set a non-root
execution posture, using the existing container definition around the operator
name and command. If appropriate for this image, also set read-only filesystem
and a non-root user/group so the cluster-scoped ServiceAccount token is not
running in an unconstrained root container.
- Around line 56-60: The operator container in the Helm template is missing
resource requests and limits, so add a resources block for the operator
container in the template that defines appropriate CPU and memory
requests/limits using the existing operator container stanza. Update the
Kubernetes manifest generation around the operator container definition so the
deployment sets bounded defaults via the chart values, and ensure the new
settings are wired through the corresponding values schema if needed.
In `@deploy/helm/covenant/templates/proxy.yaml`:
- Around line 14-35: The proxy container spec in the proxy template is missing
basic hardening. Update the proxy container definition in the template that
renders the proxy Deployment to add liveness/readiness probes, explicit resource
requests/limits, and a restricted securityContext. Also disable automatic
ServiceAccount token mounting for this pod since the proxy does not use the
Kubernetes API, and keep the changes localized around the proxy container,
volumes, and pod spec fields.
In `@Dockerfile`:
- Around line 4-6: The container image setup is missing unbuffered Python
output, which can delay logs from the operator/proxy. Update the Dockerfile near
the existing python:3.12-slim base image and WORKDIR setup to set
PYTHONUNBUFFERED=1 in the image environment so stdout/stderr are flushed
promptly for kubectl logs visibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a3b834b-d5c9-4ff0-98c7-acaab9145e7e
📒 Files selected for processing (25)
CLAUDE.mdDEVELOPER-GUIDE.mdDockerfileREADME.mdcovenant/cli.pycovenant/contract.pycovenant/operator/__init__.pycovenant/operator/handlers.pycovenant/operator/reconcile.pycovenant/proxy/server.pycovenant/report.pydeploy/helm/covenant/Chart.yamldeploy/helm/covenant/templates/crd.yamldeploy/helm/covenant/templates/operator.yamldeploy/helm/covenant/templates/proxy.yamldeploy/helm/covenant/values.yamldocker-compose.ymldocs/superpowers/specs/2026-07-03-covenant-layer4-observability-design.mddocs/superpowers/specs/2026-07-03-covenant-layer5-k8s-operator-design.mdexamples/mcp_server.pyexamples/mcpcontract.yamlpyproject.tomltests/test_metrics.pytests/test_operator.pytests/test_proxy.py
- contract.py: read_baseline catches OSError/UnicodeDecodeError on the read itself (no exists-then-read TOCTOU); parse_baseline rejects non-object JSON - handlers.py: spec["server"] access moved inside the misconfiguration try-block so a CR missing the field becomes status.result=error, not a raise - examples/mcp_server.py: always pass explicit TransportSecuritySettings (mcp 1.10.0 left DNS-rebinding protection off when settings were None) - Dockerfile: run as non-root user (uid 1000) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prometheus scrape + quarantine flip (0->1->0) watched end-to-end, queried directly and through Grafana's datasource; every layer now exercised live. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- introspect_async is the one MCP list-tools client; the proxy's _list_upstream reuses it instead of hand-rolling a session - drop Store.set_status (called by nothing in production); InMemoryStore collapses to a plain quarantine dict - bound InMemoryStore call/drift logs with deque(maxlen=1000) - a long-running proxy must not grow memory forever - exit_code derives from summarize() so one severity ladder exists - normalize _proxy RPC parsing to a single isinstance guard No behavior change: 129 tests green, refresh + drift/clean exit codes verified live against the running demo stack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
- contract.py: read_baseline catches OSError/UnicodeDecodeError on the read itself (no exists-then-read TOCTOU); parse_baseline rejects non-object JSON - handlers.py: spec["server"] access moved inside the misconfiguration try-block so a CR missing the field becomes status.result=error, not a raise - examples/mcp_server.py: always pass explicit TransportSecuritySettings (mcp 1.10.0 left DNS-rebinding protection off when settings were None) - Dockerfile: run as non-root user (uid 1000) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat(layer-5): Kubernetes operator + Helm chart (MCPContract CRD)
@
What
Layer 5, the final roadmap row: declarative contract conformance in a cluster.
MCPContractCRD (covenant.dev/v1alpha1) — spec: server URL, baseline ConfigMap ref,intervalSeconds, optionalproxyRefreshUrl.kubectl get mcpcshows RESULT / BREAKING / LAST CHECK via printer columns./covenant/refresh(best-effort) so quarantine follows.Named decisions (Layer 5 spec)
reconcile.pyhas zero kopf/kubernetes imports; the whole layer unit-tests without a cluster.handlers.pyis glue.status.result: error, never a crash-loop — only a misconfigured CR raises (kopf.PermanentError, not retried).parse_baselineextracted fromread_baseline).Verified
import covenant.operator.handlerssmoke-tested (timer registers)helm lint/ a live cluster — helm is not installed on this machine. The chart needs a kind/minikube pass before production use.Based on #4 (Layer 4) — merge that first.
🤖 Generated with Claude Code
@
Summary by CodeRabbit
MCPContractCRD, and a scheduled operator that writes drift status.