Repository: metallb/metallb
Pull request #1208 (2022-02-03 08:48:19)

E2E: Describe BGP network topology
As discussed in #1204 , it's useful to have a diagram with the test network topology.

I put it in `e2etest/README.md` and double linked to the implementing code.

oribon: nice!

Patch:
diff --git a/controller/service.go b/controller/service.go
index d32410a7..dbc1a9c5 100644
--- a/controller/service.go
+++ b/controller/service.go
@@ -15,6 +15,7 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 	"net"
 	"reflect"
@@ -25,6 +26,7 @@ import (
 	"github.com/go-kit/kit/log/level"
 	v1 "k8s.io/api/core/v1"
 
+	"go.universe.tf/metallb/internal/allocator"
 	"go.universe.tf/metallb/internal/allocator/k8salloc"
 	"go.universe.tf/metallb/internal/ipfamily"
 )
@@ -49,7 +51,7 @@ func (c *controller) convergeBalancer(l log.Logger, key string, svc *v1.Service)
 
 	// If the ClusterIPs is malformed or not set we can't determine the
 	// ipFamily to use.
-	if len(svc.Spec.ClusterIPs) == 0 {
+	if len(svc.Spec.ClusterIPs) == 0 && svc.Spec.ClusterIP == "" {
 		level.Info(l).Log("event", "clearAssignment", "reason", "noClusterIPs", "msg", "No ClusterIPs")
 		c.clearServiceState(key, svc)
 		return true
@@ -73,10 +75,10 @@ func (c *controller) convergeBalancer(l log.Logger, key string, svc *v1.Service)
 			c.client.Errorf(svc, "nolbIPsIPFamily", "Failed to retrieve LBIPs IPFamily for %q: %s", lbIPs, err)
 			return true
 		}
-		clusterIPsIPFamily, err := ipfamily.ForAddresses(svc.Spec.ClusterIPs)
+		clusterIPsIPFamily, err := ipfamily.ForService(svc)
 		if err != nil {
 			level.Error(l).Log("event", "clearAssignment", "reason", "noclusterIPsIPFamily", "msg", "Failed to retrieve clusterIPs family")
-			c.client.Errorf(svc, "noclusterIPsIPFamily", "Failed to retrieve ClusterIPs IPFamily for %q: %s", svc.Spec.ClusterIPs, err)
+			c.client.Errorf(svc, "noclusterIPsIPFamily", "Failed to retrieve ClusterIPs IPFamily for %q %s: %s", svc.Spec.ClusterIPs, svc.Spec.ClusterIP, err)
 			return true
 		}
 		// Clear the lbIP if it has a different ipFamily compared to the clusterIP.
@@ -96,6 +98,13 @@ func (c *controller) convergeBalancer(l log.Logger, key string, svc *v1.Service)
 		if err = c.ips.Assign(key, lbIPs, k8salloc.Ports(svc), k8salloc.SharingKey(svc), k8salloc.BackendKey(svc)); err != nil {
 			level.Info(l).Log("event", "clearAssignment", "error", err, "msg", "current IP not allowed by config, clearing")
 			c.clearServiceState(key, svc)
+			// Check if we cannot assign IP because services were sharing IP using
+			// "allow-shared-ip" annotation and one of them changed so instead of allocating
+			// new service IP we fail.
+			if errors.Is(err, allocator.ErrCannotShareKey) {
+				c.client.Errorf(svc, "svcCannotShareKey", "current IP not allowed by config:%s", err)
+				return false
+			}
 			lbIPs = []net.IP{}
 		}
 
@@ -176,12 +185,12 @@ func (c *controller) clearServiceState(key string, svc *v1.Service) {
 }
 
 func (c *controller) allocateIPs(key string, svc *v1.Service) ([]net.IP, error) {
-	if len(svc.Spec.ClusterIPs) == 0 {
+	if len(svc.Spec.ClusterIPs) == 0 && svc.Spec.ClusterIP == "" {
 		// (we should never get here because the caller ensured that Spec.ClusterIP != nil)
-		return nil, fmt.Errorf("invalid ClusterIP [%s], can't determine family", svc.Spec.ClusterIP)
+		return nil, fmt.Errorf("invalid ClusterIPs [%v] [%s], can't determine family", svc.Spec.ClusterIPs, svc.Spec.ClusterIP)
 	}
 
-	serviceIPFamily, err := ipfamily.ForAddresses(svc.Spec.ClusterIPs)
+	serviceIPFamily, err := ipfamily.ForService(svc)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/allocator/allocator.go b/internal/allocator/allocator.go
index 637af5b2..ff653c21 100644
--- a/internal/allocator/allocator.go
+++ b/internal/allocator/allocator.go
@@ -62,6 +62,8 @@ func New() *Allocator {
 	}
 }
 
+var ErrCannotShareKey = errors.New("services can't share key")
+
 // SetPools updates the set of address pools that the allocator owns.
 func (a *Allocator) SetPools(pools map[string]*config.Pool) error {
 	// All the fancy sharing stuff only influences how new allocations
@@ -417,7 +419,7 @@ func (a *Allocator) checkSharing(svc string, ip string, ports []Port, sk *key) e
 				}
 			}
 			if len(otherSvcs) > 0 {
-				return fmt.Errorf("can't change sharing key for %q, address also in use by %s", svc, strings.Join(otherSvcs, ","))
+				return fmt.Errorf("can't change sharing key for %q, address also in use by %s: %w", svc, strings.Join(otherSvcs, ","), ErrCannotShareKey)
 			}
 		}
 
