Fix for Policy Base Routing failure; Possible table ID collision.

Related issue: networkservicemesh/deployments-k8s/issues/9119

Added a mechanism to protect table ID selection from parallel runs.

Signed-off-by: Laszlo Kiraly <[email protected]>

pkg/kernel/networkservice/connectioncontextkernel/ipcontext/iprule/common.go

@@ -2,6 +2,8 @@
 //
 // Copyright (c) 2023 Cisco and/or its affiliates.
 //
+// Copyright (c) 2023 Nordix Foundation.
+//
 // SPDX-License-Identifier: Apache-2.0
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -40,7 +42,7 @@ import (
 	link "github.com/networkservicemesh/sdk-kernel/pkg/kernel"
 )
 
-func create(ctx context.Context, conn *networkservice.Connection, tableIDs *Map) error {
+func create(ctx context.Context, conn *networkservice.Connection, tableIDs *Map, nsRTableNextIDToConnID *NetnsRTableNextIDToConnMap) error {
 	if mechanism := kernel.ToMechanism(conn.GetMechanism()); mechanism != nil && mechanism.GetVLAN() == 0 {
 		// Construct the netlink handle for the target namespace for this kernel interface
 		netlinkHandle, err := link.GetNetlinkHandle(mechanism.GetNetNSURL())
@@ -54,52 +56,73 @@ func create(ctx context.Context, conn *networkservice.Connection, tableIDs *Map)
 		if err != nil {
 			return errors.Wrapf(err, "failed to find link %s", ifName)
 		}
-
-		ps, ok := tableIDs.Load(conn.GetId())
+		connID := conn.GetId()
+		ps, ok := tableIDs.Load(connID)
 		if !ok {
 			if len(conn.Context.IpContext.Policies) == 0 {
 				return nil
 			}
 			ps = make(map[int]*networkservice.PolicyRoute)
-			tableIDs.Store(conn.GetId(), ps)
+			tableIDs.Store(connID, ps)
 		}
 		// Get policies to add and to remove
 		toAdd, toRemove := getPolicyDifferences(ps, conn.Context.IpContext.Policies)
 
 		// Remove no longer existing policies
 		for tableID, policy := range toRemove {
-			if err := delRule(ctx, netlinkHandle, policy, tableID); err != nil {
-				return err
+			if errRule := delRule(ctx, netlinkHandle, policy, tableID); errRule != nil {
+				return errRule
 			}
 			delete(ps, tableID)
-			tableIDs.Store(conn.GetId(), ps)
+			tableIDs.Store(connID, ps)
 		}
 		// Add new policies
+		netNSInode := mechanism.GetNetNSInode()
 		for _, policy := range toAdd {
-			tableID, err := getFreeTableID(ctx, netlinkHandle)
-			if err != nil {
-				return err
-			}
-			// If policy doesn't contain any route - add default
-			if len(policy.Routes) == 0 {
-				policy.Routes = append(policy.Routes, defaultRoute())
-			}
-
-			for _, route := range policy.Routes {
-				if err := routeAdd(ctx, netlinkHandle, l, route, tableID); err != nil {
+			var tableID int
+			var nsrtid *NetnsRTableNextID
+			// get a free table ID until we succeed
+			for {
+				tableID, err = getFreeTableID(ctx, netlinkHandle)
+				if err != nil {
 					return err
 				}
+				nsrtid = NewNetnsRTableNextID(netNSInode, tableID)
+				storedConnID, _ := nsRTableNextIDToConnID.LoadOrStore(*nsrtid, connID)
+				if connID == storedConnID {
+					// No other connection adding policy using this free routing table ID
+					break
+				}
 			}
-			if err := ruleAdd(ctx, netlinkHandle, policy, tableID); err != nil {
+			if err := addPolicy(ctx, netlinkHandle, policy, l, ps, tableIDs, tableID, connID, *nsrtid, nsRTableNextIDToConnID); err != nil {
 				return err
 			}
-			ps[tableID] = policy
-			tableIDs.Store(conn.GetId(), ps)
 		}
 	}
 	return nil
 }
 
+func addPolicy(ctx context.Context, netlinkHandle *netlink.Handle, policy *networkservice.PolicyRoute, l netlink.Link, ps policies, tableIDs *Map, tableID int, connID string, nsrtid NetnsRTableNextID, nsRTableNextIDToConnID *NetnsRTableNextIDToConnMap) error {
+	// If policy doesn't contain any route - add default
+	if len(policy.Routes) == 0 {
+		policy.Routes = append(policy.Routes, defaultRoute())
+	}
+
+	for _, route := range policy.Routes {
+		if err := routeAdd(ctx, netlinkHandle, l, route, tableID); err != nil {
+			return err
+		}
+	}
+	if err := ruleAdd(ctx, netlinkHandle, policy, tableID); err != nil {
+		return err
+	}
+	ps[tableID] = policy
+	tableIDs.Store(connID, ps)
+	// release the lock
+	nsRTableNextIDToConnID.Delete(nsrtid)
+	return nil
+}
+
 func getPolicyDifferences(current map[int]*networkservice.PolicyRoute, newPolicies []*networkservice.PolicyRoute) (toAdd []*networkservice.PolicyRoute, toRemove map[int]*networkservice.PolicyRoute) {
 	type table struct {
 		tableID     int
@@ -242,7 +265,7 @@ func routeAdd(ctx context.Context, handle *netlink.Handle, l netlink.Link, route
 	return nil
 }
 
-func del(ctx context.Context, conn *networkservice.Connection, tableIDs *Map) error {
+func del(ctx context.Context, conn *networkservice.Connection, tableIDs *Map, nsRTableNextIDToConnID *NetnsRTableNextIDToConnMap) error {
 	if mechanism := kernel.ToMechanism(conn.GetMechanism()); mechanism != nil && mechanism.GetVLAN() == 0 {
 		netlinkHandle, err := link.GetNetlinkHandle(mechanism.GetNetNSURL())
 		if err != nil {
@@ -251,6 +274,11 @@ func del(ctx context.Context, conn *networkservice.Connection, tableIDs *Map) er
 		defer netlinkHandle.Close()
 		ps, ok := tableIDs.LoadAndDelete(conn.GetId())
 		if ok {
+			netNSInode := mechanism.GetNetNSInode()
+			for tableID := range ps {
+				nsrtid := NewNetnsRTableNextID(netNSInode, tableID)
+				nsRTableNextIDToConnID.Delete(*nsrtid)
+			}
 			for tableID, policy := range ps {
 				if err := delRule(ctx, netlinkHandle, policy, tableID); err != nil {
 					return err
@@ -338,7 +366,6 @@ func getFreeTableID(ctx context.Context, handle *netlink.Handle) (int, error) {
 			break
 		}
 	}
-
 	log.FromContext(ctx).
 		WithField("tableID", tableID).
 		WithField("netlink", "getFreeTableID").Debug("completed")

pkg/kernel/networkservice/connectioncontextkernel/ipcontext/iprule/gen.go

@@ -1,5 +1,7 @@
 // Copyright (c) 2022 Doc.ai and/or its affiliates.
 //
+// Copyright (c) 2023 Nordix Foundation.
+//
 // SPDX-License-Identifier: Apache-2.0
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -23,8 +25,12 @@ import (
 )
 
 //go:generate go-syncmap -output table_map.gen.go -type Map<string,policies>
+//go:generate go-syncmap -output netns_rtable_conn_map.gen.go -type NetnsRTableNextIDToConnMap<NetnsRTableNextID,string>
 
 type policies map[int]*networkservice.PolicyRoute
 
 // Map - sync.Map with key == string (connID) and value == policies
 type Map sync.Map
+
+// NetnsRTableNextIDToConnMap - sync.Map with key NetnsRTableNextID value of string (connID)
+type NetnsRTableNextIDToConnMap sync.Map

pkg/kernel/networkservice/connectioncontextkernel/ipcontext/iprule/nsrtnextid.go

@@ -0,0 +1,31 @@
+// Copyright (c) 2023 Nordix and/or its affiliates.
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at:
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package iprule
+
+// NetnsRTableNextID stores Network namespace and Next Routing Table ID
+type NetnsRTableNextID struct {
+	ns    string
+	nrtid int
+}
+
+// NewNetnsRTableNextID returns *NetnsRTableNextID entry
+func NewNetnsRTableNextID(ns string, nrtid int) *NetnsRTableNextID {
+	return &NetnsRTableNextID{
+		ns:    ns,
+		nrtid: nrtid,
+	}
+}

pkg/kernel/networkservice/connectioncontextkernel/ipcontext/iprule/server.go

@@ -2,7 +2,7 @@
 //
 // Copyright (c) 2022 Doc.ai and/or its affiliates.
 //
-// Copyright (c) 2021-2022 Nordix Foundation.
+// Copyright (c) 2023 Nordix Foundation.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -35,6 +35,10 @@ import (
 
 type ipruleServer struct {
 	tables Map
+	// Protecting route and rule setting with this sync.Map
+	// The next table ID is calculated based on a dump
+	// other connection from same client can add new table in parallel
+	nsRTableNextIDToConnID NetnsRTableNextIDToConnMap
 }
 
 // NewServer creates a new server chain element setting ip rules
@@ -55,7 +59,7 @@ func (i *ipruleServer) Request(ctx context.Context, request *networkservice.Netw
 		return nil, err
 	}
 
-	if err := create(ctx, conn, &i.tables); err != nil {
+	if err := create(ctx, conn, &i.tables, &i.nsRTableNextIDToConnID); err != nil {
 		closeCtx, cancelClose := postponeCtxFunc()
 		defer cancelClose()
 
@@ -70,6 +74,6 @@ func (i *ipruleServer) Request(ctx context.Context, request *networkservice.Netw
 }
 
 func (i *ipruleServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
-	_ = del(ctx, conn, &i.tables)
+	_ = del(ctx, conn, &i.tables, &i.nsRTableNextIDToConnID)
 	return next.Server(ctx).Close(ctx, conn)
 }