northd, controller: Use ct_next to get the CT state for direct SNAT.

In order to get the direct SNAT access working we need to commit new
connections so the reply is not marked as invalid. The CT state to
determine if the connection should be committed was populated by
ct_snat action, however this action also performs the NAT part
if the connection is already known and there is an entry for that.
This would cause issues when the there is combination of FIPs and
SNAT with very broad logical IP. To prevent that use ct_next only
which will populate the state but won't do the NAT part which can
be done later on if needed according to the conditions.

At the same time add support for ct_next in SNAT zone as ct_next
was assuming that the zone is always DNAT.

Fixes: 40136a2 ("northd: Fix direct access to SNAT network.")
Reported-at: https://issues.redhat.com/browse/FDP-744
Signed-off-by: Ales Musil <amusil@redhat.com>

controller/chassis.c

@@ -390,6 +390,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg *ovs_cfg,
     smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
     smap_replace(config, OVN_FEATURE_SAMPLE_WITH_REGISTERS,
                  ovs_cfg->sample_with_regs ? "true" : "false");
+    smap_replace(config, OVN_FEATURE_CT_NEXT_ZONE, "true");
 }
 
 /*
@@ -549,6 +550,12 @@ chassis_other_config_changed(const struct ovs_chassis_cfg *ovs_cfg,
         return true;
     }
 
+    if (!smap_get_bool(&chassis_rec->other_config,
+                       OVN_FEATURE_CT_NEXT_ZONE,
+                       false)) {
+        return true;
+    }
+
     return false;
 }
 
@@ -706,6 +713,7 @@ update_supported_sset(struct sset *supported)
     sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
     sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
     sset_add(supported, OVN_FEATURE_SAMPLE_WITH_REGISTERS);
+    sset_add(supported, OVN_FEATURE_CT_NEXT_ZONE);
 }
 
 static void

include/ovn/actions.h

@@ -260,6 +260,7 @@ struct ovnact_push_pop {
 /* OVNACT_CT_NEXT. */
 struct ovnact_ct_next {
     struct ovnact ovnact;
+    bool dnat_zone;
     uint8_t ltable;                /* Logical table ID of next table. */
 };
 

include/ovn/features.h

@@ -30,6 +30,7 @@
 #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
 #define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
 #define OVN_FEATURE_SAMPLE_WITH_REGISTERS "ovn-sample-with-registers"
+#define OVN_FEATURE_CT_NEXT_ZONE "ct-next-zone"
 
 /* OVS datapath supported features.  Based on availability OVN might generate
  * different types of openflows.

lib/actions.c

@@ -701,13 +701,32 @@ parse_CT_NEXT(struct action_context *ctx)
     }
 
     add_prerequisite(ctx, "ip");
-    ovnact_put_CT_NEXT(ctx->ovnacts)->ltable = ctx->pp->cur_ltable + 1;
+    struct ovnact_ct_next *ct_next = ovnact_put_CT_NEXT(ctx->ovnacts);
+    ct_next->dnat_zone = true;
+    ct_next->ltable = ctx->pp->cur_ltable + 1;
+
+    if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
+        return;
+    }
+
+    if (lexer_match_id(ctx->lexer, "dnat")) {
+        ct_next->dnat_zone = true;
+    } else if (lexer_match_id(ctx->lexer, "snat")) {
+        ct_next->dnat_zone = false;
+    } else {
+        lexer_error(ctx->lexer, "\"ct_next\" action accepts only"
+                                " \"dnat\" or \"snat\" parameter.");
+        return;
+    }
+
+    lexer_force_match(ctx->lexer, LEX_T_RPAREN);
 }
 
 static void
 format_CT_NEXT(const struct ovnact_ct_next *ct_next OVS_UNUSED, struct ds *s)
 {
-    ds_put_cstr(s, "ct_next;");
+    ds_put_cstr(s, "ct_next");
+    ds_put_cstr(s, ct_next->dnat_zone ? "(dnat);" : "(snat);");
 }
 
 static void
@@ -719,11 +738,17 @@ encode_CT_NEXT(const struct ovnact_ct_next *ct_next,
 
     struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
     ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next->ltable;
-    ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
-                            : mf_from_id(MFF_LOG_DNAT_ZONE);
     ct->zone_src.ofs = 0;
     ct->zone_src.n_bits = 16;
 
+    if (ep->is_switch) {
+        ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+    } else {
+        ct->zone_src.field = mf_from_id(ct_next->dnat_zone
+                                        ? MFF_LOG_DNAT_ZONE
+                                        : MFF_LOG_SNAT_ZONE);
+    }
+
     ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
     ofpacts->header = ct;
     ofpact_finish_CT(ofpacts, &ct);

northd/en-global-config.c

@@ -382,6 +382,7 @@ northd_enable_all_features(struct ed_type_global_config *data)
         .ct_commit_nat_v2 = true,
         .ct_commit_to_zone = true,
         .sample_with_reg = true,
+        .ct_next_zone = true,
     };
 }
 
@@ -452,6 +453,15 @@ build_chassis_features(const struct sbrec_chassis_table *sbrec_chassis_table,
             chassis_features->sample_with_reg) {
             chassis_features->sample_with_reg = false;
         }
+
+        bool ct_next_zone =
+                smap_get_bool(&chassis->other_config,
+                              OVN_FEATURE_CT_NEXT_ZONE,
+                              false);
+        if (!ct_next_zone &&
+            chassis_features->ct_next_zone) {
+            chassis_features->ct_next_zone = false;
+        }
     }
 }
 

northd/en-global-config.h

@@ -20,6 +20,7 @@ struct chassis_features {
     bool ct_commit_nat_v2;
     bool ct_commit_to_zone;
     bool sample_with_reg;
+    bool ct_next_zone;
 };
 
 struct global_config_tracked_data {

northd/northd.c

@@ -16079,14 +16079,14 @@ build_lrouter_out_snat_flow(struct lflow_table *lflows,
     /* For the SNAT networks, we need to make sure that connections are
      * properly tracked so we can decide whether to perform SNAT on traffic
      * exiting the network. */
