Message ID | 20240416052131.1659-1-liuxie_11@163.com |
---|---|
State | Rejected |
Headers | show |
Series | [ovs-dev] northd: Allow DHCP request from the lport if enabled DHCPv4 | expand |
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 |
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 > >
> > > > 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 >>
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 --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;)