make Add/Delete/ListPath APIs symmetric

- AddPath() with an api.Path object then DeletePath() works with the same api.Path object.
- ListPath() returns an api.Path object then DeletePath() works with the same api.Path object.

The above is guaranteed with and without PeerInfo (SourceAsn and SourceId).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

4 files changed, 191 insertions, 82 deletions

diff --git a/pkg/server/grpc_server.go b/pkg/server/grpc_server.go
index 6bc68011..fdbcb3d6 100644
--- a/pkg/server/grpc_server.go
+++ b/pkg/server/grpc_server.go
@@ -239,81 +239,77 @@ func newRoutingPolicyFromApiStruct(arg *api.SetPoliciesRequest) (*config.Routing
 	}, nil
 }
 
-func api2PathList(resource api.Resource, ApiPathList []*api.Path) ([]*table.Path, error) {
+func api2Path(resource api.Resource, path *api.Path, isWithdraw bool) (*table.Path, error) {
 	var pi *table.PeerInfo
+	var nlri bgp.AddrPrefixInterface
+	var nexthop string
 
-	pathList := make([]*table.Path, 0, len(ApiPathList))
-	for _, path := range ApiPathList {
-		var nlri bgp.AddrPrefixInterface
-		var nexthop string
-
-		if path.SourceAsn != 0 {
-			pi = &table.PeerInfo{
-				AS:      path.SourceAsn,
-				LocalID: net.ParseIP(path.SourceId),
-			}
+	if path.SourceAsn != 0 {
+		pi = &table.PeerInfo{
+			AS: path.SourceAsn,
+			ID: net.ParseIP(path.SourceId),
 		}
+	}
 
-		nlri, err := apiutil.GetNativeNlri(path)
-		if err != nil {
-			return nil, err
-		}
-		nlri.SetPathIdentifier(path.Identifier)
-
-		attrList, err := apiutil.GetNativePathAttributes(path)
-		if err != nil {
-			return nil, err
-		}
+	nlri, err := apiutil.GetNativeNlri(path)
+	if err != nil {
+		return nil, err
+	}
+	nlri.SetPathIdentifier(path.Identifier)
 
-		pattrs := make([]bgp.PathAttributeInterface, 0)
-		seen := make(map[bgp.BGPAttrType]struct{})
-		for _, attr := range attrList {
-			attrType := attr.GetType()
-			if _, ok := seen[attrType]; !ok {
-				seen[attrType] = struct{}{}
-			} else {
-				return nil, fmt.Errorf("duplicated path attribute type: %d", attrType)
-			}
+	attrList, err := apiutil.GetNativePathAttributes(path)
+	if err != nil {
+		return nil, err
+	}
 
-			switch a := attr.(type) {
-			case *bgp.PathAttributeNextHop:
-				nexthop = a.Value.String()
-			case *bgp.PathAttributeMpReachNLRI:
-				nlri = a.Value[0]
-				nexthop = a.Nexthop.String()
-			default:
-				pattrs = append(pattrs, attr)
-			}
+	pattrs := make([]bgp.PathAttributeInterface, 0)
+	seen := make(map[bgp.BGPAttrType]struct{})
+	for _, attr := range attrList {
+		attrType := attr.GetType()
+		if _, ok := seen[attrType]; !ok {
+			seen[attrType] = struct{}{}
+		} else {
+			return nil, fmt.Errorf("duplicated path attribute type: %d", attrType)
 		}
 
-		if nlri == nil {
-			return nil, fmt.Errorf("nlri not found")
-		} else if !path.IsWithdraw && nexthop == "" {
-			return nil, fmt.Errorf("nexthop not found")
-		}
-		rf := bgp.AfiSafiToRouteFamily(uint16(path.Family.Afi), uint8(path.Family.Safi))
-		if resource != api.Resource_VRF && rf == bgp.RF_IPv4_UC && net.ParseIP(nexthop).To4() != nil {
-			pattrs = append(pattrs, bgp.NewPathAttributeNextHop(nexthop))
-		} else {
-			pattrs = append(pattrs, bgp.NewPathAttributeMpReachNLRI(nexthop, []bgp.AddrPrefixInterface{nlri}))
+		switch a := attr.(type) {
+		case *bgp.PathAttributeNextHop:
+			nexthop = a.Value.String()
+		case *bgp.PathAttributeMpReachNLRI:
+			nlri = a.Value[0]
+			nexthop = a.Nexthop.String()
+		default:
+			pattrs = append(pattrs, attr)
 		}
+	}
 
-		newPath := table.NewPath(pi, nlri, path.IsWithdraw, pattrs, time.Now(), path.NoImplicitWithdraw)
-		if !path.IsWithdraw {
-			total := bytes.NewBuffer(make([]byte, 0))
-			for _, a := range newPath.GetPathAttrs() {
-				if a.GetType() == bgp.BGP_ATTR_TYPE_MP_REACH_NLRI {
-					continue
-				}
-				b, _ := a.Serialize()
-				total.Write(b)
+	if nlri == nil {
+		return nil, fmt.Errorf("nlri not found")
+	} else if !path.IsWithdraw && nexthop == "" {
+		return nil, fmt.Errorf("nexthop not found")
+	}
+	rf := bgp.AfiSafiToRouteFamily(uint16(path.Family.Afi), uint8(path.Family.Safi))
+	if resource != api.Resource_VRF && rf == bgp.RF_IPv4_UC && net.ParseIP(nexthop).To4() != nil {
+		pattrs = append(pattrs, bgp.NewPathAttributeNextHop(nexthop))
+	} else {
+		pattrs = append(pattrs, bgp.NewPathAttributeMpReachNLRI(nexthop, []bgp.AddrPrefixInterface{nlri}))
+	}
+
+	doWithdraw := (isWithdraw || path.IsWithdraw)
+	newPath := table.NewPath(pi, nlri, doWithdraw, pattrs, time.Now(), path.NoImplicitWithdraw)
+	if !doWithdraw {
+		total := bytes.NewBuffer(make([]byte, 0))
+		for _, a := range newPath.GetPathAttrs() {
+			if a.GetType() == bgp.BGP_ATTR_TYPE_MP_REACH_NLRI {
+				continue
 			}
-			newPath.SetHash(farm.Hash32(total.Bytes()))
+			b, _ := a.Serialize()
+			total.Write(b)
 		}
-		newPath.SetIsFromExternal(path.IsFromExternal)
-		pathList = append(pathList, newPath)
+		newPath.SetHash(farm.Hash32(total.Bytes()))
 	}
-	return pathList, nil
+	newPath.SetIsFromExternal(path.IsFromExternal)
+	return newPath, nil
 }
 
 func (s *server) AddPath(ctx context.Context, r *api.AddPathRequest) (*api.AddPathResponse, error) {
@@ -344,9 +340,13 @@ func (s *server) AddPathStream(stream api.GobgpApi_AddPathStreamServer) error {
 		if arg.Resource != api.Resource_GLOBAL && arg.Resource != api.Resource_VRF {
 			return fmt.Errorf("unsupported resource: %s", arg.Resource)
 		}
-		pathList, err := api2PathList(arg.Resource, arg.Paths)
-		if err != nil {
-			return err
+		pathList := make([]*table.Path, 0, len(arg.Paths))
+		for _, apiPath := range arg.Paths {
+			if path, err := api2Path(arg.Resource, apiPath, apiPath.IsWithdraw); err != nil {
+				return err
+			} else {
+				pathList = append(pathList, path)
+			}
 		}
 		err = s.bgpServer.addPathList(arg.VrfId, pathList)
 		if err != nil {
diff --git a/pkg/server/server.go b/pkg/server/server.go
index f7a09daf..9269bbd4 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -1769,11 +1769,6 @@ func getMacMobilityExtendedCommunity(etag uint32, mac net.HardwareAddr, evpnPath
 }
 
 func (s *BgpServer) fixupApiPath(vrfId string, pathList []*table.Path) error {
-	pi := &table.PeerInfo{
-		AS:      s.bgpConfig.Global.Config.As,
-		LocalID: net.ParseIP(s.bgpConfig.Global.Config.RouterId).To4(),
-	}
-
 	for _, path := range pathList {
 		if !path.IsWithdraw {
 			if _, err := path.GetOrigin(); err != nil {
@@ -1781,10 +1776,6 @@ func (s *BgpServer) fixupApiPath(vrfId string, pathList []*table.Path) error {
 			}
 		}
 
-		if path.GetSource() == nil {
-			path.SetSource(pi)
-		}
-
 		if vrfId != "" {
 			vrf := s.globalRib.Vrfs[vrfId]
 			if vrf == nil {
@@ -1854,15 +1845,14 @@ func (s *BgpServer) addPathList(vrfId string, pathList []*table.Path) error {
 func (s *BgpServer) AddPath(ctx context.Context, r *api.AddPathRequest) (*api.AddPathResponse, error) {
 	var uuidBytes []byte
 	err := s.mgmtOperation(func() error {
-		pathList, err := api2PathList(r.Resource, []*api.Path{r.Path})
+		path, err := api2Path(r.Resource, r.Path, false)
 		if err != nil {
 			return err
 		}
-		err = s.addPathList(r.VrfId, pathList)
+		err = s.addPathList(r.VrfId, []*table.Path{path})
 		if err != nil {
 			return err
 		}
-		path := pathList[0]
 		id, _ := uuid.NewV4()
 		s.uuidMap[id] = pathTokey(path)
 		uuidBytes = id.Bytes()
@@ -1877,8 +1867,8 @@ func (s *BgpServer) DeletePath(ctx context.Context, r *api.DeletePathRequest) er
 
 		pathList, err := func() ([]*table.Path, error) {
 			if r.Path != nil {
-				r.Path.IsWithdraw = true
-				return api2PathList(r.Resource, []*api.Path{r.Path})
+				path, err := api2Path(r.Resource, r.Path, true)
+				return []*table.Path{path}, err
 			}
 			return []*table.Path{}, nil
 		}()
diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go
index 3bd47304..984cf53a 100644
--- a/pkg/server/server_test.go
+++ b/pkg/server/server_test.go
@@ -23,6 +23,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/golang/protobuf/ptypes"
+	"github.com/golang/protobuf/ptypes/any"
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -1050,3 +1052,121 @@ func TestDoNotReactToDuplicateRTCMemberships(t *testing.T) {
 	s1.StopBgp(context.Background(), &api.StopBgpRequest{})
 	s2.StopBgp(context.Background(), &api.StopBgpRequest{})
 }
+
+func TestAddDeletePath(t *testing.T) {
+	ctx := context.Background()
+	s := runNewServer(1, "1.1.1.1", 10179)
+
+	nlri, _ := ptypes.MarshalAny(&api.IPAddressPrefix{
+		Prefix:    "10.0.0.0",
+		PrefixLen: 24,
+	})
+
+	a1, _ := ptypes.MarshalAny(&api.OriginAttribute{
+		Origin: 0,
+	})
+	a2, _ := ptypes.MarshalAny(&api.NextHopAttribute{
+		NextHop: "10.0.0.1",
+	})
+	attrs := []*any.Any{a1, a2}
+
+	family := &api.Family{
+		Afi:  api.Family_AFI_IP,
+		Safi: api.Family_SAFI_UNICAST,
+	}
+
+	listRib := func() []*api.Destination {
+		l := make([]*api.Destination, 0)
+		s.ListPath(ctx, &api.ListPathRequest{Type: api.Resource_GLOBAL, Family: family}, func(d *api.Destination) { l = append(l, d) })
+		return l
+	}
+
+	var err error
+	// DeletePath(AddPath()) without PeerInfo
+	getPath := func() *api.Path {
+		return &api.Path{
+			Family: family,
+			Nlri:   nlri,
+			Pattrs: attrs,
+		}
+	}
+
+	p1 := getPath()
+	_, err = s.AddPath(ctx, &api.AddPathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p1,
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 1)
+	err = s.DeletePath(ctx, &api.DeletePathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p1,
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 0)
+
+	// DeletePath(ListPath()) without PeerInfo
+	_, err = s.AddPath(ctx, &api.AddPathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p1,
+	})
+	assert.Nil(t, err)
+	l := listRib()
+	assert.Equal(t, len(l), 1)
+	err = s.DeletePath(ctx, &api.DeletePathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     l[0].Paths[0],
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 0)
+
+	p2 := getPath()
+	p2.SourceAsn = 1
+	p2.SourceId = "1.1.1.1"
+
+	// DeletePath(AddPath()) with PeerInfo
+	_, err = s.AddPath(ctx, &api.AddPathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p2,
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 1)
+	err = s.DeletePath(ctx, &api.DeletePathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p2,
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 0)
+
+	// DeletePath(ListPath()) with PeerInfo
+	_, err = s.AddPath(ctx, &api.AddPathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p2,
+	})
+	assert.Nil(t, err)
+	l = listRib()
+	assert.Equal(t, len(l), 1)
+	err = s.DeletePath(ctx, &api.DeletePathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     l[0].Paths[0],
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 0)
+
+	// DeletePath(AddPath()) with different PeerInfo
+	_, err = s.AddPath(ctx, &api.AddPathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p2,
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 1)
+	p3 := getPath()
+	p3.SourceAsn = 2
+	p3.SourceId = "1.1.1.2"
+	err = s.DeletePath(ctx, &api.DeletePathRequest{
+		Resource: api.Resource_GLOBAL,
+		Path:     p3,
+	})
+	assert.Nil(t, err)
+	assert.Equal(t, len(listRib()), 1)
+}
diff --git a/pkg/server/zclient.go b/pkg/server/zclient.go
index 33190591..a814e20a 100644
--- a/pkg/server/zclient.go
+++ b/pkg/server/zclient.go
@@ -159,10 +159,9 @@ func newIPRouteBody(dst []*table.Path) (body *zebra.IPRouteBody, isWithdraw bool
 		msgFlags |= zebra.MESSAGE_METRIC
 	}
 	var flags zebra.FLAG
-	info := path.GetSource()
-	if info.AS == info.LocalAS {
+	if path.IsIBGP() {
 		flags = zebra.FLAG_IBGP | zebra.FLAG_INTERNAL
-	} else if info.MultihopTtl > 0 {
+	} else if path.GetSource().MultihopTtl > 0 {
 		flags = zebra.FLAG_INTERNAL
 	}
 	return &zebra.IPRouteBody{