-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for audit.v3 logging #400
base: nmiyake/refactor-audit-logging-commit-1
Are you sure you want to change the base?
Add support for audit.v3 logging #400
Conversation
Does the following: * Adds the audit3log package with functions to create and log audit.3 logs and support for dual-writing audit.2 logs * Adds dual-writing capability to audit.2 logs so that logging audit.2 logs will dual-write an equivalent audit.3 log entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Thanks for putting it together. Approving because my question is nonblocking and if you decide to make the proposed change I think it can merge as is.
Reason string `json:"reason"` | ||
} | ||
|
||
type Audit3ContextualizedUser struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want omitempty
on the json struct tags to avoid a bunch of empty fields?
StackKey = auditloginternal.Audit3StackKey | ||
ServiceKey = auditloginternal.Audit3ServiceKey | ||
EnvironmentKey = auditloginternal.Audit3EnvironmentKey | ||
ProducerTypeKey = auditloginternal.Audit3ProducerTypeKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably always be SERVER
EnvironmentKey = auditloginternal.Audit3EnvironmentKey | ||
ProducerTypeKey = auditloginternal.Audit3ProducerTypeKey | ||
OrganizationsKey = auditloginternal.Audit3OrganizationsKey | ||
EventIDKey = auditloginternal.Audit3EventIDKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also logEntryId
?
ServiceKey = auditloginternal.Audit3ServiceKey | ||
EnvironmentKey = auditloginternal.Audit3EnvironmentKey | ||
ProducerTypeKey = auditloginternal.Audit3ProducerTypeKey | ||
OrganizationsKey = auditloginternal.Audit3OrganizationsKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually don't populate this field in v3 logs, so you can probably leave it out. https://github.palantir.build/foundry/foundry-audit/blob/210c2a928abb2bfee2a4681c260325f7486c2a00/audit-core-api/src/main/java/com/palantir/foundry/audit/AuditV3Logger.java#L96-L163
if uid := wloginternal.IDFromContext(ctx, wloginternal.UIDKey); uid != nil { | ||
params = append(params, UID(*uid)) | ||
} | ||
if sid := wloginternal.IDFromContext(ctx, wloginternal.SIDKey); sid != nil { | ||
params = append(params, SID(*sid)) | ||
} | ||
if tokenID := wloginternal.IDFromContext(ctx, wloginternal.TokenIDKey); tokenID != nil { | ||
params = append(params, TokenID(*tokenID)) | ||
} | ||
if orgID := wloginternal.IDFromContext(ctx, wloginternal.OrgIDKey); orgID != nil { | ||
params = append(params, OrgID(*orgID)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious where the logic that pulls these values out of the token and adds them to the Context
lives?
for i, param := range params { | ||
audit3Params[i] = Audit3Param{ | ||
Param: param.Param, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only convert an Audit2Param
to an Audit3Param
if they have the same underlying type of AuditParam
? But if you use the audit.2
logger, the audit.3
log that is dual-written will only contain fields that are in the audit.2
log? We'd probably need to make sure that all the fields required on AuditLogV3
are present. You mentioned in the existing state, clients provide the category as a RequestParam
- when converting to audit.3
, can we move any categories from the RequestParam
field to the categories
field? Also, we can consider moving values from Audit2RequestParams
and Audit2ResultParams
to Audit3RequestFields
and Audit3ResultFields
.
Also, is there a way to add fields that foundry-audit will pull from request headers - origins
, sourceOrigins
, userAgent
? Not sure how these are currently populated in audit.2
logs. In foundry-audit, they're added to the requestParams: https://github.palantir.build/foundry/foundry-audit/blob/210c2a928abb2bfee2a4681c260325f7486c2a00/audit-core-api/src/main/java/com/palantir/foundry/audit/AuditLogEntryImpl.java#L313-L340
Do existing clients of the audit.2
logger provide categories?
In audit.2
, clients provide requestParams
and resultParams
which might contain category
or categories
fields. If we wanted to dual-write to audit.3
type, we'd need to pull the
Does the following:
Before this PR
After this PR
==COMMIT_MSG==
Adds support for audit.v3 logging
==COMMIT_MSG==
Possible downsides?