bgpv2: use peer asn and address in the key

On updating peer ASN and address, we need to remove old peer and readd
it again. This change adds ASN and address in the key of peer along with
the name.

Secondly, order of changing peer is now delete, update and add. This is
done because adding before deleting old peer is going to fail when
changing ASN.

This change also allows setting local peer port to 0 (random port
selection from kernel)

Signed-off-by: harsimran pabla <hpabla@isovalent.com>

pkg/bgpv1/gobgp/peer.go

@@ -247,9 +247,8 @@ func (g *GoBGPServer) setPeerTransport(peer, existingPeer *gobgp.Peer, peerAddr
 		peer.Transport = &gobgp.Transport{}
 	}
 
-	if localPort > 0 {
-		peer.Transport.LocalPort = localPort
-	}
+	// update local port, 0 is fine as well. In which case, linux will assign a random port.
+	peer.Transport.LocalPort = localPort
 
 	if peerPort > 0 {
 		peer.Transport.RemotePort = peerPort

pkg/bgpv1/manager/reconcilerv2/neighbor.go

@@ -175,8 +175,19 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, p ReconcileParams) e
 	nset := map[string]*member{}
 
 	for i, n := range newNeigh {
+		// validate that peer has ASN and address. In current implementation these fields are
+		// mandatory for a peer. Eventually we will relax this restriction with implementation
+		// of BGP unnumbered.
+		if n.PeerASN == nil {
+			return fmt.Errorf("peer %s does not have a PeerASN", n.Name)
+		}
+
+		if n.PeerAddress == nil {
+			return fmt.Errorf("peer %s does not have a PeerAddress", n.Name)
+		}
+
 		var (
-			key = n.Name
+			key = r.neighborID(&n)
 			h   *member
 			ok  bool
 		)
@@ -210,7 +221,7 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, p ReconcileParams) e
 
 	for i, n := range curNeigh {
 		var (
-			key = n.Peer.Name
+			key = r.neighborID(n.Peer)
 			h   *member
 			ok  bool
 		)
@@ -247,19 +258,17 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, p ReconcileParams) e
 		l.Debug("No peer changes necessary")
 	}
 
-	// create new neighbors
-	for _, n := range toCreate {
-		l.WithField(types.PeerLogField, n.Peer.Name).Info("Adding peer")
+	// remove neighbors
+	for _, n := range toRemove {
+		l.WithField(types.PeerLogField, n.Peer.Name).Info("Removing peer")
 
-		if err := p.BGPInstance.Router.AddNeighbor(ctx, types.NeighborRequest{
-			Peer:       n.Peer,
-			PeerConfig: n.Config,
-			Password:   n.Password,
+		if err := p.BGPInstance.Router.RemoveNeighbor(ctx, types.NeighborRequest{
+			Peer: n.Peer,
 		}); err != nil {
-			return fmt.Errorf("failed to add neigbhor %s in instance %s: %w", n.Peer.Name, p.DesiredConfig.Name, err)
+			return fmt.Errorf("failed to remove neigbhor %s from instance %s: %w", n.Peer.Name, p.DesiredConfig.Name, err)
 		}
 		// update metadata
-		r.upsertMetadata(p.BGPInstance, p.DesiredConfig.Name, n)
+		r.deleteMetadata(p.BGPInstance, p.DesiredConfig.Name, n)
 	}
 
 	// update neighbors
@@ -277,17 +286,19 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, p ReconcileParams) e
 		r.upsertMetadata(p.BGPInstance, p.DesiredConfig.Name, n)
 	}
 
-	// remove neighbors
-	for _, n := range toRemove {
-		l.WithField(types.PeerLogField, n.Peer.Name).Info("Removing peer")
+	// create new neighbors
+	for _, n := range toCreate {
+		l.WithField(types.PeerLogField, n.Peer.Name).Info("Adding peer")
 
-		if err := p.BGPInstance.Router.RemoveNeighbor(ctx, types.NeighborRequest{
-			Peer: n.Peer,
+		if err := p.BGPInstance.Router.AddNeighbor(ctx, types.NeighborRequest{
+			Peer:       n.Peer,
+			PeerConfig: n.Config,
+			Password:   n.Password,
 		}); err != nil {
-			return fmt.Errorf("failed to remove neigbhor %s from instance %s: %w", n.Peer.Name, p.DesiredConfig.Name, err)
+			return fmt.Errorf("failed to add neigbhor %s in instance %s: %w", n.Peer.Name, p.DesiredConfig.Name, err)
 		}
 		// update metadata
-		r.deleteMetadata(p.BGPInstance, p.DesiredConfig.Name, n)
+		r.upsertMetadata(p.BGPInstance, p.DesiredConfig.Name, n)
 	}
 
 	l.Debug("Done reconciling peers")
@@ -375,3 +386,7 @@ func GetPeerAddressFromConfig(conf *v2alpha1.CiliumBGPNodeInstance, peerName str
 	}
 	return netip.Addr{}, fmt.Errorf("peer %s not found in instance %s", peerName, conf.Name)
 }
+
+func (r *NeighborReconciler) neighborID(n *v2alpha1.CiliumBGPNodePeer) string {
+	return fmt.Sprintf("%s%s%d", n.Name, *n.PeerAddress, *n.PeerASN)
+}

pkg/bgpv1/manager/reconcilerv2/neighbor_test.go

@@ -68,6 +68,17 @@ var (
 		},
 	}
 
+	peer2UpdatedASN = func() PeerData {
+		peer2Copy := PeerData{
+			Peer:     peer2.Peer.DeepCopy(),
+			Config:   peer2.Config.DeepCopy(),
+			Password: peer2.Password,
+		}
+
+		peer2Copy.Peer.PeerASN = ptr.To[int64](64125)
+		return peer2Copy
+	}()
+
 	peer2UpdatedTimers = func() PeerData {
 		peer2Copy := PeerData{
 			Peer:     peer2.Peer.DeepCopy(),
@@ -172,6 +183,12 @@ func TestNeighborReconciler(t *testing.T) {
 			newNeighbors: []PeerData{peer1},
 			err:          nil,
 		},
+		{
+			name:         "update config : ASN",
+			neighbors:    []PeerData{peer1, peer2},
+			newNeighbors: []PeerData{peer1, peer2UpdatedASN},
+			err:          nil,
+		},
 		{
 			name:         "update config : timers",
 			neighbors:    []PeerData{peer1, peer2},