diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go index 16646f84f6..17190fb7c2 100644 --- a/pkg/collector/collector.go +++ b/pkg/collector/collector.go @@ -1,11 +1,14 @@ package main import ( + "context" "flag" "fmt" "net/http" _ "net/http/pprof" "os" + "os/signal" + "syscall" "time" "github.com/eraser-dev/eraser/pkg/cri" @@ -85,11 +88,31 @@ func main() { os.Exit(1) } - if err := util.WriteImagesPipe(path, finalImages); err != nil { + // Registering the handler suppresses the default SIGTERM exit, so it covers + // exactly the one call that observes ctx. Everything above builds its own + // timeouts from Background, and Await below has no context at all; holding + // the handler across either would swallow the signal. + ctx, stopSignals := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + + if err := util.WriteImagesPipe(ctx, path, finalImages); err != nil { + stopSignals() log.Error(err, "failed to send images", "pipeFile", path) os.Exit(1) } + // Read before stopping, because stopSignals cancels this context itself: + // checked afterwards it would always report Canceled, and the collector + // would exit on every successful run instead of waiting for the erase. + sigErr := ctx.Err() + stopSignals() + + // A signal that landed while the handler was registered was consumed rather + // than killing the process, so it has to be acted on here. + if sigErr != nil { + log.Error(sigErr, "terminating before waiting for completion") + os.Exit(1) + } + data, err := completion.Await() if err != nil { log.Error(err, "failed to read pipe", "pipeFile", util.EraseCompleteCollectPath) diff --git a/pkg/remover/helpers.go b/pkg/remover/helpers.go index 9d16e1d7d6..eb203e9e14 100644 --- a/pkg/remover/helpers.go +++ b/pkg/remover/helpers.go @@ -8,10 +8,13 @@ import ( util "github.com/eraser-dev/eraser/pkg/utils" ) -func removeImages(c cri.Remover, targetImages []string) (int, error) { +func removeImages(ctx context.Context, c cri.Remover, targetImages []string) (int, error) { removed := 0 - backgroundContext, cancel := context.WithTimeout(context.Background(), timeout) + // Derived from the caller's context, not Background: signal notification is + // registered for the whole process, so nothing would observe a SIGTERM during + // the deletion loop otherwise. + backgroundContext, cancel := context.WithTimeout(ctx, timeout) defer cancel() images, err := c.ListImages(backgroundContext) diff --git a/pkg/remover/remover.go b/pkg/remover/remover.go index 5db5bb2dd5..299eea9327 100644 --- a/pkg/remover/remover.go +++ b/pkg/remover/remover.go @@ -39,6 +39,11 @@ const ( func main() { flag.Parse() + // A terminating pod should not leave the worker blocked on a peer that is + // never going to arrive. The stop func is discarded rather than deferred + // because every exit path here is os.Exit, which would skip it anyway. + ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + if *enableProfile { go func() { server := &http.Server{ @@ -74,7 +79,7 @@ func main() { } if *imageListPtr == "" { - nonCompliantImages, err := util.ReadImagesPipe(context.Background(), util.ScanErasePath) + nonCompliantImages, err := util.ReadImagesPipe(ctx, util.ScanErasePath) if err != nil { log.Error(err, "error reading non-compliant images") os.Exit(generalErr) @@ -106,7 +111,7 @@ func main() { log.Info("no images to exclude") } - removed, err := removeImages(client, imagelist) + removed, err := removeImages(ctx, client, imagelist) if err != nil { log.Error(err, "failed to remove images") os.Exit(generalErr) @@ -114,8 +119,6 @@ func main() { if os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" { // record metrics - ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) - exporter, reader, provider := metrics.ConfigureMetrics(ctx, log, os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT")) otel.SetMeterProvider(provider) @@ -123,16 +126,15 @@ func main() { log.Error(err, "error recording metrics") } metrics.ExportMetrics(log, exporter, reader) - cancel() } if *imageListPtr == "" { - if err := util.WriteCompletionPipe(util.EraseCompleteCollectPath); err != nil { + if err := util.WriteCompletionPipe(ctx, util.EraseCompleteCollectPath); err != nil { log.Error(err, "unable to signal completion", "pipeFile", util.EraseCompleteCollectPath) os.Exit(generalErr) } - err := util.WriteCompletionPipe(util.EraseCompleteScanPath) + err := util.WriteCompletionPipe(ctx, util.EraseCompleteScanPath) // if the scanner is disabled if os.IsNotExist(err) { return diff --git a/pkg/remover/remover_test.go b/pkg/remover/remover_test.go index d6ca179578..4b3974a4ed 100644 --- a/pkg/remover/remover_test.go +++ b/pkg/remover/remover_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "testing" v1 "k8s.io/cri-api/pkg/apis/runtime/v1" @@ -50,7 +51,7 @@ func TestRemoveImages(t *testing.T) { } } - _, err := removeImages(client, tc.remove) + _, err := removeImages(context.Background(), client, tc.remove) if tc.shouldErr && err == nil { t.Fatal("expected error, got none") } diff --git a/pkg/scanners/template/scanner_template.go b/pkg/scanners/template/scanner_template.go index 9f12dcfa21..ba66200059 100644 --- a/pkg/scanners/template/scanner_template.go +++ b/pkg/scanners/template/scanner_template.go @@ -88,7 +88,7 @@ func (cfg *config) SendImages(nonCompliantImages, failedImages []unversioned.Ima nonCompliantImages = append(nonCompliantImages, failedImages...) } - if err := util.WriteScanErasePipe(nonCompliantImages); err != nil { + if err := util.WriteImagesPipe(cfg.ctx, util.ScanErasePath, nonCompliantImages); err != nil { cfg.log.Error(err, "unable to write non-compliant images to scan erase pipe") return err } diff --git a/pkg/utils/handoff_test.go b/pkg/utils/handoff_test.go index 0d1e45a227..a965370971 100644 --- a/pkg/utils/handoff_test.go +++ b/pkg/utils/handoff_test.go @@ -2,9 +2,11 @@ package utils import ( "context" + "errors" "os" "path/filepath" "testing" + "time" "github.com/eraser-dev/eraser/api/unversioned" ) @@ -36,7 +38,7 @@ func TestImagesHandoffRoundTrip(t *testing.T) { } errCh := make(chan error, 1) - go func() { errCh <- WriteImagesPipe(path, want) }() + go func() { errCh <- WriteImagesPipe(context.Background(), path, want) }() got, err := ReadImagesPipe(context.Background(), path) if err != nil { @@ -66,7 +68,7 @@ func TestCompletionHandoffRoundTrip(t *testing.T) { defer func() { _ = pipe.Close() }() errCh := make(chan error, 1) - go func() { errCh <- WriteCompletionPipe(path) }() + go func() { errCh <- WriteCompletionPipe(context.Background(), path) }() data, err := pipe.Await() if err != nil { @@ -86,7 +88,7 @@ func TestCompletionHandoffRoundTrip(t *testing.T) { func TestWriteCompletionPipeAbsentPeerIsNotExist(t *testing.T) { path := filepath.Join(shortTempDir(t), "no-such-peer") - err := WriteCompletionPipe(path) + err := WriteCompletionPipe(context.Background(), path) if err == nil { t.Fatal("expected an error writing to an endpoint nobody published") } @@ -95,6 +97,48 @@ func TestWriteCompletionPipeAbsentPeerIsNotExist(t *testing.T) { } } +// The pre-open Stat exists so this precedence holds: an endpoint nobody +// published reports IsNotExist even when the caller is already shutting down. +// Left to a select, the two would race and "the scanner is disabled" would +// become indistinguishable from "we are terminating". +func TestWriteCompletionPipeAbsentPeerBeatsACanceledContext(t *testing.T) { + path := filepath.Join(shortTempDir(t), "no-such-peer") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := WriteCompletionPipe(ctx, path) + if err == nil { + t.Fatal("expected an error writing to an endpoint nobody published") + } + if !os.IsNotExist(err) { + t.Errorf("os.IsNotExist(%v) = false, want true", err) + } +} + +// The whole point of taking a context: a worker whose peer never arrives has to +// be able to give up, on either platform. +func TestWriteImagesPipeHonoursACanceledContext(t *testing.T) { + path := filepath.Join(shortTempDir(t), "never-read") + + ctx, cancel := context.WithCancel(context.Background()) + + errCh := make(chan error, 1) + go func() { errCh <- WriteImagesPipe(ctx, path, []unversioned.Image{{ImageID: "sha256:aaaa"}}) }() + + // nothing ever reads the endpoint, so the write is still waiting + cancel() + + select { + case err := <-errCh: + if !errors.Is(err, context.Canceled) { + t.Errorf("WriteImagesPipe = %v, want context.Canceled", err) + } + case <-time.After(30 * time.Second): + t.Fatal("WriteImagesPipe ignored the canceled context") + } +} + func TestCompletionPipeCloseIsIdempotentlySafe(t *testing.T) { path := filepath.Join(shortTempDir(t), "closed") diff --git a/pkg/utils/handoff_unix.go b/pkg/utils/handoff_unix.go index 49ae8dd35e..da31a31d6a 100644 --- a/pkg/utils/handoff_unix.go +++ b/pkg/utils/handoff_unix.go @@ -68,8 +68,9 @@ func (p *CompletionPipe) Close() error { return nil } -// WriteImagesPipe publishes the endpoint and blocks until the reader connects. -func WriteImagesPipe(path string, images []unversioned.Image) error { +// WriteImagesPipe publishes the endpoint and blocks until the reader connects, +// or until ctx is done. +func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Image) error { data, err := json.Marshal(images) if err != nil { return err @@ -79,13 +80,39 @@ func WriteImagesPipe(path string, images []unversioned.Image) error { return err } - //nolint:gosec // G304: Opening pipe file is intended functionality - file, err := os.OpenFile(path, os.O_WRONLY, 0) + file, err := openForWrite(ctx, path) if err != nil { return err } - _, err = file.Write(data) + return writeAndClose(ctx, file, data) +} + +// writeAndClose writes the payload and closes, which is what frames the message +// for the reader. The open is not the only place this can block: once the pipe +// buffer fills, a reader that stops draining blocks the write too, so the +// watcher closes the file to unblock it. +func writeAndClose(ctx context.Context, file *os.File, payload []byte) error { + done := make(chan struct{}) + defer close(done) + + go func() { + select { + case <-ctx.Done(): + _ = file.Close() + case <-done: + } + }() + + _, err := file.Write(payload) + + // The watcher may already have closed the file, which is what surfaced as the + // write error, so the context is checked before the error is trusted. + if ctxErr := ctx.Err(); ctxErr != nil { + _ = file.Close() + return ctxErr + } + if closeErr := file.Close(); closeErr != nil && err == nil { err = closeErr } @@ -93,6 +120,45 @@ func WriteImagesPipe(path string, images []unversioned.Image) error { return err } +// openForWrite opens a FIFO for writing, which blocks in the kernel until a +// reader arrives. The open itself is left untouched -- the rendezvous, and the +// behavior every existing deployment depends on, is exactly as before. Only +// the waiting is made interruptible, by doing it on a goroutine that hands the +// file over if anyone is still listening and closes it if not. +func openForWrite(ctx context.Context, path string) (*os.File, error) { + type opened struct { + file *os.File + err error + } + + // Unbuffered, and paired with abandoned rather than a default case. A + // buffered channel would accept the file after the caller had already + // returned, orphaning the descriptor; a default case would close a file the + // caller was about to ask for, if the open won the race to this select. + ch := make(chan opened) + abandoned := make(chan struct{}) + + go func() { + //nolint:gosec // G304: Opening pipe file is intended functionality + file, err := os.OpenFile(path, os.O_WRONLY, 0) + select { + case ch <- opened{file: file, err: err}: + case <-abandoned: + if file != nil { + _ = file.Close() + } + } + }() + + select { + case <-ctx.Done(): + close(abandoned) + return nil, ctx.Err() + case o := <-ch: + return o.file, o.err + } +} + // ReadImagesPipe waits for the endpoint to appear, then reads until the writer // finishes. It returns ctx.Err() if the context is canceled while waiting. func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, error) { @@ -143,17 +209,18 @@ func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, erro // WriteCompletionPipe signals a peer that this stage is done. The returned error // satisfies os.IsNotExist when the peer never published the endpoint, which is // how an absent scanner is detected. -func WriteCompletionPipe(path string) error { - //nolint:gosec // G304: Opening pipe file is intended functionality - file, err := os.OpenFile(path, os.O_WRONLY, 0) - if err != nil { +func WriteCompletionPipe(ctx context.Context, path string) error { + // Checked before the open so that an absent peer is reported as such even + // when ctx is already done; otherwise a terminating remover could mistake a + // disabled scanner for a cancellation, and vice versa. + if _, err := os.Stat(path); err != nil { return err } - _, err = file.WriteString(EraseCompleteMessage) - if closeErr := file.Close(); closeErr != nil && err == nil { - err = closeErr + file, err := openForWrite(ctx, path) + if err != nil { + return err } - return err + return writeAndClose(ctx, file, []byte(EraseCompleteMessage)) } diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index 0323da79dc..28cb3d5820 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -73,26 +73,57 @@ func (p *CompletionPipe) Close() error { return l.Close() } -// WriteImagesPipe blocks until the reader is listening, then sends the list. -// The unbounded retry mirrors the Unix implementation, where opening a FIFO for -// writing blocks until a reader arrives. -func WriteImagesPipe(path string, images []unversioned.Image) error { +// WriteImagesPipe blocks until the reader is listening, then sends the list, or +// returns ctx.Err() if the context is done first. +func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Image) error { data, err := json.Marshal(images) if err != nil { return err } - conn, err := dialForever(path) + conn, err := dial(ctx, path) if err != nil { return err } - if _, err := conn.Write(data); err != nil { + return sendAndClose(ctx, conn, data) +} + +// sendAndClose writes the payload and closes, which is what frames the message +// for the reader. DialContext only makes connecting cancellable, so the watcher +// covers the write itself. +// +// A single large Write does not appear to block here in practice -- 64 MiB to a +// peer that never reads completed in 11ms, because Windows accepts the whole +// overlapped send regardless of size. The watcher is kept anyway: that is an +// observation about one OS and Go version, not a documented guarantee, and the +// Unix implementation genuinely does block once the pipe buffer fills. The +// contract should not differ between the two. +func sendAndClose(ctx context.Context, conn net.Conn, payload []byte) error { + done := make(chan struct{}) + defer close(done) + + go func() { + select { + case <-ctx.Done(): + _ = conn.Close() + case <-done: + } + }() + + _, err := conn.Write(payload) + + // The watcher may already have closed the connection, which is what surfaced + // as the write error, so the context is checked before the error is trusted. + if ctxErr := ctx.Err(); ctxErr != nil { + _ = conn.Close() + return ctxErr + } + if err != nil { _ = conn.Close() return err } - // closing is what signals end-of-message to the reader return conn.Close() } @@ -141,7 +172,7 @@ func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, erro // WriteCompletionPipe signals a peer that this stage is done. The returned error // satisfies os.IsNotExist when the peer never published the endpoint, which is // how an absent scanner is detected. -func WriteCompletionPipe(path string) error { +func WriteCompletionPipe(ctx context.Context, path string) error { // Dialing a socket that is not there reports connection-refused on Windows // rather than ENOENT, so the filesystem is the only reliable way to tell // "never published" from "published but gone". @@ -149,17 +180,13 @@ func WriteCompletionPipe(path string) error { return err } - conn, err := net.Dial("unix", path) + var d net.Dialer + conn, err := d.DialContext(ctx, "unix", path) if err != nil { return err } - if _, err := conn.Write([]byte(EraseCompleteMessage)); err != nil { - _ = conn.Close() - return err - } - - return conn.Close() + return sendAndClose(ctx, conn, []byte(EraseCompleteMessage)) } func listen(path string) (net.Listener, error) { @@ -167,29 +194,48 @@ func listen(path string) (net.Listener, error) { return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath) } - // a socket left behind by a previous run would fail the bind - if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + // A socket left behind by an unclean exit would fail the bind, so it has to + // go. Anything else at this path is not ours to delete: the worker runs as + // SYSTEM and shares the volume with a scanner image we do not control. + switch fi, err := os.Lstat(path); { + case errors.Is(err, fs.ErrNotExist): + case err != nil: return nil, err + case fi.Mode()&os.ModeSocket == 0: + return nil, fmt.Errorf("refusing to replace %q: it exists and is not a socket", path) + default: + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, err + } } return net.Listen("unix", path) } -// dialForever waits for the reader to start listening. Errors are not -// classified: Windows reports a missing socket as connection-refused, so there -// is no reliable "not yet" error to match on. Retrying unconditionally mirrors -// the Unix implementation, where opening a FIFO for writing blocks until a -// reader arrives. -func dialForever(path string) (net.Conn, error) { +// dial waits for the reader to start listening. Errors are not classified: +// Windows reports a missing socket as connection-refused, so there is no +// reliable "not yet" error to match on. Retrying on a tick mirrors the Unix +// implementation, where opening a FIFO for writing blocks until a reader +// arrives. +func dial(ctx context.Context, path string) (net.Conn, error) { if len(path) > maxSocketPath { return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath) } + ticker := time.NewTicker(time.Second) + defer ticker.Stop() + + var d net.Dialer for { - conn, err := net.Dial("unix", path) + conn, err := d.DialContext(ctx, "unix", path) if err == nil { return conn, nil } - time.Sleep(time.Second) + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-ticker.C: + } } } diff --git a/pkg/utils/platform_unix_test.go b/pkg/utils/platform_unix_test.go index 9d156dc774..98062993e3 100644 --- a/pkg/utils/platform_unix_test.go +++ b/pkg/utils/platform_unix_test.go @@ -10,8 +10,77 @@ import ( "os" "path/filepath" "testing" + "time" + + "github.com/eraser-dev/eraser/api/unversioned" ) +// The rendezvous is not the only place a write can block: once the 64 KiB pipe +// buffer fills, a reader that has opened the FIFO and then stopped draining +// holds the writer in Write, which no open deadline covers. +// +// This is Unix-only on purpose. The same scenario is not reachable through the +// socket implementation: a single Write of 64 MiB to a stalled peer was measured +// completing in 11ms on Windows, because the OS accepts the whole overlapped +// send regardless of size. +func TestWriteImagesPipeHonoursCancellationWhileBlockedOnAStalledReader(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stalled") + + // well past the 64 KiB pipe buffer, so the write cannot simply complete + images := make([]unversioned.Image, 120000) + for i := range images { + images[i] = unversioned.Image{ImageID: fmt.Sprintf("sha256:%060d", i)} + } + + ctx, cancel := context.WithCancel(context.Background()) + + errCh := make(chan error, 1) + go func() { errCh <- WriteImagesPipe(ctx, path, images) }() + + stallReader(t, path) + + // The reader is attached, so the open has returned and the writer has moved + // on to filling the buffer. There is no way to observe "blocked in Write" + // directly, so give it a moment to get there -- otherwise this degrades into + // the rendezvous case already covered in handoff_test.go. + time.Sleep(500 * time.Millisecond) + cancel() + + select { + case err := <-errCh: + if !errors.Is(err, context.Canceled) { + t.Errorf("WriteImagesPipe = %v, want context.Canceled", err) + } + case <-time.After(30 * time.Second): + t.Fatal("WriteImagesPipe ignored cancellation while blocked on a stalled reader") + } +} + +// stallReader opens the FIFO for reading and never reads from it. Opening is +// what releases the writer's blocked open, so the writer proceeds into Write and +// stops once the pipe buffer is full. +func stallReader(t *testing.T, path string) { + t.Helper() + + // the writer creates the FIFO, so it may not exist yet + deadline := time.Now().Add(10 * time.Second) + for { + //nolint:gosec // G304: opening the test's own pipe is the point + f, err := os.OpenFile(path, os.O_RDONLY, 0) + if err == nil { + t.Cleanup(func() { _ = f.Close() }) + return + } + if !os.IsNotExist(err) { + t.Fatalf("open fifo for reading: %v", err) + } + if time.Now().After(deadline) { + t.Fatal("the writer never created the fifo") + } + time.Sleep(10 * time.Millisecond) + } +} + func TestGetAddressAndDialer(t *testing.T) { testCases := []struct { endpoint string diff --git a/pkg/utils/platform_windows_test.go b/pkg/utils/platform_windows_test.go index 4b4a911adb..11244a2b0a 100644 --- a/pkg/utils/platform_windows_test.go +++ b/pkg/utils/platform_windows_test.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "net" "os" "path/filepath" "strings" @@ -79,6 +80,48 @@ func TestSocketPathLimitBoundary(t *testing.T) { } } +// The worker runs as SYSTEM and shares the volume with a scanner image we do +// not control, so an occupied endpoint path is a reason to stop rather than to +// start deleting. +func TestListenRefusesToReplaceANonSocket(t *testing.T) { + dir := shortTempDir(t) + path := filepath.Join(dir, "occupied") + + if err := os.WriteFile(path, []byte("not a socket"), 0o600); err != nil { + t.Fatal(err) + } + + if l, err := listen(path); err == nil { + _ = l.Close() + t.Fatal("listen replaced a regular file, want an error") + } + + if _, err := os.Stat(path); err != nil { + t.Errorf("the file was removed anyway: %v", err) + } +} + +// A socket the previous run failed to clean up must still be reclaimable, +// otherwise a crashed worker would poison the endpoint for every retry. +func TestListenReclaimsAStaleSocket(t *testing.T) { + dir := shortTempDir(t) + path := filepath.Join(dir, "stale") + + stale, err := net.Listen("unix", path) + if err != nil { + t.Fatal(err) + } + // leaks the endpoint on purpose: Close would unlink it and remove the case + // under test + t.Cleanup(func() { _ = stale.Close() }) + + l, err := listen(path) + if err != nil { + t.Fatalf("listen over a stale socket: %v", err) + } + _ = l.Close() +} + func TestMkfifoUnsupported(t *testing.T) { if err := mkfifo("ignored", PipeMode); !errors.Is(err, ErrFifoUnsupported) { t.Errorf("mkfifo on windows = %v, want ErrFifoUnsupported", err) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index f3d4521738..4de028ddef 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -334,9 +334,10 @@ func ReadCollectScanPipe(ctx context.Context) ([]unversioned.Image, error) { } // WriteScanErasePipe is the scanner-facing spelling of WriteImagesPipe, kept -// because custom scanners may call it directly. +// because custom scanners may call it directly. It waits indefinitely; reach +// for WriteImagesPipe when the wait needs to be cancellable. func WriteScanErasePipe(vulnerableImages []unversioned.Image) error { - return WriteImagesPipe(ScanErasePath, vulnerableImages) + return WriteImagesPipe(context.Background(), ScanErasePath, vulnerableImages) } func ProcessRepoDigests(repoDigests []string) ([]string, []error) {