diff --git a/checker/registry.go b/checker/registry.go index 2a8af85..03f11c9 100644 --- a/checker/registry.go +++ b/checker/registry.go @@ -27,16 +27,8 @@ var checkerRegistry = map[string]*CheckerDefinition{} // keyed by ObservationKey. var observationProviderRegistry = map[ObservationKey]ObservationProvider{} -// 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). +// RegisterChecker registers a checker definition globally. 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 @@ -46,16 +38,7 @@ 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", @@ -70,15 +53,8 @@ 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) { - 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 + observationProviderRegistry[p.Key()] = p } // GetCheckers returns all registered checker definitions. diff --git a/checker/registry_test.go b/checker/registry_test.go deleted file mode 100644 index bfe33d2..0000000 --- a/checker/registry_test.go +++ /dev/null @@ -1,126 +0,0 @@ -// 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())) - } -} diff --git a/checker/types.go b/checker/types.go index fbbf435..763b897 100644 --- a/checker/types.go +++ b/checker/types.go @@ -155,21 +155,15 @@ 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 ( - 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 + StatusUnknown Status = iota + StatusOK + StatusInfo + StatusWarn + StatusCrit + StatusError ) // String returns the human-readable name of the status. @@ -192,46 +186,6 @@ 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"`