northd: Store skip_snat and force_snat in ct_label/mark

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 <amusil@redhat.com> Acked-by: Mark Michelson <mmichels@redhat.com> Signed-off-by: Dumitru Ceara <dceara@redhat.com> (cherry picked from commit c1d6b8a)
ovn-org · Jan 23, 2023 · ad11ebf · ad11ebf
1 parent 9682916
commit ad11ebf
Show file tree

Hide file tree

Showing 10 changed files with 283 additions and 90 deletions.
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
@@ -276,6 +276,12 @@ struct ovnact_ct_nat {
     uint8_t ltable;             /* Logical table ID of next table. */
 };
 
+enum ovnact_ct_lb_flag {
+    OVNACT_CT_LB_FLAG_NONE,
+    OVNACT_CT_LB_FLAG_SKIP_SNAT,
+    OVNACT_CT_LB_FLAG_FORCE_SNAT,
+};
+
 struct ovnact_ct_lb_dst {
     int family;
     union {
@@ -292,6 +298,7 @@ struct ovnact_ct_lb {
     size_t n_dsts;
     uint8_t ltable;             /* Logical table ID of next table. */
     char *hash_fields;
+    enum ovnact_ct_lb_flag ct_flag;
 };
 
 struct ovnact_select_dst {

diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
@@ -170,9 +170,13 @@ const struct ovn_field *ovn_field_from_name(const char *name);
 
 #define OVN_CT_BLOCKED_BIT 0
 #define OVN_CT_NATTED_BIT  1
+#define OVN_CT_LB_SKIP_SNAT_BIT 2
+#define OVN_CT_LB_FORCE_SNAT_BIT 3
 
 #define OVN_CT_BLOCKED 1
 #define OVN_CT_NATTED  2
+#define OVN_CT_LB_SKIP_SNAT 4
+#define OVN_CT_LB_FORCE_SNAT 8
 
 #define OVN_CT_ECMP_ETH_1ST_BIT 32
 #define OVN_CT_ECMP_ETH_END_BIT 79

diff --git a/lib/actions.c b/lib/actions.c
@@ -1199,6 +1199,7 @@ parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark)
     size_t allocated_dsts = 0;
     size_t n_dsts = 0;
     char *hash_fields = NULL;
+    enum ovnact_ct_lb_flag ct_flag = OVNACT_CT_LB_FLAG_NONE;
 
     if (lexer_match(ctx->lexer, LEX_T_LPAREN) &&
         !lexer_match(ctx->lexer, LEX_T_RPAREN)) {
@@ -1279,15 +1280,24 @@ parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark)
 
         if (lexer_match_id(ctx->lexer, "hash_fields")) {
             if (!lexer_match(ctx->lexer, LEX_T_EQUALS) ||
-                ctx->lexer->token.type != LEX_T_STRING ||
-                lexer_lookahead(ctx->lexer) != LEX_T_RPAREN) {
+                ctx->lexer->token.type != LEX_T_STRING) {
                 lexer_syntax_error(ctx->lexer, "invalid hash_fields");
                 free(dsts);
                 return;
             }
 
             hash_fields = xstrdup(ctx->lexer->token.s);
             lexer_get(ctx->lexer);
+            if (!lexer_match(ctx->lexer, LEX_T_SEMICOLON)) {
+                lexer_get(ctx->lexer);
+            }
+        }
+
+        if (lexer_match_id(ctx->lexer, "skip_snat")) {
+            ct_flag = OVNACT_CT_LB_FLAG_SKIP_SNAT;
+            lexer_get(ctx->lexer);
+        } else if (lexer_match_id(ctx->lexer, "force_snat")) {
+            ct_flag = OVNACT_CT_LB_FLAG_FORCE_SNAT;
             lexer_get(ctx->lexer);
         }
     }
@@ -1298,6 +1308,7 @@ parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark)
     cl->dsts = dsts;
     cl->n_dsts = n_dsts;
     cl->hash_fields = hash_fields;
+    cl->ct_flag = ct_flag;
 }
 
 static void
