Skip to content

Commit

Permalink
Merge pull request #287 from openconfig/allow-same-stmt-names
Browse files Browse the repository at this point in the history 
Allow Same Statement Names across Policies
  • Loading branch information
@wenovus
wenovus authored Oct 4, 2023
2 parents f25d124 + 8891fa0 commit edd8683
Show file tree
Hide file tree
Showing 3 changed files with 279 additions and 7 deletions.
257 changes: 253 additions & 4 deletions bgp/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func TestIntendedToGoBGPPolicies(t *testing.T) {
PolicyDefinitions: []gobgp.PolicyDefinition{{
Name: "1.1.1.1|foo",
Statements: []gobgp.Statement{{
Name: "foo-1",
Name: "1.1.1.1|foo:foo-1",
Conditions: gobgp.Conditions{
CallPolicy: "",
MatchPrefixSet: gobgp.MatchPrefixSet{PrefixSet: "V4-1", MatchSetOptions: "any"},
Expand Down Expand Up @@ -185,7 +185,7 @@ func TestIntendedToGoBGPPolicies(t *testing.T) {
},
},
}, {
Name: "foo-2",
Name: "1.1.1.1|foo:foo-2",
Conditions: gobgp.Conditions{
CallPolicy: "",
MatchPrefixSet: gobgp.MatchPrefixSet{PrefixSet: "V4-2", MatchSetOptions: "any"},
Expand Down Expand Up @@ -256,7 +256,7 @@ func TestIntendedToGoBGPPolicies(t *testing.T) {
}, {
Name: "2.2.2.2|foo",
Statements: []gobgp.Statement{{
Name: "foo-1",
Name: "2.2.2.2|foo:foo-1",
Conditions: gobgp.Conditions{
CallPolicy: "",
MatchPrefixSet: gobgp.MatchPrefixSet{PrefixSet: "V4-1", MatchSetOptions: "any"},
Expand Down Expand Up @@ -302,7 +302,7 @@ func TestIntendedToGoBGPPolicies(t *testing.T) {
},
},
}, {
Name: "foo-2",
Name: "2.2.2.2|foo:foo-2",
Conditions: gobgp.Conditions{
CallPolicy: "",
MatchPrefixSet: gobgp.MatchPrefixSet{PrefixSet: "V4-2", MatchSetOptions: "any"},
Expand Down Expand Up @@ -372,6 +372,255 @@ func TestIntendedToGoBGPPolicies(t *testing.T) {
}},
}},
},
}, {
desc: "remove-community-set-two-statements",
inOC: func() *oc.Root {
root := &oc.Root{}
bgpoc := root.GetOrCreateNetworkInstance(fakedevice.DefaultNetworkInstance).GetOrCreateProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, fakedevice.BGPRoutingProtocol).GetOrCreateBgp()
policyoc := root.GetOrCreateRoutingPolicy()

// Create prefix set
prefixSetName := "prefixset-foo"
prefixSet := root.GetOrCreateRoutingPolicy().GetOrCreateDefinedSets().GetOrCreatePrefixSet(prefixSetName)
prefixSet.SetMode(oc.PrefixSet_Mode_IPV4)
prefixSet.GetOrCreatePrefix("10.0.0.0/10", "8..32")

// DEFINED SETS
commsetName := "COMM1"
root.GetOrCreateRoutingPolicy().GetOrCreateDefinedSets().GetOrCreateBgpDefinedSets().GetOrCreateCommunitySet(commsetName).SetCommunityMember(
[]oc.RoutingPolicy_DefinedSets_BgpDefinedSets_CommunitySet_CommunityMember_Union{
oc.UnionString("[0-9]+:[0-9]+"),
},
)

// POLICY
stmtName := "stmt"
policy1 := &oc.RoutingPolicy_PolicyDefinition_Statement_OrderedMap{}
policy1Name := "foo"
policyoc.GetOrCreatePolicyDefinition(policy1Name).Statement = policy1

stmt, err := policy1.AppendNew(stmtName)
if err != nil {
t.Fatalf("Cannot append new BGP policy statement: %v", err)
}
stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetPrefixSet(prefixSetName)
stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetMatchSetOptions(oc.RoutingPolicy_MatchSetOptionsRestrictedType_ANY)

stmt.GetOrCreateActions().GetOrCreateBgpActions().GetOrCreateSetCommunity().SetOptions(oc.BgpPolicy_BgpSetCommunityOptionType_ADD)
stmt.GetOrCreateActions().GetOrCreateBgpActions().GetOrCreateSetCommunity().GetOrCreateInline().SetCommunities(
[]oc.RoutingPolicy_PolicyDefinition_Statement_Actions_BgpActions_SetCommunity_Inline_Communities_Union{
oc.UnionString("11111:11111"),
oc.UnionString("22222:22222"),
},
)
stmt.GetOrCreateActions().GetOrCreateBgpActions().GetOrCreateSetCommunity().SetMethod(oc.SetCommunity_Method_INLINE)

policy2 := &oc.RoutingPolicy_PolicyDefinition_Statement_OrderedMap{}
policy2Name := "bar"
policyoc.GetOrCreatePolicyDefinition(policy2Name).Statement = policy2

stmt, err = policy2.AppendNew(stmtName)
if err != nil {
t.Fatalf("Cannot append new BGP policy statement: %v", err)
}
stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetPrefixSet(prefixSetName)
stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetMatchSetOptions(oc.RoutingPolicy_MatchSetOptionsRestrictedType_ANY)
stmt.GetOrCreateActions().GetOrCreateBgpActions().GetOrCreateSetCommunity().SetOptions(oc.BgpPolicy_BgpSetCommunityOptionType_REMOVE)
stmt.GetOrCreateActions().GetOrCreateBgpActions().GetOrCreateSetCommunity().GetOrCreateReference().SetCommunitySetRef(commsetName)
stmt.GetOrCreateActions().GetOrCreateBgpActions().GetOrCreateSetCommunity().SetMethod(oc.SetCommunity_Method_REFERENCE)
stmt.GetOrCreateActions().SetPolicyResult(oc.RoutingPolicy_PolicyResultType_ACCEPT_ROUTE)

bgpoc.GetOrCreateNeighbor("1.1.1.1").GetOrCreateApplyPolicy().SetExportPolicy([]string{policy1Name})
bgpoc.GetOrCreateNeighbor("2.2.2.2").GetOrCreateApplyPolicy().SetExportPolicy([]string{policy2Name})
return root
}(),
wantBGPConfig: &gobgp.BgpConfigSet{
Global: gobgp.Global{
ApplyPolicy: gobgp.ApplyPolicy{
Config: gobgp.ApplyPolicyConfig{
ImportPolicyList: []string{
"default-import|1.1.1.1",
"default-import|2.2.2.2",
},
DefaultImportPolicy: "",
ExportPolicyList: []string{
"1.1.1.1|foo",
"default-export|1.1.1.1",
"2.2.2.2|bar",
"default-export|2.2.2.2",
},
DefaultExportPolicy: "",
InPolicyList: []string(nil),
DefaultInPolicy: "",
},
},
},
DefinedSets: gobgp.DefinedSets{
PrefixSets: []gobgp.PrefixSet{{
PrefixSetName: "prefixset-foo",
PrefixList: []gobgp.Prefix{{
IpPrefix: "10.0.0.0/10",
MasklengthRange: "8..32",
}},
}},
NeighborSets: []gobgp.NeighborSet{{
NeighborSetName: "1.1.1.1",
NeighborInfoList: []string{"1.1.1.1"},
}, {
NeighborSetName: "2.2.2.2",
NeighborInfoList: []string{"2.2.2.2"},
}},
BgpDefinedSets: gobgp.BgpDefinedSets{
CommunitySets: []gobgp.CommunitySet{{
CommunitySetName: "COMM1",
CommunityList: []string{"[0-9]+:[0-9]+"},
}},
},
},
PolicyDefinitions: []gobgp.PolicyDefinition{{
Name: "1.1.1.1|foo",
Statements: []gobgp.Statement{{
Name: "1.1.1.1|foo:stmt",
Conditions: gobgp.Conditions{
CallPolicy: "",
MatchPrefixSet: gobgp.MatchPrefixSet{PrefixSet: "prefixset-foo", MatchSetOptions: "any"},
MatchNeighborSet: gobgp.MatchNeighborSet{NeighborSet: "1.1.1.1", MatchSetOptions: ""}, MatchTagSet: gobgp.MatchTagSet{TagSet: "", MatchSetOptions: ""},
InstallProtocolEq: "", IgpConditions: gobgp.IgpConditions{}, BgpConditions: gobgp.BgpConditions{
MatchCommunitySet: gobgp.MatchCommunitySet{
CommunitySet: "",
MatchSetOptions: "any",
},
MatchExtCommunitySet: gobgp.MatchExtCommunitySet{
ExtCommunitySet: "",
MatchSetOptions: "",
},
MatchAsPathSet: gobgp.MatchAsPathSet{
AsPathSet: "", MatchSetOptions: "any",
},
MedEq: 0x0,
OriginEq: "",
NextHopInList: []string(nil),
AfiSafiInList: []gobgp.AfiSafiType(nil),
LocalPrefEq: 0x0,
CommunityCount: gobgp.CommunityCount{Operator: "", Value: 0x0},
AsPathLength: gobgp.AsPathLength{Operator: "", Value: 0x0},
RouteType: "",
RpkiValidationResult: "",
MatchLargeCommunitySet: gobgp.MatchLargeCommunitySet{
LargeCommunitySet: "",
MatchSetOptions: "",
},
},
},
Actions: gobgp.Actions{
RouteDisposition: "none",
IgpActions: gobgp.IgpActions{SetTag: ""},
BgpActions: gobgp.BgpActions{SetAsPathPrepend: gobgp.SetAsPathPrepend{RepeatN: 0x0, As: "0"},
SetCommunity: gobgp.SetCommunity{SetCommunityMethod: gobgp.SetCommunityMethod{CommunitiesList: []string{"11111:11111", "22222:22222"}, CommunitySetRef: ""}, Options: "add"},
SetExtCommunity: gobgp.SetExtCommunity{SetExtCommunityMethod: gobgp.SetExtCommunityMethod{CommunitiesList: []string(nil), ExtCommunitySetRef: ""}, Options: ""},
SetRouteOrigin: "",
SetLocalPref: 0x0,
SetNextHop: "",
SetMed: "",
SetLargeCommunity: gobgp.SetLargeCommunity{SetLargeCommunityMethod: gobgp.SetLargeCommunityMethod{CommunitiesList: []string(nil)}, Options: ""},
},
},
}},
}, {
Name: "default-import|1.1.1.1",
Statements: []gobgp.Statement{{
Name: "default-import|1.1.1.1",
Conditions: gobgp.Conditions{
MatchNeighborSet: gobgp.MatchNeighborSet{NeighborSet: "1.1.1.1", MatchSetOptions: ""},
},
Actions: gobgp.Actions{
RouteDisposition: "reject-route",
},
}},
}, {
Name: "default-export|1.1.1.1",
Statements: []gobgp.Statement{{
Name: "default-export|1.1.1.1",
Conditions: gobgp.Conditions{
MatchNeighborSet: gobgp.MatchNeighborSet{NeighborSet: "1.1.1.1", MatchSetOptions: ""},
},
Actions: gobgp.Actions{
RouteDisposition: "reject-route",
},
}},
}, {
Name: "2.2.2.2|bar",
Statements: []gobgp.Statement{{
Name: "2.2.2.2|bar:stmt",
Conditions: gobgp.Conditions{
CallPolicy: "",
MatchPrefixSet: gobgp.MatchPrefixSet{PrefixSet: "prefixset-foo", MatchSetOptions: "any"},
MatchNeighborSet: gobgp.MatchNeighborSet{NeighborSet: "2.2.2.2", MatchSetOptions: ""}, MatchTagSet: gobgp.MatchTagSet{TagSet: "", MatchSetOptions: ""},
InstallProtocolEq: "", IgpConditions: gobgp.IgpConditions{}, BgpConditions: gobgp.BgpConditions{
MatchCommunitySet: gobgp.MatchCommunitySet{
CommunitySet: "",
MatchSetOptions: "any",
},
MatchExtCommunitySet: gobgp.MatchExtCommunitySet{
ExtCommunitySet: "",
MatchSetOptions: "",
},
MatchAsPathSet: gobgp.MatchAsPathSet{
AsPathSet: "", MatchSetOptions: "any",
},
MedEq: 0x0,
OriginEq: "",
NextHopInList: []string(nil),
AfiSafiInList: []gobgp.AfiSafiType(nil),
LocalPrefEq: 0x0,
CommunityCount: gobgp.CommunityCount{Operator: "", Value: 0x0},
AsPathLength: gobgp.AsPathLength{Operator: "", Value: 0x0},
RouteType: "",
RpkiValidationResult: "",
MatchLargeCommunitySet: gobgp.MatchLargeCommunitySet{
LargeCommunitySet: "",
MatchSetOptions: "",
},
},
},
Actions: gobgp.Actions{
RouteDisposition: "accept-route",
IgpActions: gobgp.IgpActions{SetTag: ""},
BgpActions: gobgp.BgpActions{SetAsPathPrepend: gobgp.SetAsPathPrepend{RepeatN: 0x0, As: "0"},
SetCommunity: gobgp.SetCommunity{SetCommunityMethod: gobgp.SetCommunityMethod{CommunitiesList: []string{"[0-9]+:[0-9]+"}, CommunitySetRef: ""}, Options: "remove"},
SetExtCommunity: gobgp.SetExtCommunity{SetExtCommunityMethod: gobgp.SetExtCommunityMethod{CommunitiesList: []string(nil), ExtCommunitySetRef: ""}, Options: ""},
SetRouteOrigin: "",
SetLocalPref: 0x0,
SetNextHop: "",
SetMed: "",
SetLargeCommunity: gobgp.SetLargeCommunity{SetLargeCommunityMethod: gobgp.SetLargeCommunityMethod{CommunitiesList: []string(nil)}, Options: ""},
},
},
}},
}, {
Name: "default-import|2.2.2.2",
Statements: []gobgp.Statement{{
Name: "default-import|2.2.2.2",
Conditions: gobgp.Conditions{
MatchNeighborSet: gobgp.MatchNeighborSet{NeighborSet: "2.2.2.2", MatchSetOptions: ""},
},
Actions: gobgp.Actions{
RouteDisposition: "reject-route",
},
}},
}, {
Name: "default-export|2.2.2.2",
Statements: []gobgp.Statement{{
Name: "default-export|2.2.2.2",
Conditions: gobgp.Conditions{
MatchNeighborSet: gobgp.MatchNeighborSet{NeighborSet: "2.2.2.2", MatchSetOptions: ""},
},
Actions: gobgp.Actions{
RouteDisposition: "reject-route",
},
}},
}},
},
}}

for _, tt := range tests {
Expand Down
7 changes: 5 additions & 2 deletions bgp/ocgobgp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func convertSetCommunities(setCommunity *oc.RoutingPolicy_PolicyDefinition_State
// for another neighbour. This is necessary since all policies will go into a
// single apply-policy list.
func convertPolicyDefinition(policy *oc.RoutingPolicy_PolicyDefinition, neighAddr string, occommset map[string]*oc.RoutingPolicy_DefinedSets_BgpDefinedSets_CommunitySet, convertedCommSets []gobgp.CommunitySet, commSetIndexMap map[string]int) gobgp.PolicyDefinition {
convertedPolicyName := convertPolicyName(neighAddr, policy.GetName())
var statements []gobgp.Statement
for _, statement := range policy.Statement.Values() {
setCommunitiesList, err := convertSetCommunities(statement.GetActions().GetBgpActions().GetSetCommunity(), convertedCommSets, commSetIndexMap)
Expand All @@ -71,7 +72,9 @@ func convertPolicyDefinition(policy *oc.RoutingPolicy_PolicyDefinition, neighAdd
log.Errorf("MED value not supported: %v", err)
}
statements = append(statements, gobgp.Statement{
Name: statement.GetName(),
// In GoBGP, statements must have globally-unique names.
// Ensure uniqueness by qualifying each one with the name of the converted policy.
Name: convertedPolicyName + ":" + statement.GetName(),
Conditions: gobgp.Conditions{
MatchPrefixSet: gobgp.MatchPrefixSet{
PrefixSet: statement.GetConditions().GetMatchPrefixSet().GetPrefixSet(),
Expand Down Expand Up @@ -113,7 +116,7 @@ func convertPolicyDefinition(policy *oc.RoutingPolicy_PolicyDefinition, neighAdd
}

return gobgp.PolicyDefinition{
Name: convertPolicyName(neighAddr, policy.GetName()),
Name: convertedPolicyName,
Statements: statements,
}
}
Expand Down
22 changes: 21 additions & 1 deletion bgp/tests/local_tests/community_set_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
)

func TestCommunitySet(t *testing.T) {
installRejectPolicy := func(t *testing.T, dut1, dut2, _, _, _ *Device) {
installRejectPolicy := func(t *testing.T, dut1, dut2, dut3, _, _ *Device) {
// Policy to reject routes with the given community set conditions
policyName := "community-sets"
policy := &oc.RoutingPolicy_PolicyDefinition_Statement_OrderedMap{}
Expand Down Expand Up @@ -101,10 +101,30 @@ func TestCommunitySet(t *testing.T) {
stmt.GetOrCreateConditions().GetOrCreateBgpConditions().SetCommunitySet(invertCommSetName)
stmt.GetOrCreateActions().SetPolicyResult(oc.RoutingPolicy_PolicyResultType_REJECT_ROUTE)

// Policy to reject routes with the given community set conditions
uselessPolicyName := "useless"
uselessPolicy := &oc.RoutingPolicy_PolicyDefinition_Statement_OrderedMap{}
stmt, err = uselessPolicy.AppendNew("reject-any-community-sets")
if err != nil {
t.Fatalf("Cannot append new BGP policy statement: %v", err)
}
stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetPrefixSet(prefixSetName)
stmt.GetOrCreateConditions().GetOrCreateMatchPrefixSet().SetMatchSetOptions(oc.RoutingPolicy_MatchSetOptionsRestrictedType_ANY)
stmt.GetOrCreateConditions().GetOrCreateBgpConditions().SetCommunitySet(anyCommSetName)
stmt.GetOrCreateActions().SetPolicyResult(oc.RoutingPolicy_PolicyResultType_REJECT_ROUTE)

// Install policy
Replace(t, dut2, ocpath.Root().RoutingPolicy().PolicyDefinition(policyName).Config(), &oc.RoutingPolicy_PolicyDefinition{Statement: policy})
Replace(t, dut2, bgp.BGPPath.Neighbor(dut1.RouterID).ApplyPolicy().ImportPolicy().Config(), []string{policyName})
// This apply-policy is a no-op because everything is rejected
// in the reverse direction anyways: its purpose is to check
// that statements across different policies can have the same
// name (GoBGP requires all statement names to be unique).
Replace(t, dut2, ocpath.Root().RoutingPolicy().PolicyDefinition(uselessPolicyName).Config(), &oc.RoutingPolicy_PolicyDefinition{Statement: uselessPolicy})
Replace(t, dut2, bgp.BGPPath.Neighbor(dut3.RouterID).ApplyPolicy().ImportPolicy().Config(), []string{uselessPolicyName})

Await(t, dut2, bgp.BGPPath.Neighbor(dut1.RouterID).ApplyPolicy().ImportPolicy().State(), []string{policyName})
Await(t, dut2, bgp.BGPPath.Neighbor(dut3.RouterID).ApplyPolicy().ImportPolicy().State(), []string{uselessPolicyName})
}

routeUnderTestList := []string{
Expand Down

0 comments on commit edd8683

Please sign in to comment.