fix(component-failover-adoption): unify apiserver's dual connect paths, fix connect-time UAF
Replaces apiserver's two connect mechanisms -- a per-peer Go loop that hardcoded nmasters=1 per attempt, and a separate direct-construction path for the legacy single-host config -- with one pipeline: buildClusterConfig() parses (via task 4.1's C parser) or synthesizes a single-candidate config, validates it, then ConfigConverter (xce_cluster_config_to_client_config) produces the client config for one xce_backend_init() call. A 3-candidate config now actually exercises the C library's own failover/standby-redirect handling on initial connect, matching worker's and scheduler's task 2.1/3.1 work, instead of a Go-level per-candidate retry loop that never built a real multi-master config. Along the way: found and fixed a genuine use-after-free. The deleted tryConnect() freed its C.CString-allocated host address immediately after its single connect call returned, but xcecli_ctx_init() stores a raw pointer to the config forever (client.c:38) and every later failover rotation re-dereferences it (client_failover.c) -- so any reconnect after the first would read freed memory. Fixed by giving the cluster/client config structs package-level, process-lifetime storage (gClusterCfg/gClientCfg), mirroring worker's and scheduler's file-scope static pattern, with host/from/key written into fixed-size C arrays instead of freed heap pointers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NNB4zGPCsHyDUCDJyMbiQN
This commit is contained in:
parent
8b1e13a52b
commit
e635903996
SSH key fingerprint:
SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
3 changed files with 488 additions and 52 deletions
|
|
@ -94,7 +94,7 @@
|
|||
- _Boundary: ApiserverBackendBridge_
|
||||
- _Depends: 1.2_
|
||||
|
||||
- [ ] 4.2 Replace apiserver's dual connect mechanisms with the single unified pipeline
|
||||
- [x] 4.2 Replace apiserver's dual connect mechanisms with the single unified pipeline
|
||||
- Delete apiserver's per-peer Go connect loop and its separate direct-construction path used when no peer list is configured — both bypass the shared candidate-list validation and conversion helper entirely today
|
||||
- Build exactly one validated candidate list before connecting, whether from the configured peer list (via Task 4.1's parser) or synthesized as a single candidate from the legacy address/port fields when no peer list was given, then run it through the shared helper (via CGO) and connect once
|
||||
- Observable completion: starting apiserver with no peer list and with a single-entry peer list produce the identical connect log sequence; a three-peer config where the first is a live standby registers against the actual primary in one call
|
||||
|
|
@ -169,5 +169,6 @@
|
|||
- Task 2.3 (real E2E caught what unit tests missed — 2 production bugs slipped through tasks 2.1/2.2's approved reviews): (1) `xcecli_start()` (`src/lib/client.c`) unconditionally resets `ctx->failover.rejoin_cb = NULL` as part of its own init — any `xcecli_set_reconnect_handler()` call before `xcecli_start()` is silently wiped. Register the reconnect handler AFTER `xcecli_start()` succeeds, not before. Scheduler/apiserver's own reconnect-handler registration (3.1, 4.3) must follow this same ordering or they'll hit the identical silent-wipe bug. (2) A successful `xcecli_join()`/worker join returns the assigned slot index as `reply.status` (any `rc >= 0`, not always exactly `0`) — checking `rc == 0` for success is wrong; use `rc < 0` as the only failure test (matches `xcecd_join_cluster()`'s own established convention). Any new code checking a join/rejoin call's success must use this same `rc < 0` convention, not `rc == 0`. (3) Security lesson: an env-var runtime bypass of a security check (`XCECD_ALLOW_NONROOT`) added for test/E2E convenience was rejected on review — the correct pattern for skipping a production-only check (root privilege, etc.) in a test/virtual build is a compile-time guard against the project's existing `VIRTUAL_CLUSTER` or `XCE_TESTING` flags, never a runtime environment variable reachable in the shipped production binary.
|
||||
- Task 3.2: implemented as a shared `sched_publish_retry_loop()`/`sched_publish_check_rejoin()` pair called from both `sched_publish_dispatch()` and `sched_publish_pend_reason()`, not literally inlined into `sched_publish_dispatch()` alone as design.md's prose names it — reviewed and judged compliant (same thread, fires once after the whole retry loop's successful return, not per-attempt, not in `sched_allocator`). Testing lesson for any future scheduler.c unit test needing to intercept `xcecli_*` calls: `xcecli_jobdispatch`/`xcecli_set_pend_reason`/`xcecli_join`/`xcecli_start`/`xcecli_set_reconnect_handler` all live in the single `client.c` translation unit inside `libxce.a` alongside symbols scheduler.c genuinely needs — the "test double linked ahead of the archive" trick (used by test_driver_decision_cb.c one layer up) causes duplicate-definition errors here. Use `-Wl,--wrap=<symbol>` linker interposition instead (see `test_scheduler_decision_publish_retry.c`/its `meson.build` target) when a test needs scheduler.c itself linked live alongside intercepted libxce calls. Also confirmed: `sched_admin_request_test`'s `test_recordstart_symlink_escape_replies_error` fails independent of this spec's changes (its white-box `#include` list doesn't pull in `scheduler.c` at all) — a pre-existing, unrelated, environment-specific (root-in-sandbox symlink-permission) failure; do not re-investigate it as a regression in later tasks.
|
||||
- Task 4.1: first apiserver-group (Go+CGO) task. `xce_parse_peers()` alone (`src/lib/cluster.c`) does syntax parsing only — no semantic checks (accepts port 0, non-numeric port via silent `atoi()`-to-0, empty host). Worker/scheduler always pair it with a second `xce_cluster_config_validate()` call in their `main.c`s; round-1 review caught `parseCandidates()` (`candidates.go`) skipping that second call, silently accepting malformed candidates worker/scheduler reject — a reminder that "the C function I called is correct" isn't the same as "I called every C function the precedent calls." Fix: call `xce_cluster_config_validate(&cCfg)` after `xce_parse_peers()`, stamping a local placeholder into `cCfg.identity` first since that function also checks a non-empty top-level `identity` (unrelated to per-candidate validation, never propagated to the real connection identity elsewhere in `clib.go`). `xce_cluster_config_validate()` checks `hostname`/`port`/top-level `identity` only, never per-candidate `name` — so the `name=` prefix stays legitimately optional. Established convention confirmed pre-existing (not invented this task): a `//go:build cgo` test file with no `import "C"` of its own, pairing with a plain-Go-signature function in a separate cgo-preamble file (`_test.go` files can never `import "C"` themselves — a Go-toolchain restriction, not just a project style choice) — see `wire_dispatch_test.go`/`wire_dispatch_support.go` for the pre-existing precedent this task's `candidates_test.go`/`candidates.go` split follows. Task 4.1 is parsing-only by design — `RegisterToBackend()`'s connect loop and `tryConnect()` stay untouched; task 4.2 is the one that collapses them into a single connect call, so don't expect this diff to touch that territory. Behavior change (deliberate, disclosed, not a regression): a malformed entry now rejects the WHOLE candidate list (matches worker/scheduler's all-or-nothing precedent, `xce_parse_peers()` has no partial-success mode by construction) instead of the old Go parser's skip-with-warning leniency — surfaces via `RegisterToBackend`'s error → `cmd/main.go`'s `logger.Error`+`os.Exit(1)`, loud not silent. Tooling gap for future apiserver tasks (4.2-4.6): `make test-backend`'s documented CGO env-var setup doesn't actually export `CGO_CFLAGS`/`CGO_LDFLAGS` to a general shell (they only exist as meson `custom_target` env vars) — reconstruct via `pkg-config` against the deps `apiserver/meson.build` lists, or run the full `xce-build.sh apiserver asan` (rebuilds `libxce.a` fresh too) rather than a bare `go test`.
|
||||
- Task 4.2: found and fixed a genuine, independently-verified use-after-free while collapsing apiserver's two connect mechanisms into one — the deleted `tryConnect()` did `C.CString(host)` then `defer C.free(cAddr)` right after its single `xce_backend_init()` call returned, but `xcecli_ctx_init()` stores `ctx->conf = cfg` as a raw pointer (`src/lib/client.c:38`, never copied), and `client_failover.c`'s rotation path (`xcecli_retry_next`) re-dereferences `ac->conf->masters[cur]` on *every* failover attempt, not just at init — so any reconnect after the first would read freed heap memory. (`from`/`key` were NOT part of this bug — `xce_ctx_init()` `strdup()`s those synchronously before the defer ever fires.) Fix mirrors worker's/scheduler's own file-scope-`static` pattern for the Go/CGO side: two package-level `var`s (`gClusterCfg C.xce_cluster_config_t`, `gClientCfg C.xce_client_config_t`, alongside the pre-existing `gCtx`/`gIDCtx`) give the CGO structs a stable, process-lifetime address, and the connect path now writes host/from/key into fixed-size C arrays (`writeCCharArray()`) instead of `C.CString`/`C.free`-ing heap pointers the C library retains forever. General lesson for any future CGO code in this codebase that hands a struct to a C API storing a bare pointer to it (check the C function's own source, not just its header, for whether it copies or aliases): a Go function-local var is NOT safe once the call returns unless the C side genuinely copies everything reachable from it — verify via source read, the same standard task 2.1's original aliasing-hazard note set for the C-side daemons. `parseCandidates()` (task 4.1's deliverable) is now production-dead code — `buildClusterConfig()` calls `xce_parse_peers()` directly into the package-level `gClusterCfg` instead, since routing through `parseCandidates()`'s Go-slice return would mean re-parsing/re-copying into a fresh C struct that doesn't have the required stable address; `parseCandidates()` is exercised only by its own tests now, worth a follow-up cleanup task if it stays unused.
|
||||
- Task 3.4: pure test-coverage task, zero production changes to `scheduler.c`/`driver.c` (correct outcome — task 3.2 already implemented the retry/shutdown/rejoin behavior correctly; 3.4 proves it holds at multi-decision QUEUE granularity through the real drain loop, which task 3.2's own tests never exercised, all 6 of which call `sched_publish_dispatch()`/`sched_publish_pend_reason()` directly on one decision). New `test_scheduler_decision_redelivery.c` links real `scheduler_srcs` with `-Wl,--wrap=xcecli_jobdispatch` (task 3.2's established technique) and drives the real `xce_sched_driver_on_decision()` (driver.c) over 3 queued, distinct-jobid decisions. Two load-bearing tests, both RED→GREEN-verified against the real code by two independent reviewer rounds (not just the implementer's claim): ordering (decision 1 stuck-then-succeeds fully completes before decision 2's call ever fires) and bounded shutdown latency (decisions queued behind one that triggers `core.stop` mid-retry get ZERO publish calls — the concrete, fast, deterministic proof that `sched_publish_retry_loop()`'s check-`stop`-before-each-attempt placement is correct, verified by reviewers moving the check to bottom-of-loop and confirming only this one test catches it). Test-fixture gotcha for any future test building a bare `xce_sched_core_t` via `xce_sched_core_init()` alone (not full `sched_init()`): do NOT call `xce_sched_core_fini()` on it — `fini()` calls `dataq_fini()` on `pend_updates[0/1]`, which only `sched_init()` (not `xce_sched_core_init()`) ever `dataq_init()`s, so `fini()` on a bare core SEGVs on a NULL dataq; omit the `fini()` call entirely, matching `test_core_events.c`'s established "fixtures aren't torn down" convention for this exact minimal-fixture pattern.
|
||||
- Task 3.3: fixed a real production gap found along the way — `sched_init()`'s two all-candidates-unreachable/join-failure log lines logged only a candidate COUNT, never host:port names (same gap worker's task 2.3 fixed for the worker). Added `sched_format_candidate_list()`, threading `sched->cluster_cfg` through as a parameter rather than a new file-scope global (unlike worker's `g_cluster_cfg`) since `test_scheduler_connect.c` links `scheduler_srcs` as separate objects, not `#include`, so the `static` helper isn't directly callable from tests — coverage requires a real `dup2()`-based stdout capture around a live `sched_init()` call hitting the failure path, not a white-box direct call (see `capture_stdout_during_sched_init()` in `test_scheduler_connect.c` — reusable pattern for any future scheduler.c log-content assertion). Round-1 review caught this diagnostic fix shipping with zero test coverage (proven by reverting the 59-line diff and rebuilding: all existing tests still passed) — a reminder that "the fix works, I verified it manually/via live logs" is not the same as a committed regression test; always add the assertion, don't just observe the correct behavior once. The worst-case-occupancy backpressure test (`test_scheduler_backpressure.c`) only proves Req 3 AC2 (buffering/block/resume-with-no-loss) — it does not exercise task 3.2's `-E_XCE_NOT_READY` retry path, so don't cite it for Req 7 coverage (that's task 3.4's job). E2E kill-primary/reconnect and all-unreachable-at-startup were validated manually against a real 3-node `/xce-deploy-ha` cluster rather than committed as an automated test, following worker's task 2.3 precedent — this repo has no automated live-multi-process-deploy E2E harness for any C daemon (the only one, `test_e2e_live_ha_push.py`, is Python/apiserver-only); reviewer judged this acceptable both rounds. Manual validation confirmed join, primary-death survival, and lazy socket-orphaning-on-idle (matches design), but could not observe a live rejoin firing against real sockets — blocked by bug-010 (`xcecd` ASAN "already joined thread" abort under `unshare -r`, needed to bypass the worker daemon's root-only check in this sandbox; unconfirmed whether it's a genuine bug or a userns artifact, never tried under real root). The exact rejoin-after-publish-success code path already has solid white-box mock coverage from task 3.2's `test_rejoin_composition_retries_across_publish_calls`, so this residual gap is narrowly "proven against mocks, not yet against a live socket" — worth closing opportunistically if bug-010 ever gets resolved, not a blocker for this spec.
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ package backend
|
|||
#include "idgen.h"
|
||||
#include "controller.capnp.h"
|
||||
#include "val_helpers.h"
|
||||
#include "xce/cluster.h"
|
||||
|
||||
// backend_ctx_t wraps xcecli_ctx_t for use as the C handler's arg.
|
||||
typedef struct {
|
||||
|
|
@ -69,6 +70,43 @@ import (
|
|||
// gCtx holds the C client context for the lifetime of the process.
|
||||
var gCtx C.backend_ctx_t
|
||||
|
||||
// gClusterCfg is the single validated xce_cluster_config_t RegisterToBackend
|
||||
// builds on every call -- either by parsing config.Peers via the shared C
|
||||
// parser (xce_parse_peers), or by synthesizing a one-candidate list from the
|
||||
// legacy config.Address/config.Port fields when Peers is empty (Req 1 AC5: a
|
||||
// single controller is the candidates_count=1 case of the identical
|
||||
// validate/convert/connect pipeline, not a separately-maintained code path
|
||||
// -- see buildClusterConfig() below).
|
||||
//
|
||||
// Package-level, not a buildClusterConfig()/RegisterToBackend()-local
|
||||
// variable, for the same reason gCtx and gIDCtx already are (process-lifetime
|
||||
// stable address): xce_cluster_config_to_client_config() (ConfigConverter,
|
||||
// src/lib/cluster.c) aliases gClientCfg.masters[i] directly at this struct's
|
||||
// own candidates[i].hostname fixed-char-array storage -- no strdup, no copy.
|
||||
// A caller that built the source xce_cluster_config_t on the stack (or any
|
||||
// Go-local variable) and then let gClientCfg outlive that scope would have a
|
||||
// use-after-scope bug: confirmed by reading src/lib/client.c's
|
||||
// xcecli_ctx_init() (`ctx->conf = cfg` -- stores a RAW pointer, never
|
||||
// copies) and src/lib/client_failover.c (`ac->conf->masters[next]` is
|
||||
// dereferenced on every failover rotation, not just at initial connect).
|
||||
// Worker's and scheduler's C implementations hit this identical hazard
|
||||
// (component-failover-adoption tasks 2.1/3.1) and solved it by keeping the
|
||||
// source config on a process-lifetime struct (worker: cc_ctx->cluster_cfg;
|
||||
// scheduler: sched->cluster_cfg) -- this package-level var is the direct Go
|
||||
// equivalent.
|
||||
var gClusterCfg C.xce_cluster_config_t
|
||||
|
||||
// gClientCfg is the xce_client_config_t ConfigConverter populates from
|
||||
// gClusterCfg and xce_backend_init() connects with. Package-level for the
|
||||
// same reason as gClusterCfg above: xcecli_ctx_init() stores gCtx.ctx->conf
|
||||
// as a raw pointer to whichever struct address is passed to it, dereferenced
|
||||
// for the ctx's entire runtime life (every failover rotation reads
|
||||
// ctx->conf->masters[]/nmasters, not only the initial connect) -- so this
|
||||
// struct's address must be stable for the process lifetime, not a
|
||||
// RegisterToBackend-local variable that goes out of scope when the function
|
||||
// returns.
|
||||
var gClientCfg C.xce_client_config_t
|
||||
|
||||
// gIDCtx is the C idgen context used to generate job IDs.
|
||||
var gIDCtx C.idgen_ctx_t
|
||||
var idCtxOnce sync.Once
|
||||
|
|
@ -77,34 +115,94 @@ var idCtxOnce sync.Once
|
|||
// Must be set via StartNotificationListener before RegisterToBackend.
|
||||
var gHandler NotificationHandler
|
||||
|
||||
func tryConnect(host string, port int, from, key string) error {
|
||||
cAddr := C.CString(host)
|
||||
cFrom := C.CString(from)
|
||||
cKey := C.CString(key)
|
||||
defer func() {
|
||||
C.free(unsafe.Pointer(cAddr))
|
||||
C.free(unsafe.Pointer(cFrom))
|
||||
C.free(unsafe.Pointer(cKey))
|
||||
}()
|
||||
|
||||
var cConfig C.xce_client_config_t
|
||||
cConfig.nmasters = 1
|
||||
cConfig.masters[0] = cAddr
|
||||
cConfig.master_ports[0] = C.int(port)
|
||||
cConfig.from = cFrom
|
||||
cConfig.key = cKey
|
||||
|
||||
rc := C.xce_backend_init(&cConfig, &gCtx)
|
||||
if rc < 0 {
|
||||
return fmt.Errorf("xce_backend_init failed on %s:%d: %d", host, port, int(rc))
|
||||
// writeCCharArray copies s into dst, a fixed-size C char array field of
|
||||
// gClusterCfg (hostname/identity/key), truncating to len(dst)-1 bytes and
|
||||
// always leaving the result NUL-terminated. Because these are fixed arrays
|
||||
// embedded directly in gClusterCfg (a package-level, process-lifetime
|
||||
// struct -- see its doc comment), populating them this way needs no
|
||||
// C.CString/C.free bookkeeping at all: xce_cluster_config_to_client_config()
|
||||
// later aliases pointers directly into this same storage.
|
||||
func writeCCharArray(dst []C.char, s string) {
|
||||
n := len(s)
|
||||
if n > len(dst)-1 {
|
||||
n = len(dst) - 1
|
||||
}
|
||||
return nil
|
||||
for i := 0; i < n; i++ {
|
||||
dst[i] = C.char(s[i])
|
||||
}
|
||||
dst[n] = 0
|
||||
}
|
||||
|
||||
// RegisterToBackend initialises the XCE client library and joins as an API
|
||||
// server. StartNotificationListener should be called first so that push
|
||||
// events have a handler ready before the first push can arrive.
|
||||
func RegisterToBackend(config *BackendConfig) error {
|
||||
// formatCandidateList renders gClusterCfg's current candidates as
|
||||
// "host:port, host:port, ..." for diagnostic logging (Req 5.1, 5.2) -- the
|
||||
// same textual shape scheduler's sched_format_candidate_list() (scheduler.c)
|
||||
// produces, for cross-component log consistency.
|
||||
func formatCandidateList() string {
|
||||
n := int(gClusterCfg.candidates_count)
|
||||
parts := make([]string, 0, n)
|
||||
for i := 0; i < n; i++ {
|
||||
cand := &gClusterCfg.candidates[i]
|
||||
parts = append(parts, fmt.Sprintf("%s:%d", C.GoString(&cand.hostname[0]), int(cand.port)))
|
||||
}
|
||||
return strings.Join(parts, ", ")
|
||||
}
|
||||
|
||||
// currentClusterCandidates returns gClusterCfg's current candidates as a
|
||||
// Go-native slice. Exists so clib_test.go (a //go:build cgo test file that
|
||||
// cannot `import "C"` itself -- a Go-toolchain restriction, golang/go#13938)
|
||||
// can assert on buildClusterConfig()'s output shape without re-implementing
|
||||
// CGO field access in the test file, mirroring the candidates.go/
|
||||
// candidates_test.go split already established in this package.
|
||||
func currentClusterCandidates() []Candidate {
|
||||
n := int(gClusterCfg.candidates_count)
|
||||
out := make([]Candidate, n)
|
||||
for i := 0; i < n; i++ {
|
||||
cand := &gClusterCfg.candidates[i]
|
||||
out[i] = Candidate{
|
||||
Name: C.GoString(&cand.name[0]),
|
||||
Host: C.GoString(&cand.hostname[0]),
|
||||
Port: int(cand.port),
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// currentClientConfigNMasters exposes gClientCfg.nmasters as a plain int,
|
||||
// for the same test-file-cannot-import-C reason as currentClusterCandidates
|
||||
// above. Lets tests confirm xce_cluster_config_to_client_config() actually
|
||||
// propagated the full candidate count into the struct xce_backend_init()
|
||||
// connects with -- proving ONE call carries all N candidates, rather than N
|
||||
// separate single-candidate calls (the deleted per-peer Go loop's behavior,
|
||||
// which always hardcoded nmasters=1).
|
||||
func currentClientConfigNMasters() int {
|
||||
return int(gClientCfg.nmasters)
|
||||
}
|
||||
|
||||
// buildClusterConfig builds and validates the single xce_cluster_config_t
|
||||
// this connect attempt will use (Req 1 AC1-3/AC5), writing directly into the
|
||||
// package-level gClusterCfg (see its doc comment for why this must be
|
||||
// package-level rather than a local variable).
|
||||
//
|
||||
// A single controller is not a separately-maintained code path: when
|
||||
// config.Peers is non-empty it is parsed via the same shared C parser
|
||||
// (xce_parse_peers) worker and scheduler already call directly; otherwise a
|
||||
// one-candidate list is synthesized from config.Address/config.Port. Either
|
||||
// source is then run through the identical xce_cluster_config_validate()
|
||||
// call below -- no branch downstream of this function distinguishes the two
|
||||
// sources (Req 1 AC5), mirroring scheduler's sched_build_cluster_config()
|
||||
// (scheduler.c) and worker's equivalent (controller_connector.c).
|
||||
//
|
||||
// This intentionally does not reuse parseCandidates() (candidates.go):
|
||||
// parseCandidates() parses into its own function-local xce_cluster_config_t
|
||||
// and reconstructs a Go []Candidate slice from it -- exactly the
|
||||
// use-after-scope-prone, throwaway-C-struct pattern gClusterCfg's doc
|
||||
// comment above warns against for this call site. buildClusterConfig() must
|
||||
// parse directly into the package-level gClusterCfg so the very same struct
|
||||
// flows unmodified into xce_cluster_config_to_client_config() below.
|
||||
//
|
||||
// Does not call xce_cluster_config_to_client_config() or connect -- callers
|
||||
// (RegisterToBackend) do that separately.
|
||||
func buildClusterConfig(config *BackendConfig) error {
|
||||
from := config.From
|
||||
if from == "" {
|
||||
from = "xce_apiserver"
|
||||
|
|
@ -114,39 +212,88 @@ func RegisterToBackend(config *BackendConfig) error {
|
|||
key = "keepsecret$"
|
||||
}
|
||||
|
||||
if len(config.Peers) > 0 {
|
||||
// Parsing is routed through the same C parser (xce_parse_peers) worker
|
||||
// and scheduler already call directly -- Req 6.1/6.2. A malformed
|
||||
// entry rejects the whole candidate list up front (matching
|
||||
// worker's/scheduler's own all-or-nothing --peers parsing) rather
|
||||
// than being skipped with a warning as the old hand-written
|
||||
// parsePeer()-per-entry loop did.
|
||||
candidates, err := parseCandidates(strings.Join(config.Peers, ","))
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
gClusterCfg = C.xce_cluster_config_t{}
|
||||
|
||||
var lastErr error
|
||||
for _, cand := range candidates {
|
||||
if err := tryConnect(cand.Host, cand.Port, from, key); err != nil {
|
||||
logger.Warn("Candidate unreachable, trying next",
|
||||
zap.String("host", cand.Host), zap.Int("port", cand.Port), zap.Error(err))
|
||||
lastErr = err
|
||||
continue
|
||||
}
|
||||
logger.Info("Backend connection established (HA)",
|
||||
zap.String("host", cand.Host), zap.Int("port", cand.Port))
|
||||
return nil
|
||||
if len(config.Peers) > 0 {
|
||||
peersStr := strings.Join(config.Peers, ",")
|
||||
cPeers := C.CString(peersStr)
|
||||
rc := C.xce_parse_peers(cPeers, &gClusterCfg)
|
||||
C.free(unsafe.Pointer(cPeers))
|
||||
if rc != 0 {
|
||||
return fmt.Errorf(
|
||||
"apiserver: --peers parse error: expected \"[name=]host:port,...\" got %q",
|
||||
peersStr)
|
||||
}
|
||||
return fmt.Errorf("all HA candidates failed: %w", lastErr)
|
||||
} else {
|
||||
gClusterCfg.candidates_count = 1
|
||||
writeCCharArray(gClusterCfg.candidates[0].hostname[:], config.Address)
|
||||
gClusterCfg.candidates[0].port = C.int(config.Port)
|
||||
}
|
||||
|
||||
if err := tryConnect(config.Address, config.Port, from, key); err != nil {
|
||||
writeCCharArray(gClusterCfg.identity[:], from)
|
||||
writeCCharArray(gClusterCfg.key[:], key)
|
||||
|
||||
if rc := C.xce_cluster_config_validate(&gClusterCfg); rc != 0 {
|
||||
return fmt.Errorf(
|
||||
"apiserver: invalid controller candidate configuration (rc=%d): %s",
|
||||
int(rc), formatCandidateList())
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// RegisterToBackend initialises the XCE client library and joins as an API
|
||||
// server, using the full configured candidate list -- config.Peers if set,
|
||||
// otherwise a single-candidate list synthesized from config.Address/
|
||||
// config.Port (Req 1 AC5: the candidates_count=1 case of the identical
|
||||
// pipeline, not a separate code path -- see buildClusterConfig()). Builds
|
||||
// exactly ONE validated xce_cluster_config_t regardless of source, converts
|
||||
// it once via the shared ConfigConverter
|
||||
// (xce_cluster_config_to_client_config), and calls xce_backend_init()
|
||||
// exactly ONCE -- replacing the former dual-mechanism implementation (a
|
||||
// Go-level per-peer tryConnect() retry loop for the Peers case, and a
|
||||
// second, separately-coded tryConnect() call for the legacy Address/Port
|
||||
// case; both bypassed xce_cluster_config_t/xce_cluster_config_validate()/
|
||||
// xce_cluster_config_to_client_config() entirely, and both always
|
||||
// hardcoded nmasters=1).
|
||||
//
|
||||
// StartNotificationListener should be called first so that push events have
|
||||
// a handler ready before the first push can arrive.
|
||||
func RegisterToBackend(config *BackendConfig) error {
|
||||
if err := buildClusterConfig(config); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
logger.Info("Attempting backend connection",
|
||||
zap.Int("candidates", int(gClusterCfg.candidates_count)),
|
||||
zap.String("candidate_list", formatCandidateList()))
|
||||
|
||||
gClientCfg = C.xce_client_config_t{}
|
||||
if rc := C.xce_cluster_config_to_client_config(&gClusterCfg, &gClientCfg); rc != 0 {
|
||||
return fmt.Errorf("apiserver: candidate list conversion failed (rc=%d)", int(rc))
|
||||
}
|
||||
// ConfigConverter deliberately does not set from/key (caller-owned
|
||||
// identity/auth, per its own doc comment) -- point them at gClusterCfg's
|
||||
// own identity/key fixed arrays rather than allocating separate C
|
||||
// strings: gClusterCfg is the same package-level, process-lifetime
|
||||
// struct ConfigConverter already aliased gClientCfg.masters[i] into, so
|
||||
// this needs no additional C.CString/C.free bookkeeping.
|
||||
gClientCfg.from = &gClusterCfg.identity[0]
|
||||
gClientCfg.key = &gClusterCfg.key[0]
|
||||
|
||||
rc := C.xce_backend_init(&gClientCfg, &gCtx)
|
||||
if rc < 0 {
|
||||
logger.Warn("All configured backend candidates failed",
|
||||
zap.Int("candidates", int(gClusterCfg.candidates_count)),
|
||||
zap.String("candidate_list", formatCandidateList()),
|
||||
zap.Int("rc", int(rc)))
|
||||
return fmt.Errorf("xce_backend_init failed across %d candidate(s) (rc=%d): %s",
|
||||
int(gClusterCfg.candidates_count), int(rc), formatCandidateList())
|
||||
}
|
||||
|
||||
logger.Info("Backend connection established",
|
||||
zap.String("address", config.Address),
|
||||
zap.Int("port", config.Port))
|
||||
zap.Int("nmasters", int(gClientCfg.nmasters)),
|
||||
zap.String("candidate_list", formatCandidateList()))
|
||||
return nil
|
||||
}
|
||||
|
||||
|
|
|
|||
288
src/frontend/apiserver/backend/clib_test.go
Normal file
288
src/frontend/apiserver/backend/clib_test.go
Normal file
|
|
@ -0,0 +1,288 @@
|
|||
//go:build cgo
|
||||
|
||||
package backend
|
||||
|
||||
import (
|
||||
"os"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"go.uber.org/zap/zaptest/observer"
|
||||
)
|
||||
|
||||
// clib_test.go exercises RegisterToBackend/buildClusterConfig (clib.go), the
|
||||
// unified candidate-list connect pipeline that replaces the former
|
||||
// dual-mechanism implementation (component-failover-adoption, task 4.2,
|
||||
// boundary: ApiserverBackendBridge). Kept //go:build cgo since these
|
||||
// functions only exist in the CGO build -- this file itself does not
|
||||
// `import "C"` (a _test.go file can never do that directly, golang/go#13938):
|
||||
// it only calls the plain-Go-signature functions defined in clib.go, same
|
||||
// split candidates.go/candidates_test.go already established in this
|
||||
// package.
|
||||
//
|
||||
// Test strategy:
|
||||
// - Req 1.5 (no-Peers and single-entry-Peers converge on the identical
|
||||
// validated xce_cluster_config_t shape) and Req 1.1-1.3 (multi-candidate
|
||||
// passthrough) are exercised via buildClusterConfig() directly -- pure
|
||||
// struct-building + validation, no network I/O, so these run fast and
|
||||
// avoid any socket/leak concerns from a real (failing) connect attempt.
|
||||
// - "a single xce_backend_init() call carries the full candidate count"
|
||||
// is exercised by calling the real RegisterToBackend() against closed
|
||||
// local ports (connect() returns ECONNREFUSED near-instantly, same
|
||||
// "no backoff loop inside xcecli_start()'s per-master connect scan"
|
||||
// property scheduler's test_scheduler_connect.c and worker's task 2.3
|
||||
// test both rely on) and inspecting gClientCfg.nmasters afterward via
|
||||
// currentClientConfigNMasters() -- this is deliberately the ONLY test in
|
||||
// this file that reaches xce_backend_init()/a real connect() call, to
|
||||
// keep the (unavoidable, pre-existing -- see status report CONCERNS)
|
||||
// per-call xcecli_ctx_init() allocation this path leaks on a failed
|
||||
// connect bounded to one occurrence.
|
||||
// - "no-peers and single-entry-peers produce the identical connect log
|
||||
// sequence" is exercised behaviorally via captureLog() (handler_test.go,
|
||||
// this package), asserting on the log MESSAGE sequence (not field
|
||||
// values) RegisterToBackend emits for each source.
|
||||
// - The "source config structs are package-level, not stack-local"
|
||||
// regression guard is a source-text scan of clib.go, same genre as
|
||||
// scheduler's/worker's structural regression tests (test_scheduler_connect.c,
|
||||
// test_controller_connector.c) for the identical hazard.
|
||||
|
||||
// testPortBase is deliberately unlikely to have a real listener bound in
|
||||
// CI/dev sandboxes -- connect() against a closed local port fails fast, so
|
||||
// RegisterToBackend() calls below return quickly without needing a live
|
||||
// controller (same precedent as scheduler's TEST_PORT_BASE).
|
||||
const testPortBase = 28581
|
||||
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
// Req 1.5 -- unified candidate-list pipeline: no branch for "single host"
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
|
||||
func TestBuildClusterConfig_NoPeersAndSingleEntryPeers_IdenticalShape(t *testing.T) {
|
||||
// (a) No Peers: legacy Address/Port synthesized into a single candidate.
|
||||
cfgNoPeers := &BackendConfig{
|
||||
Address: "127.0.0.1",
|
||||
Port: testPortBase,
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
if err := buildClusterConfig(cfgNoPeers); err != nil {
|
||||
t.Fatalf("buildClusterConfig(no peers) failed: %v", err)
|
||||
}
|
||||
noPeersCandidates := currentClusterCandidates()
|
||||
|
||||
// (b) A single-entry Peers list pointed at the exact same host:port.
|
||||
cfgOnePeer := &BackendConfig{
|
||||
Peers: []string{"127.0.0.1:28581"},
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
if err := buildClusterConfig(cfgOnePeer); err != nil {
|
||||
t.Fatalf("buildClusterConfig(one peer) failed: %v", err)
|
||||
}
|
||||
onePeerCandidates := currentClusterCandidates()
|
||||
|
||||
if len(noPeersCandidates) != 1 {
|
||||
t.Fatalf("no-peers: got %d candidates, want 1", len(noPeersCandidates))
|
||||
}
|
||||
if len(onePeerCandidates) != 1 {
|
||||
t.Fatalf("single-entry-peers: got %d candidates, want 1", len(onePeerCandidates))
|
||||
}
|
||||
if noPeersCandidates[0].Host != onePeerCandidates[0].Host ||
|
||||
noPeersCandidates[0].Port != onePeerCandidates[0].Port {
|
||||
t.Errorf("no-peers and single-entry-peers candidates diverge: %+v vs %+v",
|
||||
noPeersCandidates[0], onePeerCandidates[0])
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildClusterConfig_MultiCandidatePassthrough(t *testing.T) {
|
||||
cfg := &BackendConfig{
|
||||
Peers: []string{
|
||||
"127.0.0.1:28590",
|
||||
"127.0.0.1:28591",
|
||||
"127.0.0.1:28592",
|
||||
},
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
if err := buildClusterConfig(cfg); err != nil {
|
||||
t.Fatalf("buildClusterConfig failed: %v", err)
|
||||
}
|
||||
candidates := currentClusterCandidates()
|
||||
if len(candidates) != 3 {
|
||||
t.Fatalf("got %d candidates, want 3", len(candidates))
|
||||
}
|
||||
for i, want := range []int{28590, 28591, 28592} {
|
||||
if candidates[i].Host != "127.0.0.1" || candidates[i].Port != want {
|
||||
t.Errorf("candidate %d = %+v, want host=127.0.0.1 port=%d", i, candidates[i], want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildClusterConfig_MalformedPeersRejected(t *testing.T) {
|
||||
cfg := &BackendConfig{
|
||||
Peers: []string{"not-a-valid-entry"},
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
err := buildClusterConfig(cfg)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for malformed --peers entry, got nil")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "not-a-valid-entry") {
|
||||
t.Errorf("error %q does not identify the malformed input", err.Error())
|
||||
}
|
||||
}
|
||||
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
// Req 1.1-1.3 -- one xce_backend_init() call carries the full candidate list
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
|
||||
func TestRegisterToBackend_MultiCandidate_SingleInitCallCarriesFullCount(t *testing.T) {
|
||||
captureLog(t) // RegisterToBackend logs via utils.Log; nil otherwise (utils.InitLogger is never called in unit tests)
|
||||
|
||||
cfg := &BackendConfig{
|
||||
Peers: []string{
|
||||
"127.0.0.1:28600",
|
||||
"127.0.0.1:28601",
|
||||
"127.0.0.1:28602",
|
||||
},
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
|
||||
start := time.Now()
|
||||
err := RegisterToBackend(cfg)
|
||||
elapsed := time.Since(start)
|
||||
|
||||
if err == nil {
|
||||
t.Fatal("expected RegisterToBackend to fail against 3 closed local ports")
|
||||
}
|
||||
// Generous bound: closed-port ECONNREFUSED is near-instant, well under a
|
||||
// few seconds even across 3 candidates -- proves this is NOT the deleted
|
||||
// per-candidate Go loop silently retrying/blocking somewhere.
|
||||
if elapsed > 5*time.Second {
|
||||
t.Errorf("RegisterToBackend took %v against closed ports -- too slow, "+
|
||||
"suggests a retry loop is still present", elapsed)
|
||||
}
|
||||
|
||||
// The struct xce_backend_init() was actually called with must carry the
|
||||
// FULL candidate count (nmasters=3), not the hardcoded 1 the deleted
|
||||
// per-peer Go loop always used -- proves ONE call, not three separate
|
||||
// single-candidate calls.
|
||||
if got := currentClientConfigNMasters(); got != 3 {
|
||||
t.Errorf("gClientCfg.nmasters = %d, want 3 (a single call must carry "+
|
||||
"the full candidate count)", got)
|
||||
}
|
||||
}
|
||||
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
// Req 5.1/5.2 -- no-peers and single-entry-peers produce the identical
|
||||
// connect log sequence (shape, not values)
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
|
||||
func TestRegisterToBackend_NoPeersAndSingleEntryPeers_IdenticalLogSequence(t *testing.T) {
|
||||
logs := captureLog(t)
|
||||
|
||||
cfgNoPeers := &BackendConfig{
|
||||
Address: "127.0.0.1",
|
||||
Port: testPortBase + 20,
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
if err := RegisterToBackend(cfgNoPeers); err == nil {
|
||||
t.Fatal("expected RegisterToBackend(no peers) to fail against a closed port")
|
||||
}
|
||||
noPeersMessages := logMessages(logs) // drains the observer buffer
|
||||
|
||||
cfgOnePeer := &BackendConfig{
|
||||
Peers: []string{"127.0.0.1:28601"},
|
||||
From: "xce_apiserver_test",
|
||||
Key: "keepsecret$",
|
||||
}
|
||||
if err := RegisterToBackend(cfgOnePeer); err == nil {
|
||||
t.Fatal("expected RegisterToBackend(single-entry peers) to fail against a closed port")
|
||||
}
|
||||
onePeerMessages := logMessages(logs)
|
||||
|
||||
if len(noPeersMessages) == 0 {
|
||||
t.Fatal("expected at least one log message from RegisterToBackend(no peers)")
|
||||
}
|
||||
if !equalStringSlices(noPeersMessages, onePeerMessages) {
|
||||
t.Errorf("log message sequence diverges between no-peers and "+
|
||||
"single-entry-peers:\n no-peers: %v\n single-entry: %v",
|
||||
noPeersMessages, onePeerMessages)
|
||||
}
|
||||
}
|
||||
|
||||
func logMessages(logs *observer.ObservedLogs) []string {
|
||||
entries := logs.TakeAll()
|
||||
msgs := make([]string, len(entries))
|
||||
for i, e := range entries {
|
||||
msgs[i] = e.Message
|
||||
}
|
||||
return msgs
|
||||
}
|
||||
|
||||
func equalStringSlices(a, b []string) bool {
|
||||
if len(a) != len(b) {
|
||||
return false
|
||||
}
|
||||
for i := range a {
|
||||
if a[i] != b[i] {
|
||||
return false
|
||||
}
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
// Structural regression guard: source config structs are package-level
|
||||
// (source-text scan of the real, current on-disk clib.go -- same genre as
|
||||
// scheduler's/worker's structural regression tests for the identical
|
||||
// use-after-scope hazard).
|
||||
// ════════════════════════════════════════════════════════════════════════
|
||||
|
||||
func TestClibSource_ClusterAndClientConfigsArePackageLevel(t *testing.T) {
|
||||
src, err := os.ReadFile("clib.go")
|
||||
if err != nil {
|
||||
t.Fatalf("could not read clib.go source for scan: %v", err)
|
||||
}
|
||||
text := string(src)
|
||||
|
||||
if !strings.Contains(text, "var gClusterCfg C.xce_cluster_config_t") {
|
||||
t.Error("gClusterCfg must be declared as a package-level var " +
|
||||
"(`var gClusterCfg C.xce_cluster_config_t`) -- a function-local " +
|
||||
"variable would go out of scope while xce_cluster_config_to_client_config()'s " +
|
||||
"aliased pointers (and xcecli_ctx_init()'s retained ctx->conf pointer) " +
|
||||
"are still in use, a use-after-scope bug (see tasks.md's Implementation " +
|
||||
"Notes for task 2.1)")
|
||||
}
|
||||
if !strings.Contains(text, "var gClientCfg C.xce_client_config_t") {
|
||||
t.Error("gClientCfg must be declared as a package-level var " +
|
||||
"(`var gClientCfg C.xce_client_config_t`) -- xcecli_ctx_init() " +
|
||||
"stores a raw pointer to it (ctx->conf) dereferenced for the ctx's " +
|
||||
"entire runtime life, not only the initial connect")
|
||||
}
|
||||
|
||||
// Exactly one xce_backend_init() call site: the former per-peer loop
|
||||
// called it once per candidate (a real loop, not a single call), and the
|
||||
// former no-Peers branch duplicated it a second time in the same
|
||||
// function. A single call site proves both are gone.
|
||||
first := strings.Index(text, "C.xce_backend_init(")
|
||||
if first < 0 {
|
||||
t.Fatal("C.xce_backend_init(...) call site not found in clib.go")
|
||||
}
|
||||
if strings.Index(text[first+1:], "C.xce_backend_init(") >= 0 {
|
||||
t.Error("clib.go must contain exactly ONE C.xce_backend_init() call site -- " +
|
||||
"more than one means the deleted per-peer loop or the separate " +
|
||||
"no-Peers branch is still present")
|
||||
}
|
||||
|
||||
if strings.Contains(text, "func tryConnect(") {
|
||||
t.Error("tryConnect() must be deleted entirely -- its per-call " +
|
||||
"C.CString(host)/C.CString(from)/C.CString(key) + deferred C.free " +
|
||||
"immediately after a single xce_backend_init() call left ctx->conf's " +
|
||||
"aliased masters[]/from/key pointers dangling for the ctx's remaining " +
|
||||
"runtime life (see status report CONCERNS for the precise finding)")
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue