From b032227fcb4ee0824ce5a88b44cb93efd7409008 Mon Sep 17 00:00:00 2001 From: Naveen N Date: Mon, 4 May 2015 21:45:53 +0530 Subject: [PATCH] Pick rpf nexthop for layer2 flow from layer3 route table. In case of layer2 flows rpf nexthop was getting picked from layer2 routes only, RPF check should be done on layer3 route and changing the logic for same. 1> In case of egress flow, if there is a host route then that route would be used to set RPF nh, this is because for some baremetal flow agent may not be aware of layer address 2> Ingress flow always check for layer3 route. Test case for same. Closes-bug:#1424942 Change-Id: If92f4369332ae602572bc92a627b50b51cb765d9 --- src/vnsw/agent/init/agent_init.cc | 6 +- src/vnsw/agent/oper/nexthop.cc | 4 + src/vnsw/agent/oper/nexthop.h | 2 + src/vnsw/agent/pkt/flow_table.cc | 57 ++++++- src/vnsw/agent/pkt/flow_table.h | 2 +- src/vnsw/agent/pkt/test/egress-flow.xml | 12 +- src/vnsw/agent/pkt/test/ingress-flow.xml | 12 +- src/vnsw/agent/pkt/test/l2-sg-flow.xml | 18 +-- src/vnsw/agent/pkt/test/rpf-flow.xml | 143 ++++++++++++++++++ src/vnsw/agent/pkt/test/test_xml_packet_ut.cc | 18 +++ src/vnsw/agent/test-xml/test_xml_oper.cc | 22 ++- src/vnsw/agent/test-xml/test_xml_oper.h | 1 + .../agent/vrouter/ksync/flowtable_ksync.cc | 12 +- 13 files changed, 273 insertions(+), 36 deletions(-) create mode 100644 src/vnsw/agent/pkt/test/rpf-flow.xml diff --git a/src/vnsw/agent/init/agent_init.cc b/src/vnsw/agent/init/agent_init.cc index 737ca09f173..0f329d8b6a1 100644 --- a/src/vnsw/agent/init/agent_init.cc +++ b/src/vnsw/agent/init/agent_init.cc @@ -209,6 +209,11 @@ static void CreateVrfIndependentNextHop(Agent *agent) { (agent->nexthop_table()->FindActiveEntry(&key1)); agent->nexthop_table()->set_discard_nh(nh); + //Reserve index 2, this would be used to + //set as RPF NH when packet has to be discarded + //due to source route mismatch + assert(agent->nexthop_table()->ReserveIndex() == + NextHopTable::kRpfDiscardIndex); L2ReceiveNH::Create(); L2ReceiveNHKey key2; nh = static_cast @@ -238,7 +243,6 @@ void AgentInit::CreateVrfBase() { } void AgentInit::CreateNextHopsBase() { - CreateVrfIndependentNextHop(agent_.get()); CreateNextHops(); } diff --git a/src/vnsw/agent/oper/nexthop.cc b/src/vnsw/agent/oper/nexthop.cc index fb788cf8cd6..f8fa426cf3e 100644 --- a/src/vnsw/agent/oper/nexthop.cc +++ b/src/vnsw/agent/oper/nexthop.cc @@ -77,6 +77,10 @@ NextHopTable::~NextHopTable() { FreeInterfaceId(0); } +uint32_t NextHopTable::ReserveIndex() { + return index_table_.Insert(NULL); +} + void NextHop::SendObjectLog(AgentLogEvent::type event) const { NextHopObjectLogInfo info; diff --git a/src/vnsw/agent/oper/nexthop.h b/src/vnsw/agent/oper/nexthop.h index a315e22bf62..2e3b2829cad 100644 --- a/src/vnsw/agent/oper/nexthop.h +++ b/src/vnsw/agent/oper/nexthop.h @@ -1358,6 +1358,7 @@ class CompositeNH : public NextHop { ///////////////////////////////////////////////////////////////////////////// class NextHopTable : public AgentDBTable { public: + static const uint32_t kRpfDiscardIndex = 2; NextHopTable(DB *db, const std::string &name); virtual ~NextHopTable(); @@ -1411,6 +1412,7 @@ class NextHopTable : public AgentDBTable { // NextHop index managing routines void FreeInterfaceId(size_t index) { index_table_.Remove(index); } NextHop *FindNextHop(size_t index); + uint32_t ReserveIndex(); private: NextHop *AllocWithKey(const DBRequestKey *k) const; diff --git a/src/vnsw/agent/pkt/flow_table.cc b/src/vnsw/agent/pkt/flow_table.cc index 67fbff956f2..a61f615109f 100644 --- a/src/vnsw/agent/pkt/flow_table.cc +++ b/src/vnsw/agent/pkt/flow_table.cc @@ -1252,7 +1252,36 @@ void FlowEntry::SetAclFlowSandeshData(const AclDBEntry *acl, fe_sandesh_data.set_dmac(data_.dmac.ToString()); } -bool FlowEntry::SetRpfNH(const AgentRoute *rt) { +bool FlowEntry::SetRpfNH(FlowTable *ft, const AgentRoute *rt) { + bool ret = false; + //If l2 flow has a ip route entry present in + //layer 3 table, then use that for calculating + //rpf nexthop, else use layer 2 route entry(baremetal + //scenario where layer 3 route may not be present) + + if (l3_flow() == false) { + //For ingress flow, agent always sets + //rpf NH from layer 3 route entry + //In case of egress flow if route entry is present + //and its a host route entry use it for RPF NH + //For baremetal case since IP address may not be known + //agent uses layer 2 route entry + InetUnicastRouteEntry *ip_rt = static_cast( + ft->GetUcRoute(rt->vrf(), key().src_addr)); + if (is_flags_set(FlowEntry::IngressDir) || + (ip_rt && ip_rt->IsHostRoute())) { + rt = ip_rt; + } + } + + if (!rt) { + if (data_.nh_state_ != NULL) { + data_.nh_state_ = NULL; + ret = true; + } + return ret; + } + const NextHop *nh = rt->GetActiveNextHop(); if (nh->GetType() == NextHop::COMPOSITE && !is_flags_set(FlowEntry::LocalFlow) && @@ -1394,7 +1423,7 @@ void FlowEntry::InitFwdFlow(const PktFlowInfo *info, const PktInfo *pkt, reset_flags(FlowEntry::IngressDir); } if (ctrl->rt_ != NULL) { - SetRpfNH(ctrl->rt_); + SetRpfNH(info->flow_table, ctrl->rt_); } data_.flow_source_vrf = info->flow_source_vrf; @@ -1449,7 +1478,7 @@ void FlowEntry::InitRevFlow(const PktFlowInfo *info, const PktInfo *pkt, } } if (ctrl->rt_ != NULL) { - SetRpfNH(ctrl->rt_); + SetRpfNH(info->flow_table, ctrl->rt_); } data_.flow_source_vrf = info->flow_dest_vrf; @@ -2077,6 +2106,10 @@ void InetRouteFlowUpdate::RouteDel(AgentRoute *entry) { bool InetRouteFlowUpdate::SgUpdate(FlowEntry *fe, FlowTable *table, RouteFlowKey &key, const SecurityGroupList &sg_list) { + if (fe->l3_flow() == false) { + return true; + } + fe->GetPolicyInfo(); // Update SG-ID List @@ -2178,6 +2211,9 @@ void BridgeEntryFlowUpdate::RouteDel(AgentRoute *entry) { bool BridgeEntryFlowUpdate::SgUpdate(FlowEntry *fe, FlowTable *table, RouteFlowKey &key, const SecurityGroupList &sg_list) { + if (fe->l3_flow() == true) { + return true; + } fe->GetPolicyInfo(); // Update SG-ID List @@ -2395,7 +2431,7 @@ void FlowTable::ResyncRpfNH(const RouteFlowKey &key, const AgentRoute *rt) { continue; } - if (flow->SetRpfNH(rt) == true) { + if (flow->SetRpfNH(this, rt) == true) { flow->UpdateKSync(this); FlowInfo flow_info; flow->FillFlowInfo(flow_info); @@ -2420,6 +2456,9 @@ void FlowTable::FlowRecompute(RouteFlowInfo *rt_info) { /* for reverse flow trigger a re-eval on its forward flow */ fe = fe->reverse_flow_entry(); } + if (fe->l3_flow() == false) { + continue; + } if (fe->set_pending_recompute(true)) { agent_->pkt()->pkt_handler()->SendMessage(PktHandler::FLOW, new FlowTaskMsg(fe)); @@ -2681,9 +2720,10 @@ void FlowTable::DeleteL2RouteFlowInfo(FlowEntry *fe) { } void FlowTable::DeleteRouteFlowInfo (FlowEntry *fe) { - if (fe->l3_flow_) { + if (fe->l3_flow()) { DeleteInetRouteFlowInfo(fe); } else { + DeleteInetRouteFlowInfo(fe); DeleteL2RouteFlowInfo(fe); } } @@ -2987,9 +3027,14 @@ void FlowTable::AddL2RouteFlowInfo (FlowEntry *fe) { } void FlowTable::AddRouteFlowInfo (FlowEntry *fe) { - if (fe->l3_flow_) { + if (fe->l3_flow()) { AddInetRouteFlowInfo(fe); } else { + //For layer 2 flow, we add flow entry in both + //inet and layer 2 tree entry. This + //get s used to resync rpf nexthop for a layer2 flow + //since its calculated based on layer3 route entry + AddInetRouteFlowInfo(fe); AddL2RouteFlowInfo(fe); } } diff --git a/src/vnsw/agent/pkt/flow_table.h b/src/vnsw/agent/pkt/flow_table.h index 4de153736f1..7d7f3912574 100644 --- a/src/vnsw/agent/pkt/flow_table.h +++ b/src/vnsw/agent/pkt/flow_table.h @@ -430,7 +430,7 @@ class FlowEntry { friend class FlowStatsCollector; friend void intrusive_ptr_add_ref(FlowEntry *fe); friend void intrusive_ptr_release(FlowEntry *fe); - bool SetRpfNH(const AgentRoute *rt); + bool SetRpfNH(FlowTable *ft, const AgentRoute *rt); bool InitFlowCmn(const PktFlowInfo *info, const PktControlInfo *ctrl, const PktControlInfo *rev_ctrl); void GetSourceRouteInfo(const AgentRoute *rt); diff --git a/src/vnsw/agent/pkt/test/egress-flow.xml b/src/vnsw/agent/pkt/test/egress-flow.xml index 678320381e7..a14500c6b74 100644 --- a/src/vnsw/agent/pkt/test/egress-flow.xml +++ b/src/vnsw/agent/pkt/test/egress-flow.xml @@ -35,13 +35,13 @@ sip="1.1.1.2" dip="1.1.1.1" proto="udp" sport="1" dport="3" type="flow" /> - - - @@ -73,13 +73,13 @@ sip="1.1.1.2" dip="1.1.1.1" proto="udp" sport="1" dport="13" type="flow" /> - - - diff --git a/src/vnsw/agent/pkt/test/ingress-flow.xml b/src/vnsw/agent/pkt/test/ingress-flow.xml index 82d7c69dace..4835ac567ad 100644 --- a/src/vnsw/agent/pkt/test/ingress-flow.xml +++ b/src/vnsw/agent/pkt/test/ingress-flow.xml @@ -26,10 +26,10 @@ sip="1.1.1.1" dip="1.1.1.2" proto="udp" sport="1" dport="2" type="flow" /> - - @@ -54,12 +54,12 @@ sip="1.1.1.1" dip="1.1.1.2" proto="udp" sport="1" dport="12" type="flow" /> - - + + dvn="vn1" action="pass" rpf_nh="13"/> - - - @@ -70,13 +70,13 @@ vn="vn1" vxlan_id="0" tunnel-dest="5.5.5.5" tunnel-type="vxlan" sg="1" label="5" /> - - - @@ -90,13 +90,13 @@ - - - diff --git a/src/vnsw/agent/pkt/test/rpf-flow.xml b/src/vnsw/agent/pkt/test/rpf-flow.xml new file mode 100644 index 00000000000..474b092a7aa --- /dev/null +++ b/src/vnsw/agent/pkt/test/rpf-flow.xml @@ -0,0 +1,143 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/vnsw/agent/pkt/test/test_xml_packet_ut.cc b/src/vnsw/agent/pkt/test/test_xml_packet_ut.cc index 62b6db30d9d..46eaa07ff7a 100644 --- a/src/vnsw/agent/pkt/test/test_xml_packet_ut.cc +++ b/src/vnsw/agent/pkt/test/test_xml_packet_ut.cc @@ -82,15 +82,33 @@ TEST_F(TestPkt, l2_sg_flow_1) { } } +TEST_F(TestPkt, rpf_flow) { + AgentUtXmlTest test("controller/src/vnsw/agent/pkt/test/rpf-flow.xml"); + AgentUtXmlOperInit(&test); + if (test.Load() == true) { + test.ReadXml(); + + string str; + test.ToString(&str); + cout << str << endl; + test.Run(); + } +} + + int main(int argc, char *argv[]) { GETUSERARGS(); client = XmlPktParseTestInit(init_file, ksync_init); client->agent()->flow_stats_collector()->set_expiry_time(1000*1000); client->agent()->flow_stats_collector()->set_delete_short_flow(false); + boost::system::error_code ec; + bgp_peer_ = CreateBgpPeer(Ip4Address::from_string("0.0.0.1", ec), + "xmpp channel"); usleep(1000); client->WaitForIdle(); int ret = RUN_ALL_TESTS(); + DeleteBgpPeer(bgp_peer_); TestShutdown(); delete client; return ret; diff --git a/src/vnsw/agent/test-xml/test_xml_oper.cc b/src/vnsw/agent/test-xml/test_xml_oper.cc index 4a625055902..619fd5ad2f0 100644 --- a/src/vnsw/agent/test-xml/test_xml_oper.cc +++ b/src/vnsw/agent/test-xml/test_xml_oper.cc @@ -1092,6 +1092,9 @@ bool AgentUtXmlFlowValidate::ReadXml() { GetStringAttribute(node(), "svn", &svn_); GetStringAttribute(node(), "dvn", &dvn_); GetStringAttribute(node(), "action", &action_); + if (GetUintAttribute(node(), "rpf_nh", &rpf_nh_) == false) { + rpf_nh_ = 0; + } return true; } @@ -1125,6 +1128,18 @@ bool AgentUtXmlFlowValidate::Validate() { if (MatchFlowAction(flow, action_) == false) return false; + + if (rpf_nh_) { + if (rpf_nh_ == NextHopTable::kRpfDiscardIndex) { + if (flow->data().nh_state_ != NULL) { + return false; + } + } else if (!flow->data().nh_state_ || + !flow->data().nh_state_->nh() || + flow->data().nh_state_->nh()->id() != rpf_nh_) { + return false; + } + } return true; } @@ -1211,17 +1226,18 @@ bool AgentUtXmlL2Route::Run() { if (encap == TunnelEncapType::VXLAN) bmap |= (1 << TunnelType::VXLAN); if (op_delete()) { - rt_table->DeleteReq(NULL, vrf_, MacAddress::FromString(mac_), + rt_table->DeleteReq(bgp_peer_, vrf_, + MacAddress::FromString(mac_), Ip4Address::from_string(ip_), vxlan_id_); } else { ControllerVmRoute *data = - ControllerVmRoute::MakeControllerVmRoute(NULL, + ControllerVmRoute::MakeControllerVmRoute(bgp_peer_, agent->fabric_vrf_name(), agent->router_id(), vrf_, Ip4Address::from_string(tunnel_dest_), bmap, label_, vn_, sg_list, PathPreference()); - rt_table->AddRemoteVmRouteReq(NULL, vrf_, + rt_table->AddRemoteVmRouteReq(bgp_peer_, vrf_, MacAddress::FromString(mac_), Ip4Address::from_string(ip_), vxlan_id_, data); diff --git a/src/vnsw/agent/test-xml/test_xml_oper.h b/src/vnsw/agent/test-xml/test_xml_oper.h index 44d29fffe45..951126f6e77 100644 --- a/src/vnsw/agent/test-xml/test_xml_oper.h +++ b/src/vnsw/agent/test-xml/test_xml_oper.h @@ -336,6 +336,7 @@ class AgentUtXmlFlowValidate : public AgentUtXmlValidationNode { std::string svn_; std::string dvn_; std::string action_; + uint16_t rpf_nh_; }; class AgentUtXmlL2Route : public AgentUtXmlNode { diff --git a/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc b/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc index d544e564225..b59c9c7c9e1 100644 --- a/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc +++ b/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc @@ -298,10 +298,14 @@ int FlowTableKSyncEntry::Encode(sandesh_op::type op, char *buf, int buf_len) { action = VR_FLOW_ACTION_HOLD; } - if (nh_ && enable_rpf_) { - const NHKSyncEntry *ksync_nh = - static_cast(nh_.get()); - req.set_fr_src_nh_index(ksync_nh->nh_id()); + if (enable_rpf_) { + if (nh_) { + const NHKSyncEntry *ksync_nh = + static_cast(nh_.get()); + req.set_fr_src_nh_index(ksync_nh->nh_id()); + } else { + req.set_fr_src_nh_index(NextHopTable::kRpfDiscardIndex); + } } else { //Set to discard, vrouter ignores RPF check if //nexthop is set to discard