Fix issue in Adding/Setting community list in policy action

If the route doesn't have community attribute and policy action sets/Adds a
new set of community, update is skipped.

Fix this issue and added Unit test cases to validate the case where route has empty
community before applying policy for all scenario(add/set/remove of community)

Change-Id: I58f12a23406bb86edfd8a9ea62c4bbda5cdfd66a
Related-bug: #1500698

src/bgp/routing-policy/routing_policy_action.cc

@@ -41,20 +41,18 @@ UpdateCommunity::UpdateCommunity(const std::vector<string> communities,
 void UpdateCommunity::operator()(BgpAttr *attr) const {
     if (!attr) return;
     const Community *comm = attr->community();
-    if (comm) {
-        BgpAttrDB *attr_db = attr->attr_db();
-        BgpServer *server = attr_db->server();
-        CommunityDB *comm_db = server->comm_db();
-        CommunityPtr new_community;
-        if (op_ == SET) {
-            new_community = comm_db->SetAndLocate(comm, communities_);
-        } else if (op_ == ADD) {
-            new_community = comm_db->AppendAndLocate(comm, communities_);
-        } else if (op_ == REMOVE) {
-            new_community = comm_db->RemoveAndLocate(comm, communities_);
-        }
-        attr->set_community(new_community);
+    BgpAttrDB *attr_db = attr->attr_db();
+    BgpServer *server = attr_db->server();
+    CommunityDB *comm_db = server->comm_db();
+    CommunityPtr new_community = NULL;
+    if (op_ == SET) {
+        new_community = comm_db->SetAndLocate(comm, communities_);
+    } else if (op_ == ADD) {
+        new_community = comm_db->AppendAndLocate(comm, communities_);
+    } else if (op_ == REMOVE) {
+        if (comm) new_community = comm_db->RemoveAndLocate(comm, communities_);
     }
+    attr->set_community(new_community);
 }
 
 string UpdateCommunity::ToString() const {

src/bgp/test/routing_policy_test.cc

@@ -537,8 +537,7 @@ TEST_F(RoutingPolicyTest, PolicyPrefixMatchSetCommunity) {
     peers_.push_back(
         new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec)));
 
-    AddRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32", 100,
-                 list_of("23:13")("23:44"));
+    AddRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32", 100);
     task_util::WaitForIdle();
 
     VERIFY_EQ(1, RouteCount("test.inet.0"));
@@ -549,7 +548,7 @@ TEST_F(RoutingPolicyTest, PolicyPrefixMatchSetCommunity) {
     ASSERT_TRUE(rt->BestPath()->IsFeasible() == true);
     ASSERT_EQ(GetCommunityListFromRoute(rt->BestPath()), list_of("11:13"));
     ASSERT_EQ(GetOriginalCommunityListFromRoute(rt->BestPath()),
-              list_of("23:13")("23:44"));
+              vector<string>());
     DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32");
 }
 
@@ -607,6 +606,32 @@ TEST_F(RoutingPolicyTest, PolicyPrefixMatchRemoveMultipleCommunity_1) {
     DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32");
 }
 
+// Input route has no community to begin with
+TEST_F(RoutingPolicyTest, PolicyPrefixMatchRemoveMultipleCommunity_2) {
+    string content =
+        FileRead("controller/src/bgp/testdata/routing_policy_2d.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    boost::system::error_code ec;
+    peers_.push_back(
+        new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec)));
+
+    AddRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32", 100);
+    task_util::WaitForIdle();
+
+    VERIFY_EQ(1, RouteCount("test.inet.0"));
+    BgpRoute *rt =
+        RouteLookup<InetDefinition>("test.inet.0", "1.1.1.1/32");
+    ASSERT_TRUE(rt != NULL);
+    VERIFY_EQ(peers_[0], rt->BestPath()->GetPeer());
+    ASSERT_TRUE(rt->BestPath()->IsFeasible() == true);
+    ASSERT_EQ(GetCommunityListFromRoute(rt->BestPath()), vector<string>());
+    ASSERT_EQ(GetOriginalCommunityListFromRoute(rt->BestPath()),
+              vector<string>());
+    DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32");
+}
+
 TEST_F(RoutingPolicyTest, PolicyPrefixMatchAddMultipleCommunity) {
     string content =
         FileRead("controller/src/bgp/testdata/routing_policy_2e.xml");
@@ -634,6 +659,34 @@ TEST_F(RoutingPolicyTest, PolicyPrefixMatchAddMultipleCommunity) {
     DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32");
 }
 
+// Route has not community to start with
+TEST_F(RoutingPolicyTest, PolicyPrefixMatchAddMultipleCommunity_1) {
+    string content =
+        FileRead("controller/src/bgp/testdata/routing_policy_2e.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    boost::system::error_code ec;
+    peers_.push_back(
+        new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec)));
+
+    AddRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32", 100);
+    task_util::WaitForIdle();
+
+    VERIFY_EQ(1, RouteCount("test.inet.0"));
+    BgpRoute *rt =
+        RouteLookup<InetDefinition>("test.inet.0", "1.1.1.1/32");
+    ASSERT_TRUE(rt != NULL);
+    VERIFY_EQ(peers_[0], rt->BestPath()->GetPeer());
+    ASSERT_TRUE(rt->BestPath()->IsFeasible() == true);
+    ASSERT_EQ(GetCommunityListFromRoute(rt->BestPath()),
+        list_of("11:22")("22:44")("44:88"));
+    ASSERT_EQ(GetOriginalCommunityListFromRoute(rt->BestPath()),
+              vector<string>());
+    DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", "1.1.1.1/32");
+}
+
+
 TEST_F(RoutingPolicyTest, PolicyPrefixMatchSetMultipleCommunity) {
     string content =
         FileRead("controller/src/bgp/testdata/routing_policy_2f.xml");