Skip to content

Commit c98e20d

Browse files
edimarlnxclaude
andcommitted
live log: journalctl -k is --dmesg, not --kernel
The live firewall-log view invoked `journalctl --kernel --follow`, but journalctl has no --kernel option (the kernel-only filter is -k, long form --dmesg). On a real machine the stream exited at once with "unrecognized option '--kernel'" and the view showed only "the live log ended: exit status 1" — the per-rule logging feature never worked outside the demo. The unit tests missed it because they drive the Fake stream, so the real argv was never run; a lab twin on a VM caught it. Fix the flag, and stop discarding journalctl's stderr: capture it (capped) and fold its first line into the stream's end error, so a future bad invocation says why instead of just "exit status 1". Add a test pinning the kernel-log argv to a valid flag. Verified on a libvirt VM through the TUI: w opens the live view, the stream stays attached, and logged forward packets appear with direction, verdict, addresses and ports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent a6028a9 commit c98e20d

2 files changed

Lines changed: 119 additions & 10 deletions

File tree

‎internal/nftables/logstream.go‎

Lines changed: 65 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,54 @@ func (s *LogStream) pumpReader(ctx context.Context, reader io.ReadCloser, wait f
254254
}
255255
}
256256

257+
// kernelLogArgs is the journalctl invocation the live view follows: the kernel
258+
// log, from the tail forward, in the timestamp format the parser reads. The
259+
// kernel-only filter is journalctl's -k, whose long form is --dmesg. There is
260+
// no --kernel option; passing one makes journalctl exit at once, before a line
261+
// is read, which the unit tests miss because they drive the Fake stream — so
262+
// this stays one place a test can pin.
263+
func kernelLogArgs() []string {
264+
// --lines=0 starts at the tail: the view shows packets logged from now on,
265+
// not a replay of the journal. short-iso is the timestamp the parser reads.
266+
return []string{
267+
"--dmesg", "--follow", "--lines=0", "--output=short-iso", "--no-pager",
268+
}
269+
}
270+
271+
// cappedBuffer collects a process's stderr up to a fixed size and drops the
272+
// rest, so a failing command's first diagnostic line survives without a
273+
// misbehaving one growing the buffer without bound. It always reports a full
274+
// write, so the process never blocks on a full stderr pipe.
275+
type cappedBuffer struct {
276+
buf []byte
277+
limit int
278+
}
279+
280+
func (c *cappedBuffer) Write(p []byte) (int, error) {
281+
if room := c.limit - len(c.buf); room > 0 {
282+
if len(p) < room {
283+
room = len(p)
284+
}
285+
c.buf = append(c.buf, p[:room]...)
286+
}
287+
return len(p), nil
288+
}
289+
290+
func (c *cappedBuffer) String() string { return string(c.buf) }
291+
292+
// firstStderrLine is the reason to show the operator: a process's first
293+
// non-empty stderr line, trimmed.
294+
func firstStderrLine(s string) string {
295+
s = strings.TrimSpace(s)
296+
if i := strings.IndexByte(s, '\n'); i >= 0 {
297+
s = s[:i]
298+
}
299+
return strings.TrimSpace(s)
300+
}
301+
257302
// LogStream opens the live kernel-log feed, filtered to this tool's own log
258303
// prefix. It reads journald — where the kernel `log` statement's output lands —
259-
// with `journalctl --kernel --follow`, escalating the same way the nft reads do
304+
// with `journalctl --dmesg --follow`, escalating the same way the nft reads do
260305
// because the kernel log needs privilege. A machine without journald is told so
261306
// plainly rather than left with an empty screen.
262307
func (r *Real) LogStream(ctx context.Context) (*LogStream, error) {
@@ -268,11 +313,7 @@ func (r *Real) LogStream(ctx context.Context) (*LogStream, error) {
268313
"writes to the kernel log, which journald carries — install " +
269314
"systemd's journal, or read /dev/kmsg by hand")
270315
}
271-
// --lines=0 starts at the tail: the view shows packets logged from now on,
272-
// not a replay of the journal. short-iso is the timestamp the parser reads.
273-
journalArgs := []string{
274-
"--kernel", "--follow", "--lines=0", "--output=short-iso", "--no-pager",
275-
}
316+
journalArgs := kernelLogArgs()
276317

277318
bin := path
278319
args := journalArgs
@@ -297,16 +338,30 @@ func (r *Real) LogStream(ctx context.Context) (*LogStream, error) {
297338
cancel()
298339
return nil, errorf("could not read journalctl's output: %v", err)
299340
}
300-
cmd.Stderr = io.Discard
341+
// Keep journalctl's own diagnostics. When it exits at once — an option it
342+
// does not know, a journal it cannot read — its first stderr line is the
343+
// reason, and discarding it would leave the view saying only "exit status
344+
// 1". Capped so a process that streams to stderr cannot grow it unbounded.
345+
errbuf := &cappedBuffer{limit: 4096}
346+
cmd.Stderr = errbuf
301347
if err := cmd.Start(); err != nil {
302348
cancel()
303349
return nil, errorf("could not start journalctl: %v", err)
304350
}
305351

306-
stream := newLogStream("journald kernel log · journalctl --kernel --follow · " +
352+
stream := newLogStream("journald kernel log · journalctl --dmesg --follow · " +
307353
LogPrefixMarker + " prefix")
308354
stream.cancel = cancel
309-
go stream.pumpReader(streamCtx, stdout, cmd.Wait)
355+
wait := func() error {
356+
werr := cmd.Wait()
357+
if werr != nil {
358+
if line := firstStderrLine(errbuf.String()); line != "" {
359+
return errorf("%s (%v)", line, werr)
360+
}
361+
}
362+
return werr
363+
}
364+
go stream.pumpReader(streamCtx, stdout, wait)
310365
return stream, nil
311366
}
312367

@@ -337,7 +392,7 @@ func (r Ruleset) LoggedRules() (owned, total int) {
337392
// a stream: opening the follow would need root, which a report path may not
338393
// have, so presence on PATH is what it answers.
339394
func LogSourceProbe() (readable bool, source, detail string) {
340-
source = "journald kernel log (journalctl --kernel)"
395+
source = "journald kernel log (journalctl --dmesg)"
341396
if _, err := exec.LookPath("journalctl"); err != nil {
342397
return false, source, "journalctl not found on PATH: the live view " +
343398
"needs journald, which carries the kernel log the `log` statement writes"
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package nftables
2+
3+
import (
4+
"slices"
5+
"testing"
6+
)
7+
8+
// TestKernelLogArgsUsesValidJournalctlFlag pins the one detail a unit test can
9+
// reach of an invocation the tests otherwise never run: they drive the Fake
10+
// stream, so the real journalctl argv is exercised only on a machine. The
11+
// kernel-only filter is -k / --dmesg; --kernel is not a journalctl option and
12+
// made the live view exit 1 before a line was read.
13+
func TestKernelLogArgsUsesValidJournalctlFlag(t *testing.T) {
14+
args := kernelLogArgs()
15+
if slices.Contains(args, "--kernel") {
16+
t.Fatalf("kernelLogArgs uses --kernel, which journalctl does not accept: %v", args)
17+
}
18+
if !slices.Contains(args, "--dmesg") {
19+
t.Errorf("kernelLogArgs should filter to kernel messages with --dmesg: %v", args)
20+
}
21+
for _, want := range []string{"--follow", "--lines=0"} {
22+
if !slices.Contains(args, want) {
23+
t.Errorf("kernelLogArgs missing %s: %v", want, args)
24+
}
25+
}
26+
}
27+
28+
func TestFirstStderrLine(t *testing.T) {
29+
got := firstStderrLine("journalctl: unrecognized option '--kernel'\nmore\n")
30+
want := "journalctl: unrecognized option '--kernel'"
31+
if got != want {
32+
t.Errorf("firstStderrLine = %q, want %q", got, want)
33+
}
34+
if firstStderrLine(" \n ") != "" {
35+
t.Errorf("blank stderr should yield the empty string")
36+
}
37+
}
38+
39+
func TestCappedBufferStopsAtLimit(t *testing.T) {
40+
c := &cappedBuffer{limit: 8}
41+
n, err := c.Write([]byte("hello "))
42+
if err != nil || n != 6 {
43+
t.Fatalf("first write: n=%d err=%v", n, err)
44+
}
45+
// The writer always reports a full write, so the process never blocks, but
46+
// only what fits under the limit is retained.
47+
n, err = c.Write([]byte("world and more"))
48+
if err != nil || n != 14 {
49+
t.Fatalf("second write: n=%d err=%v", n, err)
50+
}
51+
if got := c.String(); got != "hello wo" {
52+
t.Errorf("cappedBuffer kept %q, want %q", got, "hello wo")
53+
}
54+
}

0 commit comments

Comments
 (0)