Skip to content

Fixes #28770: write server audit entries to audit.log#28771

Open
harshach wants to merge 2 commits into
mainfrom
harshach/audit-log-not-writing
Open

Fixes #28770: write server audit entries to audit.log#28771
harshach wants to merge 2 commits into
mainfrom
harshach/audit-log-not-writing

Conversation

@harshach
Copy link
Copy Markdown
Collaborator

@harshach harshach commented Jun 5, 2026

Describe your changes:

Fixes #28770

Server audit logging to audit.log silently broke in #23733: gutting AuditEventHandler removed the only emitter of the AUDIT SLF4J marker, so the audit.log appender (guarded by audit-only-filter-factory) still creates the file at startup but never writes a line to it. I re-emit each audit entry through the AUDIT-marked logger from AuditLogRepository.write() — the single chokepoint all audit records flow through — gated on a successful, non-duplicate DB insert so the publisher + consumer dual-write path (and multi-server replays) can't double-log. The emitted line is a compact one-line summary at INFO, not the full ChangeEvent JSON (which can be 1–2 MB/event).

Type of change:

  • Bug fix

High-level design:

AuditLogDAO.insert now returns rows-affected (int); since the insert is idempotent (ON CONFLICT DO NOTHING / INSERT IGNORE), the file line is emitted only when > 0, giving exactly-once file logging that matches the DB's exactly-once semantics. Config hardening in conf/openmetadata.yaml: pin the audit logger to INFO so capture is independent of the global LOG_LEVEL, and add audit-exclude-filter-factory to the console appender so audit lines land only in audit.log. The DB-backed audit subsystem (REST API + UI) is unchanged.

Tests:

Use cases covered

  • A supported audit event (entity CRUD / login) is written as one line to audit.log.
  • A duplicate write (publisher + consumer replay of the same change event) does not produce a second audit line.

Unit tests

  • I added unit tests for the new/changed logic.
  • Files added/updated: openmetadata-service/src/test/java/org/openmetadata/service/audit/AuditLogRepositoryTest.java — asserts the AUDIT marker is emitted on insert, and suppressed when the insert is de-duplicated (gate). Pure unit tests (mocked DAO + in-memory logback ListAppender), no DB/container.

Backend integration tests

  • Not applicable (no backend API changes; the existing AuditLogResourceIT covers the DB-backed path).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (no UI changes).

Manual testing performed

  1. mvn -pl openmetadata-service test -Dtest=AuditLogRepositoryTest — 2/2 pass; observed the INFO + AUDIT summary line emitted on insert and suppressed on dedup.
  2. mvn -pl openmetadata-service test -Dtest=LoggingConfigurationYamlTest — 2/2 pass; confirms the edited conf/openmetadata.yaml parses in both text and JSON logging modes.
  3. mvn spotless:apply -pl openmetadata-service — clean.

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added a test that covers the exact scenario we are fixing. For complex issues, comment the issue number in the test for future reference.

Audit-to-file logging silently broke in #23733: gutting AuditEventHandler
removed the only emitter of the AUDIT log marker, so the audit.log appender
(audit-only-filter-factory) creates the file but never writes to it.

Re-emit each audit entry through the AUDIT-marked logger from
AuditLogRepository.write(), gated on a successful (non-duplicate) DB insert so
the publisher+consumer dual-write path does not double-log. Emit a compact
one-line summary at INFO rather than the full ChangeEvent JSON. Harden
conf/openmetadata.yaml: pin the audit logger to INFO so capture is independent
of LOG_LEVEL, and exclude audit lines from the console appender so they land
only in audit.log.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🟡 Playwright Results — all passed (13 flaky)

✅ 4276 passed · ❌ 0 failed · 🟡 13 flaky · ⏭️ 88 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 300 0 1 4
🟡 Shard 2 806 0 3 9
🟡 Shard 3 803 0 4 8
🟡 Shard 4 844 0 3 12
✅ Shard 5 721 0 0 47
🟡 Shard 6 802 0 2 8
🟡 13 flaky test(s) (passed on retry)
  • Features/Pagination.spec.ts › should test Pipeline Tasks normal pagination (shard 1, 1 retry)
  • Features/DataQuality/TestCaseImportExportE2eFlow.spec.ts › EditAll User: Complete export-import-validate flow (shard 2, 1 retry)
  • Features/DataQuality/TestCaseResultPermissions.spec.ts › User with only VIEW cannot PATCH results (shard 2, 1 retry)
  • Features/Glossary/GlossaryWorkflow.spec.ts › should display correct status badge color and icon (shard 2, 1 retry)
  • Features/ImpactAnalysis.spec.ts › Verify upstream downstream toggle persists pagination (shard 3, 1 retry)
  • Features/IncidentManager.spec.ts › Complete Incident lifecycle with table owner (shard 3, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Flow/ExploreAggregationCountsMatching.spec.ts › should verify left panel counts and tab search results for normal search (shard 3, 2 retries)
  • Pages/CustomProperties.spec.ts › Entity Reference (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Api Collection (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Is_Set (shard 4, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can edit teams from the user profile (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Resolved the add/add conflict in AuditLogRepositoryTest.java by combining this
branch's audit-marker emission + dedup-gate tests with the lineage-persistence
test added in #28426. AuditLogRepository.java and CollectionDAO.java auto-merged
cleanly: main's lineage event types and IntakeForm DAO coexist with the
insert-gated audit-to-file logging. Verified: 16/16 audit/logging tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Jun 5, 2026

Code Review ✅ Approved

Restores audit logging by re-emitting events through the AUDIT-marked logger within the AuditLogRepository, ensuring exactly-once writes by gating on successful database inserts. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 5, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Server audit logging silently broken: audit.log is created but never written

1 participant