Repository navigation
Conversation
… demo The AIAC Controller start check #4 (D30) stops the Controller when the global combiner (the `default` AuthorizationPolicy in rossoctl-system) is missing. On a fresh cluster that CR only exists after `make opa` deploys the bundle service and applies the changed combiner (D20), so the documented order (`make enable`, then `make opa`) made `make enable` time out waiting for aiac-agent. Swap steps 1 and 2 in demo.md (Wire OPA is now step 1, Install AIAC step 2), renumber every cross-reference, tell the two start-check-#4 messages apart in "When something fails", and fix the broken #point-it-at-your-llm link. Renumber the Part 1/Part 2 pointers in the Makefile help, enable.sh and teardown.sh to match. Refs rossoctl/rossoctl#2534 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
…op phase WIRE The agent's authproxy-routes in configmaps.yaml targeted the production github-tool-mcp, which the onboarding demo does not deploy. driver.sh's phase WIRE patched it to the stand-in github-tool at run time, restarted the agent to load it, and granted the agent client the tool's audience scope; restore.sh undid both. Ship the real route in configmaps.yaml (host github-tool, the tool's SPIFFE ID as target_audience, scope agent-team1-github-tool-aud). deploy.sh applies it before the agent Deployment, so no restart is needed. The scope grant is also redundant: the operator makes agent-<ns>-github-tool-aud a realm default scope when it registers github-tool, and deploy.sh deploys the tool first, so github-agent's client is created with it. Remove phase WIRE and --only-wire-outbound from driver.sh, simplify `make enforce`, drop restore.sh's two WIRE-revert steps, and remove the system tests' ensure_github_tool_route. The tests keep grant_exchange_scope: the failure-rollback test deploys the agent before the tool. Update demo.md and the github-agent/github-tool specs to match. Refs rossoctl/rossoctl#2534 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
omerboehm
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
oblinder
left a comment
oblinder
left a comment
There was a problem hiding this comment.
Code review: 10 findings, numbered most severe first. Findings 1–5, 9 and 10 are inline comments. Findings 6–8 are on lines outside the diff, so they are here.
The most severe is finding 1: a race in the demo driver can make every tool call deny, and the fix that the PR documents (finding 2) does not remove the race.
6. Testing docs still describe the deleted ensure_github_tool_route step
docs/testing/policy-pipeline.md:126, and docs/testing/uc1-onboarding-pipeline.md:62 and :335.
Failure scenario: A reader or agent who follows the pipeline docs looks for ensure_github_tool_route or calls it, and gets an AttributeError. The docs now contradict the harness, which says that configmaps.yaml ships the route.
7. Text left over from WIRE says restore reverts the outbound wiring
demo/use-cases/onboarding/demo.md:784 says make restore "reverts step 5's outbound wiring". The teardown.sh header (line 19, "the outbound-leg wiring") and its comment at line 224 ("the outbound wiring") still list wiring that restore.sh no longer touches.
Failure scenario: An operator expects restore to reset authproxy-routes. It now leaves the static route from configmaps.yaml in place. The docs promise state changes that do not occur, which misleads people who debug leftover state.
8. Deployment manifest states an invariant this PR makes false
demo/assets/agents/github_agent/k8s/github-agent-deployment.yaml:37 still says MCP_URL host ("github-tool-mcp") == authproxy-routes host entry ("github-tool-mcp"). The route host is now github-tool.
Failure scenario: A maintainer trusts the manifest comment as the wiring contract and "fixes" the route back to github-tool-mcp. That silently breaks the onboarding demo's outbound leg again.
🤖 Generated with Claude Code
| pass "github-agent restarted and Ready" | ||
| } | ||
|
|
||
| # ── Phase ENFORCE ───────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
1. Race: nothing grants the tool audience scope after WIRE is removed
With phase WIRE removed, the demo depends only on deploy order to give github-agent's client the agent-team1-github-tool-aud scope. But deploy.sh does not wait for the tool's Keycloak registration before it applies the agent, and the operator's realm default-default PUT is best-effort (_ = a.putRealmDefaultDefaultClientScope(...)). Nothing grants the scope as a fallback any more.
Failure scenario: deploy.sh runs kubectl rollout status deployment/github-tool and then applies the agent at once. The operator reconciles github-agent before EnsureAudienceScope for github-tool completes, or the realm default PUT fails silently. github-agent's client is then created without agent-team1-github-tool-aud, the exchange the route requests fails or yields the wrong audience, and every tool call denies for every user. The removed WIRE step (PUT optional-client-scopes, 204/409) used to fix exactly this. The system test still calls grant_exchange_scope for the same reason, so the demo and the harness now behave differently.
There was a problem hiding this comment.
Fixed in de906e5. driver.sh has a new phase PREP that runs before every ENFORCE: it grants github-agent's client agent-<ns>-github-tool-aud as optional (204, or 409 when already linked), so the demo no longer depends on the operator's best-effort realm default. This matches the harness's grant_exchange_scope.
| | Symptom | Cause / fix | | ||
| |---|---| | ||
| | **Every tool call denies, for every user** | Usually the outbound route is missing — run `make enforce`, which includes WIRE, rather than the enforce half alone. Otherwise one of the two onboardings did not finish, leaving the per-tool maps empty: `kubectl logs deployment/aiac-agent -n aiac-system \| grep -iE 'label missing\|MCP tools/list'`. JetStream redelivers after `ACK_WAIT` (600 s) and usually succeeds, so waiting then re-running `make enforce` often cures it. | | ||
| | **Every tool call denies, for every user** | Usually `github-agent`'s client lacks the `agent-team1-github-tool-aud` scope: its client was created before `github-tool` registered (the operator makes that scope a realm default only then). `make restore` and `make e2e` again, which deploys the tool first. Otherwise one of the two onboardings did not finish, leaving the per-tool maps empty: `kubectl logs deployment/aiac-agent -n aiac-system \| grep -iE 'label missing\|MCP tools/list'`. JetStream redelivers after `ACK_WAIT` (600 s) and usually succeeds, so waiting then re-running `make enforce` often cures it. | |
There was a problem hiding this comment.
2. Troubleshooting fix repeats the racy order
The new fix for 'every tool call denies' (make restore and make e2e again, which deploys the tool first) repeats the same racy order as finding 1. It does not give the direct remedy (grant the scope to github-agent's client), which this PR removed.
Failure scenario: A user hits the registration race. They follow the row, restore and redeploy, and can hit the same race again. Each cycle costs minutes of LLM-bound onboarding, while a single PUT of optional-client-scopes would fix it.
There was a problem hiding this comment.
Fixed in de906e5. The row now says to check that PREP passed (it grants the scope, and make enforce re-runs it), then to check that authproxy-routes lists host: "github-tool". No restore/redeploy cycle.
| - host: "github-tool-mcp" | ||
| target_audience: "github-tool" | ||
| token_scopes: "openid github-tool-aud github-full-access" | ||
| - host: "github-tool" |
There was a problem hiding this comment.
3. Route for production github-tool-mcp host is replaced, not kept
The route for the production github-tool-mcp host is replaced instead of kept next to the new one. But the agent's own MCP_URL (http://github-tool-mcp:9090/mcp) and the github-agent.md prerequisite still target github-tool-mcp. The agent's real tool calls now pass through with no token exchange. routes.yaml is a list, so both routes can be kept.
Failure scenario: Deploy github-agent with the production github-tool-mcp as docs/specs/demo/github-agent.md describes. The agent's MCP calls to github-tool-mcp match no route, so token-exchange never fires and the call reaches the tool with the agent's inbound token. The tool's jwt-validation rejects it (401), or OPA sees no identity.
There was a problem hiding this comment.
Fixed in de906e5. routes.yaml now lists both routes: the original github-tool-mcp route (matches MCP_URL) and the stand-in github-tool route. The deployment manifest's invariant comment and github-agent.md say to keep both.
| target_audience: "github-tool" | ||
| token_scopes: "openid github-tool-aud github-full-access" | ||
| - host: "github-tool" | ||
| target_audience: "spiffe://localtest.me/ns/team1/sa/github-tool" |
There was a problem hiding this comment.
4. Static route hardcodes team1 and localtest.me
The route that is now static hardcodes team1 and localtest.me in target_audience and token_scopes. The removed code derived both from NS (driver.sh), NAMESPACE (AIAC_DEMO_NAMESPACE) and TRUST_DOMAIN (AIAC_TRUST_DOMAIN). The harness's _tool_audience() / _tool_aud_scope() still derive them, so the two now drift apart.
Failure scenario: Run the system lane with AIAC_TRUST_DOMAIN=example.org, or the driver with NS=team2. The sidecar exchanges for spiffe://localtest.me/ns/team1/sa/github-tool with scope agent-team1-github-tool-aud. These do not exist or are the wrong audience, so every outbound or tool-inbound decision denies. exchanged_subject also no longer matches what the route actually sends, although its docstring says it mirrors the route.
There was a problem hiding this comment.
Kept static on purpose. The manifest is already fixed to team1 (metadata.namespace), and the demo and system tests use localtest.me by default. In de906e5 the comment in configmaps.yaml and github-agent.md state this, and say to edit target_audience and token_scopes for another namespace or trust domain.
| @@ -961,7 +917,6 @@ | |||
| phase_verify_trigger | |||
There was a problem hiding this comment.
5. No agent restart after onboarding; ENFORCE can probe a stale bundle
Removing WIRE also removed the only agent restart after onboarding. The system harness keeps restart_agent because the OPA sidecar must re-fetch the recomposed bundle, but the demo now probes the agent's outbound leg against whatever bundle the sidecar loaded before the CR was rewritten.
Failure scenario: AIAC writes the final github-agent CR after the agent pod started. ENFORCE's decisive checks (dev-user → source-read allow, test-user → source-read deny) run against a stale or pass-through bundle until the poller catches up. In the worst case they hard-fail. Before this PR, the WIRE pod delete forced a fresh fetch.
There was a problem hiding this comment.
Fixed in de906e5. Phase PREP restarts github-agent (pod delete + wait Ready) after the scope grant and before ENFORCE, so the OPA sidecar loads the recomposed bundle. Same as the harness's restart_agent.
| # abort that skipped the broker AND the SPI, leaving Part 1 a third done. Waiting here, after | ||
| # abort that skipped the broker AND the SPI, leaving Part 2 a third done. Waiting here, after | ||
| # the broker, matches the real dependency order. | ||
| step_wait_agent() { |
There was a problem hiding this comment.
9. make opa before make enable is enforced only in the docs
The new rule that make opa runs before make enable exists only in the docs. enable.sh has no preflight for the default AuthorizationPolicy, so running the steps out of order shows up only as a timeout waiting for aiac-agent, followed by a manual rollout restart. A kubectl get authorizationpolicy default -n rossoctl-system check at the start, using the same check as opa-kind-enable.sh's check_combiner, would fail fast with the exact fix.
Failure scenario: On a fresh cluster, the user runs make enable first (the old order, or a script that was not updated). The script installs the stack and waits until it times out on aiac-agent, because start check #4 fails.
There was a problem hiding this comment.
Fixed in de906e5. step_stack now checks for the default AuthorizationPolicy first (COMBINER_NAMESPACE, default rossoctl-system) and exits with "run make opa first" if it is missing. It checks existence only; the full combiner check stays in opa-kind-enable.sh, and the troubleshooting row covers the allows case.
| @@ -524,19 +533,7 @@ make enforce | |||
|
|
|||
| ### What happens | |||
There was a problem hiding this comment.
10. Empty lines left in step 5; no troubleshooting row for a stale route
Step 5's ### What happens section now starts with several empty lines where the WIRE text was deleted. Also, the troubleshooting table no longer lists a stale or missing route as a cause, although clusters deployed before this PR carry the old github-tool-mcp route and make enforce no longer re-wires it.
Failure scenario: A user upgrades a running demo to this revision and runs make enforce. The agent still has the old route from before this PR (host github-tool-mcp), so every tool call denies. Neither the step text nor the troubleshooting row says to restore and redeploy so that the new configmaps.yaml takes effect.
There was a problem hiding this comment.
Fixed in de906e5. Step 5's empty lines are replaced by a description of PREP. The "every tool call denies" row now covers a cluster that still has the old route: re-apply configmaps.yaml, then make enforce (PREP restarts the agent, which loads it).
…e PREP Address the review on #235: - driver.sh: add phase PREP before every ENFORCE. It grants github-agent's client the tool audience scope (204/409) and restarts the agent, closing the race with the operator's best-effort realm default and loading the recomposed bundle. - configmaps.yaml: keep the production github-tool-mcp route next to the stand-in github-tool route; document that the latter is fixed to team1 / localtest.me. - enable.sh: fail fast when the `default` AuthorizationPolicy is missing (make opa did not run). - Makefile: list `make opa` before `make enable`; describe PREP. - demo.md, teardown.sh, specs, testing docs: describe PREP, drop text about restore reverting the outbound wiring and about ensure_github_tool_route, and cover a stale route in troubleshooting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
omerboehm
changed the title
|
Findings 6–8 are fixed in de906e5:
|
There was a problem hiding this comment.
Review of PR #235: 10 findings (5 high, 2 medium, 2 low, 1 nit). Two high findings share one comment on enable.sh:150, and one medium finding has comments on two lines of demo.md. There was no separate verify pass, so some findings can be false positives.
Not on a diff line:
- [Medium]
demo/use-cases/onboarding/Makefile:9. The file header still shows the old ordermake enable && make opa && make users && make e2e. This PR makesmake opaa hard prerequisite ofmake enable, so on a new cluster that order now stops at the new preflight ("AuthorizationPolicy 'default' is missing"). Change it tomake opa && make enable && make users && make e2e.
Fix the high findings before you merge: the preflight resource name and content check (enable.sh), the fixed trust domain (configmaps.yaml), the missing route check in PREP, and the delete/wait race (driver.sh).
🤖 Generated with Claude Code
| # as the `default` AuthorizationPolicy. Without it aiac-agent never becomes Available and | ||
| # step_wait_agent only times out, so fail here with the fix instead. | ||
| echo "==> [stack] Checking that 'make opa' ran (AuthorizationPolicy 'default' in ${COMBINER_NAMESPACE})" | ||
| if ! kubectl get authorizationpolicy default -n "$COMBINER_NAMESPACE" >/dev/null 2>&1; then |
There was a problem hiding this comment.
[High] The preflight uses the short name authorizationpolicy, and it checks only that the CR exists.
- Istio's
security.istio.ioAuthorizationPolicyuses the same short name. On a Kagenti/Istio cluster, kubectl can match the Istio kind, find nothing inrossoctl-system, and stop with "Run 'make opa' first", even aftermake opahas run. The rest of the demo uses the full name for this reason (lib/_lib.py:38-40,POLICY_CRindriver.shandrestore.sh). Useauthorizationpolicies.agent.rossoctl.dev. - The check passes for the stock
defaultCR too (for example, aftermake opa-restore). Start check Chore(deps): Bump python from 3.12-slim to 3.14-slim in /src/aiac/idp/service/configuration/keycloak #4 also rejects the stock combiner, sostep_wait_agentthen times out: the failure this preflight is there to prevent. Reuse theREQ_FALLBACKcontent check fromdriver.sh(about lines 370-373).
There was a problem hiding this comment.
Fixed in ad7a196. The preflight now uses authorizationpolicies.agent.rossoctl.dev, and after the existence check it counts the request fallback rules with the same grep as driver.sh. A stock combiner (non-zero count) stops make enable with "run make opa first", before anything is installed.
| target_audience: "github-tool" | ||
| token_scopes: "openid github-tool-aud github-full-access" | ||
| - host: "github-tool" | ||
| target_audience: "spiffe://localtest.me/ns/team1/sa/github-tool" |
There was a problem hiding this comment.
[High] The trust domain localtest.me is fixed in the route.
The removed ensure_github_tool_route built this audience from _tool_audience() (AIAC_TRUST_DOMAIN), and exchanged_subject still uses _tool_audience(). If you run the system lane with a different AIAC_TRUST_DOMAIN, the sidecar exchanges for spiffe://localtest.me/..., but the github-tool inbound validates spiffe://<domain>/.... Then the outbound probes in rungs 2/3 and the failure-rollback test deny, but the direct exchange in exchanged_subject passes. The results disagree and are hard to diagnose.
There was a problem hiding this comment.
Kept static, as in the reply to finding 4. The route is fixed to team1 / localtest.me, the defaults of the demo and the system lane, and configmaps.yaml and github-agent.md say to edit target_audience and token_scopes for another trust domain. A run with a non-default AIAC_TRUST_DOMAIN must edit the route to match.
| - host: "github-tool-mcp" | ||
| target_audience: "github-tool" | ||
| token_scopes: "openid github-tool-aud github-full-access" | ||
| - host: "github-tool" |
There was a problem hiding this comment.
[Low] A route that only the demo uses is now in a shared manifest, with fixed values.
Every consumer of configmaps.yaml now gets a route for a stand-in tool that it possibly does not deploy, and a comment ("edit to match") is the only guard on the namespace and trust domain. Think about rendering the route at deploy time (for example, envsubst with NAMESPACE / TRUST_DOMAIN in deploy.sh / deploy_workload), or keep one helper with parameters. Then the demo, the system lane and non-default clusters cannot drift apart.
There was a problem hiding this comment.
Not changed in this PR. Rendering at deploy time would make a plain kubectl apply -f configmaps.yaml apply placeholders, and the docs and the troubleshooting fix use that command. The manifest is already fixed to team1 by metadata.namespace, so the route follows the same rule. Templating the namespace and trust domain for all of the demo manifests can be a follow-up.
| # 2. The agent's OPA sidecar must load the bundle AIAC recomposed after the pod started, and the | ||
| # sidecar re-reads routes.yaml, so the restart also picks up a re-applied configmaps.yaml. | ||
| # Idempotent, so it runs before every ENFORCE, --only-enforce included. | ||
| phase_prep() { |
There was a problem hiding this comment.
[High] PREP does not check that authproxy-routes has the github-tool route.
WIRE made sure of the route. Now ENFORCE assumes that the ConfigMap on the cluster matches the new configmaps.yaml. On a cluster from before this PR, a cluster that the platform installer re-seeded (the teardown caveat), or a cluster that has the file from an older checkout, only github-tool-mcp has a route. The call is not exchanged, and every tool call denies for every user, with no hint from the driver. A one-line check in PREP gives back the guarantee:
kubectl get cm authproxy-routes -n "$NS" -o yaml | grep -q 'host: "github-tool"' \
|| die "authproxy-routes has no github-tool route; re-apply configmaps.yaml"There was a problem hiding this comment.
Fixed in ad7a196. PREP now first checks that authproxy-routes in $NS lists host: "github-tool", and stops with the kubectl apply ... configmaps.yaml fix if not.
|
|
||
| step "Restarting github-agent to load the route" | ||
| step "Restarting github-agent so its OPA sidecar loads the recomposed bundle" | ||
| kubectl delete pod -n "$NS" -l "$AGENT_LABEL" || die "failed to delete github-agent pod" |
There was a problem hiding this comment.
[High] Race between kubectl delete pod -l and kubectl wait -l.
kubectl delete returns when the old pod is gone. If the ReplicaSet has not yet made the new pod, kubectl wait ... -l finds no pods, stops with "no matching resources found", and the driver dies with "github-agent did not become Ready". PREP now runs before every ENFORCE, so this race can occur on every make enforce. Use kubectl rollout restart deployment/github-agent and kubectl rollout status, as uc1_onboard.restart_agent does.
There was a problem hiding this comment.
Fixed in ad7a196. PREP now runs kubectl rollout restart deployment/github-agent and then kubectl rollout status --timeout=180s, the same as uc1_onboard.restart_agent.
| [ "$DO_ENFORCE" -eq 1 ] && phase_enforce | ||
| if [ "$DO_ENFORCE" -eq 1 ]; then | ||
| phase_prep | ||
| phase_enforce |
There was a problem hiding this comment.
[Low] PREP always runs before ENFORCE, also when nothing changed.
Each repeated make enforce costs a full agent restart (up to 120 s), more Keycloak admin calls, and the OPA sidecar's bundle-load window (about 120 s of stale verdicts or 503s that ENFORCE must retry through). Think about skipping the restart when the scope PUT returns 409 and the agent pod started after the last update of the CR.
There was a problem hiding this comment.
Done in ad7a196. PREP skips the restart when the scope PUT returns 409 and the oldest agent pod started after the newest managedFields write to the two AuthorizationPolicy CRs and authproxy-routes. Otherwise, or when a timestamp is missing, it restarts as before.
| # Phase PREP grant github-agent's client the tool's audience scope (closes the race | ||
| # with the operator's realm default) and restart the agent so its OPA sidecar | ||
| # loads the recomposed bundle. Runs before every ENFORCE; idempotent. | ||
| # Phase ENFORCE real HTTP probes through the live AuthBridge OPA plugin, reproducing |
There was a problem hiding this comment.
[Nit] After the rename, Phase ENFORCE has one space less than the other phases, so its description is out of line in the usage header.
| > **[Step 1](#step-1--wire-opa-into-both-authbridge-legs) must have run.** The AIAC Controller does | ||
| > not start without the changed combiner that step 1 applies (start check #4), so `make enable` | ||
| > times out waiting for `aiac-agent`, and its log ends with `start check #4 failed: the | ||
| > AuthorizationPolicy 'default' in namespace 'rossoctl-system' is missing`. If you hit this, run |
There was a problem hiding this comment.
[Medium] This warning is stale after the new enable.sh preflight.
If the combiner is missing, enable.sh now stops with exit 1 before it applies a stack manifest, so there is no timeout. Also, kubectl rollout restart deployment/aiac-agent -n aiac-system then fails with "deployments.apps aiac-agent not found", because the script did not install aiac-agent, the broker or the SPI. The correct fix is: run make opa, then run make enable again.
There was a problem hiding this comment.
Fixed in ad7a196. The step 2 note now says that make enable checks first and stops before it installs anything, and that the fix is to run make opa and then make enable again.
| | **ENFORCE stops: a client does not link `aiac-username-sub` as a default scope** | The token keeps `sub` = the Keycloak user ID, so the rego cannot find the user (D31). For `rossoctl`, restart the IdP Configuration Service once Keycloak is ready: `kubectl rollout restart deployment/aiac-interface -n aiac-system`. For `github-agent`, let AIAC onboard it again. Do not add a `sub` mapper by hand. | | ||
| | **The AIAC Controller does not start (start check #4)** | The global combiner is the stock one, which allows a pod that has no client CR. A `make opa-restore` or another bundle-service install brought it back. Run `make opa` again. | | ||
| | **Onboarding fails with a `401` from the LLM** | `enable.sh` defaulted to OpenAI. Patch `aiac-agent-config` and `rollout restart` — see [step 1](#point-it-at-your-llm). The values come in via `envFrom`, so a patch without a restart does nothing. | | ||
| | **The AIAC Controller does not start (start check #4)** | The log says either that the `default` AuthorizationPolicy is *missing*, or that the global combiner *allows a pod that has no client CR*. Missing: `make opa` never ran on this cluster — it must run before `make enable` (see [step 2](#step-2--install-aiac)). Allows: the combiner is the stock one, brought back by a `make opa-restore` or another bundle-service install. Either way, run `make opa`, then `kubectl rollout restart deployment/aiac-agent -n aiac-system`. | |
There was a problem hiding this comment.
[Medium] Same problem as the step 2 warning.
For the missing case, the preflight now stops make enable before it installs aiac-agent, so rollout restart deployment/aiac-agent fails with "not found". Tell the user to run make opa and then make enable again. The rollout restart fix is correct only for the allows case (stock combiner on a running install).
There was a problem hiding this comment.
Fixed in ad7a196. The row now gives one fix per case. Missing: run make opa, then make enable again. Allows (stock combiner on a running install): run make opa, then rollout restart deployment/aiac-agent.
- enable.sh: check the combiner by its full resource name (Istio shares the short name) and reject the stock combiner, as driver.sh does. - driver.sh PREP: fail fast when authproxy-routes has no github-tool route; restart with rollout restart/status instead of pod delete + wait -l (no-pods race); skip the restart when the scope was already linked and the agent started after the last CR/route write. - demo.md, Makefile: the combiner-missing remedy is now `make opa` then `make enable`; the header shows `make opa` first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
|
The finding outside the diff is fixed in ad7a196: the |
oblinder
left a comment
oblinder
left a comment
There was a problem hiding this comment.
Code review of this PR. I found 8 issues. None of them breaks the default path: namespace team1, trust domain localtest.me, a new Kind cluster. I did not run these checks on a cluster.
Root cause of findings 1 and 2: the shared demo/assets/agents/github_agent/k8s/configmaps.yaml sets the route to team1 / localtest.me. If the deploy step makes the route from the namespace and the trust domain, both issues go away.
The findings below are on lines that this PR did not change, so I cannot put them inline.
1. test/system/uc1_onboard.py:219 — AIAC_TRUST_DOMAIN no longer reaches the live outbound route
The PR removes the dynamic route helper. The route in configmaps.yaml now always uses spiffe://localtest.me/..., but _tool_audience() and exchanged_subject() still build the audience from TRUST_DOMAIN.
Failure: run the system lane with AIAC_TRUST_DOMAIN=example.org. Before this PR, ensure_github_tool_route wrote target_audience: spiffe://example.org/ns/team1/sa/github-tool. Now the sidecar exchanges to spiffe://localtest.me/..., and the inbound jwt-validation of github-tool rejects that token. Every outbound probe gives the wrong verdict, and the bundle-convergence gate times out. The rung-2 sub probe also exchanges to a different audience than the live route, so it no longer tests what AuthBridge does.
5. demo/use-cases/onboarding/driver.sh:370 — the preflight passes when the default AuthorizationPolicy is missing
2>/dev/null and || true hide the error, so a missing CR gives "0 request fallback rules" and the check passes. The PR adds a missing-CR check only to enable.sh, but demo.md still says the preflight of driver.sh "refuses to run without them".
Failure: run make opa-restore, then delete the default CR. The aiac-agent Deployment still exists, so get deployment passes. The combiner grep returns 0, so the preflight passes. DEPLOY then starts on a Controller that cannot pass start check #4, and the run fails later at VERIFY-TRIGGER with a timeout that does not show the cause.
8. test/system/uc1_onboard.py:213 — the comment is out of date
The comment says AGENT_DEPLOYMENT is restarted "so it reloads the new outbound route". This PR removes that reason: the route is now in configmaps.yaml, and the docstring of restart_agent now names only the bundle re-fetch. A maintainer can think that Part B still loads a route, and add a route patch again.
| # configmaps.yaml, which a cluster from before the route moved there (or a re-seeded one) does not. | ||
| step "Checking that authproxy-routes has the github-tool route" | ||
| kubectl get configmap authproxy-routes -n "$NS" -o jsonpath='{.data.routes\.yaml}' 2>/dev/null \ | ||
| | grep -q 'host: "github-tool"' \ |
There was a problem hiding this comment.
2. The PREP route check is too weak.
The check only looks for host: "github-tool". It does not check target_audience or token_scopes against ${NS} and the trust domain. Before this PR, WIRE built both values from ${NS}. Now they are always team1 / localtest.me.
Failure: a user edits configmaps.yaml for a different trust domain and makes a typo, or a cluster keeps a route from a different domain. PREP prints authproxy-routes lists host "github-tool" and passes. ENFORCE then denies every tool call for every user, and the cause is not clear.
Fix: also check that target_audience is spiffe://<td>/ns/${NS}/sa/github-tool and that token_scopes contains agent-${NS}-github-tool-aud.
There was a problem hiding this comment.
Fixed in 0dc171c. PREP now reads the github-tool route and checks that target_audience is spiffe://${TRUST_DOMAIN}/ns/${NS}/sa/github-tool and that token_scopes contains agent-${NS}-github-tool-aud. driver.sh has a new TRUST_DOMAIN (default localtest.me, the same name as in opa-kind-verify.sh). Each mismatch stops PREP with the value it found, the value it expected, and the kubectl apply fix.
| 2>/dev/null | sort | head -1)" | ||
| changed="$( { last_write "$POLICY_CR" github-agent; last_write "$POLICY_CR" github-tool | ||
| last_write configmap authproxy-routes; } | sort | tail -1)" | ||
| if [ "$scope_http" = "409" ] && [ -n "$started" ] && [ -n "$changed" ] && [[ "$started" > "$changed" ]]; then |
There was a problem hiding this comment.
3. When PREP skips the restart, it does not check that the agent is Ready.
The skip test uses the pod .status.startTime. That value does not change when a container in the pod restarts or crash-loops.
Failure: the scope is already linked (HTTP 409). The github-agent pod is newer than the last CR/route write, but its sidecar is in CrashLoopBackOff or NotReady. PREP reports "no restart needed", and ENFORCE probes a pod that is not ready, so it gives ERROR or 503 verdicts. The removed code always ran kubectl wait / rollout status, so the agent was always Ready before ENFORCE.
Fix: run kubectl rollout status deployment/github-agent before the early return.
There was a problem hiding this comment.
Fixed in 0dc171c by removing the skip (finding 7). PREP always runs rollout restart and then rollout status, so the agent is Ready before ENFORCE.
| echo " Run 'make opa' (demo.md step 1) first, then re-run this script." >&2 | ||
| exit 1 | ||
| fi | ||
| req_fallback=$(kubectl get "$policy_cr" default -n "$COMBINER_NAMESPACE" -o jsonpath='{.spec.policies[*].content}' \ |
There was a problem hiding this comment.
4. The fail-fast combiner check is incomplete.
The check only finds the stock fallback line. Start check #4 is an allowlist: it needs both request.rego entries, and every client_ok rule must be a safe one. Other bad combiners pass this check, and enable.sh times out on aiac-agent again. That is the failure this PR wants to remove.
Failure: a default CR with no inbound/request.rego or outbound/request.rego entry, or a fail-open rule in a different form, for example client_ok if not data.authbridge.client.inbound.request.allow. Both give grep -c 0, so req_fallback=0 and the check passes. aiac-agent then fails start check #4, and step_wait_agent times out. Also, if the jsonpath kubectl get fails for a short time, the empty output gives "0" and the check passes. demo.md says that make enable stops when the CR "is the stock combiner", but it does not stop for the other combiners that the Controller rejects.
There was a problem hiding this comment.
Fixed in 0dc171c. The new k8s/lib/combiner-check.sh runs the same allowlist as start check #4. It checks that both request.rego entries are present, that every client_ok rule is safe, and every entry of a repeated path. A CR that is missing or that cannot be read fails it. enable.sh prints each problem it finds. test/unit/k8s/test_combiner_check_sh.py runs the shell check against a fake kubectl and checks that it agrees with start.combiner_problems on the cases from test_start.py, which include your two examples. The step 2 note in demo.md now says that make enable uses the rules of start check #4.
| # this demo, which may hold a stale/incomplete policy. | ||
| POLICY_CONFIGMAP="${POLICY_CONFIGMAP:-aiac-policy}" | ||
| # Where `make opa` (k8s/opa-kind-enable.sh, RELEASE_NAMESPACE) puts the combiner's `default` CR. | ||
| COMBINER_NAMESPACE="${COMBINER_NAMESPACE:-rossoctl-system}" |
There was a problem hiding this comment.
6. The combiner check is now in three files, with three namespace variables.
This new check copies the regex from the driver.sh preflight (line 371) and from k8s/opa-kind-verify.sh. It also adds a third variable, COMBINER_NAMESPACE, for the value that driver.sh calls SYS_NS and the Controller calls AIAC_BUNDLE_SERVICE_NAMESPACE.
None of the three copies matches the Controller allowlist. The next change to start check #4 must update all three, and they can become different. A user who sets a different namespace must also set three variables to the same value.
Fix: use one shared helper (or call the combiner check in k8s/opa-kind-verify.sh) and one namespace variable.
There was a problem hiding this comment.
Fixed in 0dc171c. opa-kind-enable.sh, opa-kind-verify.sh, enable.sh and driver.sh now source the one helper in k8s/lib/combiner-check.sh. The awk from opa-kind-enable.sh moved there, and the regex copies are gone. enable.sh uses SYS_NS in place of COMBINER_NAMESPACE. opa-kind-verify.sh still counts the 2 response fallback rules, because start check #4 does not check those.
| # the scope was already linked and nothing it would reload changed after the agent pods started. | ||
| # | ||
| # last_write <resource> <name> — the newest managedFields time of the object (RFC 3339, UTC), or "". | ||
| last_write() { |
There was a problem hiding this comment.
7. The restart-skip logic is complex for a small gain.
last_write, the managedFields/startTime string comparison, and the 409 gate only save one rollout restart in a demo. The logic also assumes that Keycloak returns 409 for a scope that is already linked as optional. Nobody has checked that, and the test-plan item "PREP passes with 409" is not checked.
If Keycloak PUT optional-client-scopes returns 204 again for a scope that is already optional (and 409 only for a scope linked as default), a repeat make enforce restarts every time and the skip almost never runs. This extra code is also the cause of the readiness gap in finding 3. The system tests always restart.
Simpler: always run rollout restart and rollout status. That is idempotent and does the same thing as the test harness.
There was a problem hiding this comment.
Done in 0dc171c. last_write, the startTime comparison and the 409 gate are removed. PREP always runs rollout restart and rollout status, the same as restart_agent in the harness.
…ys-restart PREP - k8s/lib/combiner-check.sh: the one shell copy of start check #4 (the start.py allowlist: both request packages present, every client_ok rule safe, repeated paths checked, a missing or unreadable CR fails closed). opa-kind-enable.sh, opa-kind-verify.sh, enable.sh and driver.sh source it; test/unit/k8s/test_combiner_check_sh.py checks it against start.combiner_problems. - enable.sh: COMBINER_NAMESPACE is now SYS_NS, as in driver.sh and verify.sh. - driver.sh PREP: checks the github-tool route's target_audience and token_scopes against NS and the new TRUST_DOMAIN, not just the host, and always runs rollout restart + status (the restart-skip logic is removed). - uc1_onboard: require_tool_route fails Part B when the live route does not match AIAC_TRUST_DOMAIN / the namespace; the AGENT_DEPLOYMENT comment no longer says the restart loads a route. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
|
The three findings that are not on a diff line are fixed in 0dc171c:
Not tested on a cluster. The unit lane passes, except two |
…SC2148) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
Make the README true on main, not only once #235 merges: - Deploying: point at the deployment guide's "Platform preconditions" for the OPA and SPI checks; on main docs/agents/deploy.md holds only the deploy parameters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Omer Boehm <omerboehm@gmail.com>
Summary
Three fixes to the onboarding demo (
demo/use-cases/onboarding/). The first two are their own commits; the third addresses the review.1. Run the OPA step before installing AIAC
On a fresh Kind cluster, following the demo in order fails at step 1.
make enabletimes out waiting foraiac-agent, and the Controller log says:Start check #4 (D30) needs the changed combiner (D20). That CR only exists after
make opahas deployed the bundle service, andmake opadoes not need AIAC installed.demo.md: swaps steps 1 and 2. Step 1 is now Wire OPA (make opa) and Step 2 is Install AIAC (make enable). Every cross-reference is renumbered.enable.shnow fails fast with "runmake opafirst" when thedefaultAuthorizationPolicy is missing, instead of timing out onaiac-agent.#point-it-at-your-llmlink.Makefile(help order and Part pointers),enable.sh,teardown.sh: renumbered to match.2. Ship the outbound route in
configmaps.yamlauthproxy-routesindemo/assets/agents/github_agent/k8s/configmaps.yamlonly had a route for the productiongithub-tool-mcp, which this demo never deploys. At run time,driver.sh's phase WIRE patched the route to the stand-ingithub-tool, andrestore.shreverted it.configmaps.yamlnow ships both routes: the existinggithub-tool-mcproute (matches the agent'sMCP_URL) and the stand-ingithub-toolroute (target_audience: spiffe://localtest.me/ns/team1/sa/github-tool,token_scopes: openid agent-team1-github-tool-aud). The stand-in route is static, so it is fixed toteam1/localtest.me; the comment says so.deploy.shapplies it before the agent Deployment.driver.sh,restore.sh's route revert, and the system tests'ensure_github_tool_route.3. Phase PREP: the scope grant and agent restart stay
The operator's
EnsureAudienceScopemakesagent-<ns>-github-tool-auda realm default, but that PUT is best-effort anddeploy.shdoes not wait for the tool's Keycloak registration before applying the agent. Sogithub-agent's client can predate the scope. The agent's OPA sidecar also needs to load the bundle AIAC recomposed after the pod started.driver.shphase PREP runs before every ENFORCE (--only-enforceincluded). It grants the scope as optional (204, or 409 when already linked) and restarts the agent. It is idempotent.grant_exchange_scope/restart_agentfor the same reasons, so the demo and the harness match.restore.shno longer removes the optional scope separately; it deletes the whole client.demo.mddescribes PREP in step 5. The "every tool call denies" row now points at PREP and at a stale route (clusters deployed before this PR: re-applyconfigmaps.yaml, thenmake enforce).github-agent/github-toolspecs anddocs/testing/pipeline docs are updated to match.Test plan
bash -npasses ondriver.sh,restore.sh,enable.sh,teardown.sh.configmaps.yamlparses;routes.yamllists both routes with the values above.make enablebeforemake opafails fast with themake opahint.make opa,make enable,make users,make e2e.aiac-agentshould become Available, PREP should pass (204), and ENFORCE should pass.make enforceagain: PREP passes with 409.Refs rossoctl/rossoctl#2534
🤖 Generated with Claude Code