@@ -1331,12 +1342,23 @@ format_ct_lb(const struct ovnact_ct_lb *cl, struct ds *s, bool ct_lb_mark)
                 }
             }
         }
-        ds_put_char(s, ')');
 
         if (cl->hash_fields) {
-            ds_chomp(s, ')');
-            ds_put_format(s, "; hash_fields=\"%s\")", cl->hash_fields);
+            ds_put_format(s, "; hash_fields=\"%s\"", cl->hash_fields);
         }
+
+        switch (cl->ct_flag) {
+            case OVNACT_CT_LB_FLAG_SKIP_SNAT:
+                ds_put_cstr(s, "; skip_snat");
+                break;
+            case OVNACT_CT_LB_FLAG_FORCE_SNAT:
+                ds_put_cstr(s, "; force_snat");
+                break;
+            case OVNACT_CT_LB_FLAG_NONE:
+                /* None is the default value not shown in the output. */
+                break;
+        }
+        ds_put_char(s, ')');
     }
 
     ds_put_char(s, ';');
@@ -1396,6 +1418,21 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
     struct ofpact_group *og;
     uint32_t zone_reg = ep->is_switch ? MFF_LOG_CT_ZONE - MFF_REG0
                             : MFF_LOG_DNAT_ZONE - MFF_REG0;
+    const char *flag_reg = ct_lb_mark ? "ct_mark" : "ct_label";
+
+    const char *ct_flag_value;
+    switch (cl->ct_flag) {
+        case OVNACT_CT_LB_FLAG_SKIP_SNAT:
+            ct_flag_value = OVN_CT_MASKED_STR(OVN_CT_LB_SKIP_SNAT);
+            break;
+        case OVNACT_CT_LB_FLAG_FORCE_SNAT:
+            ct_flag_value = OVN_CT_MASKED_STR(OVN_CT_LB_FORCE_SNAT);
+            break;
+        case OVNACT_CT_LB_FLAG_NONE:
+        default:
+            ct_flag_value = NULL;
+            break;
+    }
 
     struct ds ds = DS_EMPTY_INITIALIZER;
     ds_put_format(&ds, "type=select,selection_method=%s",
@@ -1427,9 +1464,13 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
         ds_put_format(&ds, "),commit,table=%d,zone=NXM_NX_REG%d[0..15],"
                       "exec(set_field:"
                         OVN_CT_MASKED_STR(OVN_CT_NATTED)
-                      "->%s))",
-                      recirc_table, zone_reg,
-                      ct_lb_mark ? "ct_mark" : "ct_label");
+                      "->%s",
+                      recirc_table, zone_reg, flag_reg);
+        if (ct_flag_value) {
+            ds_put_format(&ds, ",set_field:%s->%s", ct_flag_value, flag_reg);
+        }
+
+        ds_put_cstr(&ds, "))");
     }
 
     table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds),

diff --git a/lib/logical-fields.c b/lib/logical-fields.c
@@ -145,6 +145,16 @@ ovn_init_symtab(struct shash *symtab)
                                     WR_CT_COMMIT);
     expr_symtab_add_subfield_scoped(symtab, "ct_mark.ecmp_reply_port", NULL,
                                     "ct_mark[16..31]", WR_CT_COMMIT);
+    expr_symtab_add_subfield_scoped(symtab, "ct_mark.skip_snat", NULL,
+                                    "ct_mark["
+                                    OVN_CT_STR(OVN_CT_LB_SKIP_SNAT_BIT)
+                                    "]",
+                                    WR_CT_COMMIT);
+    expr_symtab_add_subfield_scoped(symtab, "ct_mark.force_snat", NULL,
+                                    "ct_mark["
+                                    OVN_CT_STR(OVN_CT_LB_FORCE_SNAT_BIT)
+                                    "]",
+                                    WR_CT_COMMIT);
 
     expr_symtab_add_field_scoped(symtab, "ct_label", MFF_CT_LABEL, NULL,
                                  false, WR_CT_COMMIT);
