diff --git a/cmd/mesh-control/plan.go b/cmd/mesh-control/plan.go index bc93c4d..738585a 100644 --- a/cmd/mesh-control/plan.go +++ b/cmd/mesh-control/plan.go @@ -257,7 +257,7 @@ func declarationWith(ctx context.Context, open *stores, node string, // per node, so a module running on three machines has three. needed := map[string]map[string]string{} for _, m := range plan.Modules { - for name := range m.Needs { + for name := range m.OwnSecrets { sealed, err := inv.SecretForModule(ctx, node, m.Module, name) if err != nil { return nil, err diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 1b62383..b81b1c2 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -164,7 +164,7 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { }) } } - for _, name := range sortedKeys(m.Needs) { + for _, name := range sortedKeys(m.OwnSecrets) { sealed := with.Needed[m.Module][name] if sealed == "" { // Declared and not made. Refused rather than skipped: a module whose own @@ -174,7 +174,7 @@ func (r Resolution) Declaration(with Rendering) ([]map[string]any, error) { "%s needs a secret called %q and none was made for it", m.Module, name) } first = append(first, map[string]any{ - "id": NeedID(name), "type": "file", "path": m.Needs[name], "sealed": sealed, + "id": NeedID(name), "type": "file", "path": m.OwnSecrets[name], "sealed": sealed, }) } for _, to := range sortedKeys(m.Secrets) { diff --git a/internal/catalogue/filtering_test.go b/internal/catalogue/filtering_test.go index 6a2884e..1d9669d 100644 --- a/internal/catalogue/filtering_test.go +++ b/internal/catalogue/filtering_test.go @@ -276,7 +276,7 @@ func TestWhatTheMeshComputesIsAppliedBeforeWhatTheModuleDeclared(t *testing.T) { func TestAComputedModuleStillGetsWhatTheMeshMadeForIt(t *testing.T) { r := Resolution{Modules: []Manifest{{ Module: "networking", Computed: "mesh-network", - Needs: map[string]string{"key": "/var/lib/mesh/key"}, + OwnSecrets: map[string]string{"key": "/var/lib/mesh/key"}, }}} out, err := r.Declaration(Rendering{ Needed: map[string]map[string]string{"networking": {"key": "sealed"}}, diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 583be20..0ecfb78 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -29,6 +29,16 @@ const ( // // Constrained because these become resource identities, permission patterns and error messages, // and a name that is valid in one and not the others is a fault found late. +// renamed is what a field used to be called, and what it is now. +// +// Kept rather than dropped once the rename is done: a manifest written against the old name is +// refused either way, and the difference is whether whoever wrote it has to go and find out why. +var renamed = map[string]string{ + // `needs` and `secrets` were both name-to-path and differed only in whose secret it was, so + // reaching for the wrong one parsed cleanly and failed somewhere else entirely. + "needs": "own-secrets", +} + var name = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*(\.[a-z0-9][a-z0-9-]*)*$`) // Claim is a singular resource a module takes over. @@ -200,7 +210,13 @@ type Manifest struct { // makes `restart-on` precise. Secrets map[string]string `json:"secrets,omitempty"` - // Needs is a secret this module needs for itself, and where to put it. + // OwnSecrets are secrets this module needs in order to be itself, and where to put them. + // + // **Named for whose they are, not how secret they are.** `secrets` above is a credential for + // reaching something else, keyed by the provision it belongs to. These are keyed by a name the + // module chose and belong to nobody else. Both were `map[string]string` of name to path, and + // the field was called `needs` — so reaching for the wrong one parsed cleanly and failed + // somewhere else entirely, which is the shape of fault this whole design exists to prevent. // // Not tied to a consumer. A database has a superuser password, a broker has an administrator, // a registry has an account — each is a secret the module needs in order to be itself, and @@ -211,7 +227,7 @@ type Manifest struct { // module running on three machines has three passwords and the mesh can read none of them. A // manifest carrying one instead would put the same secret on every machine that ever runs the // module, in a file anybody can read, for ever. - Needs map[string]string `json:"needs,omitempty"` + OwnSecrets map[string]string `json:"own-secrets,omitempty"` // Listens is what this module accepts connections on, and from where. // @@ -404,6 +420,15 @@ func ParseManifest(raw []byte) (Manifest, error) { decoder := json.NewDecoder(bytes.NewReader(raw)) decoder.DisallowUnknownFields() if err := decoder.Decode(&m); err != nil { + // A key that used to mean something says what it became. Refusing a renamed field with + // "unknown field" is correct and unhelpful: whoever wrote it knew what they meant, and + // the mesh knows what it is called now. + for was, is := range renamed { + if strings.Contains(err.Error(), `"`+was+`"`) { + return Manifest{}, fmt.Errorf( + "this manifest says %q, which is now called %q: %w", was, is, err) + } + } return Manifest{}, fmt.Errorf("this is not a module manifest: %w", err) } @@ -539,7 +564,7 @@ func ParseManifest(raw []byte) (Manifest, error) { m.Module, c.Authority)) } } - for name, where := range m.Needs { + for name, where := range m.OwnSecrets { if !strings.HasPrefix(where, "/") { problems = append(problems, fmt.Sprintf( "%s needs %q at %q, which is not an absolute path", m.Module, name, where)) diff --git a/internal/catalogue/needs_test.go b/internal/catalogue/needs_test.go index 903d79c..4fcead8 100644 --- a/internal/catalogue/needs_test.go +++ b/internal/catalogue/needs_test.go @@ -14,7 +14,7 @@ import ( func needy() Manifest { return Manifest{ Module: "postgres", Version: "1", - Needs: map[string]string{"superuser": "/var/lib/mesh/postgres/superuser"}, + OwnSecrets: map[string]string{"superuser": "/var/lib/mesh/postgres/superuser"}, Resources: []map[string]any{ {"id": "store", "type": "container", "name": "mesh-postgres", "image": "postgres@sha256:x"}, }, @@ -62,7 +62,7 @@ func TestADeclaredNeedThatWasNotMadeIsRefused(t *testing.T) { func TestANeedIsAnAbsolutePath(t *testing.T) { _, err := ParseManifest([]byte(`{"module":"postgres","version":"1", - "needs":{"superuser":"superuser.txt"}}`)) + "own-secrets":{"superuser":"superuser.txt"}}`)) if err == nil { t.Fatal("a relative path was accepted") } @@ -74,7 +74,7 @@ func TestANeedIsAnAbsolutePath(t *testing.T) { func TestAModuleMayNeedSeveralThings(t *testing.T) { // A password and a token, say. Telling them apart is the module's business, not the mesh's. m := needy() - m.Needs["replication"] = "/var/lib/mesh/postgres/replication" + m.OwnSecrets["replication"] = "/var/lib/mesh/postgres/replication" got, _ := Resolve(shelf(m), []string{"postgres"}, reachable(), World{}) out, err := got.Declaration(Rendering{Needed: map[string]map[string]string{ "postgres": {"superuser": "b25l", "replication": "dHdv"}, @@ -92,3 +92,33 @@ func TestAModuleMayNeedSeveralThings(t *testing.T) { t.Fatalf("two needs did not land as two secrets: %v", seen) } } + +// A manifest written against the old name is told what the field became. +// +// `needs` and `secrets` were both name-to-path and differed only in whose secret it was, so +// reaching for the wrong one parsed cleanly and failed somewhere else entirely. Refusing the old +// name with "unknown field" would be correct and unhelpful: whoever wrote it knew what they meant, +// and the mesh knows what it is called now. +func TestAManifestUsingTheOldNameIsToldTheNewOne(t *testing.T) { + _, err := ParseManifest([]byte( + `{"module":"postgres","version":"1","needs":{"superuser":"/var/lib/superuser"}}`)) + if err == nil { + t.Fatal("a manifest using the old name was accepted, so two fields now mean one thing") + } + for _, want := range []string{"needs", "own-secrets"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not mention %q: %v", want, err) + } + } +} + +// And an ordinary unknown key is still refused as one, rather than being guessed at. +func TestAnInventedKeyIsNotTreatedAsARename(t *testing.T) { + _, err := ParseManifest([]byte(`{"module":"postgres","version":"1","nonsense":{}}`)) + if err == nil { + t.Fatal("an invented key was accepted") + } + if strings.Contains(err.Error(), "is now called") { + t.Fatalf("an invented key was reported as a rename: %v", err) + } +}