feat(accesslog): export HTTP access logs to S3 as gzipped JSONL - #406
feat(accesslog): export HTTP access logs to S3 as gzipped JSONL#406worstell wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c021c1330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Adds an optional access-log block that buffers one structured event per request and periodically flushes batches to S3 as gzip-compressed JSONL objects, for ingestion by log analysis and security monitoring pipelines. The middleware wraps the OPA middleware so denied requests are captured. Recording never blocks the request path: a full buffer drops events and a failed flush requeues its batch. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019ff1c0-d6a2-75dd-b08a-d3c896386071
2c021c1 to
709579f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 709579f8e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Validate checks the configuration for invalid values. | ||
| func (c Config) Validate() error { | ||
| if c.FlushInterval <= 0 { |
There was a problem hiding this comment.
When max-buffered-events is explicitly configured as zero or a negative value, validation succeeds, but Record then satisfies len(w.events) >= w.config.MaxBufferedEvents for every request and silently drops the entire access log stream. Reject non-positive capacities during startup rather than accepting a configuration that can never export an event.
Useful? React with 👍 / 👎.
| // shutdown, and Close performs the final flush only after that drain. | ||
| func (w *Writer) run(ctx context.Context) { | ||
| defer close(w.stopped) | ||
| ctx = context.WithoutCancel(ctx) |
There was a problem hiding this comment.
When a periodic PutObject is blocked as SIGTERM arrives, stripping cancellation here leaves that upload running without a deadline; Close can only time out while waiting for w.stopped, so it never performs its final flush, and the process exits with the detached batch lost. Keep the loop independent of root cancellation, but give it a writer-owned context that Close can cancel or bound with its shutdown deadline.
Useful? React with 👍 / 👎.
| fatalIfError(ctx, logger, config.Validate(), "Invalid access log config") | ||
| return accesslog.NewWriter(ctx, config, provider) |
There was a problem hiding this comment.
When access-log validation fails, this factory logs and terminates the process internally through fatalIfError instead of returning the error, preventing its caller from controlling reporting or cleanup. Return an error from the helper and handle it in main, as required by the repository's error-handling convention.
AGENTS.md reference: AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| rec := &responseRecorder{ResponseWriter: w} | ||
| start := time.Now() | ||
| next.ServeHTTP(rec, r) |
There was a problem hiding this comment.
If an OPA or strategy handler panics, control unwinds past this call and never reaches recorder.Record, while net/http recovers outside this middleware; consequently the failure produces no access event despite the middleware's one-event-per-request contract. Arrange event recording in deferred cleanup while preserving the panic so these especially important failed requests remain observable.
Useful? React with 👍 / 👎.
|
Istio already writes access logs, so that's the better place to do this logging. |
2 participants
Adds an optional
access-logconfig block that exports structured HTTP access log events to S3 as batched JSONL objects (gzipped by default,compression = "none"for plain JSONL), suitable for ingestion by log analysis and security monitoring pipelines.Events record timestamp, method, path/query, status, bytes sent, duration, client address, user agent, and configurable request headers. The middleware wraps outside OPA so denied requests are logged too. Buffering is in-memory and non-blocking: events are dropped (and counted) when the buffer is full, and failed flushes requeue for the next attempt. Batches flush periodically and on graceful shutdown, using the existing S3 client configuration. No ACLs are set on upload, so cross-account buckets should use Object Ownership = bucket owner enforced.
Generated with Amp