feat(core): Make audit logger types extensible#3089
feat(core): Make audit logger types extensible#3089elizabethhealy wants to merge 7 commits intomainfrom
Conversation
Summary of ChangesHello @elizabethhealy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the extensibility and maintainability of the audit logger. By introducing a mechanism for dynamically registering new audit types and refactoring the event creation process with dedicated DTOs, the system becomes more flexible for future additions and provides a cleaner, more robust API for generating audit events. This change allows for easier integration and expansion of audit logging capabilities across different services. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. With types now open wide, Audit logs can grow and stride, Go's mutex guards, Against concurrent hazards, New events, with grace, now glide. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured refactoring to make the audit logger extensible. The key changes include allowing dynamic registration of audit types (ObjectType, ActionType, ActionResult) and defining a clear public DTO layer for creating audit events, which decouples consumers from internal logger implementation details. My review focuses on two main areas for improvement: enhancing concurrency by using more granular locking for the new type registries, and consistently applying the new NewEvent helper function across the codebase to improve maintainability and code clarity.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
X-Test Failure Reportopentdf |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
jakedoublev
left a comment
There was a problem hiding this comment.
These changes make sense to allow upstream registered PEPs and services registered to the platform but not present within opentdf to log to audit. These changes are an incremental improvement and do not seem too difficult to walk back as the audit story evolves.
30c4819
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
X-Test Failure Report |
|
/gemini review |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable enhancement by making the audit logger types extensible and refactoring the event creation process with DTOs. This improves flexibility and provides a cleaner API. My review focuses on a potential side-effect in one of the new option functions, opportunities for code simplification, and promoting consistency by using the new NewEvent helper function across the module. Addressing these points will further improve the robustness and maintainability of the new implementation.
|
/gemini summary |
Summary of ChangesThis pull request significantly enhances the audit logging system by introducing extensibility for audit types and refactoring the event creation process. It allows dynamic registration of new object, action, and result types, and provides a cleaner API for generating audit events through dedicated Data Transfer Objects (DTOs). This change improves flexibility for future additions and better separates internal logging mechanisms from the public API. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
📝 WalkthroughWalkthroughIntroduces a pluggable audit type registration system with thread-safe registration and sealing mechanisms, creates public DTOs for audit events, refactors event creation functions to use nested wrapper fields, and integrates type registrations into server startup flow. Changes
Sequence DiagramsequenceDiagram
participant Server as Server Startup
participant Registrar as Audit Registry
participant Events as Event Creation
Server->>Registrar: ApplyTypeRegistrations(TypeRegistrations)
Note over Registrar: Register ObjectType,<br/>ActionType, ActionResult names
Registrar-->>Server: Registration applied
Server->>Registrar: SealTypeRegistrations()
Note over Registrar: Lock registry,<br/>prevent new registrations
Registrar-->>Server: Sealed
Server->>Events: Create events (GetDecision, Policy, Rewrap)
Events->>Registrar: ObjectType.String()
Registrar-->>Events: Return registered name<br/>or fallback format
Events-->>Server: EventObject with<br/>custom type names
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/logger/audit/constants_test.go`:
- Around line 10-16: The helper resetAuditTypeRegistrationState currently only
resets typeRegistrationSealed; update it to also clear or reinitialize the
global registration maps objectTypeNames, actionTypeNames, and actionResultNames
while holding auditTypeRegistryMu to avoid races, and consider using t.Cleanup
in tests that register types to unregister specific entries; ensure
auditTypeRegistryMu.Lock() is held before mutating these maps and unlocked
afterwards so the global state is fully reset between tests.
In `@service/logger/audit/constants.go`:
- Around line 89-97: RegisterObjectType currently allows empty names which can
break logs; update RegisterObjectType to validate that the provided name is
non-empty (return a suitable error, e.g., ErrInvalidName) before acquiring or
after checking typeRegistrationSealed, and only set objectTypeNames[ot]=name
when name is non-empty; apply the same validation pattern to RegisterActionType
and RegisterActionResult so all registry functions reject empty strings and
return a clear error instead of registering an empty name.
- Around line 197-217: ApplyTypeRegistrations may partially apply registrations
if the registry is sealed mid-run or a future validation fails; before iterating
over reg.ObjectTypes/reg.ActionTypes/reg.ActionResults, check the registry
sealed state once (e.g., call the existing IsSealed or equivalent) and return an
error if sealed so no partial changes occur. In other words, at the start of
ApplyTypeRegistrations validate the registry is writable, then proceed to call
RegisterObjectType, RegisterActionType, and RegisterActionResult for each entry;
this ensures either all supplied registrations are attempted or none are applied
when the registry cannot be modified.
In `@service/pkg/server/options.go`:
- Around line 269-305: The merge in WithAdditionalAuditTypeRegistrations
silently overwrites earlier registrations (StartConfig.auditTypeRegistrations)
when keys collide; instead, detect conflicting keys (for ObjectTypes,
ActionTypes, ActionResults) and surface them rather than making order
matter—either keep the batches separate by appending the incoming registrations
into a pending bucket on StartConfig (e.g.,
StartConfig.pendingAuditTypeRegistrations / additionalAuditTypeRegistrations)
for validation during Start(), or immediately record conflicts into a new
StartConfig.auditTypeRegistrationConflicts slice (include the conflicting key
and both names) so Start() can fail with a clear error; update
WithAdditionalAuditTypeRegistrations to compare keys from
c.auditTypeRegistrations and registrations, avoid silent overwrites, and
populate the chosen pending/conflict fields accordingly.
In `@service/pkg/server/start.go`:
- Around line 130-134: The code calls audit.SealTypeRegistrations() inside
Start(), making the package-global registry permanently sealed and preventing
retries; change this by deferring sealing until after Start completes
successfully (move the audit.SealTypeRegistrations() call out of the error path
and only invoke it after all startup steps succeed), or refactor to avoid global
state by introducing a registry tied to the server instance (e.g., replace
package-global sealing with an instance method on a registry object
created/owned by Start and initialized via
audit.ApplyTypeRegistrations(startConfig.auditTypeRegistrations)), ensuring
audit.ApplyTypeRegistrations can still be called on retries before any seal is
applied.
🪄 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
Run ID: 6eb9d82d-428c-480d-bf14-9ee518b37b52
📒 Files selected for processing (12)
service/logger/audit/constants.goservice/logger/audit/constants_test.goservice/logger/audit/getDecision.goservice/logger/audit/getDecision_test.goservice/logger/audit/policy.goservice/logger/audit/policy_test.goservice/logger/audit/rewrap.goservice/logger/audit/rewrap_test.goservice/logger/audit/utils.goservice/pkg/server/options.goservice/pkg/server/options_test.goservice/pkg/server/start.go
| func resetAuditTypeRegistrationState(t *testing.T) { | ||
| t.Helper() | ||
|
|
||
| auditTypeRegistryMu.Lock() | ||
| typeRegistrationSealed = false | ||
| auditTypeRegistryMu.Unlock() | ||
| } |
There was a problem hiding this comment.
Incomplete reset: registered types persist between tests.
The resetAuditTypeRegistrationState helper only resets the typeRegistrationSealed flag but doesn't clear the custom types registered in objectTypeNames, actionTypeNames, and actionResultNames maps. This leads to:
- Test pollution: Custom types registered in earlier tests remain in global maps
- Potential false positives: A test could pass because a type was registered by a previous test
- Memory accumulation: In long test runs, orphaned registrations accumulate
Consider also resetting the maps to their initial state or using t.Cleanup to remove registered test types.
🛠️ Proposed fix with map cleanup
func resetAuditTypeRegistrationState(t *testing.T) {
t.Helper()
auditTypeRegistryMu.Lock()
+ // Remove any previously registered test types (those with IDs >= 10000)
+ for k := range objectTypeNames {
+ if k >= 10000 {
+ delete(objectTypeNames, k)
+ }
+ }
+ for k := range actionTypeNames {
+ if k >= 10000 {
+ delete(actionTypeNames, k)
+ }
+ }
+ for k := range actionResultNames {
+ if k >= 10000 {
+ delete(actionResultNames, k)
+ }
+ }
typeRegistrationSealed = false
auditTypeRegistryMu.Unlock()
}📝 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.
| func resetAuditTypeRegistrationState(t *testing.T) { | |
| t.Helper() | |
| auditTypeRegistryMu.Lock() | |
| typeRegistrationSealed = false | |
| auditTypeRegistryMu.Unlock() | |
| } | |
| func resetAuditTypeRegistrationState(t *testing.T) { | |
| t.Helper() | |
| auditTypeRegistryMu.Lock() | |
| // Remove any previously registered test types (those with IDs >= 10000) | |
| for k := range objectTypeNames { | |
| if k >= 10000 { | |
| delete(objectTypeNames, k) | |
| } | |
| } | |
| for k := range actionTypeNames { | |
| if k >= 10000 { | |
| delete(actionTypeNames, k) | |
| } | |
| } | |
| for k := range actionResultNames { | |
| if k >= 10000 { | |
| delete(actionResultNames, k) | |
| } | |
| } | |
| typeRegistrationSealed = false | |
| auditTypeRegistryMu.Unlock() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/logger/audit/constants_test.go` around lines 10 - 16, The helper
resetAuditTypeRegistrationState currently only resets typeRegistrationSealed;
update it to also clear or reinitialize the global registration maps
objectTypeNames, actionTypeNames, and actionResultNames while holding
auditTypeRegistryMu to avoid races, and consider using t.Cleanup in tests that
register types to unregister specific entries; ensure auditTypeRegistryMu.Lock()
is held before mutating these maps and unlocked afterwards so the global state
is fully reset between tests.
| func RegisterObjectType(ot ObjectType, name string) error { | ||
| auditTypeRegistryMu.Lock() | ||
| defer auditTypeRegistryMu.Unlock() | ||
| if typeRegistrationSealed { | ||
| return ErrAuditTypeRegistrationSealed | ||
| } | ||
| objectTypeNames[ot] = name | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating that name is non-empty.
The Register* functions accept any string, including empty strings. An empty name would produce empty strings in audit logs, potentially breaking log parsing or monitoring.
♻️ Optional: Add name validation
func RegisterObjectType(ot ObjectType, name string) error {
+ if name == "" {
+ return errors.New("audit type name cannot be empty")
+ }
auditTypeRegistryMu.Lock()
defer auditTypeRegistryMu.Unlock()
if typeRegistrationSealed {
return ErrAuditTypeRegistrationSealed
}
objectTypeNames[ot] = name
return nil
}Apply the same pattern to RegisterActionType and RegisterActionResult.
📝 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.
| func RegisterObjectType(ot ObjectType, name string) error { | |
| auditTypeRegistryMu.Lock() | |
| defer auditTypeRegistryMu.Unlock() | |
| if typeRegistrationSealed { | |
| return ErrAuditTypeRegistrationSealed | |
| } | |
| objectTypeNames[ot] = name | |
| return nil | |
| } | |
| func RegisterObjectType(ot ObjectType, name string) error { | |
| if name == "" { | |
| return errors.New("audit type name cannot be empty") | |
| } | |
| auditTypeRegistryMu.Lock() | |
| defer auditTypeRegistryMu.Unlock() | |
| if typeRegistrationSealed { | |
| return ErrAuditTypeRegistrationSealed | |
| } | |
| objectTypeNames[ot] = name | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/logger/audit/constants.go` around lines 89 - 97, RegisterObjectType
currently allows empty names which can break logs; update RegisterObjectType to
validate that the provided name is non-empty (return a suitable error, e.g.,
ErrInvalidName) before acquiring or after checking typeRegistrationSealed, and
only set objectTypeNames[ot]=name when name is non-empty; apply the same
validation pattern to RegisterActionType and RegisterActionResult so all
registry functions reject empty strings and return a clear error instead of
registering an empty name.
| func ApplyTypeRegistrations(reg TypeRegistrations) error { | ||
| for objectType, name := range reg.ObjectTypes { | ||
| if err := RegisterObjectType(objectType, name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| for actionType, name := range reg.ActionTypes { | ||
| if err := RegisterActionType(actionType, name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| for actionResult, name := range reg.ActionResults { | ||
| if err := RegisterActionResult(actionResult, name); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ApplyTypeRegistrations can leave registry in partially applied state on failure.
If the registry becomes sealed mid-iteration (unlikely in single-threaded startup) or if a future validation is added that can fail, some registrations will have succeeded while others didn't. The caller has no way to know which succeeded.
Given that this is called at startup before sealing (per start.go), this is currently safe. However, for robustness, consider checking sealed state once upfront before any modifications.
♻️ Optional: Check sealed state upfront
func ApplyTypeRegistrations(reg TypeRegistrations) error {
+ auditTypeRegistryMu.RLock()
+ if typeRegistrationSealed {
+ auditTypeRegistryMu.RUnlock()
+ return ErrAuditTypeRegistrationSealed
+ }
+ auditTypeRegistryMu.RUnlock()
+
for objectType, name := range reg.ObjectTypes {
if err := RegisterObjectType(objectType, name); err != nil {
return err
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/logger/audit/constants.go` around lines 197 - 217,
ApplyTypeRegistrations may partially apply registrations if the registry is
sealed mid-run or a future validation fails; before iterating over
reg.ObjectTypes/reg.ActionTypes/reg.ActionResults, check the registry sealed
state once (e.g., call the existing IsSealed or equivalent) and return an error
if sealed so no partial changes occur. In other words, at the start of
ApplyTypeRegistrations validate the registry is writable, then proceed to call
RegisterObjectType, RegisterActionType, and RegisterActionResult for each entry;
this ensures either all supplied registrations are attempted or none are applied
when the registry cannot be modified.
| func WithAdditionalAuditTypeRegistrations(registrations audit.TypeRegistrations) StartOptions { | ||
| return func(c StartConfig) StartConfig { | ||
| mergedRegistrations := audit.TypeRegistrations{} | ||
|
|
||
| if len(c.auditTypeRegistrations.ObjectTypes) > 0 || len(registrations.ObjectTypes) > 0 { | ||
| mergedRegistrations.ObjectTypes = make(map[audit.ObjectType]string) | ||
| for objectType, name := range c.auditTypeRegistrations.ObjectTypes { | ||
| mergedRegistrations.ObjectTypes[objectType] = name | ||
| } | ||
| for objectType, name := range registrations.ObjectTypes { | ||
| mergedRegistrations.ObjectTypes[objectType] = name | ||
| } | ||
| } | ||
|
|
||
| if len(c.auditTypeRegistrations.ActionTypes) > 0 || len(registrations.ActionTypes) > 0 { | ||
| mergedRegistrations.ActionTypes = make(map[audit.ActionType]string) | ||
| for actionType, name := range c.auditTypeRegistrations.ActionTypes { | ||
| mergedRegistrations.ActionTypes[actionType] = name | ||
| } | ||
| for actionType, name := range registrations.ActionTypes { | ||
| mergedRegistrations.ActionTypes[actionType] = name | ||
| } | ||
| } | ||
|
|
||
| if len(c.auditTypeRegistrations.ActionResults) > 0 || len(registrations.ActionResults) > 0 { | ||
| mergedRegistrations.ActionResults = make(map[audit.ActionResult]string) | ||
| for actionResult, name := range c.auditTypeRegistrations.ActionResults { | ||
| mergedRegistrations.ActionResults[actionResult] = name | ||
| } | ||
| for actionResult, name := range registrations.ActionResults { | ||
| mergedRegistrations.ActionResults[actionResult] = name | ||
| } | ||
| } | ||
|
|
||
| c.auditTypeRegistrations = mergedRegistrations | ||
| return c | ||
| } |
There was a problem hiding this comment.
Surface registration collisions instead of making them order-dependent.
If two extensions provide the same ObjectType/ActionType/ActionResult, this merge silently keeps the later name and discards the earlier one. That makes the emitted audit taxonomy depend on option order instead of surfacing a duplicate-registration error; keep the batches separate until Start() or explicitly detect conflicting keys here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/pkg/server/options.go` around lines 269 - 305, The merge in
WithAdditionalAuditTypeRegistrations silently overwrites earlier registrations
(StartConfig.auditTypeRegistrations) when keys collide; instead, detect
conflicting keys (for ObjectTypes, ActionTypes, ActionResults) and surface them
rather than making order matter—either keep the batches separate by appending
the incoming registrations into a pending bucket on StartConfig (e.g.,
StartConfig.pendingAuditTypeRegistrations / additionalAuditTypeRegistrations)
for validation during Start(), or immediately record conflicts into a new
StartConfig.auditTypeRegistrationConflicts slice (include the conflicting key
and both names) so Start() can fail with a clear error; update
WithAdditionalAuditTypeRegistrations to compare keys from
c.auditTypeRegistrations and registrations, avoid silent overwrites, and
populate the chosen pending/conflict fields accordingly.
| if err := audit.ApplyTypeRegistrations(startConfig.auditTypeRegistrations); err != nil { | ||
| return fmt.Errorf("failed applying additional audit type registrations: %w", err) | ||
| } | ||
| audit.SealTypeRegistrations() | ||
| logger.Debug("sealed audit type registrations") |
There was a problem hiding this comment.
Sealing the registry here makes Start non-retryable.
SealTypeRegistrations flips package-global state with no production unseal path in service/logger/audit/constants.go. Once Line 133 runs, any later Start() in the same process cannot apply additional registrations, and any failure later in startup still leaves the process permanently sealed. Seal only after startup succeeds, or tie the registry lifetime to the server instance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/pkg/server/start.go` around lines 130 - 134, The code calls
audit.SealTypeRegistrations() inside Start(), making the package-global registry
permanently sealed and preventing retries; change this by deferring sealing
until after Start completes successfully (move the audit.SealTypeRegistrations()
call out of the error path and only invoke it after all startup steps succeed),
or refactor to avoid global state by introducing a registry tied to the server
instance (e.g., replace package-global sealing with an instance method on a
registry object created/owned by Start and initialized via
audit.ApplyTypeRegistrations(startConfig.auditTypeRegistrations)), ensuring
audit.ApplyTypeRegistrations can still be called on retries before any seal is
applied.
Proposed Changes
This pull request enhances the audit logging system by introducing extensibility for audit types and refactoring the event creation process. It allows dynamic registration of new object, action, and result types, and provides a cleaner API for generating audit events through dedicated Data Transfer Objects (DTOs). This change improves flexibility for future additions and better separates internal logging mechanisms from the public API.
Highlights
Checklist
Testing Instructions
Summary by CodeRabbit
Release Notes
New Features
Tests