Merge pull request #98 from bio-routing/fix/accept_did_not_terminate

Bugfix in Filter Action processing

routingtable/filter/BUILD.bazel

@@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
     name = "go_default_library",
     srcs = [
+        "action.go",
         "community_filter.go",
         "filter.go",
         "helper.go",

routingtable/filter/action.go

+package filter
+
+import (
+	"github.com/bio-routing/bio-rd/net"
+	"github.com/bio-routing/bio-rd/route"
+	"github.com/bio-routing/bio-rd/routingtable/filter/actions"
+)
+
+// Action performs actions on a `route.Path`
+type Action interface {
+	Do(p net.Prefix, pa *route.Path) actions.Result
+}

routingtable/filter/actions/BUILD.bazel

@@ -4,10 +4,10 @@ go_library(
     name = "go_default_library",
     srcs = [
         "accept_action.go",
+        "action.go",
         "add_community_action.go",
         "add_large_community_action.go",
         "as_path_prepend_action.go",
-        "filter_action.go",
         "reject_action.go",
         "set_local_pref_action.go",
         "set_nexthop_action.go",

routingtable/filter/actions/accept_action.go

@@ -8,6 +8,9 @@ import (
 type AcceptAction struct {
 }
 
-func (*AcceptAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
-	return pa, false
+func (*AcceptAction) Do(p net.Prefix, pa *route.Path) Result {
+	return Result{
+		Path:      pa,
+		Terminate: true,
+	}
 }

routingtable/filter/actions/filter_action.go → routingtable/filter/actions/action.go

 package actions
 
 import (
-	"github.com/bio-routing/bio-rd/net"
 	"github.com/bio-routing/bio-rd/route"
 )
 
-type FilterAction interface {
-	Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool)
+type Result struct {
+	Path      *route.Path
+	Reject    bool
+	Terminate bool
 }

routingtable/filter/actions/add_community_action.go

@@ -15,9 +15,9 @@ func NewAddCommunityAction(coms []uint32) *AddCommunityAction {
 	}
 }
 
-func (a *AddCommunityAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
+func (a *AddCommunityAction) Do(p net.Prefix, pa *route.Path) Result {
 	if pa.BGPPath == nil || len(a.communities) == 0 {
-		return pa, false
+		return Result{Path: pa}
 	}
 
 	modified := pa.Copy()
@@ -26,5 +26,5 @@ func (a *AddCommunityAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Pa
 		modified.BGPPath.Communities = append(modified.BGPPath.Communities, com)
 	}
 
-	return modified, false
+	return Result{Path: modified}
 }

routingtable/filter/actions/add_community_action_test.go

@@ -53,9 +53,9 @@ func TestAddingCommunities(t *testing.T) {
 			}
 
 			a := NewAddCommunityAction(test.communities)
-			modPath, _ := a.Do(net.Prefix{}, p)
+			res := a.Do(net.Prefix{}, p)
 
-			assert.Equal(t, test.expected, modPath.BGPPath.CommunitiesString())
+			assert.Equal(t, test.expected, res.Path.BGPPath.CommunitiesString())
 		})
 	}
 }

routingtable/filter/actions/add_large_community_action.go

@@ -16,13 +16,13 @@ func NewAddLargeCommunityAction(coms []types.LargeCommunity) *AddLargeCommunityA
 	}
 }
 
