fix(ingestor): quote config values in client-RX source warnings (#302)

cmd/ingestor/client_rx_sources.go

@@ -89,14 +89,47 @@ 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
 // effective restriction so the gate is visible in the boot log, and warns when
 // 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
 }

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])
+		}
+	})
+}