From ce9b6e5b41dbf0e07817e304a860e0dc112ecc67 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:31:59 +1000 Subject: [PATCH] fix: gate cluster-wide IPsec on every chassis being configured (#953) --- spinifex/daemon/daemon.go | 9 +- spinifex/daemon/ipsec_barrier.go | 174 ++++++++ spinifex/daemon/ipsec_barrier_test.go | 153 +++++++ spinifex/network/host/ipsec.go | 52 ++- spinifex/network/host/ipsec_enable.go | 273 +++++++++++- spinifex/network/host/ipsec_enable_test.go | 456 ++++++++++++++++++--- tests/e2e/multinode/ipsec_test.go | 60 ++- 7 files changed, 1084 insertions(+), 93 deletions(-) create mode 100644 spinifex/daemon/ipsec_barrier.go create mode 100644 spinifex/daemon/ipsec_barrier_test.go diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index a457ea23d..6af6c05cf 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1530,10 +1530,11 @@ func (d *Daemon) startCluster() error { } // Reconcile OVN native IPsec both ways (idempotent): a node that does not - // use it should not be left running charon on 500/4500 either. - if err := host.ReconcileOVNIPSec(d.configPath, d.clusterConfig); err != nil { - slog.Warn("Failed to reconcile OVN native IPsec", "err", err) - } + // use it should not be left running charon on 500/4500 either. Runs in the + // background because the first attempt routinely loses the race with + // ovn-central accepting connections, and a node that gives up there stays + // unconfigured while a peer may already require encryption cluster-wide. + go host.MaintainIPSec(d.ctx, d.configPath, d.clusterConfig, d.ipsecBarrier()) // Keep the host firewall's peer sets in step with cluster membership. Every // formation path reaches this, which none of the installers do. Runs in the diff --git a/spinifex/daemon/ipsec_barrier.go b/spinifex/daemon/ipsec_barrier.go new file mode 100644 index 000000000..3f1ad4981 --- /dev/null +++ b/spinifex/daemon/ipsec_barrier.go @@ -0,0 +1,174 @@ +package daemon + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "time" + + "github.com/mulgadc/spinifex/spinifex/network/host" + "github.com/nats-io/nats.go/jetstream" +) + +// ipsecStatusPrefix is the key prefix for a node's IPsec state record in the +// cluster-state bucket. +const ipsecStatusPrefix = "ipsecstatus." + +// ipsecStatusFreshness bounds how long a status record counts for. A node that +// has stopped publishing is not evidence that it is still configured. Must stay +// a large multiple of the interval that refreshes these records. +const ipsecStatusFreshness = 5 * time.Minute + +// ipsecLivenessWindow bounds how long a node counts as live on its heartbeat +// alone. Heartbeats run at heartbeatInterval, so this is six of them: long +// enough to ride out a slow pass, short enough that a node taken out of service +// stops holding the cluster's encryption decision hostage. +const ipsecLivenessWindow = 60 * time.Second + +// ipsecStatusRecord is what a node publishes about its own IPsec state. The +// timestamp is the point of the record: state that cannot go stale cannot +// distinguish a configured peer from one that stopped answering. +type ipsecStatusRecord struct { + Node string `json:"node"` + Ready bool `json:"ready"` + NBReachable bool `json:"nb_reachable"` + Written time.Time `json:"written"` +} + +// KVIPSecBarrier carries per-node IPsec state over the cluster-state KV. +type KVIPSecBarrier struct { + kv jetstream.KeyValue + // now is a var for tests; staleness is otherwise untestable without sleeping. + now func() time.Time +} + +var _ host.IPSecBarrier = (*KVIPSecBarrier)(nil) + +// NewKVIPSecBarrier returns a barrier over kv, or nil if kv is nil. A nil +// barrier is meaningful to the caller: it falls back to local knowledge, which +// is the right answer for a cluster with no peers to be out of step with. +func NewKVIPSecBarrier(kv jetstream.KeyValue) *KVIPSecBarrier { + if kv == nil { + return nil + } + return &KVIPSecBarrier{kv: kv, now: time.Now} +} + +// ipsecBarrier returns the cluster readiness barrier, or a nil interface when +// there is no cluster KV to carry it. Returning the concrete nil pointer would +// give host a non-nil interface holding nil, which is not the same thing. +func (d *Daemon) ipsecBarrier() host.IPSecBarrier { + if d.jsManager == nil || d.jsManager.clusterKV == nil { + return nil + } + return NewKVIPSecBarrier(d.jsManager.clusterKV) +} + +// Publish records this node's own IPsec state. +func (b *KVIPSecBarrier) Publish(ctx context.Context, node string, status host.IPSecNodeStatus) error { + if node == "" { + return errors.New("node name unset") + } + data, err := json.Marshal(ipsecStatusRecord{ + Node: node, + Ready: status.Ready, + NBReachable: status.NBReachable, + Written: b.now().UTC(), + }) + if err != nil { + return fmt.Errorf("marshal IPsec status: %w", err) + } + if _, err := b.kv.Put(ctx, ipsecStatusPrefix+node, data); err != nil { + return fmt.Errorf("put IPsec status for %s: %w", node, err) + } + return nil +} + +// Cluster returns the last published status of every live node. A node with a +// fresh record is live by definition; one without is live only if it is still +// heartbeating, and otherwise drops out of the set entirely. +// +// The two-step matters. A node whose KV writes are failing stops heartbeating +// too, so it leaves the set rather than being read as a chassis that lost its +// configuration — which would strip encryption from the peers still talking. +func (b *KVIPSecBarrier) Cluster(ctx context.Context, nodes []string) (map[string]host.IPSecNodeStatus, error) { + cluster := make(map[string]host.IPSecNodeStatus, len(nodes)) + for _, node := range nodes { + rec, err := b.readStatus(ctx, node) + if err != nil { + return nil, err + } + if rec != nil && b.now().UTC().Sub(rec.Written) <= ipsecStatusFreshness { + cluster[node] = host.IPSecNodeStatus{Ready: rec.Ready, NBReachable: rec.NBReachable} + continue + } + alive, err := b.heartbeating(ctx, node) + if err != nil { + return nil, err + } + if alive { + cluster[node] = host.IPSecNodeStatus{} + } + } + return cluster, nil +} + +// readStatus returns nil for a node that has published nothing, and for a record +// that cannot be parsed — both mean "no usable claim of readiness". +func (b *KVIPSecBarrier) readStatus(ctx context.Context, node string) (*ipsecStatusRecord, error) { + entry, err := b.kv.Get(ctx, ipsecStatusPrefix+node) + if errors.Is(err, jetstream.ErrKeyNotFound) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("get IPsec status for %s: %w", node, err) + } + rec, ok := decodeStatus(entry.Value()) + if !ok { + return nil, nil + } + return &rec, nil +} + +// decodeStatus reports usability rather than an error: a record that cannot be +// parsed is not a claim of readiness, which is a state this cares about and not +// a failure to read the KV. +func decodeStatus(data []byte) (ipsecStatusRecord, bool) { + var rec ipsecStatusRecord + if json.Unmarshal(data, &rec) != nil { + return ipsecStatusRecord{}, false + } + return rec, true +} + +// heartbeating reports whether the node has written a heartbeat recently enough +// to still count as part of the cluster. +func (b *KVIPSecBarrier) heartbeating(ctx context.Context, node string) (bool, error) { + entry, err := b.kv.Get(ctx, "heartbeat."+node) + if errors.Is(err, jetstream.ErrKeyNotFound) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("get heartbeat for %s: %w", node, err) + } + written, ok := decodeHeartbeatTime(entry.Value()) + if !ok { + return false, nil + } + return b.now().UTC().Sub(written) <= ipsecLivenessWindow, nil +} + +// decodeHeartbeatTime returns when the heartbeat was written. A beat that cannot +// be parsed or dated vouches for nothing, which is an answer rather than a fault. +func decodeHeartbeatTime(data []byte) (time.Time, bool) { + var h Heartbeat + if json.Unmarshal(data, &h) != nil { + return time.Time{}, false + } + written, err := time.Parse(time.RFC3339, h.Timestamp) + if err != nil { + return time.Time{}, false + } + return written.UTC(), true +} diff --git a/spinifex/daemon/ipsec_barrier_test.go b/spinifex/daemon/ipsec_barrier_test.go new file mode 100644 index 000000000..ebd31c0f9 --- /dev/null +++ b/spinifex/daemon/ipsec_barrier_test.go @@ -0,0 +1,153 @@ +//test:in-package the barrier's staleness rules are only testable from inside: +// they need the clock seam, the freshness constants, the shared JetStream test +// server, and jsManager.clusterKV to seed heartbeats against. + +package daemon + +import ( + "testing" + "time" + + "github.com/mulgadc/spinifex/spinifex/network/host" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestIPSecBarrier(t *testing.T) (*KVIPSecBarrier, *JetStreamManager) { + t.Helper() + + nc, err := nats.Connect(sharedJSNATSURL) + require.NoError(t, err) + t.Cleanup(nc.Close) + + jsm, err := NewJetStreamManager(nc, 1) + require.NoError(t, err) + require.NoError(t, jsm.InitClusterStateBucket()) + + barrier := NewKVIPSecBarrier(jsm.clusterKV) + require.NotNil(t, barrier) + return barrier, jsm +} + +// beat marks a node live without saying anything about its IPsec state. +func beat(t *testing.T, jsm *JetStreamManager, node string) { + t.Helper() + require.NoError(t, jsm.WriteHeartbeat(&Heartbeat{ + Node: node, + Timestamp: time.Now().UTC().Format(time.RFC3339), + })) +} + +func TestKVIPSecBarrier_ReportsEveryLiveNodesState(t *testing.T) { + barrier, jsm := newTestIPSecBarrier(t) + nodes := []string{"ipsec-a", "ipsec-b"} + for _, n := range nodes { + beat(t, jsm, n) + } + + require.NoError(t, barrier.Publish(t.Context(), "ipsec-a", host.IPSecNodeStatus{Ready: true, NBReachable: true})) + + cluster, err := barrier.Cluster(t.Context(), nodes) + require.NoError(t, err) + assert.Equal(t, map[string]host.IPSecNodeStatus{ + "ipsec-a": {Ready: true, NBReachable: true}, + "ipsec-b": {}, + }, cluster) + + require.NoError(t, barrier.Publish(t.Context(), "ipsec-b", host.IPSecNodeStatus{Ready: true})) + + cluster, err = barrier.Cluster(t.Context(), nodes) + require.NoError(t, err) + assert.Equal(t, map[string]host.IPSecNodeStatus{ + "ipsec-a": {Ready: true, NBReachable: true}, + "ipsec-b": {Ready: true}, + }, cluster) +} + +// A node that publishes Ready=false has said its own setup is incomplete, which +// is exactly the case the flag must not be asserted over. +func TestKVIPSecBarrier_ExplicitlyUnreadyNodeIsReported(t *testing.T) { + barrier, jsm := newTestIPSecBarrier(t) + beat(t, jsm, "ipsec-unready") + + require.NoError(t, barrier.Publish(t.Context(), "ipsec-unready", host.IPSecNodeStatus{})) + + cluster, err := barrier.Cluster(t.Context(), []string{"ipsec-unready"}) + require.NoError(t, err) + assert.Equal(t, map[string]host.IPSecNodeStatus{"ipsec-unready": {}}, cluster) +} + +// A node taken out of service has no chassis registered, so there is no tunnel +// to it to black-hole. Reporting it as unconfigured would strip encryption from +// the peers that are still talking to each other. +func TestKVIPSecBarrier_NodeThatStoppedHeartbeatingDropsOutOfTheSet(t *testing.T) { + barrier, jsm := newTestIPSecBarrier(t) + beat(t, jsm, "ipsec-gone") + + require.NoError(t, barrier.Publish(t.Context(), "ipsec-gone", host.IPSecNodeStatus{Ready: true})) + + // Far enough ahead that both the status record and the heartbeat are stale. + barrier.now = func() time.Time { return time.Now().Add(ipsecStatusFreshness + time.Hour) } + + cluster, err := barrier.Cluster(t.Context(), []string{"ipsec-gone"}) + require.NoError(t, err) + assert.Empty(t, cluster) +} + +// Still heartbeating but its status has gone stale: the node is up and can no +// longer vouch for its own configuration, so it counts as not ready. +func TestKVIPSecBarrier_LiveNodeWithAStaleStatusIsNotReady(t *testing.T) { + barrier, jsm := newTestIPSecBarrier(t) + + require.NoError(t, barrier.Publish(t.Context(), "ipsec-stale", host.IPSecNodeStatus{Ready: true})) + + // The status is stale, but a heartbeat lands at the shifted clock. + shifted := time.Now().Add(ipsecStatusFreshness + time.Minute) + barrier.now = func() time.Time { return shifted } + require.NoError(t, jsm.WriteHeartbeat(&Heartbeat{ + Node: "ipsec-stale", + Timestamp: shifted.UTC().Format(time.RFC3339), + })) + + cluster, err := barrier.Cluster(t.Context(), []string{"ipsec-stale"}) + require.NoError(t, err) + assert.Equal(t, map[string]host.IPSecNodeStatus{"ipsec-stale": {}}, cluster) +} + +// A node that has never published anything and never heartbeated is not part of +// the running cluster yet. +func TestKVIPSecBarrier_UnknownNodeIsAbsent(t *testing.T) { + barrier, _ := newTestIPSecBarrier(t) + + cluster, err := barrier.Cluster(t.Context(), []string{"ipsec-never-seen"}) + require.NoError(t, err) + assert.Empty(t, cluster) +} + +func TestKVIPSecBarrier_PublishRequiresANodeName(t *testing.T) { + barrier, _ := newTestIPSecBarrier(t) + + err := barrier.Publish(t.Context(), "", host.IPSecNodeStatus{Ready: true}) + require.Error(t, err) + assert.Contains(t, err.Error(), "node name unset") +} + +// A nil KV means no cluster to be out of step with, and the caller relies on a +// nil interface rather than a typed nil to tell. +func TestNewKVIPSecBarrier_NilKVYieldsNoBarrier(t *testing.T) { + assert.Nil(t, NewKVIPSecBarrier(nil)) +} + +// assert.Nil passes for a typed nil, so the interface conversion is checked with +// == instead: a non-nil interface holding a nil pointer panics in host. +func TestDaemonIPSecBarrier_NilManagerYieldsANilInterface(t *testing.T) { + d := &Daemon{} + assert.Equal(t, host.IPSecBarrier(nil), d.ipsecBarrier()) +} + +// The heartbeat that keeps a node in the set must be refreshed far more often +// than it is allowed to age, or a healthy node drops out between beats. +func TestIPSecLivenessWindowExceedsHeartbeatInterval(t *testing.T) { + assert.Greater(t, ipsecLivenessWindow, 3*heartbeatInterval) +} diff --git a/spinifex/network/host/ipsec.go b/spinifex/network/host/ipsec.go index 86997c93b..dfbd04e0e 100644 --- a/spinifex/network/host/ipsec.go +++ b/spinifex/network/host/ipsec.go @@ -1,6 +1,8 @@ package host import ( + "bytes" + "errors" "fmt" "strings" @@ -35,15 +37,61 @@ func EnableIPSecEncapsulation() error { return nil } +// errNBUnreachable marks the one failure that means "this node has no local NB +// DB" rather than "the read broke". A permission change on the socket, a missing +// binary or a timed-out transaction are all real faults on a management node, +// and lumping them in with this would silence them. +var errNBUnreachable = errors.New("no local OVN NB DB") + +// nbUnreachablePatterns are what ovsdb's client library prints when it cannot +// open or complete a handshake on the socket. +var nbUnreachablePatterns = []string{ + "database connection failed", + "connection refused", + "no such file or directory", +} + +// GetNBGlobalIPSec reads NB_Global.ipsec from the local OVN NB DB. The error +// doubles as the reachability answer: a present socket file says nothing about +// whether the database behind it accepts connections yet. +// +// Reads stdout alone. ovn-nbctl writes vlog lines to stderr on a successful run, +// and folding those into the value parses a live "true" as false. +func GetNBGlobalIPSec() (bool, error) { + cmd := utils.SudoCommand("ovn-nbctl", "--timeout=5", "get", "NB_Global", ".", "ipsec") + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + msg := strings.TrimSpace(stderr.String()) + wrapped := fmt.Errorf("get NB_Global ipsec: %s: %w", msg, err) + if isNBUnreachable(msg) { + return false, fmt.Errorf("%w: %w", errNBUnreachable, wrapped) + } + return false, wrapped + } + return strings.Trim(strings.TrimSpace(string(out)), `"`) == "true", nil +} + +func isNBUnreachable(msg string) bool { + lower := strings.ToLower(msg) + for _, pattern := range nbUnreachablePatterns { + if strings.Contains(lower, pattern) { + return true + } + } + return false +} + // SetNBGlobalIPSec writes NB_Global.ipsec on the local OVN NB DB, triggering // ovn-controller to add options:remote_name to Geneve tunnels for strongSwan. -// Only the management node has a local NB socket; callers gate on presence. +// Only the management node has a reachable NB DB; callers gate on that. func SetNBGlobalIPSec(enable bool) error { val := "false" if enable { val = "true" } - out, err := utils.SudoCommand("ovn-nbctl", "set", "NB_Global", ".", "ipsec="+val).CombinedOutput() + out, err := utils.SudoCommand("ovn-nbctl", "--timeout=5", "set", "NB_Global", ".", "ipsec="+val).CombinedOutput() if err != nil { return fmt.Errorf("set NB_Global ipsec=%s: %s: %w", val, strings.TrimSpace(string(out)), err) } diff --git a/spinifex/network/host/ipsec_enable.go b/spinifex/network/host/ipsec_enable.go index ad6cbce7f..ec2edd4fd 100644 --- a/spinifex/network/host/ipsec_enable.go +++ b/spinifex/network/host/ipsec_enable.go @@ -1,23 +1,63 @@ package host import ( + "context" + "errors" "fmt" "log/slog" + "maps" "os" "path/filepath" + "slices" "strings" "time" "github.com/mulgadc/spinifex/spinifex/admin" "github.com/mulgadc/spinifex/spinifex/config" + "github.com/mulgadc/spinifex/spinifex/otelsetup" "github.com/mulgadc/spinifex/spinifex/utils" ) // systemctlActiveTimeout bounds the wait for openvswitch-ipsec.service to become active. var systemctlActiveTimeout = 5 * time.Second -// ovnNBSocketPath gates the NB_Global ipsec write to the management node. -var ovnNBSocketPath = "/run/ovn/ovnnb_db.sock" +// ipsecRetryDelay is the first gap after a failed pass; it doubles up to +// ipsecReconcileInterval. Short, because until every chassis has finished its +// local setup the cluster cannot require encryption at all. +var ipsecRetryDelay = 3 * time.Second + +// ipsecReconcileInterval re-runs a successful pass. It doubles as the readiness +// heartbeat, so it must stay well inside whatever freshness window the barrier +// applies to the records this publishes. +var ipsecReconcileInterval = 60 * time.Second + +// IPSecNodeStatus is one node's published view of its own IPsec state. +type IPSecNodeStatus struct { + // Ready reports that this node's local IPsec configuration is complete. + Ready bool + + // NBReachable reports that this node can read and write NB_Global. Only + // management nodes can, and which nodes those are is not in the config, so + // they say so here and every node elects the same writer from the answer. + NBReachable bool +} + +// IPSecBarrier carries each node's IPsec state across the cluster. +// NB_Global.ipsec is cluster-wide, so asserting it from one node's local +// knowledge is what lets a partially configured mesh black-hole guest traffic. +// The interface keeps the transport (JetStream KV) out of L0. +type IPSecBarrier interface { + // Publish records this node's own state. Callers publish on every pass, so + // a live node's record stays fresh and a stopped one's goes stale. + Publish(ctx context.Context, node string, status IPSecNodeStatus) error + + // Cluster returns the last published status of every named node that is + // still live. A node that is not live is absent from the map rather than + // unready: it has no chassis registered, so there is no tunnel to it to + // black-hole, and demanding plaintext on its behalf would only strip + // encryption from the traffic that is still flowing. + Cluster(ctx context.Context, nodes []string) (map[string]IPSecNodeStatus, error) +} const ( // ovsIPSecUnit owns charon: ovs-monitor-ipsec execs the strongSwan starter @@ -34,26 +74,108 @@ const ( // root-equivalent, so unit changes go through this instead. var ipsecStateHelper = "/usr/local/lib/spinifex/spinifex-set-ipsec-state" +// MaintainIPSec schedules ReconcileOVNIPSec until it succeeds, then re-runs it +// at ipsecReconcileInterval for the lifetime of ctx. A scheduler only. +// +// A single attempt at startup routinely loses the race with ovn-central +// accepting connections, and dropping that attempt leaves the node's IPsec +// unconfigured until something restarts the daemon — while another node may +// already have made encryption mandatory cluster-wide. +func MaintainIPSec(ctx context.Context, configPath string, clusterConfig *config.ClusterConfig, barrier IPSecBarrier) { + // Said once, because a pass that reconciles to nothing logs nothing, which in + // a journal is indistinguishable from a node that armed. + switch { + case clusterConfig == nil: + slog.Info("ipsec: cluster membership unknown, leaving the host's IPsec services alone") + case !ipsecWanted(clusterConfig): + slog.Info("ipsec: not in use on this cluster, IKE and NAT-T listeners stay stopped") + } + + delay, announced := ipsecRetryDelay, false + for { + err := ReconcileOVNIPSec(ctx, configPath, clusterConfig, barrier) + switch { + case err != nil: + delay = min(delay*2, ipsecReconcileInterval) + slog.Warn("Failed to reconcile OVN native IPsec, will retry", "err", err, "retry_in_ms", otelsetup.Millis(delay)) + default: + // Only the first success is news. The steady state re-runs every + // interval for the lifetime of the daemon, and saying so each time + // buries the passes that changed something. + if !announced && ipsecWanted(clusterConfig) { + slog.Info("ipsec: local IPsec configuration complete on this node") + announced = true + } + delay = ipsecReconcileInterval + } + + select { + case <-ctx.Done(): + return + case <-time.After(delay): + } + } +} + +// ipsecWanted reports the cluster's intent. Single-node clusters have no Geneve +// tunnels to protect, so charon must not be left listening on 500/4500 there +// even though ipsec_enabled defaults to true. +func ipsecWanted(clusterConfig *config.ClusterConfig) bool { + return clusterConfig != nil && clusterConfig.Network.IPSecEnabled && len(clusterConfig.Nodes) > 1 +} + // ReconcileOVNIPSec brings the host's IPsec services in line with the cluster // config, then enables OVN IPsec if it is wanted. Runs on every startup so the // disabled case is reached too, which EnableOVNIPSec alone never is. -func ReconcileOVNIPSec(configPath string, clusterConfig *config.ClusterConfig) error { +func ReconcileOVNIPSec(ctx context.Context, configPath string, clusterConfig *config.ClusterConfig, barrier IPSecBarrier) error { // A nil config means the intent is unknown; leave the host's services alone // rather than guessing and tearing down working tunnels. if clusterConfig == nil { return nil } - want := clusterConfig.Network.IPSecEnabled && len(clusterConfig.Nodes) > 1 + want := ipsecWanted(clusterConfig) if err := ensureIPSecServices(want); err != nil { return err } if !want { - slog.Info("ipsec: not in use, IKE and NAT-T listeners stopped") + return DisableOVNIPSec(ctx, clusterConfig, barrier) + } + return EnableOVNIPSec(ctx, configPath, clusterConfig, barrier) +} + +// DisableOVNIPSec is the off direction, and it has to reach NB_Global. Charon is +// already stopped by the time this runs, so a flag left asserted demands +// encryption that no chassis can perform — the same black hole as a partial +// enable, reached by turning IPsec off instead of on. +func DisableOVNIPSec(ctx context.Context, clusterConfig *config.ClusterConfig, barrier IPSecBarrier) error { + if barrier != nil && clusterConfig != nil { + if err := barrier.Publish(ctx, clusterConfig.Node, IPSecNodeStatus{}); err != nil { + return fmt.Errorf("publish IPsec state: %w", err) + } + } + + current, err := GetNBGlobalIPSec() + if err != nil { + return skipUnlessNBReadable(err) + } + if !current { return nil } - return EnableOVNIPSec(configPath, clusterConfig) + slog.Info("ipsec: switched off for this cluster, releasing the cluster-wide encryption requirement") + return SetNBGlobalIPSec(false) +} + +// skipUnlessNBReadable turns "this node has no local NB DB" into a quiet skip +// and leaves every other failure — a socket it may not open, a missing binary, +// a timed-out transaction — as a real error, so the pass retries and says so. +func skipUnlessNBReadable(err error) error { + if errors.Is(err, errNBUnreachable) { + slog.Debug("ipsec: no local OVN NB DB, leaving NB_Global to a management node", "err", err) + return nil + } + return err } // ensureIPSecServices runs the helper only when the host does not already match @@ -99,14 +221,47 @@ func unitIsActive(unit string) bool { // EnableOVNIPSec wires the local IPsec peer cert and flips ipsec_encapsulation=true. // Idempotent. Single-node clusters short-circuit (no Geneve tunnels to encrypt). // Lives in L0 per ADR-0006 S8 (IPSec is OVN-native only; SA lifecycle invisible above L0). -func EnableOVNIPSec(configPath string, clusterConfig *config.ClusterConfig) error { +func EnableOVNIPSec(ctx context.Context, configPath string, clusterConfig *config.ClusterConfig, barrier IPSecBarrier) error { if configPath == "" { return fmt.Errorf("config path unset") } if clusterConfig != nil && len(clusterConfig.Nodes) <= 1 { - slog.Info("ipsec: single-node cluster, skipping enable (no peers)") + slog.Debug("ipsec: single-node cluster, skipping enable (no peers)") return nil } + + // Probed before the local half, because the answer is published alongside it: + // which nodes can write NB_Global is not in the config, so they have to say. + current, nbErr := GetNBGlobalIPSec() + if nbErr != nil && !errors.Is(nbErr, errNBUnreachable) { + return nbErr + } + status := IPSecNodeStatus{NBReachable: nbErr == nil} + + if err := configureLocalIPSec(configPath); err != nil { + // Said out loud, not left to expire. A record that only goes stale keeps + // this chassis counted as configured for a whole freshness window, which + // is the black hole again with a slower fuse. + publishStatus(ctx, clusterConfig, barrier, status) + return err + } + status.Ready = true + + if barrier != nil && clusterConfig != nil { + if err := barrier.Publish(ctx, clusterConfig.Node, status); err != nil { + return fmt.Errorf("publish IPsec state: %w", err) + } + } + + if nbErr != nil { + return skipUnlessNBReadable(nbErr) + } + return reconcileNBGlobalIPSec(ctx, clusterConfig, barrier, current) +} + +// configureLocalIPSec does this node's own half: point OVS at the peer cert and +// turn on encapsulation, once ovs-monitor-ipsec is up to act on both. +func configureLocalIPSec(configPath string) error { configDir := filepath.Dir(configPath) certPath, keyPath := admin.IPSecCertPaths(configDir) caCertPath := filepath.Join(configDir, "ca.pem") @@ -124,26 +279,102 @@ func EnableOVNIPSec(configPath string, clusterConfig *config.ClusterConfig) erro if err := SetIPSecCertPaths(certPath, keyPath, caCertPath); err != nil { return err } + return EnableIPSecEncapsulation() +} - if err := EnableIPSecEncapsulation(); err != nil { +// publishStatus is the best-effort form used on failure paths, where the error +// being reported matters more than the one this might add. +func publishStatus(ctx context.Context, clusterConfig *config.ClusterConfig, barrier IPSecBarrier, status IPSecNodeStatus) { + if barrier == nil || clusterConfig == nil { + return + } + if err := barrier.Publish(ctx, clusterConfig.Node, status); err != nil { + slog.Warn("ipsec: could not publish this node's state, peers will wait for it to go stale", "err", err) + } +} + +// reconcileNBGlobalIPSec holds NB_Global.ipsec in step with the whole cluster. +// Without this flag ovn-controller skips options:remote_name on Geneve tunnels +// and ovs-monitor-ipsec materialises no strongSwan connections; with it, a +// chassis that has not finished its own setup silently drops guest traffic. So +// it tracks the slowest live chassis, not this one. +func reconcileNBGlobalIPSec(ctx context.Context, clusterConfig *config.ClusterConfig, barrier IPSecBarrier, current bool) error { + if barrier == nil || clusterConfig == nil { + if current { + return nil + } + return SetNBGlobalIPSec(true) + } + + cluster, err := barrier.Cluster(ctx, nodeNames(clusterConfig)) + if err != nil { + // Unreadable is not evidence that a chassis is unconfigured. Retracting on + // it would drop a working encrypted mesh to plaintext over a KV outage. + return fmt.Errorf("read IPsec cluster state: %w", err) + } + + // One writer. Every management node can reach the NB DB, so without this they + // race on one cluster-global row from snapshots taken at different instants, + // and two that disagree for a single pass flip the flag — tearing down and + // rebuilding every strongSwan connection in the cluster on each flip. + writer := nbGlobalWriter(cluster) + if writer != clusterConfig.Node { + slog.Debug("ipsec: another node owns the NB_Global write this pass", "writer", writer) + return nil + } + + pending := notReady(cluster) + switch { + case len(pending) == 0 && current: + return nil + case len(pending) == 0: + slog.Info("ipsec: every live chassis reports a complete configuration, requiring encryption cluster-wide") + return SetNBGlobalIPSec(true) + case !current: + slog.Info("ipsec: holding encryption off until every chassis is configured", "pending", pending) + return nil + } + + // Plaintext Geneve is where the cluster sat before IPsec was asked for, and it + // is both recoverable and visible. A black hole is neither. + slog.Error("ipsec: retracting cluster-wide encryption, chassis are unconfigured and cross-chassis guest traffic would black-hole", "pending", pending) + if err := SetNBGlobalIPSec(false); err != nil { + slog.Error("ipsec: NB_Global still demands encryption that unconfigured chassis cannot perform, cross-chassis guest traffic is being dropped now", + "pending", pending, "err", err) return err } + return nil +} - // NB_Global.ipsec is cluster-wide; only the management node has a local NB socket. - // Without this flag, ovn-controller skips adding options:remote_name to Geneve - // tunnels and ovs-monitor-ipsec never materialises strongSwan connections. - if _, err := os.Stat(ovnNBSocketPath); err == nil { - if err := SetNBGlobalIPSec(true); err != nil { - return err +// nbGlobalWriter picks the one node that may write NB_Global this pass: the +// first live management node in name order. Every node computes it from the same +// published set, so they agree without a lock. +func nbGlobalWriter(cluster map[string]IPSecNodeStatus) string { + var writer string + for _, node := range slices.Sorted(maps.Keys(cluster)) { + if cluster[node].NBReachable { + writer = node + break } } + return writer +} - slog.Info("OVN native IPsec enabled on intra-AZ Geneve", - "cert", certPath, - "key", keyPath, - "ca", caCertPath, - ) - return nil +// notReady names the live nodes whose own IPsec configuration is incomplete. +func notReady(cluster map[string]IPSecNodeStatus) []string { + var pending []string + for _, node := range slices.Sorted(maps.Keys(cluster)) { + if !cluster[node].Ready { + pending = append(pending, node) + } + } + return pending +} + +// nodeNames returns cluster membership in a stable order so logged pending sets +// do not churn between passes. +func nodeNames(clusterConfig *config.ClusterConfig) []string { + return slices.Sorted(maps.Keys(clusterConfig.Nodes)) } // ensureOVSMonitorIPSecActive polls openvswitch-ipsec.service for "active". diff --git a/spinifex/network/host/ipsec_enable_test.go b/spinifex/network/host/ipsec_enable_test.go index 00be84469..c3643b7fb 100644 --- a/spinifex/network/host/ipsec_enable_test.go +++ b/spinifex/network/host/ipsec_enable_test.go @@ -1,10 +1,14 @@ package host import ( + "context" + "errors" "os" "os/exec" "path/filepath" + "slices" "strings" + "sync" "testing" "time" @@ -29,10 +33,33 @@ type recordingSudo struct { activeOutput string enabledOutput map[string]string activePerUnit map[string]string + // nbUnreachable stands in for a node whose ovn-central is not answering, or + // which is not a management node at all. The socket file exists in both + // cases, which is why its presence was never the right question. + nbUnreachable bool + // nbIPSec is what NB_Global.ipsec currently reads as. + nbIPSec string + // nbError is a failure that is not "there is no local NB DB here" — a + // permission change on the socket, a missing binary, a timed-out + // transaction. On a management node each of those is a real fault. + nbError string } func (r *recordingSudo) stub(name string, args ...string) *exec.Cmd { r.runs = append(r.runs, append([]string{name}, args...)) + if name == "ovn-nbctl" { + if r.nbError != "" { + return exec.Command("sh", "-c", `echo "`+r.nbError+`" >&2; exit 1`) + } + if r.nbUnreachable { + return exec.Command("sh", "-c", + `echo "ovn-nbctl: unix:/var/run/ovn/ovnnb_db.sock: database connection failed" >&2; exit 1`) + } + if slices.Contains(args, "get") { + return exec.Command("printf", "%s\n", r.nbIPSec) + } + return exec.Command("true") + } if name != "systemctl" || len(args) < 2 { return exec.Command("true") } @@ -74,64 +101,186 @@ func multiNodeIPSecConfig(enabled bool) *config.ClusterConfig { return cfg } -func TestEnableOVNIPSec(t *testing.T) { - recorder := &recordingSudo{} - t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) +// fakeBarrier stands in for the cluster readiness channel. +type fakeBarrier struct { + published []IPSecNodeStatus + publishedNode string + publishErr error + cluster map[string]IPSecNodeStatus + clusterErr error +} + +func (f *fakeBarrier) Publish(_ context.Context, node string, status IPSecNodeStatus) error { + f.publishedNode = node + f.published = append(f.published, status) + return f.publishErr +} + +func (f *fakeBarrier) Cluster(_ context.Context, _ []string) (map[string]IPSecNodeStatus, error) { + return f.cluster, f.clusterErr +} + +// allReady is the steady state: every chassis has finished its local setup and +// node1 is the management node that owns the NB_Global write. +func allReady() *fakeBarrier { + return &fakeBarrier{cluster: map[string]IPSecNodeStatus{ + "node1": {Ready: true, NBReachable: true}, + "node2": {Ready: true}, + }} +} + +// node2Pending is a cluster where this node is ready and its peer is not. +func node2Pending() *fakeBarrier { + return &fakeBarrier{cluster: map[string]IPSecNodeStatus{ + "node1": {Ready: true, NBReachable: true}, + "node2": {}, + }} +} + +// ipsecTestConfigDir lays out the credentials EnableOVNIPSec insists on. +func ipsecTestConfigDir(t *testing.T) string { + t.Helper() configDir := t.TempDir() configPath := filepath.Join(configDir, "spinifex.toml") require.NoError(t, os.WriteFile(configPath, []byte("placeholder"), 0600)) - for _, rel := range []string{"ca.pem", "ipsec/peer.pem", "ipsec/peer.key"} { full := filepath.Join(configDir, rel) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0750)) require.NoError(t, os.WriteFile(full, []byte("x"), 0600)) } + return configDir +} - // Worker path: no local NB socket → no ovn-nbctl call. - origNBSock := ovnNBSocketPath - ovnNBSocketPath = filepath.Join(configDir, "no-such-socket") - t.Cleanup(func() { ovnNBSocketPath = origNBSock }) - - require.NoError(t, EnableOVNIPSec(configPath, multiNodeClusterConfig())) - - require.Len(t, recorder.runs, 3) - assert.Equal(t, []string{"systemctl", "is-active", "openvswitch-ipsec.service"}, recorder.runs[0]) - for _, run := range recorder.runs[1:] { - assert.Equal(t, "ovs-vsctl", run[0]) - assert.Equal(t, "set", run[1]) - assert.Equal(t, "Open_vSwitch", run[2]) +// nbctlWrites returns the ovn-nbctl invocations that write, dropping the reads. +// Matched on content: keying off an argument index would silently return +// nothing the moment anyone adds a flag, turning every "no write happened" +// assertion into one that asserts nothing. +func (r *recordingSudo) nbctlWrites() [][]string { + var out [][]string + for _, run := range r.runs { + if run[0] == "ovn-nbctl" && slices.Contains(run, "set") { + out = append(out, run) + } } - joined := strings.Join(recorder.runs[1], " ") - assert.Contains(t, joined, "other_config:certificate="+filepath.Join(configDir, "ipsec", "peer.pem")) - assert.Contains(t, joined, "other_config:private_key="+filepath.Join(configDir, "ipsec", "peer.key")) - assert.Contains(t, joined, "other_config:ca_cert="+filepath.Join(configDir, "ca.pem")) - assert.Contains(t, strings.Join(recorder.runs[2], " "), "other_config:ipsec_encapsulation=true") + return out +} + +// ovsSets returns the ovs-vsctl writes, joined for substring assertions. +func (r *recordingSudo) ovsSets() []string { + var out []string + for _, run := range r.runs { + if run[0] == "ovs-vsctl" && slices.Contains(run, "set") { + out = append(out, strings.Join(run, " ")) + } + } + return out +} + +func TestEnableOVNIPSec(t *testing.T) { + recorder := &recordingSudo{nbUnreachable: true} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configDir := ipsecTestConfigDir(t) + configPath := filepath.Join(configDir, "spinifex.toml") + + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), allReady())) + + sets := recorder.ovsSets() + require.Len(t, sets, 2) + assert.Contains(t, sets[0], "other_config:certificate="+filepath.Join(configDir, "ipsec", "peer.pem")) + assert.Contains(t, sets[0], "other_config:private_key="+filepath.Join(configDir, "ipsec", "peer.key")) + assert.Contains(t, sets[0], "other_config:ca_cert="+filepath.Join(configDir, "ca.pem")) + assert.Contains(t, sets[1], "other_config:ipsec_encapsulation=true") + + // The NB DB is unreachable, so the read is attempted and nothing is written. + assert.Empty(t, recorder.nbctlWrites()) } func TestEnableOVNIPSec_Management(t *testing.T) { recorder := &recordingSudo{} t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) - configDir := t.TempDir() - configPath := filepath.Join(configDir, "spinifex.toml") - require.NoError(t, os.WriteFile(configPath, []byte("placeholder"), 0600)) - for _, rel := range []string{"ca.pem", "ipsec/peer.pem", "ipsec/peer.key"} { - full := filepath.Join(configDir, rel) - require.NoError(t, os.MkdirAll(filepath.Dir(full), 0750)) - require.NoError(t, os.WriteFile(full, []byte("x"), 0600)) - } + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") - sockPath := filepath.Join(configDir, "ovnnb_db.sock") - require.NoError(t, os.WriteFile(sockPath, []byte{}, 0600)) - origNBSock := ovnNBSocketPath - ovnNBSocketPath = sockPath - t.Cleanup(func() { ovnNBSocketPath = origNBSock }) + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), allReady())) - require.NoError(t, EnableOVNIPSec(configPath, multiNodeClusterConfig())) + assert.Equal(t, [][]string{{"ovn-nbctl", "--timeout=5", "set", "NB_Global", ".", "ipsec=true"}}, + recorder.nbctlWrites()) +} - require.Len(t, recorder.runs, 4) - assert.Equal(t, []string{"ovn-nbctl", "set", "NB_Global", ".", "ipsec=true"}, recorder.runs[3]) +// The reported incident: one node asserts encryption cluster-wide while the +// others have not finished, and every guest crossing chassis black-holes. +func TestEnableOVNIPSec_HoldsFlagUntilEveryChassisIsReady(t *testing.T) { + recorder := &recordingSudo{} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + barrier := node2Pending() + + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), barrier)) + + assert.Equal(t, "node1", barrier.publishedNode) + assert.Equal(t, []IPSecNodeStatus{{Ready: true, NBReachable: true}}, barrier.published, + "the local half completed, so this node must report itself ready") + assert.Empty(t, recorder.nbctlWrites(), + "NB_Global must not be asserted while a chassis is unconfigured") +} + +// A flag left asserted over an unconfigured chassis drops guest traffic on the +// floor; plaintext is the state the cluster had before IPsec was asked for. +func TestEnableOVNIPSec_RetractsFlagWhenAChassisRegresses(t *testing.T) { + recorder := &recordingSudo{nbIPSec: "true"} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), + node2Pending())) + + assert.Equal(t, [][]string{{"ovn-nbctl", "--timeout=5", "set", "NB_Global", ".", "ipsec=false"}}, + recorder.nbctlWrites()) +} + +// An unreadable barrier is not evidence that a chassis is unconfigured, so it +// must never downgrade a working encrypted mesh to plaintext. +func TestEnableOVNIPSec_BarrierErrorLeavesTheFlagAlone(t *testing.T) { + recorder := &recordingSudo{nbIPSec: "true"} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + + err := EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), + &fakeBarrier{clusterErr: errors.New("kv unavailable")}) + require.Error(t, err) + assert.Contains(t, err.Error(), "read IPsec cluster state") + assert.Empty(t, recorder.nbctlWrites()) +} + +// Already true and everyone ready: no write, so a steady-state pass is free. +func TestEnableOVNIPSec_AlreadyAssertedIsANoOp(t *testing.T) { + recorder := &recordingSudo{nbIPSec: "true"} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), allReady())) + assert.Empty(t, recorder.nbctlWrites()) +} + +// A node that cannot publish must fail its pass rather than proceed: silently +// dropping the record would make it pending forever to everyone else. +func TestEnableOVNIPSec_PublishFailureFailsThePass(t *testing.T) { + recorder := &recordingSudo{} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + + err := EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), + &fakeBarrier{cluster: allReady().cluster, publishErr: errors.New("kv unavailable")}) + require.Error(t, err) + assert.Contains(t, err.Error(), "publish IPsec state") + assert.Empty(t, recorder.nbctlWrites()) } func TestEnableOVNIPSec_SingleNodeSkip(t *testing.T) { @@ -148,7 +297,7 @@ func TestEnableOVNIPSec_SingleNodeSkip(t *testing.T) { Node: "node1", Nodes: map[string]config.Config{"node1": {}}, } - require.NoError(t, EnableOVNIPSec(configPath, cfg)) + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, cfg, allReady())) } func TestEnableOVNIPSec_MonitorIPSecInactive(t *testing.T) { @@ -168,7 +317,7 @@ func TestEnableOVNIPSec_MonitorIPSecInactive(t *testing.T) { require.NoError(t, os.WriteFile(full, []byte("x"), 0600)) } - err := EnableOVNIPSec(configPath, multiNodeClusterConfig()) + err := EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), allReady()) require.Error(t, err) assert.Contains(t, err.Error(), "ovs-monitor-ipsec") assert.Contains(t, err.Error(), "not active") @@ -180,23 +329,24 @@ func TestEnableOVNIPSec_MonitorIPSecInactive(t *testing.T) { } func TestEnableOVNIPSec_MissingCert(t *testing.T) { - t.Cleanup(utils.SetSudoCommandForTest(func(name string, args ...string) *exec.Cmd { - t.Fatalf("utils.SudoCommand must not run when cert files are absent") - return exec.Command("true") - })) + recorder := &recordingSudo{} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) configDir := t.TempDir() configPath := filepath.Join(configDir, "spinifex.toml") require.NoError(t, os.WriteFile(configPath, []byte("placeholder"), 0600)) require.NoError(t, os.WriteFile(filepath.Join(configDir, "ca.pem"), []byte("x"), 0600)) - err := EnableOVNIPSec(configPath, multiNodeClusterConfig()) + err := EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), allReady()) require.Error(t, err) assert.Contains(t, err.Error(), "missing IPsec credential") + + assert.Empty(t, recorder.ovsSets(), "OVS must not be touched when the credentials are absent") + assert.Empty(t, recorder.nbctlWrites()) } func TestEnableOVNIPSec_NoConfigPath(t *testing.T) { - err := EnableOVNIPSec("", nil) + err := EnableOVNIPSec(t.Context(), "", nil, nil) require.Error(t, err) assert.Contains(t, err.Error(), "config path unset") } @@ -205,7 +355,7 @@ func TestReconcileOVNIPSec_DisabledStopsCharon(t *testing.T) { recorder := &recordingSudo{} t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) - require.NoError(t, ReconcileOVNIPSec("/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false))) + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false), allReady())) assert.Equal(t, [][]string{{ipsecStateHelper, "off"}}, recorder.helperRuns()) @@ -220,7 +370,7 @@ func TestReconcileOVNIPSec_NeverCallsSystemctlDirectly(t *testing.T) { recorder := &recordingSudo{} t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) - require.NoError(t, ReconcileOVNIPSec("/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false))) + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false), allReady())) for _, run := range recorder.runs { if run[0] != "systemctl" { @@ -240,7 +390,7 @@ func TestReconcileOVNIPSec_SingleNodeStopsCharon(t *testing.T) { cfg := &config.ClusterConfig{Node: "node1", Nodes: map[string]config.Config{"node1": {}}} cfg.Network.IPSecEnabled = true - require.NoError(t, ReconcileOVNIPSec("/etc/spinifex/spinifex.toml", cfg)) + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", cfg, allReady())) assert.Equal(t, [][]string{{ipsecStateHelper, "off"}}, recorder.helperRuns()) } @@ -258,20 +408,10 @@ func TestReconcileOVNIPSec_EnabledTurnsTheServicesOn(t *testing.T) { } t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) - configDir := t.TempDir() - configPath := filepath.Join(configDir, "spinifex.toml") - require.NoError(t, os.WriteFile(configPath, []byte("placeholder"), 0600)) - for _, rel := range []string{"ca.pem", "ipsec/peer.pem", "ipsec/peer.key"} { - full := filepath.Join(configDir, rel) - require.NoError(t, os.MkdirAll(filepath.Dir(full), 0750)) - require.NoError(t, os.WriteFile(full, []byte("x"), 0600)) - } + recorder.nbUnreachable = true + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") - origNBSock := ovnNBSocketPath - ovnNBSocketPath = filepath.Join(configDir, "no-such-socket") - t.Cleanup(func() { ovnNBSocketPath = origNBSock }) - - require.NoError(t, ReconcileOVNIPSec(configPath, multiNodeIPSecConfig(true))) + require.NoError(t, ReconcileOVNIPSec(t.Context(), configPath, multiNodeIPSecConfig(true), allReady())) assert.Equal(t, [][]string{{ipsecStateHelper, "on"}}, recorder.helperRuns()) @@ -292,7 +432,7 @@ func TestReconcileOVNIPSec_AlreadyInStateIsNoOp(t *testing.T) { } t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) - require.NoError(t, ReconcileOVNIPSec("/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false))) + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false), allReady())) assert.Empty(t, recorder.helperRuns()) } @@ -309,7 +449,7 @@ func TestReconcileOVNIPSec_UnknownUnitIsNotAnError(t *testing.T) { } t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) - require.NoError(t, ReconcileOVNIPSec("/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false))) + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", multiNodeIPSecConfig(false), allReady())) assert.Empty(t, recorder.helperRuns()) } @@ -321,5 +461,191 @@ func TestReconcileOVNIPSec_NilConfigLeavesUnitsAlone(t *testing.T) { return exec.Command("true") })) - require.NoError(t, ReconcileOVNIPSec("/etc/spinifex/spinifex.toml", nil)) + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", nil, allReady())) +} + +// A single attempt is what left two of three chassis unconfigured for minutes +// while a peer already required encryption; the pass has to be retried. +func TestMaintainIPSec_RetriesAFailedPass(t *testing.T) { + recorder := &recordingSudo{} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + origRetry, origInterval := ipsecRetryDelay, ipsecReconcileInterval + ipsecRetryDelay, ipsecReconcileInterval = time.Millisecond, time.Millisecond + t.Cleanup(func() { ipsecRetryDelay, ipsecReconcileInterval = origRetry, origInterval }) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + barrier := &flakyBarrier{failFor: 2} + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := make(chan struct{}) + go func() { + MaintainIPSec(ctx, configPath, multiNodeIPSecConfig(true), barrier) + close(done) + }() + + require.Eventually(t, func() bool { return barrier.succeeded() }, 5*time.Second, 5*time.Millisecond, + "MaintainIPSec gave up after the first failure") + + cancel() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("MaintainIPSec did not stop on context cancellation") + } +} + +// flakyBarrier fails its first failFor reads, then reports the cluster ready. +type flakyBarrier struct { + mu sync.Mutex + failFor int + calls int +} + +func (f *flakyBarrier) Publish(context.Context, string, IPSecNodeStatus) error { return nil } + +func (f *flakyBarrier) Cluster(context.Context, []string) (map[string]IPSecNodeStatus, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.calls++ + if f.calls <= f.failFor { + return nil, errors.New("kv unavailable") + } + return allReady().cluster, nil +} + +func (f *flakyBarrier) succeeded() bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.calls > f.failFor +} + +// Every management node can reach the NB DB, so without a single elected writer +// two of them disagreeing for one pass flip the flag and rebuild every +// strongSwan connection in the cluster. +func TestEnableOVNIPSec_OnlyTheElectedWriterTouchesNBGlobal(t *testing.T) { + recorder := &recordingSudo{} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + + // node1 is this node, but node0 sorts first and is also NB-reachable. + barrier := &fakeBarrier{cluster: map[string]IPSecNodeStatus{ + "node0": {Ready: true, NBReachable: true}, + "node1": {Ready: true, NBReachable: true}, + }} + + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), barrier)) + + assert.Empty(t, recorder.nbctlWrites(), + "a node that is not the elected writer must not race the flag") +} + +// The elected writer is the first live management node, not merely the first +// node: a hypervisor cannot write the flag and must not veto the one that can. +func TestNBGlobalWriter_SkipsNodesWithoutANBDB(t *testing.T) { + writer := nbGlobalWriter(map[string]IPSecNodeStatus{ + "node1": {Ready: true}, + "node2": {Ready: true, NBReachable: true}, + "node3": {Ready: true, NBReachable: true}, + }) + assert.Equal(t, "node2", writer) +} + +// A node that fails its local half must say so, not leave its last "ready" +// record to expire: silence keeps it counted as configured for a whole +// freshness window, which is the black hole again with a slower fuse. +func TestEnableOVNIPSec_PublishesUnreadyWhenTheLocalHalfFails(t *testing.T) { + recorder := &recordingSudo{activeOutput: "inactive\n"} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + origTimeout := systemctlActiveTimeout + systemctlActiveTimeout = 100 * time.Millisecond + t.Cleanup(func() { systemctlActiveTimeout = origTimeout }) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + barrier := allReady() + + err := EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), barrier) + require.Error(t, err) + assert.Contains(t, err.Error(), "ovs-monitor-ipsec") + + assert.Equal(t, "node1", barrier.publishedNode) + require.Len(t, barrier.published, 1) + assert.False(t, barrier.published[0].Ready, + "a node that cannot encrypt must publish that, not go quiet") + assert.True(t, barrier.published[0].NBReachable) +} + +// Turning IPsec off stops charon on every node. A flag left asserted then +// demands encryption nothing can perform — the same black hole, reached from +// the other direction. +func TestReconcileOVNIPSec_DisabledRetractsNBGlobal(t *testing.T) { + recorder := &recordingSudo{nbIPSec: "true"} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + barrier := allReady() + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", + multiNodeIPSecConfig(false), barrier)) + + assert.Equal(t, [][]string{{"ovn-nbctl", "--timeout=5", "set", "NB_Global", ".", "ipsec=false"}}, + recorder.nbctlWrites()) + require.Len(t, barrier.published, 1) + assert.False(t, barrier.published[0].Ready) +} + +// Already released: the off path must not rewrite a flag that is already false. +func TestReconcileOVNIPSec_DisabledIsANoOpWhenAlreadyReleased(t *testing.T) { + recorder := &recordingSudo{} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + require.NoError(t, ReconcileOVNIPSec(t.Context(), "/etc/spinifex/spinifex.toml", + multiNodeIPSecConfig(false), allReady())) + + assert.Empty(t, recorder.nbctlWrites()) +} + +// A management node whose NB read fails for any reason other than "there is no +// local DB here" has a real fault. Swallowing it was how the node that holds +// the flag could go blind and never retract. +func TestEnableOVNIPSec_NBReadFailureFailsThePass(t *testing.T) { + recorder := &recordingSudo{nbError: "ovn-nbctl: unix:/var/run/ovn/ovnnb_db.sock: permission denied"} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + + err := EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), allReady()) + require.Error(t, err) + assert.Contains(t, err.Error(), "permission denied") + assert.Empty(t, recorder.nbctlWrites()) +} + +// A node with no local NB DB is the ordinary hypervisor case, not a fault. +func TestEnableOVNIPSec_NoLocalNBDBIsNotAnError(t *testing.T) { + recorder := &recordingSudo{nbUnreachable: true} + t.Cleanup(utils.SetSudoCommandForTest(recorder.stub)) + + configPath := filepath.Join(ipsecTestConfigDir(t), "spinifex.toml") + barrier := allReady() + + require.NoError(t, EnableOVNIPSec(t.Context(), configPath, multiNodeClusterConfig(), barrier)) + + require.Len(t, barrier.published, 1) + assert.True(t, barrier.published[0].Ready) + assert.False(t, barrier.published[0].NBReachable, + "a node that cannot read NB_Global must not offer itself as the writer") +} + +// ovn-nbctl writes vlog lines to stderr on a successful run. Folding those into +// the value parses a live "true" as false, which sends the reconcile into the +// "holding encryption off" branch while the flag is in fact still asserted. +func TestGetNBGlobalIPSec_IgnoresStderrChatter(t *testing.T) { + t.Cleanup(utils.SetSudoCommandForTest(func(string, ...string) *exec.Cmd { + return exec.Command("sh", "-c", `echo "ovsdb-idl|WARN|reconnecting" >&2; echo true`) + })) + + value, err := GetNBGlobalIPSec() + require.NoError(t, err) + assert.True(t, value) } diff --git a/tests/e2e/multinode/ipsec_test.go b/tests/e2e/multinode/ipsec_test.go index d44518d45..5035aa5ad 100644 --- a/tests/e2e/multinode/ipsec_test.go +++ b/tests/e2e/multinode/ipsec_test.go @@ -4,6 +4,7 @@ package multinode import ( "context" + "errors" "fmt" "strings" "testing" @@ -43,6 +44,7 @@ func runIPSec(t *testing.T, fix *Fixture) { harness.Step(t, "OVS DB carries cert pointers + ipsec_encapsulation=true on every node") required := []string{"certificate=", "private_key=", "ca_cert=", "ipsec_encapsulation=\"true\""} + incomplete := map[string]string{} for _, n := range fix.Cluster.Nodes { c, cancel := context.WithTimeout(context.Background(), 10*time.Second) raw, err := ssh.Run(c, n.Addr, "sudo ovs-vsctl get Open_vSwitch . other_config") @@ -53,12 +55,15 @@ func runIPSec(t *testing.T, fix *Fixture) { s := strings.TrimSpace(string(raw)) for _, key := range required { if !strings.Contains(s, key) { - t.Fatalf("%s OVS other_config missing %q: %s", n.Name, key, s) + incomplete[n.Name] = s + t.Errorf("%s OVS other_config missing %q: %s", n.Name, key, s) } } harness.Detail(t, "node", n.Name, "other_config", s) } + assertNBGlobalMatchesChassis(t, fix, ssh, incomplete) + harness.Step(t, "xfrm SAs with AES-GCM established on every node") harness.EventuallyErr(t, func() error { for _, n := range fix.Cluster.Nodes { @@ -132,3 +137,56 @@ func ipsecRequested(t *testing.T, ssh *harness.PeerSSH, n harness.Node) bool { harness.Detail(t, "node", n.Name, "ipsec_enabled", value) return value != "false" } + +// assertNBGlobalMatchesChassis checks the invariant a partial IPsec enable +// breaks: NB_Global.ipsec is cluster-wide, so asserting it while any chassis is +// still unconfigured black-holes every guest that crosses chassis, with no +// control-plane signal that anything is wrong. +func assertNBGlobalMatchesChassis(t *testing.T, fix *Fixture, ssh *harness.PeerSSH, incomplete map[string]string) { + harness.Step(t, "NB_Global ipsec is not asserted over an unconfigured chassis") + + // The flag is asserted only once every node has published its own readiness, + // so it lags the per-node config by up to a reconcile interval. Polling for it + // is the difference between testing the invariant and testing the clock. + var asserted string + harness.EventuallyErr(t, func() error { + var unreadable []string + for _, n := range fix.Cluster.Nodes { + value, err := readNBGlobalIPSec(t, ssh, n) + if err != nil { + unreadable = append(unreadable, fmt.Sprintf("%s: %v", n.Name, err)) + continue + } + harness.Detail(t, "node", n.Name, "nb_global_ipsec", value) + if value == "true" { + asserted = n.Name + return nil + } + } + if len(unreadable) == len(fix.Cluster.Nodes) { + return fmt.Errorf("no node could read NB_Global: %s", strings.Join(unreadable, "; ")) + } + return errors.New("no node reports NB_Global ipsec=true — ovn-controller adds no options:remote_name, so intra-AZ Geneve is plaintext") + }, 120*time.Second, 5*time.Second) + + if len(incomplete) > 0 { + t.Fatalf("NB_Global ipsec=true (read on %s) while %d chassis are unconfigured: %v — cross-chassis guest traffic is black-holing", + asserted, len(incomplete), incomplete) + } +} + +// readNBGlobalIPSec returns the flag as the node reports it. A node with no +// local NB DB returns an error rather than "false": conflating the two is the +// defect the production change exists to remove, and the test must not reinstate +// it by discarding the exit status. +func readNBGlobalIPSec(t *testing.T, ssh *harness.PeerSSH, n harness.Node) (string, error) { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + raw, err := ssh.Run(ctx, n.Addr, "sudo ovn-nbctl --timeout=5 get NB_Global . ipsec") + if err != nil { + return "", fmt.Errorf("%w: %s", err, strings.TrimSpace(string(raw))) + } + return strings.Trim(strings.TrimSpace(string(raw)), `"`), nil +}