Harden admission violation export: Dapr guard, recovery, and docs - #4782
Open
ShiyunXu wants to merge 3 commits into
Open
Harden admission violation export: Dapr guard, recovery, and docs#4782ShiyunXu wants to merge 3 commits into
ShiyunXu wants to merge 3 commits into
Conversation
Admission violation export depends on the disk driver's durable, bounded spool. Reject admission violations in the Dapr driver so the restriction holds even under raw-manifest deployments that bypass the Helm guard. Signed-off-by: ShiyunXu <ShiyunXu@users.noreply.github.com>
Guard against a non-positive maxBytes deleting complete records, avoid clobbering an existing .recovered.log on repeat recovery, and fix findLastNewline to scan only the bytes actually read. Signed-off-by: ShiyunXu <ShiyunXu@users.noreply.github.com>
Explain why the 64 KiB complete-record limit is well chosen, what it measures, and how it protects the bounded queue and spool. Note the Dapr guard in the export docs. Signed-off-by: ShiyunXu <ShiyunXu@users.noreply.github.com>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens admission violation export by restricting Dapr usage, improving disk recovery, and clarifying operational limits.
Changes:
- Rejects admission violations sent through Dapr.
- Preserves complete records during disk recovery and avoids deterministic filename collisions.
- Expands tests and documentation for recovery and record-size limits.
Show a summary per file
| File | Description |
|---|---|
website/docs/export.md |
Documents backend restrictions and record limits. |
pkg/webhook/export.go |
Expands record-limit rationale. |
pkg/export/disk/disk.go |
Clarifies disk record bounds. |
pkg/export/disk/admission_file.go |
Hardens recovery and newline scanning. |
pkg/export/disk/admission_file_test.go |
Tests recovery edge cases. |
pkg/export/dapr/dapr.go |
Adds the Dapr admission guard. |
pkg/export/dapr/dapr_test.go |
Tests supported payload shapes. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
Comment on lines
+1045
to
1049
| readyPath, err := recoveredReadyPath(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(path, readyPath) |
Comment on lines
+68
to
+73
| case json.RawMessage: | ||
| var msg util.ExportMsg | ||
| if err := json.Unmarshal(value, &msg); err != nil { | ||
| return false | ||
| } | ||
| return msg.EventType == util.AdmissionViolationEventType |
| - the request identity (username, UID, and groups), | ||
| - and the JSON structure itself (field names, quotes, and escaping). | ||
|
|
||
| Because the limit is measured in bytes, the number of characters that fit depends on encoding. A 64 KiB record holds up to 65,536 bytes: roughly 65,000 plain ASCII characters, about half that for accented Latin or other two-byte UTF-8 text, and fewer still for three-byte (for example, CJK) or four-byte (for example, emoji) characters. Characters that must be JSON-escaped, such as `"`, `\`, and newlines, cost two bytes each. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three focused, independent follow-ups to the webhook/admission violation export feature. Each commit is self-contained, compiles, and carries its own tests; the series bisects cleanly.
1. Reject admission violation export on the Dapr driver
Admission violation export relies on the disk driver's durable, bounded on-disk spool (record-size limits, rotation, retention, crash recovery). The Dapr driver provides none of these guarantees. The Helm chart already blocks
enableAdmissionViolationExportunlessexportBackend: disk, but raw-manifest deployments bypass that guard. This adds a runtime check inDapr.Publishthat rejects admission violation messages (acrossExportMsg,*ExportMsg, andjson.RawMessageshapes), surfaced through the existing export status, logs, and metrics.2. Harden
recoverAdmissionOpenFilemaxBytestruncating away complete records..recovered.logwhen the same base segment is recovered more than once (deterministic name first, random suffix as fallback).findLastNewlineto scan only the bytes actually read (bytes.LastIndexByte(chunk[:n])) instead of the whole buffer converted from a possibly short read.3. Document admission export record and disk limits
Expand the code comments and
website/docs/export.mdto explain what the 64 KiB complete-record limit measures (the full encoded record, including policydetailsand resource labels, not just the message), why it is well chosen, character-capacity guidance, and how it protects the bounded queue and spool. Also notes the new Dapr driver guard.Testing
go build/go vetonpkg/export/...andpkg/webhook/...go test -racefor the Dapr publish guard, disk recovery, and webhook export packagesgofmtcleanDocs-only content in commit 3 (aside from the code comments) needs no test.