On Thu, Apr 6, 2023 at 4:48 PM Ilya Maximets <[email protected]> wrote:
> On 4/6/23 16:37, Ales Musil wrote: > > There are essentially three problems with the current > > combination of DGP + SNAT + LB: > > > > 1) The first packet is being SNATed in common zone due > > to a problem with pinctrl not preserving ct_mark/ct_label. > > The commit would create a SNAT entry within the same with DNAT > > entry. > > > > 2) The unSNAT for reply always happened in common zone because of > > the loopback check which would be triggered only when we loop > > the packet through the LR. Now there are two possibilities how > > the reply packet would be handled: > > > > a) If the entry for SNAT in common zone did not time out yet, the > > packet would be passed through unSNAT in common zone which would > > be fine and continue on. However, the unDNAT wouldn't work due to > > the limitation of CT not capable of doing unSNAT/unDNAT on the same > > packet twice in the same zone. So the reply would arrive to > > the original interface, but with wrong source address. > > > > b) If the entry for SNAT timed out it would loop and do unSNAT correctly > > in separate zone and then also unDNAT. This is not possible anymore with > > a recent change 8c341b9d (northd: Drop packets destined to router owned > NAT IP for DGP). > > The reply would be dropped before looping after that change co the > traffic > > would never arrive to the original interface. > > > > 3) The unDNAT was happening only if the DGP was outport meaning > > the reply traffic was routed out, but in the opposite case > > the unDNAT was happening only because of the looping which made > > outport=inport. That's why it worked before introduction of explicit > drop. > > > > In order to fix all those issues do two changes: > > > > 1) Include inport in the unDNAT match, so that we have both > > routing directions covered e.g. (inport == "dgp_port" || outport == > "dpg_port"). > > > > 2) Always use the separate zone for SNAT and DNAT. As the common > > zone was needed for HWOL make the common zone optional with > > configuration option called "use_common_zone". This option is > > by default "false" and can be specified per LR. Use of separate > > zones also eliminates the need for the flag propagation > > in "lr_out_chk_dnat_local" stage, removing the match on ct_mark/ct_label. > > > > The "SNAT in separate zone from DNAT" system test is moved to run only > > with kernel datapath. The reason is that this test doesn't work with > > userspace datapath due to recirculation limit, currently set to 6 [0]. > > > > [0] > https://github.com/openvswitch/ovs/blob/9d840923d32124fe427de76e8234c49d64e4bb77/lib/dpif-netdev.c#L102 > > Reported-at: https://bugzilla.redhat.com/2161281 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > v2: Fix flaky system test. > > v3: Rebase on top of current main. > > v4: Rebase on top of current main. > > Move the system test to system-ovn-kmod > > to unblock the failures with userspace. > > v5: Rebase on top of current main. > > v6: Rebase on top of current main. > > Change the config to a global option instead. > > v7: Address comments from Dumitru: > > Rename the stateless helper to better reflect > > that it applies only to snat_and_dnat type. > > Call the common zone and separate zone functions > > separately based on condition. > > --- > > northd/northd.c | 536 +++++++++++++++++++++------------------ > > northd/ovn-northd.8.xml | 90 +------ > > ovn-nb.xml | 9 + > > tests/ovn-northd.at | 213 +++++++++++----- > > tests/ovn.at | 3 + > > tests/system-ovn-kmod.at | 166 ++++++++++++ > > tests/system-ovn.at | 117 --------- > > 7 files changed, 634 insertions(+), 500 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 7a3886de0..1d13c3d8e 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -66,6 +66,9 @@ static bool check_lsp_is_up; > > > > static bool install_ls_lb_from_router; > > > > +/* Use common zone for SNAT and DNAT if this option is set to "true". */ > > +static bool use_common_zone = false; > > + > > /* MAC allocated for service monitor usage. Just one mac is allocated > > * for this purpose and ovn-controller's on each chassis will make use > > * of this mac when sending out the packets to monitor the services > > @@ -10662,6 +10665,8 @@ build_distr_lrouter_nat_flows_for_lb(struct > lrouter_nat_lb_flows_ctx *ctx, > > enum lrouter_nat_lb_flow_type type, > > struct ovn_datapath *od) > > { > > + struct ovn_port *dgp = od->l3dgw_ports[0]; > > + > > const char *undnat_action; > > > > switch (type) { > > @@ -10673,9 +10678,12 @@ build_distr_lrouter_nat_flows_for_lb(struct > lrouter_nat_lb_flows_ctx *ctx, > > break; > > case LROUTER_NAT_LB_FLOW_NORMAL: > > case LROUTER_NAT_LB_FLOW_MAX: > > - undnat_action = od->is_gw_router ? "ct_dnat;" : > "ct_dnat_in_czone;"; > > + undnat_action = !od->is_gw_router && use_common_zone > > + ? "ct_dnat_in_czone;" > > + : "ct_dnat;"; > > break; > > } > > + > > /* Store the match lengths, so we can reuse the ds buffer. */ > > size_t new_match_len = ctx->new_match->length; > > size_t undnat_match_len = ctx->undnat_match->length; > > @@ -10702,10 +10710,9 @@ build_distr_lrouter_nat_flows_for_lb(struct > lrouter_nat_lb_flows_ctx *ctx, > > return; > > } > > > > - ds_put_format(ctx->undnat_match, > > - ") && outport == %s && is_chassis_resident(%s)", > > - od->l3dgw_ports[0]->json_key, > > - od->l3dgw_ports[0]->cr_port->json_key); > > + ds_put_format(ctx->undnat_match, ") && (inport == %s || outport == > %s)" > > + " && is_chassis_resident(%s)", dgp->json_key, > dgp->json_key, > > + dgp->cr_port->json_key); > > ovn_lflow_add_with_hint(ctx->lflows, od, S_ROUTER_OUT_UNDNAT, 120, > > ds_cstr(ctx->undnat_match), undnat_action, > > &ctx->lb->nlb->header_); > > @@ -11157,6 +11164,14 @@ lrouter_dnat_and_snat_is_stateless(const struct > nbrec_nat *nat) > > !strcmp(nat->type, "dnat_and_snat"); > > } > > > > +static inline bool > > +lrouter_use_common_zone_in_nat(const struct ovn_datapath *od, > > + const struct nbrec_nat *nat) > > +{ > > + return !od->is_gw_router && use_common_zone && > > + !lrouter_dnat_and_snat_is_stateless(nat); > > +} > > + > > /* Handles the match criteria and actions in logical flow > > * based on external ip based NAT rule filter. > > * > > @@ -13648,85 +13663,90 @@ build_lrouter_ipv4_ip_input(struct ovn_port > *op, > > } > > } > > > > +static void > > +build_lrouter_in_unsnat_in_czone_flow(struct hmap *lflows, > > + struct ovn_datapath *od, > > + const struct nbrec_nat *nat, > > + struct ds *match, bool > distributed, > > + bool is_v6, struct ovn_port > *l3dgw_port) > > +{ > > + if (strcmp(nat->type, "snat") && strcmp(nat->type, > "dnat_and_snat")) { > > + return; > > + } > > + > > + ds_clear(match); > > + > > + struct ds zone_match = DS_EMPTY_INITIALIZER; > > + > > + ds_put_format(match, "ip && ip%c.dst == %s && inport == %s", > > + is_v6 ? '6' : '4', nat->external_ip, > l3dgw_port->json_key); > > + ds_clone(&zone_match, match); > > Not sure how big of a problem it is, but it would be better to avoid > cloning. It's an extra memory allocation and copy. Instead, it should > be possible to save the length, create one flow, truncate, create a > second one. We do that in many other places. > > > + > > + ds_put_cstr(match, " && flags.loopback == 0"); > > + > > + /* Update common zone match for the hairpin traffic. */ > > + ds_put_cstr(&zone_match, " && flags.loopback == 1" > > + " && flags.use_snat_zone == 1"); > > + > > + > > + if (!distributed && od->n_l3dgw_ports) { > > + /* Flows for NAT rules that are centralized are only > > + * programmed on the gateway chassis. */ > > + ds_put_format(match, " && is_chassis_resident(%s)", > > + l3dgw_port->cr_port->json_key); > > + ds_put_format(&zone_match, " && is_chassis_resident(%s)", > > + l3dgw_port->cr_port->json_key); > > If we add this before checking flags, we'll not need to update both > matches, > i.e. we can do that before the clone / before saving the length. > That was my original idea. There is one caveat that it affects a ton of tests. Would you agree to have this change as next patch that will target specifically this so the tests are easier to review? > > > + } > > + > > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, > > + 100, ds_cstr(match), "ct_snat_in_czone;", > > + &nat->header_); > > + > > + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, > > + 100, ds_cstr(&zone_match), "ct_snat;", > > + &nat->header_); > > + > > + ds_destroy(&zone_match); > > +} > > Best regards, Ilya Maximets. > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
