diff --git a/docs/adr/52107-logentry-interface-for-log-entry-structs.md b/docs/adr/52107-logentry-interface-for-log-entry-structs.md new file mode 100644 index 00000000000..a9e93c48f0d --- /dev/null +++ b/docs/adr/52107-logentry-interface-for-log-entry-structs.md @@ -0,0 +1,49 @@ +# ADR-52107: LogEntry Interface for Heterogeneous Log-Entry Structs + +**Date**: 2026-08-11 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +`pkg/cli` contains four independently-defined structs — `AccessLogEntry`, `FirewallLogEntry`, `AuditLogEntry`, and `GatewayLogEntry` — each modelling a parsed log line from a different source. They share no common type, so any code that wants to handle "a log entry" generically (formatters, filters, report generators) must special-case every concrete type. The structs also have structurally incompatible fields: `Timestamp` is a `string` in three types but a `float64` in `AuditLogEntry`, and only `GatewayLogEntry` carries a `Level` field. These differences make a shared embedded base struct impractical without changing wire formats. + +### Decision + +We will define a `LogEntry` interface in `pkg/cli/log_entry.go` with four accessor methods — `EntryTimestamp()`, `EntrySource()`, `EntryLevel()`, and `EntryMessage()` — and implement it on all four existing log-entry types using value receivers. Timestamp normalisation (epoch seconds → RFC3339 UTC) is handled inside the implementations so callers see a uniform string regardless of the underlying field type. Compile-time conformance is enforced with blank-identifier assertions (`var _ LogEntry = AccessLogEntry{}`). A `FormatLogEntry` function serves as the first generic consumer. + +### Alternatives Considered + +#### Alternative 1: Embedded Base Struct + +Define a shared `BaseLogEntry` struct and embed it in the four types. This would promote common fields directly and avoid the interface layer. + +Rejected because the four types have incompatible field layouts: `AuditLogEntry.Timestamp` is `float64` while the others are `string`, and only `GatewayLogEntry` has `Level`. Adding these fields to a base struct would require changing the JSON tags or adding duplicate fields, breaking existing serialisation and parse call sites. + +#### Alternative 2: Type Switch / Ad-Hoc Polymorphism + +Continue the current pattern: any code that needs to act on "any log entry" performs an explicit type switch over all four concrete types. + +Rejected because it duplicates the dispatch logic in every consumer, makes adding a fifth log-entry type a multi-site change, and provides no compile-time guarantee that all types are handled. The very motivation of the linked issue (#52091) was to eliminate this duplication. + +### Consequences + +#### Positive +- Formatting, filtering, and reporting code can operate uniformly over any `[]LogEntry` without knowing the underlying type. +- Adding a fifth log-entry type in the future requires only implementing four methods, with no changes to existing consumers. +- Compile-time `var _ LogEntry = ...` assertions catch interface drift immediately at build time. +- No existing struct fields, JSON tags, or parsers are touched, so serialisation and all current call sites are unaffected. + +#### Negative +- Epoch-to-RFC3339 timestamp normalisation is now encapsulated inside each implementation, making the per-source conversion logic less visible to callers who might expect raw values. +- All four implementations use value receivers; callers passing large `[]LogEntry` slices by value incur copying overhead that would not exist with pointer receivers or a concrete slice type. + +#### Neutral +- The `LogEntry` interface is defined in the same `cli` package as the concrete types, so there is no cross-package dependency change. +- Test coverage is added in `pkg/cli/log_entry_test.go` using table-driven tests; the interface itself is not exported beyond the `cli` package boundary at this point. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/log_entry.go b/pkg/cli/log_entry.go new file mode 100644 index 00000000000..23e4a2ef531 --- /dev/null +++ b/pkg/cli/log_entry.go @@ -0,0 +1,169 @@ +package cli + +import ( + "fmt" + "strconv" + "strings" + "time" +) + +// LogEntrySource identifies the log stream a LogEntry was parsed from. +type LogEntrySource string + +const ( + // LogSourceAccess identifies squid access log entries. + LogSourceAccess LogEntrySource = "access" + // LogSourceFirewall identifies firewall log entries. + LogSourceFirewall LogEntrySource = "firewall" + // LogSourceAudit identifies audit.jsonl entries. + LogSourceAudit LogEntrySource = "audit" + // LogSourceGateway identifies MCP gateway.jsonl entries. + LogSourceGateway LogEntrySource = "gateway" +) + +// Shared severity levels reported by LogEntry.EntryLevel. +const ( + LogLevelInfo = "info" + LogLevelError = "error" +) + +// LogEntry is the shared shape implemented by every parsed log-line type +// (AccessLogEntry, FirewallLogEntry, AuditLogEntry and GatewayLogEntry). +// It lets formatting, filtering and reporting code operate on any log entry +// without special-casing each concrete type. +type LogEntry interface { + // EntryTimestamp returns the entry timestamp, normalized to RFC3339 (UTC) + // for sources that record epoch timestamps. Unparseable timestamps are + // returned unchanged. + EntryTimestamp() string + // EntrySource returns the log stream the entry was parsed from. + EntrySource() LogEntrySource + // EntryLevel returns the entry severity: LogLevelInfo or LogLevelError. + EntryLevel() string + // EntryMessage returns a short human-readable description of the entry. + EntryMessage() string +} + +// Compile-time checks that all four log-entry types share the LogEntry shape. +var ( + _ LogEntry = AccessLogEntry{} + _ LogEntry = FirewallLogEntry{} + _ LogEntry = AuditLogEntry{} + _ LogEntry = GatewayLogEntry{} +) + +// FormatLogEntry renders any log entry in a uniform "timestamp [source] level: message" form. +func FormatLogEntry(entry LogEntry) string { + return fmt.Sprintf("%s [%s] %s: %s", entry.EntryTimestamp(), entry.EntrySource(), entry.EntryLevel(), entry.EntryMessage()) +} + +// formatEpochSeconds converts fractional epoch seconds to RFC3339 in UTC. +func formatEpochSeconds(seconds float64) string { + return time.Unix(0, int64(seconds*float64(time.Second))).UTC().Format(time.RFC3339) +} + +// formatEpochTimestamp converts a textual epoch-seconds timestamp to RFC3339 in UTC, +// returning the input unchanged when it is not a numeric epoch value. +func formatEpochTimestamp(timestamp string) string { + seconds, err := strconv.ParseFloat(strings.TrimSpace(timestamp), 64) + if err != nil { + return timestamp + } + return formatEpochSeconds(seconds) +} + +// levelFromAllowed maps an allow/deny outcome to a shared severity level. +func levelFromAllowed(allowed bool) string { + if allowed { + return LogLevelInfo + } + return LogLevelError +} + +// joinEntryMessage builds a message from the first non-placeholder parts available. +func joinEntryMessage(parts ...string) string { + kept := make([]string, 0, len(parts)) + for _, part := range parts { + part = strings.TrimSpace(part) + if part == "" || part == "-" { + continue + } + kept = append(kept, part) + } + return strings.Join(kept, " ") +} + +// EntryTimestamp implements LogEntry. +func (a AccessLogEntry) EntryTimestamp() string { return formatEpochTimestamp(a.Timestamp) } + +// EntrySource implements LogEntry. +func (a AccessLogEntry) EntrySource() LogEntrySource { return LogSourceAccess } + +// EntryLevel implements LogEntry. +func (a AccessLogEntry) EntryLevel() string { return levelFromAllowed(isAllowedSquidStatus(a.Status)) } + +// EntryMessage implements LogEntry. +func (a AccessLogEntry) EntryMessage() string { return joinEntryMessage(a.Method, a.URL, a.Status) } + +// EntryTimestamp implements LogEntry. +func (f FirewallLogEntry) EntryTimestamp() string { return formatEpochTimestamp(f.Timestamp) } + +// EntrySource implements LogEntry. +func (f FirewallLogEntry) EntrySource() LogEntrySource { return LogSourceFirewall } + +// EntryLevel implements LogEntry. +func (f FirewallLogEntry) EntryLevel() string { + return levelFromAllowed(isRequestAllowed(f.Decision, f.Status)) +} + +// EntryMessage implements LogEntry. +func (f FirewallLogEntry) EntryMessage() string { + target := f.URL + if strings.TrimSpace(target) == "" || target == "-" { + target = f.Domain + } + return joinEntryMessage(f.Method, target, f.Decision) +} + +// EntryTimestamp implements LogEntry. +func (a AuditLogEntry) EntryTimestamp() string { return formatEpochSeconds(a.Timestamp) } + +// EntrySource implements LogEntry. +func (a AuditLogEntry) EntrySource() LogEntrySource { return LogSourceAudit } + +// EntryLevel implements LogEntry. +func (a AuditLogEntry) EntryLevel() string { return levelFromAllowed(isEntryAllowed(a)) } + +// EntryMessage implements LogEntry. +func (a AuditLogEntry) EntryMessage() string { + target := a.URL + if strings.TrimSpace(target) == "" || target == "-" { + target = a.Host + } + return joinEntryMessage(a.Method, target, a.Decision) +} + +// EntryTimestamp implements LogEntry. +func (g GatewayLogEntry) EntryTimestamp() string { return g.Timestamp } + +// EntrySource implements LogEntry. +func (g GatewayLogEntry) EntrySource() LogEntrySource { return LogSourceGateway } + +// EntryLevel implements LogEntry. It mirrors the error classification used +// by gateway log metrics processing (see processGatewayLogEntry), treating +// Status == "error", a non-empty Error, or Level == "error" as failure +// signals, and normalizes the result to the interface's two documented +// levels (LogLevelInfo / LogLevelError). +func (g GatewayLogEntry) EntryLevel() string { + return levelFromAllowed(g.Status != "error" && g.Error == "" && g.Level != "error") +} + +// EntryMessage implements LogEntry. +func (g GatewayLogEntry) EntryMessage() string { + for _, candidate := range []string{g.Message, g.Error, g.Event, g.Type} { + if strings.TrimSpace(candidate) != "" { + return candidate + } + } + return "" +} diff --git a/pkg/cli/log_entry_test.go b/pkg/cli/log_entry_test.go new file mode 100644 index 00000000000..a46d95d2227 --- /dev/null +++ b/pkg/cli/log_entry_test.go @@ -0,0 +1,203 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestLogEntryInterfaceAccessors(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entry LogEntry + expectedTimestamp string + expectedSource LogEntrySource + expectedLevel string + expectedMessage string + }{ + { + name: "allowed access log entry", + entry: AccessLogEntry{ + Timestamp: "1701234567.123", + Status: "TCP_MISS/200", + Method: "GET", + URL: "http://example.com/api", + }, + expectedTimestamp: "2023-11-29T05:09:27Z", + expectedSource: LogSourceAccess, + expectedLevel: LogLevelInfo, + expectedMessage: "GET http://example.com/api TCP_MISS/200", + }, + { + name: "denied access log entry", + entry: AccessLogEntry{ + Timestamp: "1701234568.456", + Status: "TCP_DENIED/403", + Method: "CONNECT", + URL: "github.com:443", + }, + expectedTimestamp: "2023-11-29T05:09:28Z", + expectedSource: LogSourceAccess, + expectedLevel: LogLevelError, + expectedMessage: "CONNECT github.com:443 TCP_DENIED/403", + }, + { + name: "allowed firewall log entry", + entry: FirewallLogEntry{ + Timestamp: "1761332530.474", + Domain: "api.github.com:443", + Method: "CONNECT", + Status: "200", + Decision: "TCP_TUNNEL:HIER_DIRECT", + URL: "api.github.com:443", + }, + expectedTimestamp: "2025-10-24T19:02:10Z", + expectedSource: LogSourceFirewall, + expectedLevel: LogLevelInfo, + expectedMessage: "CONNECT api.github.com:443 TCP_TUNNEL:HIER_DIRECT", + }, + { + name: "blocked firewall log entry falls back to domain", + entry: FirewallLogEntry{ + Timestamp: "1761332530.500", + Domain: "blocked.example.com:443", + Method: "-", + Status: "403", + Decision: "NONE_NONE:HIER_NONE", + URL: "-", + }, + expectedTimestamp: "2025-10-24T19:02:10Z", + expectedSource: LogSourceFirewall, + expectedLevel: LogLevelError, + expectedMessage: "blocked.example.com:443 NONE_NONE:HIER_NONE", + }, + { + name: "allowed audit log entry", + entry: AuditLogEntry{ + Timestamp: 1701234567.123, + Host: "api.github.com:443", + Method: "CONNECT", + Status: 200, + Decision: "TCP_TUNNEL", + }, + expectedTimestamp: "2023-11-29T05:09:27Z", + expectedSource: LogSourceAudit, + expectedLevel: LogLevelInfo, + expectedMessage: "CONNECT api.github.com:443 TCP_TUNNEL", + }, + { + name: "denied audit log entry", + entry: AuditLogEntry{ + Timestamp: 1701234567.123, + Host: "evil.com:443", + Method: "CONNECT", + Status: 403, + Decision: "NONE_NONE", + }, + expectedTimestamp: "2023-11-29T05:09:27Z", + expectedSource: LogSourceAudit, + expectedLevel: LogLevelError, + expectedMessage: "CONNECT evil.com:443 NONE_NONE", + }, + { + name: "gateway log entry uses its own level", + entry: GatewayLogEntry{ + Timestamp: "2024-01-12T10:00:00Z", + Level: LogLevelInfo, + Type: "request", + Event: "tool_call", + Message: "calling search_issues", + }, + expectedTimestamp: "2024-01-12T10:00:00Z", + expectedSource: LogSourceGateway, + expectedLevel: LogLevelInfo, + expectedMessage: "calling search_issues", + }, + { + name: "gateway log entry without level falls back to error state", + entry: GatewayLogEntry{ + Timestamp: "2024-01-12T10:00:01Z", + Type: "response", + Event: "tool_call", + Error: "connection timeout", + }, + expectedTimestamp: "2024-01-12T10:00:01Z", + expectedSource: LogSourceGateway, + expectedLevel: LogLevelError, + expectedMessage: "connection timeout", + }, + { + name: "gateway log entry reports error level when status is error even with info level", + entry: GatewayLogEntry{ + Timestamp: "2024-01-12T10:00:02Z", + Level: LogLevelInfo, + Status: "error", + Event: "tool_call", + Message: "call failed", + }, + expectedTimestamp: "2024-01-12T10:00:02Z", + expectedSource: LogSourceGateway, + expectedLevel: LogLevelError, + expectedMessage: "call failed", + }, + { + name: "gateway log entry with all-blank fields returns empty message", + entry: GatewayLogEntry{ + Timestamp: "2024-01-12T10:00:03Z", + Level: LogLevelInfo, + }, + expectedTimestamp: "2024-01-12T10:00:03Z", + expectedSource: LogSourceGateway, + expectedLevel: LogLevelInfo, + expectedMessage: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expectedTimestamp, tt.entry.EntryTimestamp()) + assert.Equal(t, tt.expectedSource, tt.entry.EntrySource()) + assert.Equal(t, tt.expectedLevel, tt.entry.EntryLevel()) + assert.Equal(t, tt.expectedMessage, tt.entry.EntryMessage()) + }) + } +} + +func TestFormatLogEntryIsGenericAcrossSources(t *testing.T) { + t.Parallel() + + entries := []LogEntry{ + AccessLogEntry{Timestamp: "1701234567.123", Status: "TCP_MISS/200", Method: "GET", URL: "http://example.com"}, + FirewallLogEntry{Timestamp: "1701234567.123", Method: "CONNECT", Status: "200", Decision: "TCP_TUNNEL", URL: "example.com:443"}, + AuditLogEntry{Timestamp: 1701234567.123, Host: "example.com:443", Method: "CONNECT", Status: 200, Decision: "TCP_TUNNEL"}, + GatewayLogEntry{Timestamp: "2024-01-12T10:00:00Z", Level: LogLevelInfo, Event: "tool_call"}, + } + + formatted := make([]string, 0, len(entries)) + for _, entry := range entries { + formatted = append(formatted, FormatLogEntry(entry)) + } + + assert.Equal(t, []string{ + "2023-11-29T05:09:27Z [access] info: GET http://example.com TCP_MISS/200", + "2023-11-29T05:09:27Z [firewall] info: CONNECT example.com:443 TCP_TUNNEL", + "2023-11-29T05:09:27Z [audit] info: CONNECT example.com:443 TCP_TUNNEL", + "2024-01-12T10:00:00Z [gateway] info: tool_call", + }, formatted) +} + +func TestFormatEpochTimestampKeepsNonEpochValues(t *testing.T) { + t.Parallel() + + assert.Equal(t, "2024-01-12T10:00:00Z", formatEpochTimestamp("2024-01-12T10:00:00Z")) + assert.Empty(t, formatEpochTimestamp("")) + assert.Equal(t, "-", formatEpochTimestamp("-")) + // A zero epoch is a valid timestamp, not a placeholder like "-" or "", so + // it is normalized like any other numeric epoch value. + assert.Equal(t, "1970-01-01T00:00:00Z", formatEpochTimestamp("0")) +}