@@ -167,6 +177,16 @@ ovn_init_symtab(struct shash *symtab)
                                     "ct_label[80..95]", WR_CT_COMMIT);
     expr_symtab_add_subfield_scoped(symtab, "ct_label.label", NULL,
                                     "ct_label[96..127]", WR_CT_COMMIT);
+    expr_symtab_add_subfield_scoped(symtab, "ct_label.skip_snat", NULL,
+                                    "ct_label["
+                                    OVN_CT_STR(OVN_CT_LB_SKIP_SNAT_BIT)
+                                    "]",
+                                    WR_CT_COMMIT);
+    expr_symtab_add_subfield_scoped(symtab, "ct_label.force_snat", NULL,
+                                    "ct_label["
+                                    OVN_CT_STR(OVN_CT_LB_FORCE_SNAT_BIT)
+                                    "]",
+                                    WR_CT_COMMIT);
 
     expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false);
 

diff --git a/northd/northd.c b/northd/northd.c
@@ -9914,6 +9914,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
 {
     const char *ct_natted = ct_lb_mark ? "ct_mark.natted" : "ct_label.natted";
     char *skip_snat_new_action = NULL;
+    char *force_snat_new_action = NULL;
     char *skip_snat_est_action = NULL;
     char *new_match;
     char *est_match;
@@ -9924,6 +9925,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.
@@ -9942,8 +9948,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
     enum lb_snat_type snat_type = NO_FORCE_SNAT;
     if (smap_get_bool(&lb->nlb->options, "skip_snat", false)) {
         snat_type = 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;");
     }
@@ -10003,11 +10010,17 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
                       lb_vip->vip_port);
     }
 
+    force_snat_new_action = xasprintf("flags.force_snat_for_lb = 1; %s%s",
+                                      ds_cstr(action),
+                                      drop ? "" : "; force_snat);");
+    if (!drop) {
+        ds_put_cstr(action, ");");
+    }
+
     for (size_t i = 0; i < lb->n_nb_lr; i++) {
         struct ovn_datapath *od = lb->nb_lr[i];
         char *new_match_p = new_match;
         char *est_match_p = est_match;
-        char *est_actions = NULL;
         const char *meter = NULL;
 
         if (reject) {
@@ -10055,17 +10068,12 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
                                     est_match_p, skip_snat_est_action,
                                     &lb->nlb->header_);
         } else if (snat_type == FORCE_SNAT) {
-            char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s",
-                                          ds_cstr(action));
             ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio,
-                                      new_match_p, new_actions, NULL,
+                                      new_match_p, force_snat_new_action, NULL,
                                       meter, &lb->nlb->header_);
-            free(new_actions);
-
-            est_actions = xasprintf("flags.force_snat_for_lb = 1; "
-                                    "next;");
             ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DNAT, prio,
-                                    est_match_p, est_actions,
+                                    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,
@@ -10084,7 +10092,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
         }
 
         if (!od->n_l3dgw_ports || !lb_vip->n_backends) {
-            goto next;
+            continue;
         }
 
         char *undnat_match_p = xasprintf(
@@ -10098,7 +10106,8 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
                                     &lb->nlb->header_);
         } else if (snat_type == FORCE_SNAT) {
             ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120,
-                                    undnat_match_p, est_actions,
+                                    undnat_match_p,
+                                    "flags.force_snat_for_lb = 1; next;",
                                     &lb->nlb->header_);
         } else {
             ovn_lflow_add_with_hint(
@@ -10107,14 +10116,13 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip,
                 &lb->nlb->header_);
         }
         free(undnat_match_p);
-next:
-        free(est_actions);
     }
 
     ds_destroy(&unsnat_match);
     ds_destroy(&undnat_match);
 
     free(skip_snat_new_action);
+    free(force_snat_new_action);
     free(skip_snat_est_action);
     free(est_match);
     free(new_match);
@@ -13450,7 +13458,7 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
     ovn_lflow_add(lflows, od, S_ROUTER_OUT_EGR_LOOP, 0, "1", "next;");
     ovn_lflow_add(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 0, "1", "next;");
 