diff --git a/internal/bgp/bgp.go b/internal/bgp/bgp.go
index 794fa5f3..7738f5b4 100644
--- a/internal/bgp/bgp.go
+++ b/internal/bgp/bgp.go
@@ -16,8 +16,6 @@ import (
 type Advertisement struct {
 	// The prefix being advertised to the peer.
 	Prefix *net.IPNet
-	// The address of the router to which the peer should forward traffic.
-	NextHop net.IP
 	// The local preference of this route. Only propagated to IBGP
 	// peers (i.e. where the peer ASN matches the local ASN).
 	LocalPref uint32
@@ -30,9 +28,6 @@ func (a *Advertisement) Equal(b *Advertisement) bool {
 	if a.Prefix.String() != b.Prefix.String() {
 		return false
 	}
-	if !a.NextHop.Equal(b.NextHop) {
-		return false
-	}
 	if a.LocalPref != b.LocalPref {
 		return false
 	}
diff --git a/internal/bgp/frr/frr.go b/internal/bgp/frr/frr.go
index 2333e835..f502c1f8 100644
--- a/internal/bgp/frr/frr.go
+++ b/internal/bgp/frr/frr.go
@@ -57,18 +57,6 @@ func sessionName(myAddr string, myAsn uint32, addr string, asn uint32) string {
 }
 
 func validate(adv *bgp.Advertisement) error {
-	if adv.Prefix.IP.To4() != nil {
-		if adv.NextHop != nil && adv.NextHop.To4() == nil {
-			return fmt.Errorf("next-hop must be IPv4, got %q", adv.NextHop)
-		}
-	} else if adv.Prefix.IP.To16() != nil {
-		if adv.NextHop != nil && adv.NextHop.To16() == nil {
-			return fmt.Errorf("next-hop must be IPv6, got %q", adv.NextHop)
-		}
-	} else {
-		return fmt.Errorf("unable to validate IP address")
-	}
-
 	if len(adv.Communities) > 63 {
 		return fmt.Errorf("max supported communities is 63, got %d", len(adv.Communities))
 	}
diff --git a/internal/bgp/native/messages.go b/internal/bgp/native/messages.go
index 3ee20d71..255ea764 100644
--- a/internal/bgp/native/messages.go
+++ b/internal/bgp/native/messages.go
@@ -289,7 +289,7 @@ func readCapabilities(r io.Reader, ret *openResult) error {
 	}
 }
 
-func sendUpdate(w io.Writer, asn uint32, ibgp, fbasn bool, defaultNextHop net.IP, adv *bgp.Advertisement) error {
+func sendUpdate(w io.Writer, asn uint32, ibgp, fbasn bool, nextHop net.IP, adv *bgp.Advertisement) error {
 	var b bytes.Buffer
 
 	hdr := struct {
@@ -307,7 +307,7 @@ func sendUpdate(w io.Writer, asn uint32, ibgp, fbasn bool, defaultNextHop net.IP
 		return err
 	}
 	l := b.Len()
-	if err := encodePathAttrs(&b, asn, ibgp, fbasn, defaultNextHop, adv); err != nil {
+	if err := encodePathAttrs(&b, asn, ibgp, fbasn, nextHop, adv); err != nil {
 		return err
 	}
 	binary.BigEndian.PutUint16(b.Bytes()[21:23], uint16(b.Len()-l))
@@ -335,7 +335,7 @@ func bytesForBits(n int) int {
 	return ((n + 7) &^ 7) / 8
 }
 
-func encodePathAttrs(b *bytes.Buffer, asn uint32, ibgp, fbasn bool, defaultNextHop net.IP, adv *bgp.Advertisement) error {
+func encodePathAttrs(b *bytes.Buffer, asn uint32, ibgp, fbasn bool, nextHop net.IP, adv *bgp.Advertisement) error {
 	b.Write([]byte{
 		0x40, 1, // mandatory, origin
 		1, // len
@@ -370,11 +370,9 @@ func encodePathAttrs(b *bytes.Buffer, asn uint32, ibgp, fbasn bool, defaultNextH
 		0x40, 3, // mandatory, next-hop
 		4, // len
 	})
-	if adv.NextHop != nil {
-		b.Write(adv.NextHop.To4())
-	} else {
-		b.Write(defaultNextHop)
-	}
+
+	b.Write(nextHop)
+
 	if ibgp {
 		b.Write([]byte{
 			0x40, 5, // well-known, localpref
diff --git a/internal/bgp/native/native.go b/internal/bgp/native/native.go
index 044a7825..d5c2140c 100644
--- a/internal/bgp/native/native.go
+++ b/internal/bgp/native/native.go
@@ -49,7 +49,7 @@ type session struct {
 	closed         bool
 	conn           net.Conn
 	actualHoldTime time.Duration
-	defaultNextHop net.IP
+	nextHop        net.IP
 	advertised     map[string]*bgp.Advertisement
 	new            map[string]*bgp.Advertisement
 }
@@ -142,7 +142,7 @@ func (s *session) sendUpdates() bool {
 	}
 
 	for c, adv := range s.advertised {
-		if err := sendUpdate(s.conn, s.myASN, ibgp, fbasn, s.defaultNextHop, adv); err != nil {
+		if err := sendUpdate(s.conn, s.myASN, ibgp, fbasn, s.nextHop, adv); err != nil {
 			s.abort()
 			level.Error(s.logger).Log("op", "sendUpdate", "ip", c, "error", err, "msg", "failed to send BGP update")
 			return true
@@ -175,7 +175,7 @@ func (s *session) sendUpdates() bool {
 				continue
 			}
 
-			if err := sendUpdate(s.conn, s.myASN, ibgp, fbasn, s.defaultNextHop, adv); err != nil {
+			if err := sendUpdate(s.conn, s.myASN, ibgp, fbasn, s.nextHop, adv); err != nil {
 				s.abort()
 				level.Error(s.logger).Log("op", "sendUpdate", "prefix", c, "error", err, "msg", "failed to send BGP update")
 				return true
@@ -232,11 +232,11 @@ func (s *session) connect() error {
 		conn.Close()
 		return fmt.Errorf("getting local addr for default nexthop to %q: %s", s.addr, err)
 	}
-	s.defaultNextHop = addr.IP
+	s.nextHop = addr.IP
 
 	routerID := s.routerID
 	if routerID == nil {
-		routerID, err = getRouterID(s.defaultNextHop, s.myNode)
+		routerID, err = getRouterID(s.nextHop, s.myNode)
 		if err != nil {
 			return err
 		}
@@ -446,9 +446,6 @@ func validate(adv *bgp.Advertisement) error {
 		return fmt.Errorf("cannot advertise non-v4 prefix %q", adv.Prefix)
 	}
 
-	if adv.NextHop != nil && adv.NextHop.To4() == nil {
-		return fmt.Errorf("next-hop must be IPv4, got %q", adv.NextHop)
-	}
 	if len(adv.Communities) > 63 {
 		return fmt.Errorf("max supported communities is 63, got %d", len(adv.Communities))
 	}
diff --git a/internal/ipfamily/ipfamily.go b/internal/ipfamily/ipfamily.go
index ad813948..b7cb01a1 100644
--- a/internal/ipfamily/ipfamily.go
+++ b/internal/ipfamily/ipfamily.go
@@ -5,6 +5,8 @@ package ipfamily // import "go.universe.tf/metallb/internal/ipfamily"
 import (
 	"fmt"
 	"net"
+
+	v1 "k8s.io/api/core/v1"
 )
 
 // IP family helps identifying single stack IPv4/IPv6 vs Dual-stack ["IPv4", "IPv6"] or ["IPv6", "Ipv4"].
@@ -71,3 +73,13 @@ func ForAddress(ip net.IP) Family {
 	}
 	return IPv4
 }
+
+// ForService returns the address family of a given service.
+func ForService(svc *v1.Service) (Family, error) {
+	if len(svc.Spec.ClusterIPs) > 0 {
+		return ForAddresses(svc.Spec.ClusterIPs)
+	}
+	// fallback to clusterip if clusterips are not set
+	addresses := []string{svc.Spec.ClusterIP}
+	return ForAddresses(addresses)
+}
diff --git a/tasks.py b/tasks.py
index b4006d43..977e0be3 100644
--- a/tasks.py
+++ b/tasks.py
@@ -321,7 +321,8 @@ def dev_env(ctx, architecture="amd64", name="kind", protocol=None, frr_volume_di
 
     if helm_install:
         run("helm install metallb charts/metallb/ --set controller.image.tag=dev-{} "
-                "--set speaker.image.tag=dev-{} --set speaker.frr.enabled={}".format(architecture,
+                "--set speaker.image.tag=dev-{} --set speaker.frr.enabled={} --set speaker.logLevel=debug "
+                "--set controller.logLevel=debug".format(architecture,
                 architecture, "true" if bgp_type == "frr" else "false"), echo=True)
     else:
         run("kubectl delete po -nmetallb-system --all", echo=True)
@@ -340,6 +341,8 @@ def dev_env(ctx, architecture="amd64", name="kind", protocol=None, frr_volume_di
                 manifest = re.sub("image: quay.io/metallb/{}:.*".format(image),
                             "image: quay.io/metallb/{}:dev-{}".format(image, architecture), manifest)
                 manifest = re.sub("--log-level=info", "--log-level={}".format(log_level), manifest)
+            manifest.replace("--log-level=info", "--log-level=debug")
+
             with open(tmpdir + "/metallb.yaml", "w") as f:
                 f.write(manifest)
                 f.flush()


Test patch:
diff --git a/controller/controller_test.go b/controller/controller_test.go
index e76646eb..f24fa264 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -720,6 +720,21 @@ func TestControllerMutation(t *testing.T) {
 				Status: statusAssigned([]string{"1.2.3.0", "1000::"}),
 			},
 		},
+		{
+			desc: "request dual-stack loadbalancer IPs via custom annotation in a single stack cluster",
+			in: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationLoadBalancerIPs: "1.2.3.0,1000::",
+					},
+				},
+				Spec: v1.ServiceSpec{
+					ClusterIP: "1.2.3.4",
+					Type:      "LoadBalancer",
+				},
+			},
+			wantErr: true,
+		},
 	}
 
 	for i := 0; i < 100; i++ {
@@ -1032,3 +1047,73 @@ func TestControllerDualStackConfig(t *testing.T) {
 		t.Fatalf("SetConfig that deletes the config was accepted")
 	}
 }
+
+func TestControllerSvcAdddressSharing(t *testing.T) {
+	k := &testK8S{t: t}
+	c := &controller{
+		ips:    allocator.New(),
+		client: k,
+	}
+	l := log.NewNopLogger()
+
+	// Set a config with some IPs. Still no allocation, not synced.
+	cfg := &config.Config{
+		Pools: map[string]*config.Pool{
+			"default": {
+				AutoAssign: true,
+				CIDR:       []*net.IPNet{ipnet("4.5.6.0/24")},
+			},
+		},
+	}
+	if c.SetConfig(l, cfg) == k8s.SyncStateError {
+		t.Fatalf("SetConfig failed")
+	}
+	c.MarkSynced(l)
+
+	// Configure svc1
+	svc1 := &v1.Service{
+		Spec: v1.ServiceSpec{
+			Type:                  "LoadBalancer",
+			ClusterIPs:            []string{"4.5.6.1"},
+			ExternalTrafficPolicy: "Local",
+			Ports: []v1.ServicePort{
+				{
+					Protocol: v1.ProtocolTCP,
+					Port:     3000,
+				},
+			},
+		},
+	}
+
+	if c.SetBalancer(l, "test1", svc1, k8s.EpsOrSlices{}) == k8s.SyncStateError {
+		t.Fatalf("SetBalancer failed")
+	}
+	gotSvc1 := k.gotService(svc1)
+	if len(gotSvc1.Status.LoadBalancer.Ingress) == 0 || gotSvc1.Status.LoadBalancer.Ingress[0].IP != "4.5.6.0" {
+		t.Fatal("svc1 didn't get an IP")
+	}
+	k.reset()
+	// Configure svc2 with IP sharing with svc1 and different port/protocol/ETP
+	svc2 := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				"metallb.universe.tf/allow-shared-ip": "share",
+			},
+		},
+		Spec: v1.ServiceSpec{
+			Type:                  "LoadBalancer",
+			ClusterIPs:            []string{"4.5.6.1"},
+			ExternalTrafficPolicy: "Cluster",
+			Ports: []v1.ServicePort{
+				{
+					Protocol: v1.ProtocolUDP,
+					Port:     1000,
+				},
+			},
+		},
+		Status: statusAssigned([]string{"4.5.6.0"}),
+	}
+	if c.SetBalancer(l, "test2", svc2, k8s.EpsOrSlices{}) != k8s.SyncStateError {
+		t.Fatalf("SetBalancer did not fail")
+	}
+}
diff --git a/e2etest/README.md b/e2etest/README.md
index 1ae43fae..dfdaf73c 100644
--- a/e2etest/README.md
+++ b/e2etest/README.md
@@ -1,4 +1,5 @@
 # E2E Tests
+
 The MetalLB E2E test suite can be run by creating a development cluster:
 
 ```
@@ -26,3 +27,39 @@ inv e2etest --focus L2
 The test suite will run the appropriate tests against the cluster.
 Be sure to cleanup any previously created development clusters using `inv dev-env-cleanup`.
 
+
+## BGP tests network topology
+
+In order to test the multiple scenarios required for BGP, three different containers are required.
+The following diagram describes the networks, the pods and the containers involved.
+
+```
+  ┌────────────────────────────────────────────┐
+  │                exec network                │
+  │ ┌─────────┐                                │
+  │ │         │                                │
+  │ │ Speaker ├────┐      ┌─────────────────┐  │
+  │ │         │    │      │                 │  │
+  │ └─────────┘    │      │ ibgp-single-hop │  │
+  │                │      │                 │  │
+  │ ┌─────────┐    │      └─────────────────┘  │
+  │ │         │    │                           │
+  │ │ Speaker ├────┤   ┌───────────────────────┼──────────────────────────┐
+  │ │         │    │   │                       │                          │
+  │ └─────────┘    │   │  ┌─────────────────┐  │       ┌────────────────┐ │
+  │                │   │  │                 │  │       │                │ │
+  │ ┌─────────┐    ├───┼──┤ ebgp-single-hop ├──┼───────┤ ibgp-multi-hop │ │
+  │ │         │    │   │  │                 │  │       │                │ │
+  │ │ Speaker │    │   │  └─────────────────┘  │       └────────────────┘ │
+  │ │         ├────┘   │                       │                          │
+  │ └─────────┘        │                       │       ┌────────────────┐ │
+  │                    │                       │       │                │ │
+  └────────────────────┼───────────────────────┘       │ ebgp-multi-hop │ │
+                       │                               │                │ │
+                       │       multi-hop-net           └────────────────┘ │
+                       │       172.30.0.0/16                              │
+                       │  fc00:f853:ccd:e798::/64                         │
+                       └──────────────────────────────────────────────────┘
+```
+
+The above diagram is implemented in `infra_setup.go`.
diff --git a/e2etest/bgptests/infra_setup.go b/e2etest/bgptests/infra_setup.go
index cb6eb280..79dc85f6 100644
--- a/e2etest/bgptests/infra_setup.go
+++ b/e2etest/bgptests/infra_setup.go
@@ -62,6 +62,8 @@ func InfraSetup(ipv4Addresses, ipv6Addresses []string, cs *clientset.Clientset)
 		We then wire these networks by adding static routes to both the speaker nodes
 		containers (we're using kind) and the ibgp/ebgp containers connected to multi-hop-net,
 		using the 2nd container as a gateway.
+
+		See `e2etest/README.md` for more details.
 	*/
 
 	ibgpSingleHopContainerConfig := frrcontainer.Config{
diff --git a/internal/bgp/frr/frr_test.go b/internal/bgp/frr/frr_test.go
index 583e572d..71ae2df4 100644
--- a/internal/bgp/frr/frr_test.go
+++ b/internal/bgp/frr/frr_test.go
@@ -241,7 +241,6 @@ func TestSingleAdvertisement(t *testing.T) {
 	communities = append(communities, community)
 	adv := &bgp.Advertisement{
 		Prefix:      prefix,
-		NextHop:     net.ParseIP("10.1.1.1"),
 		Communities: communities,
 		LocalPref:   300,
 	}
@@ -272,8 +271,7 @@ func TestSingleAdvertisementNoRouterID(t *testing.T) {
 	}
 
 	adv := &bgp.Advertisement{
-		Prefix:  prefix,
-		NextHop: net.ParseIP("10.1.1.1"),
+		Prefix: prefix,
 	}
 
 	err = session.Set(adv)
@@ -284,33 +282,6 @@ func TestSingleAdvertisementNoRouterID(t *testing.T) {
 	testCheckConfigFile(t)
 }
 
-func TestSingleAdvertisementInvalidPrefix(t *testing.T) {
-	testSetup(t)
-
-	l := log.NewNopLogger()
-	sessionManager := NewSessionManager(l, logging.LevelInfo)
-	defer close(sessionManager.reloadConfig)
-	session, err := sessionManager.NewSession(l, "10.2.2.254:179", net.ParseIP("10.1.1.254"), 100, net.ParseIP("10.1.1.254"), 200, time.Second, time.Second, "password", "hostname", "", true)
-	if err != nil {
-		t.Fatalf("Could not create session: %s", err)
-	}
-	defer session.Close()
-
-	prefix := &net.IPNet{}
-
-	adv := &bgp.Advertisement{
-		Prefix:  prefix,
-		NextHop: net.ParseIP("10.1.1.1"),
-	}
-
-	err = session.Set(adv)
-	if err == nil {
-		t.Fatalf("Set should return error")
-	}
-
-	testCheckConfigFile(t)
-}
-
 func TestSingleAdvertisementInvalidNoPort(t *testing.T) {
 	testSetup(t)
 
@@ -326,36 +297,6 @@ func TestSingleAdvertisementInvalidNoPort(t *testing.T) {
 	// Not checking the file since this test won't create it
 }
 
-func TestSingleAdvertisementInvalidNextHop(t *testing.T) {
-	t.Skip("TODO: bgp.Validate() incorrectly(?) returns err == nil")
-	testSetup(t)
-
-	l := log.NewNopLogger()
-	sessionManager := NewSessionManager(l, logging.LevelInfo)
-	defer close(sessionManager.reloadConfig)
-	session, err := sessionManager.NewSession(l, "10.2.2.254:179", net.ParseIP("10.1.1.254"), 100, net.ParseIP("10.1.1.254"), 200, time.Second, time.Second, "password", "hostname", "", true)
-	if err != nil {
-		t.Fatalf("Could not create session: %s", err)
-	}
-	defer session.Close()
-
-	prefix := &net.IPNet{
-		IP:   net.ParseIP("172.16.1.10"),
-		Mask: classCMask,
-	}
-
-	adv := &bgp.Advertisement{
-		Prefix: prefix,
-	}
-
-	err = session.Set(adv)
-	if err != nil {
-		t.Fatalf("Could not advertise prefix: %s", err)
-	}
-
-	testCheckConfigFile(t)
-}
-
 func TestSingleAdvertisementStop(t *testing.T) {
 	testSetup(t)
 
@@ -374,8 +315,7 @@ func TestSingleAdvertisementStop(t *testing.T) {
 	}
 
 	adv := &bgp.Advertisement{
-		Prefix:  prefix,
-		NextHop: net.ParseIP("10.1.1.1"),
+		Prefix: prefix,
 	}
 
 	err = session.Set(adv)
@@ -409,8 +349,7 @@ func TestSingleAdvertisementChange(t *testing.T) {
 	}
 
 	adv := &bgp.Advertisement{
-		Prefix:  prefix,
-		NextHop: net.ParseIP("10.1.1.1"),
+		Prefix: prefix,
 	}
 
 	err = session.Set(adv)
@@ -424,8 +363,7 @@ func TestSingleAdvertisementChange(t *testing.T) {
 	}
 
 	adv = &bgp.Advertisement{
-		Prefix:  prefix,
-		NextHop: net.ParseIP("10.1.1.1"),
+		Prefix: prefix,
 	}
 
 	err = session.Set(adv)
@@ -457,7 +395,6 @@ func TestTwoAdvertisements(t *testing.T) {
 	communities = append(communities, community)
 	adv1 := &bgp.Advertisement{
 		Prefix:      prefix1,
-		NextHop:     net.ParseIP("10.1.1.1"),
 		Communities: communities,
 	}
 
@@ -467,8 +404,7 @@ func TestTwoAdvertisements(t *testing.T) {
 	}
 
 	adv2 := &bgp.Advertisement{
-		Prefix:  prefix2,
-		NextHop: net.ParseIP("10.1.1.1"),
+		Prefix: prefix2,
 	}
 
 	err = session.Set(adv1, adv2)
@@ -506,7 +442,6 @@ func TestTwoAdvertisementsTwoSessions(t *testing.T) {
 	communities = append(communities, community)
 	adv1 := &bgp.Advertisement{
 		Prefix:      prefix1,
-		NextHop:     net.ParseIP("10.1.1.1"),
 		Communities: communities,
 	}
 
@@ -517,7 +452,6 @@ func TestTwoAdvertisementsTwoSessions(t *testing.T) {
 
 	adv2 := &bgp.Advertisement{
 		Prefix:      prefix2,
-		NextHop:     net.ParseIP("10.1.1.2"),
 		Communities: communities,
 		LocalPref:   2,
 	}
diff --git a/internal/bgp/native/native_test.go b/internal/bgp/native/native_test.go
index 1c8ce22c..115670b8 100644
--- a/internal/bgp/native/native_test.go
+++ b/internal/bgp/native/native_test.go
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier:Apache-2.0
 
+//go:build disabled
 // +build disabled
 
 package native
@@ -94,7 +95,6 @@ func TestInterop(t *testing.T) {
 
 	adv := &Advertisement{
 		Prefix:      ipnet("1.2.3.0/24"),
-		NextHop:     net.ParseIP("10.20.30.40"),
 		LocalPref:   42,
 		Communities: []uint32{1234, 2345},
 	}
@@ -134,7 +134,6 @@ func TestTCPMD5(t *testing.T) {
 
 	adv := &Advertisement{
 		Prefix:      ipnet("1.2.3.0/24"),
-		NextHop:     net.ParseIP("10.20.30.40"),
 		LocalPref:   42,
 		Communities: []uint32{1234, 2345},
 	}
@@ -178,7 +177,6 @@ func checkPath(path *api.Path, adv *Advertisement) error {
 			if err := ptypes.UnmarshalAny(attr, &nh); err != nil {
 				return err
 			}
-			nexthop = nh.NextHop
 		case ptypes.Is(attr, &api.LocalPrefAttribute{}):
 			var lp api.LocalPrefAttribute
 			if err := ptypes.UnmarshalAny(attr, &lp); err != nil {
@@ -194,9 +192,6 @@ func checkPath(path *api.Path, adv *Advertisement) error {
 		}
 	}
 
-	if nexthop != adv.NextHop.String() {
-		return fmt.Errorf("wrong nexthop, got %s, want %s", nexthop, adv.NextHop)
-	}
 	if localpref != adv.LocalPref {
 		return fmt.Errorf("wrong localpref, got %d, want %d", localpref, adv.LocalPref)
 	}
diff --git a/speaker/bgp_controller_test.go b/speaker/bgp_controller_test.go
index e2a052eb..f2e717bd 100644
--- a/speaker/bgp_controller_test.go
+++ b/speaker/bgp_controller_test.go
@@ -73,9 +73,6 @@ func sortAds(ads map[string][]*bgp.Advertisement) {
 			if a.LocalPref != b.LocalPref {
 				return a.LocalPref < b.LocalPref
 			}
-			if a.NextHop.String() != b.NextHop.String() {
-				return a.NextHop.String() < b.NextHop.String()
-			}
 			if len(a.Communities) != len(b.Communities) {
 				return len(a.Communities) < len(b.Communities)
 			}