diff --git a/bgp/config_test.go b/bgp/config_test.go index a8b84d9b..c9b64c98 100644 --- a/bgp/config_test.go +++ b/bgp/config_test.go @@ -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"}, @@ -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"}, @@ -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"}, @@ -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"}, @@ -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 { diff --git a/bgp/ocgobgp.go b/bgp/ocgobgp.go index 515ed6c6..a0fc5c85 100644 --- a/bgp/ocgobgp.go +++ b/bgp/ocgobgp.go @@ -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) @@ -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(), @@ -113,7 +116,7 @@ func convertPolicyDefinition(policy *oc.RoutingPolicy_PolicyDefinition, neighAdd } return gobgp.PolicyDefinition{ - Name: convertPolicyName(neighAddr, policy.GetName()), + Name: convertedPolicyName, Statements: statements, } } diff --git a/bgp/tests/local_tests/community_set_test.go b/bgp/tests/local_tests/community_set_test.go index 71bf91c2..94c0f2af 100644 --- a/bgp/tests/local_tests/community_set_test.go +++ b/bgp/tests/local_tests/community_set_test.go @@ -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{} @@ -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{