diff --git a/docs/puppet/remote.md b/docs/puppet/remote.md index c3a877c..8c7435e 100644 --- a/docs/puppet/remote.md +++ b/docs/puppet/remote.md @@ -5,6 +5,3 @@ Shared classification, TTL and eviction rules: [caching](../common/caching.md). | Type | Mutable (auto-detected) | Immutable (auto-detected) | |---|---|---| | `puppet` | `v3/modules/*`, `v3/releases*` | `.tar.gz` | - -`/v3/releases` forwards the `module`, `limit` and `offset` query params -upstream; set `base_url` to the Forge API root, e.g. `https://forgeapi.puppet.com`. diff --git a/docs/puppet/virtual.md b/docs/puppet/virtual.md index 80fe520..11266a9 100644 --- a/docs/puppet/virtual.md +++ b/docs/puppet/virtual.md @@ -13,7 +13,7 @@ dependencies resolved across members. - `/v3/releases/-` and `/v3/files/` 302 to the member owning that release. - A member error fails the request with 502; a member without the module is - skipped. Merges are cached for 60s. + skipped. Merges, and modules no member has, are cached for 60s. ```hcl resource "artifactapi_virtual" "puppet" { diff --git a/internal/provider/puppet/puppet.go b/internal/provider/puppet/puppet.go index c2396d8..03bf1d8 100644 --- a/internal/provider/puppet/puppet.go +++ b/internal/provider/puppet/puppet.go @@ -3,7 +3,6 @@ package puppet import ( "context" "net/http" - "strconv" "strings" "git.unkin.net/unkin/artifactapi/internal/auth" @@ -40,32 +39,6 @@ func (p *Provider) UpstreamURL(remote models.Remote, path string) string { return strings.TrimRight(remote.BaseURL, "/") + "/" + strings.TrimLeft(path, "/") } -// ForwardedQuery keeps the module filter and pagination on release listings. -func (p *Provider) ForwardedQuery(path string) map[string]provider.QueryValidator { - if path != "v3/releases" { - return nil - } - return map[string]provider.QueryValidator{"module": moduleParam, "limit": pageLimit, "offset": pageOffset} -} - -func moduleParam(v string) (string, bool) { - v = NormalizeSlug(v) - return v, ValidSlug(v) -} - -func pageLimit(v string) (string, bool) { - n, err := strconv.ParseUint(v, 10, 32) - if err != nil || n == 0 { - return "", false - } - return strconv.FormatUint(min(n, 100), 10), true -} - -func pageOffset(v string) (string, bool) { - n, err := strconv.ParseUint(v, 10, 32) - return strconv.FormatUint(n, 10), err == nil -} - func (p *Provider) RewritesResponse() {} func (p *Provider) RewriteResponse(body []byte, remote models.Remote, proxyBaseURL string) ([]byte, error) { diff --git a/internal/provider/puppet/puppet_test.go b/internal/provider/puppet/puppet_test.go index 1209e82..33b76a3 100644 --- a/internal/provider/puppet/puppet_test.go +++ b/internal/provider/puppet/puppet_test.go @@ -76,28 +76,3 @@ func TestAuthHeaders(t *testing.T) { t.Error("no credentials, no header") } } - -func TestForwardedQuery(t *testing.T) { - p := &Provider{} - if p.ForwardedQuery("v3/files/a-b-1.0.0.tar.gz") != nil || p.ForwardedQuery("v3/modules/a-b") != nil { - t.Fatal("only release listings forward a query") - } - f := p.ForwardedQuery("v3/releases") - for name, c := range map[string]struct { - in, want string - ok bool - }{ - "module": {"Puppetlabs/stdlib", "puppetlabs-stdlib", true}, - "limit": {"500", "100", true}, - "offset": {"40", "40", true}, - } { - if got, ok := f[name](c.in); got != c.want || ok != c.ok { - t.Errorf("%s(%q) = %q %v, want %q %v", name, c.in, got, ok, c.want, c.ok) - } - } - for name, bad := range map[string]string{"module": "../x", "limit": "0", "offset": "-1"} { - if _, ok := f[name](bad); ok { - t.Errorf("%s(%q) accepted", name, bad) - } - } -} diff --git a/internal/virtual/puppet_merger.go b/internal/virtual/puppet_merger.go index 67e183f..941eab9 100644 --- a/internal/virtual/puppet_merger.go +++ b/internal/virtual/puppet_merger.go @@ -34,11 +34,13 @@ type PuppetMember struct { } // PuppetDoc is a module's merged releases, newest version first, with -// file_uri routed via the virtual; Owners maps each version to its member. +// file_uri routed via the virtual; Owners maps each version to its member; +// missing caches that no member has the module. type PuppetDoc struct { Releases []map[string]any Owners map[string]string body []byte + missing bool } func (d *PuppetDoc) index() []byte { return d.body } @@ -114,7 +116,11 @@ func (e *Engine) PuppetReleases(ctx context.Context, virt models.Virtual, slug s if err != nil { return nil, err } - return MergePuppetReleases(slug, members) + d, err := MergePuppetReleases(slug, members) + if errors.Is(err, ErrNotFound) { + return &PuppetDoc{missing: true}, nil + } + return d, err }) if err != nil { return nil, err @@ -122,7 +128,10 @@ func (e *Engine) PuppetReleases(ctx context.Context, virt models.Virtual, slug s if inserted { e.evictOldest(prefix, maxPuppetModules) } - return repo.(*PuppetDoc), nil + if d := repo.(*PuppetDoc); !d.missing { + return d, nil + } + return nil, ErrNotFound } // PuppetRedirect maps v3/releases/- or v3/files/ to the diff --git a/internal/virtual/puppet_merger_test.go b/internal/virtual/puppet_merger_test.go index 4b2c51b..f6dc785 100644 --- a/internal/virtual/puppet_merger_test.go +++ b/internal/virtual/puppet_merger_test.go @@ -180,3 +180,26 @@ func TestPuppetRedirect(t *testing.T) { } } } + +func TestPuppetReleasesCachesMissingModule(t *testing.T) { + var mu sync.Mutex + asked := map[string]int{} + e := puppetEngine(func(r models.Remote, p string) ([]byte, error) { + mu.Lock() + asked[r.Name]++ + mu.Unlock() + if r.Name == "internal" { + return nil, &proxy.ProxyError{Status: 404} + } + return forgePages("unkin-nope", nil, p) + }) + virt := models.Virtual{Name: "puppet", PackageType: models.PackagePuppet, Members: []string{"internal", "forge"}} + for range 2 { + if _, err := e.PuppetReleases(context.Background(), virt, "unkin-nope"); !errors.Is(err, ErrNotFound) { + t.Fatalf("want ErrNotFound, got %v", err) + } + } + if asked["internal"] != 1 || asked["forge"] != 1 { + t.Fatalf("a module no member has must be cached, asked %v", asked) + } +}