-func (a *AddLargeCommunityAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
+func (a *AddLargeCommunityAction) Do(p net.Prefix, pa *route.Path) Result {
 	if pa.BGPPath == nil || len(a.communities) == 0 {
-		return pa, false
+		return Result{Path: pa}
 	}
 
 	modified := pa.Copy()
 	modified.BGPPath.LargeCommunities = append(modified.BGPPath.LargeCommunities, a.communities...)
 
-	return modified, false
+	return Result{Path: modified}
 }

routingtable/filter/actions/add_large_community_action_test.go

@@ -71,7 +71,7 @@ func TestAddingLargeCommunities(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		t.Run(test.name, func(te *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			p := &route.Path{
 				BGPPath: &route.BGPPath{
 					LargeCommunities: test.current,
@@ -79,9 +79,9 @@ func TestAddingLargeCommunities(t *testing.T) {
 			}
 
 			a := NewAddLargeCommunityAction(test.communities)
-			modPath, _ := a.Do(net.Prefix{}, p)
+			res := a.Do(net.Prefix{}, p)
 
-			assert.Equal(te, test.expected, modPath.BGPPath.LargeCommunitiesString())
+			assert.Equal(t, test.expected, res.Path.BGPPath.LargeCommunitiesString())
 		})
 	}
 }

routingtable/filter/actions/as_path_prepend_action.go

@@ -17,13 +17,13 @@ func NewASPathPrependAction(asn uint32, times uint16) *ASPathPrependAction {
 	}
 }
 
-func (a *ASPathPrependAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
+func (a *ASPathPrependAction) Do(p net.Prefix, pa *route.Path) Result {
 	if pa.BGPPath == nil {
-		return pa, false
+		return Result{Path: pa}
 	}
 
 	modified := pa.Copy()
 	modified.BGPPath.Prepend(a.asn, a.times)
 
-	return modified, false
+	return Result{Path: modified}
 }

routingtable/filter/actions/as_path_prepend_action_test.go

@@ -54,9 +54,9 @@ func TestAppendPath(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		t.Run(test.name, func(te *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			a := NewASPathPrependAction(12345, test.times)
-			p, _ := a.Do(bnet.NewPfx(bnet.IPv4FromOctets(10, 0, 0, 0), 8), &route.Path{
+			res := a.Do(bnet.NewPfx(bnet.IPv4FromOctets(10, 0, 0, 0), 8), &route.Path{
 				BGPPath: test.bgpPath,
 			})
 
@@ -64,8 +64,8 @@ func TestAppendPath(t *testing.T) {
 				return
 			}
 
-			assert.Equal(te, test.expectedPath, p.BGPPath.ASPath.String(), "ASPath")
-			assert.Equal(te, test.expectedLength, p.BGPPath.ASPathLen, "ASPathLen")
+			assert.Equal(t, test.expectedPath, res.Path.BGPPath.ASPath.String(), "ASPath")
+			assert.Equal(t, test.expectedLength, res.Path.BGPPath.ASPathLen, "ASPathLen")
 		})
 	}
 }

routingtable/filter/actions/reject_action.go

@@ -8,6 +8,10 @@ import (
 type RejectAction struct {
 }
 
-func (*RejectAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
-	return pa, true
+func (*RejectAction) Do(p net.Prefix, pa *route.Path) Result {
+	return Result{
+		Path:      pa,
+		Reject:    true,
+		Terminate: true,
+	}
 }

routingtable/filter/actions/set_local_pref_action.go

@@ -15,13 +15,13 @@ func NewSetLocalPrefAction(pref uint32) *SetLocalPrefAction {
 	}
 }
 
-func (a *SetLocalPrefAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
+func (a *SetLocalPrefAction) Do(p net.Prefix, pa *route.Path) Result {
 	if pa.BGPPath == nil {
-		return pa, false
+		return Result{Path: pa}
 	}
 
 	modified := *pa
 	modified.BGPPath.LocalPref = a.pref
 
-	return &modified, false
+	return Result{Path: &modified}
 }

routingtable/filter/actions/set_local_pref_action_test.go

@@ -27,14 +27,14 @@ func TestSetLocalPref(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		t.Run(test.name, func(te *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			a := NewSetLocalPrefAction(150)
-			p, _ := a.Do(bnet.NewPfx(bnet.IPv4FromOctets(10, 0, 0, 0), 8), &route.Path{
+			res := a.Do(bnet.NewPfx(bnet.IPv4FromOctets(10, 0, 0, 0), 8), &route.Path{
 				BGPPath: test.bgpPath,
 			})
 
 			if test.expectedLocalPref > 0 {
-				assert.Equal(te, test.expectedLocalPref, p.BGPPath.LocalPref)
+				assert.Equal(t, test.expectedLocalPref, res.Path.BGPPath.LocalPref)
 			}
 		})
 	}

routingtable/filter/actions/set_nexthop_action.go

@@ -16,13 +16,13 @@ func NewSetNextHopAction(ip bnet.IP) *SetNextHopAction {
 	}
 }
 
-func (a *SetNextHopAction) Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
+func (a *SetNextHopAction) Do(p net.Prefix, pa *route.Path) Result {
 	if pa.BGPPath == nil {
-		return pa, false
+		return Result{Path: pa}
 	}
 
 	modified := pa.Copy()
 	modified.BGPPath.NextHop = a.ip
 
-	return modified, false
+	return Result{Path: modified}
 }

routingtable/filter/actions/set_nexthop_action_test.go

@@ -31,12 +31,12 @@ func TestSetNextHopTest(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			a := NewSetNextHopAction(bnet.IPv4FromOctets(100, 64, 2, 1))
-			p, _ := a.Do(net.NewPfx(bnet.IPv4FromOctets(10, 0, 0, 0), 8), &route.Path{
+			res := a.Do(net.NewPfx(bnet.IPv4FromOctets(10, 0, 0, 0), 8), &route.Path{
 				BGPPath: test.bgpPath,
 			})
 
 			if test.bgpPath != nil {
-				assert.Equal(t, test.expected, p.BGPPath.NextHop)
+				assert.Equal(t, test.expected, res.Path.BGPPath.NextHop)
 			}
 		})
 	}

routingtable/filter/filter.go

@@ -21,10 +21,11 @@ func (f *Filter) ProcessTerms(p net.Prefix, pa *route.Path) (modPath *route.Path
 	modPath = pa
 
 	for _, t := range f.terms {
-		modPath, reject = t.Process(p, modPath)
-		if reject {
-			return modPath, true
+		res := t.Process(p, modPath)
+		if res.Terminate {
+			return res.Path, res.Reject
 		}
+		modPath = res.Path
 	}
 
 	return modPath, false

routingtable/filter/filter_test.go

@@ -23,7 +23,7 @@ func TestProcessTerms(t *testing.T) {
 			prefix: net.NewPfx(net.IPv4(0), 0),
 			path:   &route.Path{},
 			term: &Term{
-				then: []FilterAction{
+				then: []Action{
 					&actions.AcceptAction{},
 				},
 			},
@@ -35,19 +35,32 @@ func TestProcessTerms(t *testing.T) {
 			prefix: net.NewPfx(net.IPv4(0), 0),
 			path:   &route.Path{},
 			term: &Term{
-				then: []FilterAction{
+				then: []Action{
 					&actions.RejectAction{},
 				},
 			},
 			exptectAccept:  false,
 			expectModified: false,
 		},
+		{
+			name:   "accept before reject",
+			prefix: net.NewPfx(net.IPv4(0), 0),
+			path:   &route.Path{},
+			term: &Term{
+				then: []Action{
+					&actions.AcceptAction{},
+					&actions.RejectAction{},
+				},
+			},
+			exptectAccept:  true,
+			expectModified: false,
+		},
 		{
 			name:   "modified",
 			prefix: net.NewPfx(net.IPv4(0), 0),
 			path:   &route.Path{},
 			term: &Term{
-				then: []FilterAction{
+				then: []Action{
 					&mockAction{},
 					&actions.AcceptAction{},
 				},
@@ -58,7 +71,7 @@ func TestProcessTerms(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		t.Run(test.name, func(te *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			f := NewFilter([]*Term{test.term})
 			p, reject := f.ProcessTerms(test.prefix, test.path)
 

routingtable/filter/helper.go

@@ -10,7 +10,7 @@ func NewAcceptAllFilter() *Filter {
 		[]*Term{
 			NewTerm(
 				[]*TermCondition{},
-				[]FilterAction{
+				[]Action{
 					&actions.AcceptAction{},
 				}),
 		})
@@ -22,7 +22,7 @@ func NewDrainFilter() *Filter {
 		[]*Term{
 			NewTerm(
 				[]*TermCondition{},
-				[]FilterAction{
+				[]Action{
 					&actions.RejectAction{},
 				}),
 		})

routingtable/filter/term.go

@@ -5,18 +5,20 @@ import (
 	"github.com/bio-routing/bio-rd/route"
 )
 
-type FilterAction interface {
-	Do(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool)
-}
-
 // Term matches a path against a list of conditions and performs actions if it matches
 type Term struct {
 	from []*TermCondition
-	then []FilterAction
+	then []Action
+}
+
+type TermResult struct {
+	Path      *route.Path
+	Terminate bool
+	Reject    bool
 }
 
 // NewTerm creates a new term
-func NewTerm(from []*TermCondition, then []FilterAction) *Term {
+func NewTerm(from []*TermCondition, then []Action) *Term {
 	t := &Term{
 		from: from,
 		then: then,
@@ -26,7 +28,7 @@ func NewTerm(from []*TermCondition, then []FilterAction) *Term {
 }
 
 // Process processes a path returning if the path should be rejected and returns a possible modified version of the path
-func (t *Term) Process(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
+func (t *Term) Process(p net.Prefix, pa *route.Path) TermResult {
 	orig := pa
 
 	if len(t.from) == 0 {
@@ -39,18 +41,23 @@ func (t *Term) Process(p net.Prefix, pa *route.Path) (modPath *route.Path, rejec
 		}
 	}
 
-	return orig, false
+	return TermResult{Path: orig}
 }
 
-func (t *Term) processActions(p net.Prefix, pa *route.Path) (modPath *route.Path, reject bool) {
-	modPath = pa
+func (t *Term) processActions(p net.Prefix, pa *route.Path) TermResult {
+	modPath := pa
 
 	for _, action := range t.then {
-		modPath, reject = action.Do(p, modPath)
-		if reject {
-			continue
+		res := action.Do(p, modPath)
+		if res.Terminate {
+			return TermResult{
+				Path:      modPath,
+				Terminate: true,
+				Reject:    res.Reject,
+			}
 		}
+		modPath = res.Path
 	}
 
-	return modPath, reject
+	return TermResult{Path: modPath}
 }