diff --git a/cmd/hhfab/main.go b/cmd/hhfab/main.go index 812ac1d9c..aeca1e29a 100644 --- a/cmd/hhfab/main.go +++ b/cmd/hhfab/main.go @@ -255,7 +255,7 @@ func Run(ctx context.Context) error { }, &cli.UintFlag{ Name: "multihomed-servers", - Usage: "number of multihomed servers (2 connections to 2 different orphan leaves)", + Usage: "number of multihomed servers (2 connections to 2 different leaves, preferably orphans)", Destination: &wgMultiHomedServers, }, &cli.BoolFlag{ diff --git a/pkg/hhfab/endpoints.go b/pkg/hhfab/endpoints.go index 911c5aa4a..28fe6a30d 100644 --- a/pkg/hhfab/endpoints.go +++ b/pkg/hhfab/endpoints.go @@ -91,9 +91,24 @@ type serverAttachment struct { subnetName string subnetCIDR netip.Prefix hostBGP bool + eslag bool + l3vni bool attachName string // for diagnostics } +// eslagL3VNIOnly reports whether every one of the server's attachments is an +// ESLAG connection into a non-L2VNI VPC. Those servers never run hhnet, so +// discovering no addresses on them is expected rather than a failure. +func eslagL3VNIOnly(atts []serverAttachment) bool { + if len(atts) == 0 { + return false + } + + return !slices.ContainsFunc(atts, func(a serverAttachment) bool { + return !a.eslag || !a.l3vni + }) +} + // CollectServerEndpoints observes the live cluster and produces one // *Endpoint per (server, vpc, subnet) attachment for each server in the // `servers` filter (nil → all servers attached to at least one VPC). @@ -183,6 +198,8 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes subnetName: subnetName, subnetCIDR: cidr, hostBGP: subnet.HostBGP, + eslag: conn.Spec.ESLAG != nil, + l3vni: vpc.Spec.Mode != vpcapi.VPCModeL2VNI, attachName: attach.Name, }) } @@ -230,7 +247,7 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes for _, p := range probed { atts := serverAttachments[p.serverName] used := make([]bool, len(atts)) - if len(p.ips) == 0 { + if len(p.ips) == 0 && eslagL3VNIOnly(atts) { // Expected for ESLAG servers in L3VNI mode, which never run hhnet; not recorded as a drop slog.Warn("Server has no configured IPs, skipping endpoints", "server", p.serverName, "attachments", len(atts)) diff --git a/pkg/hhfab/endpoints_test.go b/pkg/hhfab/endpoints_test.go index 0089a7ba5..99f8282b8 100644 --- a/pkg/hhfab/endpoints_test.go +++ b/pkg/hhfab/endpoints_test.go @@ -189,127 +189,6 @@ func TestReplaceServerDrops(t *testing.T) { require.Equal(t, "vpc-new", m.dropped[1].VPC) } -func TestValidate(t *testing.T) { - a := serverEP("server-1", "vpc-1", "default", "10.0.1.1") - b := serverEP("server-2", "vpc-2", "default", "10.0.2.1") - ext := &Endpoint{External: &ExternalEndpoint{ExternalName: "ext-1"}} - ext2 := &Endpoint{External: &ExternalEndpoint{ExternalName: "ext-2"}} - - newMatrix := func() *ConnectivityMatrix { - m := NewConnectivityMatrix() - m.AllEndpoints = []*Endpoint{a, b, ext, ext2} - - return m - } - - t.Run("all-deny topology is valid", func(t *testing.T) { - // Isolated VPCs with no peerings: no Allow entry anywhere is a - // legitimate thing to assert, not a degenerate matrix. - require.NoError(t, newMatrix().Validate()) - }) - - t.Run("no endpoints", func(t *testing.T) { - require.ErrorContains(t, NewConnectivityMatrix().Validate(), "no endpoints") - }) - - t.Run("nil matrix", func(t *testing.T) { - var m *ConnectivityMatrix - require.Error(t, m.Validate()) - }) - - t.Run("discovery drop", func(t *testing.T) { - m := newMatrix() - m.dropped = []DroppedEndpoint{{ - Server: "server-3", VPC: "vpc-3", Subnet: "default", Reason: "attachment has no matching address", - }} - require.ErrorContains(t, m.Validate(), "server-3 (vpc-3/default): attachment has no matching address") - }) - - t.Run("unevaluated server pair", func(t *testing.T) { - m := newMatrix() - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: b}, - Verdict: VerdictUnknown, - Detail: "gw peering with non-empty expose 'As'", - }) - err := m.Validate() - require.ErrorContains(t, err, "server-1(vpc-1/default) → server-2(vpc-2/default)") - require.ErrorContains(t, err, "non-empty expose 'As'") - - // A test that overlays the real expectation clears it. - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictAllow, - }) - require.NoError(t, m.Validate()) - }) - - t.Run("unevaluated default entry shadowed by proto-port entries", func(t *testing.T) { - // Proto-scoped pairs are probed only by runMatrixProtoPortPhase, which - // never reads the default entry, so an ACL test overlaying just the - // proto verdicts leaves nothing unasserted. - m := newMatrix() - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictUnknown, Detail: "unsupported", - }) - require.ErrorContains(t, m.Validate(), "server-1(vpc-1/default) → server-2(vpc-2/default)") - - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictAllow, - ProtoPort: ProtoPort{Protocol: "tcp", Port: 5301}, - }) - require.NoError(t, m.Validate()) - - // ...unless the default entry is a port-forward, which the - // port-forward phase does read. - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictUnknown, Detail: "unsupported", - NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.0.2.1"), DestinationPort: 15201}, - }) - require.ErrorContains(t, m.Validate(), "server-1(vpc-1/default) → server-2(vpc-2/default)") - }) - - t.Run("unevaluated external is settled by another external's allow", func(t *testing.T) { - // The external oracle ORs over every External in the cluster, so - // one Allow decides the source's expectation for all of them. - m := newMatrix() - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: ext}, Verdict: VerdictUnknown, Detail: "unsupported", - }) - require.ErrorContains(t, m.Validate(), "external:ext-1") - - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: a, Destination: ext2}, Verdict: VerdictAllow, - }) - require.NoError(t, m.Validate()) - - // ...but only for that source. - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: b, Destination: ext}, Verdict: VerdictUnknown, Detail: "unsupported", - }) - require.ErrorContains(t, m.Validate(), "server-2(vpc-2/default) → external:ext-1") - - // A DNAT-only Allow grants no egress, and an Allow scoped to one - // proto/port isn't seen by the untargeted curl probe. Neither settles - // the Unknown, or Validate would wave through an entry the curl phase - // goes on to assert as denied. - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: b, Destination: ext2}, Verdict: VerdictAllow, - NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.0.2.1"), DestinationPort: 8080}, - }) - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: b, Destination: ext2}, Verdict: VerdictAllow, - ProtoPort: ProtoPort{Protocol: "tcp", Port: 8080}, - }) - require.ErrorContains(t, m.Validate(), "server-2(vpc-2/default) → external:ext-1") - - // An unscoped, non-DNAT Allow does. - m.Add(ConnectivityExpectation{ - Pair: EndpointPair{Source: b, Destination: ext2}, Verdict: VerdictAllow, - }) - require.NoError(t, m.Validate()) - }) -} - func TestReplaceServerEndpoints_PreservesHostBGP(t *testing.T) { m := NewConnectivityMatrix() ep := &Endpoint{Server: &ServerEndpoint{ @@ -327,3 +206,27 @@ func TestReplaceServerEndpoints_PreservesHostBGP(t *testing.T) { require.Same(t, ep, m.AllEndpoints[0]) require.True(t, ep.Server.HostBGP, "HostBGP should be copied across on in-place update") } + +func TestESLAGL3VNIOnly(t *testing.T) { + att := func(eslag, l3vni bool) serverAttachment { + return serverAttachment{vpcName: "vpc-1", subnetName: "default", eslag: eslag, l3vni: l3vni} + } + + for _, tc := range []struct { + name string + atts []serverAttachment + want bool + }{ + {name: "no attachments", atts: nil, want: false}, + {name: "eslag l3vni", atts: []serverAttachment{att(true, true)}, want: true}, + {name: "eslag l2vni", atts: []serverAttachment{att(true, false)}, want: false}, + {name: "unbundled l3vni", atts: []serverAttachment{att(false, true)}, want: false}, + {name: "unbundled l2vni", atts: []serverAttachment{att(false, false)}, want: false}, + {name: "all eslag l3vni", atts: []serverAttachment{att(true, true), att(true, true)}, want: true}, + {name: "mixed", atts: []serverAttachment{att(true, true), att(false, true)}, want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, eslagL3VNIOnly(tc.atts)) + }) + } +} diff --git a/pkg/hhfab/matrix.go b/pkg/hhfab/matrix.go index 5afca7322..1e9c0932e 100644 --- a/pkg/hhfab/matrix.go +++ b/pkg/hhfab/matrix.go @@ -83,7 +83,8 @@ type ConnectivityExpectation struct { Peering string - // free-form context for Reason, currently only set on VerdictUnknown entries + // free-form context for Reason: why a VerdictUnknown entry could not be + // evaluated, or what withholds a VerdictDeny (e.g. an ACL on the peering) Detail string ProtoPort ProtoPort @@ -296,26 +297,19 @@ func (m *ConnectivityMatrix) Validate() error { len(m.dropped), strings.Join(reasons, "; "))) } - extAllowBySource := map[*Endpoint]bool{} - for _, src := range m.AllEndpoints { - if _, ok := m.externalCurlAllowed(src); ok { - extAllowBySource[src] = true - } - } - unknowns := []string{} - for pair, byPP := range m.entries { - for pp, e := range byPP { - if e.Verdict != VerdictUnknown { - continue - } - if pair.Destination != nil && pair.Destination.External != nil && extAllowBySource[pair.Source] { + unprobed := []string{} + for _, byPP := range m.entries { + for _, e := range byPP { + owner := m.entryOwner(e) + if owner == matrixPhaseUnprobed { + unprobed = append(unprobed, describeMatrixEntry(e)) + continue } - if m.defaultEntryShadowed(pair, pp, e) { - continue + if owner != matrixPhaseSkipped && e.Verdict == VerdictUnknown { + unknowns = append(unknowns, describeMatrixEntry(e)) } - unknowns = append(unknowns, describeUnknownEntry(pair, pp, e)) } } if len(unknowns) > 0 { @@ -323,26 +317,150 @@ func (m *ConnectivityMatrix) Validate() error { errs = append(errs, fmt.Errorf("%d matrix entries could not be evaluated and were not overlaid by the test: %s", //nolint:goerr113 len(unknowns), strings.Join(unknowns, "; "))) } + if len(unprobed) > 0 { + slices.Sort(unprobed) + errs = append(errs, fmt.Errorf("%d matrix entries no probe phase can read, so they would pass unasserted: %s", //nolint:goerr113 + len(unprobed), strings.Join(unprobed, "; "))) + } return errors.Join(errs...) } -// defaultEntryShadowed reports whether a default-ProtoPort entry is read by no -// phase at all: runMatrixServerServerPhase hands a pair carrying proto-port -// entries to runMatrixProtoPortPhase, which reads only those. Port-forward -// entries are exempt, runMatrixPortForwardPhase still reads them. -func (m *ConnectivityMatrix) defaultEntryShadowed(pair EndpointPair, pp ProtoPort, e ConnectivityExpectation) bool { - if pp != (ProtoPort{}) { - return false +// matrixPhase is the probe phase that owns an entry. Single source of truth for +// the phases' skip gates and for Validate. Ownership is not the same as being +// probed: the --source/--destination filters gate pairs on top of it, and +// Validate does not model them, so an owned entry whose pair those filters +// exclude is never probed. +type matrixPhase int + +const ( + // nothing reads the entry, and nothing makes that intentional + matrixPhaseUnprobed matrixPhase = iota + // nothing reads the entry by design: self-pair, or default entry superseded + // by the pair's proto-port entries + matrixPhaseSkipped + matrixPhaseServerServer + matrixPhasePortForward + matrixPhaseProtoPort + matrixPhaseCurl +) + +// entryOwner assumes Validate has already rejected VerdictUnknown: the phases +// carry no verdict guard of their own, and reachabilityFromExpectation maps +// Unknown to Reachable:false, so a phase reached without Validate would assert +// deny on an entry explicitly marked unevaluable. +func (m *ConnectivityMatrix) entryOwner(e ConnectivityExpectation) matrixPhase { + src, dst := e.Pair.Source, e.Pair.Destination + if src == nil || dst == nil || src.Server == nil { + return matrixPhaseUnprobed } - if pair.Source == nil || pair.Destination == nil || pair.Source.Server == nil || pair.Destination.Server == nil { - return false + if src == dst || IsSameEndpointNode(src, dst) { + return matrixPhaseSkipped } - if e.NAT != nil && e.NAT.DestinationPort != 0 { - return false + // a translated port makes the path L4-only + portForward := e.NAT != nil && e.NAT.DestinationPort != 0 + if portForward && !e.NAT.DestinationIP.IsValid() { + return matrixPhaseUnprobed + } + scoped := e.ProtoPort != (ProtoPort{}) + // the proto-port probes dial ProtoPort.Port, so they would aim past the + // translation, and the port-forward probe carries no protocol dimension + if portForward && scoped { + return matrixPhaseUnprobed + } + + switch { + case dst.Server != nil: + switch { + case scoped: + return matrixPhaseProtoPort + case portForward: + return matrixPhasePortForward + case m.HasProtoPortEntries(src, dst): + return matrixPhaseSkipped + default: + return matrixPhaseServerServer + } + case dst.External != nil: + switch { + case scoped: + // the curl probe carries no protocol/port dimension and + // runMatrixProtoPortPhase only probes server destinations + return matrixPhaseUnprobed + case portForward: + return matrixPhasePortForward + case e.Verdict == VerdictAllow && e.NAT != nil && !e.NAT.SourcePool.IsValid(): + // the curl oracle discards a NAT with no source pool, so it would + // assert the opposite of what this entry claims + return matrixPhaseUnprobed + case e.Verdict == VerdictUnknown && m.hasExternalCurlAllow(src): + // one untargeted curl per source, so a sibling external's Allow + // already pins its outcome + return matrixPhaseSkipped + default: + return matrixPhaseCurl + } + default: + return matrixPhaseUnprobed + } +} + +// checkProbesEnabled rejects a run whose enabled probes cannot assert every +// entry a phase will read: checkPing/checkIPerf/checkCurl no-op when their count +// is off, so those entries would pass unasserted. +// +// It walks endpoint pairs rather than m.entries because that is what the phases +// do, and populate only stores reachable or unevaluable pairs — so a topology +// whose pairs are all denies has no stored entries at all. The source and +// destination gates mirror deps.inSources/inDestinations, which the phases apply +// on top of ownership. +func (m *ConnectivityMatrix) checkProbesEnabled(opts TestConnectivityOpts) error { + var serverServer, external, icmp int + for _, src := range m.AllEndpoints { + if src.Server == nil || (len(opts.Sources) > 0 && !slices.Contains(opts.Sources, src.Server.Name)) { + continue + } + for _, dst := range m.AllEndpoints { + // an external destination has no server name to filter on + if dst.Server != nil && len(opts.Destinations) > 0 && !slices.Contains(opts.Destinations, dst.Server.Name) { + continue + } + entries := m.ProtoPortEntries(src, dst) + // plus the pair's default entry, which Lookup synthesizes as a Deny + // when populate stored nothing for it + entries = append(entries, m.Lookup(src, dst, ProtoPort{})) + for _, e := range entries { + owner := m.entryOwner(e) + if owner == matrixPhaseServerServer { + serverServer++ + } + if owner == matrixPhaseCurl { + external++ + } + if owner == matrixPhaseProtoPort && e.ProtoPort.Protocol == "icmp" { + icmp++ + } + } + } + } + + if opts.PingsCount <= 0 && opts.IPerfsSeconds <= 0 && serverServer > 0 { + return fmt.Errorf("matrix has %d server-to-server entries but both pings and iperfs are disabled", serverServer) //nolint:goerr113 + } + if opts.CurlsCount <= 0 && external > 0 { + return fmt.Errorf("matrix has %d external entries but curls are disabled", external) //nolint:goerr113 + } + if opts.PingsCount <= 0 && icmp > 0 { + return fmt.Errorf("matrix has %d icmp proto-port entries but pings are disabled", icmp) //nolint:goerr113 } - return m.HasProtoPortEntries(pair.Source, pair.Destination) + return nil +} + +func (m *ConnectivityMatrix) hasExternalCurlAllow(src *Endpoint) bool { + _, ok := m.externalCurlAllowed(src) + + return ok } func (m *ConnectivityMatrix) externalCurlAllowed(src *Endpoint) (ConnectivityExpectation, bool) { @@ -367,10 +485,10 @@ func (m *ConnectivityMatrix) externalCurlAllowed(src *Endpoint) (ConnectivityExp return ConnectivityExpectation{}, false } -func describeUnknownEntry(pair EndpointPair, pp ProtoPort, e ConnectivityExpectation) string { - out := fmt.Sprintf("%s → %s", endpointLabel(pair.Source), endpointLabel(pair.Destination)) - if pp != (ProtoPort{}) { - out += fmt.Sprintf(" [%s/%d]", pp.Protocol, pp.Port) +func describeMatrixEntry(e ConnectivityExpectation) string { + out := fmt.Sprintf("%s → %s", endpointLabel(e.Pair.Source), endpointLabel(e.Pair.Destination)) + if e.ProtoPort != (ProtoPort{}) { + out += fmt.Sprintf(" [%s/%d]", e.ProtoPort.Protocol, e.ProtoPort.Port) } if e.Detail != "" { out += ": " + e.Detail @@ -436,6 +554,7 @@ func reachabilityFromExpectation(e ConnectivityExpectation) Reachability { Reachable: e.Verdict == VerdictAllow, Reason: e.Reason, Peering: e.Peering, + Detail: e.Detail, } } @@ -459,6 +578,13 @@ type matrixTestDeps struct { errChan chan<- error } +// probesServerPair reports whether the server-to-server phases will probe this +// pair, so listener setup covers exactly what gets probed. +func (d *matrixTestDeps) probesServerPair(src, dst *Endpoint) bool { + return src.Server != nil && dst.Server != nil && + d.inSources(src.Server.Name) && d.inDestinations(dst.Server.Name) +} + func runMatrixServerServerPhase(ctx context.Context, opts TestConnectivityOpts, matrix *ConnectivityMatrix, deps *matrixTestDeps) error { for _, src := range matrix.AllEndpoints { if src.Server == nil { @@ -468,25 +594,15 @@ func runMatrixServerServerPhase(ctx context.Context, opts TestConnectivityOpts, continue } for _, dst := range matrix.AllEndpoints { - if dst.Server == nil || src == dst { - continue - } - if IsSameEndpointNode(src, dst) { + if dst.Server == nil { continue } if !deps.inDestinations(dst.Server.Name) { continue } - // Protocol/port-scoped pairs are owned entirely by runMatrixProtoPortPhase - if matrix.HasProtoPortEntries(src, dst) { - continue - } - entry := matrix.Lookup(src, dst, ProtoPort{}) - // Port-forward destinations (DestinationPort set) are L4-only - // and handled by runMatrixPortForwardPhase below. - if entry.NAT != nil && entry.NAT.DestinationPort != 0 { + if matrix.entryOwner(entry) != matrixPhaseServerServer { continue } @@ -596,14 +712,8 @@ func runMatrixPortForwardPhase(ctx context.Context, opts TestConnectivityOpts, m continue } for _, dst := range matrix.AllEndpoints { - if src == dst { - continue - } e := matrix.Lookup(src, dst, ProtoPort{}) - if e.NAT == nil || (e.Verdict != VerdictAllow && e.Verdict != VerdictDeny) { - continue - } - if !e.NAT.DestinationIP.IsValid() || e.NAT.DestinationPort == 0 { + if matrix.entryOwner(e) != matrixPhasePortForward { continue } switch { @@ -617,9 +727,6 @@ func runMatrixPortForwardPhase(ctx context.Context, opts TestConnectivityOpts, m } extTargets[key] = reachabilityFromExpectation(e) case dst.Server != nil: - if IsSameEndpointNode(src, dst) { - continue - } if !deps.inDestinations(dst.Server.Name) { continue } @@ -654,14 +761,14 @@ func startMatrixProtoPortListeners(ctx context.Context, matrix *ConnectivityMatr } wanted := map[hostPort]struct{}{} for _, src := range matrix.AllEndpoints { - if src.Server == nil { - continue - } for _, dst := range matrix.AllEndpoints { - if dst.Server == nil || src == dst { + if !deps.probesServerPair(src, dst) { continue } for _, e := range matrix.ProtoPortEntries(src, dst) { + if matrix.entryOwner(e) != matrixPhaseProtoPort { + continue + } pp := e.ProtoPort if pp.Protocol != "tcp" && pp.Protocol != "udp" { continue @@ -684,7 +791,8 @@ func startMatrixProtoPortListeners(ctx context.Context, matrix *ConnectivityMatr if ssh == nil { continue } - cmd := fmt.Sprintf("sudo docker exec iperf3 pkill -f 'iperf3 -s -p %d'", hp.port) + // anchored so port 5301 does not match a listener on 53010 + cmd := fmt.Sprintf("sudo docker exec iperf3 pkill -f 'iperf3 -s -p %d$'", hp.port) if _, stderr, err := retrySSHCmd(tctx, ssh, cmd, hp.host); err != nil { slog.Warn("Failed to stop proto-port iperf3 listener", "host", hp.host, "port", hp.port, "err", err, "stderr", stderr) } @@ -719,20 +827,14 @@ func startMatrixProtoPortListeners(ctx context.Context, matrix *ConnectivityMatr // loss check. func runMatrixProtoPortPhase(ctx context.Context, opts TestConnectivityOpts, matrix *ConnectivityMatrix, deps *matrixTestDeps) { for _, src := range matrix.AllEndpoints { - if src.Server == nil || !deps.inSources(src.Server.Name) { - continue - } for _, dst := range matrix.AllEndpoints { - if dst.Server == nil || src == dst { - continue - } - if IsSameEndpointNode(src, dst) { - continue - } - if !deps.inDestinations(dst.Server.Name) { + if !deps.probesServerPair(src, dst) { continue } for _, entry := range matrix.ProtoPortEntries(src, dst) { + if matrix.entryOwner(entry) != matrixPhaseProtoPort { + continue + } expected := reachabilityFromExpectation(entry) pp := entry.ProtoPort fromName := src.Server.Name @@ -758,14 +860,14 @@ func runMatrixProtoPortPhase(ctx context.Context, opts TestConnectivityOpts, mat case "tcp": port := pp.Port deps.wg.Go(func() { - if ie := checkTCPPort(ctx, deps.iperfs, fromName, fromSSH, toIP, port, expected.Reachable); ie != nil { + if ie := checkTCPPort(ctx, deps.iperfs, fromName, fromSSH, toIP, port, expected); ie != nil { deps.errChan <- ie } }) case "udp": port := pp.Port deps.wg.Go(func() { - if ie := checkUDPPort(ctx, opts, deps.iperfs, fromName, fromSSH, toIP, port, expected.Reachable); ie != nil { + if ie := checkUDPPort(ctx, opts, deps.iperfs, fromName, fromSSH, toIP, port, expected); ie != nil { deps.errChan <- ie } }) @@ -787,6 +889,9 @@ func (c *Config) TestConnectivityWithMatrix(ctx context.Context, vlab *VLAB, opt if err := matrix.Validate(); err != nil { return fmt.Errorf("connectivity matrix is not a sound oracle: %w", err) } + if err := matrix.checkProbesEnabled(opts); err != nil { + return err + } start := time.Now() if opts.PingsParallel <= 0 { @@ -918,12 +1023,7 @@ func runMatrixIperfPortForward(ctx context.Context, opts TestConnectivityOpts, i slog.Debug("Checking iperf3 through port-forward NAT (matrix)", logArgs...) if !expected.Reachable { - ie := checkTCPPort(ctx, nil, from, ssh, toIP, toPort, false) - if ie != nil { - ie.Why = why - } - - return ie + return checkTCPPort(ctx, nil, from, ssh, toIP, toPort, expected) } // Gate on TCP reachability: a successful TCP connect is the precise signal diff --git a/pkg/hhfab/matrix_test.go b/pkg/hhfab/matrix_test.go index 5852ff01b..3e50e0351 100644 --- a/pkg/hhfab/matrix_test.go +++ b/pkg/hhfab/matrix_test.go @@ -4,6 +4,7 @@ package hhfab import ( + "net/netip" "testing" "github.com/stretchr/testify/require" @@ -79,6 +80,52 @@ func TestHasProtoPortEntries_IgnoresDefaultOnly(t *testing.T) { require.True(t, m.HasProtoPortEntries(src, dst)) } +func TestCheckProbesEnabled(t *testing.T) { + m := NewConnectivityMatrix() + a := serverEP("server-1", "vpc-1", "default", "10.0.1.1") + b := serverEP("server-2", "vpc-2", "default", "10.0.2.2") + m.AllEndpoints = []*Endpoint{a, b} + + // populate stores nothing for a pair it denies, so an all-deny topology has + // no entries at all — but the phase still probes every pair off the + // synthesized Deny, and with both probes off every one would pass unasserted. + require.Empty(t, m.entries) + require.ErrorContains(t, m.checkProbesEnabled(TestConnectivityOpts{CurlsCount: 1}), "2 server-to-server entries") + require.NoError(t, m.checkProbesEnabled(TestConnectivityOpts{PingsCount: 1})) + require.NoError(t, m.checkProbesEnabled(TestConnectivityOpts{IPerfsSeconds: 3})) + + // The --source/--destination filters gate pairs on top of ownership: the + // first leaves only server-1 → server-2, the second only the self-pair, which + // no phase probes. + require.ErrorContains(t, m.checkProbesEnabled(TestConnectivityOpts{ + CurlsCount: 1, Sources: []string{"server-1"}, + }), "1 server-to-server entries") + require.NoError(t, m.checkProbesEnabled(TestConnectivityOpts{ + CurlsCount: 1, Sources: []string{"server-1"}, Destinations: []string{"server-1"}, + })) + + // An icmp proto-port entry has no probe but ping, so iperfs alone cannot + // assert it. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, + Verdict: VerdictAllow, + ProtoPort: ProtoPort{Protocol: "icmp"}, + }) + require.ErrorContains(t, m.checkProbesEnabled(TestConnectivityOpts{IPerfsSeconds: 3}), "1 icmp proto-port entries") + require.NoError(t, m.checkProbesEnabled(TestConnectivityOpts{PingsCount: 1})) + + // External destinations are only ever probed by curl. + m.AllEndpoints = append(m.AllEndpoints, &Endpoint{External: &ExternalEndpoint{ExternalName: "ext-1"}}) + require.ErrorContains(t, m.checkProbesEnabled(TestConnectivityOpts{PingsCount: 1}), "2 external entries") + require.NoError(t, m.checkProbesEnabled(TestConnectivityOpts{PingsCount: 1, CurlsCount: 1})) + + // an external destination carries no server name, so --destination cannot + // exclude it — both sources still curl it + require.ErrorContains(t, m.checkProbesEnabled(TestConnectivityOpts{ + PingsCount: 1, Destinations: []string{"server-2"}, + }), "2 external entries") +} + func TestParseNCReturnCode(t *testing.T) { cases := []struct { name string @@ -138,3 +185,180 @@ func TestSetVPCToVPCProtoVerdict_AccumulatesScopesAndPreservesPeering(t *testing // Not applied to a server outside the destination VPC. require.False(t, m.HasProtoPortEntries(a1, c1), "vpc-3 destination untouched") } + +func TestValidate(t *testing.T) { + a := serverEP("server-1", "vpc-1", "default", "10.0.1.1") + b := serverEP("server-2", "vpc-2", "default", "10.0.2.1") + ext := &Endpoint{External: &ExternalEndpoint{ExternalName: "ext-1"}} + ext2 := &Endpoint{External: &ExternalEndpoint{ExternalName: "ext-2"}} + + newMatrix := func() *ConnectivityMatrix { + m := NewConnectivityMatrix() + m.AllEndpoints = []*Endpoint{a, b, ext, ext2} + + return m + } + + t.Run("all-deny topology is valid", func(t *testing.T) { + // Isolated VPCs with no peerings: no Allow entry anywhere is a + // legitimate thing to assert, not a degenerate matrix. + require.NoError(t, newMatrix().Validate()) + }) + + t.Run("no endpoints", func(t *testing.T) { + require.ErrorContains(t, NewConnectivityMatrix().Validate(), "no endpoints") + }) + + t.Run("nil matrix", func(t *testing.T) { + var m *ConnectivityMatrix + require.Error(t, m.Validate()) + }) + + t.Run("discovery drop", func(t *testing.T) { + m := newMatrix() + m.dropped = []DroppedEndpoint{{ + Server: "server-3", VPC: "vpc-3", Subnet: "default", Reason: "attachment has no matching address", + }} + require.ErrorContains(t, m.Validate(), "server-3 (vpc-3/default): attachment has no matching address") + }) + + t.Run("unevaluated server pair", func(t *testing.T) { + m := newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, + Verdict: VerdictUnknown, + Detail: "gw peering with non-empty expose 'As'", + }) + err := m.Validate() + require.ErrorContains(t, err, "server-1(vpc-1/default) → server-2(vpc-2/default)") + require.ErrorContains(t, err, "non-empty expose 'As'") + + // A test that overlays the real expectation clears it. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictAllow, + }) + require.NoError(t, m.Validate()) + }) + + t.Run("unevaluated default entry shadowed by proto-port entries", func(t *testing.T) { + // Proto-scoped pairs are probed only by runMatrixProtoPortPhase, which + // never reads the default entry, so an ACL test overlaying just the + // proto verdicts leaves nothing unasserted. + m := newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictUnknown, Detail: "unsupported", + }) + require.ErrorContains(t, m.Validate(), "server-1(vpc-1/default) → server-2(vpc-2/default)") + + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictAllow, + ProtoPort: ProtoPort{Protocol: "tcp", Port: 5301}, + }) + require.NoError(t, m.Validate()) + + // ...unless the default entry is a port-forward, which the + // port-forward phase does read. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictUnknown, Detail: "unsupported", + NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.0.2.1"), DestinationPort: 15201}, + }) + require.ErrorContains(t, m.Validate(), "server-1(vpc-1/default) → server-2(vpc-2/default)") + }) + + t.Run("unevaluated external is settled by another external's allow", func(t *testing.T) { + // The external oracle ORs over every External in the cluster, so + // one Allow decides the source's expectation for all of them. + m := newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: ext}, Verdict: VerdictUnknown, Detail: "unsupported", + }) + require.ErrorContains(t, m.Validate(), "external:ext-1") + + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: ext2}, Verdict: VerdictAllow, + }) + require.NoError(t, m.Validate()) + + // ...but only for that source. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: b, Destination: ext}, Verdict: VerdictUnknown, Detail: "unsupported", + }) + require.ErrorContains(t, m.Validate(), "server-2(vpc-2/default) → external:ext-1") + + // A DNAT-only Allow grants no egress, so it doesn't settle the Unknown, + // or Validate would wave through an entry the curl phase goes on to + // assert as denied. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: b, Destination: ext2}, Verdict: VerdictAllow, + NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.0.2.1"), DestinationPort: 8080}, + }) + require.ErrorContains(t, m.Validate(), "server-2(vpc-2/default) → external:ext-1") + + // An unscoped, non-DNAT Allow does. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: b, Destination: ext2}, Verdict: VerdictAllow, + }) + require.NoError(t, m.Validate()) + + // ...but it settles only the curl expectation. A port-forward entry is + // read by its own phase, which has its own verdict to assert. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: b, Destination: ext}, Verdict: VerdictUnknown, Detail: "unsupported", + NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.99.0.1"), DestinationPort: 8080}, + }) + err := m.Validate() + require.ErrorContains(t, err, "could not be evaluated") + require.ErrorContains(t, err, "server-2(vpc-2/default) → external:ext-1") + }) + + t.Run("entries no phase can read", func(t *testing.T) { + // runMatrixProtoPortPhase probes server destinations only and the curl + // probe is untargeted, so this expectation asserts nothing. + m := newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: ext}, Verdict: VerdictAllow, + ProtoPort: ProtoPort{Protocol: "tcp", Port: 8080}, + }) + err := m.Validate() + require.ErrorContains(t, err, "no probe phase can read") + require.ErrorContains(t, err, "server-1(vpc-1/default) → external:ext-1 [tcp/8080]") + + // A translated port with no address to aim it at is unprobeable too. + m = newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictAllow, + NAT: &TranslatedAddress{DestinationPort: 15201}, + }) + require.ErrorContains(t, m.Validate(), "server-1(vpc-1/default) → server-2(vpc-2/default)") + + // So is a proto-scoped entry that inherited a port-forward: the proto + // probe would dial 5301 on the NAT address instead of the mapped port. + m = newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: b}, Verdict: VerdictAllow, + ProtoPort: ProtoPort{Protocol: "tcp", Port: 5301}, + NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.99.0.1"), DestinationPort: 15201}, + }) + err = m.Validate() + require.ErrorContains(t, err, "no probe phase can read") + require.ErrorContains(t, err, "server-1(vpc-1/default) → server-2(vpc-2/default) [tcp/5301]") + + // An external Allow behind a NAT with no source pool is a contradiction: + // the curl oracle discards it and asserts the source cannot get out. + m = newMatrix() + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: ext}, Verdict: VerdictAllow, + NAT: &TranslatedAddress{DestinationIP: netip.MustParseAddr("10.99.0.1")}, + }) + err = m.Validate() + require.ErrorContains(t, err, "no probe phase can read") + require.ErrorContains(t, err, "server-1(vpc-1/default) → external:ext-1") + + // A masquerade pool makes it assertable again. + m.Add(ConnectivityExpectation{ + Pair: EndpointPair{Source: a, Destination: ext}, Verdict: VerdictAllow, + NAT: &TranslatedAddress{SourcePool: netip.MustParsePrefix("10.99.0.0/24")}, + }) + require.NoError(t, m.Validate()) + }) +} diff --git a/pkg/hhfab/rt_acl_tests.go b/pkg/hhfab/rt_acl_tests.go index b0a9dbb95..f2433a73d 100644 --- a/pkg/hhfab/rt_acl_tests.go +++ b/pkg/hhfab/rt_acl_tests.go @@ -14,10 +14,14 @@ import ( const ( // aclProbePort is served by the always-on iperf3 daemon (TCP+UDP), so // tests probing it need no on-demand listener. - aclProbePort uint16 = 5201 + aclProbePort uint16 = persistentIperf3Port aclAltPort uint16 = 6201 aclAltPortRange = "6000-6500" aclUnprobedPort uint16 = 9999 + + // the dataplane has no icmp keyword yet, a numeric protocol is the only + // way to match ICMP explicitly + aclProtoICMP gwapi.ACLMatchProtocol = "1" ) func setACLDirVerdicts(m *ConnectivityMatrix, srcVPC, dstVPC string, icmp, tcp, udp ConnectivityVerdict) { @@ -415,6 +419,102 @@ func gatewayACLPrecedenceDenyThenAllowTest(ctx context.Context, testCtx *VPCPeer }) } +// gatewayACLNumericProtocolICMPTest: match ICMP explicitly via the numeric +// protocol 1 (there is no icmp keyword yet). Every other ACL case only ever +// sees ICMP fall through to the default action, so this is the one that asserts +// a rule can match it. TCP/UDP hit the default deny. +func gatewayACLNumericProtocolICMPTest(ctx context.Context, testCtx *VPCPeeringTestCtx, matrix *ConnectivityMatrix) (bool, []RevertFunc, error) { + return testCtx.runNATTest(ctx, matrix, natTestSpec{ + Name: "gateway ACL numeric protocol ICMP", + BuildSpec: func(vpc1, vpc2 *vpcapi.VPC) (peeringSpecs, error) { + specs := emptyPeeringSpecs() + acl := &gwapi.PeeringACL{ + Default: gwapi.ACLDefaultDeny, + Rules: []gwapi.PeeringACLRule{ + {Name: "allow-icmp-fwd", From: vpc1.Name, To: vpc2.Name, Action: gwapi.ACLActionAllow, Scope: gwapi.ACLScopePacket, Match: gwapi.PeeringACLMatch{Protocol: aclProtoICMP}}, + {Name: "allow-icmp-rev", From: vpc2.Name, To: vpc1.Name, Action: gwapi.ACLActionAllow, Scope: gwapi.ACLScopePacket, Match: gwapi.PeeringACLMatch{Protocol: aclProtoICMP}}, + }, + } + err := appendGwPeeringSpec(specs.Gateway, vpc1, vpc2, &GwPeeringOptions{ACL: acl}) + + return specs, err + }, + Overlay: func(vpc1, vpc2 *vpcapi.VPC, matrix *ConnectivityMatrix) error { + setACLDirVerdicts(matrix, vpc1.Name, vpc2.Name, VerdictAllow, VerdictDeny, VerdictDeny) + setACLDirVerdicts(matrix, vpc2.Name, vpc1.Name, VerdictAllow, VerdictDeny, VerdictDeny) + + return nil + }, + }) +} + +// gatewayACLPreNATDestinationTest: ACL rules are evaluated before NAT, so +// match.dst is compared against the address the initiator dialed — the peer's +// advertised (expose "as") pool — not the destination's native IP. Both +// directions get bidirectional static NAT and a rule matching the peer's NAT +// pool; an implementation that matched post-NAT would deny everything. +func gatewayACLPreNATDestinationTest(ctx context.Context, testCtx *VPCPeeringTestCtx, matrix *ConnectivityMatrix) (bool, []RevertFunc, error) { + const ( + vpc1NATCIDR = "192.168.91.0/24" + vpc2NATCIDR = "192.168.92.0/24" + ) + + return testCtx.runNATTest(ctx, matrix, natTestSpec{ + Name: "gateway ACL pre-NAT destination match", + BuildSpec: func(vpc1, vpc2 *vpcapi.VPC) (peeringSpecs, error) { + specs := emptyPeeringSpecs() + acl := &gwapi.PeeringACL{ + Default: gwapi.ACLDefaultDeny, + Rules: []gwapi.PeeringACLRule{ + { + Name: "allow-as-fwd", From: vpc1.Name, To: vpc2.Name, + Action: gwapi.ACLActionAllow, Scope: gwapi.ACLScopePacket, + Match: gwapi.PeeringACLMatch{ + Destination: []gwapi.PeeringACLMatchEndpoint{{CIDR: vpc2NATCIDR}}, + }, + }, + { + Name: "allow-as-rev", From: vpc2.Name, To: vpc1.Name, + Action: gwapi.ACLActionAllow, Scope: gwapi.ACLScopePacket, + Match: gwapi.PeeringACLMatch{ + Destination: []gwapi.PeeringACLMatchEndpoint{{CIDR: vpc1NATCIDR}}, + }, + }, + }, + } + err := appendGwPeeringSpec(specs.Gateway, vpc1, vpc2, &GwPeeringOptions{ + VPC1NATCIDR: []string{vpc1NATCIDR}, + VPC2NATCIDR: []string{vpc2NATCIDR}, + ACL: acl, + }) + + return specs, err + }, + Overlay: func(vpc1, vpc2 *vpcapi.VPC, matrix *ConnectivityMatrix) error { + vpc1CIDR, err := vpcFirstSubnetCIDR(vpc1) + if err != nil { + return err + } + vpc2CIDR, err := vpcFirstSubnetCIDR(vpc2) + if err != nil { + return err + } + // static NAT is bidirectional: each side only knows the other by + // its advertised pool, so every probe targets the DNAT address + if err := overlayVPCToVPCStaticDNAT(matrix, vpc1.Name, vpc2.Name, vpc2CIDR, vpc2NATCIDR); err != nil { + return fmt.Errorf("overlaying vpc2 static DNAT: %w", err) + } + if err := overlayVPCToVPCStaticDNAT(matrix, vpc2.Name, vpc1.Name, vpc1CIDR, vpc1NATCIDR); err != nil { + return fmt.Errorf("overlaying vpc1 static DNAT: %w", err) + } + setACLDirVerdicts(matrix, vpc1.Name, vpc2.Name, VerdictAllow, VerdictAllow, VerdictAllow) + setACLDirVerdicts(matrix, vpc2.Name, vpc1.Name, VerdictAllow, VerdictAllow, VerdictAllow) + + return nil + }, + }) +} + func getACLTestCases() []JUnitTestCase { return []JUnitTestCase{ {Name: "Gateway Peering ACL Default Deny", F: gatewayACLDefaultDenyTest, SkipFlags: SkipFlags{NoGateway: true, NoServers: true}}, @@ -427,5 +527,7 @@ func getACLTestCases() []JUnitTestCase { {Name: "Gateway Peering ACL Port Range Scoping", F: gatewayACLPortScopingTest, SkipFlags: SkipFlags{NoGateway: true, NoServers: true}}, {Name: "Gateway Peering ACL Precedence Allow-Then-Deny", F: gatewayACLPrecedenceAllowThenDenyTest, SkipFlags: SkipFlags{NoGateway: true, NoServers: true}}, {Name: "Gateway Peering ACL Precedence Deny-Then-Allow", F: gatewayACLPrecedenceDenyThenAllowTest, SkipFlags: SkipFlags{NoGateway: true, NoServers: true}}, + {Name: "Gateway Peering ACL Numeric Protocol ICMP", F: gatewayACLNumericProtocolICMPTest, SkipFlags: SkipFlags{NoGateway: true, NoServers: true}}, + {Name: "Gateway Peering ACL Pre-NAT Destination Match", F: gatewayACLPreNATDestinationTest, SkipFlags: SkipFlags{NoGateway: true, NoServers: true}}, } } diff --git a/pkg/hhfab/rt_acl_tests.md b/pkg/hhfab/rt_acl_tests.md index 7ecce90a9..a5f565ed0 100644 --- a/pkg/hhfab/rt_acl_tests.md +++ b/pkg/hhfab/rt_acl_tests.md @@ -45,6 +45,10 @@ Port 5201 is served by the always-on `iperf3 -s` daemon (TCP+UDP). Any other por flow/conntrack state where masquerade (or port-forward) NAT is present, so a `flow`-scoped rule is only valid on such a peering; there conntrack permits the return automatically. NAT-free cases therefore use explicit `packet` scope. + This is a temporary restriction per [docs#338](https://github.com/githedgehog/docs/pull/338): + once `flow` works without NAT it becomes the CRD default, and the + packet-everywhere choice below needs revisiting since the default path would + then have no coverage. ## Test cases @@ -63,6 +67,8 @@ noted, and run with `SkipFlags{NoGateway, NoServers}`. | **Port Range Scoping** | allow tcp; fwd dst-port range `6000-6500`, rev src-port range | fwd tcp/6201 allow; tcp/5201 + udp + icmp + all rev = deny | | **Precedence Allow-Then-Deny** | `[allow tcp/5201, deny-all]` fwd + src-port return rule | fwd tcp/5201 allow; everything else deny | | **Precedence Deny-Then-Allow** | same rules, `deny-all` first | all deny (first match wins) | +| **Numeric Protocol ICMP** | `allow proto "1"` both dirs | icmp allow both ways; tcp+udp deny (fall to default) | +| **Pre-NAT Destination Match** | bidirectional static NAT + allow both dirs matched by `dst` = peer's `as` pool | all allow both ways — matching post-NAT would deny everything | ## Coverage @@ -70,17 +76,22 @@ noted, and run with `SkipFlags{NoGateway, NoServers}`. |---|---| | Default action `deny` / `deny-unless-exposed` | Default Deny / Deny-Unless-Exposed | | Rule action `allow` / `deny` | all allow cases / carve-out + precedence | -| Protocol `tcp` / `udp` / any | Protocol Scoping, Port Range / UDP Carve-Out / Explicit Allow, Subnet | +| Protocol `tcp` / `udp` / numeric / any | Protocol Scoping, Port Range / UDP Carve-Out / Numeric Protocol ICMP / Explicit Allow, Subnet | | Selector `VPCSubnet` / `CIDR` | Subnet/CIDR Scoping (both, one per direction) | | Ports single / range, dst / src side | Precedence (single dst+src) / Port Range (range dst+src) | | Scope `packet` / `flow` | all packet cases / Flow Scope Masquerade | | Rule precedence (first-match) | Precedence Allow-Then-Deny + Deny-Then-Allow | | Stateless return-path requirement | Packet One-Way (negative) | +| ACL evaluated before NAT (`dst` = advertised address) | Pre-NAT Destination Match | ### Known limitations / assumptions - **UDP is verifiable only as "denied while TCP allowed."** The `iperf3 -u` probe opens a TCP control channel on the same port, so "deny TCP + allow UDP on one port" cannot be distinguished from a UDP block — no case relies on it. -- **ICMP falls to the default action** (it matches neither `tcp` nor `udp` - rules). Numeric-protocol matching (e.g. proto `1` for ICMP) is **not** covered. +- **ACLs on external peerings cannot be asserted.** `runMatrixProtoPortPhase` + probes server destinations only, and the external oracle is a single untargeted + curl per source server with no protocol/port dimension. `Validate` now rejects a + proto-scoped entry towards an external rather than letting it report green while + asserting nothing, so covering the docs' egress-allow-list example needs a + targeted external probe first. diff --git a/pkg/hhfab/rt_eslag_fallback.go b/pkg/hhfab/rt_eslag_fallback.go index 69271b733..60175d3c7 100644 --- a/pkg/hhfab/rt_eslag_fallback.go +++ b/pkg/hhfab/rt_eslag_fallback.go @@ -22,7 +22,7 @@ const ( eslagPxeAttemptsPerLeg = 3 ) -func eslagFallbackTest(ctx context.Context, testCtx *VPCPeeringTestCtx, _ *ConnectivityMatrix) (bool, []RevertFunc, error) { +func eslagFallbackTest(ctx context.Context, testCtx *VPCPeeringTestCtx, matrix *ConnectivityMatrix) (bool, []RevertFunc, error) { if testCtx.setupOpts.VPCMode == vpcapi.VPCModeL3VNI { return true, nil, fmt.Errorf("L3VNI mode is not compatible with ESLAG") //nolint:goerr113 } @@ -87,7 +87,14 @@ func eslagFallbackTest(ctx context.Context, testCtx *VPCPeeringTestCtx, _ *Conne return false, reverts, fmt.Errorf("building netconf command for %s: %w", server, err) } reverts = append(reverts, func(ctx context.Context) error { - return restoreServerBond(ctx, ssh, netconfCmd) + if err := restoreServerBond(ctx, ssh, netconfCmd); err != nil { + return fmt.Errorf("restoring server bond for %s: %w", server, err) + } + if err := testCtx.rebindMatrixServerEndpoint(ctx, matrix, server); err != nil { + return fmt.Errorf("refreshing matrix endpoint for %s after restoring the bond: %w", server, err) + } + + return nil }) failures := make([]string, 0) diff --git a/pkg/hhfab/rt_nat_tests.go b/pkg/hhfab/rt_nat_tests.go index 3de3c0cfc..949c14d8e 100644 --- a/pkg/hhfab/rt_nat_tests.go +++ b/pkg/hhfab/rt_nat_tests.go @@ -217,6 +217,12 @@ func overrideVPCToVPCVerdict(matrix *ConnectivityMatrix, srcVPCName, dstVPCName func setVPCToVPCProtoVerdict(matrix *ConnectivityMatrix, srcVPCName, dstVPCName string, pp ProtoPort, verdict ConnectivityVerdict) { srcPred := ServerInVPC(srcVPCName) dstPred := ServerInVPC(dstVPCName) + // the peering allows the pair, so only an ACL can deny one protocol/port of + // it — which is what makes a verdict proto-scoped in the first place + detail := "" + if verdict == VerdictDeny { + detail = "ACL" + } for _, src := range matrix.AllEndpoints { if !srcPred(src) { continue @@ -231,6 +237,7 @@ func setVPCToVPCProtoVerdict(matrix *ConnectivityMatrix, srcVPCName, dstVPCName Verdict: verdict, Reason: ReachabilityReasonGatewayPeering, Peering: existing.Peering, + Detail: detail, NAT: existing.NAT, ProtoPort: pp, }) diff --git a/pkg/hhfab/testing.go b/pkg/hhfab/testing.go index 0a4cd3e01..2e4636525 100644 --- a/pkg/hhfab/testing.go +++ b/pkg/hhfab/testing.go @@ -2250,14 +2250,33 @@ func (ce *CurlError) Error() string { ce.Source, ce.Msg, ce.Expected, whySuffix(ce.Why)) } +// expectationWhy renders the because-clause of a probe error. ReachabilityReason +// only ever names the peering that would allow a pair, so a deny has to be +// phrased against it: either something on that peering withholds the pair +// (Detail, e.g. an ACL), or no peering covers the pair at all. func expectationWhy(r Reachability) string { + via := string(r.Reason) + if r.Reason != "" && r.Peering != "" { + via = fmt.Sprintf("%s %q", r.Reason, r.Peering) + } + + if r.Reachable { + if via == "" { + return "no reason recorded" + } + + return via + } + switch { - case r.Reason != "" && r.Peering != "": - return fmt.Sprintf("%s %q", r.Reason, r.Peering) - case r.Reason != "": - return string(r.Reason) + case r.Detail != "" && via != "": + return fmt.Sprintf("%s on %s", r.Detail, via) + case r.Detail != "": + return r.Detail + case via != "": + return via + " does not allow it" default: - return "no reason recorded" + return "no peering allows it" } } @@ -2678,6 +2697,7 @@ type Reachability struct { Reachable bool Reason ReachabilityReason Peering string + Detail string } type ReachabilityReason string @@ -3405,9 +3425,9 @@ func parseNCReturnCode(stdout string) (int, bool) { // expected. It runs `nc -zw2 ` on the source: a completed handshake // means the path is open (allow), a refused/timed-out connect (nc exit 1) means // it is blocked (deny). -func checkTCPPort(ctx context.Context, sem *semaphore.Weighted, from string, fromSSH *sshutil.Config, toIP netip.Addr, port uint16, expected bool) *IperfError { +func checkTCPPort(ctx context.Context, sem *semaphore.Weighted, from string, fromSSH *sshutil.Config, toIP netip.Addr, port uint16, expected Reachability) *IperfError { target := fmt.Sprintf("%s:%d", toIP.String(), port) - ie := &IperfError{Source: from, Destination: target} + ie := &IperfError{Source: from, Destination: target, Why: expectationWhy(expected)} if sem != nil { if err := sem.Acquire(ctx, 1); err != nil { @@ -3452,14 +3472,14 @@ func checkTCPPort(ctx context.Context, sem *semaphore.Weighted, from string, fro return ie } - slog.Debug("TCP port probe result", "from", from, "to", target, "expected", expected, "ok", connectOk, "rc", rc, "stderr", stderr) + slog.Debug("TCP port probe result", "from", from, "to", target, "expected", expected.Reachable, "ok", connectOk, "rc", rc, "stderr", stderr) - if expected && !connectOk { + if expected.Reachable && !connectOk { ie.ClientMsg = "should be reachable but TCP connect was refused/timed out" return ie } - if !expected && connectOk { + if !expected.Reachable && connectOk { ie.ClientMsg = "should not be reachable but TCP connect succeeded" return ie @@ -3477,12 +3497,52 @@ const ( udpAllowLossThreshold = 90.0 ) +const ( + // A denied path neither completes nor refuses the iperf3 -u control connect, + // so iperf3 blocks in it until something bounds it. Deny probes get the short + // budget: on a path that is in fact open the handshake completes in + // milliseconds. Allow probes get a longer one so a momentarily slow but + // working path is not called down. + udpProbeDenyConnect = 5 * time.Second + udpProbeAllowConnect = 15 * time.Second + // Backstop slack around the transfer itself, for iperf3 startup and teardown. + udpProbeInnerSlack = 10 * time.Second + // Room for the SSH round trip once the backstop has fired: without it, output + // from a probe that did its job is cut off and read as no result at all. + udpProbeSSHHeadroom = 30 * time.Second + udpProbeAttempts = 2 +) + +// udpProbeTiming holds the nested deadlines of one UDP probe: iperf3's own +// control-connect budget, the `timeout` backstop around it, and the SSH +// deadline around that. +type udpProbeTiming struct { + connect time.Duration + inner time.Duration + outer time.Duration +} + +func udpProbeTimingFor(secs int, expectReachable bool) udpProbeTiming { + connect := udpProbeDenyConnect + if expectReachable { + connect = udpProbeAllowConnect + } + inner := connect + time.Duration(secs)*time.Second + udpProbeInnerSlack + + return udpProbeTiming{connect: connect, inner: inner, outer: inner + udpProbeSSHHeadroom} +} + +func udpProbeCmd(toIP netip.Addr, port uint16, secs int, timing udpProbeTiming) string { + return fmt.Sprintf("sudo docker exec iperf3 timeout -k 5 %d iperf3 -u -J --connect-timeout %d -c %s -p %d -t %d -b 10M -l 1000", + int(timing.inner.Seconds()), timing.connect.Milliseconds(), toIP.String(), port, secs) +} + // NOTE: iperf3 -u first opens a TCP control channel on the target port, so this // cannot distinguish "TCP denied + UDP allowed" on the same port (the control // channel would be blocked). Callers must avoid that combination. -func checkUDPPort(ctx context.Context, opts TestConnectivityOpts, sem *semaphore.Weighted, from string, fromSSH *sshutil.Config, toIP netip.Addr, port uint16, expected bool) *IperfError { +func checkUDPPort(ctx context.Context, opts TestConnectivityOpts, sem *semaphore.Weighted, from string, fromSSH *sshutil.Config, toIP netip.Addr, port uint16, expected Reachability) *IperfError { target := fmt.Sprintf("%s:%d", toIP.String(), port) - ie := &IperfError{Source: from, Destination: target} + ie := &IperfError{Source: from, Destination: target, Why: expectationWhy(expected)} if sem != nil { if err := sem.Acquire(ctx, 1); err != nil { @@ -3497,15 +3557,41 @@ func checkUDPPort(ctx context.Context, opts TestConnectivityOpts, sem *semaphore if secs <= 0 { secs = 3 } - ctx, cancel := context.WithTimeout(ctx, time.Duration(secs+30)*time.Second) - defer cancel() + timing := udpProbeTimingFor(secs, expected.Reachable) + cmd := udpProbeCmd(toIP, port, secs, timing) - cmd := fmt.Sprintf("sudo docker exec iperf3 timeout %d iperf3 -u -J -c %s -p %d -t %d -b 10M -l 1000", secs+25, toIP.String(), port, secs) - stdout, stderr, err := retrySSHCmd(ctx, fromSSH, cmd, from) - report, parseErr := parseIPerf3Report([]byte(stdout)) + var ( + report *iperf3Report + stderr string + err error + failMsg string + ) + // No report is no verdict, so retry once: the usual cause is a stall on the + // SSH/docker path rather than anything about the path under test. + for attempt := range udpProbeAttempts { + if attempt > 0 { + if ctx.Err() != nil { + break + } + slog.Debug("Retrying UDP port probe", "from", from, "to", target, "reason", failMsg) + } - if parseErr != nil { - ie.ClientMsg = fmt.Sprintf("iperf3 UDP probe produced no parseable report (cmd err: %v, stderr: %q): %s", err, strings.TrimSpace(stderr), parseErr) + probeCtx, cancel := context.WithTimeout(ctx, timing.outer) + var stdout string + stdout, stderr, err = retrySSHCmd(probeCtx, fromSSH, cmd, from) + cancel() + + var parseErr error + report, parseErr = parseIPerf3Report([]byte(stdout)) + if parseErr == nil { + break + } + report = nil + failMsg = fmt.Sprintf("iperf3 UDP probe produced no parseable report (cmd err: %v, stderr: %q): %s", err, strings.TrimSpace(stderr), parseErr) + } + + if report == nil { + ie.ClientMsg = failMsg return ie } @@ -3517,11 +3603,11 @@ func checkUDPPort(ctx context.Context, opts TestConnectivityOpts, sem *semaphore delivered := reportErr == "" && packets > 0 && lostPercent < udpAllowLossThreshold blocked := reportErr != "" || packets == 0 || lostPercent >= udpDenyLossThreshold - slog.Debug("UDP port probe result", "from", from, "to", target, "expected", expected, + slog.Debug("UDP port probe result", "from", from, "to", target, "expected", expected.Reachable, "delivered", delivered, "blocked", blocked, "packets", packets, "lost", lost, "lostPercent", lostPercent, "err", err, "reportErr", reportErr, "stderr", stderr) - if expected { + if expected.Reachable { if !delivered { if reportErr != "" { ie.ClientMsg = fmt.Sprintf("should be reachable but UDP probe reported error: %s", reportErr) @@ -3535,9 +3621,16 @@ func checkUDPPort(ctx context.Context, opts TestConnectivityOpts, sem *semaphore return nil } - // expected == deny. + // expected == deny. Loss between the two thresholds is neither delivered nor + // blocked, so don't claim delivery on what may just be a lossy path. if !blocked { - ie.ClientMsg = fmt.Sprintf("should not be reachable but UDP datagrams delivered (packets %d, loss %.1f%%)", packets, lostPercent) + if delivered { + ie.ClientMsg = fmt.Sprintf("should not be reachable but UDP datagrams were delivered (packets %d, loss %.1f%% < %.0f%%)", + packets, lostPercent, udpAllowLossThreshold) + } else { + ie.ClientMsg = fmt.Sprintf("should not be reachable but UDP traffic was not blocked; inconclusive: loss %.1f%% is between the %.0f%%/%.0f%% delivered/blocked thresholds (packets %d)", + lostPercent, udpAllowLossThreshold, udpDenyLossThreshold, packets) + } return ie } diff --git a/pkg/hhfab/testing_test.go b/pkg/hhfab/testing_test.go index 25952e37a..9b606182c 100644 --- a/pkg/hhfab/testing_test.go +++ b/pkg/hhfab/testing_test.go @@ -7,9 +7,11 @@ import ( "net/netip" "slices" "testing" + "time" "github.com/stretchr/testify/require" "go.githedgehog.com/fabric/api/meta" + wiringapi "go.githedgehog.com/fabric/api/wiring/v1beta1" ) func TestVLANsFrom(t *testing.T) { @@ -375,6 +377,137 @@ rtt min/avg/max/mdev = 0.611/0.912/1.308/0.251 ms } } +func TestGetServerHostBGPCmd(t *testing.T) { + unbundled := func(port string) *wiringapi.Connection { + return &wiringapi.Connection{Spec: wiringapi.ConnectionSpec{ + Unbundled: &wiringapi.ConnUnbundled{ + Link: wiringapi.ServerToSwitchLink{Server: wiringapi.BasePortName{Port: port}}, + }, + }} + } + bundled := func(ports ...string) *wiringapi.Connection { + links := []wiringapi.ServerToSwitchLink{} + for _, port := range ports { + links = append(links, wiringapi.ServerToSwitchLink{Server: wiringapi.BasePortName{Port: port}}) + } + + return &wiringapi.Connection{Spec: wiringapi.ConnectionSpec{Bundled: &wiringapi.ConnBundled{Links: links}}} + } + + for _, test := range []struct { + name string + params []HostBGPParams + expected string + wantErr bool + }{ + {name: "no params", wantErr: true}, + { + name: "single vpc single connection", + params: []HostBGPParams{{ + VPCLabel: "vpc-01", + Connections: []*wiringapi.Connection{unbundled("server-01/enp2s1")}, + VLAN: 1001, + Subnet: netip.MustParsePrefix("10.0.1.0/24"), + }}, + expected: "vpc-01:v=1001:i=enp2s1:a=10.0.1.0/32", + }, + { + name: "server offset walks the subnet", + params: []HostBGPParams{{ + VPCLabel: "vpc-01", + Connections: []*wiringapi.Connection{bundled("server-01/enp2s1", "server-01/enp2s2")}, + VLAN: 1001, + Subnet: netip.MustParsePrefix("10.0.1.0/24"), + ServerOffset: 3, + }}, + expected: "vpc-01:v=1001:i=enp2s1:i=enp2s2:a=10.0.1.3/32", + }, + { + name: "two vpcs are space separated", + params: []HostBGPParams{ + { + VPCLabel: "vpc-01", + Connections: []*wiringapi.Connection{unbundled("server-01/enp2s1")}, + VLAN: 1001, + Subnet: netip.MustParsePrefix("10.0.1.0/24"), + }, + { + VPCLabel: "vpc-02", + Connections: []*wiringapi.Connection{unbundled("server-01/enp2s2"), unbundled("server-01/enp2s3")}, + VLAN: 1002, + Subnet: netip.MustParsePrefix("10.0.2.0/24"), + ServerOffset: 1, + }, + }, + expected: "vpc-01:v=1001:i=enp2s1:a=10.0.1.0/32 vpc-02:v=1002:i=enp2s2:i=enp2s3:a=10.0.2.1/32", + }, + { + name: "no connections", + params: []HostBGPParams{{VPCLabel: "vpc-01", VLAN: 1001, Subnet: netip.MustParsePrefix("10.0.1.0/24")}}, + wantErr: true, + }, + { + name: "nil connection", + params: []HostBGPParams{{VPCLabel: "vpc-01", Connections: []*wiringapi.Connection{nil}, VLAN: 1001}}, + wantErr: true, + }, + { + name: "unsupported connection type", + params: []HostBGPParams{{ + VPCLabel: "vpc-01", + Connections: []*wiringapi.Connection{{Spec: wiringapi.ConnectionSpec{}}}, + VLAN: 1001, + }}, + wantErr: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + cmd, err := getServerHostBGPCmd(test.params) + if test.wantErr { + require.Error(t, err) + + return + } + require.NoError(t, err) + require.Equal(t, test.expected, cmd) + }) + } +} + +func TestUDPProbeCmd(t *testing.T) { + for _, test := range []struct { + name string + secs int + reachable bool + expected string + }{ + { + name: "deny probe bounds the control connect", + secs: 3, + expected: "sudo docker exec iperf3 timeout -k 5 18 iperf3 -u -J --connect-timeout 5000 -c 10.0.1.2 -p 5201 -t 3 -b 10M -l 1000", + }, + { + name: "allow probe gets the longer connect budget", + secs: 3, + reachable: true, + expected: "sudo docker exec iperf3 timeout -k 5 28 iperf3 -u -J --connect-timeout 15000 -c 10.0.1.2 -p 5201 -t 3 -b 10M -l 1000", + }, + { + name: "extended run stretches the backstop", + secs: 10, + expected: "sudo docker exec iperf3 timeout -k 5 25 iperf3 -u -J --connect-timeout 5000 -c 10.0.1.2 -p 5201 -t 10 -b 10M -l 1000", + }, + } { + t.Run(test.name, func(t *testing.T) { + timing := udpProbeTimingFor(test.secs, test.reachable) + require.Equal(t, test.expected, udpProbeCmd(netip.MustParseAddr("10.0.1.2"), 5201, test.secs, timing)) + // The SSH deadline must outlive the backstop, or a probe that ran to + // completion is reported as having produced no result. + require.Greater(t, timing.outer, timing.inner+20*time.Second) + }) + } +} + func mapSlice[IN, OUT any](f func(IN) OUT, in []IN) []OUT { out := make([]OUT, len(in)) for i, v := range in { @@ -387,3 +520,51 @@ func mapSlice[IN, OUT any](f func(IN) OUT, in []IN) []OUT { func prefixToString(prefix netip.Prefix) string { return prefix.String() } + +func TestExpectationWhy(t *testing.T) { + for _, test := range []struct { + name string + r Reachability + expected string + }{ + { + name: "allow names the peering that grants it", + r: Reachability{Reachable: true, Reason: ReachabilityReasonGatewayPeering, Peering: "vpc-01--vpc-02"}, + expected: `gateway-peering "vpc-01--vpc-02"`, + }, + { + name: "allow with no peering falls back to the reason alone", + r: Reachability{Reachable: true, Reason: ReachabilityReasonIntraVPC}, + expected: "intra-vpc", + }, + { + name: "an ACL deny reads as withholding a peered pair", + r: Reachability{Reason: ReachabilityReasonGatewayPeering, Peering: "vpc-01--vpc-02", Detail: "ACL"}, + expected: `ACL on gateway-peering "vpc-01--vpc-02"`, + }, + { + name: "a deny on a peered pair with no detail still reads as a deny", + r: Reachability{Reason: ReachabilityReasonGatewayPeering, Peering: "vpc-01--vpc-02"}, + expected: `gateway-peering "vpc-01--vpc-02" does not allow it`, + }, + { + name: "a deny with only a detail reports it", + r: Reachability{Detail: "gw peering with non-empty expose 'As'"}, + expected: "gw peering with non-empty expose 'As'", + }, + { + name: "a pair no peering covers says so", + r: Reachability{}, + expected: "no peering allows it", + }, + { + name: "an allow with nothing recorded admits it", + r: Reachability{Reachable: true}, + expected: "no reason recorded", + }, + } { + t.Run(test.name, func(t *testing.T) { + require.Equal(t, test.expected, expectationWhy(test.r)) + }) + } +} diff --git a/pkg/hhfab/vlabbuilder.go b/pkg/hhfab/vlabbuilder.go index cc51c88bd..07e974889 100644 --- a/pkg/hhfab/vlabbuilder.go +++ b/pkg/hhfab/vlabbuilder.go @@ -365,6 +365,7 @@ func (b *VLABBuilderDefault) Build(ctx context.Context, l *apiutil.Loader, fabri extESLAGConns := uint8(0) extOrphanConns := uint8(0) mhLeaves := []string{} + mhESLAGLeaves := []string{} for eslagID := uint8(0); eslagID < uint8(len(eslagLeafGroups)); eslagID++ { //nolint:gosec sg := fmt.Sprintf("eslag-%d", eslagID+1) @@ -377,10 +378,7 @@ func (b *VLABBuilderDefault) Build(ctx context.Context, l *apiutil.Loader, fabri for eslagLeafID := uint8(0); eslagLeafID < leafs; eslagLeafID++ { leafName := fmt.Sprintf("leaf-%02d", leafID+eslagLeafID) leafNames = append(leafNames, leafName) - // add eslag leaves to candidates for multihomed servers if there are not enough orphan laves - if b.OrphanLeafsCount < 2 { - mhLeaves = append(mhLeaves, leafName) - } + mhESLAGLeaves = append(mhESLAGLeaves, leafName) if _, err := b.createSwitch(ctx, leafName, wiringapi.SwitchSpec{ Role: wiringapi.SwitchRoleServerLeaf, @@ -566,6 +564,12 @@ func (b *VLABBuilderDefault) Build(ctx context.Context, l *apiutil.Loader, fabri } } + // eslag leaves only top up the multihomed candidates when there aren't enough + // orphans, and go last so orphans get picked first + if b.OrphanLeafsCount < 2 { + mhLeaves = append(mhLeaves, mhESLAGLeaves...) + } + mhIdx := 0 for range int(b.MultiHomedServers) { serverName := fmt.Sprintf("server-%02d", serverID)