From e0e5681980a51fa8d261c8c2f23711e384392bcb Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sun, 16 Aug 2026 18:40:49 +0200 Subject: [PATCH] ignore unknown namespaces in ParseStrict for forward compatibility --- CHANGELOG.md | 4 + maintainerconfig/maintainerconfig.go | 109 +++++++++++++++++++--- maintainerconfig/maintainerconfig_test.go | 48 ++++++++-- 3 files changed, 140 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17af84a..24098bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- `maintainerconfig`: `ParseStrict` now ignores unknown top-level namespaces instead of rejecting them, so a repo adopting a newer bot's namespace no longer breaks binaries built before that namespace existed. Typos INSIDE a known namespace stay fatal, which is the property `ParseStrict` exists for. Fixes a silent prod wedge: adding `goUpdate:` to two repos made the deployed github-releaser-agent fail planning with `field goUpdate not found`, clearing the task assignee so the release never tagged and never retried. Trade-off: a misspelled namespace is now indistinguishable from a newer one, so both are ignored and logged at WARNING. + ## v0.48.1 - `maintainerconfig`: add `goUpdate.autoUpdate` bool to a new `GoUpdateConfig`, following the existing `ReleaseConfig`/`PrReviewerConfig` shape — the per-repo consent flag the upcoming github-update-go-watcher gates on. Defaults false (key, section, or file absent all read false). Schema-only in this repo; the watcher itself ships as a follow-up. diff --git a/maintainerconfig/maintainerconfig.go b/maintainerconfig/maintainerconfig.go index 6902d87..795e346 100644 --- a/maintainerconfig/maintainerconfig.go +++ b/maintainerconfig/maintainerconfig.go @@ -17,16 +17,24 @@ // MaintainerConfig — every consumer imports this one type, so there is // never a divergent copy of the file's shape. // -// Unknown fields (top-level OR nested) are REJECTED at parse time -// (yaml.NewDecoder + KnownFields(true)). This catches typos like -// `changelogRwrite` or `prRevierer` that would otherwise produce a -// silent default-false config — a high-trust .maintainer.yaml is -// load-bearing for release gating, so a typo must fail loudly. To -// add a new bot's namespace, extend MaintainerConfig with the new -// field FIRST (one PR), then deploy the bot (next PR); the brief -// window between the two is the only time a forward-incompat -// .maintainer.yaml would error, and it errors loudly rather than -// silently downgrading. +// Typos in a KNOWN namespace are REJECTED by ParseStrict (`changelogRwrite` +// inside `release:`), because a high-trust .maintainer.yaml is load-bearing for +// release gating and a typo must fail loudly rather than produce a silent +// default-false config. +// +// UNKNOWN top-level namespaces are IGNORED, even by ParseStrict. This is +// forward compatibility, and it is not optional: one schema is read by several +// independently-deployed binaries, so a repo adopting a new bot's namespace +// must not break the bots that have not been rebuilt yet. +// +// This package previously rejected unknown top-level keys too, on the +// assumption that "add the field, then deploy the bot" left only a brief +// incompatible window. It does not. The window lasts until every consumer is +// rebuilt AND redeployed, and until then the failure is severe and quiet: +// on 2026-08-16, adding `goUpdate:` to two repos made the deployed +// github-releaser-agent fail its planning step with +// `field goUpdate not found`, which cleared the task's assignee and wedged the +// release. No tag, no retry, no alert — the repo simply stopped releasing. // // Parse does NO I/O — fetching the bytes is each consumer's job (the // watcher fetches via the GitHub API; the agent reads the cloned workDir @@ -36,8 +44,11 @@ package maintainerconfig import ( "bytes" "context" + "reflect" + "strings" "github.com/bborbe/errors" + "github.com/golang/glog" "gopkg.in/yaml.v3" ) @@ -131,10 +142,16 @@ func Parse(ctx context.Context, content []byte) (MaintainerConfig, error) { } // ParseStrict unmarshals a `.maintainer.yaml` document with `KnownFields(true)` -// so any unrecognized top-level or nested key produces a wrapped error. -// Use this when the caller wants typos like `changelogRwrite` to fail loudly -// (e.g. the github-releaser planning step where a silent zero-value would -// disable the rewrite pipeline without operator signal). +// applied to the namespaces this binary knows, so an unrecognized key INSIDE a +// known namespace produces a wrapped error. Use this when the caller wants +// typos like `changelogRwrite` to fail loudly (e.g. the github-releaser +// planning step where a silent zero-value would disable the rewrite pipeline +// without operator signal). +// +// Unknown top-level namespaces are ignored rather than rejected — see the +// package doc for why that is required, and for the failure it prevents. The +// cost is that a misspelled namespace is indistinguishable from a newer one; +// both are ignored, and both are logged at WARNING. // // The lib's lenient Parse remains the default for fleet readers (watcher). func ParseStrict(ctx context.Context, content []byte) (MaintainerConfig, error) { @@ -154,6 +171,19 @@ func parseInternal( // short-circuit keeps the contract crisp. return cfg, nil } + if strict { + // Drop namespaces this binary does not know about BEFORE the strict + // decode. KnownFields(true) cannot distinguish "typo inside release:" + // from "namespace added by a newer schema", and conflating those makes + // every additive schema change a fleet-wide outage. Filtering first + // keeps typos inside known namespaces fatal, which is the property + // ParseStrict exists for. + filtered, err := dropUnknownNamespaces(ctx, content) + if err != nil { + return MaintainerConfig{}, err + } + content = filtered + } dec := yaml.NewDecoder(bytes.NewReader(content)) if strict { dec.KnownFields(true) @@ -163,3 +193,54 @@ func parseInternal( } return cfg, nil } + +// dropUnknownNamespaces removes top-level keys that MaintainerConfig does not +// declare, so a document written against a newer schema still parses here. +// Values are round-tripped as yaml.Node, which preserves the nested content +// verbatim for the strict decode that follows. +func dropUnknownNamespaces(ctx context.Context, content []byte) ([]byte, error) { + var raw map[string]yaml.Node + if err := yaml.Unmarshal(content, &raw); err != nil { + return nil, errors.Wrap(ctx, err, "unmarshal .maintainer.yaml") + } + known := knownNamespaces() + for key := range raw { + if _, ok := known[key]; ok { + continue + } + // Warning, not V(2). This is the one downside of tolerating unknown + // namespaces: a misspelled one (`prRevierer:`) is now indistinguishable + // from a genuinely newer one, so neither fails the parse. Logging it + // loudly is what keeps a typo discoverable. Volume is low — only the + // strict path filters, and that runs once per release, not per fleet + // scan (the watcher uses lenient Parse, which never reaches here). + glog.Warningf( + "ignoring unknown .maintainer.yaml namespace %q — either a newer schema than this binary, or a typo", + key, + ) + delete(raw, key) + } + data, err := yaml.Marshal(raw) + if err != nil { + return nil, errors.Wrap(ctx, err, "marshal filtered .maintainer.yaml") + } + return data, nil +} + +// knownNamespaces reads the yaml tags off MaintainerConfig rather than +// hardcoding a list, so adding a namespace stays a one-field edit. +func knownNamespaces() map[string]struct{} { + t := reflect.TypeOf(MaintainerConfig{}) + out := make(map[string]struct{}, t.NumField()) + for i := 0; i < t.NumField(); i++ { + tag := t.Field(i).Tag.Get("yaml") + if tag == "" || tag == "-" { + continue + } + name, _, _ := strings.Cut(tag, ",") + if name != "" { + out[name] = struct{}{} + } + } + return out +} diff --git a/maintainerconfig/maintainerconfig_test.go b/maintainerconfig/maintainerconfig_test.go index 4c581e3..eaf715b 100644 --- a/maintainerconfig/maintainerconfig_test.go +++ b/maintainerconfig/maintainerconfig_test.go @@ -182,13 +182,41 @@ var _ = Describe("Parse", func() { Expect(cfg.PrReviewer.AutoApprove).To(BeTrue()) }) - It("ParseStrict rejects unknown top-level field", func() { - _, err := maintainerconfig.ParseStrict( + It("ParseStrict ignores an unknown top-level namespace and still reads known ones", func() { + // Forward compatibility. A repo may adopt a namespace belonging to a + // bot this binary predates; that must not fail the parse, because the + // binary reading it may not be rebuilt for days. Rejecting this wedged + // two repos' releases on 2026-08-16 when `goUpdate:` was introduced. + cfg, err := maintainerconfig.ParseStrict( ctx, []byte("build-fix:\n enabled: true\nprReviewer:\n autoApprove: true\n"), ) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.PrReviewer.AutoApprove).To(BeTrue()) + }) + + It("ParseStrict ignores the goUpdate namespace on a binary that predates it", func() { + // The exact document that broke github-releaser-agent in prod. + cfg, err := maintainerconfig.ParseStrict( + ctx, + []byte( + "release:\n autoRelease: true\n changelogRewrite: false\nprReviewer:\n autoApprove: true\ngoUpdate:\n autoUpdate: true\n", + ), + ) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.Release.AutoRelease).To(BeTrue()) + Expect(cfg.PrReviewer.AutoApprove).To(BeTrue()) + }) + + It("ParseStrict still rejects a typo inside a known namespace", func() { + // The property ParseStrict exists for must survive the change above: + // forward compatibility applies to unknown NAMESPACES, never to + // unknown keys inside a namespace this binary owns. + _, err := maintainerconfig.ParseStrict( + ctx, + []byte("build-fix:\n enabled: true\nrelease:\n autoReleese: true\n"), + ) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("unmarshal .maintainer.yaml")) Expect(err.Error()).To(ContainSubstring("not found")) }) @@ -212,13 +240,19 @@ var _ = Describe("Parse", func() { Expect(cfg.Release.AllowFork).To(BeTrue()) }) - It("ParseStrict rejects typo in top-level prReviewer key", func() { - _, err := maintainerconfig.ParseStrict( + It("ParseStrict no longer rejects a typo'd top-level namespace", func() { + // The accepted cost of forward compatibility, pinned so it is a + // decision rather than a surprise: a misspelled NAMESPACE is + // indistinguishable from one belonging to a newer bot, so it is + // ignored instead of fatal, and the gate it meant to set stays false. + // It is logged at WARNING so the typo is still discoverable. + // Typos INSIDE a known namespace remain fatal — asserted above. + cfg, err := maintainerconfig.ParseStrict( ctx, []byte("prRevierer:\n autoApprove: true\n"), ) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("unmarshal .maintainer.yaml")) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.PrReviewer.AutoApprove).To(BeFalse()) }) It("release.changelogRewrite: non-bool string value -> wrapped error", func() {