feat(core): add generic audit transaction recorder - #3816
Conversation
Add an externally constructible audit event contract and a non-panicking recorder for injected services. Snapshot events at record time and reject writes after transaction finalization while preserving the existing audit wire shape.\n\nRefs: PEP-5181 Signed-off-by: strantalis <strantalis@virtru.com>
Serialize external events before taking the transaction lock so custom values cannot panic or reenter while the lifecycle mutex is held. Return ErrInvalidEvent for snapshot failures.\n\nRefs: PEP-5181 Signed-off-by: strantalis <strantalis@virtru.com>
Decode generic audit snapshots with json.Number so large integer metadata remains exact through finalization and emission.\n\nRefs: PEP-5181 Signed-off-by: strantalis <strantalis@virtru.com>
📝 WalkthroughWalkthroughChangesAudit event recording
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant Logger.Record
participant RecordedEvent
participant auditTransaction
participant buildRecordedLogEntry
Caller->>Logger.Record: submit verb and RecordedEvent
Logger.Record->>RecordedEvent: create event snapshot
Logger.Record->>auditTransaction: enqueue snapshot
auditTransaction->>buildRecordedLogEntry: normalize queued event during closure
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
strantalis
changed the title
strantalis
changed the title
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@service/logger/audit/recorded_event.go`:
- Around line 38-43: Change the Attributes field in RecordedObject from a value
type to a nil-able *RecordedObjectAttributes while preserving its JSON and audit
tags, so omitempty omits unset attributes instead of serializing an empty
object. Update affected construction and access sites to use the pointer
representation.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9dbb1f7f-4de5-4694-9ce0-22391b539a8e
📒 Files selected for processing (6)
service/logger/audit/context.goservice/logger/audit/enrichment.goservice/logger/audit/logger.goservice/logger/audit/recorded_event.goservice/logger/audit/recorded_event_external_test.goservice/logger/audit/recorded_event_test.go
| type RecordedObject struct { | ||
| Type string `json:"type" audit:"reserved"` | ||
| ID string `json:"id"` | ||
| Name string `json:"name,omitempty"` | ||
| Attributes RecordedObjectAttributes `json:"attributes,omitempty"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Go's encoding/json omitempty tag omit a non-pointer struct field when all its inner fields are zero values?
💡 Result:
No, the encoding/json package's omitempty tag does not omit a non-pointer struct field simply because its inner fields are zero values [1][2][3]. In Go, the omitempty tag option defines an "empty" value specifically as false, 0, a nil pointer, a nil interface value, or any array, slice, map, or string of length zero [4][5][6]. Because a struct is not included in this definition of "empty," the standard library's JSON encoder does not recursively check if a non-pointer struct's inner fields are zero values; it treats the struct itself as a non-empty value [1][2]. Consequently, an initialized non-pointer struct will be marshaled into JSON (often as an empty object {}) even if all of its exported fields would themselves be omitted [2][3]. To achieve the desired behavior of omitting the struct, you can use one of the following approaches: 1. Use a pointer to the struct: By changing the field type to a pointer (*StructType), the field becomes a nil pointer when not initialized, which the omitempty tag correctly identifies as an empty value and omits [2]. 2. Use the omitzero tag (Go 1.24+): The newer omitzero tag option provides more flexible behavior [5][7]. If the struct type implements an IsZero bool method, the encoder will use that method to determine if the struct should be considered zero and therefore omitted [8][5][7]. If no IsZero method is implemented, omitzero follows the same rules as the zero value for that type [5][7]. If you are using an older version of Go, implementing a custom MarshalJSON method or using a pointer is the standard way to handle this requirement [1][2].
Citations:
- 1: https://cyrillschumacher.com/2016/02/01/go-encoding/jsonxml-omitempty-with-non-pointer-structs/
- 2: https://stackoverflow.com/questions/18088294/how-to-not-marshal-an-empty-struct-into-json-with-go
- 3: encoding/json: rendered empty struct on omitempty golang/go#59170
- 4: https://pkg.go.dev/encoding/json
- 5: https://tip.golang.org/src/encoding/json/v2_encode.go?m=text
- 6: https://github.com/golang/go/blob/ee8972cd126f0d575be90c0ffbb08fc09d6ede19/src/encoding/json/encode.go
- 7: https://github.com/golang/go/blob/go1.24rc2/src/encoding/json/encode.go
- 8: https://pkg.go.dev/encoding/json/v2
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate file"
fd -a 'recorded_event.go' . || true
echo "## Relevant source outline"
ast-grep outline service/logger/audit/recorded_event.go --view expanded || true
echo "## Relevant source snippets"
sed -n '1,260p' service/logger/audit/recorded_event.go | nl -ba | sed -n '1,260p'
echo "## Search for RecordedObjectAttributes definitions/usages and legacy EventObject"
rg -n "type Record Object Attributes|RecordedObjectAttributes|type EventObject|Attributes|snapshotRecordedEvent|cloneRecordedEvent|Record\\(" service/logger/audit -S
echo "## Go tooling availability"
command -v go || true
go version 2>/dev/null || trueRepository: opentdf/platform
Length of output: 3038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## RecordedEvent definitions and serialization helpers"
sed -n '25,115p' service/logger/audit/recorded_event.go | awk '{printf "%4d\t%s\n", NR+24, $0}'
echo "## Mapping and legacy conversion helpers"
sed -n '107,237p' service/logger/audit/recorded_event.go | awk '{printf "%4d\t%s\n", NR+106, $0}'
echo "## Search for EventObject and legacy attributes"
rg -n "type EventObject|EventObject.*struct|Attributes|recordedObjectAttributesFromMap|recordedEventFromLegacy|snapshotRecordedEvent|json\\.Marshal|json\\.Unmarshal" service/logger/audit service/logger -S
echo "## Go availability and behavior probe"
command -v go || true
go version 2>/dev/null || true
cat > /tmp/json_omit_probe.go <<'GO'
package main
import (
"encoding/json"
"fmt"
)
type RecordedObjectAttributes struct {
Assertions []string `json:"assertions,omitempty"`
Attrs []string `json:"attrs,omitempty"`
Permissions []string `json:"permissions,omitempty"`
}
type NonPointerRecordedObject struct {
Type string `json:"type"`
Attributes RecordedObjectAttributes `json:"attributes,omitempty"`
}
type PointerRecordedObject struct {
Type string `json:"type"`
Attributes *RecordedObjectAttributes `json:"attributes,omitempty"`
}
func main() {
np, _ := json.Marshal(NonPointerRecordedObject{Type: "create"})
fmt.Println("non-pointer omitted:", string(np))
p, _ := json.Marshal(PointerRecordedObject{Type: "create"})
fmt.Println("pointer omitted:", string(p))
}
GO
go run /tmp/json_omit_probe.go 2>&1 || trueRepository: opentdf/platform
Length of output: 21968
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## service/logger/audit/utils.go EventObject definitions"
sed -n '1,105p' service/logger/audit/utils.go | awk '{printf "%4d\t%s\n", NR, $0}'
echo "## service/logger/logger.go EventObject declaration and write path"
sed -n '1,30p' service/logger/logger.go | awk '{printf "%4d\t%s\n", NR, $0}'
sed -n '160,210p' service/logger/logger.go | awk '{printf "%4d\t%s\n", NR, $0}'
echo "## Relevant recorded event JSON test expectations"
sed -n '1,140p' service/logger/audit/recorded_event_test.go | awk '{printf "%4d\t%s\n", NR, $0}'Repository: opentdf/platform
Length of output: 9252
Make attributes,omitempty omit unset attributes instead of emitting {}. The current struct value is never empty to encoding/json; a nil-able field such as *RecordedObjectAttributes is required.
🤖 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 `@service/logger/audit/recorded_event.go` around lines 38 - 43, Change the
Attributes field in RecordedObject from a value type to a nil-able
*RecordedObjectAttributes while preserving its JSON and audit tags, so omitempty
omits unset attributes instead of serializing an empty object. Update affected
construction and access sites to use the pointer representation.
1 participant
Proposed Changes
RecordedEventcontract for injected servicesLogger.Recorderrors for missing, closed, and invalid transactions/eventsAUDIT/msg/auditoutput contractThis is PR 1 of 2 for PEP-5181. PR #3817 adds finalized-event processing and startup injection on top of this recorder.
Design and compatibility
LogAuditEventretains its panic behavior for source compatibilityaudit.jwt_claim_mappingsremains supportedChecklist
Testing Instructions
cd service && go test -race ./logger/audit ./pkg/config ./pkg/server ./internal/servercd service && golangci-lint run --new-from-rev=origin/main ./logger/audit/... ./logger/... ./pkg/server/...cd sdk && go test -run TestREADMECodeBlocksgit diff --check origin/main...HEADRepository-wide
make lintis blocked locally by an invalid Buf API token. Repository-widemake testreaches environment-dependent Keycloak/Docker suites that are unavailable locally; the focused race suites above pass.Summary by CodeRabbit