From c1c5ff987c144698ecb733bb787681346040aebc Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 4 Oct 2026 15:56:30 +1100 Subject: [PATCH] Reject bare * and unknown remotes on evict, serialise with fetch lock - return 400 for a bare * and 404 for an unknown remote - take the per-path fetch lock for single-path evicts - pass the evictor to Routes so local routes no longer carry it - test If-None-Match revalidation, ForgetPath/ForgetPrefix glob escaping --- internal/api/v2/local_evict_cleanup_test.go | 2 +- internal/api/v2/local_objects_test.go | 2 +- internal/api/v2/objects.go | 25 ++++--- internal/api/v2/objects_evict_test.go | 38 ++++++++--- internal/cache/cache_test.go | 69 +++++++++++++++++++ internal/proxy/engine.go | 32 ++++++++- internal/proxy/evict_test.go | 74 +++++++++++++++++++-- internal/server/server.go | 8 +-- 8 files changed, 218 insertions(+), 32 deletions(-) diff --git a/internal/api/v2/local_evict_cleanup_test.go b/internal/api/v2/local_evict_cleanup_test.go index dc4d0f2..a6d493d 100644 --- a/internal/api/v2/local_evict_cleanup_test.go +++ b/internal/api/v2/local_evict_cleanup_test.go @@ -49,7 +49,7 @@ func TestLocalEvictCleansRPMMetadata(t *testing.T) { t.Fatal(err) } - h := NewObjectsHandler(db, nil) + h := NewObjectsHandler(db) router := chi.NewRouter() router.Route("/locals/{name}/objects", func(r chi.Router) { r.Delete("/*", h.LocalRoutes().ServeHTTP) diff --git a/internal/api/v2/local_objects_test.go b/internal/api/v2/local_objects_test.go index e908f18..9f0d029 100644 --- a/internal/api/v2/local_objects_test.go +++ b/internal/api/v2/local_objects_test.go @@ -40,7 +40,7 @@ func TestLocalObjectsListing(t *testing.T) { t.Fatal(err) } - h := NewObjectsHandler(db, nil) + h := NewObjectsHandler(db) router := chi.NewRouter() router.Route("/locals/{name}/objects", func(r chi.Router) { r.Get("/", h.LocalRoutes().ServeHTTP) diff --git a/internal/api/v2/objects.go b/internal/api/v2/objects.go index 361a628..7d66451 100644 --- a/internal/api/v2/objects.go +++ b/internal/api/v2/objects.go @@ -2,6 +2,7 @@ package v2 import ( "context" + "errors" "fmt" "net/http" "strconv" @@ -9,6 +10,7 @@ import ( "github.com/go-chi/chi/v5" "git.unkin.net/unkin/artifactapi/internal/database" + "git.unkin.net/unkin/artifactapi/internal/proxy" ) // Evictor drops a remote path from every cache layer. @@ -17,18 +19,20 @@ type Evictor interface { } type ObjectsHandler struct { - db *database.DB - evictor Evictor + db *database.DB } -func NewObjectsHandler(db *database.DB, evictor Evictor) *ObjectsHandler { - return &ObjectsHandler{db: db, evictor: evictor} +func NewObjectsHandler(db *database.DB) *ObjectsHandler { + return &ObjectsHandler{db: db} } -func (h *ObjectsHandler) Routes() chi.Router { +// Routes lists and evicts objects for remote repos; evictor serves the DELETE. +func (h *ObjectsHandler) Routes(evictor Evictor) chi.Router { r := chi.NewRouter() r.Get("/", h.list) - r.Delete("/*", h.evict) + r.Delete("/*", func(w http.ResponseWriter, r *http.Request) { + evict(w, r, evictor) + }) return r } @@ -89,11 +93,16 @@ func (h *ObjectsHandler) evictLocal(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } -func (h *ObjectsHandler) evict(w http.ResponseWriter, r *http.Request) { +func evict(w http.ResponseWriter, r *http.Request, evictor Evictor) { remoteName := chi.URLParam(r, "name") path := chi.URLParam(r, "*") - if err := h.evictor.Evict(r.Context(), remoteName, path); err != nil { + if err := evictor.Evict(r.Context(), remoteName, path); err != nil { + var proxyErr *proxy.ProxyError + if errors.As(err, &proxyErr) { + http.Error(w, proxyErr.Message, proxyErr.Status) + return + } http.Error(w, fmt.Sprintf("evict failed: %v", err), http.StatusInternalServerError) return } diff --git a/internal/api/v2/objects_evict_test.go b/internal/api/v2/objects_evict_test.go index b06c9f4..872c8a4 100644 --- a/internal/api/v2/objects_evict_test.go +++ b/internal/api/v2/objects_evict_test.go @@ -2,31 +2,47 @@ package v2 import ( "context" + "net/http" "net/http/httptest" "testing" "github.com/go-chi/chi/v5" + + "git.unkin.net/unkin/artifactapi/internal/proxy" ) -type fakeEvictor struct{ remote, path string } +type fakeEvictor struct { + remote, path string + err error +} func (f *fakeEvictor) Evict(_ context.Context, remote, path string) error { f.remote, f.path = remote, path - return nil + return f.err +} + +func deleteObject(ev Evictor, path string) int { + router := chi.NewRouter() + router.Route("/remotes/{name}/objects", func(r chi.Router) { + r.Delete("/*", NewObjectsHandler(nil).Routes(ev).ServeHTTP) + }) + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("DELETE", "/remotes/epel/objects/"+path, nil)) + return w.Code } func TestRemoteEvictDelegatesToEvictor(t *testing.T) { for _, path := range []string{"8/Everything/x86_64/repodata/repomd.xml", "8/Everything/x86_64/repodata/*"} { ev := &fakeEvictor{} - h := NewObjectsHandler(nil, ev) - router := chi.NewRouter() - router.Route("/remotes/{name}/objects", func(r chi.Router) { - r.Delete("/*", h.Routes().ServeHTTP) - }) - w := httptest.NewRecorder() - router.ServeHTTP(w, httptest.NewRequest("DELETE", "/remotes/epel/objects/"+path, nil)) - if w.Code != 204 || ev.remote != "epel" || ev.path != path { - t.Errorf("DELETE %s: code=%d evicted=%q/%q", path, w.Code, ev.remote, ev.path) + if code := deleteObject(ev, path); code != 204 || ev.remote != "epel" || ev.path != path { + t.Errorf("DELETE %s: code=%d evicted=%q/%q", path, code, ev.remote, ev.path) } } } + +func TestRemoteEvictMapsProxyErrorStatus(t *testing.T) { + ev := &fakeEvictor{err: &proxy.ProxyError{Status: http.StatusNotFound, Message: "remote not found"}} + if code := deleteObject(ev, "x"); code != http.StatusNotFound { + t.Errorf("code = %d, want 404", code) + } +} diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 5edbf41..4093a60 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -131,3 +131,72 @@ func TestFlushRemote(t *testing.T) { t.Error("expected keys flushed") } } + +func setPathKeys(t *testing.T, remote string, paths ...string) { + t.Helper() + ctx := context.Background() + for _, p := range paths { + if err := testRedis.SetTTL(ctx, remote, p, time.Minute); err != nil { + t.Fatal(err) + } + if err := testRedis.SetETag(ctx, remote, p, `"e"`, time.Minute); err != nil { + t.Fatal(err) + } + } +} + +func pathKeysExist(t *testing.T, remote, path string) (ttl, etag bool) { + t.Helper() + ctx := context.Background() + ttl, _ = testRedis.CheckTTL(ctx, remote, path) + e, _ := testRedis.GetETag(ctx, remote, path) + return ttl, e != "" +} + +func TestForgetPath(t *testing.T) { + requireRedis(t) + const meta = `repo/a*b?[c]\d.xml` + setPathKeys(t, "fp", meta, "repo/aXb.xml") + setPathKeys(t, "fp-other", meta) + + if err := testRedis.ForgetPath(context.Background(), "fp", meta); err != nil { + t.Fatal(err) + } + if ttl, etag := pathKeysExist(t, "fp", meta); ttl || etag { + t.Errorf("forgotten path keys remain: ttl=%v etag=%v", ttl, etag) + } + for _, k := range [][2]string{{"fp", "repo/aXb.xml"}, {"fp-other", meta}} { + if ttl, etag := pathKeysExist(t, k[0], k[1]); !ttl || !etag { + t.Errorf("%s:%s lost keys: ttl=%v etag=%v", k[0], k[1], ttl, etag) + } + } +} + +func TestForgetPrefix(t *testing.T) { + requireRedis(t) + const prefix = `r*[1]?\/` + under := []string{prefix + "repomd.xml", prefix + "sub/x.rpm"} + // Each would match the prefix if its glob metacharacters were left unescaped. + globMatches := []string{`rX11/repomd.xml`, `r[1]?\/x`, `r*1Z/x`} + setPathKeys(t, "fx", append(under, globMatches...)...) + setPathKeys(t, "fx-other", under...) + + if err := testRedis.ForgetPrefix(context.Background(), "fx", prefix); err != nil { + t.Fatal(err) + } + for _, p := range under { + if ttl, etag := pathKeysExist(t, "fx", p); ttl || etag { + t.Errorf("%s keys remain: ttl=%v etag=%v", p, ttl, etag) + } + } + for _, p := range globMatches { + if ttl, etag := pathKeysExist(t, "fx", p); !ttl || !etag { + t.Errorf("%s outside prefix lost keys: ttl=%v etag=%v", p, ttl, etag) + } + } + for _, p := range under { + if ttl, etag := pathKeysExist(t, "fx-other", p); !ttl || !etag { + t.Errorf("other remote %s lost keys: ttl=%v etag=%v", p, ttl, etag) + } + } +} diff --git a/internal/proxy/engine.go b/internal/proxy/engine.go index c59af9b..e760962 100644 --- a/internal/proxy/engine.go +++ b/internal/proxy/engine.go @@ -16,6 +16,8 @@ import ( "sync/atomic" "time" + "github.com/jackc/pgx/v5" + "git.unkin.net/unkin/artifactapi/internal/cache" "git.unkin.net/unkin/artifactapi/internal/database" "git.unkin.net/unkin/artifactapi/internal/provider" @@ -210,10 +212,21 @@ func (e *Engine) Fetch(ctx context.Context, remote models.Remote, path string, p // Evict drops path from every cache layer (artifact row, index object, Redis // freshness and ETag keys) so the next request refetches from upstream. A -// trailing "*" evicts every path under that prefix. +// trailing "*" evicts every path under that prefix; a bare "*" is rejected. func (e *Engine) Evict(ctx context.Context, remoteName, path string) error { prefix, wildcard := strings.CutSuffix(path, "*") + if wildcard && prefix == "" { + return &ProxyError{Status: http.StatusBadRequest, Message: "refusing to evict an entire remote"} + } + if _, err := e.db.GetRemote(ctx, remoteName); errors.Is(err, pgx.ErrNoRows) { + return &ProxyError{Status: http.StatusNotFound, Message: fmt.Sprintf("remote %q not found", remoteName)} + } else if err != nil { + return fmt.Errorf("get remote: %w", err) + } if !wildcard { + if e.waitForLock(ctx, remoteName, path) { + defer e.cache.ReleaseLock(ctx, remoteName, path) + } if err := e.db.DeleteArtifact(ctx, remoteName, path); err != nil { return fmt.Errorf("delete artifact: %w", err) } @@ -231,6 +244,23 @@ func (e *Engine) Evict(ctx context.Context, remoteName, path string) error { return e.cache.ForgetPrefix(ctx, remoteName, prefix) } +// waitForLock takes the per-path fetch lock so an in-flight Fetch cannot +// re-set TTL/ETag keys after an evict. It gives up once the lock would have +// expired anyway, or when ctx ends. +func (e *Engine) waitForLock(ctx context.Context, remoteName, path string) bool { + deadline := time.Now().Add(fetchLockTTL) + for { + if ok, err := e.cache.AcquireLock(ctx, remoteName, path, fetchLockTTL); ok || err != nil || time.Now().After(deadline) { + return ok + } + select { + case <-ctx.Done(): + return false + case <-time.After(50 * time.Millisecond): + } + } +} + // HeadResult carries artifact metadata for a HEAD request. There is no body. type HeadResult struct { ContentType string diff --git a/internal/proxy/evict_test.go b/internal/proxy/evict_test.go index fc5f5dd..c27e47a 100644 --- a/internal/proxy/evict_test.go +++ b/internal/proxy/evict_test.go @@ -2,28 +2,40 @@ package proxy import ( "context" + "errors" "net/http" "net/http/httptest" "sync/atomic" "testing" + "time" _ "git.unkin.net/unkin/artifactapi/internal/provider/rpm" "git.unkin.net/unkin/artifactapi/pkg/models" ) // changingUpstream serves every path with the current revision, as a mirror -// does after a sync replaces its repodata. -func changingUpstream(t *testing.T) (*httptest.Server, *atomic.Value) { +// does after a sync replaces its repodata. It answers 304 to a matching +// If-None-Match and counts conditional requests. +func changingUpstream(t *testing.T) (*httptest.Server, *atomic.Value, *atomic.Int32) { t.Helper() var rev atomic.Value + var conditional atomic.Int32 rev.Store("rev1") srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { v := rev.Load().(string) - w.Header().Set("ETag", `"`+v+`"`) + etag := `"` + v + `"` + w.Header().Set("ETag", etag) + if inm := r.Header.Get("If-None-Match"); inm != "" { + conditional.Add(1) + if inm == etag { + w.WriteHeader(http.StatusNotModified) + return + } + } _, _ = w.Write([]byte(v + ":" + r.URL.Path)) })) t.Cleanup(srv.Close) - return srv, &rev + return srv, &rev, &conditional } func fetchBody(t *testing.T, r models.Remote, path string) string { @@ -41,7 +53,7 @@ func rpmRemote(t *testing.T, name, baseURL string) models.Remote { func TestEvictMutableIndexRefetches(t *testing.T) { requireStack(t) - srv, rev := changingUpstream(t) + srv, rev, conditional := changingUpstream(t) r := rpmRemote(t, "evict-idx", srv.URL) const path = "8/Everything/x86_64/repodata/repomd.xml" @@ -55,14 +67,64 @@ func TestEvictMutableIndexRefetches(t *testing.T) { if err := testEngine.Evict(context.Background(), r.Name, path); err != nil { t.Fatalf("evict: %v", err) } + conditional.Store(0) if got := fetchBody(t, r, path); got != "rev2:/"+path { t.Fatalf("after evict = %q, want rev2", got) } + if n := conditional.Load(); n != 0 { + t.Errorf("post-evict fetch revalidated with the evicted ETag (%d conditional requests)", n) + } +} + +func TestEvictRejectsBareWildcard(t *testing.T) { + requireStack(t) + srv, _, _ := changingUpstream(t) + r := rpmRemote(t, "evict-bare", srv.URL) + const path = "8/Everything/x86_64/repodata/repomd.xml" + fetchBody(t, r, path) + + var pe *ProxyError + if err := testEngine.Evict(context.Background(), r.Name, "*"); !errors.As(err, &pe) || pe.Status != http.StatusBadRequest { + t.Fatalf("evict * = %v, want 400", err) + } + if fresh, _ := testEngine.cache.CheckTTL(context.Background(), r.Name, path); !fresh { + t.Error("bare * evicted cached keys") + } +} + +func TestEvictUnknownRemote(t *testing.T) { + requireStack(t) + var pe *ProxyError + if err := testEngine.Evict(context.Background(), "evict-no-such-remote", "a/b"); !errors.As(err, &pe) || pe.Status != http.StatusNotFound { + t.Fatalf("evict unknown remote = %v, want 404", err) + } +} + +func TestEvictWaitsForFetchLock(t *testing.T) { + requireStack(t) + srv, _, _ := changingUpstream(t) + r := rpmRemote(t, "evict-lock", srv.URL) + ctx := context.Background() + const path = "8/Everything/x86_64/repodata/repomd.xml" + if ok, err := testEngine.cache.AcquireLock(ctx, r.Name, path, time.Minute); !ok || err != nil { + t.Fatalf("acquire: %v %v", ok, err) + } + done := make(chan error, 1) + go func() { done <- testEngine.Evict(ctx, r.Name, path) }() + select { + case err := <-done: + t.Fatalf("evict returned while fetch lock held: %v", err) + case <-time.After(200 * time.Millisecond): + } + _ = testEngine.cache.ReleaseLock(ctx, r.Name, path) + if err := <-done; err != nil { + t.Fatalf("evict: %v", err) + } } func TestEvictWildcardClearsPrefixOnly(t *testing.T) { requireStack(t) - srv, rev := changingUpstream(t) + srv, rev, _ := changingUpstream(t) r := rpmRemote(t, "evict-wild", srv.URL) const ( repomd = "8/Everything/x86_64/repodata/repomd.xml" diff --git a/internal/server/server.go b/internal/server/server.go index 85622c5..19e3677 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -195,13 +195,13 @@ func (s *Server) routes() chi.Router { r.Mount("/probe", probeHandler.Routes()) r.Route("/remotes/{name}/objects", func(r chi.Router) { - objHandler := v2.NewObjectsHandler(s.db, s.engine) - r.Get("/", objHandler.Routes().ServeHTTP) - r.Delete("/*", objHandler.Routes().ServeHTTP) + objHandler := v2.NewObjectsHandler(s.db) + r.Get("/", objHandler.Routes(s.engine).ServeHTTP) + r.Delete("/*", objHandler.Routes(s.engine).ServeHTTP) }) r.Route("/locals/{name}/objects", func(r chi.Router) { - objHandler := v2.NewObjectsHandler(s.db, s.engine) + objHandler := v2.NewObjectsHandler(s.db) r.Get("/", objHandler.LocalRoutes().ServeHTTP) r.Delete("/*", objHandler.LocalRoutes().ServeHTTP) })