diff mbox series

[ovs-dev] northd: Allow DHCP request from the lport if enabled DHCPv4

Message ID 20240416052131.1659-1-liuxie_11@163.com
State New
Headers show
Series [ovs-dev] northd: Allow DHCP request from the lport if enabled DHCPv4 | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Xie Liu April 16, 2024, 5:21 a.m. UTC 
From: shylou <liuxie_11@163.com>

DHCP for VM fails when removing default security group rules
using a CMS like Neutron ML2/OVN [1]. This is because DHCP
requests from VMs may be dropped by ACLs. To fix this issue,
we add a lflow with a priority of 34000 to allow DHCP requests
from the logical port if the CMS has enabled native DHCPv4
for this port.

[1]https://bugs.launchpad.net/neutron/+bug/1926515

Signed-off-by: Xie Liu <liushyshy@gmail.com>
---
 northd/northd.c     | 10 ++++++++++
 tests/ovn-northd.at |  5 +++++
 2 files changed, 15 insertions(+)

Comments

Numan Siddique April 18, 2024, 2:06 p.m. UTC | #1 
On Tue, Apr 16, 2024 at 1:22 AM <liuxie_11@163.com> wrote:

> From: shylou <liuxie_11@163.com>
>
> DHCP for VM fails when removing default security group rules
> using a CMS like Neutron ML2/OVN [1]. This is because DHCP
> requests from VMs may be dropped by ACLs. To fix this issue,
> we add a lflow with a priority of 34000 to allow DHCP requests
> from the logical port if the CMS has enabled native DHCPv4
> for this port.
>
> [1]https://bugs.launchpad.net/neutron/+bug/1926515
>
> Signed-off-by: Xie Liu <liushyshy@gmail.com>
>

Thanks for the patch.

I don't think this is the correct fix.  If neutron wants to allow DHCP
overriding any ACL rules,
it should add a high priority ACL to allow DHCP.   What if a user wants to
add an explicit ACL to
drop DHCP from certain ports ?

Thanks
Numan


