WIP: OLS-3273: MutatingAdmissionWebhook for ProposalApproval identity injection#201
WIP: OLS-3273: MutatingAdmissionWebhook for ProposalApproval identity injection#201vimalk78 wants to merge 2 commits into
Conversation
Deny paths (Analysis, Execution, Verification, Escalation) called denyProposal without first emitting audit.approval.received or ending the approval wait span, leaving denied proposals invisible in traces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@vimalk78: This pull request references OLS-3273 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a mutating admission webhook ( ChangesProposalApproval Mutating Webhook and Audit Identity Propagation
Sequence DiagramsequenceDiagram
participant User as User/kubectl
participant APIServer as Kubernetes API Server
participant Mutator as ProposalApprovalMutator
participant Reconciler as ProposalReconciler
participant Audit as ProductionAuditLogger
User->>APIServer: UPDATE ProposalApproval
APIServer->>Mutator: AdmissionReview (UPDATE)
Mutator->>Mutator: Build ApproverInfo from UserInfo + UTC timestamp
Mutator->>Mutator: Check spec.approver presence
Mutator-->>APIServer: admission.Patched with /spec/approver (add or replace)
APIServer-->>User: 200 OK (patched object stored)
APIServer->>Reconciler: Reconcile triggered
Reconciler->>Reconciler: Load ProposalApproval, evaluate stage decision
alt Stage Approved
Reconciler->>Audit: EndApprovalWait(proposal, approval)
Audit->>Audit: Set span attrs approver.uid, approver.username, approver.approvedAt
Audit->>Audit: Set approval.decision from last stage
Reconciler->>Audit: EmitApprovalReceived(proposal, approval)
Audit->>Audit: Emit payload with ApproverInfo fields
else Stage Denied
Reconciler->>Audit: EndApprovalWait(proposal, approval)
Reconciler->>Audit: EmitApprovalReceived(proposal, approval)
Reconciler->>Reconciler: Deny proposal
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)cmd/main.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.17][ERROR]: unable to find a config; path controller/proposal/approval_webhook.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path api/v1alpha1/proposalapproval_types.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.29][ERROR]: unable to find a config; path
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
vimalk78
force-pushed
the
ols-3273-approval-webhook
branch
from
34f93df to
1bfef28
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
controller/proposal/approval_webhook_test.go (1)
26-254: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for UPDATE requests with missing or
nullspec.Current tests always include
Spec, so they won’t catch invalid child-path patching when/specis missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/proposal/approval_webhook_test.go` around lines 26 - 254, Add a new test function (similar to the existing tests like TestApprovalWebhook_InjectsApproverOnUpdate) that creates a ProposalApproval object with either a missing or null Spec field, sends it through the ProposalApprovalMutator.Handle() method with an UPDATE operation, and verifies that the mutator handles this edge case gracefully without attempting to patch child paths of a non-existent spec. This regression test will catch issues where the code tries to patch `/spec/approver` when the spec field itself doesn't exist.controller/proposal/audit_test.go (1)
1513-1513: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a focused test for non-nil
approvalinEndApprovalWait.These updates only validate the nil path. Add one test that passes an approval containing
spec.approverand stage decisions, then assert span attributes includeapprover.uid,approver.username,approver.timestamp, andapproval.decision.Diff sketch
+func TestEndApprovalWait_SetsApproverAndDecisionAttributes(t *testing.T) { + sr := tracetest.NewSpanRecorder() + tp := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr), sdktrace.WithIDGenerator(&ProposalIDGenerator{})) + otel.SetTracerProvider(tp) + defer otel.SetTracerProvider(sdktrace.NewTracerProvider()) + + auditLogger := NewProductionAuditLogger(zap.NewNop()).(*ProductionAuditLogger) + proposal := &agenticv1alpha1.Proposal{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "test-ns", + UID: types.UID("a1b2c3d4-e5f6-7890-1234-567890abcdef"), + }, + } + approval := &agenticv1alpha1.ProposalApproval{ + Spec: agenticv1alpha1.ProposalApprovalSpec{ + Approver: &agenticv1alpha1.ApproverInfo{ + UID: "u-1", Username: "alice", Timestamp: "2026-06-22T10:00:00Z", + }, + Stages: []agenticv1alpha1.ApprovalStage{ + {Type: agenticv1alpha1.ApprovalStageAnalysis, Decision: agenticv1alpha1.ApprovalDecisionApproved}, + }, + }, + } + + auditLogger.StartApprovalWait(context.Background(), proposal) + auditLogger.EndApprovalWait(proposal, approval) + + // Assert proposal.human_approval span contains approver.* + approval.decision attrs. +}Also applies to: 1561-1561, 1603-1603
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/proposal/audit_test.go` at line 1513, The current tests for EndApprovalWait in the audit_test.go file only test the nil approval path. Add a new test case that constructs a non-nil approval object with populated spec.approver details and stage decisions, then calls EndApprovalWait with this approval instead of nil. After the call, assert that the resulting span attributes contain the expected fields: approver.uid, approver.username, approver.timestamp, and approval.decision to validate that approval metadata is properly captured in the audit logs when an actual approval is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/default/kustomization.yaml`:
- Line 9: The kustomization.yaml file includes the webhook configuration but is
missing the required NetworkPolicy resource to restrict ingress to the webhook
service. Create a NetworkPolicy resource that restricts ingress to the
agentic-operator-webhook-service on ports 443 and 9443, ensuring only authorized
traffic can reach the webhook endpoint. Add this NetworkPolicy manifest to the
configuration stack and reference it in the kustomization.yaml file alongside
the webhook include to ensure network security requirements are met.
In `@config/manager/manager.yaml`:
- Around line 82-86: The webhook-certs secret volume mount in the manager
deployment is configured with `optional: true`, which allows the pod to start
without the certificate secret, causing webhook TLS initialization failures.
Change the `optional: true` field to `optional: false` in the secret volume
specification for webhook-certs to ensure the secret must exist for the pod to
start, preventing admission failures due to missing certificates.
In `@config/webhook/manifests.yaml`:
- Around line 15-19: The webhook rule defined in the
ValidatingWebhookConfiguration for proposalapprovals resources is currently
matching across all namespaces. Since the controller is namespaced and
failurePolicy is set to Fail, this creates unnecessary cluster-wide admission
coupling when the webhook becomes unavailable. Add a namespaceSelector field to
the webhook rule to restrict it to only the operator namespace where the
controller is running. This will ensure the webhook only affects resources in
the specific namespace rather than creating a cluster-wide dependency.
In `@controller/proposal/approval_webhook.go`:
- Around line 44-58: Before attempting to add the `/spec/approver` patch
operation in the switch case, verify that the spec field actually exists in the
object. If the spec field is absent (when the `ok` check on line 45 fails), you
cannot patch `/spec/approver` because the parent path `/spec` does not exist.
Add a check for the missing spec case and either: create the spec first with an
appropriate patch operation before adding the approver, or handle this scenario
separately to ensure the JSON Patch operations target valid parent paths.
In `@hack/quickstart/install.sh`:
- Around line 255-276: The MutatingWebhookConfiguration for
agentic-operator-mutating-webhook with failurePolicy set to Fail is being
applied before the webhook backend and certificates are ready, creating a window
where ProposalApproval UPDATE requests can be rejected. Reorder the install
sequence to first apply the agentic-operator-webhook-service, perform the
readiness wait check, and only then apply the MutatingWebhookConfiguration
resource. This ensures the webhook backend is fully initialized and responding
before the fail-closed policy takes effect.
---
Nitpick comments:
In `@controller/proposal/approval_webhook_test.go`:
- Around line 26-254: Add a new test function (similar to the existing tests
like TestApprovalWebhook_InjectsApproverOnUpdate) that creates a
ProposalApproval object with either a missing or null Spec field, sends it
through the ProposalApprovalMutator.Handle() method with an UPDATE operation,
and verifies that the mutator handles this edge case gracefully without
attempting to patch child paths of a non-existent spec. This regression test
will catch issues where the code tries to patch `/spec/approver` when the spec
field itself doesn't exist.
In `@controller/proposal/audit_test.go`:
- Line 1513: The current tests for EndApprovalWait in the audit_test.go file
only test the nil approval path. Add a new test case that constructs a non-nil
approval object with populated spec.approver details and stage decisions, then
calls EndApprovalWait with this approval instead of nil. After the call, assert
that the resulting span attributes contain the expected fields: approver.uid,
approver.username, approver.timestamp, and approval.decision to validate that
approval metadata is properly captured in the audit logs when an actual approval
is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6923565b-0377-4ba8-9526-4aba2d95a389
⛔ Files ignored due to path filters (2)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.deepcopy.goconfig/crd/bases/agentic.openshift.io_proposalapprovals.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (18)
api/v1alpha1/proposalapproval_types.gocmd/main.goconfig/default/kustomization.yamlconfig/manager/manager.yamlconfig/webhook/kustomization.yamlconfig/webhook/manifests.yamlconfig/webhook/service.yamlcontroller/proposal/approval_webhook.gocontroller/proposal/approval_webhook_test.gocontroller/proposal/audit.gocontroller/proposal/audit_test.gocontroller/proposal/handlers.gocontroller/proposal/reconciler.gocontroller/setup.gogo.modhack/deploy-jaeger.shhack/quickstart/install.shhack/quickstart/uninstall.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| var spec map[string]json.RawMessage | ||
| if specRaw, ok := obj["spec"]; ok { | ||
| if err := json.Unmarshal(specRaw, &spec); err != nil { | ||
| return admission.Errored(http.StatusBadRequest, err) | ||
| } | ||
| } | ||
|
|
||
| var patches []jsonpatch.JsonPatchOperation | ||
| _, hasApprover := spec["approver"] | ||
|
|
||
| switch { | ||
| case !hasApprover: | ||
| patches = []jsonpatch.JsonPatchOperation{ | ||
| {Operation: "add", Path: "/spec/approver", Value: &rawApprover}, | ||
| } |
There was a problem hiding this comment.
Handle missing spec before patching /spec/approver.
If spec is absent (or null), Line 57 emits add /spec/approver with a non-existent parent path. JSON Patch application then fails and the UPDATE gets rejected.
Suggested fix
@@
- var spec map[string]json.RawMessage
- if specRaw, ok := obj["spec"]; ok {
+ var spec map[string]json.RawMessage
+ hasSpec := false
+ if specRaw, ok := obj["spec"]; ok && string(specRaw) != "null" {
+ hasSpec = true
if err := json.Unmarshal(specRaw, &spec); err != nil {
return admission.Errored(http.StatusBadRequest, err)
}
}
var patches []jsonpatch.JsonPatchOperation
- _, hasApprover := spec["approver"]
+ if !hasSpec {
+ patches = []jsonpatch.JsonPatchOperation{
+ {Operation: "add", Path: "/spec", Value: map[string]json.RawMessage{"approver": rawApprover}},
+ }
+ return admission.Patched("injected spec.approver", patches...)
+ }
+ _, hasApprover := spec["approver"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var spec map[string]json.RawMessage | |
| if specRaw, ok := obj["spec"]; ok { | |
| if err := json.Unmarshal(specRaw, &spec); err != nil { | |
| return admission.Errored(http.StatusBadRequest, err) | |
| } | |
| } | |
| var patches []jsonpatch.JsonPatchOperation | |
| _, hasApprover := spec["approver"] | |
| switch { | |
| case !hasApprover: | |
| patches = []jsonpatch.JsonPatchOperation{ | |
| {Operation: "add", Path: "/spec/approver", Value: &rawApprover}, | |
| } | |
| var spec map[string]json.RawMessage | |
| hasSpec := false | |
| if specRaw, ok := obj["spec"]; ok && string(specRaw) != "null" { | |
| hasSpec = true | |
| if err := json.Unmarshal(specRaw, &spec); err != nil { | |
| return admission.Errored(http.StatusBadRequest, err) | |
| } | |
| } | |
| var patches []jsonpatch.JsonPatchOperation | |
| if !hasSpec { | |
| patches = []jsonpatch.JsonPatchOperation{ | |
| {Operation: "add", Path: "/spec", Value: map[string]json.RawMessage{"approver": rawApprover}}, | |
| } | |
| return admission.Patched("injected spec.approver", patches...) | |
| } | |
| _, hasApprover := spec["approver"] | |
| switch { | |
| case !hasApprover: | |
| patches = []jsonpatch.JsonPatchOperation{ | |
| {Operation: "add", Path: "/spec/approver", Value: &rawApprover}, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/proposal/approval_webhook.go` around lines 44 - 58, Before
attempting to add the `/spec/approver` patch operation in the switch case,
verify that the spec field actually exists in the object. If the spec field is
absent (when the `ok` check on line 45 fails), you cannot patch `/spec/approver`
because the parent path `/spec` does not exist. Add a check for the missing spec
case and either: create the spec first with an appropriate patch operation
before adding the approver, or handle this scenario separately to ensure the
JSON Patch operations target valid parent paths.
There was a problem hiding this comment.
please review again, on the new pushed code
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat controller/proposal/approval_webhook.goLength of output: 2066
🏁 Script executed:
#!/bin/bash
# Fetch the current file content via GitHub API
gh api repos/openshift/lightspeed-agentic-operator/contents/controller/proposal/approval_webhook.go \
--jq '.content' | base64 -dLength of output: 575
🏁 Script executed:
#!/bin/bash
# Get PR `#201` head branch
PR_INFO=$(gh api repos/openshift/lightspeed-agentic-operator/pulls/201 --jq '{head_sha: .head.sha, head_ref: .head.ref}')
echo "$PR_INFO"
HEAD_SHA=$(echo "$PR_INFO" | jq -r '.head_sha')
# Fetch file at the PR head commit
gh api "repos/openshift/lightspeed-agentic-operator/contents/controller/proposal/approval_webhook.go?ref=${HEAD_SHA}" \
--jq '.content' | base64 -dLength of output: 2515
@vimalk78 — the code on the current HEAD (f292ce5) is identical to the original diff. The concern is still unaddressed.
The exact problem: if obj["spec"] is absent or spec is null, the spec map stays nil. The map lookup _, hasApprover := spec["approver"] returns false on a nil map (no panic, but silently falls through). The switch then emits add /spec/approver — but JSON Patch RFC 6902 requires the parent path /spec to exist for an add operation, so the patch application fails and the admission webhook rejects the UPDATE request.
For ProposalApproval CRDs the spec will almost always be present in an UPDATE, but an explicit spec: null via strategic merge patch is a realistic edge case that would trigger this.
The fix from the earlier suggestion still applies:
var spec map[string]json.RawMessage
hasSpec := false
if specRaw, ok := obj["spec"]; ok && string(specRaw) != "null" {
hasSpec = true
if err := json.Unmarshal(specRaw, &spec); err != nil {
return admission.Errored(http.StatusBadRequest, err)
}
}
var patches []jsonpatch.JsonPatchOperation
if !hasSpec {
patches = []jsonpatch.JsonPatchOperation{
{Operation: "add", Path: "/spec", Value: map[string]json.RawMessage{"approver": rawApprover}},
}
return admission.Patched("injected spec.approver", patches...)
}
_, hasApprover := spec["approver"]
switch {
case !hasApprover:
patches = []jsonpatch.JsonPatchOperation{
{Operation: "add", Path: "/spec/approver", Value: &rawApprover},
}
// ...🧠 Learnings used
Learnt from: sakshipatels98-byte
Repo: openshift/lightspeed-agentic-operator PR: 17
File: controller/proposal/sandbox_templates.go:197-205
Timestamp: 2026-06-09T17:19:11.943Z
Learning: In this repository’s Go code, error wrapping may use inline `fmt.Errorf("...: %w", err)` (with literal message strings) without requiring the project’s non-use of a `const ErrFoo = "..."` + `fmt.Errorf("%s: %w", ErrFoo, err)` style. During code reviews, do not flag inline `fmt.Errorf` message strings as violations of any const-error-message guideline for this repo.
Learnt from: onmete
Repo: openshift/lightspeed-agentic-operator PR: 116
File: cli/system/helpers.go:18-23
Timestamp: 2026-06-15T07:37:06.162Z
Learning: In this repo, when registering Kubernetes schemes via `SomeScheme.AddToScheme(s)` (e.g., `_ = Scheme.AddToScheme(s)` to intentionally discard the returned error), treat the blank-identifier error discard as an established convention. Code reviewers should NOT flag these `_ = <scheme>.AddToScheme(<schemeBuilder>)` / `_ = <scheme>.AddToScheme(s)` usages as violations of the “never ignore error returns” guideline. Only apply this exception to the scheme-registration `AddToScheme` pattern; ignore/flag other blank identifier error discards as usual.
1bfef28 to
9e5122f
Compare
vimalk78
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
vimalk78
force-pushed
the
ols-3273-approval-webhook
branch
from
9e5122f to
a4c879e
Compare
Intercepts UPDATE operations on ProposalApproval CRs to inject the authenticated user's identity (uid, username, approvedAt) from the AdmissionReview userInfo into spec.approver, overwriting any client-submitted values. Fail-closed webhook using OpenShift service-serving-certs for TLS, scoped to the operator namespace. The webhook only mutates — audit logging is handled by the controller's EmitApprovalReceived (JSON log + OTEL span event). EndApprovalWait now sets approver attributes and approval.decision on the human_approval span before ending it. Also adds NetworkPolicy restricting ingress to the webhook port, and reorders install.sh to register the MutatingWebhookConfiguration only after the operator is ready. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
vimalk78
force-pushed
the
ols-3273-approval-webhook
branch
from
a4c879e to
f292ce5
Compare
|
@vimalk78: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/proposalapproval_types.go`:
- Around line 258-261: The YAML documentation example for the approver section
in the ProposalApproval types shows an incorrect field name. Change the field
name from `timestamp` to `approvedAt` in the example comment (around the
approver section showing uid, username, and the timestamp field) to match the
actual JSON field name defined in the struct at line 45. This ensures users
copying the example will use the correct field name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bebf849c-f03c-4f77-8fde-baf580edf72e
⛔ Files ignored due to path filters (2)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.deepcopy.goconfig/crd/bases/agentic.openshift.io_proposalapprovals.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (19)
api/v1alpha1/proposalapproval_types.gocmd/main.goconfig/default/kustomization.yamlconfig/manager/manager.yamlconfig/webhook/kustomization.yamlconfig/webhook/manifests.yamlconfig/webhook/networkpolicy.yamlconfig/webhook/service.yamlcontroller/proposal/approval_webhook.gocontroller/proposal/approval_webhook_test.gocontroller/proposal/audit.gocontroller/proposal/audit_test.gocontroller/proposal/handlers.gocontroller/proposal/reconciler.gocontroller/setup.gogo.modhack/deploy-jaeger.shhack/quickstart/install.shhack/quickstart/uninstall.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
✅ Files skipped from review due to trivial changes (5)
- hack/deploy-jaeger.sh
- config/webhook/kustomization.yaml
- config/manager/manager.yaml
- go.mod
- controller/setup.go
🚧 Files skipped from review as they are similar to previous changes (11)
- config/webhook/manifests.yaml
- config/default/kustomization.yaml
- controller/proposal/reconciler.go
- cmd/main.go
- config/webhook/service.yaml
- controller/proposal/audit_test.go
- controller/proposal/audit.go
- hack/quickstart/uninstall.sh
- controller/proposal/handlers.go
- controller/proposal/approval_webhook.go
- controller/proposal/approval_webhook_test.go
| // approver: | ||
| // uid: "abc123" | ||
| // username: "admin@example.com" | ||
| // timestamp: "2026-06-22T10:00:00Z" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Example uses wrong key timestamp. The field's JSON name is approvedAt (Line 45), but the doc example renders it as timestamp. Users copying this example will produce an unrecognized field.
Proposed fix
// approver:
// uid: "abc123"
// username: "admin@example.com"
-// timestamp: "2026-06-22T10:00:00Z"
+// approvedAt: "2026-06-22T10:00:00Z"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // approver: | |
| // uid: "abc123" | |
| // username: "admin@example.com" | |
| // timestamp: "2026-06-22T10:00:00Z" | |
| // approver: | |
| // uid: "abc123" | |
| // username: "admin@example.com" | |
| // approvedAt: "2026-06-22T10:00:00Z" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1alpha1/proposalapproval_types.go` around lines 258 - 261, The YAML
documentation example for the approver section in the ProposalApproval types
shows an incorrect field name. Change the field name from `timestamp` to
`approvedAt` in the example comment (around the approver section showing uid,
username, and the timestamp field) to match the actual JSON field name defined
in the struct at line 45. This ensures users copying the example will use the
correct field name.
There was a problem hiding this comment.
please check previous comments
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh pr view 201 --repo openshift/lightspeed-agentic-operator --comments | head -200Length of output: 2105
🏁 Script executed:
#!/bin/bash
gh api repos/openshift/lightspeed-agentic-operator/pulls/201/comments \
--jq '.[].body' | head -300Length of output: 9689
🏁 Script executed:
#!/bin/bash
gh api repos/openshift/lightspeed-agentic-operator/pulls/201/comments \
--jq '.[].body' | tail -300Length of output: 10175
🏁 Script executed:
#!/bin/bash
# Get the PR head branch and check the current file content
PR_DATA=$(gh api repos/openshift/lightspeed-agentic-operator/pulls/201 --jq '{head_sha: .head.sha, head_ref: .head.ref}')
echo "PR data: $PR_DATA"
HEAD_SHA=$(echo "$PR_DATA" | jq -r '.head_sha')
echo "Head SHA: $HEAD_SHA"
# Get the current content of proposalapproval_types.go
gh api "repos/openshift/lightspeed-agentic-operator/contents/api/v1alpha1/proposalapproval_types.go?ref=$HEAD_SHA" \
--jq '.content' | base64 -d | grep -n -A5 -B5 'timestamp\|approvedAt\|approver:' | head -60Length of output: 1849
🏁 Script executed:
#!/bin/bash
HEAD_SHA="f292ce5c4402931caaf6ed8c60b046c288df707e"
echo "=== manager.yaml - optional cert ==="
gh api "repos/openshift/lightspeed-agentic-operator/contents/config/manager/manager.yaml?ref=$HEAD_SHA" \
--jq '.content' | base64 -d | grep -n -A2 -B2 'optional'
echo ""
echo "=== approval_webhook.go - missing spec handling ==="
gh api "repos/openshift/lightspeed-agentic-operator/contents/controller/proposal/approval_webhook.go?ref=$HEAD_SHA" \
--jq '.content' | base64 -d | cat -n
echo ""
echo "=== NetworkPolicy in webhook config ==="
gh api "repos/openshift/lightspeed-agentic-operator/contents/config/webhook/kustomization.yaml?ref=$HEAD_SHA" \
--jq '.content' | base64 -d
echo ""
echo "=== webhook manifests.yaml - namespaceSelector ==="
gh api "repos/openshift/lightspeed-agentic-operator/contents/config/webhook/manifests.yaml?ref=$HEAD_SHA" \
--jq '.content' | base64 -dLength of output: 4571
@vimalk78, here's the status after reviewing the latest commit (f292ce5):
✅ Fixed
namespaceSelectorscoped to operator namespace —manifests.yamlnow has the correctnamespaceSelector.NetworkPolicy—networkpolicy.yamlis now included inconfig/webhook/kustomization.yaml.
❌ Still open (3 issues)
1. timestamp → approvedAt in the doc example (api/v1alpha1/proposalapproval_types.go line 261)
The example comment still says timestamp but the JSON field is approvedAt.
-// timestamp: "2026-06-22T10:00:00Z"
+// approvedAt: "2026-06-22T10:00:00Z"2. optional: true on webhook cert secret (config/manager/manager.yaml line 86)
The pod can still start without the cert, causing TLS failures at admission time with failurePolicy: Fail. Remove optional: true (or set it to false).
3. Missing spec guard in approval_webhook.go (lines 44–57)
When obj["spec"] is absent, spec stays nil, hasApprover is false, and the patch emits add /spec/approver — but JSON Patch requires the parent path /spec to exist first. The update will be rejected. Need to handle the no-spec case:
var spec map[string]json.RawMessage
hasSpec := false
if specRaw, ok := obj["spec"]; ok && string(specRaw) != "null" {
hasSpec = true
if err := json.Unmarshal(specRaw, &spec); err != nil {
return admission.Errored(http.StatusBadRequest, err)
}
}
var patches []jsonpatch.JsonPatchOperation
if !hasSpec {
patches = []jsonpatch.JsonPatchOperation{
{Operation: "add", Path: "/spec", Value: map[string]json.RawMessage{"approver": rawApprover}},
}
return admission.Patched("injected spec.approver", patches...)
}
_, hasApprover := spec["approver"]
// ... rest unchanged🧠 Learnings used
Learnt from: sakshipatels98-byte
Repo: openshift/lightspeed-agentic-operator PR: 17
File: controller/proposal/sandbox_templates.go:197-205
Timestamp: 2026-06-09T17:19:11.943Z
Learning: In this repository’s Go code, error wrapping may use inline `fmt.Errorf("...: %w", err)` (with literal message strings) without requiring the project’s non-use of a `const ErrFoo = "..."` + `fmt.Errorf("%s: %w", ErrFoo, err)` style. During code reviews, do not flag inline `fmt.Errorf` message strings as violations of any const-error-message guideline for this repo.
Learnt from: onmete
Repo: openshift/lightspeed-agentic-operator PR: 116
File: cli/system/helpers.go:18-23
Timestamp: 2026-06-15T07:37:06.162Z
Learning: In this repo, when registering Kubernetes schemes via `SomeScheme.AddToScheme(s)` (e.g., `_ = Scheme.AddToScheme(s)` to intentionally discard the returned error), treat the blank-identifier error discard as an established convention. Code reviewers should NOT flag these `_ = <scheme>.AddToScheme(<schemeBuilder>)` / `_ = <scheme>.AddToScheme(s)` usages as violations of the “never ignore error returns” guideline. Only apply this exception to the scheme-registration `AddToScheme` pattern; ignore/flag other blank identifier error discards as usual.
|
@CodeRabbit please release the lock |
|
✅ Action performedReviews resumed. |
2 participants
Summary
Adds a
MutatingAdmissionWebhookthat intercepts UPDATE operations onProposalApprovalCRs and injects the authenticated user's identity intospec.approver. This closes the gap where audit logs and OTEL traces showed empty approver fields — the API server now stamps every approval/denial with whodid it.
Also includes a bug fix for missing audit events on deny paths (OLS-3024), discovered during end-to-end testing.
Commits
Emit audit events on deny paths before rejecting proposals — All four deny paths (Analysis, Execution, Verification, Escalation) called
denyProposalwithout emitting
audit.approval.receivedor ending the approval wait span. Denied proposals were invisible in traces.Add mutating webhook for ProposalApproval identity injection — The webhook, CRD changes, audit integration, install/uninstall scripts, and
approval.decisionattribute on thehuman_approvalOTEL span.What changed
spec.approver(uid,username,timestamp) toProposalApprovalProposalApprovalMutatorinjectsspec.approvervia JSON patch on every UPDATE; CREATE passes throughEndApprovalWaitnow records approver identity andapproval.decision(Approved/Denied) on thehuman_approvalspan;EmitApprovalReceivedreads from
spec.approverinstead of hardcoded empty stringsaudit.approval.receivedand end the approval wait span before callingdenyProposalagentic-operator-webhook-certssecretMutatingWebhookConfiguration(step 5/6);GITHUB_RAWis overridable for fork-based testing;IMAGE_PULL_POLICYsupportMutatingWebhookConfigurationbefore the operator deployment to avoid fail-closed dead-endpoint windowoc new-projectswitching active namespace (--skip-config-write)How it works