fix: gateway pre-flight check could never actually fail
Verified live that after deploying the "fixed" bridge-bound pre-flight, it
still let a known-bad vmbr0+192.168.8.2 config straight through to a full
pct_create with no error. Root cause: the check used
`strings.Contains(pingOut, "REACHABLE")` against markers "REACHABLE" /
"UNREACHABLE" — but "UNREACHABLE" contains "REACHABLE" as a substring, so the
containment check was true for BOTH outcomes. The pre-flight was structurally
incapable of ever failing, regardless of the actual ping result.
Fixed with distinct, non-overlapping markers (PREFLIGHT_OK/PREFLIGHT_FAIL)
and exact-match comparison, pulled into a small gatewayPreflightPassed()
helper with a unit test asserting the exact historical bug case
("UNREACHABLE" must be false) so this bug class can't silently recur.
Re-verified live end-to-end: manually re-tested the exact ping command
(confirmed UNREACHABLE via vmbr0), and this was caught only by actually
running the check against production, not by reading the code.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -88,6 +88,32 @@ func TestJSONErrValidForNastyOutput(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestGatewayPreflightPassed guards the exact bug found live: "UNREACHABLE"
|
||||||
|
// contains "REACHABLE" as a substring, so a strings.Contains(out,"REACHABLE")
|
||||||
|
// check is true for BOTH outcomes and can never fail. Exact-match only.
|
||||||
|
func TestGatewayPreflightPassed(t *testing.T) {
|
||||||
|
cases := []struct {
|
||||||
|
out string
|
||||||
|
want bool
|
||||||
|
}{
|
||||||
|
{"PREFLIGHT_OK", true},
|
||||||
|
{"PREFLIGHT_OK\n", true},
|
||||||
|
{" PREFLIGHT_OK ", true},
|
||||||
|
{"PREFLIGHT_FAIL", false},
|
||||||
|
{"PREFLIGHT_FAIL\n", false},
|
||||||
|
{"", false},
|
||||||
|
{"some garbage output", false},
|
||||||
|
// the specific historical bug: a naive substring check on the old
|
||||||
|
// REACHABLE/UNREACHABLE markers would have called this true.
|
||||||
|
{"UNREACHABLE", false},
|
||||||
|
}
|
||||||
|
for _, c := range cases {
|
||||||
|
if got := gatewayPreflightPassed(c.out); got != c.want {
|
||||||
|
t.Errorf("gatewayPreflightPassed(%q) = %v, want %v", c.out, got, c.want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestProvisionScript(t *testing.T) {
|
func TestProvisionScript(t *testing.T) {
|
||||||
s := provisionScript([]string{"docker.io", "git"}, "echo hi > /root/x")
|
s := provisionScript([]string{"docker.io", "git"}, "echo hi > /root/x")
|
||||||
// Network/DNS gate must come before apt.
|
// Network/DNS gate must come before apt.
|
||||||
|
|||||||
@@ -439,8 +439,8 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
|
|||||||
// actually experience, not what the host's broader routing table can
|
// actually experience, not what the host's broader routing table can
|
||||||
// reach.
|
// reach.
|
||||||
if isStatic && cfg.GW != "" {
|
if isStatic && cfg.GW != "" {
|
||||||
pingOut, pingErr := sshExec(ctx, host, user, fmt.Sprintf("ping -I %s -c1 -W2 %s >/dev/null 2>&1 && echo REACHABLE || echo UNREACHABLE", cfg.Bridge, cfg.GW))
|
pingOut, pingErr := sshExec(ctx, host, user, fmt.Sprintf("ping -I %s -c1 -W2 %s >/dev/null 2>&1 && echo PREFLIGHT_OK || echo PREFLIGHT_FAIL", cfg.Bridge, cfg.GW))
|
||||||
if pingErr != nil || !strings.Contains(pingOut, "REACHABLE") {
|
if pingErr != nil || !gatewayPreflightPassed(pingOut) {
|
||||||
msg := fmt.Sprintf(
|
msg := fmt.Sprintf(
|
||||||
"gateway %s is not reachable from %s on bridge %s — this almost always means the bridge doesn't carry that subnet on this host (each bridge only reaches the network it's physically wired to). "+
|
"gateway %s is not reachable from %s on bridge %s — this almost always means the bridge doesn't carry that subnet on this host (each bridge only reaches the network it's physically wired to). "+
|
||||||
"Do not retry with a different gateway guess in the same subnet: find an existing LXC on this host with an IP in the same /28 and copy its exact bridge+gateway, or use DHCP instead.",
|
"Do not retry with a different gateway guess in the same subnet: find an existing LXC on this host with an IP in the same /28 and copy its exact bridge+gateway, or use DHCP instead.",
|
||||||
@@ -637,6 +637,17 @@ func jsonErr(format string, args ...any) []byte {
|
|||||||
// the host's template cache. Exact match wins; a bare distro hint (e.g.
|
// the host's template cache. Exact match wins; a bare distro hint (e.g.
|
||||||
// "debian-13" or "debian") matches by prefix; empty picks the newest debian
|
// "debian-13" or "debian") matches by prefix; empty picks the newest debian
|
||||||
// (falling back to any) template available. Returns "" when nothing fits.
|
// (falling back to any) template available. Returns "" when nothing fits.
|
||||||
|
// gatewayPreflightPassed interprets the PREFLIGHT_OK/PREFLIGHT_FAIL markers
|
||||||
|
// from the pct_create gateway pre-flight check. Pulled out as its own
|
||||||
|
// function (rather than an inline strings.Contains at the call site) so it's
|
||||||
|
// unit-testable: a prior version checked for "REACHABLE", which is a
|
||||||
|
// substring of "UNREACHABLE" — the check could never actually fail, and it
|
||||||
|
// took a live deployment to notice. Exact-match markers plus a test make
|
||||||
|
// that specific bug class structurally unable to recur silently.
|
||||||
|
func gatewayPreflightPassed(out string) bool {
|
||||||
|
return strings.TrimSpace(out) == "PREFLIGHT_OK"
|
||||||
|
}
|
||||||
|
|
||||||
func resolveTemplate(requested string, available []string) string {
|
func resolveTemplate(requested string, available []string) string {
|
||||||
if len(available) == 0 {
|
if len(available) == 0 {
|
||||||
return ""
|
return ""
|
||||||
|
|||||||
Reference in New Issue
Block a user