---
>  northd/northd.c     | 10 ++++++++++
>  tests/ovn-northd.at |  5 +++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 2c3560ce2..ca641a19e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8414,6 +8414,16 @@ build_dhcpv4_options_flows(struct ovn_port *op,
>                                                       meter_groups),
>                                        &op->nbsp->dhcpv4_options->header_,
>                                        lflow_ref);
> +            /* Add 34000 priority flow to allow DHCP request from the
> lport
> +             * if the CMS has enabled native DHCPv4 for this lport.
> +             * */
> +            ovn_lflow_add_with_lport_and_hint(lflows, op->od,
> +                                              S_SWITCH_IN_ACL_EVAL, 34000,
> +                                              ds_cstr(&match),
> +                                              REGBIT_ACL_VERDICT_ALLOW" =
> 1; next;",
> +                                              op->key,
> +                                              &op->nbsp->header_,
> +                                              lflow_ref);
>              ds_clear(&match);
>
>              /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6fdd761da..7657aefff 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4897,6 +4897,11 @@ ovn-nbctl --wait=sb lsp-set-dhcpv4-options
> sw0-port1 $CIDR_UUID
>  ovn-sbctl dump-flows sw0 > sw0flows
>  AT_CAPTURE_FILE([sw0flows])
>
> +AT_CHECK([grep "_acl_eval" sw0flows | grep sw0-port1 | ovn_strip_lflows],
> [0], [dnl
> +  table=??(ls_in_acl_eval     ), priority=34000, 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=(reg8[[16]] = 1; next;)
> +  table=??(ls_out_acl_eval    ), priority=34000, match=(outport ==
> "sw0-port1" && eth.src == c0:ff:ee:00:00:01 && ip4.src == 10.0.0.1 && udp
> && udp.src == 67 && udp.dst == 68), action=(reg8[[16]] = 1; next;)
> +])
> +
>  AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | ovn_strip_lflows], [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 == {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;)
> --
> 2.42.0.windows.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
shy liu April 19, 2024, 3:19 a.m. UTC | #2 
>
>
>
> On Tue, Apr 16, 2024 at 1:22 AM <liuxie_11@163.com> wrote:
>>
>> From: shylou <liuxie_11@163.com>
>>
>> DHCP for VM fails when removing default security group rules
>> using a CMS like Neutron ML2/OVN [1]. This is because DHCP
>> requests from VMs may be dropped by ACLs. To fix this issue,
>> we add a lflow with a priority of 34000 to allow DHCP requests
>> from the logical port if the CMS has enabled native DHCPv4
>> for this port.
>>
>> [1]https://bugs.launchpad.net/neutron/+bug/1926515
>>
>> Signed-off-by: Xie Liu <liushyshy@gmail.com>
>
>
> Thanks for the patch.
>
> I don't think this is the correct fix.  If neutron wants to allow DHCP overriding any ACL rules,
> it should add a high priority ACL to allow DHCP.   What if a user wants to add an explicit ACL to
> drop DHCP from certain ports ?
>
> Thanks
> Numan
>
Thanks,  Numan
I have a puzzling question: Why would users want to block
DHCP request packets after enabling DHCP for any LSPs ?
They could justly not enable DHCP for them at all, right?
>
>> ---
>>  northd/northd.c     | 10 ++++++++++
>>  tests/ovn-northd.at |  5 +++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 2c3560ce2..ca641a19e 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -8414,6 +8414,16 @@ build_dhcpv4_options_flows(struct ovn_port *op,
>>                                                       meter_groups),
>>                                        &op->nbsp->dhcpv4_options->header_,
>>                                        lflow_ref);
>> +            /* Add 34000 priority flow to allow DHCP request from the lport
>> +             * if the CMS has enabled native DHCPv4 for this lport.
>> +             * */
>> +            ovn_lflow_add_with_lport_and_hint(lflows, op->od,
>> +                                              S_SWITCH_IN_ACL_EVAL, 34000,
>> +                                              ds_cstr(&match),
>> +                                              REGBIT_ACL_VERDICT_ALLOW" = 1; next;",
>> +                                              op->key,
>> +                                              &op->nbsp->header_,
>> +                                              lflow_ref);
>>              ds_clear(&match);
>>
>>              /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 6fdd761da..7657aefff 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -4897,6 +4897,11 @@ ovn-nbctl --wait=sb lsp-set-dhcpv4-options sw0-port1 $CIDR_UUID
>>  ovn-sbctl dump-flows sw0 > sw0flows
>>  AT_CAPTURE_FILE([sw0flows])
>>
>> +AT_CHECK([grep "_acl_eval" sw0flows | grep sw0-port1 | ovn_strip_lflows], [0], [dnl
>> +  table=??(ls_in_acl_eval     ), priority=34000, 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=(reg8[[16]] = 1; next;)
>> +  table=??(ls_out_acl_eval    ), priority=34000, match=(outport == "sw0-port1" && eth.src == c0:ff:ee:00:00:01 && ip4.src == 10.0.0.1 && udp && udp.src == 67 && udp.dst == 68), action=(reg8[[16]] = 1; next;)
>> +])
>> +
>>  AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | ovn_strip_lflows], [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 == {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;)
>> --
>> 2.42.0.windows.2
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Numan Siddique April 19, 2024, 2:52 p.m. UTC | #3 
On Thu, Apr 18, 2024 at 11:20 PM shy liu <liushyshy@gmail.com> wrote:

> >
> >
> >
> > On Tue, Apr 16, 2024 at 1:22 AM <liuxie_11@163.com> wrote:
> >>
> >> From: shylou <liuxie_11@163.com>
> >>
> >> DHCP for VM fails when removing default security group rules
> >> using a CMS like Neutron ML2/OVN [1]. This is because DHCP
> >> requests from VMs may be dropped by ACLs. To fix this issue,
> >> we add a lflow with a priority of 34000 to allow DHCP requests
> >> from the logical port if the CMS has enabled native DHCPv4
> >> for this port.
> >>
> >> [1]https://bugs.launchpad.net/neutron/+bug/1926515
> >>
> >> Signed-off-by: Xie Liu <liushyshy@gmail.com>
> >
> >
> > Thanks for the patch.
> >
> > I don't think this is the correct fix.  If neutron wants to allow DHCP
> overriding any ACL rules,
> > it should add a high priority ACL to allow DHCP.   What if a user wants
> to add an explicit ACL to
> > drop DHCP from certain ports ?
> >
> > Thanks
> > Numan
> >
> Thanks,  Numan
> I have a puzzling question: Why would users want to block
> DHCP request packets after enabling DHCP for any LSPs ?
> They could justly not enable DHCP for them at all, right?
>

When a user doesn't enable DHCP for an LSP,  it doesn't mean it will
be blocked.  It just means that OVN will not respond to that DHCP request.
The DHCP discovery/request packet will continue the pipeline and if there
is any DHCP server
configured, it will reply to it.  Whereas an ACL to block DHCP will just
drop the packet from that LSP
in the ACL pipeline.

Thanks
Numan



> >> ---
> >>  northd/northd.c     | 10 ++++++++++
> >>  tests/ovn-northd.at |  5 +++++
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index 2c3560ce2..ca641a19e 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -8414,6 +8414,16 @@ build_dhcpv4_options_flows(struct ovn_port *op,
> >>                                                       meter_groups),
> >>
> &op->nbsp->dhcpv4_options->header_,
> >>                                        lflow_ref);
> >> +            /* Add 34000 priority flow to allow DHCP request from the
> lport
> >> +             * if the CMS has enabled native DHCPv4 for this lport.
> >> +             * */
> >> +            ovn_lflow_add_with_lport_and_hint(lflows, op->od,
> >> +                                              S_SWITCH_IN_ACL_EVAL,
> 34000,
> >> +                                              ds_cstr(&match),
> >> +
> REGBIT_ACL_VERDICT_ALLOW" = 1; next;",
> >> +                                              op->key,
> >> +                                              &op->nbsp->header_,
> >> +                                              lflow_ref);
> >>              ds_clear(&match);
> >>
> >>              /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >> index 6fdd761da..7657aefff 100644
> >> --- a/tests/ovn-northd.at
> >> +++ b/tests/ovn-northd.at
> >> @@ -4897,6 +4897,11 @@ ovn-nbctl --wait=sb lsp-set-dhcpv4-options
> sw0-port1 $CIDR_UUID
> >>  ovn-sbctl dump-flows sw0 > sw0flows
> >>  AT_CAPTURE_FILE([sw0flows])
> >>
> >> +AT_CHECK([grep "_acl_eval" sw0flows | grep sw0-port1 |
> ovn_strip_lflows], [0], [dnl
> >> +  table=??(ls_in_acl_eval     ), priority=34000, 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=(reg8[[16]] = 1; next;)
> >> +  table=??(ls_out_acl_eval    ), priority=34000, match=(outport ==
> "sw0-port1" && eth.src == c0:ff:ee:00:00:01 && ip4.src == 10.0.0.1 && udp
> && udp.src == 67 && udp.dst == 68), action=(reg8[[16]] = 1; next;)
> >> +])
> >> +
> >>  AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | ovn_strip_lflows],
> [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 == {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;)
> >> --
> >> 2.42.0.windows.2
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index 2c3560ce2..ca641a19e 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8414,6 +8414,16 @@  build_dhcpv4_options_flows(struct ovn_port *op,
                                                      meter_groups),
                                       &op->nbsp->dhcpv4_options->header_,
                                       lflow_ref);