-    if (features->ct_commit_to_zone && !strcmp(nat->type, "snat") &&
-        !od->is_gw_router) {
+    if (features->ct_commit_to_zone && features->ct_next_zone &&
+        !strcmp(nat->type, "snat") && !od->is_gw_router) {
         /* For traffic that comes from SNAT network, initiate CT state before
          * entering S_ROUTER_OUT_SNAT to allow matching on various CT states.
          */
         ds_truncate(match, original_match_len);
         ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 70,
-                      ds_cstr(match), "ct_snat;",
+                      ds_cstr(match), "ct_next(snat);",
                       lflow_ref);
 
         build_lrouter_out_snat_match(lflows, od, nat, match,

ovn-sb.xml

@@ -1369,6 +1369,8 @@
         </dd>
 
         <dt><code>ct_next;</code></dt>
+        <dt><code>ct_next(dnat);</code></dt>
+        <dt><code>ct_next(snat);</code></dt>
         <dd>
           <p>
             Apply connection tracking to the flow, initializing

tests/ovn.at

@@ -1263,11 +1263,27 @@ ct_lb_mark(backends=192.168.1.2:80,192.168.1.3:80; hash_fields="eth_src,eth_dst,
 
 # ct_next
 ct_next;
+    formats as ct_next(dnat);
+    encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]])
+    has prereqs ip
+ct_next(dnat);
+    encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]])
+    has prereqs ip
+ct_next(snat);
     encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]])
     has prereqs ip
 ct_clear; ct_next;
+    formats as ct_clear; ct_next(dnat);
     encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]])
     has prereqs ip
+ct_next(snat, dnat);
+    Syntax error at `,' expecting `)'.
+ct_next(dnat, ignore);
+    Syntax error at `,' expecting `)'.
+ct_next(ignore);
+    "ct_next" action accepts only "dnat" or "snat" parameter.
+ct_next();
+    "ct_next" action accepts only "dnat" or "snat" parameter.
 
 # ct_commit
 ct_commit;

tests/system-ovn.at

@@ -3518,7 +3518,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.3 192.168.1.2 foo1 00:0
 AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.1.3 foo2 00:00:02:02:03:05])
 
 # Add a SNAT rule
-AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16])
+AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 0.0.0.0/0])
 
 # Add default route to ext-net
 AT_CHECK([ovn-nbctl lr-route-add R1 10.0.0.0/24 172.16.1.2])
@@ -3724,8 +3724,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::3 fd11::2 foo1 00:00:02:02
 AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::4 fd11::3 foo2 00:00:02:02:03:05])
 
 # Add a SNAT rule
-AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd11::/64])
-AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd12::/64])
+AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 ::/0])
 
 ovn-nbctl --wait=hv sync
 OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=fd20::1)'])
@@ -3920,7 +3919,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.3 192.168.1.2 foo1 00:0
 AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat 172.16.1.4 192.168.2.2 bar1 00:00:02:02:03:05])
 
 # Add a SNAT rule
-AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.0.0/16])
+AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 0.0.0.0/0])
 
 ovn-nbctl --wait=hv sync
 OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=172.16.1.1)'])
@@ -4104,8 +4103,7 @@ AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::3 fd11::2 foo1 00:00:02:02
 AT_CHECK([ovn-nbctl lr-nat-add R1 dnat_and_snat fd20::4 fd12::2 bar1 00:00:02:02:03:05])
 
 # Add a SNAT rule
-AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd11::/64])
-AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 fd12::/64])
+AT_CHECK([ovn-nbctl lr-nat-add R1 snat fd20::1 ::/0])
 
 ovn-nbctl --wait=hv sync
 OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int | grep 'nat(src=fd20::1)'])