On Tue, Jan 24, 2023 at 12:28 PM Ales Musil <amu...@redhat.com> wrote:
> To make it easier to add flows to this stage, refactor the function.
> This also has the benefit that we should see fewer allocations due to
> rearranging how we create flows and how we manipulate the match string.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> v5: Rebase on top of current main.
> Add missing ");" if ct_lb_related feature is not supported.
> v6: Rebase on top of current main.
> Fix wrong enclosing for reject action and add missing test case.
> ---
> northd/northd.c | 483 ++++++++++++++++++++------------------------
> tests/ovn-northd.at | 69 ++++++-
> 2 files changed, 280 insertions(+), 272 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 0944a7b56..4b0d37375 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3878,10 +3878,13 @@ static bool
> build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
> struct ovn_northd_lb_vip *lb_vip_nb,
> struct ds *action, char *selection_fields,
> - bool ls_dp, bool ct_lb_mark)
> + struct ds *skip_snat_action, struct ds
> *force_snat_action,
> + bool ls_dp, const struct chassis_features *features)
> {
> - const char *ct_lb_action = ct_lb_mark ? "ct_lb_mark" : "ct_lb";
> - bool skip_hash_fields = false, reject = false;
> + const char *ct_lb_action =
> + features->ct_no_masked_label ? "ct_lb_mark" : "ct_lb";
> + bool reject = !lb_vip->n_backends && lb_vip->empty_backend_rej;
> + bool drop = false;
>
> if (lb_vip_nb->lb_health_check) {
> ds_put_format(action, "%s(backends=", ct_lb_action);
> @@ -3902,23 +3905,12 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
> ds_put_format(action, "%s:%"PRIu16",",
> backend->ip_str, backend->port);
> }
> + ds_chomp(action, ',');
>
> - if (!n_active_backends) {
> - if (!lb_vip->empty_backend_rej) {
> - ds_clear(action);
> - ds_put_cstr(action, debug_drop_action());
> - skip_hash_fields = true;
> - } else {
> - reject = true;
> - }
> - } else {
> - ds_chomp(action, ',');
> - ds_put_cstr(action, ");");
> - }
> - } else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
> - reject = true;
> + drop = !n_active_backends && !lb_vip->empty_backend_rej;
> + reject = !n_active_backends && lb_vip->empty_backend_rej;
> } else {
> - ds_put_format(action, "%s(backends=%s);", ct_lb_action,
> + ds_put_format(action, "%s(backends=%s", ct_lb_action,
> lb_vip_nb->backend_ips);
> }
>
> @@ -3927,12 +3919,29 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
> : ovn_stage_get_table(S_ROUTER_OUT_SNAT);
> ds_clear(action);
> ds_put_format(action, "reg0 = 0; reject { outport <-> inport; "
> - "next(pipeline=egress,table=%d);};", stage);
> - } else if (!skip_hash_fields && selection_fields &&
> selection_fields[0]) {
> - ds_chomp(action, ';');
> - ds_chomp(action, ')');
> - ds_put_format(action, "; hash_fields=\"%s\");", selection_fields);
> + "next(pipeline=egress,table=%d);};", stage);
> + } else if (drop) {
> + ds_clear(action);
> + ds_put_cstr(action, debug_drop_action());
> + } else if (selection_fields && selection_fields[0]) {
> + ds_put_format(action, "; hash_fields=\"%s\"", selection_fields);
> + }
> +
> + bool is_lb_action = !(reject || drop);
> + const char *enclose = is_lb_action ? ");" : "";
> +
> + if (!ls_dp) {
> + bool flag_supported = is_lb_action && features->ct_lb_related;
> + ds_put_format(skip_snat_action, "flags.skip_snat_for_lb = 1;
> %s%s",
> + ds_cstr(action),
> + flag_supported ? "; skip_snat);" : enclose);
> + ds_put_format(force_snat_action, "flags.force_snat_for_lb = 1;
> %s%s",
> + ds_cstr(action),
> + flag_supported ? "; force_snat);" : enclose);
> }
> +
> + ds_put_cstr(action, enclose);
> +
> return reject;
> }
>
> @@ -7502,9 +7511,9 @@ build_lb_affinity_default_flows(struct ovn_datapath
> *od, struct hmap *lflows)
> }
>
> static void
> -build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool
> ct_lb_mark,
> - struct ds *match, struct ds *action,
> - const struct shash *meter_groups)
> +build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb,
> + const struct chassis_features *features, struct ds *match,
> + struct ds *action, const struct shash *meter_groups)
> {
> for (size_t i = 0; i < lb->n_vips; i++) {
> struct ovn_lb_vip *lb_vip = &lb->vips[i];
> @@ -7527,8 +7536,8 @@ build_lb_rules(struct hmap *lflows, struct
> ovn_northd_lb *lb, bool ct_lb_mark,
> /* New connections in Ingress table. */
> const char *meter = NULL;
> bool reject = build_lb_vip_actions(lb_vip, lb_vip_nb, action,
> - lb->selection_fields, true,
> - ct_lb_mark);
> + lb->selection_fields, NULL,
> + NULL, true, features);
>
> ds_put_format(match, "ct.new && %s.dst == %s", ip_match,
> lb_vip->vip_str);
> @@ -10465,49 +10474,123 @@ get_force_snat_ip(struct ovn_datapath *od,
> const char *key_type,
> return true;
> }
>
> +#define LROUTER_NAT_LB_FLOW_INIT(MATCH, ACTION, PRIO) \
> + (struct lrouter_nat_lb_flow) \
> + { .action = (ACTION), .lflow_ref = NULL, \
> + .hash = ovn_logical_flow_hash( \
> + ovn_stage_get_table(S_ROUTER_IN_DNAT), \
> + ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), \
> + (PRIO), ds_cstr(MATCH), (ACTION)) }
> +
> +enum lrouter_nat_lb_flow_type {
> + LROUTER_NAT_LB_FLOW_NORMAL = 0,
> + LROUTER_NAT_LB_FLOW_SKIP_SNAT,
> + LROUTER_NAT_LB_FLOW_FORCE_SNAT,
> + LROUTER_NAT_LB_FLOW_MAX,
> +};
> +
> +struct lrouter_nat_lb_flow {
> + char *action;
> + struct ovn_lflow *lflow_ref;
> +
> + uint32_t hash;
> +};
> +
> +struct lrouter_nat_lb_flows_ctx {
> + struct lrouter_nat_lb_flow new[LROUTER_NAT_LB_FLOW_MAX];
> + struct lrouter_nat_lb_flow est[LROUTER_NAT_LB_FLOW_MAX];
> +
> + struct ds *new_match;
> + struct ds *est_match;
> + struct ds *undnat_match;
> +
> + struct ovn_lb_vip *lb_vip;
> + struct ovn_northd_lb *lb;
> + bool reject;
> +
> + int prio;
> +
> + struct hmap *lflows;
> + const struct shash *meter_groups;
> +};
> +
> static void
> -build_gw_lrouter_nat_flows_for_lb(struct ovn_northd_lb *lb,
> - struct ovn_datapath **dplist, int
> n_dplist,
> - bool reject, char *new_match,
> - char *new_action, char *est_match,
> - char *est_action, struct hmap *lflows,
> - int prio, const struct shash
> *meter_groups)
> -{
> - if (!n_dplist) {
> +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)
> +{
> + char *gw_action = od->is_gw_router ? "ct_dnat;" : "ct_dnat_in_czone;";
> + /* Store the match lengths, so we can reuse the ds buffer. */
> + size_t new_match_len = ctx->new_match->length;
> + size_t est_match_len = ctx->est_match->length;
> + size_t undnat_match_len = ctx->undnat_match->length;
> +
> +
> + const char *meter = NULL;
> +
> + if (ctx->reject) {
> + meter = copp_meter_get(COPP_REJECT, od->nbr->copp,
> ctx->meter_groups);
> + }
> +
> + if (ctx->lb_vip->n_backends || !ctx->lb_vip->empty_backend_rej) {
> + ds_put_format(ctx->new_match, " && is_chassis_resident(%s)",
> + od->l3dgw_ports[0]->cr_port->json_key);
> + ds_put_format(ctx->est_match, " && is_chassis_resident(%s)",
> + od->l3dgw_ports[0]->cr_port->json_key);
> + }
> +
> + ovn_lflow_add_with_hint__(ctx->lflows, od, S_ROUTER_IN_DNAT,
> ctx->prio,
> + ds_cstr(ctx->new_match),
> ctx->new[type].action,
> + NULL, meter, &ctx->lb->nlb->header_);
> + ovn_lflow_add_with_hint(ctx->lflows, od, S_ROUTER_IN_DNAT, ctx->prio,
> + ds_cstr(ctx->est_match),
> ctx->est[type].action,
> + &ctx->lb->nlb->header_);
> +
> + ds_truncate(ctx->new_match, new_match_len);
> + ds_truncate(ctx->est_match, est_match_len);
> +
> + if (!ctx->lb_vip->n_backends) {
> return;
> }
>
> - struct ovn_lflow *lflow_ref_new = NULL, *lflow_ref_est = NULL;
> - uint32_t hash_new = ovn_logical_flow_hash(
> - ovn_stage_get_table(S_ROUTER_IN_DNAT),
> - ovn_stage_get_pipeline(S_ROUTER_IN_DNAT),
> - prio, new_match, new_action);
> - uint32_t hash_est = ovn_logical_flow_hash(
> - ovn_stage_get_table(S_ROUTER_IN_DNAT),
> - ovn_stage_get_pipeline(S_ROUTER_IN_DNAT),
> - prio, est_match, est_action);
> + char *action = type == LROUTER_NAT_LB_FLOW_NORMAL
> + ? gw_action : ctx->est[type].action;
>
> - for (size_t i = 0; i < n_dplist; i++) {
> - struct ovn_datapath *od = dplist[i];
> - const char *meter = NULL;
> + 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);
> + ovn_lflow_add_with_hint(ctx->lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> + ds_cstr(ctx->undnat_match), action,
> + &ctx->lb->nlb->header_);
> + ds_truncate(ctx->undnat_match, undnat_match_len);
> +}
>
> - if (reject) {
> - meter = copp_meter_get(COPP_REJECT, od->nbr->copp,
> meter_groups);
> - }
> - if (meter || !ovn_dp_group_add_with_reference(lflow_ref_new, od))
> {
> - struct ovn_lflow *lflow = ovn_lflow_add_at_with_hash(lflows,
> od,
> - S_ROUTER_IN_DNAT, prio, new_match, new_action,
> - NULL, meter, &lb->nlb->header_, OVS_SOURCE_LOCATOR,
> - hash_new);
> - lflow_ref_new = meter ? NULL : lflow;
> - }
> +static void
> +build_gw_lrouter_nat_flows_for_lb(struct lrouter_nat_lb_flows_ctx *ctx,
> + enum lrouter_nat_lb_flow_type type,
> + struct ovn_datapath *od)
> +{
> + const char *meter = NULL;
> + struct lrouter_nat_lb_flow *new_flow = &ctx->new[type];
> + struct lrouter_nat_lb_flow *est_flow = &ctx->est[type];
>
> - if (!ovn_dp_group_add_with_reference(lflow_ref_est, od)) {
> - lflow_ref_est = ovn_lflow_add_at_with_hash(lflows, od,
> - S_ROUTER_IN_DNAT, prio, est_match, est_action,
> - NULL, NULL, &lb->nlb->header_,
> - OVS_SOURCE_LOCATOR, hash_est);
> - }
> + if (ctx->reject) {
> + meter = copp_meter_get(COPP_REJECT, od->nbr->copp,
> ctx->meter_groups);
> + }
> + if (meter || !ovn_dp_group_add_with_reference(new_flow->lflow_ref,
> od)) {
> + struct ovn_lflow *lflow = ovn_lflow_add_at_with_hash(ctx->lflows,
> od,
> + S_ROUTER_IN_DNAT, ctx->prio, ds_cstr(ctx->new_match),
> + new_flow->action, NULL, meter, &ctx->lb->nlb->header_,
> + OVS_SOURCE_LOCATOR, new_flow->hash);
> + new_flow->lflow_ref = meter ? NULL : lflow;
> + }
> +
> + if (!ovn_dp_group_add_with_reference(est_flow->lflow_ref, od)) {
> + est_flow->lflow_ref = ovn_lflow_add_at_with_hash(ctx->lflows, od,
> + S_ROUTER_IN_DNAT, ctx->prio, ds_cstr(ctx->est_match),
> + est_flow->action, NULL, NULL, &ctx->lb->nlb->header_,
> + OVS_SOURCE_LOCATOR, est_flow->hash);
> }
> }
>
> @@ -10523,22 +10606,25 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> const char *ct_natted = features->ct_no_masked_label
> ? "ct_mark.natted"
> : "ct_label.natted";
> - char *skip_snat_new_action = NULL;
> - char *skip_snat_est_action = NULL;
> - char *new_match;
> - char *est_match;
> +
> + bool ipv4 = lb_vip->address_family == AF_INET;
> + const char *ip_match = ipv4 ? "ip4" : "ip6";
> + const char *ip_reg = ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6;
> +
> + int prio = 110;
> +
> + struct ds skip_snat_act = DS_EMPTY_INITIALIZER;
> + struct ds force_snat_act = DS_EMPTY_INITIALIZER;
> + struct ds est_match = DS_EMPTY_INITIALIZER;
> + struct ds undnat_match = DS_EMPTY_INITIALIZER;
> + struct ds unsnat_match = DS_EMPTY_INITIALIZER;
>
> ds_clear(match);
> ds_clear(action);
>
> bool reject = build_lb_vip_actions(lb_vip, vips_nb, action,
> - lb->selection_fields, false,
> - features->ct_no_masked_label);
> - bool drop = !!strncmp(ds_cstr(action), "ct_lb", strlen("ct_lb"));
> - if (!drop) {
> - /* Remove the trailing ");". */
> - ds_truncate(action, action->length - 2);
> - }
> + lb->selection_fields,
> &skip_snat_act,
> + &force_snat_act, false, features);
>
> /* Higher priority rules are added for load-balancing in DNAT
> * table. For every match (on a VIP[:port]), we add two flows.
> @@ -10546,52 +10632,23 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> * of "ct_lb_mark($targets);". The other flow is for ct.est with
> * an action of "next;".
> */
> - if (lb_vip->address_family == AF_INET) {
> - ds_put_format(match, "ip4 && "REG_NEXT_HOP_IPV4" == %s",
> - lb_vip->vip_str);
> - } else {
> - ds_put_format(match, "ip6 && "REG_NEXT_HOP_IPV6" == %s",
> - lb_vip->vip_str);
> - }
> -
> - if (lb->skip_snat) {
> - const char *skip_snat = features->ct_lb_related && !drop
> - ? "; skip_snat);"
> - : "";
> - skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1;
> %s%s",
> - ds_cstr(action), skip_snat);
> - skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; "
> - "next;");
> - }
> -
> - int prio = 110;
> - if (lb_vip->port_str) {
> + ds_put_format(match, "ct.new && !ct.rel && %s && %s == %s",
> + ip_match, ip_reg, lb_vip->vip_str);
> + if (lb_vip->vip_port) {
> prio = 120;
> - new_match = xasprintf("ct.new && !ct.rel && %s && %s && "
> - REG_ORIG_TP_DPORT_ROUTER" == %s",
> - ds_cstr(match), lb->proto,
> lb_vip->port_str);
> - est_match = xasprintf("ct.est && !ct.rel && %s && %s && "
> - REG_ORIG_TP_DPORT_ROUTER" == %s && %s == 1",
> - ds_cstr(match), lb->proto, lb_vip->port_str,
> - ct_natted);
> - } else {
> - new_match = xasprintf("ct.new && !ct.rel && %s", ds_cstr(match));
> - est_match = xasprintf("ct.est && !ct.rel && %s && %s == 1",
> - ds_cstr(match), ct_natted);
> + ds_put_format(match, " && %s && "REG_ORIG_TP_DPORT_ROUTER" == %d",
> + lb->proto, lb_vip->vip_port);
> }
>
> - const char *ip_match = NULL;
> - if (lb_vip->address_family == AF_INET) {
> - ip_match = "ip4";
> - } else {
> - ip_match = "ip6";
> - }
> + ds_put_cstr(&est_match, "ct.est");
> + /* Clone the match after initial "ct.new" (6 bytes). */
> + ds_put_cstr(&est_match, ds_cstr(match) + 6);
> + ds_put_format(&est_match, " && %s == 1", ct_natted);
>
> /* Add logical flows to UNDNAT the load balanced reverse traffic in
> * the router egress pipleine stage - S_ROUTER_OUT_UNDNAT if the
> logical
> * router has a gateway router port associated.
> */
> - struct ds undnat_match = DS_EMPTY_INITIALIZER;
> ds_put_format(&undnat_match, "%s && (", ip_match);
>
> for (size_t i = 0; i < lb_vip->n_backends; i++) {
> @@ -10606,12 +10663,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> ds_put_cstr(&undnat_match, ") || ");
> }
> }
> - ds_chomp(&undnat_match, ' ');
> - ds_chomp(&undnat_match, '|');
> - ds_chomp(&undnat_match, '|');
> - ds_chomp(&undnat_match, ' ');
> + /* Remove the trailing " || ". */
> + ds_truncate(&undnat_match, undnat_match.length - 4);
>
> - struct ds unsnat_match = DS_EMPTY_INITIALIZER;
> ds_put_format(&unsnat_match, "%s && %s.dst == %s && %s",
> ip_match, ip_match, lb_vip->vip_str, lb->proto);
> if (lb_vip->port_str) {
> @@ -10619,21 +10673,34 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> lb_vip->port_str);
> }
>
> - struct ovn_datapath **gw_router_skip_snat =
> - xcalloc(lb->n_nb_lr, sizeof *gw_router_skip_snat);
> - int n_gw_router_skip_snat = 0;
> + struct lrouter_nat_lb_flows_ctx ctx = {
> + .lb_vip = lb_vip,
> + .lb = lb,
> + .reject = reject,
> + .prio = prio,
> + .lflows = lflows,
> + .meter_groups = meter_groups,
> + .new_match = match,
> + .est_match = &est_match,
> + .undnat_match = &undnat_match
> + };
>
> - struct ovn_datapath **gw_router_force_snat =
> - xcalloc(lb->n_nb_lr, sizeof *gw_router_force_snat);
> - int n_gw_router_force_snat = 0;
> + ctx.new[LROUTER_NAT_LB_FLOW_NORMAL] =
> + LROUTER_NAT_LB_FLOW_INIT(match, ds_cstr(action), prio);
> + ctx.est[LROUTER_NAT_LB_FLOW_NORMAL] =
> + LROUTER_NAT_LB_FLOW_INIT(&est_match, "next;", prio);
>
> - struct ovn_datapath **gw_router =
> - xcalloc(lb->n_nb_lr, sizeof *gw_router);
> - int n_gw_router = 0;
> + ctx.new[LROUTER_NAT_LB_FLOW_SKIP_SNAT] =
> + LROUTER_NAT_LB_FLOW_INIT(match, ds_cstr(&skip_snat_act), prio);
> + ctx.est[LROUTER_NAT_LB_FLOW_SKIP_SNAT] =
> + LROUTER_NAT_LB_FLOW_INIT(&est_match,
> + "flags.skip_snat_for_lb = 1; next;",
> prio);
>
> - struct ovn_datapath **distributed_router =
> - xcalloc(lb->n_nb_lr, sizeof *distributed_router);
> - int n_distributed_router = 0;
> + ctx.new[LROUTER_NAT_LB_FLOW_FORCE_SNAT] =
> + LROUTER_NAT_LB_FLOW_INIT(match, ds_cstr(&force_snat_act), prio);
> + ctx.est[LROUTER_NAT_LB_FLOW_FORCE_SNAT] =
> + LROUTER_NAT_LB_FLOW_INIT(&est_match,
> + "flags.force_snat_for_lb = 1; next;",
> prio);
>
> struct ovn_datapath **lb_aff_force_snat_router =
> xcalloc(lb->n_nb_lr, sizeof *lb_aff_force_snat_router);
> @@ -10647,18 +10714,22 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
> *lb_vip,
> * lflow generation for them.
> */
> for (size_t i = 0; i < lb->n_nb_lr; i++) {
> + enum lrouter_nat_lb_flow_type type;
> struct ovn_datapath *od = lb->nb_lr[i];
> +
> + if (lb->skip_snat) {
> + type = LROUTER_NAT_LB_FLOW_SKIP_SNAT;
> + } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> + od->lb_force_snat_router_ip) {
> + type = LROUTER_NAT_LB_FLOW_FORCE_SNAT;
> + } else {
> + type = LROUTER_NAT_LB_FLOW_NORMAL;
> + }
> +
> if (!od->n_l3dgw_ports) {
> - if (lb->skip_snat) {
> - gw_router_skip_snat[n_gw_router_skip_snat++] = od;
> - } else if
> (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> - od->lb_force_snat_router_ip) {
> - gw_router_force_snat[n_gw_router_force_snat++] = od;
> - } else {
> - gw_router[n_gw_router++] = od;
> - }
> + build_gw_lrouter_nat_flows_for_lb(&ctx, type, od);
> } else {
> - distributed_router[n_distributed_router++] = od;
> + build_distr_lrouter_nat_flows_for_lb(&ctx, type, od);
> }
>
> if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> @@ -10680,149 +10751,36 @@ build_lrouter_nat_flows_for_lb(struct
> ovn_lb_vip *lb_vip,
> * unsnat stage, the conntrack flags are not set properly, and
> * it doesn't hit the established state flows in
> * S_ROUTER_IN_DNAT stage. */
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120,
> - ds_cstr(&unsnat_match), "next;",
> - &lb->nlb->header_);
> + ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_UNSNAT, 120,
> + ds_cstr(&unsnat_match), "next;",
> + &lb->nlb->header_);
> }
> }
>
> - /* GW router logic */
> - build_gw_lrouter_nat_flows_for_lb(lb, gw_router_skip_snat,
> - n_gw_router_skip_snat, reject, new_match,
> - skip_snat_new_action, est_match,
> - skip_snat_est_action, lflows, prio, meter_groups);
> -
> - const char *force_snat = features->ct_lb_related && !drop
> - ? "; force_snat);"
> - : "";
> - char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s",
> - ds_cstr(action), 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,
> - "flags.force_snat_for_lb = 1; next;",
> - lflows, prio, meter_groups);
> -
> /* LB affinity flows for datapaths where CMS has specified
> * force_snat_for_lb floag option.
> */
> - build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match,
> + build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match),
> "flags.force_snat_for_lb = 1; ",
> lb_aff_force_snat_router,
> n_lb_aff_force_snat_router);
>
> - if (!drop) {
> - ds_put_cstr(action, ");");
> - }
> -
> - build_gw_lrouter_nat_flows_for_lb(lb, gw_router, n_gw_router,
> - reject, new_match, ds_cstr(action), est_match,
> - "next;", lflows, prio, meter_groups);
> -
> /* LB affinity flows for datapaths where CMS has specified
> * skip_snat_for_lb floag option or regular datapaths.
> */
> char *lb_aff_action =
> lb->skip_snat ? "flags.skip_snat_for_lb = 1; " : NULL;
> - build_lb_affinity_lr_flows(lflows, lb, lb_vip, new_match,
> lb_aff_action,
> - lb_aff_router, n_lb_aff_router);
> -
> - /* Distributed router logic */
> - for (size_t i = 0; i < n_distributed_router; i++) {
> - struct ovn_datapath *od = distributed_router[i];
> - char *new_match_p = new_match;
> - char *est_match_p = est_match;
> - const char *meter = NULL;
> - bool is_dp_lb_force_snat =
> - !lport_addresses_is_empty(&od->lb_force_snat_addrs) ||
> - od->lb_force_snat_router_ip;
> -
> - if (reject) {
> - meter = copp_meter_get(COPP_REJECT, od->nbr->copp,
> meter_groups);
> - }
> -
> - if (lb_vip->n_backends || !lb_vip->empty_backend_rej) {
> - new_match_p = xasprintf("%s && is_chassis_resident(%s)",
> - new_match,
> -
> od->l3dgw_ports[0]->cr_port->json_key);
> - est_match_p = xasprintf("%s && is_chassis_resident(%s)",
> - est_match,
> -
> od->l3dgw_ports[0]->cr_port->json_key);
> - }
> -
> - if (lb->skip_snat) {
> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> - new_match_p, skip_snat_new_action,
> - NULL, meter, &lb->nlb->header_);
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> - est_match_p, skip_snat_est_action,
> - &lb->nlb->header_);
> - } else if (is_dp_lb_force_snat) {
> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> - new_match_p, new_actions, NULL,
> - meter, &lb->nlb->header_);
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> - est_match_p,
> - "flags.force_snat_for_lb = 1; next;",
> - &lb->nlb->header_);
> - } else {
> - ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
> - new_match_p, ds_cstr(action), NULL,
> - meter, &lb->nlb->header_);
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
> - est_match_p, "next;",
> - &lb->nlb->header_);
> - }
> -
> - if (new_match_p != new_match) {
> - free(new_match_p);
> - }
> - if (est_match_p != est_match) {
> - free(est_match_p);
> - }
> -
> - if (!lb_vip->n_backends) {
> - continue;
> - }
> -
> - char *undnat_match_p = xasprintf(
> - "%s) && outport == %s && is_chassis_resident(%s)",
> - ds_cstr(&undnat_match),
> - od->l3dgw_ports[0]->json_key,
> - od->l3dgw_ports[0]->cr_port->json_key);
> - if (lb->skip_snat) {
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> - undnat_match_p, skip_snat_est_action,
> - &lb->nlb->header_);
> - } else if (is_dp_lb_force_snat) {
> - ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
> - undnat_match_p,
> - "flags.force_snat_for_lb = 1; next;",
> - &lb->nlb->header_);
> - } else {
> - ovn_lflow_add_with_hint(
> - lflows, od, S_ROUTER_OUT_UNDNAT, 120, undnat_match_p,
> - od->is_gw_router ? "ct_dnat;" : "ct_dnat_in_czone;",
> - &lb->nlb->header_);
> - }
> - free(undnat_match_p);
> - }
> + build_lb_affinity_lr_flows(lflows, lb, lb_vip, ds_cstr(match),
> + lb_aff_action, lb_aff_router,
> n_lb_aff_router);
>
> ds_destroy(&unsnat_match);
> ds_destroy(&undnat_match);
> + ds_destroy(&est_match);
> + ds_destroy(&skip_snat_act);
> + ds_destroy(&force_snat_act);
>
> - free(skip_snat_new_action);
> - free(skip_snat_est_action);
> - free(est_match);
> - free(new_match);
> - free(new_actions);
> -
> - free(gw_router_force_snat);
> - free(gw_router_skip_snat);
> - free(distributed_router);
> free(lb_aff_force_snat_router);
> free(lb_aff_router);
> - free(gw_router);
> }
>
> static void
> @@ -10867,8 +10825,7 @@ build_lswitch_flows_for_lb(struct ovn_northd_lb
> *lb, struct hmap *lflows,
> * REGBIT_CONNTRACK_COMMIT. */
> build_lb_rules_pre_stateful(lflows, lb, features->ct_no_masked_label,
> match, action);
> - build_lb_rules(lflows, lb, features->ct_no_masked_label,
> - match, action, meter_groups);
> + build_lb_rules(lflows, lb, features, match, action, meter_groups);
> }
>
> /* If there are any load balancing rules, we should send the packet to
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5d5c32ee3..3fa02d2b3 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3773,15 +3773,6 @@ AT_CHECK([grep "lr_in_dnat" lr0flows | sort], [0],
> [dnl
> table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
> ct_commit_nat;)
> ])
>
> -AT_CHECK([grep "lr_out_undnat" lr0flows | sed 's/table=./table=?/' |
> sort], [0], [dnl
> - table=? (lr_out_undnat ), priority=0 , match=(1), action=(next;)
> - table=? (lr_out_undnat ), priority=50 , match=(ip),
> action=(flags.loopback = 1; ct_dnat;)
> -])
> -
> -AT_CHECK([grep "lr_out_post_undnat" lr0flows | sed 's/table=./table=?/' |
> sort], [0], [dnl
> - table=? (lr_out_post_undnat ), priority=0 , match=(1), action=(next;)
> - table=? (lr_out_post_undnat ), priority=50 , match=(ip && ct.new),
> action=(ct_commit { } ; next; )
> -])
>
> check ovn-nbctl --wait=sb set logical_router lr0
> options:lb_force_snat_ip="20.0.0.4 aef0::4"
>
> @@ -5616,6 +5607,66 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
> 's/table=./table=?/' | sort], [0], [
> table=? (lr_out_snat ), priority=120 , match=(nd_ns),
> action=(next;)
> ])
>
> +# LB with reject configured
> +check ovn-nbctl --wait=sb remove logical_router lr0 options
> lb_force_snat_ip
> +check ovn-nbctl --wait=sb sync
> +
> +check ovn-nbctl lr-lb-del lr0
> +check ovn-nbctl lsp-add sw0 vip1
> +check ovn-nbctl lsp-add sw0 vip2
> +check ovn-nbctl --reject lb-add lb5 172.168.10.10 10.0.20.10,10.0.20.20
> +check ovn-nbctl --wait=sb set load_balancer lb5
> ip_port_mappings:10.0.20.10=vip1:10.0.0.2
> +check ovn-nbctl --wait=sb set load_balancer lb5
> ip_port_mappings:10.0.20.20=vip2:20.0.0.2
> +
> +check ovn-nbctl --wait=sb lr-lb-add lr0 lb5
> +AT_CHECK([ovn-nbctl --wait=sb -- --id=@hc create \
> +Load_Balancer_Health_Check vip="172.168.10.10" -- add Load_Balancer lb5 \
> +health_check @hc | uuidfilt], [0], [<0>
> +])
> +wait_row_count Service_Monitor 2
> +
> +# Set the service monitor for vip1 and vip2 to offline
> +sm_vip1=$(fetch_column Service_Monitor _uuid logical_port=vip1)
> +sm_vip2=$(fetch_column Service_Monitor _uuid logical_port=vip2)
> +
> +ovn-sbctl set service_monitor $sm_vip1 status=offline
> +ovn-sbctl set service_monitor $sm_vip2 status=offline
> +
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
> + table=7 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> + table=7 (lr_in_dnat ), priority=110 , match=(ct.est && !ct.rel
> && ip4 && reg0 == 172.168.10.10 && ct_mark.natted == 1), action=(next;)
> + table=7 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel
> && ip4 && reg0 == 172.168.10.10), action=(reg0 = 0; reject { outport <->
> inport; next(pipeline=egress,table=3);};)
> + table=7 (lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est
> && !ct.new), action=(ct_commit_nat;)
> + table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
> 1; ct_commit_nat;)
> + table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
> ct_commit_nat;)
> +])
> +
> +# LB with reject and skip_snat
> +check ovn-nbctl --wait=sb set load_balancer lb5 options:skip_snat=true
> +
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
> + table=7 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> + table=7 (lr_in_dnat ), priority=110 , match=(ct.est && !ct.rel
> && ip4 && reg0 == 172.168.10.10 && ct_mark.natted == 1),
> action=(flags.skip_snat_for_lb = 1; next;)
> + table=7 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel
> && ip4 && reg0 == 172.168.10.10), action=(flags.skip_snat_for_lb = 1; reg0
> = 0; reject { outport <-> inport; next(pipeline=egress,table=3);};)
> + table=7 (lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est
> && !ct.new), action=(ct_commit_nat;)
> + table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
> 1; ct_commit_nat;)
> + table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
> ct_commit_nat;)
> +])
> +
> +check ovn-nbctl --wait=sb remove load_balancer lb5 options skip_snat
> +
> +# LB with reject and force_snat
> +check ovn-nbctl --wait=sb set logical_router lr0
> options:lb_force_snat_ip="router_ip"
> +
> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
> + table=7 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
> + table=7 (lr_in_dnat ), priority=110 , match=(ct.est && !ct.rel
> && ip4 && reg0 == 172.168.10.10 && ct_mark.natted == 1),
> action=(flags.force_snat_for_lb = 1; next;)
> + table=7 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel
> && ip4 && reg0 == 172.168.10.10), action=(flags.force_snat_for_lb = 1; reg0
> = 0; reject { outport <-> inport; next(pipeline=egress,table=3);};)
> + table=7 (lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est
> && !ct.new), action=(ct_commit_nat;)
> + table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
> 1; ct_commit_nat;)
> + table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
> ct_commit_nat;)
> +])
> +
> AT_CLEANUP
> ])
>
> --
> 2.39.1
>
>
The diff between v5 and v6:
diff --git a/northd/northd.c b/northd/northd.c
index d22316bd3..4b0d37375 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3928,20 +3928,20 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
}
bool is_lb_action = !(reject || drop);
+ const char *enclose = is_lb_action ? ");" : "";
if (!ls_dp) {
bool flag_supported = is_lb_action && features->ct_lb_related;
ds_put_format(skip_snat_action, "flags.skip_snat_for_lb = 1; %s%s",
ds_cstr(action),
- flag_supported ? "; skip_snat);" : ");");
+ flag_supported ? "; skip_snat);" : enclose);
ds_put_format(force_snat_action, "flags.force_snat_for_lb = 1;
%s%s",
ds_cstr(action),
- flag_supported ? "; force_snat);" : ");");
- }
- if (is_lb_action) {
- ds_put_cstr(action, ");");
+ flag_supported ? "; force_snat);" : enclose);
}
+ ds_put_cstr(action, enclose);
+
return reject;
}
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index dfa064d53..3fa02d2b3 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5607,6 +5607,66 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
's/table=./table=?/' | sort], [0], [
table=? (lr_out_snat ), priority=120 , match=(nd_ns),
action=(next;)
])
+# LB with reject configured
+check ovn-nbctl --wait=sb remove logical_router lr0 options
lb_force_snat_ip
+check ovn-nbctl --wait=sb sync
+
+check ovn-nbctl lr-lb-del lr0
+check ovn-nbctl lsp-add sw0 vip1
+check ovn-nbctl lsp-add sw0 vip2
+check ovn-nbctl --reject lb-add lb5 172.168.10.10 10.0.20.10,10.0.20.20
+check ovn-nbctl --wait=sb set load_balancer lb5
ip_port_mappings:10.0.20.10=vip1:10.0.0.2
+check ovn-nbctl --wait=sb set load_balancer lb5
ip_port_mappings:10.0.20.20=vip2:20.0.0.2
+
+check ovn-nbctl --wait=sb lr-lb-add lr0 lb5
+AT_CHECK([ovn-nbctl --wait=sb -- --id=@hc create \
+Load_Balancer_Health_Check vip="172.168.10.10" -- add Load_Balancer lb5 \
+health_check @hc | uuidfilt], [0], [<0>
+])
+wait_row_count Service_Monitor 2
+
+# Set the service monitor for vip1 and vip2 to offline
+sm_vip1=$(fetch_column Service_Monitor _uuid logical_port=vip1)
+sm_vip2=$(fetch_column Service_Monitor _uuid logical_port=vip2)
+
+ovn-sbctl set service_monitor $sm_vip1 status=offline
+ovn-sbctl set service_monitor $sm_vip2 status=offline
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+ table=7 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
+ table=7 (lr_in_dnat ), priority=110 , match=(ct.est && !ct.rel
&& ip4 && reg0 == 172.168.10.10 && ct_mark.natted == 1), action=(next;)
+ table=7 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel
&& ip4 && reg0 == 172.168.10.10), action=(reg0 = 0; reject { outport <->
inport; next(pipeline=egress,table=3);};)
+ table=7 (lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est
&& !ct.new), action=(ct_commit_nat;)
+ table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
&& !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
1; ct_commit_nat;)
+ table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
&& !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
ct_commit_nat;)
+])
+
+# LB with reject and skip_snat
+check ovn-nbctl --wait=sb set load_balancer lb5 options:skip_snat=true
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+ table=7 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
+ table=7 (lr_in_dnat ), priority=110 , match=(ct.est && !ct.rel
&& ip4 && reg0 == 172.168.10.10 && ct_mark.natted == 1),
action=(flags.skip_snat_for_lb = 1; next;)
+ table=7 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel
&& ip4 && reg0 == 172.168.10.10), action=(flags.skip_snat_for_lb = 1; reg0
= 0; reject { outport <-> inport; next(pipeline=egress,table=3);};)
+ table=7 (lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est
&& !ct.new), action=(ct_commit_nat;)
+ table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
&& !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
1; ct_commit_nat;)
+ table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
&& !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
ct_commit_nat;)
+])
+
+check ovn-nbctl --wait=sb remove load_balancer lb5 options skip_snat
+
+# LB with reject and force_snat
+check ovn-nbctl --wait=sb set logical_router lr0
options:lb_force_snat_ip="router_ip"
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+ table=7 (lr_in_dnat ), priority=0 , match=(1), action=(next;)
+ table=7 (lr_in_dnat ), priority=110 , match=(ct.est && !ct.rel
&& ip4 && reg0 == 172.168.10.10 && ct_mark.natted == 1),
action=(flags.force_snat_for_lb = 1; next;)
+ table=7 (lr_in_dnat ), priority=110 , match=(ct.new && !ct.rel
&& ip4 && reg0 == 172.168.10.10), action=(flags.force_snat_for_lb = 1; reg0
= 0; reject { outport <-> inport; next(pipeline=egress,table=3);};)
+ table=7 (lr_in_dnat ), priority=50 , match=(ct.rel && !ct.est
&& !ct.new), action=(ct_commit_nat;)
+ table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
&& !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb =
1; ct_commit_nat;)
+ table=7 (lr_in_dnat ), priority=70 , match=(ct.rel && !ct.est
&& !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1;
ct_commit_nat;)
+])
+
AT_CLEANUP
])
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
