diff --git a/.ai/spec/how/reconciler.md b/.ai/spec/how/reconciler.md index 42e716ed..7d6cd0ac 100644 --- a/.ai/spec/how/reconciler.md +++ b/.ai/spec/how/reconciler.md @@ -26,7 +26,7 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec | File | Types / primary responsibilities | Key functions / methods | |------|----------------------------------|-------------------------| | `reconciler.go` | `AgenticRunReconciler` (embeds `client.Client`, `Agent AgentCaller`, `Log`) | `Reconcile`, `SetupWithManager` | -| `handlers.go` | (methods on `AgenticRunReconciler`) | `handleAnalysis`, `handleRevision`, `handleExecution`, `handleVerification`, `handleEscalation`, `handleFailed`, `denyAgenticRun`, `conditionTime` | +| `handlers.go` | (methods on `AgenticRunReconciler`) | `handleAnalysis`, `handleRevision`, `handleExecution`, `handleVerification`, `handleEscalation`, `handleFailed`, `denyAgenticRun`, `conditionTime`, `hasMutationSuccess`, `isObservationAction` | | `helpers.go` | `revisionData`, `analysisQuery`, `executionQuery`, `verificationQuery`, `escalationData`; embedded templates via `//go:embed templates/*.tmpl` | `renderTemplate`, `failStep`, `statusPatch`, `hasSandboxClaims`, `isTerminal`, `setVerificationSkipped`, `getLatestAnalysisResult`, `selectedOption`, `trimNonSelectedOptions`, `resetExecutionAndVerification`, `maxAttempts`, `buildEscalationRequest`, `needsRevision`, `buildRevisionContext`, `buildAnalysisQuery`, `buildExecutionQuery`, `buildVerificationQuery`, `prettyJSON` | | `approval.go` | — | `getApprovalPolicy`, `getAgenticRunApproval`, `ensureAgenticRunApproval`, `isStageApproved`, `isStageDenied`, `getStageOverrideAgent`, `getStageOption` | | `resolve.go` | `resolvedStep`, `resolvedWorkflow` | `resolveAgenticRun`, `stepAgentName` | @@ -38,7 +38,7 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec | `sandbox_templates.go` | `templateHashInput`; label constants (`LabelManaged`, `LabelRun`, etc.); MCP env DTOs | `EnsureAgentTemplate`, `SandboxTemplateServiceAccount`, `computeTemplateHash`, `agentTemplateName`, `gcOldTemplates`, `patchLLMCredentials`, `credentialsSecretName`, `providerURL`, `patchRequiredSecrets`, `patchMCPServers`, `patchSkillsImage`, `patchSkillsPaths`, `patchProbes`, unstructured helpers (`firstContainer`, `setEnvVar`, `addEnvFromSecret`, …) | | `client.go` | `AgentHTTPClientInterface`, `AgentHTTPClient`; `agentRunRequest`, `agentContext`, `agentExecutionResult`, `agentPreviousAttempt`, `agentRunResponse` | `NewAgentHTTPClient`, `(*AgentHTTPClient).Run`, `executionOutputToAgentResult` | | `schemas.go` | Package vars: default/minimal analysis schemas, execution/verification/escalation schemas; `defaultOutputSchemas`, `builtInPropertyJSON` | `init` (precompute property JSON), `injectBuiltInProperty`, `outputSchemaForStep` | -| `rbac.go` | — | `ensureExecutionRBAC`, `cleanupExecutionRBAC`, `annotatedRBACNamespaces`, `deleteIfExists`, `rbacTargetNamespaces`, `truncateK8sName`, `executionRoleName`, `clusterRoleName`, `rbacLabels`, `rbacRulesToPolicyRules`, `normalizeCoreAPIGroup` | +| `rbac.go` | `readerBindings atomic.Value` (cached CRB names) | `ensureExecutionRBAC`, `cleanupExecutionRBAC`, `resolveReaderBindings`, `addReaderSubject`, `removeReaderSubject`, `addSubjectToBinding`, `removeSubjectFromBinding`, `annotatedRBACNamespaces`, `deleteIfExists`, `rbacTargetNamespaces`, `truncateK8sName`, `executionRoleName`, `clusterRoleName`, `rbacLabels`, `rbacRulesToPolicyRules`, `normalizeCoreAPIGroup` | | `results.go` | `statusHolder` interface (defined; no references elsewhere in this package) | `resultCRName`, `agenticRunOwnerRef`, `resultLabels`, `executionRetryIndex`, `resultConditions`, `createAnalysisResult`, `createExecutionResult`, `createVerificationResult`, `createEscalationResult`, `createIdempotent` | | `templates/*.tmpl` | Text templates | Names: `analysis_query.tmpl`, `execution_query.tmpl`, `verification_query.tmpl`, `revision_context.tmpl`, `escalation_request.tmpl` | | `reconciler_test.go` | `testAgentCaller`, fixtures | `testScheme`, `testDefaultAgent`, `testAgenticRun`, `reconcileOnce`, `getAgenticRun`, … | @@ -175,6 +175,8 @@ Audience: AI agents. Behavioral rules and phase semantics live in **what/** spec - **Creation:** `handleExecution`, when selected option has non-empty `RBAC` rules, calls `ensureExecutionRBAC(ctx, Client, proposal, &selectedOption.RBAC, defaultSandboxSA, proposal.Namespace)`. Creates namespaced `Role`/`RoleBinding` per target namespace (from `Spec.TargetNamespaces` or rule namespace fields), persists comma-joined namespaces in annotation `agentic.openshift.io/rbac-namespaces`, and cluster `ClusterRole`/`ClusterRoleBinding` when cluster rules present. Sandbox SA name constant `defaultSandboxSA` (`lightspeed-agent` in `helpers.go`). - **Cleanup:** `cleanupExecutionRBAC` reads annotation to delete bindings/roles; deletes cluster RBAC by derived name. Invoked on: run deletion (finalizer), `handleFailed` if annotation set, after successful escalation completion, and terminal phases via sandbox release path is separate. - **`normalizeCoreAPIGroup`:** Maps LLM-facing `"core"` to `""` in K8s `PolicyRule.APIGroups`. +- **Read RBAC (multi-binding):** `resolveReaderBindings` discovers **all** `ClusterRoleBinding`s where `lightspeed-agent` is a subject (e.g. `cluster-reader`, `cluster-monitoring-view`), caches the list in `readerBindings` (`atomic.Value` storing `[]string`), and returns it. `addReaderSubject`/`removeReaderSubject` iterate all discovered bindings. Individual binding updates are in `addSubjectToBinding`/`removeSubjectFromBinding` with conflict retry loops. [OLS-3712] +- **Execution outcome override:** In `handleExecution`, when the agent reports `success=false`, the controller calls `hasMutationSuccess(actions)` to check whether all mutating actions actually succeeded. If yes, it overrides `execResult.Success = true` and proceeds to verification (or trust-mode completion). `isObservationAction(type)` classifies non-mutating action types (`pre-check`, `post-check`, `verification`, `check`, `wait`); everything else is treated as a mutation. [OLS-3558] --- diff --git a/.ai/spec/what/run-lifecycle.md b/.ai/spec/what/run-lifecycle.md index 20293240..27696acb 100644 --- a/.ai/spec/what/run-lifecycle.md +++ b/.ai/spec/what/run-lifecycle.md @@ -40,7 +40,7 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat 15. **Success**: `Verified=True` MUST yield `Completed` once rule 9 reaches the `Verified` branch, unless an earlier branch already returned `Escalated` or `Denied` per rules 9–10. 16. **Step failure**: Any of `Analyzed`, `Executed`, or `Verified` with status `False` and reasons that are not the dedicated retrying-execution reason MUST yield `Failed` when reached by the derivation order in rule 9 (unless superseded by `Escalated` / `Denied` per rules 9–10). 17. **Escalation failure**: `Escalated` with status `False` MUST yield `Failed` once rule 9 evaluates the `Escalated` presence branch (non-`True`, non-`Unknown`). -18. **Result CR linkage**: Each analysis/execution/verification/escalation attempt SHOULD append a `status.steps.*.results[]` entry naming the corresponding result resource with an outcome matching agent success/failure for that attempt. +18. **Result CR linkage**: Each analysis/execution/verification/escalation attempt SHOULD append a `status.steps.*.results[]` entry naming the corresponding result resource with an outcome matching agent success/failure for that attempt. **Exception:** when the execution agent reports `success=false` but all mutating actions succeeded (only inline verification checks failed), the controller MUST override the outcome to `Succeeded` and proceed to the verification step. Observation action types (`pre-check`, `post-check`, `verification`, `check`, `wait`) are not considered when determining mutation success. 19. **Observed generation**: Conditions SHOULD carry `observedGeneration` aligned with `metadata.generation` when the controller updates them for the current spec generation, except revision completion MAY pin the analyzed condition to the generation that triggered the revision, per existing reconciliation behavior. 20. **Immutable spec (excluding revision)**: Once set, `spec.request`, `spec.targetNamespaces`, `spec.analysisOutput`, `spec.tools`, `spec.analysis`, `spec.execution`, and `spec.verification` MUST NOT change; CEL on the CRD enforces this. Only `spec.revisionFeedback` is mutable for iterative feedback. 21. **Option trim after analysis**: When multiple remediation options exist, execution MUST use the option selected through the approval resource; non-selected options MAY be removed from the stored analysis result before execution (see `approval.md`). @@ -72,3 +72,4 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat - [DONE: OLS-3018] `EmergencyStopped` phase and condition type added to run lifecycle. See `system-config.md` for full kill switch specification. - [PLANNED: OLS-3268] `NoActionRequired` terminal phase: when analysis returns `actionRequired=false`, the operator sets `Analyzed=True` with reason `NoActionRequired` and the run auto-completes, bypassing approval/execution/verification. - [DONE: OLS-3295] Renamed `Proposal` CRD kind to `AgenticRun`, `ProposalApproval` to `AgenticRunApproval`, and updated all associated API surface (labels, RBAC resources, CLI commands, audit events, OTEL spans). +- [DONE: OLS-3558] Execution outcome override — controller no longer hard-fails when `success=false` but all mutating actions succeeded; defers outcome to the verification step. See `sandbox-execution.md` rule 21b. diff --git a/.ai/spec/what/sandbox-execution.md b/.ai/spec/what/sandbox-execution.md index fbfebe57..c93915a8 100644 --- a/.ai/spec/what/sandbox-execution.md +++ b/.ai/spec/what/sandbox-execution.md @@ -14,8 +14,8 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 8. **HTTP contract**: Each step MUST call the agent **`POST /v1/agent/run`** with JSON body carrying at least `query`, `outputSchema`, and `context`; optional `systemPrompt` and `timeout_ms` exist in the wire shape but **system prompt MUST be sent empty** in the current implementation (prompt material lives in `query` and templates). 9. **Response handling**: HTTP success responses MUST be parsed as JSON matching the per-step schema (analysis/execution/verification/escalation). Non-success HTTP MUST fail the step with an error surfaced to run conditions. 10. **Output schema selection**: `outputSchema` MUST be the step-specific JSON schema: analysis schema depends on `spec.analysisOutput.mode`, whether execution/verification steps exist in the run, and optional injected `components` sub-schema from `spec.analysisOutput.schema`; other steps use fixed schemas for their response shapes. -11. **Analysis query payload**: The `query` string MUST encode the user request or revision-augmented request and encode workflow flags indicating whether execution/verification steps exist (template-rendered). The analysis prompt MUST instruct the agent to use kubectl/oc to inspect cluster state before diagnosing, produce a concrete remediation script of executable bash commands (including pre-checks, mutations, waits, and post-checks), and derive RBAC by tracing every command in the script — requiring `get`/`list`/`watch` as minimum read verbs for every resource. -12. **Execution query payload**: The `query` MUST include JSON describing the approved remediation option, which contains a concrete remediation script (ordered bash commands). The execution prompt MUST instruct the agent to follow the script exactly, execute commands in order without substitution, and dry-run every mutation command with `--dry-run=server` before applying. +11. **Analysis query payload**: The `query` string MUST encode the user request or revision-augmented request and encode workflow flags indicating whether execution/verification steps exist (template-rendered). The analysis prompt MUST instruct the agent to use kubectl/oc to inspect cluster state before diagnosing, produce a concrete remediation script of executable bash commands (mutations and waits only — pre-checks and post-checks are excluded because analysis already inspected the cluster and verification is a separate step), and derive RBAC for mutations and subresource access only (the execution environment already has cluster-wide read access to standard resources). +12. **Execution query payload**: The `query` MUST include JSON describing the approved remediation option, which contains a concrete remediation script (ordered bash commands). The execution prompt MUST instruct the agent to follow the script exactly, execute commands in order without substitution, dry-run every mutation command with `--dry-run=server` before applying, and fix syntax errors only (no semantic changes). The execution prompt MUST NOT instruct the agent to perform inline verification — that is the verification step's responsibility. 13. **Verification query payload**: The `query` MUST include the approved option JSON and a JSON description of the latest execution output (actions and inline verification) when available. 14. **Context envelope**: The `context` object MUST include `targetNamespaces` from `spec.targetNamespaces`, synthesized `previousAttempts` from failed prior `status.steps.*.results` entries, `approvedOption` when executing/verifying, and `executionResult` when verifying. Note: the sandbox context prefix formatter (see sandbox `run-api.md`) only expands `targetNamespaces`, `attempt`, `previousAttempts`, and `approvedOption` into the model prompt; `executionResult` is carried in `context` for tracing but verification execution details are primarily conveyed to the model via the `query` body (rendered from the verification template). 15. **Secrets — run** `spec.tools.requiredSecrets` / per-step tools: Secret objects MUST live in the **same namespace as the `AgenticRun`**. Mounting into sandbox MUST honor `SecretMountSpec`: environment variable injection OR file mount at configured absolute path. @@ -49,6 +49,8 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes 19b. [PLANNED: OLS-3594] **OLSConfig / classic-operator handoff** for introspection and connection details is deferred with 19a (includes OLS-3572). 20. **Sandbox observability patch**: Immediately after creating a claim (or Pod in `bare-pod` mode), the controller MUST patch `AgenticRun.status.steps..sandbox` with the resource name and operator namespace so consoles/CLIs can tail logs before the sandbox is ready. In `bare-pod` mode, `status.steps..sandbox.claimName` MUST be set to the Pod name (same field used for SandboxClaim names in `sandbox-claim` mode). 21. **Execution RBAC materialization**: When the approved remediation option includes RBAC requests, before execution the controller MUST create a **per-run ServiceAccount** named `ls-exec-{run-namespace}-{run-name}` in the operator namespace (truncated to 63 chars), then create namespace-scoped `Role`+`RoleBinding` pairs in each target namespace and `ClusterRole`+`ClusterRoleBinding` for cluster-scoped rules, binding subjects to **this per-run SA** (not the shared `lightspeed-agent` SA). The per-run SA MUST NOT carry an owner reference — cross-namespace owner refs are unsupported by Kubernetes GC (the SA lives in the operator namespace while the AgenticRun may be in a different namespace). Cleanup is handled explicitly by `cleanupExecutionRBAC` via the AgenticRun's finalizer. Idempotent create MUST tolerate existing objects (`AlreadyExists` is a no-op). Only execution sandbox pods use this SA — analysis and verification pods continue using `lightspeed-agent`. The operator's `cluster-admin` privilege (external prerequisite) ensures no RBAC escalation issues when creating arbitrary Roles/SAs. See the workspace-level agentic security spec at `ols/.ai/spec/what/agentic-security.md` rules 7-15 for full specification. +21a. **Read RBAC multi-binding resolution**: The shared `lightspeed-agent` SA may have **multiple** `ClusterRoleBinding`s granting read access (e.g. `cluster-reader` and `cluster-monitoring-view`). When granting read permissions to a per-run execution SA, the controller MUST discover **all** `ClusterRoleBinding`s where `lightspeed-agent` is a subject and add the per-run SA to every one of them — not just the first match. Discovery results MUST be cached for the lifetime of the process (the infrastructure CRBs are static). On cleanup, the per-run SA MUST be removed from all discovered bindings. [OLS-3712] +21b. **Execution outcome override**: When the execution agent reports `success=false` but **all mutating actions succeeded**, the controller MUST NOT hard-fail the run. Instead it MUST override the execution outcome to `Succeeded` and proceed to the verification step (or trust-mode completion if verification is absent). The controller determines mutation success by inspecting `ExecutionAction` entries: observation action types (`pre-check`, `post-check`, `verification`, `check`, `wait`) are excluded; all remaining action types are considered mutations. The run MUST hard-fail only when (a) no mutating actions exist (agent never attempted the fix), or (b) any mutating action has `outcome=Failed`. [OLS-3558] 22. **RBAC subjects namespace**: RoleBindings MUST reference the **per-run service account** in the **operator namespace** (where sandbox pods run), even when roles live in target namespaces. 23. **RBAC tracking annotation**: The controller SHOULD persist the list of namespaces receiving namespace-scoped RBAC on the `AgenticRun` via a dedicated annotation so cleanup can run after retries or status resets. 24. **RBAC cleanup**: The per-run SA and all associated Roles/RoleBindings/ClusterRoles/ClusterRoleBindings MUST be **explicitly deleted immediately after execution completes** (before verification starts). This ensures the execution SA does not persist into subsequent phases. On terminal outcomes (failure, escalation, deletion, `EmergencyStopped`), the controller MUST also run cleanup as a fallback (see `system-config.md` rule 14). No owner references are used (cross-namespace GC unsupported); the AgenticRun finalizer guarantees cleanup on deletion even if in-flight cleanup fails. @@ -97,5 +99,7 @@ Behavioral specification for how workflow steps run inside ephemeral **sandboxes - [PLANNED: OLS-3038] **TLS verification and network policy** for agent traffic may replace permissive internal TLS client behavior. - [PLANNED: OLS-2894] Support **multiple concurrent skills images** in template derivation beyond the first `skills` entry if product requires composite skill bundles. - [DONE: OLS-3295] Renamed per-run ServiceAccount from `ls-exec-{proposal-namespace}-{proposal-name}` to `ls-exec-{run-namespace}-{run-name}` and updated all associated RBAC resource naming. +- [DONE: OLS-3712] Read RBAC multi-binding resolution — controller discovers all `ClusterRoleBinding`s for `lightspeed-agent` SA, not just the first. See rule 21a. +- [DONE: OLS-3558] Execution outcome override — controller overrides `success=false` to `Succeeded` when all mutations passed, deferring outcome to verification. See rule 21b. - [PLANNED: OLS-3572] Inter-operator configuration handoff — refactor `PodSpecBuilder` to read a base `corev1.PodSpec` from the `lightspeed-sandbox-config` ConfigMap (produced by lightspeed-operator) and apply all per-run config (LLM creds, skills, per-run MCP, SA, probes, audit, reasoning) through a single overlay code path. Eliminates the current duplication between `PodSpecBuilder` (bare-pod) and `EnsureAgentTemplate` (sandbox-claim) — one function builds the complete PodSpec, then the mode determines delivery: bare-pod creates a Pod directly, sandbox-claim converts the PodSpec into a `SandboxTemplate`. The `--sandbox-mode` flag is deprecated in favor of `sandbox-mode` key in the ConfigMap (sourced from `OLSConfig.spec.agenticOLS.sandboxMode`). If the ConfigMap is missing, the agentic operator fails hard after timeout — no fallback. [PLANNED: OLS-3572 — design doc TBD]. - [PLANNED: OLS-3594] Optional default ocp-mcp auto-injection into sandbox pods — deferred; product value unconfirmed. Blocked by OLS-3526 (standalone HTTPS ocp-mcp) and OLS-3572 (inter-operator config handoff). Not near-term. diff --git a/controller/agenticrun/handlers.go b/controller/agenticrun/handlers.go index 26b64129..d7993b08 100644 --- a/controller/agenticrun/handlers.go +++ b/controller/agenticrun/handlers.go @@ -334,7 +334,11 @@ func (r *AgenticRunReconciler) handleExecution( return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionExecuted, err) } if !execResult.Success { - return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionExecuted, fmt.Errorf("execution agent reported failure")) + if !hasMutationSuccess(execResult.ActionsTaken) { + return r.failStep(spanCtx, run, agenticv1alpha1.AgenticRunConditionExecuted, fmt.Errorf("execution agent reported failure")) + } + log.Info("execution agent reported success=false but all mutations succeeded; deferring outcome to verification step") + execResult.Success = true } base = run.DeepCopy() @@ -752,6 +756,34 @@ func conditionTime(conditions []metav1.Condition, condType string) *metav1.Time return nil } +// hasMutationSuccess returns true if at least one mutating action succeeded +// and none failed. Returns false when no mutating actions exist (the agent +// never attempted the fix) or when any mutation was rejected. +// Non-mutating action types (pre-check, post-check, verification, check, wait) +// are skipped — their outcome reflects observation, not execution success. +func hasMutationSuccess(actions []agenticv1alpha1.ExecutionAction) bool { + found := false + for i := range actions { + if isObservationAction(actions[i].Type) { + continue + } + if actions[i].Outcome != agenticv1alpha1.ActionOutcomeSucceeded { + return false + } + found = true + } + return found +} + +func isObservationAction(actionType string) bool { + switch actionType { + case "pre-check", "post-check", "verification", "check", "wait": + return true + default: + return false + } +} + // denyAgenticRun transitions the run to Denied (terminal). func (r *AgenticRunReconciler) denyAgenticRun( ctx context.Context, diff --git a/controller/agenticrun/handlers_test.go b/controller/agenticrun/handlers_test.go index 1e0011fd..03d1e8a8 100644 --- a/controller/agenticrun/handlers_test.go +++ b/controller/agenticrun/handlers_test.go @@ -1016,7 +1016,7 @@ func TestReconcile_ExecutingPhase_DoesNotReExecute(t *testing.T) { } } -func TestReconcile_ExecutionOutcomeFailed_FailsStep(t *testing.T) { +func TestReconcile_ExecutionMutationFailed_FailsStep(t *testing.T) { scheme := testScheme() run := testAgenticRun() @@ -1031,16 +1031,75 @@ func TestReconcile_ExecutionOutcomeFailed_FailsStep(t *testing.T) { } r := &AgenticRunReconciler{Client: fc, Agent: agent, Namespace: "default"} - // Analysis → Executing reconcileOnce(r, "fix-crash") - // Approve approveAgenticRun(t, fc, "fix-crash") - // Execution with success=false → Failed reconcileOnce(r, "fix-crash") p, _ := getAgenticRun(r, "fix-crash") if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseFailed { - t.Fatalf("expected Failed when execution success=false, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + t.Fatalf("expected Failed when mutation action failed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } +} + +func TestReconcile_ExecutionPreCheckFailed_ProceedsToVerification(t *testing.T) { + scheme := testScheme() + run := testAgenticRun() + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build() + + agent := newTestAgentCaller() + agent.executeResult = &ExecutionOutput{ + Success: false, + ActionsTaken: []agenticv1alpha1.ExecutionAction{ + {Type: "pre-check", Description: "Confirmed problem exists", Outcome: agenticv1alpha1.ActionOutcomeFailed}, + {Type: "patch", Description: "Patched deployment", Outcome: agenticv1alpha1.ActionOutcomeSucceeded}, + }, + } + r := &AgenticRunReconciler{Client: fc, Agent: agent, Namespace: "default"} + + reconcileOnce(r, "fix-crash") + approveAgenticRun(t, fc, "fix-crash") + reconcileOnce(r, "fix-crash") + + p, _ := getAgenticRun(r, "fix-crash") + phase := agenticv1alpha1.DerivePhase(p.Status.Conditions) + if phase != agenticv1alpha1.AgenticRunPhaseVerifying { + t.Fatalf("expected Verifying when only pre-check failed (observational), got %s", phase) + } +} + +func TestReconcile_ExecutionInlineVerificationFailed_ProceedsToVerification(t *testing.T) { + scheme := testScheme() + run := testAgenticRun() + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build() + + agent := newTestAgentCaller() + agent.executeResult = &ExecutionOutput{ + Success: false, + ActionsTaken: []agenticv1alpha1.ExecutionAction{ + {Type: "patch", Description: "Patched NetworkPolicy", Outcome: agenticv1alpha1.ActionOutcomeSucceeded}, + {Type: "verification", Description: "Checked frontend logs", Outcome: agenticv1alpha1.ActionOutcomeFailed}, + }, + Verification: agenticv1alpha1.ExecutionVerification{ + ConditionOutcome: agenticv1alpha1.ConditionOutcomeUnchanged, + Summary: "Logs still show timeout — checked too early", + }, + } + r := &AgenticRunReconciler{Client: fc, Agent: agent, Namespace: "default"} + + reconcileOnce(r, "fix-crash") + approveAgenticRun(t, fc, "fix-crash") + reconcileOnce(r, "fix-crash") + + p, _ := getAgenticRun(r, "fix-crash") + phase := agenticv1alpha1.DerivePhase(p.Status.Conditions) + if phase != agenticv1alpha1.AgenticRunPhaseVerifying { + t.Fatalf("expected Verifying when only inline verification failed, got %s", phase) } } diff --git a/controller/agenticrun/helpers.go b/controller/agenticrun/helpers.go index 9d95cf65..104477c6 100644 --- a/controller/agenticrun/helpers.go +++ b/controller/agenticrun/helpers.go @@ -42,7 +42,11 @@ const ( templogCleanupAttemptsAnnotation = "agentic.openshift.io/templog-cleanup-attempts" templogMaxCleanupAttempts = 3 - templogCleanupRequeueAfter = 30 * time.Second + templogCleanupRequeueAfter = 5 * time.Second + + rbacCleanupAttemptsAnnotation = "agentic.openshift.io/rbac-cleanup-attempts" + rbacMaxCleanupAttempts = 3 + rbacCleanupRequeueAfter = 1 * time.Second reasonInProgress = "InProgress" reasonComplete = "Complete" diff --git a/controller/agenticrun/rbac.go b/controller/agenticrun/rbac.go index eb272de9..4321315d 100644 --- a/controller/agenticrun/rbac.go +++ b/controller/agenticrun/rbac.go @@ -36,43 +36,46 @@ const ( ErrDeleteExecutionSA = "delete execution SA" ) -var readerRoleBinding atomic.Value +var readerBindings atomic.Value // []string — cached CRB names; nil until first discovery func init() { - readerRoleBinding.Store(defaultReaderClusterRoleBinding) + readerBindings.Store([]string(nil)) } -// discoverReaderBinding finds the ClusterRoleBinding for the lightspeed-agent SA -// when the default name doesn't exist. Updates readerRoleBinding on success. -func discoverReaderBinding(ctx context.Context, c client.Client, operatorNS string) error { +// resolveReaderBindings returns all ClusterRoleBindings that list the +// lightspeed-agent SA as a subject. Results are discovered once and cached +// for the lifetime of the process (CRBs are static infrastructure). +// If discovery has not yet succeeded, it is triggered on demand. +func resolveReaderBindings(ctx context.Context, c client.Client, operatorNS string) ([]string, error) { + if names := readerBindings.Load().([]string); len(names) > 0 { + return names, nil + } + log := logf.FromContext(ctx) - log.Info("default reader ClusterRoleBinding not found, discovering by SA subject", LogKeyName, defaultReaderClusterRoleBinding) + log.Info("discovering reader ClusterRoleBindings by SA subject") crbList := &rbacv1.ClusterRoleBindingList{} if err := c.List(ctx, crbList); err != nil { - return fmt.Errorf("list ClusterRoleBindings for reader discovery: %w", err) + return nil, fmt.Errorf("list ClusterRoleBindings for reader discovery: %w", err) } - var matches []string + var names []string for i := range crbList.Items { for _, s := range crbList.Items[i].Subjects { if s.Kind == rbacv1.ServiceAccountKind && s.Name == defaultSandboxSA && s.Namespace == operatorNS { - matches = append(matches, crbList.Items[i].Name) + names = append(names, crbList.Items[i].Name) break } } } - if len(matches) == 0 { - return fmt.Errorf("no ClusterRoleBinding found with subject %s/%s — ensure reader RBAC is configured", operatorNS, defaultSandboxSA) - } - if len(matches) > 1 { - log.Info("multiple ClusterRoleBindings found for lightspeed-agent SA", "all", matches, "selected", matches[0]) + if len(names) == 0 { + return nil, fmt.Errorf("no ClusterRoleBinding found with subject %s/%s — ensure reader RBAC is configured", operatorNS, defaultSandboxSA) } - readerRoleBinding.Store(matches[0]) - log.Info("resolved reader ClusterRoleBinding", LogKeyName, matches[0]) - return nil + log.Info("resolved reader ClusterRoleBindings", "bindings", names) + readerBindings.Store(names) + return names, nil } // executionSAName returns the per-run ServiceAccount name for execution RBAC isolation. @@ -107,27 +110,35 @@ func ensureExecutionSA(ctx context.Context, c client.Client, run *agenticv1alpha return saName, nil } -// addReaderSubject adds the SA as a subject to the shared cluster-reader ClusterRoleBinding. -// Idempotent — skips if the subject is already present. Retries on conflict (optimistic locking). -// If the default binding name doesn't exist, discovers the correct name by scanning SA subjects. +// addReaderSubject adds the SA as a subject to every ClusterRoleBinding that +// references the lightspeed-agent SA (e.g. cluster-reader, cluster-monitoring-view). +// Idempotent — skips bindings where the subject is already present. +// Retries each binding independently on conflict (optimistic locking). func addReaderSubject(ctx context.Context, c client.Client, saName, namespace string) error { + names, err := resolveReaderBindings(ctx, c, namespace) + if err != nil { + return fmt.Errorf("%s: %w", ErrAddReaderSubject, err) + } + subject := rbacv1.Subject{ Kind: rbacv1.ServiceAccountKind, Name: saName, Namespace: namespace, } + for _, bindingName := range names { + if err := addSubjectToBinding(ctx, c, bindingName, subject); err != nil { + return err + } + } + return nil +} + +func addSubjectToBinding(ctx context.Context, c client.Client, bindingName string, subject rbacv1.Subject) error { for attempts := 0; attempts < 3; attempts++ { - bindingName := readerRoleBinding.Load().(string) crb := &rbacv1.ClusterRoleBinding{} if err := c.Get(ctx, client.ObjectKey{Name: bindingName}, crb); err != nil { - if apierrors.IsNotFound(err) { - if discoverErr := discoverReaderBinding(ctx, c, namespace); discoverErr != nil { - return fmt.Errorf("%s: %w", ErrAddReaderSubject, discoverErr) - } - continue - } - return fmt.Errorf("%s: %w", ErrAddReaderSubject, err) + return fmt.Errorf("%s %s: %w", ErrAddReaderSubject, bindingName, err) } for _, s := range crb.Subjects { @@ -142,27 +153,40 @@ func addReaderSubject(ctx context.Context, c client.Client, saName, namespace st return nil } if !apierrors.IsConflict(err) { - return fmt.Errorf("%s: %w", ErrAddReaderSubject, err) + return fmt.Errorf("%s %s: %w", ErrAddReaderSubject, bindingName, err) } } return fmt.Errorf("%s: conflict after retries", ErrAddReaderSubject) } -// removeReaderSubject removes the SA from the shared cluster-reader ClusterRoleBinding. -// Idempotent — no-op if the subject is not present. Retries on conflict (optimistic locking). -// If the default binding name doesn't exist, discovers the correct name (mirrors addReaderSubject). +// removeReaderSubject removes the SA from every cached reader ClusterRoleBinding. +// Idempotent — no-op if the subject is not present. Returns nil when no bindings +// are found (they may have been deleted during teardown); propagates all other errors. func removeReaderSubject(ctx context.Context, c client.Client, saName, namespace string) error { + names, err := resolveReaderBindings(ctx, c, namespace) + if err != nil { + if strings.Contains(err.Error(), "no ClusterRoleBinding found") { + return nil + } + return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err) + } + + for _, bindingName := range names { + if err := removeSubjectFromBinding(ctx, c, bindingName, saName, namespace); err != nil { + return err + } + } + return nil +} + +func removeSubjectFromBinding(ctx context.Context, c client.Client, bindingName, saName, namespace string) error { for attempts := 0; attempts < 3; attempts++ { - bindingName := readerRoleBinding.Load().(string) crb := &rbacv1.ClusterRoleBinding{} if err := c.Get(ctx, client.ObjectKey{Name: bindingName}, crb); err != nil { if apierrors.IsNotFound(err) { - if discoverErr := discoverReaderBinding(ctx, c, namespace); discoverErr != nil { - return nil - } - continue + return nil // binding gone, nothing to remove } - return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err) + return fmt.Errorf("%s %s: %w", ErrRemoveReaderSubject, bindingName, err) } filtered := make([]rbacv1.Subject, 0, len(crb.Subjects)) @@ -183,7 +207,7 @@ func removeReaderSubject(ctx context.Context, c client.Client, saName, namespace return nil } if !apierrors.IsConflict(err) { - return fmt.Errorf("%s: %w", ErrRemoveReaderSubject, err) + return fmt.Errorf("%s %s: %w", ErrRemoveReaderSubject, bindingName, err) } } return fmt.Errorf("%s: conflict after retries", ErrRemoveReaderSubject) diff --git a/controller/agenticrun/rbac_test.go b/controller/agenticrun/rbac_test.go index 3ac21ddd..0c1d15ce 100644 --- a/controller/agenticrun/rbac_test.go +++ b/controller/agenticrun/rbac_test.go @@ -19,6 +19,10 @@ import ( agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" ) +func resetReaderBindings() { + readerBindings.Store([]string(nil)) +} + // readerBinding returns the pre-existing cluster-reader ClusterRoleBinding fixture // that must exist for execution SA setup to succeed. func readerBinding() *rbacv1.ClusterRoleBinding { @@ -39,6 +43,7 @@ func readerBinding() *rbacv1.ClusterRoleBinding { func TestEnsureExecutionRBAC_NamespaceScopedOnly(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -127,6 +132,7 @@ func TestEnsureExecutionRBAC_NamespaceScopedOnly(t *testing.T) { func TestEnsureExecutionRBAC_ClusterScopedOnly(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -171,6 +177,7 @@ func TestEnsureExecutionRBAC_ClusterScopedOnly(t *testing.T) { func TestEnsureExecutionRBAC_BothScopes(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -207,6 +214,7 @@ func TestEnsureExecutionRBAC_BothScopes(t *testing.T) { func TestEnsureExecutionRBAC_MultipleNamespaces(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -245,6 +253,7 @@ func TestEnsureExecutionRBAC_MultipleNamespaces(t *testing.T) { func TestEnsureExecutionRBAC_Idempotent(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -272,6 +281,7 @@ func TestEnsureExecutionRBAC_Idempotent(t *testing.T) { func TestEnsureExecutionRBAC_NilResult(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -285,6 +295,7 @@ func TestEnsureExecutionRBAC_NilResult(t *testing.T) { func TestEnsureExecutionRBAC_EmptyRules(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -306,6 +317,7 @@ func TestEnsureExecutionRBAC_EmptyRules(t *testing.T) { func TestEnsureExecutionRBAC_NamespacesFromRBACRules(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() ns1 := "app-ns" @@ -335,6 +347,7 @@ func TestEnsureExecutionRBAC_NamespacesFromRBACRules(t *testing.T) { func TestEnsureExecutionRBAC_ResourceNames(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -370,6 +383,7 @@ func TestEnsureExecutionRBAC_ResourceNames(t *testing.T) { func TestCleanupExecutionRBAC_NamespaceAndCluster(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -435,6 +449,7 @@ func TestCleanupExecutionRBAC_NamespaceAndCluster(t *testing.T) { func TestCleanupExecutionRBAC_NoAnnotation(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -466,6 +481,7 @@ func TestCleanupExecutionRBAC_NoAnnotation(t *testing.T) { func TestCleanupExecutionRBAC_MissingResources(t *testing.T) { ctx := context.Background() + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() run := &agenticv1alpha1.AgenticRun{ @@ -813,12 +829,12 @@ func TestRBACLabels_TruncatesLongAgenticRunName(t *testing.T) { } // --------------------------------------------------------------------------- -// addReaderSubject / removeReaderSubject / discoverReaderBinding +// addReaderSubject / removeReaderSubject / resolveReaderBindings // --------------------------------------------------------------------------- func TestAddReaderSubject_Idempotent(t *testing.T) { ctx := context.Background() - readerRoleBinding.Store(defaultReaderClusterRoleBinding) + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() if err := addReaderSubject(ctx, fc, "ls-exec-test", "default"); err != nil { @@ -844,9 +860,46 @@ func TestAddReaderSubject_Idempotent(t *testing.T) { } } +func TestAddReaderSubject_MultipleBindings(t *testing.T) { + ctx := context.Background() + resetReaderBindings() + + monitoringBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "lightspeed-agent-monitoring-view"}, + RoleRef: rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "ClusterRole", Name: "cluster-monitoring-view"}, + Subjects: []rbacv1.Subject{{ + Kind: rbacv1.ServiceAccountKind, + Name: defaultSandboxSA, + Namespace: "default", + }}, + } + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding(), monitoringBinding).Build() + + if err := addReaderSubject(ctx, fc, "ls-exec-test", "default"); err != nil { + t.Fatalf("addReaderSubject: %v", err) + } + + for _, name := range []string{defaultReaderClusterRoleBinding, "lightspeed-agent-monitoring-view"} { + var crb rbacv1.ClusterRoleBinding + if err := fc.Get(ctx, types.NamespacedName{Name: name}, &crb); err != nil { + t.Fatalf("get %s: %v", name, err) + } + found := false + for _, s := range crb.Subjects { + if s.Name == "ls-exec-test" { + found = true + break + } + } + if !found { + t.Fatalf("subject ls-exec-test not added to %s", name) + } + } +} + func TestRemoveReaderSubject_NotPresent(t *testing.T) { ctx := context.Background() - readerRoleBinding.Store(defaultReaderClusterRoleBinding) + resetReaderBindings() fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() if err := removeReaderSubject(ctx, fc, "ls-exec-nonexistent", "default"); err != nil { @@ -854,9 +907,45 @@ func TestRemoveReaderSubject_NotPresent(t *testing.T) { } } -func TestDiscoverReaderBinding_NoMatches(t *testing.T) { +func TestRemoveReaderSubject_MultipleBindings(t *testing.T) { ctx := context.Background() - readerRoleBinding.Store(defaultReaderClusterRoleBinding) + resetReaderBindings() + + saName := "ls-exec-remove-test" + monitoringBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "lightspeed-agent-monitoring-view"}, + RoleRef: rbacv1.RoleRef{APIGroup: rbacv1.GroupName, Kind: "ClusterRole", Name: "cluster-monitoring-view"}, + Subjects: []rbacv1.Subject{ + {Kind: rbacv1.ServiceAccountKind, Name: defaultSandboxSA, Namespace: "default"}, + {Kind: rbacv1.ServiceAccountKind, Name: saName, Namespace: "default"}, + }, + } + readerWithExec := readerBinding() + readerWithExec.Subjects = append(readerWithExec.Subjects, rbacv1.Subject{ + Kind: rbacv1.ServiceAccountKind, Name: saName, Namespace: "default", + }) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerWithExec, monitoringBinding).Build() + + if err := removeReaderSubject(ctx, fc, saName, "default"); err != nil { + t.Fatalf("removeReaderSubject: %v", err) + } + + for _, name := range []string{defaultReaderClusterRoleBinding, "lightspeed-agent-monitoring-view"} { + var crb rbacv1.ClusterRoleBinding + if err := fc.Get(ctx, types.NamespacedName{Name: name}, &crb); err != nil { + t.Fatalf("get %s: %v", name, err) + } + for _, s := range crb.Subjects { + if s.Name == saName { + t.Fatalf("subject %s should have been removed from %s", saName, name) + } + } + } +} + +func TestResolveReaderBindings_NoMatches(t *testing.T) { + ctx := context.Background() + resetReaderBindings() unrelatedBinding := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{Name: "unrelated-binding"}, @@ -869,7 +958,7 @@ func TestDiscoverReaderBinding_NoMatches(t *testing.T) { } fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(unrelatedBinding).Build() - err := discoverReaderBinding(ctx, fc, "default") + _, err := resolveReaderBindings(ctx, fc, "default") if err == nil { t.Fatal("expected error when no matching bindings found") } @@ -878,9 +967,9 @@ func TestDiscoverReaderBinding_NoMatches(t *testing.T) { } } -func TestDiscoverReaderBinding_MultipleMatches(t *testing.T) { +func TestResolveReaderBindings_MultipleMatches(t *testing.T) { ctx := context.Background() - readerRoleBinding.Store(defaultReaderClusterRoleBinding) + resetReaderBindings() binding1 := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{Name: "custom-reader-1"}, @@ -902,20 +991,44 @@ func TestDiscoverReaderBinding_MultipleMatches(t *testing.T) { } fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(binding1, binding2).Build() - err := discoverReaderBinding(ctx, fc, "default") + resolved, err := resolveReaderBindings(ctx, fc, "default") if err != nil { t.Fatalf("unexpected error: %v", err) } + if len(resolved) != 2 { + t.Fatalf("expected 2 bindings, got %d: %v", len(resolved), resolved) + } + + found := map[string]bool{} + for _, name := range resolved { + found[name] = true + } + if !found["custom-reader-1"] || !found["custom-reader-2"] { + t.Fatalf("expected both custom bindings, got: %v", resolved) + } +} + +func TestResolveReaderBindings_Cached(t *testing.T) { + ctx := context.Background() + resetReaderBindings() + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(readerBinding()).Build() - resolved := readerRoleBinding.Load().(string) - if resolved != "custom-reader-1" && resolved != "custom-reader-2" { - t.Fatalf("expected one of the custom bindings, got: %s", resolved) + first, err := resolveReaderBindings(ctx, fc, "default") + if err != nil { + t.Fatalf("first resolve: %v", err) + } + second, err := resolveReaderBindings(ctx, fc, "default") + if err != nil { + t.Fatalf("second resolve: %v", err) + } + if len(first) != len(second) || first[0] != second[0] { + t.Fatalf("cached result should match: %v vs %v", first, second) } } func TestAddReaderSubject_ConflictRetryExhaustion(t *testing.T) { ctx := context.Background() - readerRoleBinding.Store(defaultReaderClusterRoleBinding) + resetReaderBindings() callCount := 0 fc := fake.NewClientBuilder(). diff --git a/controller/agenticrun/reconciler.go b/controller/agenticrun/reconciler.go index 81b7e0ac..52c35812 100644 --- a/controller/agenticrun/reconciler.go +++ b/controller/agenticrun/reconciler.go @@ -22,6 +22,7 @@ const ( ErrRemoveFinalizer = "remove finalizer" ErrAddFinalizer = "add finalizer" ErrPatchTemplogCleanupAttempts = "patch templog cleanup attempts" + ErrPatchRBACCleanupAttempts = "patch rbac cleanup attempts" ) // TempLogCleaner is the interface for deleting templog records on CR deletion. @@ -74,20 +75,7 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) r.Audit.Cleanup(&run) } if controllerutil.ContainsFinalizer(&run, rbacCleanupFinalizer) { - if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { - return ctrl.Result{}, err - } - if err := cleanupExecutionRBAC(ctx, r.Client, &run, r.Namespace); err != nil { - return ctrl.Result{}, err - } - original := run.DeepCopy() - controllerutil.RemoveFinalizer(&run, rbacCleanupFinalizer) - if err := r.Patch(ctx, &run, client.MergeFrom(original)); err != nil { - return ctrl.Result{}, fmt.Errorf("%s: %w", ErrRemoveFinalizer, err) - } - // Requeue so templog cleanup Patches a fresh object (avoids conflict - // from two sequential Patches in one reconcile). - return ctrl.Result{Requeue: true}, nil + return r.handleRBACCleanup(ctx, &run) } if controllerutil.ContainsFinalizer(&run, templogCleanupFinalizer) { return r.handleTemplogCleanup(ctx, &run) @@ -322,3 +310,54 @@ func (r *AgenticRunReconciler) handleTemplogCleanup(ctx context.Context, run *ag } return ctrl.Result{}, nil } + +// handleRBACCleanup releases sandboxes and deletes execution RBAC resources +// for a deleting AgenticRun. Retries up to rbacMaxCleanupAttempts, then +// removes the finalizer regardless to avoid leaving the CR stuck in Terminating. +func (r *AgenticRunReconciler) handleRBACCleanup(ctx context.Context, run *agenticv1alpha1.AgenticRun) (ctrl.Result, error) { + log := logf.FromContext(ctx) + + attempts := 0 + if v, ok := run.Annotations[rbacCleanupAttemptsAnnotation]; ok { + parsed, err := strconv.Atoi(v) + if err != nil || parsed < 0 { + log.Info("ignoring invalid rbac cleanup attempts annotation", "value", v) + attempts = 0 + } else { + attempts = parsed + } + } + + if attempts < rbacMaxCleanupAttempts { + sandboxErr := r.Agent.ReleaseSandboxes(ctx, run) + rbacErr := cleanupExecutionRBAC(ctx, r.Client, run, r.Namespace) + if sandboxErr != nil || rbacErr != nil { + if sandboxErr != nil { + log.Error(sandboxErr, "sandbox release failed, will retry", "attempt", attempts+1, "max", rbacMaxCleanupAttempts) + } + if rbacErr != nil { + log.Error(rbacErr, "RBAC cleanup failed, will retry", "attempt", attempts+1, "max", rbacMaxCleanupAttempts) + } + original := run.DeepCopy() + if run.Annotations == nil { + run.Annotations = make(map[string]string) + } + run.Annotations[rbacCleanupAttemptsAnnotation] = fmt.Sprintf("%d", attempts+1) + if patchErr := r.Patch(ctx, run, client.MergeFrom(original)); patchErr != nil { + return ctrl.Result{}, fmt.Errorf("%s: %w", ErrPatchRBACCleanupAttempts, patchErr) + } + return ctrl.Result{RequeueAfter: rbacCleanupRequeueAfter}, nil + } + } else { + log.Info("RBAC cleanup exhausted retries, removing finalizer with orphaned resources", "attempts", attempts) + } + + original := run.DeepCopy() + controllerutil.RemoveFinalizer(run, rbacCleanupFinalizer) + if err := r.Patch(ctx, run, client.MergeFrom(original)); err != nil { + return ctrl.Result{}, fmt.Errorf("%s: %w", ErrRemoveFinalizer, err) + } + // Requeue so templog cleanup Patches a fresh object (avoids conflict + // from two sequential Patches in one reconcile). + return ctrl.Result{Requeue: true}, nil +} diff --git a/controller/agenticrun/state_machine_test.go b/controller/agenticrun/state_machine_test.go index 4b5c6718..6ba67105 100644 --- a/controller/agenticrun/state_machine_test.go +++ b/controller/agenticrun/state_machine_test.go @@ -625,10 +625,10 @@ func TestManualApproval_NoAutoApprovedStages(t *testing.T) { } // --------------------------------------------------------------------------- -// Execution not reported as success → fails +// Execution success=false with no mutations → hard failure // --------------------------------------------------------------------------- -func TestManualApproval_ExecutionReportsFailure(t *testing.T) { +func TestManualApproval_ExecutionSuccessFalse_NoMutations_Fails(t *testing.T) { run := testAgenticRun() agent := newTestAgentCaller() agent.executeResult = &ExecutionOutput{Success: false} diff --git a/controller/agenticrun/templates/analysis_query.tmpl b/controller/agenticrun/templates/analysis_query.tmpl index d367aeee..3384b38c 100644 --- a/controller/agenticrun/templates/analysis_query.tmpl +++ b/controller/agenticrun/templates/analysis_query.tmpl @@ -7,38 +7,29 @@ For each option you propose: - **Diagnose** the root cause with confidence level. - **Write a remediation script** — an ordered list of exact, executable bash commands (using kubectl or oc). Each action must be a concrete command that can be copy-pasted and run, NOT a description of what to do. Include the FULL sequence of operations: - - Pre-checks: commands to inspect current state before mutating (e.g., get the resource to confirm current values) - Mutations: the actual fix commands - Wait/status commands: commands to wait for changes to propagate (e.g., kubectl rollout status, wait for pods to become ready) - - Post-checks: commands to verify the mutation took effect - Think step by step: if you were executing this remediation by hand in a terminal, what commands would you run in sequence? Include ALL of them — not just the key mutation. For example, "scale a deployment" is not one command — it's: get current state, patch the replicas, wait for new pods, verify ready count. + Think step by step: if you were executing this remediation by hand in a terminal, what commands would you run in sequence? Include ALL of them — not just the key mutation. For example, "scale a deployment" is not one command — it's: patch the replicas, then wait for new pods to become ready. Good: kubectl set image deployment/foo container=registry/foo:v1.3 -n production Bad: Update the deployment image to the latest version {{- if .HasExecution}} -- **Derive RBAC from your script** — walk through EVERY command and determine the exact API calls it makes. Use these rules: +- **Derive RBAC for mutations and subresource access only** — the execution environment already has all required read access to standard resources. Only derive RBAC for API calls that require write verbs (`create`, `patch`, `update`, `delete`) or access subresources (`get` on `scale`, `create` on `pods/exec`, etc.). Use these rules: **Command → API mapping** (use ONLY the verbs each command needs): - - `kubectl get ` → verb `get`, resourceNames: [name] - - `kubectl get ` or `kubectl get -l ` → verb `list`, NO resourceNames - - `kubectl describe ` → verb `get`, resourceNames: [name] - `kubectl patch ` → verb `patch`, resourceNames: [name] - `kubectl delete ` → verb `delete`, resourceNames: [name] - `kubectl scale ` → verbs `get`, `update` on subresource `/scale`, resourceNames: [name] - - `kubectl rollout status /` → `get` + `watch` on ``, plus `list` + `watch` on replicasets - `kubectl rollout undo /` → `patch` on ``, plus `get` + `list` on replicasets (replicaset names are auto-generated — never restrict by resourceNames) - `kubectl rollout restart /` → `patch` on ``, resourceNames: [name] - - `kubectl logs deploy/` → `get` on the deployment + `list` pods (no resourceNames) + `get` pods/log - - `kubectl logs ` → `get` pods/log, resourceNames: [pod] - `kubectl exec -- ` → `get` pods + `create` pods/exec, resourceNames: [pod] - `kubectl apply/create -f` → `create` (or `patch` for apply) on the resource type being applied - `kubectl label/annotate ` → `patch`, resourceNames: [name] **K8s RBAC constraints** (violating these causes silent permission failures): - - `list` and `watch` verbs CANNOT be restricted by `resourceNames` — Kubernetes ignores resourceNames for these verbs unless the request uses a `metadata.name` field selector, which kubectl does not do. Always emit `list`/`watch` without resourceNames. If you also need `get` (with resourceNames) on the same resource, emit TWO separate RBAC rules. - - `create` CANNOT be restricted by `resourceNames` for standard resources — only for subresources like `pods/exec`. Only use `resourceNames` with `get`, `patch`, `update`, `delete`, and `create` on subresources. + - `create` CANNOT be restricted by `resourceNames` for standard resources — only for subresources like `pods/exec`. Only use `resourceNames` with `patch`, `update`, `delete`, and `create` on subresources. - Replicasets accessed by `rollout` commands have auto-generated names — never use resourceNames for replicasets. **Scope rule**: only include RBAC for API calls YOUR commands make. Do not add permissions for internal Kubernetes side-effects (e.g., patching a Deployment does not require separate replicaset permissions unless your script explicitly runs a rollout command that watches replicasets). diff --git a/controller/agenticrun/templates/execution_query.tmpl b/controller/agenticrun/templates/execution_query.tmpl index a7b37364..9fc7914a 100644 --- a/controller/agenticrun/templates/execution_query.tmpl +++ b/controller/agenticrun/templates/execution_query.tmpl @@ -4,13 +4,15 @@ The approved option contains a concrete remediation script — an ordered list o 1. **Execute commands in order.** Do not skip, reorder, or substitute commands. Run each command exactly as written. -2. **Dry-run every mutation** before applying. For any command that modifies cluster state (create, patch, apply, set, scale, delete, etc.), first run it with `--dry-run=server`. If the command rejects the `--dry-run` flag itself (unknown flag, unsupported option), skip the dry-run and apply directly. If the dry-run fails for any other reason (validation error, resource conflict, permission denied), you may fix **only non-semantic errors** (e.g., a missing `--namespace` flag already implied by the target namespace context). You MUST NOT change the command's intent, target resource, or action. If the failure indicates the command's intent cannot succeed as-written, **stop and report failure**. Any command you modify must be flagged in your output with both the original and modified versions. +2. **Dry-run every mutation** before applying. For any command that modifies cluster state, first run it with `--dry-run=server`. If the command does not support `--dry-run`, skip it in dry-run execution. -3. **Watch for runtime failures** after mutations. After applying a change, briefly verify it took effect (e.g., check pod status, watch for ImagePullBackOff, CrashLoopBackOff, or other error states). If the mutation succeeds but the workload enters a failure state, report it. +3. **Fix syntax errors, nothing else.** If any command fails due to a syntax error (malformed flag, missing `--namespace`, wrong argument order), fix the syntax and retry. Do not change the command's intent, target, or action. -4. **Stop on failure** unless the error is clearly non-blocking. If a command fails and cannot be fixed, stop execution and report all actions taken so far. +4. **Skip non-blocking failures.** If a command fails but the error is clearly non-blocking (e.g., resource already exists, already patched to the desired state), treat it as succeeded and continue. -5. **Report each action** you take, its outcome (Succeeded/Failed), and any output or errors. +5. **Stop on blocking failure.** If a command fails for any other reason, **stop and report failure**. + +6. **Report each action** you take, its outcome (Succeeded/Failed), and any output or errors. ## Approved Option diff --git a/hack/quickstart/README.md b/hack/quickstart/README.md index 97e521d0..f37bfacb 100644 --- a/hack/quickstart/README.md +++ b/hack/quickstart/README.md @@ -45,9 +45,11 @@ The script: to install it. Declining **stops** the script (re-run with `--ols-bundle-image=…`). - **No user image, OLS present:** three-way choice — (1) leave OLS as-is and - continue to agentic, (2) update to the related_images bundle, or (3) stop - and re-run with `--ols-bundle-image=…`. -3. After a successful OLS install/update: writes/exports an `OLSConfig` file + continue to agentic (still creates `OLSConfig` if the CR is missing), + (2) update to the related_images bundle, or (3) stop and re-run with + `--ols-bundle-image=…`. +3. After a successful OLS install/update, or when leaving OLS as-is but + `OLSConfig` is missing: writes/exports an `OLSConfig` file (`OLS_CONFIG_FILE`), lets you edit/apply in a loop, and waits until `status.overallStatus=Ready` (dumps diagnostics on failure). 4. Installs Agentic Operator CRDs, Deployment, ApprovalPolicy, and webhook. @@ -79,6 +81,7 @@ OLS decisions, set `REMOVE_OLS=1` (uninstall OLS) or `REMOVE_OLS=0` (keep OLS). | Flag | Description | |---|---| | `--ols-bundle-image=IMAGE` | Lightspeed Operator OLM bundle for `operator-sdk run bundle` / `bundle-upgrade`. Required non-empty when set. If omitted, resolves `lightspeed-operator-bundle` from `related_images.json` and prompts (see Install flow above). | +| `--sandbox-image=IMAGE` | Agentic sandbox container image (passed to the operator as `--agentic-sandbox-image`). Overrides `SANDBOX_IMAGE`. Defaults to the Konflux `:main` sandbox image. | | `-h`, `--help` | Show usage and exit | ## Environment Variables @@ -87,7 +90,7 @@ OLS decisions, set `REMOVE_OLS=1` (uninstall OLS) or `REMOVE_OLS=0` (keep OLS). |---|---|---| | `NAMESPACE` | `openshift-lightspeed` | Target namespace | | `OPERATOR_IMAGE` | Konflux `:main` | Agentic operator container image | -| `SANDBOX_IMAGE` | Konflux `:main` | Agent sandbox container image | +| `SANDBOX_IMAGE` | Konflux `:main` | Agent sandbox container image; same as `--sandbox-image` (flag overrides) | | `SANDBOX_MODE` | `bare-pod` | Sandbox mode (`bare-pod` or `sandbox-claim`) | | `IMAGE_PULL_POLICY` | *(empty — Kubernetes default)* | Image pull policy for operator and sandbox pods (`Always`, `IfNotPresent`, `Never`) | | `OLS_BUNDLE_IMAGE` | *(empty)* | Same as `--ols-bundle-image`; the flag overrides this env var | @@ -104,7 +107,8 @@ Example with overrides: ```bash NAMESPACE=my-ns SANDBOX_MODE=sandbox-claim bash install-agentic.sh \ - --ols-bundle-image=quay.io/example/lightspeed-operator-bundle:main + --ols-bundle-image=quay.io/example/lightspeed-operator-bundle:main \ + --sandbox-image=quay.io/example/lightspeed-agentic-sandbox:main ``` For dev environments with floating tags like `:main`, force fresh pulls: diff --git a/hack/quickstart/install.sh b/hack/quickstart/install.sh index cc80d677..200e9204 100755 --- a/hack/quickstart/install.sh +++ b/hack/quickstart/install.sh @@ -14,6 +14,7 @@ # https://raw.githubusercontent.com/openshift/lightspeed-agentic-operator/main/hack/quickstart/install.sh # bash install-agentic.sh # bash install-agentic.sh --ols-bundle-image=quay.io/.../lightspeed-operator-bundle:tag +# bash install-agentic.sh --sandbox-image=quay.io/.../lightspeed-agentic-sandbox:tag # # Do not use: curl … | bash (stdin is the script; prompts cannot read y/n) # @@ -66,6 +67,10 @@ Options: If omitted, resolves lightspeed-operator-bundle from related_images.json (${OLS_RELATED_IMAGES_URL}) and prompts (decline / option 3 stops the script). + --sandbox-image=IMAGE Agentic sandbox container image passed to the + operator as --agentic-sandbox-image. + Overrides SANDBOX_IMAGE. Default: + ${SANDBOX_IMAGE} -h, --help Show this help and exit Environment variables (OPERATOR_IMAGE, SANDBOX_IMAGE, NAMESPACE, …) are @@ -86,6 +91,17 @@ while [ $# -gt 0 ]; do [ -n "${OLS_BUNDLE_IMAGE}" ] || fail "--ols-bundle-image requires a non-empty image" shift 2 ;; + --sandbox-image=*) + SANDBOX_IMAGE="${1#*=}" + [ -n "${SANDBOX_IMAGE}" ] || fail "--sandbox-image requires a non-empty image" + shift + ;; + --sandbox-image) + [ $# -ge 2 ] || fail "--sandbox-image requires an image argument" + SANDBOX_IMAGE="$2" + [ -n "${SANDBOX_IMAGE}" ] || fail "--sandbox-image requires a non-empty image" + shift 2 + ;; -h|--help) usage exit 0 @@ -106,10 +122,20 @@ if [ -n "${_ols_bundle_raw}" ] && [ -z "${OLS_BUNDLE_IMAGE}" ]; then fi unset _ols_bundle_raw +_sandbox_raw="${SANDBOX_IMAGE}" +SANDBOX_IMAGE="${SANDBOX_IMAGE#"${SANDBOX_IMAGE%%[![:space:]]*}"}" +SANDBOX_IMAGE="${SANDBOX_IMAGE%"${SANDBOX_IMAGE##*[![:space:]]}"}" +if [ -z "${SANDBOX_IMAGE}" ]; then + fail "--sandbox-image / SANDBOX_IMAGE must be non-empty" +fi +unset _sandbox_raw + if [ -n "${OLS_BUNDLE_IMAGE}" ] && [[ "${OLS_BUNDLE_IMAGE}" != *[:/]* ]]; then fail "OLS_BUNDLE_IMAGE looks invalid: ${OLS_BUNDLE_IMAGE}" fi - +if [[ "${SANDBOX_IMAGE}" != *[:/]* ]]; then + fail "SANDBOX_IMAGE looks invalid: ${SANDBOX_IMAGE}" +fi # Set when a related_images.json bundle is resolved (for logging / summary). OLS_RELATED_BUNDLE_IMAGE="" # install | update | left-as-is | none — what happened to OLS in this run. @@ -141,6 +167,10 @@ ols_crd_installed() { oc get crd olsconfigs.ols.openshift.io >/dev/null 2>&1 } +olsconfig_exists() { + oc get olsconfig cluster >/dev/null 2>&1 +} + require_tty_for_prompts() { if [[ ! -t 0 ]]; then fail "Interactive prompts require a terminal. @@ -291,7 +321,7 @@ prompt_ols_already_installed() { ${bundle_image} Choose: - 1) Leave current OLS as-is (continue to agentic) + 1) Leave current OLS as-is 2) Update OLS to the related_images bundle above 3) Stop and re-run with --ols-bundle-image= @@ -452,6 +482,7 @@ if [ -z "${OLS_BUNDLE_IMAGE}" ]; then else info "OLS bundle image override: ${OLS_BUNDLE_IMAGE}" fi +info "Sandbox image: ${SANDBOX_IMAGE}" oc whoami >/dev/null 2>&1 || fail "Not logged into a cluster. Run: oc login ..." info "Logged in as $(oc whoami)" @@ -500,6 +531,13 @@ else leave) warn "Leaving current Lightspeed Operator as-is" OLS_ACTION="left-as-is" + # CRD present does not imply OLSConfig exists — create one if missing. + if olsconfig_exists; then + info "OLSConfig cluster already present — skipping OLSConfig setup" + else + warn "OLSConfig cluster not found — will create one before agentic install" + CONFIGURE_OLSCONFIG=1 + fi ;; update) install_or_update_ols update "${OLS_RELATED_BUNDLE_IMAGE}"