From 29ac6234699159dcd4bab8adcc900d6578332e3e Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 15:35:22 +0200 Subject: [PATCH 1/2] test(ingestor): pin client-RX source warning details (#302) Red-before tests for the four follow-ups from the review of #290: every config-derived value must be %q-quoted (a value with an embedded newline forges a log line), the ambiguous-name clause separator and match count must be pinned (two mutants survived #290), the warnings must not claim in the present tense that coverage is accepted while clientRxCoverage.enabled is false, and a case-only duplicate in the allowlist must be collapsed before reporting. --- cmd/ingestor/client_rx_sources_302_test.go | 207 +++++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 cmd/ingestor/client_rx_sources_302_test.go diff --git a/cmd/ingestor/client_rx_sources_302_test.go b/cmd/ingestor/client_rx_sources_302_test.go new file mode 100644 index 000000000..8b6e6a660 --- /dev/null +++ b/cmd/ingestor/client_rx_sources_302_test.go @@ -0,0 +1,207 @@ +package main + +import ( + "strconv" + "strings" + "testing" +) + +// Follow-ups to the #290 client-RX source warnings (#302): quote every +// config-derived value, pin the ambiguous-name message shape, keep the +// warnings consistent with the state line while coverage is disabled, and +// collapse a duplicated allowlist entry before reporting it. + +// clientRxLogRecords splits captured output into log records. captureLog sets +// the log flags to 0, so one log.Printf is exactly one line — unless a value +// interpolated into it carries a newline, which is the whole point of N1. +func clientRxLogRecords(out string) []string { + var recs []string + for _, l := range strings.Split(out, "\n") { + if strings.TrimSpace(l) != "" { + recs = append(recs, l) + } + } + return recs +} + +func clientRxWarnings(out string) []string { + var warn []string + for _, l := range clientRxLogRecords(out) { + if strings.Contains(l, "WARNING") { + warn = append(warn, l) + } + } + return warn +} + +// TestCheckClientRxSourcesQuotesConfigValues (N1): the startup lines +// interpolate operator-supplied config values, so an entry with an embedded +// newline could split a warning and forge an extra "[client-rx] …" line. Every +// config-derived value must go through %q, which renders the newline as \n and +// keeps one log.Printf to one line. +func TestCheckClientRxSourcesQuotesConfigValues(t *testing.T) { + const forged = "evil\n[client-rx] WARNING: forged line injected from config" + + t.Run("state line and unknown-name warning", func(t *testing.T) { + cfg := coverageCfgWithSources(forged) + out := captureLog(t, func() { checkClientRxSources(cfg, []MQTTSource{{Name: "auth"}}) }) + + // The state line plus the unknown-name warning: two records, no more. + if recs := clientRxLogRecords(out); len(recs) != 2 { + t.Errorf("expected 2 log records (state + unknown warning), got %d:\n%s", len(recs), out) + } + if strings.Contains(out, forged) { + t.Errorf("the raw config value reached the log unquoted:\n%s", out) + } + if n := strings.Count(out, strconv.Quote(forged)); n != 2 { + t.Errorf("expected the %%q-quoted value in both records, found %d:\n%s", n, out) + } + }) + + t.Run("ambiguous-name warning", func(t *testing.T) { + // Two sources carrying the same odd name, so the entry is ambiguous and + // its own spelling is interpolated into the warning. + cfg := coverageCfgWithSources(forged) + out := captureLog(t, func() { + checkClientRxSources(cfg, []MQTTSource{{Name: forged}, {Name: forged}}) + }) + + if recs := clientRxLogRecords(out); len(recs) != 2 { + t.Errorf("expected 2 log records (state + ambiguous warning), got %d:\n%s", len(recs), out) + } + if strings.Contains(out, forged) { + t.Errorf("the raw config value reached the log unquoted:\n%s", out) + } + }) +} + +// TestCheckClientRxSourcesAmbiguousMessageShape (N2): #290 left two mutants +// alive in this message — the "; " clause separator and the match count, which +// no test distinguished from the literal 2. One entry matching three sources +// and a second matching two pin both. +func TestCheckClientRxSourcesAmbiguousMessageShape(t *testing.T) { + sources := []MQTTSource{ + {Name: "auth"}, {Name: "Auth"}, {Name: " AUTH "}, + {Name: "legacy"}, {Name: "LEGACY"}, + } + cfg := coverageCfgWithSources("auth", "legacy") + + out := captureLog(t, func() { checkClientRxSources(cfg, sources) }) + warn := clientRxWarnings(out) + if len(warn) != 1 { + t.Fatalf("expected exactly one warning line, got %d:\n%s", len(warn), out) + } + + const wantClauses = `"auth" matches 3 sources ("auth", "Auth", " AUTH "); ` + + `"legacy" matches 2 sources ("legacy", "LEGACY")` + if !strings.Contains(warn[0], wantClauses) { + t.Errorf("ambiguous warning must read\n\t%s\ngot\n\t%s", wantClauses, warn[0]) + } +} + +// TestCheckClientRxSourcesDisabledWordingAgrees (N3): with +// clientRxCoverage.enabled false the state line says no coverage is ingested at +// all, so the warnings must not claim in the present tense that coverage is (or +// will never be) accepted. They speak in the conditional instead. +func TestCheckClientRxSourcesDisabledWordingAgrees(t *testing.T) { + // One ambiguous entry and one unknown entry, so both warnings fire. + sources := []MQTTSource{{Name: "auth"}, {Name: "AUTH"}} + + t.Run("disabled", func(t *testing.T) { + cfg := &Config{ClientRxCoverage: &ClientRxCoverageConfig{ + Enabled: false, + Sources: []string{"auth", "typo-broker"}, + }} + out := captureLog(t, func() { checkClientRxSources(cfg, sources) }) + + if !strings.Contains(out, "enabled is false") { + t.Fatalf("the state line must flag the disabled feature:\n%s", out) + } + if len(clientRxWarnings(out)) != 2 { + t.Fatalf("expected both warnings, got:\n%s", out) + } + for _, want := range []string{ + "coverage would be accepted from every one of them", + "coverage would never be accepted for those names", + } { + if !strings.Contains(out, want) { + t.Errorf("disabled coverage must be reported as %q:\n%s", want, out) + } + } + for _, bad := range []string{ + "coverage is accepted from every one of them", + "coverage will never be accepted for those names", + } { + if strings.Contains(out, bad) { + t.Errorf("the state line says no coverage is ingested, so %q contradicts it:\n%s", bad, out) + } + } + }) + + t.Run("enabled", func(t *testing.T) { + cfg := coverageCfgWithSources("auth", "typo-broker") + out := captureLog(t, func() { checkClientRxSources(cfg, sources) }) + + if strings.Contains(out, "enabled is false") { + t.Fatalf("coverage is enabled here:\n%s", out) + } + for _, want := range []string{ + "coverage is accepted from every one of them", + "coverage will never be accepted for those names", + } { + if !strings.Contains(out, want) { + t.Errorf("enabled coverage must be reported as %q:\n%s", want, out) + } + } + if strings.Contains(out, "would") { + t.Errorf("the conditional mood belongs to disabled coverage only:\n%s", out) + } + }) +} + +// TestCheckClientRxSourcesDeduplicatesAllowlist (N4): matching is +// case-insensitive, so ["auth","AUTH"] is one gate written twice — a plausible +// copy/paste. Reporting each spelling repeated the whole clause. The allowlist +// is collapsed case-insensitively before reporting, keeping the first spelling. +func TestCheckClientRxSourcesDeduplicatesAllowlist(t *testing.T) { + t.Run("ambiguous clause once", func(t *testing.T) { + cfg := coverageCfgWithSources("auth", "AUTH", " Auth ") + out := captureLog(t, func() { + checkClientRxSources(cfg, []MQTTSource{{Name: "auth"}, {Name: "Auth"}}) + }) + + warn := clientRxWarnings(out) + if len(warn) != 1 { + t.Fatalf("expected exactly one warning line, got %d:\n%s", len(warn), out) + } + if n := strings.Count(warn[0], " matches "); n != 1 { + t.Errorf("the clause must appear once, got %d:\n%s", n, warn[0]) + } + if strings.Contains(warn[0], `"AUTH"`) { + t.Errorf("only the first spelling is reported:\n%s", warn[0]) + } + // The state line counts distinct names, not config entries. + if !strings.Contains(out, `restricted to 1 MQTT source(s): "auth"`) { + t.Errorf("state line must report one distinct allowlisted name:\n%s", out) + } + }) + + t.Run("unknown name once", func(t *testing.T) { + cfg := coverageCfgWithSources("typo-broker", "TYPO-BROKER") + var unknown []string + out := captureLog(t, func() { + unknown = checkClientRxSources(cfg, []MQTTSource{{Name: "auth"}}) + }) + + if len(unknown) != 1 || unknown[0] != "typo-broker" { + t.Fatalf("unknown names = %q, want [typo-broker]", unknown) + } + warn := clientRxWarnings(out) + if len(warn) != 1 { + t.Fatalf("expected exactly one warning line, got %d:\n%s", len(warn), out) + } + if !strings.Contains(warn[0], `1 clientRxCoverage.sources name(s)`) { + t.Errorf("the duplicate must not be counted twice:\n%s", warn[0]) + } + }) +} From f9e3c49cc1662c27d61f263e35dbd05be08cf700 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 15:35:22 +0200 Subject: [PATCH 2/2] fix(ingestor): quote config values in client-RX source warnings (#302) Quote every config-derived value in the three startup lines with %q, so an allowlist entry with an embedded newline can no longer split a warning and forge an extra [client-rx] record. Collapse case-only duplicates in the allowlist before reporting, so ["auth","AUTH"] no longer repeats its whole clause. While clientRxCoverage.enabled is false the allowlist is inert, so both warnings now speak in the conditional and agree with the state line logged above them. Log text only; the function's return value and the gate it reports are unchanged. --- cmd/ingestor/client_rx_sources.go | 52 ++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/cmd/ingestor/client_rx_sources.go b/cmd/ingestor/client_rx_sources.go index dfbd51fb6..43ff176be 100644 --- a/cmd/ingestor/client_rx_sources.go +++ b/cmd/ingestor/client_rx_sources.go @@ -89,6 +89,39 @@ func (c *Config) warnClientRxSourceDrop(tag, name string, now time.Time) { tag, name, clientRxSourceWarnInterval) } +// quoteNames renders config-derived names with %q. Every value these startup +// lines interpolate comes from the operator's config file, and an entry with an +// embedded newline would otherwise split the line it appears in and forge a +// further "[client-rx] …" record in the boot log (#302). +func quoteNames(names []string) []string { + out := make([]string, len(names)) + for i, name := range names { + out[i] = fmt.Sprintf("%q", name) + } + return out +} + +// dedupeNamesFold collapses names that differ only in case, keeping the first +// spelling. Allowlist matching is case-insensitive, so ["auth","AUTH"] is one +// gate written twice — a plausible copy/paste — and reporting each spelling +// separately only repeats the same clause (#302). +func dedupeNamesFold(names []string) []string { + if len(names) < 2 { + return names + } + seen := make(map[string]struct{}, len(names)) + out := make([]string, 0, len(names)) + for _, name := range names { + key := strings.ToLower(name) + if _, dup := seen[key]; dup { + continue + } + seen[key] = struct{}{} + out = append(out, name) + } + return out +} + // checkClientRxSources reports, once at startup, every clientRxCoverage.sources // entry that matches no configured mqttSources[].name — an allowlist of names // that can never match would silently drop all coverage. It also logs the @@ -96,7 +129,7 @@ func (c *Config) warnClientRxSourceDrop(tag, name string, now time.Time) { // one entry matches more than one configured source (#278). Returns the // unknown names (nil when there is no allowlist or every name matches). func checkClientRxSources(cfg *Config, sources []MQTTSource) []string { - allow := cfg.ClientRxCoverageSources() + allow := dedupeNamesFold(cfg.ClientRxCoverageSources()) if len(allow) == 0 { return nil } @@ -114,23 +147,28 @@ func checkClientRxSources(cfg *Config, sources []MQTTSource) []string { case len(matched) > 1: // mqttSources[].name is not required to be unique and matching is // case-insensitive, so one entry can admit several brokers (#278). - ambiguous = append(ambiguous, fmt.Sprintf("%s matches %d sources (%s)", want, len(matched), strings.Join(matched, ", "))) + ambiguous = append(ambiguous, fmt.Sprintf("%q matches %d sources (%s)", want, len(matched), strings.Join(matched, ", "))) } } state := "" + // Mood of the two warnings. While the feature is off the allowlist is inert, + // so a present-tense claim about coverage would contradict the state line + // logged one line above; the conditional agrees with it instead (#302). + accepted, neverAccepted := "is accepted", "will never be accepted" if !cfg.ClientRxCoverageEnabled() { // The allowlist is inert while the feature is off; say so rather than // implying coverage is being ingested from the listed sources. state = " (clientRxCoverage.enabled is false, so no coverage is ingested at all)" + accepted, neverAccepted = "would be accepted", "would never be accepted" } - log.Printf("[client-rx] coverage restricted to %d MQTT source(s): %s%s", len(allow), strings.Join(allow, ", "), state) + log.Printf("[client-rx] coverage restricted to %d MQTT source(s): %s%s", len(allow), strings.Join(quoteNames(allow), ", "), state) if len(unknown) > 0 { - log.Printf("[client-rx] WARNING: %d clientRxCoverage.sources name(s) match no configured mqttSources[].name: %s — coverage will never be accepted for those names; check the spelling", - len(unknown), strings.Join(unknown, ", ")) + log.Printf("[client-rx] WARNING: %d clientRxCoverage.sources name(s) match no configured mqttSources[].name: %s — coverage %s for those names; check the spelling", + len(unknown), strings.Join(quoteNames(unknown), ", "), neverAccepted) } if len(ambiguous) > 0 { - log.Printf("[client-rx] WARNING: clientRxCoverage.sources name(s) match more than one configured mqttSources[].name (case-insensitive): %s — coverage is accepted from every one of them; give each source a unique name", - strings.Join(ambiguous, "; ")) + log.Printf("[client-rx] WARNING: clientRxCoverage.sources name(s) match more than one configured mqttSources[].name (case-insensitive): %s — coverage %s from every one of them; give each source a unique name", + strings.Join(ambiguous, "; "), accepted) } return unknown }