-
Notifications
You must be signed in to change notification settings - Fork 545
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.
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
SwSS Changes for DHCP DoS Mitigation Feature #3130
base: master
Are you sure you want to change the base?
Changes from all commits
5a8b39f
0fb822a
dc20bf1
903a89d
132a6e0
312356d
eb52c01
4d10719
6ec063c
703e8a5
79fea07
5279d61
c89f3d4
d167ade
b761b27
d22882a
893c800
462c777
768e1c9
01331df
19cdea5
4c2135a
d5cf62b
11f2592
5b850db
d079550
f71c0a7
e242495
3c7139a
774eccd
264efe0
14b0a66
7e2c5b2
04c723f
5a74163
9337839
6a70578
ef712b5
e9881aa
b7c1fc4
c640432
4ead285
a14dad0
7036b0f
246d96a
3381caf
07a72f4
f3a94c5
0c51f71
1b1cc19
d2d94c9
efa1b55
05ea555
68dadc9
fc7fff5
18e0f71
1690ddc
400c702
18fb715
fb0e7e1
aaa4f73
3e3fa50
5e68783
0a42fe6
58fd4b3
0a30441
855217a
b7ad4d8
1980e65
67d31c9
710e6aa
9d65c51
062ca41
a189347
45c0287
50a5d01
f892368
566cf98
c1dbc9c
e53e05c
e00d1d1
ef7d365
ed8c0c1
c31c19c
03e41ae
b974eee
19f8424
dae8b75
a19e91a
90cd544
eaf0c2e
d5f4f1a
9d61b92
9d40e2b
6d7ddff
d4b5f75
4e03068
adf5682
8903a1a
8434878
7ab9069
2089cbe
17b8e3c
08afacc
0facc0a
ea59664
74d19c1
4649e15
a01f989
0b22bfb
0d0353b
54aab00
548d4c9
a69d1e4
0d35b35
993ced8
4978f89
b3beb1f
9114f52
6a59c15
24d943a
7cf982f
6f6f5b1
029201e
3552d82
6011d06
531f590
71ac850
60eef36
934b537
27deb39
6b9fae7
fd88d5c
c8d2a23
2590581
4159740
9d3bf33
eef26a6
0116750
8ef09c0
610311e
158d51b
40f5d38
f0d5d32
1d683cc
36e4c75
1c4daf6
6abc4da
9b94bea
bd8cf95
2eb9e75
9e22d55
cc5e246
fb25005
bea9d80
1a4c03e
a3d6b2d
e1b1b8d
0954291
d69faa2
f1ab66b
98ebf69
4aa6953
eac36b8
d28829a
4443dd8
76a85d0
18dec77
e11f7da
bb8b3de
82f7152
ad6b25e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
#include "exec.h" | ||
#include "shellcmd.h" | ||
#include <swss/redisutility.h> | ||
#include <iostream> | ||
|
||
using namespace std; | ||
using namespace swss; | ||
|
@@ -76,6 +77,51 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up) | |
return true; | ||
} | ||
|
||
bool PortMgr::setPortDHCPMitigationRate(const string &alias, const string &dhcp_rate_limit) | ||
{ | ||
stringstream cmd; | ||
string res, cmd_str; | ||
int ret; | ||
int byte_rate = stoi(dhcp_rate_limit) * DHCP_PACKET_SIZE; | ||
|
||
if (dhcp_rate_limit != "0") | ||
{ | ||
/* tc qdisc add dev <port_name> handle ffff: ingress | ||
&& | ||
tc filter add dev <port_name> protocol ip parent ffff: prio 1 u32 match ip protocol 17 0xff match ip dport 67 0xffff police rate <byte_rate>bps burst <byte_rate>b conform-exceed drop*/ | ||
cmd << TC_CMD << " qdisc add dev " << shellquote(alias) << " handle ffff: ingress" << " && " \ | ||
<< TC_CMD << " filter add dev " << shellquote(alias) << " protocol ip parent ffff: prio 1 u32 match ip protocol 17 0xff match ip dport 67 0xffff police rate " << to_string(byte_rate) << "bps burst " << to_string(byte_rate) << "b conform-exceed drop"; | ||
cmd_str = cmd.str(); | ||
ret = swss::exec(cmd_str, res); | ||
if (!ret) | ||
{ | ||
SWSS_LOG_INFO("writing dhcp_rate_limit to appl_db"); | ||
return writeConfigToAppDb(alias, "dhcp_rate_limit", dhcp_rate_limit); | ||
} | ||
else if (!isPortStateOk(alias)) | ||
{ | ||
// Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notif | ||
SWSS_LOG_WARN("Setting dhcp_rate_limit to alias:%s netdev failed with cmd:%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str()); | ||
return false; | ||
} | ||
} | ||
else | ||
{ | ||
// tc qdisc del dev <port_name> handle ffff: ingress | ||
cmd << TC_CMD << " qdisc del dev " << shellquote(alias) << " handle ffff: ingress"; | ||
cmd_str = cmd.str(); | ||
ret = swss::exec(cmd_str, res); | ||
if (ret) | ||
{ | ||
// Log the failure and return false to indicate an issue | ||
SWSS_LOG_WARN("Failed to delete ingress qdisc "); | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
bool PortMgr::isPortStateOk(const string &alias) | ||
{ | ||
vector<FieldValueTuple> temp; | ||
|
@@ -155,19 +201,19 @@ void PortMgr::doTask(Consumer &consumer) | |
*/ | ||
bool portOk = isPortStateOk(alias); | ||
|
||
string admin_status, mtu; | ||
string admin_status, mtu, dhcp_rate_limit; | ||
std::vector<FieldValueTuple> field_values; | ||
|
||
bool configured = (m_portList.find(alias) != m_portList.end()); | ||
|
||
/* If this is the first time we set port settings | ||
* assign default admin status and mtu | ||
* assign default admin status and mtu and dhcp_rate_limit | ||
*/ | ||
if (!configured) | ||
{ | ||
admin_status = DEFAULT_ADMIN_STATUS_STR; | ||
mtu = DEFAULT_MTU_STR; | ||
|
||
dhcp_rate_limit = DEFAULT_DHCP_RATE_LIMIT_STR; | ||
m_portList.insert(alias); | ||
} | ||
else if (!portOk) | ||
|
@@ -182,6 +228,11 @@ void PortMgr::doTask(Consumer &consumer) | |
{ | ||
mtu = fvValue(i); | ||
} | ||
else if (fvField(i) == "dhcp_rate_limit") | ||
{ | ||
dhcp_rate_limit = fvValue(i); | ||
|
||
} | ||
else if (fvField(i) == "admin_status") | ||
{ | ||
admin_status = fvValue(i); | ||
|
@@ -205,30 +256,37 @@ void PortMgr::doTask(Consumer &consumer) | |
{ | ||
writeConfigToAppDb(alias, field_values); | ||
} | ||
|
||
if (!portOk) | ||
{ | ||
SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str()); | ||
writeConfigToAppDb(alias, "mtu", mtu); | ||
writeConfigToAppDb(alias, "admin_status", admin_status); | ||
writeConfigToAppDb(alias, "dhcp_rate_limit", dhcp_rate_limit); | ||
|
||
/* Retry setting these params after the netdev is created */ | ||
field_values.clear(); | ||
field_values.emplace_back("mtu", mtu); | ||
field_values.emplace_back("admin_status", admin_status); | ||
field_values.emplace_back("dhcp_rate_limit", dhcp_rate_limit); | ||
|
||
it->second = KeyOpFieldsValuesTuple{alias, SET_COMMAND, field_values}; | ||
it++; | ||
continue; | ||
} | ||
|
||
if (!mtu.empty()) | ||
{ | ||
setPortMtu(alias, mtu); | ||
SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str()); | ||
} | ||
|
||
if (!admin_status.empty()) | ||
{ | ||
setPortAdminStatus(alias, admin_status == "up"); | ||
SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str()); | ||
} | ||
if (!dhcp_rate_limit.empty()) | ||
{ | ||
setPortDHCPMitigationRate(alias, dhcp_rate_limit); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be set only if there is explicit configuration of |
||
SWSS_LOG_NOTICE("Configure %s DHCP rate limit to %s", alias.c_str(), dhcp_rate_limit.c_str()); | ||
} | ||
} | ||
else if (op == DEL_COMMAND) | ||
|
@@ -256,4 +314,4 @@ bool PortMgr::writeConfigToAppDb(const std::string &alias, std::vector<FieldValu | |
{ | ||
m_appPortTable.set(alias, field_values); | ||
return true; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What changed here? Please remove unintended changes wrt spacing and new lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed white spaces and new lines |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ namespace swss { | |
/* Port default admin status is down */ | ||
#define DEFAULT_ADMIN_STATUS_STR "down" | ||
#define DEFAULT_MTU_STR "9100" | ||
#define DEFAULT_DHCP_RATE_LIMIT_STR "300" | ||
#define DHCP_PACKET_SIZE 406 | ||
|
||
class PortMgr : public Orch | ||
{ | ||
|
@@ -36,7 +38,7 @@ class PortMgr : public Orch | |
bool writeConfigToAppDb(const std::string &alias, std::vector<FieldValueTuple> &field_values); | ||
bool setPortMtu(const std::string &alias, const std::string &mtu); | ||
bool setPortAdminStatus(const std::string &alias, const bool up); | ||
bool setPortDHCPMitigationRate(const std::string &alias, const std::string &dhcp_rate_limit); | ||
bool isPortStateOk(const std::string &alias); | ||
}; | ||
|
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix newline? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1270,9 +1270,7 @@ bool PortHelper::validatePortConfig(PortConfig &port) const | |
|
||
port.admin_status.value = false; | ||
port.admin_status.is_set = true; | ||
|
||
port.fieldValueMap[PORT_ADMIN_STATUS] = PORT_STATUS_DOWN; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please fix all these unintended changes.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
|
||
return true; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,7 +138,7 @@ def _verify_db_contents(): | |
return (True, None) | ||
|
||
# Verify that ASIC DB has been fully initialized | ||
init_polling_config = PollingConfig(2, 30, strict=True) | ||
init_polling_config = PollingConfig(2, 40, strict=True) | ||
wait_for_result(_verify_db_contents, init_polling_config) | ||
|
||
def _generate_oid_to_interface_mapping(self) -> None: | ||
|
@@ -1152,6 +1152,13 @@ def set_mtu(self, interface, mtu): | |
tbl.set(interface, fvs) | ||
time.sleep(1) | ||
|
||
def set_dhcp_rate_limit(self, interface, dhcp_rate_limit): | ||
tbl_name = "PORT" | ||
tbl = swsscommon.Table(self.cdb, tbl_name) | ||
fvs = swsscommon.FieldValuePairs([("dhcp_rate_limit", dhcp_rate_limit)]) | ||
tbl.set(interface, fvs) | ||
time.sleep(20) | ||
|
||
# deps: acl, mirror_port_erspan | ||
def add_neighbor(self, interface, ip, mac): | ||
tbl = swsscommon.ProducerStateTable(self.pdb, "NEIGH_TABLE") | ||
|
@@ -1701,12 +1708,10 @@ def get_topo_neigh(self): | |
def handle_neighconn(self): | ||
if self.oper != "create": | ||
return | ||
|
||
instance_to_neighbor_map = self.get_topo_neigh() | ||
for ctnname, nbraddrs in instance_to_neighbor_map.items(): | ||
if ctnname not in self.dvss: | ||
continue | ||
|
||
for server, neighbor_address in nbraddrs: | ||
self.dvss[ctnname].servers[server].runcmd("ifconfig eth0 down") | ||
self.dvss[ctnname].servers[server].runcmd("ifconfig eth0 up") | ||
|
@@ -1863,6 +1868,7 @@ def update_dvs(log_path, new_dvs_env=[]): | |
dvs.destroy_servers() | ||
dvs.create_servers() | ||
dvs.restart() | ||
time.sleep(60) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need sleep here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a delay here to ensure that once the DVS switch starts, there is a pause before it performs any further actions, allowing sufficient time for the ports to initialize properly. |
||
|
||
return dvs | ||
|
||
|
@@ -1878,6 +1884,7 @@ def update_dvs(log_path, new_dvs_env=[]): | |
if dvs.persistent: | ||
dvs.runcmd("mv /etc/sonic/config_db.json.orig /etc/sonic/config_db.json") | ||
dvs.ctn_restart() | ||
time.sleep(60) | ||
|
||
@pytest.fixture(scope="module") | ||
def dvs(request, manage_dvs) -> DockerVirtualSwitch: | ||
|
@@ -2038,4 +2045,4 @@ def dpb_setup_fixture(dvs): | |
else: | ||
dvs.vct.restart() | ||
yield | ||
remove_dpb_config_file(dvs) | ||
remove_dpb_config_file(dvs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,11 +52,9 @@ def wait_for_result( | |
|
||
if status: | ||
return (True, result) | ||
|
||
time.sleep(polling_config.polling_interval) | ||
|
||
if polling_config.strict: | ||
message = failure_message or f"Operation timed out after {polling_config.timeout} seconds with result {result}" | ||
assert False, message | ||
|
||
return (False, result) | ||
return (False, result) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix new line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we not checking the value of ret here? What is the action if this command fails? Does it need to be logged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added logs on command failures