From 8e2ba83a0d66ec4235797f66612a970a4000f30d Mon Sep 17 00:00:00 2001 From: Pierre-Olivier Mercier Date: Wed, 8 Apr 2026 20:42:21 +0700 Subject: [PATCH 1/2] registry: refuse duplicate registrations with a warning RegisterChecker and RegisterObservationProvider previously overwrote existing entries silently, which lets a misconfigured plugin shadow a built-in (or another plugin) without any signal. Refuse the duplicate and keep the existing entry instead. RegisterExternalizableChecker now performs the dedup check before appending the "endpoint" AdminOpt, so a rejected re-registration no longer mutates the live definition. --- checker/registry.go | 28 ++++++++- checker/registry_test.go | 126 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) create mode 100644 checker/registry_test.go diff --git a/checker/registry.go b/checker/registry.go index 03f11c9..2a8af85 100644 --- a/checker/registry.go +++ b/checker/registry.go @@ -27,8 +27,16 @@ var checkerRegistry = map[string]*CheckerDefinition{} // keyed by ObservationKey. var observationProviderRegistry = map[ObservationKey]ObservationProvider{} -// RegisterChecker registers a checker definition globally. +// RegisterChecker registers a checker definition globally. A second +// registration under the same ID is refused with a warning rather than +// silently overwriting the previous entry: in production this almost +// always indicates a deployment mistake (two plugins shipping the same +// checker, or a plugin shadowing a built-in). func RegisterChecker(c *CheckerDefinition) { + if _, exists := checkerRegistry[c.ID]; exists { + log.Printf("Warning: checker %q is already registered; ignoring duplicate registration", c.ID) + return + } log.Println("Registering new checker:", c.ID) c.BuildRulesInfo() checkerRegistry[c.ID] = c @@ -38,7 +46,16 @@ func RegisterChecker(c *CheckerDefinition) { // delegated to a remote HTTP endpoint. It appends an "endpoint" AdminOpt // so the administrator can optionally configure a remote URL. // When the endpoint is left empty, the checker runs locally as usual. +// +// The duplicate check happens before the AdminOpt append so that a +// rejected second registration does not mutate the in-memory definition +// of the already-registered checker (which a caller might still hold a +// pointer to). func RegisterExternalizableChecker(c *CheckerDefinition) { + if _, exists := checkerRegistry[c.ID]; exists { + log.Printf("Warning: checker %q is already registered; ignoring duplicate registration", c.ID) + return + } c.Options.AdminOpts = append(c.Options.AdminOpts, CheckerOptionDocumentation{ Id: "endpoint", @@ -53,8 +70,15 @@ func RegisterExternalizableChecker(c *CheckerDefinition) { } // RegisterObservationProvider registers an observation provider globally. +// A second registration under the same key is refused with a warning for +// the same reason as RegisterChecker. func RegisterObservationProvider(p ObservationProvider) { - observationProviderRegistry[p.Key()] = p + key := p.Key() + if _, exists := observationProviderRegistry[key]; exists { + log.Printf("Warning: observation provider %q is already registered; ignoring duplicate registration", key) + return + } + observationProviderRegistry[key] = p } // GetCheckers returns all registered checker definitions. diff --git a/checker/registry_test.go b/checker/registry_test.go new file mode 100644 index 0000000..bfe33d2 --- /dev/null +++ b/checker/registry_test.go @@ -0,0 +1,126 @@ +// Copyright 2020-2026 The happyDomain Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package checker + +import ( + "context" + "testing" +) + +// resetRegistries clears the global registries between tests so that one +// test's registration cannot leak into the next. The package-level maps are +// the only shared state. +func resetRegistries() { + checkerRegistry = map[string]*CheckerDefinition{} + observationProviderRegistry = map[ObservationKey]ObservationProvider{} +} + +type stubProvider struct { + key ObservationKey +} + +func (s stubProvider) Key() ObservationKey { return s.key } +func (s stubProvider) Collect(ctx context.Context, opts CheckerOptions) (any, error) { + return nil, nil +} + +func TestRegisterChecker_DuplicateIgnored(t *testing.T) { + resetRegistries() + + first := &CheckerDefinition{ID: "dup", Name: "First"} + second := &CheckerDefinition{ID: "dup", Name: "Second"} + + RegisterChecker(first) + RegisterChecker(second) + + got := FindChecker("dup") + if got == nil { + t.Fatal("expected checker to remain registered after duplicate") + } + if got.Name != "First" { + t.Errorf("duplicate registration overwrote original: got Name=%q, want %q", got.Name, "First") + } + if len(GetCheckers()) != 1 { + t.Errorf("registry has %d entries, want 1", len(GetCheckers())) + } +} + +func TestRegisterExternalizableChecker_AppendsEndpointOnce(t *testing.T) { + resetRegistries() + + c := &CheckerDefinition{ID: "ext", Name: "Ext"} + + RegisterExternalizableChecker(c) + if n := len(c.Options.AdminOpts); n != 1 { + t.Fatalf("first registration: AdminOpts has %d entries, want 1", n) + } + if c.Options.AdminOpts[0].Id != "endpoint" { + t.Errorf("expected first AdminOpt id %q, got %q", "endpoint", c.Options.AdminOpts[0].Id) + } + + // Second registration of the same definition pointer must NOT append a + // second "endpoint" AdminOpt — the duplicate check has to fire before + // the append, otherwise we silently mutate the live definition. + RegisterExternalizableChecker(c) + if n := len(c.Options.AdminOpts); n != 1 { + t.Errorf("after duplicate registration: AdminOpts has %d entries, want 1", n) + } +} + +func TestRegisterExternalizableChecker_DuplicateDifferentPointerIgnored(t *testing.T) { + resetRegistries() + + first := &CheckerDefinition{ID: "ext", Name: "First"} + second := &CheckerDefinition{ID: "ext", Name: "Second"} + + RegisterExternalizableChecker(first) + RegisterExternalizableChecker(second) + + got := FindChecker("ext") + if got == nil || got.Name != "First" { + t.Errorf("expected first registration to win, got %+v", got) + } + // The rejected second definition must not have been mutated either. + if len(second.Options.AdminOpts) != 0 { + t.Errorf("rejected definition was mutated: AdminOpts=%+v", second.Options.AdminOpts) + } +} + +func TestRegisterObservationProvider_DuplicateIgnored(t *testing.T) { + resetRegistries() + + first := stubProvider{key: "dns.A"} + second := stubProvider{key: "dns.A"} + + RegisterObservationProvider(first) + RegisterObservationProvider(second) + + got := FindObservationProvider("dns.A") + if got == nil { + t.Fatal("expected observation provider to remain registered after duplicate") + } + // Identity check: the registry must still hold the first instance. + if _, ok := got.(stubProvider); !ok { + t.Fatalf("unexpected provider type %T", got) + } + if got != ObservationProvider(first) { + // Both stubProvider values compare equal by value, so this also + // guards against an accidental overwrite-then-restore pattern. + t.Errorf("registry no longer holds the first registration") + } + if len(GetObservationProviders()) != 1 { + t.Errorf("registry has %d entries, want 1", len(GetObservationProviders())) + } +} From 6be3578c334ec16fbef1def31f4bd7262edda261 Mon Sep 17 00:00:00 2001 From: Pierre-Olivier Mercier Date: Wed, 8 Apr 2026 22:16:39 +0700 Subject: [PATCH 2/2] checker: reorder Status with negatives for good, JSON as string Make StatusUnknown the zero value (0) so an uninitialized CheckState reads as "no signal yet" rather than as healthy. Push StatusOK and StatusInfo to negative values so the natural int ordering matches severity ordering: aggregators can simply take max() to compute the worst status, and Unknown correctly sits above OK/Info but below Warn. Status now (un)marshals as its string name ("OK", "WARN", ...) so the wire format is stable across any future renumbering. UnmarshalJSON still accepts raw ints for backward compatibility with older snapshots and clients. --- checker/types.go | 58 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/checker/types.go b/checker/types.go index 763b897..fbbf435 100644 --- a/checker/types.go +++ b/checker/types.go @@ -155,15 +155,21 @@ type CheckerOptionsDocumentation struct { } // Status represents the result status of a check evaluation. +// +// Numeric ordering is severity ordering: lower = better, higher = worse. +// StatusUnknown is intentionally the zero value, so an uninitialized +// CheckState reads as "no signal yet" rather than as a healthy OK. +// "Good" statuses are negative so that aggregators can simply take the +// max() of a set of statuses to compute the worst one. type Status int const ( - StatusUnknown Status = iota - StatusOK - StatusInfo - StatusWarn - StatusCrit - StatusError + StatusOK Status = -2 + StatusInfo Status = -1 + StatusUnknown Status = 0 // zero value: not initialized / no signal yet + StatusWarn Status = 1 + StatusCrit Status = 2 + StatusError Status = 3 ) // String returns the human-readable name of the status. @@ -186,6 +192,46 @@ func (s Status) String() string { } } +// MarshalJSON serializes Status as its string name so the wire format +// is stable across any future reordering of the underlying int values. +func (s Status) MarshalJSON() ([]byte, error) { + return json.Marshal(s.String()) +} + +// UnmarshalJSON accepts either the string name (preferred) or a raw int +// (for backward compatibility with older clients/snapshots). +func (s *Status) UnmarshalJSON(data []byte) error { + if len(data) > 0 && data[0] == '"' { + var name string + if err := json.Unmarshal(data, &name); err != nil { + return err + } + switch name { + case "OK": + *s = StatusOK + case "INFO": + *s = StatusInfo + case "UNKNOWN", "": + *s = StatusUnknown + case "WARN": + *s = StatusWarn + case "CRIT": + *s = StatusCrit + case "ERROR": + *s = StatusError + default: + return fmt.Errorf("unknown status %q", name) + } + return nil + } + var n int + if err := json.Unmarshal(data, &n); err != nil { + return err + } + *s = Status(n) + return nil +} + // CheckState is the result of evaluating a single rule. type CheckState struct { Status Status `json:"status"`