Skip to content

Commit

Permalink
Reduce number of DHCP responder flows for LSPs
Browse files Browse the repository at this point in the history 
Currently for every LSP two DHCP responder flows are created. These two
flows are almost exactly the same differing only in the match.

ex.
Unicast flow (for RENEW/REBIND):
"match":"inport == \"0e4b7821-b5ae-4bff-9424-d7245c679150\" && eth.src == fa:16:3e:68:5e:c1 && ip4.src == 1.65.5.169 && ip4.dst == {1.65.5.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67"

Broadcast flow (for DISCOVER):
"match":"inport == \"0e4b7821-b5ae-4bff-9424-d7245c679150\" && eth.src == fa:16:3e:68:5e:c1 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67"

I consolidated the flows to use the conjunctive match (ip4.src == {1.65.5.169, 0.0.0.0}  && ip4.dst == {1.65.5.1, 255.255.255.255})

there is potential for a faulty DHCP discovery and DHCP Request packet getting through
  - added a check in pinctrl.c to check for illegal dhcp discovery packet sending to host

Submitted-at: #224
Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from commit 2132acb)
  • Loading branch information
@JacobTanenbaum @numansiddique
JacobTanenbaum authored and numansiddique committed Nov 25, 2024
1 parent c51bf57 commit faeb969
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 29 deletions.
1 change: 1 addition & 0 deletions AUTHORS.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,7 @@ Hyojoon Kim joonk@gatech.edu
Igor Ganichev
Igor Sever igor@xorops.com
Jacob Cherkas cherkasj@vmware.com
Jacob Tanenbaum jtanenba@redhat.com
Jad Naous jnaous@gmail.com
Jamal Hadi Salim hadi@cyberus.ca
James Schmidt jschmidt@vmware.com
Expand Down
5 changes: 5 additions & 0 deletions controller/pinctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2053,6 +2053,11 @@ pinctrl_handle_put_dhcp_opts(
switch (*in_dhcp_msg_type) {
case DHCP_MSG_DISCOVER:
msg_type = DHCP_MSG_OFFER;
if (in_flow->nw_dst != htonl(INADDR_BROADCAST)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "DHCP DISCOVER must be Broadcast");
goto exit;
}
break;
case DHCP_MSG_REQUEST: {
msg_type = DHCP_MSG_ACK;
Expand Down
23 changes: 2 additions & 21 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6726,7 +6726,8 @@ build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
server_mac, server_ip);

ds_put_format(ipv4_addr_match,
"ip4.src == "IP_FMT" && ip4.dst == {%s, 255.255.255.255}",
"(ip4.src == {"IP_FMT", 0.0.0.0} "
"&& ip4.dst == {%s, 255.255.255.255})",
IP_ARGS(offer_ip), server_ip);
smap_destroy(&dhcpv4_options);
return true;
Expand Down Expand Up @@ -9454,27 +9455,7 @@ build_dhcpv4_options_flows(struct ovn_port *op,
op, lsp_addrs->ipv4_addrs[j].addr,
&options_action, &response_action, &ipv4_addr_match)) {
ds_clear(&match);
ds_put_format(
&match, "inport == %s && eth.src == %s && "
"ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && "
"udp.src == 68 && udp.dst == 67",
inport->json_key, lsp_addrs->ea_s);

if (is_external) {
ds_put_format(&match, " && is_chassis_resident(%s)",
op->json_key);
}

ovn_lflow_add_with_hint__(lflows, op->od,
S_SWITCH_IN_DHCP_OPTIONS, 100,
ds_cstr(&match),
ds_cstr(&options_action),
inport->key,
copp_meter_get(COPP_DHCPV4_OPTS,
op->od->nbs->copp,
meter_groups),
&op->nbsp->dhcpv4_options->header_);
ds_clear(&match);
/* Allow ip4.src = OFFER_IP and
* ip4.dst = {SERVER_IP, 255.255.255.255} for the below
* cases
Expand Down
9 changes: 3 additions & 6 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4817,8 +4817,7 @@ AT_CAPTURE_FILE([sw0flows])

AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl
table=??(ls_in_dhcp_options ), priority=0 , match=(1), action=(next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 10.0.0.2 && ip4.dst == {10.0.0.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2, 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
])

check ovn-nbctl --wait=sb lsp-set-options sw0-port1 hostname="\"port1\""
Expand All @@ -4827,8 +4826,7 @@ AT_CAPTURE_FILE([sw0flows])

AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl
table=??(ls_in_dhcp_options ), priority=0 , match=(1), action=(next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "port1", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 10.0.0.2 && ip4.dst == {10.0.0.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "port1", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2, 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "port1", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
])

ovn-nbctl dhcp-options-set-options $CIDR_UUID lease_time=3600 router=10.0.0.1 server_id=10.0.0.1 server_mac=c0:ff:ee:00:00:01
Expand All @@ -4838,8 +4836,7 @@ AT_CAPTURE_FILE([sw0flows])

AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl
table=??(ls_in_dhcp_options ), priority=0 , match=(1), action=(next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "bar", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 10.0.0.2 && ip4.dst == {10.0.0.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "bar", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2, 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "bar", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;)
])

AT_CLEANUP
Expand Down
4 changes: 2 additions & 2 deletions tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19772,7 +19772,7 @@ wait_for_ports_up ls1-lp_ext1
as hv1 ovs-ofctl dump-flows br-int > brintflows
AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | \
grep controller | grep "0a.00.00.06" | grep reg14=0x$ln_public_key | \
wc -l], [0], [3
wc -l], [0], [1
])
AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | \
grep controller | grep tp_src=546 | grep \
Expand Down Expand Up @@ -20024,7 +20024,7 @@ wait_for_ports_up ls1-lp_ext1
# There should be OF flows for DHCP4/v6 for the ls1-lp_ext1 port in hv2
AT_CHECK([as hv2 ovs-ofctl dump-flows br-int | \
grep controller | grep "0a.00.00.06" | grep reg14=0x$ln_public_key | \
wc -l], [0], [3
wc -l], [0], [1
])
AT_CHECK([as hv2 ovs-ofctl dump-flows br-int | \
grep controller | grep tp_src=546 | grep \
Expand Down

0 comments on commit faeb969

Please sign in to comment.