| Message ID | 20220815141851.78904-1-odivlad@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: support vtep LSP-attached LS to use L3 services | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odivlad@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). > > Signed-off-by: Vladislav Odintsov <odivlad@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 > --- > northd/northd.c | 33 +++++++++++++++++--- > northd/ovn-northd.8.xml | 4 +++ > tests/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 b/tests/ovn-northd.at > index 5b5eeb0ee..3ffa39419 100644 > --- a/tests/ovn-northd.at > +++ b/tests/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 > +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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Thanks Numan. regards, Vladislav Odintsov > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org> wrote: > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odivlad@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). >> >> Signed-off-by: Vladislav Odintsov <odivlad@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 >> --- >> northd/northd.c | 33 +++++++++++++++++--- >> northd/ovn-northd.8.xml | 4 +++ >> tests/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 b/tests/ovn-northd.at >> index 5b5eeb0ee..3ffa39419 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/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 >> +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 >> 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
On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <VlOdintsov@croc.ru> wrote: > > Thanks Numan. > > regards, > Vladislav Odintsov > > > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org> wrote: > > > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odivlad@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 <odivlad@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 > >> --- > >> northd/northd.c | 33 +++++++++++++++++--- > >> northd/ovn-northd.8.xml | 4 +++ > >> tests/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 b/tests/ovn-northd.at > >> index 5b5eeb0ee..3ffa39419 100644 > >> --- a/tests/ovn-northd.at > >> +++ b/tests/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 > >> +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 > >> 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 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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. 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 <hzhou@ovn.org> wrote: > > > On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <VlOdintsov@croc.ru <mailto:VlOdintsov@croc.ru>> wrote: > > > > Thanks Numan. > > > > regards, > > Vladislav Odintsov > > > > > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> wrote: > > > > > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@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 <odivlad@gmail.com <mailto:odivlad@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 > > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov <odivlad@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> - <LR> - <ls2 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? But if that's the case why would you set the gateway-chassis to it at all? 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 <hzhou@ovn.org> wrote: > > > On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <VlOdintsov@croc.ru> > wrote: > > > > Thanks Numan. > > > > regards, > > Vladislav Odintsov > > > > > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org> wrote: > > > > > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov < > odivlad@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 <odivlad@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 > > >> --- > > >> northd/northd.c | 33 +++++++++++++++++--- > > >> northd/ovn-northd.8.xml | 4 +++ > > >> tests/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 b/tests/ovn-northd.at > > >> index 5b5eeb0ee..3ffa39419 100644 > > >> --- a/tests/ovn-northd.at > > >> +++ b/tests/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 > > >> +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 > > >> 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 > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
Regards, Vladislav Odintsov > On 18 Aug 2022, at 20:07, Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> wrote: >> >> >> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <VlOdintsov@croc.ru <mailto:VlOdintsov@croc.ru>> wrote: >> > >> > Thanks Numan. >> > >> > regards, >> > Vladislav Odintsov >> > >> > > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> wrote: >> > > >> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@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 <odivlad@gmail.com <mailto:odivlad@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 >> > >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > > _______________________________________________ >> > > dev mailing list >> > > dev@openvswitch.org <mailto:dev@openvswitch.org> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > _______________________________________________ >> > dev mailing list >> > dev@openvswitch.org <mailto:dev@openvswitch.org> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
On Thu, Aug 18, 2022 at 12:50 PM Vladislav Odintsov <odivlad@gmail.com> wrote: > > > Regards, > Vladislav Odintsov > > On 18 Aug 2022, at 20:07, Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov <odivlad@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> - <LR> - <ls2 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. > I see. Sorry that I didn't know the usage of gateway chassis in this way, but for the patch [1], I wonder if the VTEP chassis (where the ovn-controller-vtep is running) should be responsible for replying ARP requests for the LRP IP from the LS's ARP responder pipeline, instead of requiring a DGP to perform this? I didn't expect DGP to be required for VTEP to work. But anyway, this answers my question to the current patch. Thanks! Han > > 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 <hzhou@ovn.org> wrote: >> >> >> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <VlOdintsov@croc.ru> >> wrote: >> > >> > Thanks Numan. >> > >> > regards, >> > Vladislav Odintsov >> > >> > > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org> wrote: >> > > >> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov < >> odivlad@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 <odivlad@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 >> > >> --- >> > >> northd/northd.c | 33 +++++++++++++++++--- >> > >> northd/ovn-northd.8.xml | 4 +++ >> > >> tests/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 b/tests/ovn-northd.at >> > >> index 5b5eeb0ee..3ffa39419 100644 >> > >> --- a/tests/ovn-northd.at >> > >> +++ b/tests/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 >> > >> +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 >> > >> 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 >> > _______________________________________________ >> > dev mailing list >> > dev@openvswitch.org >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> >
Regards, Vladislav Odintsov > On 19 Aug 2022, at 04:05, Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Aug 18, 2022 at 12:50 PM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> wrote: > > > Regards, > Vladislav Odintsov > >> On 18 Aug 2022, at 20:07, Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> wrote: >> >> >> >> On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@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. > > I see. Sorry that I didn't know the usage of gateway chassis in this way, but for the patch [1], I wonder if the VTEP chassis (where the ovn-controller-vtep is running) should be responsible for replying ARP requests for the LRP IP from the LS's ARP responder pipeline, instead of requiring a DGP to perform this? I didn't expect DGP to be required for VTEP to work. ovn-controller-vtep is only responsible for converting ovn-sb entities to hardware_vtep schema for futher processing by thirdparies like cumulus vtep, vtep emulator by ovs, arista vtep and so on. It doesn’t directly participate in datapath processing. > > But anyway, this answers my question to the current patch. Thanks! > Glad to help you! > Han > > 1: https://github.com/ovn-org/ovn/commit/4e90bcf55c2ef1ab9940836455e4bfe36e57f307 <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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> wrote: >>> >>> >>> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav <VlOdintsov@croc.ru <mailto:VlOdintsov@croc.ru>> wrote: >>> > >>> > Thanks Numan. >>> > >>> > regards, >>> > Vladislav Odintsov >>> > >>> > > On 16 Aug 2022, at 04:56, Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> wrote: >>> > > >>> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@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 <odivlad@gmail.com <mailto:odivlad@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 >>> > >> dev@openvswitch.org <mailto:dev@openvswitch.org> >>> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>> > > _______________________________________________ >>> > > dev mailing list >>> > > dev@openvswitch.org <mailto:dev@openvswitch.org> >>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >>> > _______________________________________________ >>> > dev mailing list >>> > dev@openvswitch.org <mailto:dev@openvswitch.org> >>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >
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 b/tests/ovn-northd.at index 5b5eeb0ee..3ffa39419 100644 --- a/tests/ovn-northd.at +++ b/tests/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 +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
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). Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- 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 --- northd/northd.c | 33 +++++++++++++++++--- northd/ovn-northd.8.xml | 4 +++ tests/ovn-northd.at | 69 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 4 deletions(-)