Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[routeorch] Add support for blackhole routes #1723

Merged
merged 4 commits into from
Apr 27, 2021
Merged

[routeorch] Add support for blackhole routes #1723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
45a282e
Add support for blackhole
Apr 26, 2021
f5447e9
Add unit tests
Apr 26, 2021
47139f6
Remove todo message for blackhole
Apr 26, 2021
97e7455
Fix extra nhg
Apr 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 61 additions & 10 deletions orchagent/routeorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ void RouteOrch::doTask(Consumer& consumer)
string remote_macs;
bool& excp_intfs_flag = ctx.excp_intfs_flag;
bool overlay_nh = false;
bool blackhole = false;

for (auto i : kfvFieldsValues(t))
{
Expand All @@ -529,6 +530,9 @@ void RouteOrch::doTask(Consumer& consumer)

if (fvField(i) == "router_mac")
remote_macs = fvValue(i);

if (fvField(i) == "blackhole")
blackhole = fvValue(i) == "true";
}

vector<string>& ipv = ctx.ipv;
Expand All @@ -544,7 +548,7 @@ void RouteOrch::doTask(Consumer& consumer)

/* Resize the ip vector to match ifname vector
* as tokenize(",", ',') will miss the last empty segment. */
if (alsv.size() == 0)
if (alsv.size() == 0 && !blackhole)
{
SWSS_LOG_WARN("Skip the route %s, for it has an empty ifname field.", key.c_str());
it = consumer.m_toSync.erase(it);
Expand Down Expand Up @@ -597,7 +601,11 @@ void RouteOrch::doTask(Consumer& consumer)
string nhg_str = "";
NextHopGroupKey& nhg = ctx.nhg;

if (overlay_nh == false)
if (blackhole)
{
nhg = NextHopGroupKey();
}
else if (overlay_nh == false)
{
if (alsv[0] == "tun0" && !(IpAddress(ipv[0]).isZero()))
{
Expand Down Expand Up @@ -630,10 +638,8 @@ void RouteOrch::doTask(Consumer& consumer)

if (ipv.size() == 1 && IpAddress(ipv[0]).isZero())
{
/* blackhole to be done */
if (alsv[0] == "unknown")
{
/* add addBlackholeRoute or addRoute support empty nhg */
it = consumer.m_toSync.erase(it);
}
/* skip direct routes to tun0 */
Expand Down Expand Up @@ -1349,6 +1355,7 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
bool status = false;
bool curNhgIsFineGrained = false;
bool prevNhgWasFineGrained = false;
bool blackhole = false;

if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end())
{
Expand Down Expand Up @@ -1377,6 +1384,11 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
return false;
}
}
else if (nextHops.getSize() == 0)
{
/* The route is pointing to a blackhole */
blackhole = true;
}
else if (nextHops.getSize() == 1)
{
/* The route is pointing to a next hop */
Expand Down Expand Up @@ -1544,8 +1556,16 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
*/
if (it_route == m_syncdRoutes.at(vrf_id).end())
{
route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID;
route_attr.value.oid = next_hop_id;
if (blackhole)
{
route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION;
route_attr.value.s32 = SAI_PACKET_ACTION_DROP;
}
else
{
route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID;
route_attr.value.oid = next_hop_id;
}

/* Default SAI_ROUTE_ATTR_PACKET_ACTION is SAI_PACKET_ACTION_FORWARD */
object_statuses.emplace_back();
Expand All @@ -1559,8 +1579,8 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
}
else
{
/* Set the packet action to forward when there was no next hop (dropped) */
if (it_route->second.getSize() == 0)
/* Set the packet action to forward when there was no next hop (dropped) and not pointing to blackhole*/
if (it_route->second.getSize() == 0 && !blackhole)
{
route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION;
route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD;
Expand All @@ -1584,6 +1604,15 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
object_statuses.emplace_back();
gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr);
}

if (blackhole)
{
route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION;
route_attr.value.s32 = SAI_PACKET_ACTION_DROP;

object_statuses.emplace_back();
gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr);
}
}
return false;
}
Expand All @@ -1595,6 +1624,7 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey
const sai_object_id_t& vrf_id = ctx.vrf_id;
const IpPrefix& ipPrefix = ctx.ip_prefix;
bool isFineGrained = false;
bool blackhole = false;

const auto& object_statuses = ctx.object_statuses;

