Fix VIPs used as local IPs in TAPA

Due to the implementation of the NSM monitor connections in the TAPA,
the split between local IPs and VIP addresses was not good enough, so
the TAPA was registering itself with some VIP + local IPs.

cmd/proxy/internal/client/fullmesh.go

@@ -126,7 +126,7 @@ func (fmnsc *FullMeshNetworkServiceClient) addNetworkServiceClient(networkServic
 	request.Connection.NetworkServiceEndpointName = networkServiceEndpointName
 	// UUID part at the start of the conn id will be used by NSM to generate the interface name (we want it to be unique)
 	request.Connection.Id = fmt.Sprintf("%s-%s-%s-%s", uuid.New().String(), fmnsc.config.Name, request.Connection.NetworkService, request.Connection.NetworkServiceEndpointName)
-	fmnsc.logger.Info("Add endpoint", "service", networkServiceEndpointName, "id", request.Connection.NetworkService, request.Connection.Id)
+	fmnsc.logger.Info("Add endpoint", "service", networkServiceEndpointName, "NetworkService", request.Connection.NetworkService, "id", request.Connection.Id)
 
 	// Request would try forever, but what if the NetworkServiceEndpoint is removed in the meantime?
 	// The recv() method must not be blocked by a pending Request that might not ever succeed.

pkg/ambassador/tap/conduit/conduit.go

@@ -39,6 +39,7 @@ import (
 	"github.com/nordix/meridio/pkg/log"
 	"github.com/nordix/meridio/pkg/networking"
 	"github.com/nordix/meridio/pkg/retry"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // Conduit implements types.Conduit (/pkg/ambassador/tap/types)
@@ -209,7 +210,11 @@ func (c *Conduit) SetVIPs(ctx context.Context, vips []string) error {
 	if !c.isConnected() {
 		return nil
 	}
-	// TODO: remove VIPs overlapping with internal subnets.
+	// remove VIPs overlapping with internal subnets.
+	vipsInGatewaySubnet := getIPsInGatewaySubnet(vips, c.getGateways())
+	vipsInGatewaySubnetSet := sets.New(vipsInGatewaySubnet...)
+	vipsSet := sets.New(formatPrefixes(vips)...)
+	vips = vipsSet.Difference(vipsInGatewaySubnetSet).UnsortedList()
 	// prepare SrcIpAddrs (IPs allocated by the proxy + VIPs)
 	c.connection.Context.IpContext.SrcIpAddrs = append(c.localIPs, vips...)
 	// prepare the routes (nexthops = proxy bridge IPs)
@@ -271,7 +276,7 @@ func (c *Conduit) SetVIPs(ctx context.Context, vips []string) error {
 		retry.WithDelay(c.RetryDelay))
 	c.logger.Info("VIPs updated", "vips", vips)
 	c.vips = vips
-	c.localIPs = getLocalIPs(c.connection.GetContext().GetIpContext().GetSrcIpAddrs(), vips)
+	c.localIPs = getLocalIPs(c.connection.GetContext().GetIpContext().GetSrcIpAddrs(), vips, c.getGateways())
 	return nil
 }
 
@@ -329,7 +334,7 @@ func (c *Conduit) monitorConnection(ctx context.Context, initialConnection *netw
 					c.mu.Lock()
 					if c.isConnected() {
 						c.connection.Context = connection.GetContext()
-						c.localIPs = getLocalIPs(c.connection.GetContext().GetIpContext().GetSrcIpAddrs(), c.vips)
+						c.localIPs = getLocalIPs(c.connection.GetContext().GetIpContext().GetSrcIpAddrs(), c.vips, c.getGateways())
 					}
 					c.mu.Unlock()
 					break
@@ -342,8 +347,13 @@ func (c *Conduit) monitorConnection(ctx context.Context, initialConnection *netw
 		retry.WithErrorIngnored())
 }
 
-// TODO: verify the IPs are in the same subnet as ExtraPrefixes / DstIpAddrs
-func getLocalIPs(srcIpAddrs []string, vips []string) []string {
+// removes return addresses that are not in the VIP list and that are in one of the gateway subnet
+func getLocalIPs(addresses []string, vips []string, gateways []string) []string {
+	return getIPsInGatewaySubnet(getLocalIPsBasedOnVIP(addresses, vips), gateways)
+}
+
+// remove VIPs from addresses list
+func getLocalIPsBasedOnVIP(addresses []string, vips []string) []string {
 	res := []string{}
 	vipsMap := map[string]struct{}{}
 	for _, vip := range vips {
@@ -354,17 +364,60 @@ func getLocalIPs(srcIpAddrs []string, vips []string) []string {
 		prefixLength, _ := ipNet.Mask.Size()
 		vipsMap[fmt.Sprintf("%s/%d", ip, prefixLength)] = struct{}{}
 	}
-	for _, srcIpAddr := range srcIpAddrs {
-		ip, ipNet, err := net.ParseCIDR(srcIpAddr)
+	for _, address := range addresses {
+		ip, ipNet, err := net.ParseCIDR(address)
 		if err != nil {
 			continue
 		}
 		prefixLength, _ := ipNet.Mask.Size()
-		cidr := fmt.Sprintf("%s/%d", ip, prefixLength) // reformat in case srcIpAddrs have been modified (e.g. IPv6 format)
+		cidr := fmt.Sprintf("%s/%d", ip, prefixLength) // reformat in case address have been modified (e.g. IPv6 format)
 		_, exists := vipsMap[cidr]
 		if !exists {
 			res = append(res, cidr)
 		}
 	}
 	return res
 }
+
+// Remove all addresses that are not in subnet of gateways
+func getIPsInGatewaySubnet(addresses []string, gateways []string) []string {
+	res := []string{}
+	subnets := []*net.IPNet{}
+	for _, prefix := range gateways {
+		_, ipNet, err := net.ParseCIDR(prefix)
+		if err != nil {
+			continue
+		}
+		subnets = append(subnets, ipNet)
+	}
+	for _, prefix := range addresses {
+		ip, ipNet, err := net.ParseCIDR(prefix)
+		if err != nil {
+			continue
+		}
+		prefixLength, _ := ipNet.Mask.Size()
+		for _, subnet := range subnets {
+			subnetLength, _ := subnet.Mask.Size()
+			if subnet.Contains(ip) && prefixLength >= subnetLength {
+				cidr := fmt.Sprintf("%s/%d", ip, prefixLength) // reformat in case srcIpAddrs have been modified (e.g. IPv6 format)
+				res = append(res, cidr)
+				break
+			}
+		}
+	}
+	return res
+}
+
+func formatPrefixes(prefixes []string) []string {
+	res := []string{}
+	for _, prefix := range prefixes {
+		ip, ipNet, err := net.ParseCIDR(prefix)
+		if err != nil {
+			continue
+		}
+		prefixLength, _ := ipNet.Mask.Size()
+		cidr := fmt.Sprintf("%s/%d", ip, prefixLength)
+		res = append(res, cidr)
+	}
+	return res
+}

pkg/ambassador/tap/conduit/conduit_test.go

@@ -253,7 +253,7 @@ func Test_SetVIPs(t *testing.T) {
 	targetName := "abc"
 	namespace := "red"
 	node := "worker"
-	vips := []string{"20.0.0.1/32", "[2000::1]/128"}
+	vips := []string{"20.0.0.1/32", "2000::1/128"}
 	srcIpAddrs := []string{"172.16.0.1/24", "fd00::1/64"}
 	dstIpAddrs := []string{"172.16.0.2/24", "fd00::2/64"}
 	extraPrefixes := []string{"172.16.0.100/24", "fd00::100/64"}
@@ -286,10 +286,10 @@ func Test_SetVIPs(t *testing.T) {
 		assert.NotNil(t, in.GetConnection().GetContext())
 		ipContext := in.GetConnection().GetContext().GetIpContext()
 		assert.NotNil(t, ipContext)
-		assert.Equal(t, dstIpAddrs, ipContext.DstIpAddrs)
-		assert.Equal(t, append(srcIpAddrs, vips...), ipContext.SrcIpAddrs)
+		assert.ElementsMatch(t, dstIpAddrs, ipContext.DstIpAddrs)
+		assert.ElementsMatch(t, append(srcIpAddrs, vips...), ipContext.SrcIpAddrs)
 		assert.Len(t, ipContext.Policies, 2)
-		assert.Equal(t, []*networkservice.PolicyRoute{
+		assert.ElementsMatch(t, []*networkservice.PolicyRoute{
 			{
 				From: "20.0.0.1/32",
 				Routes: []*networkservice.Route{
@@ -300,7 +300,7 @@ func Test_SetVIPs(t *testing.T) {
 				},
 			},
 			{
-				From: "[2000::1]/128",
+				From: "2000::1/128",
 				Routes: []*networkservice.Route{
 					{
 						Prefix:  "::/0",
@@ -365,7 +365,7 @@ func Test_LocalIPs_Switch(t *testing.T) {
 	targetName := "abc"
 	namespace := "red"
 	node := "worker"
-	vips := []string{"20.0.0.1/32", "[2000::1]/128"}
+	vips := []string{"20.0.0.1/32", "2000::1/128"}
 	srcIpAddrs := []string{"172.16.0.1/24", "fd00::1/64"}
 	dstIpAddrs := []string{"172.16.0.2/24", "fd00::2/64"}
 	extraPrefixes := []string{"172.16.0.100/24", "fd00::100/64"}
@@ -402,8 +402,8 @@ func Test_LocalIPs_Switch(t *testing.T) {
 		assert.NotNil(t, in.GetConnection().GetContext())
 		ipContext := in.GetConnection().GetContext().GetIpContext()
 		assert.NotNil(t, ipContext)
-		assert.Equal(t, dstIpAddrs, ipContext.DstIpAddrs)
-		assert.Equal(t, append(srcIpAddrs, vips...), ipContext.SrcIpAddrs)
+		assert.ElementsMatch(t, dstIpAddrs, ipContext.DstIpAddrs)
+		assert.ElementsMatch(t, append(srcIpAddrs, vips...), ipContext.SrcIpAddrs)
 		assert.Len(t, ipContext.Policies, 2)
 		ipContext.DstIpAddrs = dstIpAddrs2
 		ipContext.SrcIpAddrs = srcIpAddrs2