-
Notifications
You must be signed in to change notification settings - Fork 138
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
CBG-3975: Configuration wiring for audit logging and /db/_config APIs #6918
Conversation
func (l *AuditLogger) getAuditLoggerConfig() *AuditLoggerConfig { | ||
c := AuditLoggerConfig{} | ||
if l != nil { | ||
// Copy config struct to avoid mutating running config |
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 help a lot given that all the member of AuditLoggerConfig are pointers and this is a shallow copy?
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.
Doesn't seem nessesary since there's no runtime configuration beyond the embedded FileLoggerConfig (yet), but I think I'd rather just keep this here in case we do add runtime-configurable options to auditLogger
. Follows the same pattern as console and file loggers.
eceb115
to
59589c2
Compare
8fbc22c
to
47b7d4a
Compare
… be read/written through /db/_config api
…onfig like the other loggers
dblc.Audit = &DbAuditLoggingConfig{ | ||
Enabled: base.BoolPtr(false), | ||
EnabledEvents: base.DefaultAuditEventIDs, | ||
} |
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.
I would have expected these to inherit the value from the bootstrap and override if appropriate?
Is this change specific to capella, since audit logging should be enabled globally, but potentially turned off per database?
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.
It's anticipated that users will enable audit logging (globally), and then opt-into databases one at a time where required. Enabling globally then disabling subsets of databases creates unnessesary audit work/noise for the sake of one extra API call. It's expected that users will want to customise their enabled audit events anyway.
…#6918) * Wire up config for audit logging. Per-database audit log settings can be read/written through /db/_config api * Show correct 'enabled' state for event in /db/_config/audit * Allow rutime setting of bootstrap logging.audit.enabled from root /_config like the other loggers
…#6918) * Wire up config for audit logging. Per-database audit log settings can be read/written through /db/_config api * Show correct 'enabled' state for event in /db/_config/audit * Allow rutime setting of bootstrap logging.audit.enabled from root /_config like the other loggers
CBG-3975
Configuration wiring for audit logging.
GET /db/_config/audit
shows correct 'enabled' state for each available event. Read-only until CBG-3981Dependencies
Integration Tests
GSI=true,xattrs=true
https://jenkins.sgwdev.com/job/SyncGateway-Integration/000/