From 8e2ba83a0d66ec4235797f66612a970a4000f30d Mon Sep 17 00:00:00 2001 From: Pierre-Olivier Mercier Date: Wed, 8 Apr 2026 20:42:21 +0700 Subject: [PATCH] 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())) + } +}