-    /* Ingress DNAT and DEFRAG Table (Priority 50).
+    /* Ingress DNAT and DEFRAG Table (Priority 50/70).
      *
      * The defrag stage needs to have flows for ICMP in order to get
      * the correct ct_state that can be used by DNAT stage.
@@ -13463,10 +13471,29 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath *od, struct hmap *lflows,
      * related traffic such as an ICMP Port Unreachable through
      * that's generated from a non-listening UDP port.  */
     if (od->has_lb_vip) {
+        ds_clear(match);
+        const char *ct_flag_reg = ct_lb_mark ? "ct_mark" : "ct_label";
+
+        ds_put_cstr(match, "ct.rel && !ct.est && !ct.new");
+        size_t match_len = match->length;
+
         ovn_lflow_add(lflows, od, S_ROUTER_IN_DEFRAG, 50, "icmp || icmp6",
                       "ct_dnat;");
+
+        ds_put_format(match, " && %s.skip_snat == 1", ct_flag_reg);
+        ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 70, ds_cstr(match),
+                      "flags.skip_snat_for_lb = 1; ct_commit_nat;");
+
+        ds_truncate(match, match_len);
+        ds_put_format(match, " && %s.force_snat == 1", ct_flag_reg);
+        ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 70, ds_cstr(match),
+                      "flags.force_snat_for_lb = 1; ct_commit_nat;");
+
+        ds_truncate(match, match_len);
         ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
                       "ct.rel && !ct.est && !ct.new", "ct_commit_nat;");
+
+        ds_clear(match);
     }
 
     /* If the router has load balancer or DNAT rules, re-circulate every packet

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
@@ -3278,7 +3278,9 @@ icmp6 {
             For the related traffic, a priority 50 flow that matches
             <code>ct.rel &amp;&amp; !ct.est &amp;&amp; !ct.new </code>
             with an action of <code>ct_commit_nat;</code>, if the router
-            has load balancer assigned to it.
+            has load balancer assigned to it. Along with two priority 70 flows
+            that match <code>skip_snat</code> and <code>force_snat</code>
+            flags.
         </p>
       </li>
     </ul>

diff --git a/ovn-sb.xml b/ovn-sb.xml
@@ -1998,7 +1998,7 @@
         </dd>
 
         <dt><code>ct_lb;</code></dt>
-        <dt><code>ct_lb(backends=<var>ip</var>[:<var>port</var>][,...][; hash_fields=<var>field1</var>,<var>field2</var>,...]);</code></dt>
+        <dt><code>ct_lb(backends=<var>ip</var>[:<var>port</var>][,...][; hash_fields=<var>field1</var>,<var>field2</var>,...][; ct_flag]);</code></dt>
         <dd>
           <p>
             With arguments, <code>ct_lb</code> commits the packet
@@ -2010,7 +2010,10 @@
             <code>dp_hash</code> is used as the OpenFlow group selection
             method, but if <code>hash_fields</code> is specified,
             <code>hash</code> is used as the selection method, and the fields
-            listed are used as the hash fields.
+            listed are used as the hash fields. The <code>ct_flag</code>
+            field represents one of supported flag: <code>skip_snat</code> or
+            <code>force_snat</code>, this flag will be stored in
+            <code>ct_label</code> register.
           </p>
           <p>
             Without arguments, <code>ct_lb</code> sends the packet to the
@@ -2032,7 +2035,7 @@
         </dd>
 
         <dt><code>ct_lb_mark;</code></dt>
-        <dt><code>ct_lb_mark(backends=<var>ip</var>[:<var>port</var>][,...][; hash_fields=<var>field1</var>,<var>field2</var>,...]);</code></dt>
+        <dt><code>ct_lb_mark(backends=<var>ip</var>[:<var>port</var>][,...][; hash_fields=<var>field1</var>,<var>field2</var>,...][; ct_flag]);</code></dt>
         <dd>
           <p>
               Same as <code>ct_lb</code>, except that it internally uses ct_mark