Repository navigation
feat(kdr): add actor fields to RuntimeIncidentIngesterOnFinishedMessage - #113
Conversation
Carry the Kubernetes user (name and groups) of admission incidents such as exec to pod, so notification channels (Slack, Teams, etc.) can show who triggered the incident. Signed-off-by: Ben <ben@armosec.io>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
|
Summary:
|
rotemamsa
left a comment
There was a problem hiding this comment.
LGTM. Additive omitempty fields, safe in any deploy order.
matthyx
left a comment
There was a problem hiding this comment.
Approve: this is a needed, narrowly scoped schema prerequisite for admission-actor notifications. No confirmed blockers (critical/high/medium/low: 0); independent code review recommends approval and architecture review is CLEAR.
Reviewed head 915c397d8b637db5c69098bbaf8554ba6859dfbf against main at 5f9ba1405db4bc451cc5750b285b0c498c379cf1. In pulsar/common/kdr/datastructures.go:26–29, the optional string and string-slice fields match the stated contract. ProduceMessage uses json.Marshal (pulsar/connector/producer.go:144), so the addition directly enables actor serialization without changing absent-value payloads. No dependency, execution-path, logging, concurrency, or resource-lifecycle change was introduced. Standard Go JSON consumers tolerate the added members; strict external decoders were not inspected.
History: scanned all 112 PR records returned by the all-state list (limit 200) and the repository's one returned issue (limit 100), plus indexed searches for UserGroups, admission, RuntimeIncidentIngesterOnFinishedMessage, actor, username, and notification terms. No actor-field duplicate or superseding fix found. #74 added policy names and #102 added optional classification to this same DTO; both merged and remain complementary. Closed #73 proposed full policy objects plus an infrastructure dependency; its discussions contain no explicit rejection, and #74 subsequently used a smaller map. Replacement is an inference, not a recorded maintainer decision. Closed #40 concerned a runtime command, with a maintainer asking whether it was still needed; that does not reject actor metadata. Search scope is this repository's visible records and indexed text, not private downstream history or a guarantee of exhaustiveness.
Validation: in a credential-free, restricted Go 1.25 container, go test ./pulsar/common/kdr and go vet ./pulsar/common/kdr passed. A temporary review-only TestReviewActorRoundTrip failed on the target branch with “actor lost” and passed on this head; it checks populated username/groups retention and omission for absent, empty, and null values. git diff --check and formatting inspection passed. CI run 37606179770 belongs to this exact head and reports successful build, integration tests, unit tests, lint, and CodeQL. The Basic-Test credentials/vulnerability scan steps were skipped. Full-suite and integration checks were not rerun locally; no test was added to the source branch.
Limits: the customer request and upstream AdmissionAlert.UserInfo flow are described by the author, not independently reproduced end-to-end. Producer population, matching armoapi-go fields, notification rendering, and external consumer compatibility remain follow-up validation; this schema PR alone does not deliver visible actor notifications. Semantic reference analysis had incomplete dependency resolution and does not establish exhaustive downstream coverage. These limits do not block this additive contract change.
Existing review and comments were read and rechecked; no inline threads or unresolved concerns were found. The PR remains open, non-draft, mergeable, and clean at the unchanged head/base. Current CI is green and the latest head already has a maintainer approval; the visible main-branch rules require approval and last-push approval. Ready to merge as the schema prerequisite. No merge performed.
Summary
Adds optional
UserNameandUserGroupsfields toRuntimeIncidentIngesterOnFinishedMessage.Admission incidents (e.g. R2000 exec to pod) already record the requesting Kubernetes user in
AdmissionAlert.UserInfo, but the onFinish message that drives runtime incident notifications drops it. With these fields, event-ingester-service can pass the actor through and users-notification-service can show it in Slack and the other alert channels (requested by a customer).Notes
omitempty, so existing producers and consumers are unaffected.notifications.NewRuntimeIncident(same JSON tags), then the backend producer and notification template changes.AI-skills: armosec-shared-rules:writing-docs | cmds: /model