Fix two preflight defects a live production rehearsal found
Ran the rehearsal read-only against a live production instance. It completed, and two preflight checks were wrong in ways a test instance could not have shown. 1. Store-backend detection missed the config entirely. A config generated by `stalwart --init` declares `type = "rocksdb"` inside a `[store.rocksdb]` section, which is what every test fixture here used. The production config has no section headers at all and declares `store.rocksdb.type = "rocksdb"` flat. Detection only matched a bare `type` key, so it reported "no known store backend type found in config" and left topology.store_backend empty. That mattered more than a warning suggests: backup.Run treated an unrecognized backend as a *skip*, so a real run would have continued with no filesystem or database backup at all - the single artifact that phase exists to produce, quietly absent. Flat dotted keys are now detected, and an unrecognized backend is a hard failure rather than a skip. 2. The cluster warning didn't say where it matched. It fires on any occurrence of "cluster" anywhere in the config, which is the correct bias - a missed cluster corrupts a shared store - but on the production config the only match was inside the value of an unrelated setting, leaving a whole config to search to establish that. It now names the location and distinguishes a match in the setting name from one in its value. Both verified against the real config: store-backend now reports "rocksdb (store.rocksdb)", and the cluster warning names the setting, making a false positive dismissible at a glance. No production data is in this commit: the fixtures use example.com and the flat-key shape only. Coverage numbers from that run matched the earlier scrubbed-corpus measurement exactly.
This commit is contained in:
@@ -166,15 +166,21 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi
|
||||
}
|
||||
|
||||
if _, err := runCheck("cluster-config", func() (CheckResult, string) {
|
||||
clustered, err := LooksClustered(c.opts.ConfigPath)
|
||||
mentions, err := ClusterMentions(c.opts.ConfigPath)
|
||||
if err != nil {
|
||||
return CheckResult{Status: StatusFail, Detail: err.Error()}, ""
|
||||
}
|
||||
if clustered {
|
||||
return CheckResult{
|
||||
Status: StatusWarn,
|
||||
Detail: "config mentions clustering - confirm every peer node is stopped before this run proceeds; the tool does not verify this for you",
|
||||
}, ""
|
||||
if len(mentions) > 0 {
|
||||
shown := mentions
|
||||
if len(shown) > 5 {
|
||||
shown = shown[:5]
|
||||
}
|
||||
detail := fmt.Sprintf("config mentions clustering at %s", strings.Join(shown, ", "))
|
||||
if len(mentions) > len(shown) {
|
||||
detail += fmt.Sprintf(" (and %d more)", len(mentions)-len(shown))
|
||||
}
|
||||
detail += " - confirm every peer node is stopped before this run proceeds; the tool does not verify this for you"
|
||||
return CheckResult{Status: StatusWarn, Detail: detail}, ""
|
||||
}
|
||||
return CheckResult{Status: StatusOK, Detail: "no cluster configuration detected"}, ""
|
||||
}); err != nil {
|
||||
|
||||
@@ -17,9 +17,60 @@ import (
|
||||
// false negative is the dangerous direction, so this errs toward matching
|
||||
// broadly rather than requiring an exact schema match.
|
||||
func LooksClustered(configPath string) (bool, error) {
|
||||
locations, err := ClusterMentions(configPath)
|
||||
if err != nil {
|
||||
return false, err
|
||||
}
|
||||
return len(locations) > 0, nil
|
||||
}
|
||||
|
||||
// ClusterMentions returns the setting keys whose name or value mentions
|
||||
// clustering, so a warning can say *where* it matched.
|
||||
//
|
||||
// The matching stays deliberately broad - a missed cluster is the dangerous
|
||||
// direction - but a bare "config mentions clustering" leaves an operator
|
||||
// with a whole config to search. Against a real instance the single match
|
||||
// was inside the value of an unrelated key, which is obvious in one glance
|
||||
// when the warning names it and needs investigation when it doesn't.
|
||||
func ClusterMentions(configPath string) ([]string, error) {
|
||||
data, err := os.ReadFile(configPath)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("preflight: read config %s: %w", configPath, err)
|
||||
return nil, fmt.Errorf("preflight: read config %s: %w", configPath, err)
|
||||
}
|
||||
return strings.Contains(strings.ToLower(string(data)), "cluster"), nil
|
||||
var locations []string
|
||||
section := ""
|
||||
for _, raw := range strings.Split(string(data), "\n") {
|
||||
line := strings.TrimSpace(raw)
|
||||
if line == "" || strings.HasPrefix(line, "#") {
|
||||
continue
|
||||
}
|
||||
if strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]") {
|
||||
section = strings.Trim(line, "[]")
|
||||
// A [cluster] header is itself the declaration; don't let
|
||||
// recording the section swallow the match.
|
||||
if strings.Contains(strings.ToLower(section), "cluster") {
|
||||
locations = append(locations, "["+section+"]")
|
||||
}
|
||||
continue
|
||||
}
|
||||
if !strings.Contains(strings.ToLower(line), "cluster") {
|
||||
continue
|
||||
}
|
||||
key := line
|
||||
if eq := strings.Index(line, "="); eq >= 0 {
|
||||
key = strings.TrimSpace(line[:eq])
|
||||
}
|
||||
where := key
|
||||
if section != "" {
|
||||
where = section + "." + key
|
||||
}
|
||||
// Say whether it was the setting itself or only its value, since
|
||||
// that is the difference between "this is clustered" and "this
|
||||
// mentions the word".
|
||||
if !strings.Contains(strings.ToLower(key), "cluster") {
|
||||
where += " (in its value, not the setting name)"
|
||||
}
|
||||
locations = append(locations, where)
|
||||
}
|
||||
return locations, nil
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ package preflight
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -35,3 +36,28 @@ func TestLooksClustered(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Against a real instance the only match was inside the value of an
|
||||
// unrelated setting. A bare "config mentions clustering" left a whole
|
||||
// config to search; naming the location makes it dismissible at a glance.
|
||||
func TestClusterMentionsSaysWhereItMatched(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "config.toml")
|
||||
body := "server.hostname = \"mail.example.com\"\nconfig.local-keys.08 = \"cluster.*\"\n"
|
||||
if err := os.WriteFile(path, []byte(body), 0o640); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
mentions, err := ClusterMentions(path)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(mentions) != 1 {
|
||||
t.Fatalf("mentions = %v, want one", mentions)
|
||||
}
|
||||
if !strings.Contains(mentions[0], "config.local-keys.08") {
|
||||
t.Errorf("mention = %q, should name the setting", mentions[0])
|
||||
}
|
||||
if !strings.Contains(mentions[0], "in its value") {
|
||||
t.Errorf("mention = %q, should distinguish a value match from a real cluster setting", mentions[0])
|
||||
}
|
||||
}
|
||||
|
||||
@@ -67,8 +67,25 @@ func scanTOMLBackends(data []byte) ([]BackendMatch, error) {
|
||||
}
|
||||
if m := tomlKVRe.FindStringSubmatch(line); m != nil {
|
||||
key, value := m[1], strings.ToLower(m[2])
|
||||
if key == "type" && knownBackends[value] {
|
||||
if !knownBackends[value] {
|
||||
continue
|
||||
}
|
||||
// Two spellings, both real. A config written with sections
|
||||
// declares `type = "rocksdb"` under `[store.rocksdb]`; one
|
||||
// written flat declares `store.rocksdb.type = "rocksdb"` with
|
||||
// no sections at all. A real production instance uses the
|
||||
// second form exclusively - checking only for a bare `type`
|
||||
// key found nothing there, and an undetected backend makes the
|
||||
// backup phase skip the filesystem snapshot entirely.
|
||||
switch {
|
||||
case key == "type":
|
||||
matches = append(matches, BackendMatch{Path: section, Backend: value})
|
||||
case strings.HasSuffix(key, ".type"):
|
||||
path := strings.TrimSuffix(key, ".type")
|
||||
if section != "" {
|
||||
path = section + "." + path
|
||||
}
|
||||
matches = append(matches, BackendMatch{Path: path, Backend: value})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -87,3 +87,53 @@ func TestDetectStoreBackendsNoMatch(t *testing.T) {
|
||||
t.Errorf("got %d matches, want 0: %+v", len(matches), matches)
|
||||
}
|
||||
}
|
||||
|
||||
// A real production config declares its backend with flat dotted keys and
|
||||
// has no section headers at all. Checking only for a bare `type` key found
|
||||
// nothing there, and an undetected backend makes the backup phase skip the
|
||||
// filesystem snapshot - the artifact it exists to produce.
|
||||
func TestDetectStoreBackendsHandlesFlatDottedKeys(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "config.toml")
|
||||
// Shape taken from a real 0.15.5 instance: no [sections] anywhere.
|
||||
body := `storage.blob = "rocksdb"
|
||||
storage.data = "rocksdb"
|
||||
storage.directory = "internal"
|
||||
store.rocksdb.compression = "lz4"
|
||||
store.rocksdb.path = "/opt/stalwart/data"
|
||||
store.rocksdb.type = "rocksdb"
|
||||
directory.internal.type = "internal"
|
||||
`
|
||||
if err := os.WriteFile(path, []byte(body), 0o640); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
matches, err := DetectStoreBackends(path)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(matches) != 1 {
|
||||
t.Fatalf("found %d backend(s), want 1: %+v", len(matches), matches)
|
||||
}
|
||||
if matches[0].Backend != "rocksdb" {
|
||||
t.Errorf("backend = %q, want rocksdb", matches[0].Backend)
|
||||
}
|
||||
if matches[0].Path != "store.rocksdb" {
|
||||
t.Errorf("path = %q, want store.rocksdb", matches[0].Path)
|
||||
}
|
||||
}
|
||||
|
||||
// The sectioned form must keep working; both spellings are real.
|
||||
func TestDetectStoreBackendsStillHandlesSections(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "config.toml")
|
||||
if err := os.WriteFile(path, []byte("[store.rocksdb]\ntype = \"rocksdb\"\npath = \"/var/lib/stalwart\"\n"), 0o640); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
matches, err := DetectStoreBackends(path)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(matches) != 1 || matches[0].Backend != "rocksdb" || matches[0].Path != "store.rocksdb" {
|
||||
t.Errorf("matches = %+v, want one rocksdb at store.rocksdb", matches)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user