bgpv1: Don't use net package for addressing

We're now discouraging using net package for addressing. Convert all
net.IP and net.IPNet to netip.Addr and netip.Prefix. During the
conversion, I found routerID resolution code is duplicated to two
places. As a part of the cleanup, I consolidated them into the single
function.

Related: #24246

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>

pkg/bgpv1/agent/annotations.go

@@ -6,7 +6,7 @@ package agent
 import (
 	"errors"
 	"math"
-	"net"
+	"net/netip"
 	"strconv"
 	"strings"
 
@@ -165,8 +165,8 @@ func parseAnnotation(key string, value string) (int, Attributes, error) {
 		}
 		switch kv[0] {
 		case "router-id":
-			ip := net.ParseIP(kv[1])
-			if ip.IsUnspecified() {
+			addr, _ := netip.ParseAddr(kv[1])
+			if addr.IsUnspecified() {
 				return 0, out, ErrAttrib{key, kv[0], "could not parse in an IPv4 address"}
 			}
 			out.RouterID = kv[1]

pkg/bgpv1/agent/controller.go

@@ -6,7 +6,7 @@ package agent
 import (
 	"context"
 	"fmt"
-	"net"
+	"net/netip"
 	"time"
 
 	"github.com/sirupsen/logrus"
@@ -15,6 +15,7 @@ import (
 
 	"github.com/cilium/cilium/pkg/hive"
 	"github.com/cilium/cilium/pkg/hive/cell"
+	"github.com/cilium/cilium/pkg/ip"
 	v2alpha1api "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
 	"github.com/cilium/cilium/pkg/k8s/resource"
 	slimlabels "github.com/cilium/cilium/pkg/k8s/slim/k8s/apis/labels"
@@ -86,9 +87,27 @@ type ControlPlaneState struct {
 	// control plane is running on.
 	Annotations AnnotationMap
 	// The current IPv4 address of the agent, reachable externally.
-	IPv4 net.IP
+	IPv4 netip.Addr
 	// The current IPv6 address of the agent, reachable externally.
-	IPv6 net.IP
+	IPv6 netip.Addr
+}
+
+// ResolveRouterID resolves router ID, if we have an annotation and it can be
+// parsed into a valid ipv4 address use it. If not, determine if Cilium is
+// configured with an IPv4 address, if so use it. If neither, return an error,
+// we cannot assign an router ID.
+func (cstate *ControlPlaneState) ResolveRouterID(localASN int) (string, error) {
+	if _, ok := cstate.Annotations[localASN]; ok {
+		if parsed, err := netip.ParseAddr(cstate.Annotations[localASN].RouterID); err == nil && !parsed.IsUnspecified() {
+			return parsed.String(), nil
+		}
+	}
+
+	if !cstate.IPv4.IsUnspecified() {
+		return cstate.IPv4.String(), nil
+	}
+
+	return "", fmt.Errorf("router id not specified by annotation and no IPv4 address assigned by cilium, cannot resolve router id for virtual router with local ASN %v", localASN)
 }
 
 type policyLister interface {
@@ -369,12 +388,15 @@ func (c *Controller) Reconcile(ctx context.Context) error {
 		return fmt.Errorf("failed to retrieve Node's pod CIDR ranges: %w", err)
 	}
 
+	ipv4, _ := ip.AddrFromIP(nodeaddr.GetIPv4())
+	ipv6, _ := ip.AddrFromIP(nodeaddr.GetIPv6())
+
 	// define our current point-in-time control plane state.
 	state := &ControlPlaneState{
 		PodCIDRs:    podCIDRs,
 		Annotations: annoMap,
-		IPv4:        nodeaddr.GetIPv4(),
-		IPv6:        nodeaddr.GetIPv6(),
+		IPv4:        ipv4,
+		IPv6:        ipv6,
 	}
 
 	// call bgp sub-systems required to apply this policy's BGP topology.

pkg/bgpv1/agent/controller_test.go

@@ -6,7 +6,7 @@ package agent_test
 import (
 	"context"
 	"errors"
-	"net"
+	"net/netip"
 	"testing"
 
 	"github.com/cilium/cilium/pkg/bgpv1/agent"
@@ -20,7 +20,7 @@ import (
 var (
 	// the standard node name we'll use throughout our tests.
 	nodeName = "node-under-test-01"
-	nodeIPv4 = net.ParseIP("192.168.0.1")
+	nodeIPv4 = netip.MustParseAddr("192.168.0.1")
 )
 
 // a mock agent.nodeSpecer implementation.
@@ -92,7 +92,7 @@ func TestControllerSanity(t *testing.T) {
 				if p != wantPolicy {
 					t.Fatalf("got: %+v, want: %+v", p, wantPolicy)
 				}
-				if !c.IPv4.Equal(nodeIPv4) {
+				if c.IPv4 != nodeIPv4 {
 					t.Fatalf("got: %v, want: %v", c.IPv4, nodeIPv4)
 				}
 				return nil
@@ -182,7 +182,7 @@ func TestControllerSanity(t *testing.T) {
 	}
 	for _, tt := range table {
 		t.Run(tt.name, func(t *testing.T) {
-			nodeaddr.SetIPv4(nodeIPv4)
+			nodeaddr.SetIPv4(nodeIPv4.AsSlice())
 			nodetypes.SetName(nodeName)
 			nodeSpecer := &fakeNodeSpecer{
 				Annotations_: tt.annotations,
@@ -202,7 +202,7 @@ func TestControllerSanity(t *testing.T) {
 			}
 			err := c.Reconcile(context.Background())
 			if (tt.err == nil) != (err == nil) {
-				t.Fatalf("wanted error: %v", tt.err == nil)
+				t.Fatalf("want: %v, got: %v", tt.err, err)
 			}
 		})
 	}

pkg/bgpv1/gobgp/server.go

@@ -6,7 +6,7 @@ package gobgp
 import (
 	"context"
 	"fmt"
-	"net"
+	"net/netip"
 
 	gobgp "github.com/osrg/gobgp/v3/api"
 	"github.com/osrg/gobgp/v3/pkg/server"
@@ -91,17 +91,16 @@ func NewGoBGPServerWithConfig(ctx context.Context, log *logrus.Entry, params typ
 // AddNeighbor will add the CiliumBGPNeighbor to the gobgp.BgpServer, creating
 // a BGP peering connection.
 func (g *GoBGPServer) AddNeighbor(ctx context.Context, n types.NeighborRequest) error {
-	// cilium neighbor uses CIDR string, gobgp neighbor uses IP string, convert.
-	var ip net.IP
-	var err error
-	if ip, _, err = net.ParseCIDR(n.Neighbor.PeerAddress); err != nil {
+	// cilium neighbor uses prefix string, gobgp neighbor uses IP string, convert.
+	prefix, err := netip.ParsePrefix(n.Neighbor.PeerAddress)
+	if err != nil {
 		// unlikely, we validate this on CR write to k8s api.
 		return fmt.Errorf("failed to parse PeerAddress: %w", err)
 	}
 	peerReq := &gobgp.AddPeerRequest{
 		Peer: &gobgp.Peer{
 			Conf: &gobgp.PeerConf{
-				NeighborAddress: ip.String(),
+				NeighborAddress: prefix.Addr().String(),
 				PeerAsn:         uint32(n.Neighbor.PeerASN),
 			},
 			// tells the peer we are capable of unicast IPv4 and IPv6
@@ -129,15 +128,14 @@ func (g *GoBGPServer) AddNeighbor(ctx context.Context, n types.NeighborRequest)
 // RemoveNeighbor will remove the CiliumBGPNeighbor from the gobgp.BgpServer,
 // disconnecting the BGP peering connection.
 func (g *GoBGPServer) RemoveNeighbor(ctx context.Context, n types.NeighborRequest) error {
-	// cilium neighbor uses CIDR string, gobgp neighbor uses IP string, convert.
-	var ip net.IP
-	var err error
-	if ip, _, err = net.ParseCIDR(n.Neighbor.PeerAddress); err != nil {
+	// cilium neighbor uses prefix string, gobgp neighbor uses IP string, convert.
+	prefix, err := netip.ParsePrefix(n.Neighbor.PeerAddress)
+	if err != nil {
 		// unlikely, we validate this on CR write to k8s api.
 		return fmt.Errorf("failed to parse PeerAddress: %w", err)
 	}
 	peerReq := &gobgp.DeletePeerRequest{
-		Address: ip.String(),
+		Address: prefix.Addr().String(),
 	}
 	if err := g.server.DeletePeer(ctx, peerReq); err != nil {
 		return fmt.Errorf("failed while reconciling neighbor %v %v: %w", n.Neighbor.PeerAddress, n.Neighbor.PeerASN, err)
@@ -163,17 +161,16 @@ func (g *GoBGPServer) AdvertisePath(ctx context.Context, p types.PathRequest) (t
 	var err error
 	var path *gobgp.Path
 	var resp *gobgp.AddPathResponse
-	ip := p.Advert.Net
+	prefix := p.Advert.Prefix
 
 	origin, _ := apb.New(&gobgp.OriginAttribute{
 		Origin: 0,
 	})
 	switch {
-	case ip.IP.To4() != nil:
-		prefixLen, _ := ip.Mask.Size()
+	case prefix.Addr().Is4():
 		nlri, _ := apb.New(&gobgp.IPAddressPrefix{
-			PrefixLen: uint32(prefixLen),
-			Prefix:    ip.IP.String(),
+			PrefixLen: uint32(prefix.Bits()),
+			Prefix:    prefix.Addr().String(),
 		})
 		// Currently, we only support advertising locally originated paths (the paths generated in Cilium
 		// node itself, not the paths received from another BGP Peer or redistributed from another routing
@@ -201,11 +198,10 @@ func (g *GoBGPServer) AdvertisePath(ctx context.Context, p types.PathRequest) (t
 		resp, err = g.server.AddPath(ctx, &gobgp.AddPathRequest{
 			Path: path,
 		})
-	case ip.IP.To16() != nil:
-		prefixLen, _ := ip.Mask.Size()
+	case prefix.Addr().Is6():
 		nlri, _ := apb.New(&gobgp.IPAddressPrefix{
-			PrefixLen: uint32(prefixLen),
-			Prefix:    ip.IP.String(),
+			PrefixLen: uint32(prefix.Bits()),
+			Prefix:    prefix.Addr().String(),
 		})
 		nlriAttrs, _ := apb.New(&gobgp.MpReachNLRIAttribute{ // MP BGP NLRI
 			Family: GoBGPIPv6Family,
@@ -222,14 +218,14 @@ func (g *GoBGPServer) AdvertisePath(ctx context.Context, p types.PathRequest) (t
 			Path: path,
 		})
 	default:
-		return types.PathResponse{}, fmt.Errorf("provided IP returned nil for both IPv4 and IPv6 lengths: %v", len(ip.IP))
+		return types.PathResponse{}, fmt.Errorf("unknown address family for prefix %s", prefix.String())
 	}
 	if err != nil {
 		return types.PathResponse{}, err
 	}
 	return types.PathResponse{
 		Advert: types.Advertisement{
-			Net:           ip,
+			Prefix:        prefix,
 			GoBGPPathUUID: resp.Uuid,
 		},
 	}, err

pkg/bgpv1/manager/advertisements_reconciler.go

@@ -6,7 +6,7 @@ package manager
 import (
 	"context"
 	"fmt"
-	"net"
+	"net/netip"
 
 	"github.com/sirupsen/logrus"
 
@@ -24,12 +24,12 @@ type advertisementsReconcilerParams struct {
 	newc *v2alpha1api.CiliumBGPVirtualRouter
 
 	currentAdvertisements []types.Advertisement
-	toAdvertise           []*net.IPNet
+	toAdvertise           []netip.Prefix
 }
 
 // exportAdvertisementsReconciler reconciles the state of the BGP advertisements
-// with the provided toAdvertise list of IPNets and returns a list of the
-// advertisements currently being announced.
+// with the provided toAdvertise list and returns a list of the advertisements
+// currently being announced.
 func exportAdvertisementsReconciler(params *advertisementsReconcilerParams) ([]types.Advertisement, error) {
 	var (
 		l = log.WithFields(
@@ -55,7 +55,7 @@ func exportAdvertisementsReconciler(params *advertisementsReconcilerParams) ([]t
 		l.Debugf("%s advertisements disabled for virtual router with local ASN %v", params.name, params.newc.LocalASN)
 
 		for _, advrt := range params.currentAdvertisements {
-			l.Debugf("Withdrawing %s advertisement %v for local ASN %v", params.name, advrt.Net.String(), params.newc.LocalASN)
+			l.Debugf("Withdrawing %s advertisement %v for local ASN %v", params.name, advrt.Prefix.String(), params.newc.LocalASN)
 			if err := params.sc.Server.WithdrawPath(params.ctx, types.PathRequest{Advert: advrt}); err != nil {
 				return nil, err
 			}
@@ -74,18 +74,18 @@ func exportAdvertisementsReconciler(params *advertisementsReconcilerParams) ([]t
 	aset := map[string]*member{}
 
 	// populate the advrts that must be present, universe a
-	for _, ipNet := range params.toAdvertise {
+	for _, prefix := range params.toAdvertise {
 		var (
 			m  *member
 			ok bool
 		)
 
-		key := ipNet.String()
+		key := prefix.String()
 		if m, ok = aset[key]; !ok {
 			aset[key] = &member{
 				a: true,
 				advrt: &types.Advertisement{
-					Net: ipNet,
+					Prefix: prefix,
 				},
 			}
 			continue
@@ -99,12 +99,12 @@ func exportAdvertisementsReconciler(params *advertisementsReconcilerParams) ([]t
 			m  *member
 			ok bool
 		)
-		key := advrt.Net.String()
+		key := advrt.Prefix.String()
 		if m, ok = aset[key]; !ok {
 			aset[key] = &member{
 				b: true,
 				advrt: &types.Advertisement{
-					Net:           advrt.Net,
+					Prefix:        advrt.Prefix,
 					GoBGPPathUUID: advrt.GoBGPPathUUID,
 				},
 			}
@@ -138,17 +138,17 @@ func exportAdvertisementsReconciler(params *advertisementsReconcilerParams) ([]t
 
 	// create new adverts
 	for _, advrt := range toAdvertise {
-		l.Debugf("Advertising %s %v for policy with local ASN: %v", params.name, advrt.Net.String(), params.newc.LocalASN)
+		l.Debugf("Advertising %s %v for policy with local ASN: %v", params.name, advrt.Prefix.String(), params.newc.LocalASN)
 		advrtResp, err := params.sc.Server.AdvertisePath(params.ctx, types.PathRequest{Advert: advrt})
 		if err != nil {
-			return nil, fmt.Errorf("failed to advertise %s prefix %v: %w", params.name, advrt.Net, err)
+			return nil, fmt.Errorf("failed to advertise %s prefix %v: %w", params.name, advrt.Prefix, err)
 		}
 		newAdverts = append(newAdverts, advrtResp.Advert)
 	}
 
 	// withdraw uneeded adverts
 	for _, advrt := range toWithdraw {
-		l.Debugf("Withdrawing %s %v for policy with local ASN: %v", params.name, advrt.Net, params.newc.LocalASN)
+		l.Debugf("Withdrawing %s %v for policy with local ASN: %v", params.name, advrt.Prefix, params.newc.LocalASN)
 		if err := params.sc.Server.WithdrawPath(params.ctx, types.PathRequest{Advert: advrt}); err != nil {
 			return nil, err
 		}

pkg/bgpv1/manager/manager.go

@@ -6,7 +6,6 @@ package manager
 import (
 	"context"
 	"fmt"
-	"net"
 	"sort"
 
 	"github.com/cilium/cilium/api/v1/models"
@@ -211,22 +210,9 @@ func (m *BGPRouterManager) registerBGPServer(ctx context.Context, c *v2alpha1api
 		}
 	}
 
-	// resolve router ID, if we have an annotation and it can be parsed into
-	// a valid ipv4 address use this,
-	//
-	// if not determine if Cilium is configured with an IPv4 address, if so use
-	// this.
-	//
-	// if neither, return an error, we cannot assign an router ID.
-	var routerID string
-	_, ok := cstate.Annotations[c.LocalASN]
-	switch {
-	case ok && !net.ParseIP(cstate.Annotations[c.LocalASN].RouterID).IsUnspecified():
-		routerID = cstate.Annotations[c.LocalASN].RouterID
-	case !cstate.IPv4.IsUnspecified():
-		routerID = cstate.IPv4.String()
-	default:
-		return fmt.Errorf("router id not specified by annotation and no IPv4 address assigned by cilium, cannot resolve router id for virtual router with local ASN %v", c.LocalASN)
+	routerID, err := cstate.ResolveRouterID(c.LocalASN)
+	if err != nil {
+		return err
 	}
 
 	globalConfig := types.ServerParameters{