diff --git a/cmd/mesh-controller/collect.go b/cmd/mesh-controller/collect.go index c90e98a..5464800 100644 --- a/cmd/mesh-controller/collect.go +++ b/cmd/mesh-controller/collect.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "time" "github.com/novox/mesh-controller/internal/artifacts" "github.com/novox/mesh-controller/internal/inventory" @@ -49,28 +50,43 @@ func collect(ctx context.Context, inv *inventory.Inventory) { return } + // **Bounded, because this runs inside somebody's build.** The first sweep of a mesh that has + // never collected has the whole history to get through, and a person waiting on `build` should + // not pay for it. Two bounds, and what is left over is simply offered again next time — + // builds are frequent, and the point is that the store stops growing, not that it empties + // tonight. + within, stop := context.WithTimeout(ctx, sweepBudget) + defer stop() store := artifacts.Store{Address: address} + var done []string - var refused int - for _, reference := range references { - switch err := store.LetGo(ctx, reference); { - case err == nil, errors.Is(err, artifacts.Gone): + var left int + for i, reference := range references { + if i >= mostPerSweep || within.Err() != nil { + left = len(references) - i + break + } + err := store.LetGo(within, reference) + if err == nil || errors.Is(err, artifacts.Gone) { // Gone is the outcome wanted, already true. Recorded so the next sweep does not ask // again for ever. done = append(done, reference) - default: - refused++ - if refused == 1 { - // Once per sweep. A store that refuses one refuses all of them, and a hundred - // identical lines would bury the reason. - fmt.Fprintf(os.Stderr, "the artifact store kept %s: %v\n", reference, err) - } + continue } + // **Stopped at the first refusal, not pushed through.** A store that refuses one refuses + // all of them — deletion disabled, the store down, the network gone — so going on would + // be a hundred identical failures and a hundred identical log lines in front of whoever + // was building something. + fmt.Fprintf(os.Stderr, "the artifact store kept %s, so nothing more was asked of it: %v\n", + reference, err) + left = len(references) - i + break } + if len(done) > 0 { + // Recorded outside `within`: the deletions happened, and losing the record of them because + // the sweep ran out of budget would mean asking about them again for ever. if err := inv.MarkCollected(ctx, done); err != nil { - // Said, and that is all: the artifacts are gone either way, and the only cost of an - // unrecorded collection is that the next sweep asks about them again. fmt.Fprintf(os.Stderr, "the store let go of %d artifact(s) and the record of it did not keep: %v\n", len(done), err) return @@ -78,7 +94,15 @@ func collect(ctx context.Context, inv *inventory.Inventory) { fmt.Fprintf(os.Stderr, "the artifact store let go of %d artifact(s) the mesh no longer keeps\n", len(done)) } - if refused > 0 { - fmt.Fprintf(os.Stderr, "%d artifact(s) were not collected; the next build asks again\n", refused) + if left > 0 { + fmt.Fprintf(os.Stderr, "%d more to collect; the next build asks again\n", left) } } + +// mostPerSweep is how many artifacts one sweep will ask about. Enough that a mesh building +// several times a day converges within days of this landing; small enough that no single build +// waits on the whole backlog. +const mostPerSweep = 200 + +// sweepBudget is the longest a sweep will keep a build waiting. +const sweepBudget = 60 * time.Second diff --git a/internal/catalogue/consumer_into_serves.go b/internal/catalogue/consumer_into_serves.go index 1a40bfb..897411d 100644 --- a/internal/catalogue/consumer_into_serves.go +++ b/internal/catalogue/consumer_into_serves.go @@ -177,7 +177,7 @@ func sortedAnyKeys(values map[string]any) []string { // choice servedOnThisMachine makes for the consumer's half. Nothing serving it on this machine is // not an error: a contribution can reach a machine whose provider is a record or an adapter, and // then there is nothing derived to tell. -func (r Resolution) derivedFor(provision, as string, settings SettingsBy) (map[string]any, error) { +func (r Resolution) derivedFor(provision, as, consumer, local string, settings SettingsBy) (map[string]any, error) { for _, m := range r.Modules { serves, said := m.Serves[provision] if !said { @@ -195,6 +195,27 @@ func (r Resolution) derivedFor(provision, as string, settings SettingsBy) (map[s if names == nil { return nil, nil } + // **A consumer that keeps several holders of this provision is refused** — this is issue + // 124's own failure one case to the side, and it would be just as quiet. + // + // Each holder gets its own login, `…_` (ADR 0094), and a provider derives from the + // login, so it would make one resource per holder. The consumer's side has no such + // dimension: one binding file per provision, one `${bound::}`, both + // derived from the un-suffixed identity. So the provider would create the holder's + // resource and the consumer would be configured against a name nothing made — it would + // authenticate successfully and be refused on every object, which reads like a credential + // fault and is not one. + // + // Lifting this means giving the consumer's side a local dimension. That is a decision, + // not an omission, and until it is taken the mesh says so rather than guessing. + if local != "" { + return nil, fmt.Errorf( + "%s keeps several holders of %s (this one is %q), and %s derives %s for each "+ + "consumer from the login the mesh minted. Each holder has its own login, and a "+ + "consumer is told one value per requirement — so the two ends would name "+ + "different things and nothing would compare them (novox/hq ADR 0201)", + consumer, local, provision, m.Module, orNothing(sortedAnyKeys(names))) + } settled, err := Settle(names, settings[m.Module]) if err != nil { return nil, fmt.Errorf("%s serving %s: %w", m.Module, provision, err) diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 1e5c78e..b2f651b 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -1307,7 +1307,7 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, continue } as := holderAs(ConsumerIdentity(g.Consumer, IdentitySource(g.Slug, g.From)), g.Local) - derived, err := r.derivedFor(g.Provision, as, settings) + derived, err := r.derivedFor(g.Provision, as, g.From, g.Local, settings) if err != nil { return nil, err } diff --git a/internal/catalogue/derived_for_consumer_test.go b/internal/catalogue/derived_for_consumer_test.go index 20a8da4..5d85206 100644 --- a/internal/catalogue/derived_for_consumer_test.go +++ b/internal/catalogue/derived_for_consumer_test.go @@ -295,3 +295,49 @@ func storeGrants(t *testing.T, out []map[string]any) []Contribution { t.Fatalf("the provider was given no contributions file: %v", out) return nil } + +// A consumer that keeps SEVERAL holders of one provision is refused, rather than told one thing +// while its provider is told another. +// +// **This is issue 124's own failure, one case to the side.** The mesh gives each holder its own +// login — `mesh_node_mod_` (ADR 0094) — and the provider derives from the login, so it +// would make one resource per holder. The consumer's side has no such dimension: there is one +// binding file per provision and one `${bound::}`, both derived from the +// un-suffixed identity. So the provider would create `…-mod-cold` and the consumer would be +// configured against `…-mod`: it would authenticate successfully and be refused on every object, +// which is exactly the fault this whole record exists to end. +// +// Refused, loudly, at the one place that can see both halves. Lifting it means giving the +// consumer's side a local dimension, which is a decision and not an omission. +func TestAConsumerWithSeveralHoldersOfADerivingProviderIsRefused(t *testing.T) { + m := files() + // Two holders of the one provision, the shape ADR 0094 gives a module that keeps several. + m.Secrets = nil + m.SecretsMany = map[string]map[string]string{"s3-bucket": { + "hot": "/var/lib/files/hot.secret", + "cold": "/var/lib/files/cold.secret", + }} + m.Resources = []map[string]any{{ + "id": "env", "type": "file", "path": "/var/lib/files/env", "mode": "0600", + "content": "BUCKET=${bound:s3-bucket:bucket}\n", + }} + r, err := Resolve(shelf(store(), m), []string{"store", "files"}, reachable(), World{}) + if err != nil { + t.Fatal(err) + } + _, err = r.Declaration(Rendering{Grants: []Grant{ + {Provision: "s3-bucket", Consumer: "workstation", From: "files", Slug: "files", + Local: "hot", Values: map[string]any{}, Sealed: "c2VhbGVk"}, + {Provision: "s3-bucket", Consumer: "workstation", From: "files", Slug: "files", + Local: "cold", Values: map[string]any{}, Sealed: "c2VhbGVk"}, + }}) + if err == nil { + t.Fatal("a consumer with several holders of a deriving provider was accepted; " + + "its two ends would have disagreed in silence") + } + for _, want := range []string{"files", "s3-bucket", "bucket"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal does not name %q: %v", want, err) + } + } +}