On 12/7/22 16:34, Ales Musil wrote:
> In order to have the SNAT action available for
> related traffic store the flag in CT register.
>
> Extend the ct_lb and ct_lb_mark action to
> allow additional parameter that sets the
> corresponding flag if needed in ct_label/ct_mark
> register. This allows us to later on match on it
> for related traffic and set the corresponding flags
> fro additional flows.
>
> Currently only two flags are supported "skip_snat"
> and "force_snat" which are mutually exclusive.
>
> Reported-at: https://bugzilla.redhat.com/2126083
> Signed-off-by: Ales Musil <[email protected]>
> ---
Hi Ales,
First of all thanks a lot for fixing this, I know it wasn't easy to
debug!
[...]
> diff --git a/northd/northd.c b/northd/northd.c
> index a16e8a8a7..cb5b14181 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
[...]
> @@ -10411,6 +10422,11 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> bool reject = build_lb_vip_actions(lb_vip, vips_nb, action,
> lb->selection_fields, false,
> ct_lb_mark);
> + bool drop = !!strncmp(ds_cstr(action), "ct_lb", strlen("ct_lb"));
> + if (!drop) {
> + /* Remove the trailing ");". */
> + ds_truncate(action, action->length - 2);
> + }
>
> /* Higher priority rules are added for load-balancing in DNAT
> * table. For every match (on a VIP[:port]), we add two flows.
> @@ -10427,8 +10443,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> }
>
> if (lb->skip_snat) {
> - skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s",
> - ds_cstr(action));
> + skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s%s",
> + ds_cstr(action),
> + drop ? "" : "; skip_snat);");
> skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; "
> "next;");
> }
> @@ -10561,8 +10578,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> skip_snat_new_action, est_match,
> skip_snat_est_action, lflows, prio, meter_groups);
>
> - char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s",
> - ds_cstr(action));
> + char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s",
> + ds_cstr(action),
> + drop ? "" : "; force_snat);");
> build_gw_lrouter_nat_flows_for_lb(lb, gw_router_force_snat,
> n_gw_router_force_snat, reject, new_match,
> new_actions, est_match,
> @@ -10577,6 +10595,10 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> lb_aff_force_snat_router,
> n_lb_aff_force_snat_router);
>
> + if (!drop) {
> + ds_put_cstr(action, ");");
> + }
> +
I think I should've spoken earlier but I was a bit behind with reviewing
this patch until now: this whole part seems a bit fragile because we
make assumptions about how the string built by build_lb_vip_actions()
looks like. What if in the future the action string built by
build_lb_vip_actions() doesn't start with "ct_lb.."? What if it
doesn't end with ");"?
I also don't really like the 'drop' variable: it's almost always equal
to 'reject' with one exception, when healthcheck is enabled and no
backends are active but the LB VIP is not configured to reject on
empty backend sets.
But I won't block the release for this part. Instead, I think we
should find a way to refactor all this code in the near future so that
all LB actions are built in a single place. build_lb_vip_actions() was
supposed to do that but now part of it is done outside the function.
What do you think, does it sound feasible to you?
I added Mark's ack and pushed the patch to main and to branch-22.12.
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