+            /* Add 34000 priority flow to allow DHCP request from the lport
+             * if the CMS has enabled native DHCPv4 for this lport.
+             * */
+            ovn_lflow_add_with_lport_and_hint(lflows, op->od,
+                                              S_SWITCH_IN_ACL_EVAL, 34000,
+                                              ds_cstr(&match),
+                                              REGBIT_ACL_VERDICT_ALLOW" = 1; next;",
+                                              op->key,
+                                              &op->nbsp->header_,
+                                              lflow_ref);
             ds_clear(&match);
 
             /* If REGBIT_DHCP_OPTS_RESULT is set, it means the
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 6fdd761da..7657aefff 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4897,6 +4897,11 @@  ovn-nbctl --wait=sb lsp-set-dhcpv4-options sw0-port1 $CIDR_UUID
 ovn-sbctl dump-flows sw0 > sw0flows
 AT_CAPTURE_FILE([sw0flows])
 
+AT_CHECK([grep "_acl_eval" sw0flows | grep sw0-port1 | ovn_strip_lflows], [0], [dnl
+  table=??(ls_in_acl_eval     ), priority=34000, 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=(reg8[[16]] = 1; next;)
+  table=??(ls_out_acl_eval    ), priority=34000, match=(outport == "sw0-port1" && eth.src == c0:ff:ee:00:00:01 && ip4.src == 10.0.0.1 && udp && udp.src == 67 && udp.dst == 68), action=(reg8[[16]] = 1; next;)
+])
+
 AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | ovn_strip_lflows], [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 == {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;)