[RORDEV-2016] Audit improvements#320
Conversation
This comment was marked as spam.
This comment was marked as spam.
![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 `@details/audit.md`:
- Line 461: Update the audit docs to use the same log4j2.properties path as
other docs: replace the occurrence of "$ES_PATH_CONF/config/log4j2.properties"
in details/audit.md with "$ES_PATH_CONF/log4j2.properties" and keep the
surrounding guidance about matching the logger name to the logger_name value
(default readonlyrest_audit) intact so references remain consistent with
elasticsearch.md and the debug logging docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| #### Custom logging settings via log4j2 | ||
|
|
||
| For advanced log configuration — custom patterns, external syslog appenders, etc. — configure the logger in `$ES_PATH_CONF/config/log4j2.properties`. The logger name must match the `logger_name` value in the output config (or the default `readonlyrest_audit`): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to log4j2.properties path in documentation
rg -n "log4j2\.properties" -g "*.md" -C2Repository: beshu-tech/readonlyrest-docs
Length of output: 2315
🏁 Script executed:
#!/bin/bash
# List all occurrences of both path variants and print tight context
rg -n "\$ES_PATH_CONF/(config/)?log4j2\.properties" -g "*.md" -C2Repository: beshu-tech/readonlyrest-docs
Length of output: 54
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== occurrences of \$ES_PATH_CONF/config/log4j2.properties =="
rg -n "\$ES_PATH_CONF/config/log4j2\.properties" -g "*.md" -C2 || true
echo
echo "== occurrences of \$ES_PATH_CONF/log4j2.properties =="
rg -n "\$ES_PATH_CONF/log4j2\.properties" -g "*.md" -C2 || trueRepository: beshu-tech/readonlyrest-docs
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== occurrences of config/log4j2.properties (no $ prefix) =="
rg -n "config/log4j2\.properties" -g "*.md" -C2 || true
echo
echo "== occurrences of log4j2.properties (no config/) =="
rg -n "(^|[^/])log4j2\.properties" -g "*.md" -C2 || true
echo
echo "== show details/audit.md around line ~450-470 =="
nl -ba details/audit.md | sed -n '445,470p'
echo
echo "== show elasticsearch.md around line ~3000-3135 =="
nl -ba elasticsearch.md | sed -n '2985,3150p'Repository: beshu-tech/readonlyrest-docs
Length of output: 2138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== details/audit.md around line 461 =="
awk 'NR>=452 && NR<=468 {printf "%d:%s\n", NR, $0}' details/audit.md
echo
echo "== elasticsearch.md around line 3010-3020 =="
awk 'NR>=3008 && NR<=3020 {printf "%d:%s\n", NR, $0}' elasticsearch.md
echo
echo "== elasticsearch.md around line 3110-3120 =="
awk 'NR>=3110 && NR<=3120 {printf "%d:%s\n", NR, $0}' elasticsearch.md
echo
echo "== elasticsearch.md around line 3122-3140 =="
awk 'NR>=3122 && NR<=3140 {printf "%d:%s\n", NR, $0}' elasticsearch.mdRepository: beshu-tech/readonlyrest-docs
Length of output: 2922
Align log4j2.properties path in audit docs
details/audit.mdpoints to$ES_PATH_CONF/config/log4j2.properties, but elsewhere in the docs (elasticsearch.md, debug logging) the path is$ES_PATH_CONF/log4j2.properties. Updatedetails/audit.mdto use the same documented path.
🤖 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 `@details/audit.md` at line 461, Update the audit docs to use the same
log4j2.properties path as other docs: replace the occurrence of
"$ES_PATH_CONF/config/log4j2.properties" in details/audit.md with
"$ES_PATH_CONF/log4j2.properties" and keep the surrounding guidance about
matching the logger name to the logger_name value (default readonlyrest_audit)
intact so references remain consistent with elasticsearch.md and the debug
logging docs.
|
|
||
| You can configure multiple outputs for audit events. When the audit is enabled, at least one output has to be defined. | ||
| If you omit `outputs` definition, the default `index` output will be used. | ||
| **Note**: Even when `audit.enabled` is `false` or not set, the built-in ACL log is a special case — it writes a human-readable decision line to Elasticsearch logs for every request by default. See [The default ACL log](#the-default-acl-log) below. |
There was a problem hiding this comment.
Maybe when we talk about defaults, we could show what the explicitly defined equivalent would look like?
| | Setting | Default | Description | | ||
| |---|---|---| | ||
| | `enabled` | `false` | Master switch for the audit subsystem | | ||
| | `default_acl_log_enabled` | `true` | Controls the built-in ACL log sink (see below) | |
There was a problem hiding this comment.
We use "output" and "sink" words. Is this required or/and interntionl? Or maybe we can just use "output" in every place?
There was a problem hiding this comment.
changed to output everywhere
| - name: Admin users | ||
| auth_key: admin:admin | ||
| audit: | ||
| # keep both sinks active (default behaviour) |
There was a problem hiding this comment.
Do we miss sth here? I see "audit:" and the comment only
There was a problem hiding this comment.
I think I meant an empty audit section, but that is not a very useful example, so now without audit: and with better comment.
| name: custom-acl-log | ||
| logger_name: my.custom.acl.logger | ||
| serializer: | ||
| type: acl # reproduces the built-in ACL log format |
There was a problem hiding this comment.
ok, but it would be nice to tell users about the format. Because I assume the format stays the same here.
And what bout the logger_name. What's this? Users may not know.
There was a problem hiding this comment.
Added explanation what logger_name does and that acl serializer produces the same format as the built-in acl log
|
|
||
| `enabled_audit_sinks` and `disabled_audit_sinks` are mutually exclusive — you cannot specify both on the same block. | ||
|
|
||
| **⚠️IMPORTANT**: When `audit.enabled: false` for a specific block, there will be no audit events at all when that block is matched — this suppresses both custom outputs and the default ACL log. **This is a change in behaviour from previous versions**, where block-level `audit: {enabled: false}` only suppressed the ES audit sinks while the ACL log continued to write. |
There was a problem hiding this comment.
But what about verbosity? I didn't check it yet, but I assume we have backward compatibility, so if someone declares verbosity: info explicitly, it should be treated as enabled ACL log with log_allowed_events: true` right?
There was a problem hiding this comment.
- Yes,
verbosity: infomaps toBlock.Audit.Enabled(logAllowedEvents = true)with all sinks active, so the ACL log continues to work exactly as before - The IMPORTANT note applies only when
audit: {enabled: false}is set explicitly in a block, which maps to Block.Audit.Disabled and suppresses everything
Blocks using verbosity settings are unaffected - never mapped to Disabled.
| default_acl_log_enabled: true | ||
| outputs: | ||
| - type: index | ||
| name: my-index-sink |
There was a problem hiding this comment.
Should we describe what this is? Is this required? If not, when users need it
There was a problem hiding this comment.
♻️ Duplicate comments (1)
details/audit.md (1)
475-475: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix unresolved log4j2.properties path from previous review.
Line 475 still documents
$ES_PATH_CONF/config/log4j2.properties, but the previous review explicitly identified that other documentation (elasticsearch.md, debug logging docs) uses$ES_PATH_CONF/log4j2.propertieswithout the/config/segment. This inconsistency will confuse users who copy the path. Update to$ES_PATH_CONF/log4j2.properties.📝 Proposed fix
-For advanced log configuration — custom patterns, external syslog appenders, etc. — configure the logger in `$ES_PATH_CONF/config/log4j2.properties`. The logger name must match the `logger_name` value in the output config (or the default `readonlyrest_audit`): +For advanced log configuration — custom patterns, external syslog appenders, etc. — configure the logger in `$ES_PATH_CONF/log4j2.properties`. The logger name must match the `logger_name` value in the output config (or the default `readonlyrest_audit`):🤖 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 `@details/audit.md` at line 475, The audit logging documentation still references the wrong log4j2.properties location, causing an inconsistency with the other docs. Update the path in the audit configuration guidance to use $ES_PATH_CONF/log4j2.properties instead of $ES_PATH_CONF/config/log4j2.properties, and keep the surrounding text about the logger name and readonlyrest_audit behavior unchanged.
🧹 Nitpick comments (3)
details/audit.md (3)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify "sinks" vs "outputs" terminology in prose.
The prose still uses "sinks" in multiple places (e.g., line 73 "per-block sink routing", line 80 "sinks", line 141-142 "sink names") despite the past review resolution to use "output" everywhere. Since the YAML keys remain
*_audit_sinks, consider standardizing prose to "output" while parenthetically noting the configuration key names, or add a brief note early in the document that "sink" refers to the configuration key*_audit_sinkswhile "output" refers to the conceptual destination. This will reduce reader confusion.🤖 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 `@details/audit.md` at line 73, The audit docs prose still mixes “sinks” and “outputs,” which conflicts with the preferred terminology. Update the wording in the documentation sections around outputs, block-level routing, and sink names so the conceptual term is consistently “output,” and only mention `*_audit_sinks` parenthetically when referring to the YAML keys. If needed, add a short note near the top of the document clarifying that “output” is the conceptual destination and `*_audit_sinks` is the configuration key used for routing.
141-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse "output" instead of "sink" in table descriptions.
The table descriptions for
enabled_audit_sinksanddisabled_audit_sinksuse "sink names" but should use "output names" for consistency with the resolution to standardize on "output" terminology. The YAML key names in the left column already make the configuration key clear.🤖 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 `@details/audit.md` around lines 141 - 142, Update the table descriptions for enabled_audit_sinks and disabled_audit_sinks in audit.md to use “output names” instead of “sink names” for consistency with the standardized terminology. Keep the configuration keys unchanged, and adjust the wording in those two rows so the descriptions refer to audit.outputs and default_acl_log as output names rather than sinks.
500-500: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse "output" instead of "sink" for consistency.
"custom log sink" should be "custom log output" to align with the resolution to standardize on "output" terminology throughout the document.
🤖 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 `@details/audit.md` at line 500, The wording in the audit document uses inconsistent terminology for the logging destination; update the phrase in the affected section to use “output” instead of “sink” so it matches the standardized terminology used elsewhere. Look for the “custom log sink” wording in the audit content and replace it with “custom log output” without changing the surrounding meaning.
🤖 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.
Duplicate comments:
In `@details/audit.md`:
- Line 475: The audit logging documentation still references the wrong
log4j2.properties location, causing an inconsistency with the other docs. Update
the path in the audit configuration guidance to use
$ES_PATH_CONF/log4j2.properties instead of
$ES_PATH_CONF/config/log4j2.properties, and keep the surrounding text about the
logger name and readonlyrest_audit behavior unchanged.
---
Nitpick comments:
In `@details/audit.md`:
- Line 73: The audit docs prose still mixes “sinks” and “outputs,” which
conflicts with the preferred terminology. Update the wording in the
documentation sections around outputs, block-level routing, and sink names so
the conceptual term is consistently “output,” and only mention `*_audit_sinks`
parenthetically when referring to the YAML keys. If needed, add a short note
near the top of the document clarifying that “output” is the conceptual
destination and `*_audit_sinks` is the configuration key used for routing.
- Around line 141-142: Update the table descriptions for enabled_audit_sinks and
disabled_audit_sinks in audit.md to use “output names” instead of “sink names”
for consistency with the standardized terminology. Keep the configuration keys
unchanged, and adjust the wording in those two rows so the descriptions refer to
audit.outputs and default_acl_log as output names rather than sinks.
- Line 500: The wording in the audit document uses inconsistent terminology for
the logging destination; update the phrase in the affected section to use
“output” instead of “sink” so it matches the standardized terminology used
elsewhere. Look for the “custom log sink” wording in the audit content and
replace it with “custom log output” without changing the surrounding meaning.
2 participants
Summary by CodeRabbit