On Wed, Aug 21, 2024 at 10:55 AM Ales Musil <[email protected]> wrote:
> 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]>
> ---
> 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 | 6 +++---
> ovn-sb.xml | 2 ++
> tests/ovn-northd.at | 23 ++++++++++++++---------
> tests/ovn.at | 16 ++++++++++++++++
> tests/system-ovn.at | 10 ++++------
> 11 files changed, 89 insertions(+), 22 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 73367b910..19a484ceb 100644
> --- a/northd/northd.c
> +++ b/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,
> 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..8816749c4 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")), 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")), 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")), 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")), 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)'])
> --
> 2.46.0
>
>
There is still an issue when external ports tries to access the FIP with
SNAT being 0.0.0.0/0. I'll send v2 eventually, please ignore this for now.
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected]
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
