diff --git a/TODO.md b/TODO.md index fd593ce..063f159 100644 --- a/TODO.md +++ b/TODO.md @@ -1,13 +1,18 @@ # TODO -## Header validation in service registration +## Response headers don't work with host ACL conditions -A bad header value from a client (e.g., `Access-Control-Allow-Headers: Content-Type, Authorization, ...` with commas/spaces) can produce invalid HAProxy config syntax. When HAProxy config validation fails, the reconciler skips the write — but the bad service persists, so the next reconcile cycle fails identically. This blocks ALL config updates for ALL services until the bad registration is fixed manually. +`http-response set-header ... if host_X` doesn't work because `hdr(host)` is a request-phase fetch that HAProxy can't evaluate in the response phase. The headers are generated but silently ignored (HAProxy logs a WARNING). -### Needed +Fix: capture the host in a transaction variable during the request phase, then use it in response rules: +```haproxy +http-request set-var(txn.host) hdr(host) +acl is_keila var(txn.host) -i keila.cloud.payne.io +http-response set-header Access-Control-Allow-Origin "https://cloud.payne.io" if is_keila +``` -1. **Input validation at registration time** — reject or sanitize header values that would break HAProxy config. HAProxy header values with spaces/commas need quoting; values with special characters (newlines, NULs) should be rejected outright. +Affects: keila (CORS headers), plausible (Private-Network-Access header). -2. **Graceful degradation in reconciliation** — if generating config for one service produces an invalid HAProxy config, skip that service and log a warning rather than failing the entire reconciliation. Approach: generate config, validate, if invalid → bisect to find the offending service → exclude it and regenerate. +## Input validation for header values -3. **Current workaround** — header values are Go-`%q`-quoted in the HAProxy config output. This handles spaces/commas but may produce backslash escapes that HAProxy doesn't understand for edge cases. +Header values with special characters (newlines, NULs) should be rejected at registration time. Currently values are Go-`%q`-quoted in the HAProxy output which handles spaces/commas but may produce backslash escapes that HAProxy doesn't understand for edge cases. diff --git a/internal/api/v1/helpers.go b/internal/api/v1/helpers.go index 2b93600..c01a9de 100644 --- a/internal/api/v1/helpers.go +++ b/internal/api/v1/helpers.go @@ -122,13 +122,51 @@ func (api *API) reconcileNetworking() { } // Generate and write HAProxy config - haproxyCfg := api.haproxy.GenerateWithOpts(instanceRoutes, nil, haproxy.GenerateOpts{ + genOpts := haproxy.GenerateOpts{ HTTPRoutes: activeHTTPRoutes, CertsDir: certsDir, - }) + } + haproxyCfg := api.haproxy.GenerateWithOpts(instanceRoutes, nil, genOpts) if err := api.haproxy.WriteConfig(haproxyCfg); err != nil { - slog.Error("reconcile: failed to write HAProxy config", "error", err) + // Validation failed — identify and exclude broken services, then retry + brokenDomains := haproxy.FindBrokenServices(haproxyCfg, haproxy.ParseValidationErrors(err.Error())) + if len(brokenDomains) > 0 { + slog.Error("reconcile: excluding broken services and retrying", + "broken", brokenDomains, "error", err) + + exclude := map[string]bool{} + for _, d := range brokenDomains { + exclude[d] = true + } + + var filteredInstances []haproxy.InstanceRoute + for _, r := range instanceRoutes { + if !exclude[r.Domain] { + filteredInstances = append(filteredInstances, r) + } + } + var filteredHTTP []haproxy.HTTPRoute + for _, r := range activeHTTPRoutes { + if !exclude[r.Domain] { + filteredHTTP = append(filteredHTTP, r) + } + } + + genOpts.HTTPRoutes = filteredHTTP + haproxyCfg = api.haproxy.GenerateWithOpts(filteredInstances, nil, genOpts) + if err := api.haproxy.WriteConfig(haproxyCfg); err != nil { + slog.Error("reconcile: retry also failed", "error", err) + } else { + if err := api.haproxy.ReloadService(); err != nil { + slog.Warn("reconcile: failed to reload HAProxy", "error", err) + } else { + api.broadcastHaproxyEvent("haproxy:config", "HAProxy config regenerated (excluded broken services)") + } + } + } else { + slog.Error("reconcile: failed to write HAProxy config (no broken services identified)", "error", err) + } } else { if err := api.haproxy.ReloadService(); err != nil { slog.Warn("reconcile: failed to reload HAProxy", "error", err) diff --git a/internal/haproxy/config.go b/internal/haproxy/config.go index 91a5ee9..175c8c4 100644 --- a/internal/haproxy/config.go +++ b/internal/haproxy/config.go @@ -156,6 +156,7 @@ frontend http_in continue // handled below } aclName := "is_l7_" + sanitizeName(route.Name) + fmt.Fprintf(&sb, " # service: %s\n", route.Domain) fmt.Fprintf(&sb, " acl %s req_ssl_sni -m str %s\n", aclName, route.Domain) fmt.Fprintf(&sb, " use_backend be_l7_termination if %s\n", aclName) } @@ -165,6 +166,7 @@ frontend http_in continue } aclName := "is_l7_" + sanitizeName(route.Name) + fmt.Fprintf(&sb, " # service: %s\n", route.Domain) fmt.Fprintf(&sb, " acl %s req_ssl_sni -m end .%s\n", aclName, route.Domain) fmt.Fprintf(&sb, " acl %s req_ssl_sni -m str %s\n", aclName, route.Domain) fmt.Fprintf(&sb, " use_backend be_l7_termination if %s\n", aclName) @@ -180,6 +182,7 @@ frontend http_in } hasExact = true aclName := "is_" + sanitizeName(inst.Name) + fmt.Fprintf(&sb, " # service: %s\n", inst.Domain) fmt.Fprintf(&sb, " acl %s req_ssl_sni -m str %s\n", aclName, inst.Domain) fmt.Fprintf(&sb, " use_backend be_https_%s if %s\n", sanitizeName(inst.Name), aclName) } @@ -194,6 +197,7 @@ frontend http_in continue // already handled in step 2 } aclName := "is_" + sanitizeName(inst.Name) + fmt.Fprintf(&sb, " # service: %s\n", inst.Domain) fmt.Fprintf(&sb, " acl %s req_ssl_sni -m end .%s\n", aclName, inst.Domain) fmt.Fprintf(&sb, " acl %s req_ssl_sni -m str %s\n", aclName, inst.Domain) fmt.Fprintf(&sb, " use_backend be_https_%s if %s\n", sanitizeName(inst.Name), aclName) @@ -209,6 +213,7 @@ frontend http_in // Instance L4 backends for _, inst := range instances { + fmt.Fprintf(&sb, "# service: %s\n", inst.Domain) fmt.Fprintf(&sb, "backend be_https_%s\n", sanitizeName(inst.Name)) sb.WriteString(" mode tcp\n") sb.WriteString(" option tcp-check\n") @@ -239,6 +244,7 @@ frontend http_in // Host ACLs for all services for _, httpRoute := range opts.HTTPRoutes { hostACL := "host_" + sanitizeName(httpRoute.Name) + fmt.Fprintf(&sb, " # service: %s\n", httpRoute.Domain) if httpRoute.Subdomains { fmt.Fprintf(&sb, " acl %s hdr(host) -i -m end .%s\n", hostACL, httpRoute.Domain) fmt.Fprintf(&sb, " acl %s hdr(host) -i -m str %s\n", hostACL, httpRoute.Domain) @@ -319,6 +325,7 @@ frontend http_in routeName := sanitizeName(httpRoute.Name) for i, rb := range httpRoute.Routes { backendName := fmt.Sprintf("be_l7_%s_%d", routeName, i) + fmt.Fprintf(&sb, "# service: %s\n", httpRoute.Domain) fmt.Fprintf(&sb, "backend %s\n", backendName) sb.WriteString(" mode http\n") if rb.HealthPath != "" { @@ -359,6 +366,61 @@ func sortedKeys(m map[string]string) []string { return keys } +// ParseValidationErrors extracts line numbers from haproxy -c ALERT output. +// HAProxy format: [ALERT] (pid) : config : parsing [/path/to/file:LINE]: message +func ParseValidationErrors(output string) []int { + var lines []int + for _, line := range strings.Split(output, "\n") { + if !strings.Contains(line, "[ALERT]") || !strings.Contains(line, "parsing") { + continue + } + // Find the bracketed path:line — e.g., [/tmp/test.cfg:111] + bracketStart := strings.LastIndex(line, "[") + bracketEnd := strings.Index(line[bracketStart:], "]") + if bracketStart < 0 || bracketEnd < 0 { + continue + } + bracket := line[bracketStart+1 : bracketStart+bracketEnd] + // Extract line number after the last colon in the bracket + colonIdx := strings.LastIndex(bracket, ":") + if colonIdx < 0 { + continue + } + if n, err := strconv.Atoi(bracket[colonIdx+1:]); err == nil { + lines = append(lines, n) + } + } + return lines +} + +// FindBrokenServices maps failing line numbers to service domains using +// "# service: domain" comment markers in the generated config. +// For each failing line, scans backwards to find the nearest marker. +func FindBrokenServices(config string, failingLines []int) []string { + configLines := strings.Split(config, "\n") + seen := map[string]bool{} + var broken []string + + for _, lineNum := range failingLines { + if lineNum < 1 || lineNum > len(configLines) { + continue + } + // Scan backwards from the failing line to find the nearest service marker + for i := lineNum - 1; i >= 0; i-- { + trimmed := strings.TrimSpace(configLines[i]) + if strings.HasPrefix(trimmed, "# service: ") { + domain := strings.TrimPrefix(trimmed, "# service: ") + if !seen[domain] { + seen[domain] = true + broken = append(broken, domain) + } + break + } + } + } + return broken +} + // sanitizeName converts a name to a valid HAProxy identifier (alphanumeric + underscore) func sanitizeName(name string) string { return strings.Map(func(r rune) rune { diff --git a/internal/haproxy/config_test.go b/internal/haproxy/config_test.go index 41875a3..8162588 100644 --- a/internal/haproxy/config_test.go +++ b/internal/haproxy/config_test.go @@ -726,3 +726,110 @@ func TestGenerateWithOpts_NoL7Fields_BackwardCompat(t *testing.T) { t.Errorf("unexpected response header in simple route, got:\n%s", out) } } + +// --- Service marker and validation error tests --- + +func TestGenerateWithOpts_ServiceMarkers(t *testing.T) { + m := NewManager("") + instances := []InstanceRoute{ + {Name: "cloud", Domain: "cloud.example.com", BackendIP: "10.0.0.1", Subdomains: true}, + } + httpRoutes := []HTTPRoute{ + {Name: "api", Domain: "api.example.com", Routes: []HTTPRouteBackend{{Backend: "10.0.0.5:8080"}}}, + } + + out := m.GenerateWithOpts(instances, nil, GenerateOpts{HTTPRoutes: httpRoutes}) + + // L4 instance should have service marker + if !strings.Contains(out, "# service: cloud.example.com") { + t.Errorf("expected service marker for cloud.example.com, got:\n%s", out) + } + // L7 HTTP should have service marker + if !strings.Contains(out, "# service: api.example.com") { + t.Errorf("expected service marker for api.example.com, got:\n%s", out) + } +} + +func TestParseValidationErrors(t *testing.T) { + output := `[NOTICE] (123) : haproxy version is 3.0.10 +[WARNING] (123) : config : parsing [/tmp/test.cfg:42] : some warning +[ALERT] (123) : config : parsing [/tmp/test.cfg:111]: 'http-response set-header' expects 'if' +[ALERT] (123) : config : parsing [/tmp/test.cfg:115]: unknown keyword +[ALERT] (123) : config : Error(s) found in configuration file : /tmp/test.cfg` + + lines := ParseValidationErrors(output) + + if len(lines) != 2 { + t.Fatalf("expected 2 failing lines, got %d: %v", len(lines), lines) + } + if lines[0] != 111 { + t.Errorf("expected first line 111, got %d", lines[0]) + } + if lines[1] != 115 { + t.Errorf("expected second line 115, got %d", lines[1]) + } +} + +func TestParseValidationErrors_NoAlerts(t *testing.T) { + output := `[NOTICE] (123) : haproxy version is 3.0.10 +[WARNING] (123) : config : parsing [/tmp/test.cfg:42] : some warning +Warnings were found.` + + lines := ParseValidationErrors(output) + if len(lines) != 0 { + t.Errorf("expected 0 failing lines for warnings-only output, got %d", len(lines)) + } +} + +func TestFindBrokenServices(t *testing.T) { + config := `# boilerplate +frontend l7_https + bind 127.0.0.1:8443 ssl crt /etc/haproxy/certs/ + mode http + + # service: good.example.com + acl host_good hdr(host) -i good.example.com + # service: bad.example.com + acl host_bad hdr(host) -i bad.example.com + http-response set-header X-Bad broken value if host_bad + # service: also-good.example.com + acl host_also_good hdr(host) -i also-good.example.com +` + // Line 10 is the broken http-response line (1-indexed) + broken := FindBrokenServices(config, []int{10}) + + if len(broken) != 1 { + t.Fatalf("expected 1 broken service, got %d: %v", len(broken), broken) + } + if broken[0] != "bad.example.com" { + t.Errorf("expected bad.example.com, got %s", broken[0]) + } +} + +func TestFindBrokenServices_MultipleLinesOneDomain(t *testing.T) { + config := `line1 + # service: bad.example.com +line3 +line4 +line5 +` + // Multiple lines from the same service should deduplicate + broken := FindBrokenServices(config, []int{3, 4, 5}) + + if len(broken) != 1 { + t.Fatalf("expected 1 broken service (deduped), got %d: %v", len(broken), broken) + } +} + +func TestFindBrokenServices_NoMarker(t *testing.T) { + config := `line1 +line2 +line3 +` + // No service markers — should return empty + broken := FindBrokenServices(config, []int{2}) + + if len(broken) != 0 { + t.Errorf("expected 0 broken services when no markers, got %d", len(broken)) + } +}