reduce number of DHCP responder flows for LSPs

[JacobTanenbaum]

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})

OVN may now respond to illegal DHCP packets like src=0.0.0.0/dst=1.65.5.1 but we have COPP for this and we should be able to drop the illegal packets in ovn-controller

[dceara]

Thanks for the PR! The northd changes make sense; but we need to make sure we don't change the behavior of only replying if the external port is bound locally.

However, for this part:

OVN may now respond to illegal DHCP packets like src=0.0.0.0/dst=1.65.5.1 but we have COPP for this and we should be able to drop the illegal packets in ovn-controller

AFAICT pinctrl (ovn-controller) doesn't check for illegal packets like that. Don't we need to add a check there too?

LE: removed the note about external port, that was already how the PR behaved.

[JacobTanenbaum]

@dceara does the check in pinctrl.c fit the required check?

[dceara]

@dceara does the check in pinctrl.c fit the required check?

Don't we also need checks for DHCP_MSG_REQUEST?

[dceara]

LGTM overall but needs a few minor changes (reported by checkpatch):

$ ./utilities/checkpatch.py -1
== Checking 8a77a6226b7f ("reduce number of DHCP responder flows for LSPs") ==
ERROR: Author Jacob Tanenbaum <[email protected]> needs to sign off.
WARNING: Line is 88 characters long (recommended limit is 79)
#49 FILE: controller/pinctrl.c:2058:
            VLOG_WARN_RL(&rl, "DHCP REQUEST cannot be sent from an unassigned address");

WARNING: Line is 90 characters long (recommended limit is 79)
#64 FILE: northd/northd.c:6808:
                  "(ip4.src == {"IP_FMT", 0.0.0.0}  && ip4.dst == {%s, 255.255.255.255})",

Lines checked: 155, Warnings: 2, Errors: 1

[JacobTanenbaum]

@dceara fixed the checkpath issues

[dceara]

@dceara does the check in pinctrl.c fit the required check?

Don't we also need checks for DHCP_MSG_REQUEST?

Just to close the loop on this one, it's valid to get requests from both unassigned (0.0.0.0) and unicast IPs.

[dceara]

This LGTM, @numansiddique wdyt, do you mind having a second look?

[numansiddique]

Ack. I'll take a look

[numansiddique]

I've submitted the patch to the mailing list - https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/

I'll apply the patch after taking a look.

[numansiddique]

Applied the patch manually. Closing the PR.

@JacobTanenbaum - Thanks for the contribution. Added your name to AUTHORS.rst too