Regards, Vladislav Odintsov
> On 18 Aug 2022, at 20:07, Han Zhou <hz...@ovn.org> wrote: > > > > On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov <odiv...@gmail.com > <mailto:odiv...@gmail.com>> wrote: > Hi Han, > > As I could understand, admission control flows were introduced for > "gateway/outside" LSs, with GW LRPs. > The scenario, which was covered with this change shouldn’t overlap with a > "gateway/outside" LS scenario: > User creates an LS, adds normal VIF ports (VMs) to LS and additionally he may > want to attach vtep lport to this LS to extend L2 domain outside of the OVN > with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this could > be the only one problem place. > > Thanks for explaining, but I am still confused. Here is your original > description of the problem from the earlier thread: > --- > The initial problem is that user may want to create simple topology: > > <vm1> - <ls1 192.168.0.0/24 <http://192.168.0.0/24>> - <LR> - <ls2 > 192.168.1.0/24 <http://192.168.1.0/24>> - <vm2> > > And add VTEP LSP to ls2 (setting GW chassis to LRP in ls2). > If user wants to add GW services to this setup, VMs from ls2 will not be able > to access > external network due to lr_in_admission rules. > --- > So, the LRP to LS2 (Let's call it LRP-LS2) is the one with GW chassis set, > which means it is L3 GW LRP (DGP), right? LS2 has a DGP, a VTEP, and a VIF > (VM2), and you want to allow traffic to LRP-LS2 not only on the GW chassis, > but also on any HVs, right? Maybe you are saying, you don't connect a > localnet port to the LS2, and LS2 is an overlay network, instead of underlay? Yes this is exactly what I mean. > But if that's the case why would you set the gateway-chassis to it at all? This is the only way how to enable routing with HW VTEP gateway (which is a L2 service). You can check this out [1] to see more details about GW chassis for the VTEP L3 scenario. 1: https://github.com/ovn-org/ovn/commit/4e90bcf55c2ef1ab9940836455e4bfe36e57f307 > > Thanks, > han > > And this is_chassis_redirect() check is not added to flow only if VTEP lport > is in associated LS. All other cases left untouched. > > Let me know if you have any questions. > > Regards, > Vladislav Odintsov > >> On 18 Aug 2022, at 18:45, Han Zhou <hz...@ovn.org <mailto:hz...@ovn.org>> >> wrote: >> >> >> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <vlodint...@croc.ru >> <mailto:vlodint...@croc.ru>> wrote: >> > >> > Thanks Numan. >> > >> > regards, >> > Vladislav Odintsov >> > >> > > On 16 Aug 2022, at 04:56, Numan Siddique <num...@ovn.org >> > > <mailto:num...@ovn.org>> wrote: >> > > >> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odiv...@gmail.com >> > > <mailto:odiv...@gmail.com>> wrote: >> > >> >> > >> If LRP's logical switch has attached LSP of vtep type, the >> > >> is_chassis_resident() part is not added to lflow to allow traffic >> > >> originated from logical switch to reach LR services (LBs, NAT). >> > >> >> >> Sorry that I just noticed this and have a question here. I think we had >> is_chassis_resident() for the admission control flows for a reason. I don't >> remember all details, but probably one of the reasons is to prevent underlay >> BUM packets to be handled by multiple chassises? >> If we disable that check, would it create problems? Is VTEP attached LS >> immune to those problems? >> >> Thanks, >> Han >> >> > >> > >> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com >> > >> <mailto:odiv...@gmail.com>> >> > > >> > > Thanks for v2 and for the test case. >> > > Applied the patch to main. >> > > >> > > Numan >> > > >> > > >> > >> --- >> > >> This is a continuation from [1] as a v2 edition after Numan's review. >> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing option >> > >> 'is_distributed' >> > >> - added related tests >> > >> - updated ovn-northd flows docs >> > >> >> > >> 1: >> > >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html >> > >> <https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html> >> > >> --- >> > >> northd/northd.c | 33 +++++++++++++++++--- >> > >> northd/ovn-northd.8.xml | 4 +++ >> > >> tests/ovn-northd.at <http://ovn-northd.at/> | 69 >> > >> +++++++++++++++++++++++++++++++++++++++++ >> > >> 3 files changed, 102 insertions(+), 4 deletions(-) >> > >> >> > >> diff --git a/northd/northd.c b/northd/northd.c >> > >> index facd41a59..b1e9ffc87 100644 >> > >> --- a/northd/northd.c >> > >> +++ b/northd/northd.c >> > >> @@ -637,6 +637,7 @@ struct ovn_datapath { >> > >> bool has_lb_vip; >> > >> bool has_unknown; >> > >> bool has_acls; >> > >> + bool has_vtep_lports; >> > >> >> > >> /* IPAM data. */ >> > >> struct ipam_info ipam_info; >> > >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct >> > >> nbrec_logical_switch_port *nbsp) >> > >> return !strcmp(nbsp->type, "localnet"); >> > >> } >> > >> >> > >> +static bool >> > >> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp) >> > >> +{ >> > >> + return !strcmp(nbsp->type, "vtep"); >> > >> +} >> > >> + >> > >> static bool >> > >> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp) >> > >> { >> > >> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input >> > >> *input_data, >> > >> od->localnet_ports[od->n_localnet_ports++] = op; >> > >> } >> > >> >> > >> + if (lsp_is_vtep(nbsp)) { >> > >> + od->has_vtep_lports = true; >> > >> + } >> > >> + >> > >> op->lsp_addrs >> > >> = xmalloc(sizeof *op->lsp_addrs * >> > >> nbsp->n_addresses); >> > >> for (size_t j = 0; j < nbsp->n_addresses; j++) { >> > >> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op, >> > >> struct hmap *lflows, >> > >> ds_put_format(actions, "set_queue(%s); ", queue_id); >> > >> } >> > >> >> > >> - if (!strcmp(op->nbsp->type, "vtep")) { >> > >> + if (lsp_is_vtep(op->nbsp)) { >> > >> ds_put_format(actions, REGBIT_FROM_RAMP" = 1; "); >> > >> ds_put_format(actions, "next(pipeline=ingress, table=%d);", >> > >> ovn_stage_get_table(S_SWITCH_IN_HAIRPIN)); >> > >> @@ -10894,6 +10905,22 @@ build_gateway_mtu_flow(struct hmap *lflows, >> > >> struct ovn_port *op, >> > >> va_end(extra_actions_args); >> > >> } >> > >> >> > >> +static bool >> > >> +consider_l3dwg_port_is_centralized(struct ovn_port *op) >> > >> +{ >> > >> + if (op->peer && op->peer->od->has_vtep_lports) { >> > >> + return false; >> > >> + } >> > >> + >> > >> + if (is_l3dgw_port(op)) { >> > >> + /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s >> > >> + * should only be received on the gateway chassis. */ >> > >> + return true; >> > >> + } >> > >> + >> > >> + return false; >> > >> +} >> > >> + >> > >> /* Logical router ingress Table 0: L2 Admission Control >> > >> * This table drops packets that the router shouldn’t see at all based >> > >> * on their Ethernet headers. >> > >> @@ -10930,9 +10957,7 @@ build_adm_ctrl_flows_for_lrouter_port( >> > >> ds_clear(match); >> > >> ds_put_format(match, "eth.dst == %s && inport == %s", >> > >> op->lrp_networks.ea_s, op->json_key); >> > >> - if (is_l3dgw_port(op)) { >> > >> - /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s >> > >> - * should only be received on the gateway chassis. */ >> > >> + if (consider_l3dwg_port_is_centralized(op)) { >> > >> ds_put_format(match, " && is_chassis_resident(%s)", >> > >> op->cr_port->json_key); >> > >> } >> > >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> > >> index ff21c0737..9b6459d9e 100644 >> > >> --- a/northd/ovn-northd.8.xml >> > >> +++ b/northd/ovn-northd.8.xml >> > >> @@ -2114,6 +2114,10 @@ output; >> > >> gateway chassis), the above flow matching >> > >> <code>eth.dst == <var>E</var></code> is only programmed on >> > >> the gateway port instance on the gateway chassis. >> > >> + If LRP's logical switch has attached LSP of >> > >> <code>vtep</code> type, >> > >> + the <code>is_chassis_resident()</code> part is not added to >> > >> lflow to >> > >> + allow traffic originated from logical switch to reach LR >> > >> services >> > >> + (LBs, NAT). >> > >> </p> >> > >> >> > >> <p> >> > >> diff --git a/tests/ovn-northd.at <http://ovn-northd.at/> >> > >> b/tests/ovn-northd.at <http://ovn-northd.at/> >> > >> index 5b5eeb0ee..3ffa39419 100644 >> > >> --- a/tests/ovn-northd.at <http://ovn-northd.at/> >> > >> +++ b/tests/ovn-northd.at <http://ovn-northd.at/> >> > >> @@ -5747,6 +5747,75 @@ AT_CHECK([grep lr_in_gw_redirect lrflows | grep >> > >> cr-DR | sed 's/table=../table=?? >> > >> AT_CLEANUP >> > >> ]) >> > >> >> > >> +OVN_FOR_EACH_NORTHD([ >> > >> +AT_SETUP([ovn-northd -- lr admission with vtep lports]) >> > >> +AT_KEYWORDS([multiple-l3dgw-ports]) >> > >> +ovn_start NORTHD_TYPE >> > >> +check ovn-sbctl chassis-add ch1 geneve 127.0.0.2 >> > >> + >> > >> +check ovn-nbctl lr-add lr1 >> > >> +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24 >> > >> <http://10.0.0.1/24> >> > >> +check ovn-nbctl ls-add ls1 >> > >> +check ovn-nbctl lsp-add ls1 lsp1 -- \ >> > >> + lsp-set-addresses lsp1 router -- \ >> > >> + lsp-set-type lsp1 router -- \ >> > >> + lsp-set-options lsp1 router-port=lrp1 >> > >> + >> > >> +# ensure initial flows are installed without is_chassis_resident match >> > >> part >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> > >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> > >> 00:00:00:00:00:01 && inport == "lrp1"), action=(xreg0[[0..47]] = >> > >> 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast && >> > >> inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +# make lrp a cr-port and check its flows >> > >> +check ovn-nbctl lrp-set-gateway-chassis lrp1 ch1 >> > >> + >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> > >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> > >> 00:00:00:00:00:01 && inport == "lrp1" && >> > >> is_chassis_resident("cr-lrp1")), action=(xreg0[[0..47]] = >> > >> 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast && >> > >> inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +# attach vtep logical port to logical switch and check flows. >> > >> +# there should not be is_chassis_resident part. >> > >> +check ovn-nbctl lsp-add ls1 lsp-vtep -- lsp-set-type lsp-vtep vtep >> > >> + >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> > >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> > >> 00:00:00:00:00:01 && inport == "lrp1"), action=(xreg0[[0..47]] = >> > >> 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast && >> > >> inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +# delete vtep lport and check lrp has is_chassis_resident match part >> > >> again. >> > >> +check ovn-nbctl lsp-del lsp-vtep >> > >> + >> > >> +ovn-nbctl --wait=sb sync >> > >> +ovn-sbctl dump-flows lr1 > lrflows >> > >> +AT_CAPTURE_FILE([lrflows]) >> > >> + >> > >> +# Check the flows in lr_in_admission stage >> > >> +AT_CHECK([grep lr_in_admission lrflows | grep lrp1 | sed >> > >> 's/table=../table=??/' | sort], [0], [dnl >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.dst == >> > >> 00:00:00:00:00:01 && inport == "lrp1" && >> > >> is_chassis_resident("cr-lrp1")), action=(xreg0[[0..47]] = >> > >> 00:00:00:00:00:01; next;) >> > >> + table=??(lr_in_admission ), priority=50 , match=(eth.mcast && >> > >> inport == "lrp1"), action=(xreg0[[0..47]] = 00:00:00:00:00:01; next;) >> > >> +]) >> > >> + >> > >> +AT_CLEANUP >> > >> +]) >> > >> + >> > >> + >> > >> OVN_FOR_EACH_NORTHD([ >> > >> AT_SETUP([check options:requested-chassis fills requested_chassis col]) >> > >> ovn_start NORTHD_TYPE >> > >> -- >> > >> 2.36.1 >> > >> >> > >> _______________________________________________ >> > >> dev mailing list >> > >> d...@openvswitch.org <mailto:d...@openvswitch.org> >> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > > _______________________________________________ >> > > dev mailing list >> > > d...@openvswitch.org <mailto:d...@openvswitch.org> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > _______________________________________________ >> > dev mailing list >> > d...@openvswitch.org <mailto:d...@openvswitch.org> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
