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: 40136a2f2c84 ("northd: Fix direct access to SNAT network.")
Reported-at: https://issues.redhat.com/browse/FDP-744
Signed-off-by: Ales Musil <[email protected]>
---
v2: Make sure we don't SNAT FIP reply traffic
---
controller/chassis.c | 8 ++++++++
include/ovn/actions.h | 1 +
include/ovn/features.h | 1 +
lib/actions.c | 33 +++++++++++++++++++++++++++++----
northd/en-global-config.c | 10 ++++++++++
northd/en-global-config.h | 1 +
northd/northd.c | 8 +++-----
ovn-sb.xml | 2 ++
tests/ovn-northd.at | 23 ++++++++++++++---------
tests/ovn.at | 16 ++++++++++++++++
tests/system-ovn.at | 10 ++++------
11 files changed, 89 insertions(+), 24 deletions(-)
diff --git a/controller/chassis.c b/controller/chassis.c
index 2991a0af3..ee839084a 100644
--- a/controller/chassis.c
+++ b/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
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index c8dd66ed8..a95a0daf7 100644
--- a/include/ovn/actions.h
+++ b/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. */
};
diff --git a/include/ovn/features.h b/include/ovn/features.h
index 4275f7526..3566ab60f 100644
--- a/include/ovn/features.h
+++ b/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.
diff --git a/lib/actions.c b/lib/actions.c
index c12d087e7..2e05d4134 100644
--- a/lib/actions.c
+++ b/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);
diff --git a/northd/en-global-config.c b/northd/en-global-config.c
index 0ce7f8308..fff2aaa16 100644
--- a/northd/en-global-config.c
+++ b/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;
+ }
}
}
diff --git a/northd/en-global-config.h b/northd/en-global-config.h
index 0cf34482a..767810542 100644
--- a/northd/en-global-config.h
+++ b/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 {
diff --git a/northd/northd.c b/northd/northd.c
index ed959535b..ed5706f40 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -16051,7 +16051,6 @@ build_lrouter_out_snat_flow(struct lflow_table *lflows,
build_lrouter_out_snat_match(lflows, od, nat, match, distributed_nat,
cidr_bits, is_v6, l3dgw_port, lflow_ref,
false);
- size_t original_match_len = match->length;
if (!od->is_gw_router && distributed_nat) {
ds_put_format(actions, "eth.src = "ETH_ADDR_FMT"; ",
@@ -16073,14 +16072,13 @@ 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,
diff --git a/ovn-sb.xml b/ovn-sb.xml
index c11296d7c..95116ac56 100644
--- a/ovn-sb.xml
+++ b/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
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 93ccbce6b..564c9446e 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1133,7 +1133,8 @@ ovn_start
# DR is connected to S1 and CR is connected to S2
check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \
- -- set chassis gw1 other_config:ct-commit-to-zone="true"
+ -- set chassis gw1 other_config:ct-commit-to-zone="true" \
+ -- set chassis gw1 other_config:ct-next-zone="true"
check ovn-nbctl lr-add DR
check ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24
@@ -5721,7 +5722,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows |
ovn_strip_lflows], [0], [dnl
])
check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \
- -- set chassis gw1 other_config:ct-commit-to-zone="true"
+ -- set chassis gw1 other_config:ct-commit-to-zone="true" \
+ -- set chassis gw1 other_config:ct-next-zone="true"
# Create a distributed gw port on lr0
check ovn-nbctl ls-add public
@@ -5822,8 +5824,8 @@ AT_CHECK([grep "lr_out_undnat" lr0flows |
ovn_strip_lflows], [0], [dnl
AT_CHECK([grep "lr_out_post_undnat" lr0flows | ovn_strip_lflows], [0], [dnl
table=??(lr_out_post_undnat ), priority=0 , match=(1), action=(next;)
- table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.0/24 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
- table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.10 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
+ table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.0/24 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
action=(ct_next(snat);)
+ table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.10 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
action=(ct_next(snat);)
])
AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl
@@ -5980,8 +5982,8 @@ AT_CHECK([grep "lr_out_undnat" lr0flows |
ovn_strip_lflows], [0], [dnl
AT_CHECK([grep "lr_out_post_undnat" lr0flows | ovn_strip_lflows], [0], [dnl
table=??(lr_out_post_undnat ), priority=0 , match=(1), action=(next;)
- table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.0/24 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
- table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.10 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
+ table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.0/24 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
action=(ct_next(snat);)
+ table=??(lr_out_post_undnat ), priority=70 , match=(ip && ip4.src == 10.0.0.10 && outport ==
"lr0-public" && is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
action=(ct_next(snat);)
])
AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl
@@ -7876,13 +7878,16 @@ ovn_start
# distributed gateway LRPs.
check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \
- -- set chassis gw1 other_config:ct-commit-to-zone="true"
+ -- set chassis gw1 other_config:ct-commit-to-zone="true" \
+ -- set chassis gw1 other_config:ct-next-zone="true"
check ovn-sbctl chassis-add gw2 geneve 128.0.0.1 \
- -- set chassis gw2 other_config:ct-commit-to-zone="true"
+ -- set chassis gw2 other_config:ct-commit-to-zone="true" \
+ -- set chassis gw2 other_config:ct-next-zone="true"
check ovn-sbctl chassis-add gw3 geneve 129.0.0.1 \
- -- set chassis gw3 other_config:ct-commit-to-zone="true"
+ -- set chassis gw3 other_config:ct-commit-to-zone="true" \
+ -- set chassis gw3 other_config:ct-next-zone="true"
check ovn-nbctl lr-add DR
check ovn-nbctl lrp-add DR DR-S1 02:ac:10:01:00:01 172.16.1.1/24
diff --git a/tests/ovn.at b/tests/ovn.at
index 50c9f04da..632f060cc 100644
--- a/tests/ovn.at
+++ b/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;
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 6e4ec4247..01f71161a 100644
--- a/tests/system-ovn.at
+++ b/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)'])
