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..2dfda19433 100644 --- a/pkg/remover/remover.go +++ b/pkg/remover/remover.go @@ -106,16 +106,29 @@ func main() { log.Info("no images to exclude") } - removed, err := removeImages(client, imagelist) + // Registering the handler suppresses the default SIGTERM exit, so it starts + // only here: once the peer publishes its endpoint the read above blocks in a + // call no context can interrupt, and covering it would swallow the signal + // until SIGKILL. The stop func is discarded rather than deferred because + // every exit path below is os.Exit, which would skip it anyway. + ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + + removed, err := removeImages(ctx, client, imagelist) if err != nil { log.Error(err, "failed to remove images") os.Exit(generalErr) } + // A signal that landed during removal was consumed rather than killing the + // process, and with --imagelist there is no completion write below to report + // it, so an interrupted run would otherwise exit 0 having removed nothing. + if err := ctx.Err(); err != nil { + log.Error(err, "terminating before removal finished", "removed", removed) + os.Exit(generalErr) + } + 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 +136,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..efa80da72b 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") } @@ -85,3 +86,26 @@ func TestRemoveImages(t *testing.T) { }) } } + +// removeImages builds its deadline from the caller rather than Background, so +// that a SIGTERM registered process-wide actually reaches the runtime. Nothing +// above notices if that regresses -- every case passes a context that is never +// done -- so this pins it: a caller who has already gone must not get images +// deleted on their behalf. +func TestRemoveImagesPassesTheCallersContextToTheRuntime(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + client := &testClient{t: t, images: []*v1.Image{{Id: "sha256:aaaa"}}} + + removed, err := removeImages(ctx, client, []string{"sha256:aaaa"}) + if err != nil { + t.Fatalf("removeImages: %v", err) + } + if removed != 0 { + t.Errorf("removed = %d, want 0", removed) + } + if len(client.images) != 1 { + t.Error("the image was deleted for a caller that was already gone") + } +} diff --git a/pkg/remover/test_client_test.go b/pkg/remover/test_client_test.go index 799c93e6a4..7fa6e47b73 100644 --- a/pkg/remover/test_client_test.go +++ b/pkg/remover/test_client_test.go @@ -87,8 +87,12 @@ func (c *testClient) removeImageFromSlice(index int) { c.images = s } -func (c *testClient) DeleteImage(_ context.Context, image string) (err error) { +func (c *testClient) DeleteImage(ctx context.Context, image string) (err error) { c.logf("DeleteImage: %s", image) + // a real CRI client fails the call rather than deleting anyway + if err := ctx.Err(); err != nil { + return err + } if image == "" { return errImageEmpty } 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..48610fca20 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" ) @@ -27,8 +29,23 @@ func shortTempDir(t *testing.T) string { return dir } +// testContext bounds the round trips. Both halves of a handoff block until the +// peer shows up, so on context.Background a rendezvous that never completes +// hangs until the package-wide test timeout -- ten minutes of nothing, with no +// indication of which test is stuck. A deadline turns that into a failure in +// the test that caused it. +func testContext(t *testing.T) context.Context { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + + return ctx +} + func TestImagesHandoffRoundTrip(t *testing.T) { path := filepath.Join(shortTempDir(t), "images") + ctx := testContext(t) want := []unversioned.Image{ {ImageID: "sha256:aaaa", Names: []string{"repo/one:v1"}}, @@ -36,9 +53,9 @@ func TestImagesHandoffRoundTrip(t *testing.T) { } errCh := make(chan error, 1) - go func() { errCh <- WriteImagesPipe(path, want) }() + go func() { errCh <- WriteImagesPipe(ctx, path, want) }() - got, err := ReadImagesPipe(context.Background(), path) + got, err := ReadImagesPipe(ctx, path) if err != nil { t.Fatalf("ReadImagesPipe: %v", err) } @@ -58,6 +75,7 @@ func TestImagesHandoffRoundTrip(t *testing.T) { func TestCompletionHandoffRoundTrip(t *testing.T) { path := filepath.Join(shortTempDir(t), "complete") + ctx := testContext(t) pipe, err := CreateCompletionPipe(path) if err != nil { @@ -66,18 +84,36 @@ func TestCompletionHandoffRoundTrip(t *testing.T) { defer func() { _ = pipe.Close() }() errCh := make(chan error, 1) - go func() { errCh <- WriteCompletionPipe(path) }() + go func() { errCh <- WriteCompletionPipe(ctx, path) }() - data, err := pipe.Await() - if err != nil { - t.Fatalf("Await: %v", err) + type awaited struct { + data []byte + err error + } + + // Await takes no context, so it cannot be given the deadline directly; the + // select is what enforces it. + awaitCh := make(chan awaited, 1) + go func() { + data, err := pipe.Await() + awaitCh <- awaited{data: data, err: err} + }() + + var got awaited + select { + case got = <-awaitCh: + case <-ctx.Done(): + t.Fatalf("Await did not return within the deadline: %v", ctx.Err()) + } + if got.err != nil { + t.Fatalf("Await: %v", got.err) } if err := <-errCh; err != nil { t.Fatalf("WriteCompletionPipe: %v", err) } - if string(data) != EraseCompleteMessage { - t.Errorf("payload = %q, want %q", string(data), EraseCompleteMessage) + if string(got.data) != EraseCompleteMessage { + t.Errorf("payload = %q, want %q", string(got.data), EraseCompleteMessage) } } @@ -86,7 +122,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 +131,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..7d5b36c5ed 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,42 @@ 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{}) + closedByWatcher := make(chan bool, 1) + + go func() { + select { + case <-ctx.Done(): + _ = file.Close() + closedByWatcher <- true + case <-done: + closedByWatcher <- false + } + }() + + _, err := file.Write(payload) + + // Joining the watcher before touching the file again is what makes the rest + // unambiguous: once it has reported, no cancellation close can still land, + // and whoever closed the file is known rather than guessed from the error. + close(done) + if <-closedByWatcher { + return ctx.Err() + } + if closeErr := file.Close(); closeErr != nil && err == nil { err = closeErr } @@ -93,6 +123,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 +212,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..4bb83baf13 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -52,13 +52,29 @@ func CreateCompletionPipe(path string) (*CompletionPipe, error) { // Await blocks until a peer signals completion. The payload is returned // unvalidated so callers keep their existing handling of unexpected content. func (p *CompletionPipe) Await() ([]byte, error) { - conn, err := p.l.Accept() - if err != nil { - return nil, err - } - defer func() { _ = conn.Close() }() + for { + conn, err := p.l.Accept() + if err != nil { + return nil, err + } - return io.ReadAll(conn) + data, err := io.ReadAll(conn) + _ = conn.Close() + if err != nil { + return nil, err + } + + // A connect that says nothing is not the peer: listen probes this + // endpoint to tell a live socket from a stale one, and the volume is + // shared, so the peer is not the only thing that can knock. Accepting + // one of those as the signal would strand the worker that meant to send + // it. + if len(data) == 0 { + continue + } + + return data, nil + } } // Close releases the endpoint, which also unpublishes it. Callers both defer @@ -73,26 +89,61 @@ 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{}) + closedByWatcher := make(chan bool, 1) + + go func() { + select { + case <-ctx.Done(): + _ = conn.Close() + closedByWatcher <- true + case <-done: + closedByWatcher <- false + } + }() + + _, err := conn.Write(payload) + + // Joining the watcher before touching the connection again is what makes the + // rest unambiguous: once it has reported, no cancellation close can still + // land, and whoever closed it is known rather than guessed from the error. + close(done) + if <-closedByWatcher { + return ctx.Err() + } + + if err != nil { _ = conn.Close() return err } - // closing is what signals end-of-message to the reader return conn.Close() } @@ -116,32 +167,39 @@ func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, erro } }() - conn, err := l.Accept() - if err != nil { - if ctxErr := ctx.Err(); ctxErr != nil { - return nil, ctxErr + for { + conn, err := l.Accept() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + return nil, err } - return nil, err - } - defer func() { _ = conn.Close() }() - data, err := io.ReadAll(conn) - if err != nil { - return nil, err - } + data, err := io.ReadAll(conn) + _ = conn.Close() + if err != nil { + return nil, err + } - images := []unversioned.Image{} - if err := json.Unmarshal(data, &images); err != nil { - return nil, err - } + // see Await: a connect that says nothing is not the writer + if len(data) == 0 { + continue + } - return images, nil + images := []unversioned.Image{} + if err := json.Unmarshal(data, &images); err != nil { + return nil, err + } + + return images, nil + } } // 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 +207,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 +221,47 @@ 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) { + // Nothing of ours outlives the pod here: the shared volume is an emptyDir + // created with it, and restartPolicy is Never, so a worker that dies is + // replaced by a new pod with a new volume rather than restarted onto this + // one. Whatever is at this path is therefore not a previous run to clean up, + // and the volume is shared with a scanner image we do not control. Unix + // refuses the same way, because mkfifo returns EEXIST. + switch _, err := os.Lstat(path); { + case errors.Is(err, fs.ErrNotExist): + case err != nil: return nil, err + default: + return nil, fmt.Errorf("refusing to bind %q: something already exists at that path", path) } 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..352aa3ed59 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 @@ -74,7 +143,7 @@ func TestUnixDialerConnects(t *testing.T) { t.Fatalf("getAddressAndDialer: %v", err) } - conn, err := dialer(context.Background(), addr) + conn, err := dialer(testContext(t), addr) if err != nil { t.Fatalf("dial %q: %v", addr, err) } diff --git a/pkg/utils/platform_windows_test.go b/pkg/utils/platform_windows_test.go index 4b4a911adb..5fd6943b9d 100644 --- a/pkg/utils/platform_windows_test.go +++ b/pkg/utils/platform_windows_test.go @@ -3,13 +3,14 @@ package utils import ( - "context" "errors" "fmt" + "net" "os" "path/filepath" "strings" "testing" + "time" "github.com/Microsoft/go-winio" ) @@ -79,6 +80,126 @@ 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) + } + + assertListenRefuses(t, path) +} + +// Nothing of ours outlives the pod at these paths, so both cases below are +// anomalies rather than something to tidy up -- and they are indistinguishable +// on disk anyway, which is why the distinction is no longer attempted. +func TestListenRefusesAnEndpointThatAlreadyExists(t *testing.T) { + t.Run("left behind by a dead listener", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stale") + + // Go unlinks the socket on Close, so the only endpoint left on disk is + // one nobody closed. SetUnlinkOnClose reproduces that without having to + // crash a process: the file stays, the listener does not. + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + if err != nil { + t.Fatal(err) + } + stale.SetUnlinkOnClose(false) + if err := stale.Close(); err != nil { + t.Fatal(err) + } + + assertListenRefuses(t, path) + }) + + t.Run("still being served", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "live") + + live, err := net.Listen("unix", path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = live.Close() }() + + assertListenRefuses(t, path) + }) +} + +func assertListenRefuses(t *testing.T, path string) { + t.Helper() + + if _, err := os.Lstat(path); err != nil { + t.Fatalf("the endpoint should be on disk before the call: %v", err) + } + + if l, err := listen(path); err == nil { + _ = l.Close() + t.Fatal("listen bound over an existing endpoint, want an error") + } + + if _, err := os.Lstat(path); err != nil { + t.Errorf("the endpoint was removed anyway: %v", err) + } +} + +// The endpoints here serve exactly one connection, and the volume is shared +// with a scanner image we do not control, so a connect that says nothing must +// not be mistaken for the peer -- doing so would hand the listener an empty +// payload and leave the real worker with nothing waiting for it. +func TestAwaitIgnoresAConnectThatSaysNothing(t *testing.T) { + path := filepath.Join(shortTempDir(t), "complete") + ctx := testContext(t) + + pipe, err := CreateCompletionPipe(path) + if err != nil { + t.Fatalf("CreateCompletionPipe: %v", err) + } + defer func() { _ = pipe.Close() }() + + // the listener is already published, so this is queued ahead of the peer + probe, err := net.DialTimeout("unix", path, time.Second) + if err != nil { + t.Fatalf("connecting to the endpoint: %v", err) + } + if err := probe.Close(); err != nil { + t.Fatal(err) + } + + errCh := make(chan error, 1) + go func() { errCh <- WriteCompletionPipe(ctx, path) }() + + type awaited struct { + data []byte + err error + } + awaitCh := make(chan awaited, 1) + go func() { + data, err := pipe.Await() + awaitCh <- awaited{data: data, err: err} + }() + + var got awaited + select { + case got = <-awaitCh: + case <-ctx.Done(): + t.Fatalf("Await did not return within the deadline: %v", ctx.Err()) + } + if got.err != nil { + t.Fatalf("Await: %v", got.err) + } + if err := <-errCh; err != nil { + t.Fatalf("WriteCompletionPipe: %v", err) + } + + if string(got.data) != EraseCompleteMessage { + t.Errorf("payload = %q, want %q -- the probe was taken for the peer", string(got.data), EraseCompleteMessage) + } +} + func TestMkfifoUnsupported(t *testing.T) { if err := mkfifo("ignored", PipeMode); !errors.Is(err, ErrFifoUnsupported) { t.Errorf("mkfifo on windows = %v, want ErrFifoUnsupported", err) @@ -104,7 +225,7 @@ func TestNpipeDialerConnects(t *testing.T) { t.Fatalf("getAddressAndDialer: %v", err) } - conn, err := dialer(context.Background(), addr) + conn, err := dialer(testContext(t), addr) if err != nil { t.Fatalf("dial %q: %v", addr, 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) {