Expand All @@ -1612,6 +1642,11 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey
/* Route is pointing to Fine Grained ECMP nexthop group */
isFineGrained = true;
}
else if (nextHops.getSize() == 0)
{
/* The route is pointing to a blackhole */
blackhole = true;
}
else if (nextHops.getSize() == 1)
{
/* The route is pointing to a next hop */
Expand Down Expand Up @@ -1739,8 +1774,8 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey
{
sai_status_t status;

/* Set the packet action to forward when there was no next hop (dropped) */
if (it_route->second.getSize() == 0)
/* Set the packet action to forward when there was no next hop (dropped) and not pointing to blackhole */
if (it_route->second.getSize() == 0 && !blackhole)
{
status = *it_status++;
if (status != SAI_STATUS_SUCCESS)
Expand Down Expand Up @@ -1790,6 +1825,22 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey
}
}

if (blackhole)
{
/* Set the packet action to drop for blackhole routes */
status = *it_status++;
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set blackhole route %s with packet action drop, %d",
ipPrefix.to_string().c_str(), status);
task_process_status handle_status = handleSaiSetStatus(SAI_API_ROUTE, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}
}

SWSS_LOG_INFO("Post set route %s with next hop(s) %s",
ipPrefix.to_string().c_str(), nextHops.to_string().c_str());
}
Expand Down
61 changes: 61 additions & 0 deletions tests/test_route.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,13 @@ def _access_function():

wait_for_result(_access_function)

def get_asic_db_key(self, destination):
route_entries = self.adb.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY")
for route_entry in route_entries:
if json.loads(route_entry)["dest"] == destination:
return route_entry
return None

def check_route_entries_with_vrf(self, destinations, vrf_oids):
def _access_function():
route_entries = self.adb.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY")
Expand Down Expand Up @@ -839,6 +846,60 @@ def test_RouteAndNexthopInDifferentVrf(self, dvs, testlog):
dvs.servers[3].runcmd("ip route del default dev eth0")
dvs.servers[3].runcmd("ip address del 20.0.1.2/24 dev eth0")

def test_RouteAddRemoveIpv4BlackholeRoute(self, dvs, testlog):
self.setup_db(dvs)

self.clear_srv_config(dvs)

# add route entry
dvs.runcmd("vtysh -c \"configure terminal\" -c \"ip route 2.2.2.0/24 blackhole\"")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we add the route to config_db and test the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two reasons not to do it in this way: 1. It seems that bgpcfgd is not running in docker-sonic-vs. Without bgpcfgd the static route table in config_db will not be applied. 2. I feel these vs tests mainly aim to test the orchagent functionalities. If we choose to add it in config_db, it would include the bgpcfgd functions which are not currently in the scope of dvs tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree to point 1. But for 2, I would say then we could directly add to APP_DB, why even vtysh. IMO, if the infra is available, lets use that path to configure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Not sure if we are planning to update the infra to include the support configuring from config_db in the near future. If not, maybe we should keep it as is and change the configuration after the infra is ready.


# check application database
self.pdb.wait_for_entry("ROUTE_TABLE", "2.2.2.0/24")

# check ASIC route database
self.check_route_entries(["2.2.2.0/24"])
key = self.get_asic_db_key("2.2.2.0/24")
assert key
fvs = self.adb.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY", key)
assert fvs.get("SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION") == "SAI_PACKET_ACTION_DROP"

# remove route entry
dvs.runcmd("vtysh -c \"configure terminal\" -c \"no ip route 2.2.2.0/24 blackhole\"")

# check application database
self.pdb.wait_for_deleted_entry("ROUTE_TABLE", "2.2.2.0/24")

# check ASIC route database
self.check_deleted_route_entries(["2.2.2.0/24"])

def test_RouteAddRemoveIpv6BlackholeRoute(self, dvs, testlog):
self.setup_db(dvs)

self.clear_srv_config(dvs)

# add route entry
dvs.runcmd("vtysh -c \"configure terminal\" -c \"ipv6 route 3000::0/64 blackhole\"")

# check application database
self.pdb.wait_for_entry("ROUTE_TABLE", "3000::/64")

# check ASIC route database
self.check_route_entries(["3000::/64"])
key = self.get_asic_db_key("3000::/64")
assert key
fvs = self.adb.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY", key)
assert fvs.get("SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION") == "SAI_PACKET_ACTION_DROP"

# remove route entry
dvs.runcmd("vtysh -c \"configure terminal\" -c \"no ipv6 route 3000::0/64 blackhole\"")

# check application database
self.pdb.wait_for_deleted_entry("ROUTE_TABLE", "3000::/64")

# check ASIC route database
self.check_deleted_route_entries(["3000::/64"])

class TestRoutePerf(TestRouteBase):
""" Performance tests for route """
def test_PerfAddRemoveRoute(self, dvs, testlog):
Expand Down