test-connectivity follow-up + multihomed unbundled + ACLs

cmd/hhfab/main.go

@@ -1440,7 +1440,7 @@ Examples:
 								return err
 							}
 
-							if _, err := hhfab.DoVLABSetupVPCs(ctx, workDir, cacheDir, hhfab.SetupVPCsOpts{
+							if _, _, err := hhfab.DoVLABSetupVPCs(ctx, workDir, cacheDir, hhfab.SetupVPCsOpts{
 								WaitSwitchesReady: c.Bool("wait-switches-ready"),
 								ForceCleanup:      c.Bool("force-cleanup"),
 								VLANNamespace:     c.String("vlanns"),

pkg/hhfab/cmdvlab.go

@@ -183,10 +183,10 @@ func DoShowTech(ctx context.Context, workDir, cacheDir string) error {
 	return c.VLABShowTech(ctx, vlab, ShowTechOpts{})
 }
 
-func DoVLABSetupVPCs(ctx context.Context, workDir, cacheDir string, opts SetupVPCsOpts) ([]*Endpoint, error) {
+func DoVLABSetupVPCs(ctx context.Context, workDir, cacheDir string, opts SetupVPCsOpts) ([]*Endpoint, []DroppedEndpoint, error) {
 	c, vlab, err := loadVLABForHelpers(ctx, workDir, cacheDir)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	return c.SetupVPCs(ctx, vlab, opts)

pkg/hhfab/endpoints.go

@@ -20,14 +20,8 @@ import (
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// SSHResolver returns the SSH config for a server. The collector and per-
-// server refresh helpers take one of these instead of a pre-built
-// map[string]*sshutil.Config so callers can share whatever SSH plumbing
-// they already have (SetupVPCs builds a map keyed by VM name; rt_* tests
-// resolve lazily through testCtx.getSSH).
 type SSHResolver func(server string) (*sshutil.Config, error)
 
-// SSHResolverFromMap adapts a pre-built map into an SSHResolver.
 func SSHResolverFromMap(m map[string]*sshutil.Config) SSHResolver {
 	return func(server string) (*sshutil.Config, error) {
 		cfg, ok := m[server]
@@ -39,9 +33,6 @@ func SSHResolverFromMap(m map[string]*sshutil.Config) SSHResolver {
 	}
 }
 
-// discoveredIP pairs a server-side interface name with the address found
-// on it. The interface tells us whether the address is a hostBGP /32 VIP
-// (on `lo`) or a regular subnet address (on the bond/VLAN interface).
 type discoveredIP struct {
 	iface  string
 	prefix netip.Prefix
@@ -77,8 +68,24 @@ func discoverServerIPs(ctx context.Context, sshCfg *sshutil.Config, server strin
 	return out, nil
 }
 
-// serverAttachment is the (vpc, subnet) information the collector resolves
-// from a VPCAttachment + its referenced VPC CRD.
+// DroppedEndpoint records an attachment or address that endpoint discovery
+// could not turn into a testable *Endpoint. ConnectivityMatrix.Validate()
+// refuses to run a test against a matrix that carries any of these.
+type DroppedEndpoint struct {
+	Server string
+	VPC    string
+	Subnet string
+	Reason string
+}
+
+func (d DroppedEndpoint) String() string {
+	if d.VPC == "" {
+		return fmt.Sprintf("%s: %s", d.Server, d.Reason)
+	}
+
+	return fmt.Sprintf("%s (%s/%s): %s", d.Server, d.VPC, d.Subnet, d.Reason)
+}
+
 type serverAttachment struct {
 	vpcName    string
 	subnetName string
@@ -90,22 +97,10 @@ type serverAttachment struct {
 // 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).
-//
-// Algorithm: list VPCAttachments → group by server (resolved via each
-// attachment's Connection); for each candidate server, SSH it and read all
-// IPv4 addresses; match each address to one of its attachments by CIDR
-// containment (narrowest prefix wins on ties). HostBGP is derived from
-// VPCSubnet.HostBGP. An attachment with no matching configured address is
-// dropped with a warning; a server with no configured addresses contributes
-// nothing — this matches today's SetupVPCs behavior, where ESLAG servers
-// in L3VNI mode are skipped before hhnet runs and therefore have no IPs.
-//
-// Returns an error only when SSH itself fails on a queried server or when a
-// VPCAttachment cannot be resolved to a Connection.
-func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHResolver, servers []string) ([]*Endpoint, error) {
+func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHResolver, servers []string) ([]*Endpoint, []DroppedEndpoint, error) {
 	attaches := &vpcapi.VPCAttachmentList{}
 	if err := kube.List(ctx, attaches); err != nil {
-		return nil, fmt.Errorf("listing VPCAttachments: %w", err)
+		return nil, nil, fmt.Errorf("listing VPCAttachments: %w", err)
 	}
 
 	connCache := map[string]*wiringapi.Connection{}
@@ -145,17 +140,13 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes
 	for _, attach := range attaches.Items {
 		conn, err := getConn(attach.Spec.Connection)
 		if err != nil {
-			return nil, fmt.Errorf("resolving attachment %q: %w", attach.Name, err)
+			return nil, nil, fmt.Errorf("resolving attachment %q: %w", attach.Name, err)
 		}
 		_, srvs, _, _, err := conn.Spec.Endpoints()
 		if err != nil {
-			return nil, fmt.Errorf("getting endpoints of connection %q: %w", conn.Name, err)
+			return nil, nil, fmt.Errorf("getting endpoints of connection %q: %w", conn.Name, err)
 		}
 		if len(srvs) != 1 {
-			// VPCAttachments only reference server-facing connections; if a
-			// connection has no server endpoint we skip it as a malformed
-			// attachment rather than erroring out, since fabric webhooks
-			// already gate this.
 			continue
 		}
 		serverName := srvs[0]
@@ -165,16 +156,26 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes
 
 		vpc, err := getVPC(attach.Spec.VPCName())
 		if err != nil {
-			return nil, fmt.Errorf("resolving attachment %q: %w", attach.Name, err)
+			return nil, nil, fmt.Errorf("resolving attachment %q: %w", attach.Name, err)
 		}
 		subnetName := attach.Spec.SubnetName()
 		subnet, ok := vpc.Spec.Subnets[subnetName]
 		if !ok {
-			return nil, fmt.Errorf("attachment %q references missing subnet %s/%s", attach.Name, vpc.Name, subnetName) //nolint:goerr113
+			return nil, nil, fmt.Errorf("attachment %q references missing subnet %s/%s", attach.Name, vpc.Name, subnetName) //nolint:goerr113
 		}
 		cidr, err := netip.ParsePrefix(subnet.Subnet)
 		if err != nil {
-			return nil, fmt.Errorf("parsing VPC %s/%s subnet CIDR %q: %w", vpc.Name, subnetName, subnet.Subnet, err)
+			return nil, nil, fmt.Errorf("parsing VPC %s/%s subnet CIDR %q: %w", vpc.Name, subnetName, subnet.Subnet, err)
+		}
+
+		// A multihomed hostBGP server has several VPCAttachments pointing at the
+		// same (vpc, subnet) — one per connection — but a single /32 VIP. Collapse
+		// them to one candidate attachment so the server yields exactly one
+		// endpoint instead of dropping the duplicates with misleading warnings.
+		if subnet.HostBGP && slices.ContainsFunc(serverAttachments[serverName], func(a serverAttachment) bool {
+			return a.vpcName == vpc.Name && a.subnetName == subnetName
+		}) {
+			continue
 		}
 
 		serverAttachments[serverName] = append(serverAttachments[serverName], serverAttachment{
@@ -186,8 +187,6 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes
 		})
 	}
 
-	// Probe every candidate server in parallel; errgroup mirrors what
-	// SetupVPCs does for the hhnet config loop.
 	type collected struct {
 		serverName string
 		ips        []discoveredIP
@@ -220,20 +219,19 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes
 		})
 	}
 	if err := eg.Wait(); err != nil {
-		return nil, fmt.Errorf("probing servers for IPs: %w", err)
+		return nil, nil, fmt.Errorf("probing servers for IPs: %w", err)
 	}
 
 	// Build endpoints by matching each discovered IP against the server's
-	// candidate attachments (already narrowed by VPCAttachment list). When
-	// multiple attachments contain the IP, the narrowest-prefix attachment
-	// wins (handles the P2P /31 case where a /31 sits inside the parent
-	// /24).
+	// candidate attachments; narrowest-prefix attachment wins
 	slices.SortFunc(probed, func(a, b collected) int { return strings.Compare(a.serverName, b.serverName) })
 	out := []*Endpoint{}
+	dropped := []DroppedEndpoint{}
 	for _, p := range probed {
 		atts := serverAttachments[p.serverName]
 		used := make([]bool, len(atts))
 		if len(p.ips) == 0 {
+			// 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))
 
 			continue
@@ -252,6 +250,10 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes
 			}
 			if bestIdx < 0 {
 				slog.Warn("Server IP does not match any attachment subnet", "server", p.serverName, "iface", ip.iface, "addr", ip.prefix.String())
+				dropped = append(dropped, DroppedEndpoint{
+					Server: p.serverName,
+					Reason: fmt.Sprintf("address %s on %s matches none of the server's %d attachment subnets", ip.prefix.String(), ip.iface, len(atts)),
+				})
 
 				continue
 			}
@@ -279,35 +281,25 @@ func CollectServerEndpoints(ctx context.Context, kube kclient.Client, ssh SSHRes
 			if !used[i] {
 				slog.Warn("Attachment has no matching IP on server, dropping endpoint",
 					"server", p.serverName, "vpc", att.vpcName, "subnet", att.subnetName, "attachment", att.attachName)
+				dropped = append(dropped, DroppedEndpoint{
+					Server: p.serverName,
+					VPC:    att.vpcName,
+					Subnet: att.subnetName,
+					Reason: fmt.Sprintf("attachment %s has no matching address among the server's %d configured addresses", att.attachName, len(p.ips)),
+				})
 			}
 		}
 	}
 
-	return out, nil
+	return out, dropped, nil
 }
 
 // ReplaceServerEndpoints reconciles the matrix's endpoints for one
 // server against newEPs:
-//
 //   - Existing endpoints whose (vpc, subnet) match an entry in newEPs
-//     are updated in place — the IP and HostBGP fields are copied
-//     across, but the *Endpoint pointer stays the same. Matrix entries
-//     keyed on that pointer remain valid, so suite setups that don't
-//     wipe between tests keep their verdicts.
-//   - Existing endpoints with no (vpc, subnet) match in newEPs are
-//     dropped from AllEndpoints; entries referencing them are deleted
-//     (the topology those verdicts assumed no longer holds).
-//   - newEPs that don't match any existing endpoint are appended; the
-//     matrix has no entries for them until the next Repopulate or
-//     overlay.
-//
-// Use after a runtime topology change (server moved to a different VPC,
-// or DHCP lease refreshed) to bring the matrix back in sync with the
-// cluster while preserving entries for attachments that didn't change.
-//
-// Entries in newEPs whose Server.Name != name are appended as-is, so
-// mixing External endpoints into the slice is a no-op for the matching
-// pass.
+//     are updated in place
+//   - Existing endpoints with no (vpc, subnet) match in newEPs are dropped
+//   - newEPs that don't match any existing endpoint are appended
 func (m *ConnectivityMatrix) ReplaceServerEndpoints(name string, newEPs []*Endpoint) {
 	if m == nil {
 		return
@@ -364,3 +356,22 @@ func (m *ConnectivityMatrix) ReplaceServerEndpoints(name string, newEPs []*Endpo
 		}
 	}
 }
+
+func (m *ConnectivityMatrix) ReplaceServerDrops(name string, dropped []DroppedEndpoint) {
+	if m == nil {
+		return
+	}
+
+	kept := make([]DroppedEndpoint, 0, len(m.dropped)+len(dropped))
+	for _, d := range m.dropped {
+		if d.Server != name {
+			kept = append(kept, d)
+		}
+	}
+	for _, d := range dropped {
+		if d.Server == name {
+			kept = append(kept, d)
+		}
+	}
+	m.dropped = kept
+}