From 6d5dbf0b96fde197f5492341a202d21a4ac452cb Mon Sep 17 00:00:00 2001
From: Daniel Czerwonk <daniel@dan-nrw.de>
Date: Sun, 15 Jul 2018 14:37:30 +0200
Subject: [PATCH] fixed bug in term processing
---
routingtable/filter/BUILD.bazel | 1 +
routingtable/filter/action.go | 12 +++++++
routingtable/filter/actions/BUILD.bazel | 2 +-
routingtable/filter/actions/accept_action.go | 7 ++--
routingtable/filter/actions/action.go | 11 ++++++
.../filter/actions/add_community_action.go | 6 ++--
.../actions/add_community_action_test.go | 4 +--
.../actions/add_large_community_action.go | 6 ++--
.../add_large_community_action_test.go | 6 ++--
.../filter/actions/as_path_prepend_action.go | 6 ++--
.../actions/as_path_prepend_action_test.go | 8 ++---
routingtable/filter/actions/filter_action.go | 10 ------
routingtable/filter/actions/reject_action.go | 8 +++--
.../filter/actions/set_local_pref_action.go | 6 ++--
.../actions/set_local_pref_action_test.go | 6 ++--
.../filter/actions/set_nexthop_action.go | 6 ++--
.../filter/actions/set_nexthop_action_test.go | 4 +--
routingtable/filter/filter.go | 7 ++--
routingtable/filter/filter_test.go | 21 ++++++++---
routingtable/filter/helper.go | 4 +--
routingtable/filter/term.go | 35 +++++++++++--------
routingtable/filter/term_test.go | 32 ++++++++---------
22 files changed, 125 insertions(+), 83 deletions(-)
create mode 100644 routingtable/filter/action.go
create mode 100644 routingtable/filter/actions/action.go
delete mode 100644 routingtable/filter/actions/filter_action.go
diff --git a/routingtable/filter/BUILD.bazel b/routingtable/filter/BUILD.bazel
index 4ac13572..17bee85a 100644
--- a/routingtable/filter/BUILD.bazel
+++ b/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",
diff --git a/routingtable/filter/action.go b/routingtable/filter/action.go
new file mode 100644
index 00000000..888ad3f5
--- /dev/null
+++ b/routingtable/filter/action.go
@@ -0,0 +1,12 @@
+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
+}
diff --git a/routingtable/filter/actions/BUILD.bazel b/routingtable/filter/actions/BUILD.bazel
index b4195842..d974ffd1 100644
--- a/routingtable/filter/actions/BUILD.bazel
+++ b/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",
diff --git a/routingtable/filter/actions/accept_action.go b/routingtable/filter/actions/accept_action.go
index 6c0cc2ac..6e340d53 100644
--- a/routingtable/filter/actions/accept_action.go
+++ b/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,
+ }
}
diff --git a/routingtable/filter/actions/action.go b/routingtable/filter/actions/action.go
new file mode 100644
index 00000000..3cd6f0ac
--- /dev/null
+++ b/routingtable/filter/actions/action.go
@@ -0,0 +1,11 @@
+package actions
+
+import (
+ "github.com/bio-routing/bio-rd/route"
+)
+
+type Result struct {
+ Path *route.Path
+ Reject bool
+ Terminate bool
+}
diff --git a/routingtable/filter/actions/add_community_action.go b/routingtable/filter/actions/add_community_action.go
index c24107e8..4931056a 100644
--- a/routingtable/filter/actions/add_community_action.go
+++ b/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}
}
diff --git a/routingtable/filter/actions/add_community_action_test.go b/routingtable/filter/actions/add_community_action_test.go
index 9ec3d140..3527437b 100644
--- a/routingtable/filter/actions/add_community_action_test.go
+++ b/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())
})
}
}
diff --git a/routingtable/filter/actions/add_large_community_action.go b/routingtable/filter/actions/add_large_community_action.go
index cb5f1a02..9e0a9da4 100644
--- a/routingtable/filter/actions/add_large_community_action.go
+++ b/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}
}
diff --git a/routingtable/filter/actions/add_large_community_action_test.go b/routingtable/filter/actions/add_large_community_action_test.go
index 6b569d2e..fe077817 100644
--- a/routingtable/filter/actions/add_large_community_action_test.go
+++ b/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())
})
}
}
diff --git a/routingtable/filter/actions/as_path_prepend_action.go b/routingtable/filter/actions/as_path_prepend_action.go
index 30d63ca7..2e53b1e8 100644
--- a/routingtable/filter/actions/as_path_prepend_action.go
+++ b/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}
}
diff --git a/routingtable/filter/actions/as_path_prepend_action_test.go b/routingtable/filter/actions/as_path_prepend_action_test.go
index f7c7af59..12cc0267 100644
--- a/routingtable/filter/actions/as_path_prepend_action_test.go
+++ b/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")
})
}
}
diff --git a/routingtable/filter/actions/filter_action.go b/routingtable/filter/actions/filter_action.go
deleted file mode 100644
index 7a266514..00000000
--- a/routingtable/filter/actions/filter_action.go
+++ /dev/null
@@ -1,10 +0,0 @@
-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)
-}
diff --git a/routingtable/filter/actions/reject_action.go b/routingtable/filter/actions/reject_action.go
index d7401396..4693bbfa 100644
--- a/routingtable/filter/actions/reject_action.go
+++ b/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,
+ }
}
diff --git a/routingtable/filter/actions/set_local_pref_action.go b/routingtable/filter/actions/set_local_pref_action.go
index e60b6faa..26d038ec 100644
--- a/routingtable/filter/actions/set_local_pref_action.go
+++ b/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}
}
diff --git a/routingtable/filter/actions/set_local_pref_action_test.go b/routingtable/filter/actions/set_local_pref_action_test.go
index 7b104d39..40ef0f6b 100644
--- a/routingtable/filter/actions/set_local_pref_action_test.go
+++ b/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)
}
})
}
diff --git a/routingtable/filter/actions/set_nexthop_action.go b/routingtable/filter/actions/set_nexthop_action.go
index de1e9bc9..8b00a871 100644
--- a/routingtable/filter/actions/set_nexthop_action.go
+++ b/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}
}
diff --git a/routingtable/filter/actions/set_nexthop_action_test.go b/routingtable/filter/actions/set_nexthop_action_test.go
index 7f691d23..bebbafeb 100644
--- a/routingtable/filter/actions/set_nexthop_action_test.go
+++ b/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)
}
})
}
diff --git a/routingtable/filter/filter.go b/routingtable/filter/filter.go
index cb880879..27f3b1f4 100644
--- a/routingtable/filter/filter.go
+++ b/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
diff --git a/routingtable/filter/filter_test.go b/routingtable/filter/filter_test.go
index 52ebc8b9..0eeb4942 100644
--- a/routingtable/filter/filter_test.go
+++ b/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)
diff --git a/routingtable/filter/helper.go b/routingtable/filter/helper.go
index 17a7d737..1cfb6e5c 100644
--- a/routingtable/filter/helper.go
+++ b/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{},
}),
})
diff --git a/routingtable/filter/term.go b/routingtable/filter/term.go
index d644acba..cdf56c7f 100644
--- a/routingtable/filter/term.go
+++ b/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}
}
diff --git a/routingtable/filter/term_test.go b/routingtable/filter/term_test.go
index 876eb669..591fd234 100644
--- a/routingtable/filter/term_test.go
+++ b/routingtable/filter/term_test.go
@@ -12,11 +12,11 @@ import (
type mockAction struct {
}
-func (*mockAction) Do(p net.Prefix, pa *route.Path) (*route.Path, bool) {
+func (*mockAction) Do(p net.Prefix, pa *route.Path) actions.Result {
cp := *pa
cp.Type = route.OSPFPathType
- return &cp, false
+ return actions.Result{Path: &cp}
}
func TestProcess(t *testing.T) {
@@ -25,7 +25,7 @@ func TestProcess(t *testing.T) {
prefix net.Prefix
path *route.Path
from []*TermCondition
- then []FilterAction
+ then []Action
expectReject bool
expectModified bool
}{
@@ -34,7 +34,7 @@ func TestProcess(t *testing.T) {
prefix: net.NewPfx(net.IPv4FromOctets(100, 64, 0, 1), 8),
path: &route.Path{},
from: []*TermCondition{},
- then: []FilterAction{
+ then: []Action{
&actions.AcceptAction{},
},
expectReject: false,
@@ -48,7 +48,7 @@ func TestProcess(t *testing.T) {
NewTermConditionWithPrefixLists(
NewPrefixList(net.NewPfx(net.IPv4FromOctets(100, 64, 0, 1), 8))),
},
- then: []FilterAction{
+ then: []Action{
&actions.AcceptAction{},
},
expectReject: false,
@@ -62,7 +62,7 @@ func TestProcess(t *testing.T) {
NewTermConditionWithPrefixLists(
NewPrefixList(net.NewPfx(net.IPv4(0), 32))),
},
- then: []FilterAction{
+ then: []Action{
&actions.AcceptAction{},
},
expectReject: false,
@@ -76,7 +76,7 @@ func TestProcess(t *testing.T) {
NewTermConditionWithPrefixLists(
NewPrefixList(net.NewPfx(net.IPv4FromOctets(100, 64, 0, 1), 8))),
},
- then: []FilterAction{
+ then: []Action{
&mockAction{},
},
expectReject: false,
@@ -90,7 +90,7 @@ func TestProcess(t *testing.T) {
NewTermConditionWithRouteFilters(
NewRouteFilter(net.NewPfx(net.IPv4FromOctets(100, 64, 0, 1), 8), Exact())),
},
- then: []FilterAction{
+ then: []Action{
&mockAction{},
&actions.AcceptAction{},
},
@@ -109,7 +109,7 @@ func TestProcess(t *testing.T) {
},
},
},
- then: []FilterAction{
+ then: []Action{
&actions.AcceptAction{},
},
expectReject: false,
@@ -118,18 +118,18 @@ func TestProcess(t *testing.T) {
}
for _, test := range tests {
- t.Run(test.name, func(te *testing.T) {
+ t.Run(test.name, func(t *testing.T) {
term := NewTerm(test.from, test.then)
- pa, reject := term.Process(test.prefix, test.path)
- assert.Equal(te, test.expectReject, reject, "reject")
+ res := term.Process(test.prefix, test.path)
+ assert.Equal(t, test.expectReject, res.Reject, "reject")
- if pa != test.path && !test.expectModified {
- te.Fatal("expected path to be not modified but was")
+ if res.Path != test.path && !test.expectModified {
+ t.Fatal("expected path to be not modified but was")
}
- if pa == test.path && test.expectModified {
- te.Fatal("expected path to be modified but was same reference")
+ if res.Path == test.path && test.expectModified {
+ t.Fatal("expected path to be modified but was same reference")
}
})
}
--
GitLab