On Fri, May 6, 2022 at 9:47 AM Mark Michelson <[email protected]> wrote:
> Commit 4deac4509 introduced the new "czone" variants for ct_dnat and > ct_snat. This makes it so that a single conntrack zone is used for both > DNAT and SNAT on the datapath. The common CT zone chosen in this commit > was the DNAT zone. > > The ct-snat-zone option for logical routers makes it so that an SNAT > zone can be explicitly configured instead of having a zone selected at > random by OVN. The problem is that since the DNAT zone is used as the > common zone ID, it means that when SNAT is performed, the SNAT is not > performed in the configured ct-snat-zone. > > The fix for this is to change to using the SNAT zone as the common zone > if ct-snat-zone is configured on the logical router. This way, SNAT will > use the configured zone as expected. DNAT will also use this zone when > possible. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2061619 > > Signed-off-by: Mark Michelson <[email protected]> > Thanks for fixing this. Acked-by: Numan Siddique <[email protected]> Numan > --- > controller/lflow.c | 18 +++++++++ > include/ovn/actions.h | 2 + > include/ovn/logical-fields.h | 2 - > lib/actions.c | 4 +- > tests/ovn.at | 72 ++++++++++++++++++++++++++++++++++++ > tests/test-ovn.c | 1 + > 6 files changed, 95 insertions(+), 4 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index b7ddeb1c0..3c953e18b 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -1104,6 +1104,23 @@ lflow_parse_ctrl_meter(const struct > sbrec_logical_flow *lflow, > } > } > > +static int > +get_common_nat_zone(const struct local_datapath *ldp) > +{ > + /* Normally, the common NAT zone defaults to the DNAT zone. However, > + * if the "snat-ct-zone" is set on the datapath, the user is > + * expecting an explicit CT zone to be used for SNAT. If we default > + * to the DNAT zone, then it means SNAT will not use the configured > + * value. The way we get around this is to use the SNAT zone as the > + * common zone if "snat-ct-zone" is set. > + */ > + if (smap_get(&ldp->datapath->external_ids, "snat-ct-zone")) { > + return MFF_LOG_SNAT_ZONE; > + } else { > + return MFF_LOG_DNAT_ZONE; > + } > +} > + > static void > add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, > const struct local_datapath *ldp, > @@ -1153,6 +1170,7 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *lflow, > .fdb_ptable = OFTABLE_GET_FDB, > .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB, > .ctrl_meter_id = ctrl_meter_id, > + .common_nat_ct_zone = get_common_nat_zone(ldp), > }; > ovnacts_encode(ovnacts->data, ovnacts->size, &ep, &ofpacts); > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index f55d77d47..547797584 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -808,6 +808,8 @@ struct ovnact_encode_params { > * 'lookup_fdb' to resubmit. */ > uint32_t ctrl_meter_id; /* Meter to be used if the resulting flow > sends packets to controller. */ > + uint32_t common_nat_ct_zone; /* When performing NAT in a common CT > zone, > + this determines which CT zone to use > */ > }; > > void ovnacts_encode(const struct ovnact[], size_t ovnacts_len, > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h > index 25b5a62a3..18516634e 100644 > --- a/include/ovn/logical-fields.h > +++ b/include/ovn/logical-fields.h > @@ -36,8 +36,6 @@ enum ovn_controller_event { > * (32 bits). */ > #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway > router > * (32 bits). */ > -#define MFF_LOG_NAT_ZONE MFF_LOG_DNAT_ZONE /* conntrack zone for both > snat > - * and dnat. */ > #define MFF_LOG_CT_ZONE MFF_REG13 /* Logical conntrack zone for lports > * (32 bits). */ > #define MFF_LOG_INPORT MFF_REG14 /* Logical input port (32 bits). */ > diff --git a/lib/actions.c b/lib/actions.c > index a3cf747db..a9c27600c 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -1131,7 +1131,7 @@ encode_CT_DNAT_IN_CZONE(const struct ovnact_ct_nat > *cn, > const struct ovnact_encode_params *ep, > struct ofpbuf *ofpacts) > { > - encode_ct_nat(cn, ep, false, MFF_LOG_NAT_ZONE, ofpacts); > + encode_ct_nat(cn, ep, false, ep->common_nat_ct_zone, ofpacts); > } > > static void > @@ -1139,7 +1139,7 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat > *cn, > const struct ovnact_encode_params *ep, > struct ofpbuf *ofpacts) > { > - encode_ct_nat(cn, ep, true, MFF_LOG_NAT_ZONE, ofpacts); > + encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts); > } > > static void > diff --git a/tests/ovn.at b/tests/ovn.at > index 799a6aabd..06c607218 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -30505,3 +30505,75 @@ test_mac_binding_flows 150 00:00:11:22:33:88 0 > > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([snat-ct-zone with common NAT zone]) > +# This test sets up a couple of simple NATs. OVN will program logical > +# flows for ct_snat_in_czone() and ct_dnat_in_czone() as a result. We > +# want to ensure that the common zone used by these flows is the DNAT > +# zone. Then, we will set the snat-ct-zone option on our datapath and > +# ensure that the flows now use the SNAT zone as the common zone. > + > +ovn_start > + > +net_add n1 > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +check ovs-vsctl add-port br-int ls0-hv -- set Interface ls0-hv > external-ids:iface-id=ls0-hv > + > +check ovn-nbctl lr-add lr0 > + > +check ovn-nbctl ls-add ls0 > +check ovn-nbctl lsp-add ls0 ls0-lr0 > +check ovn-nbctl lsp-set-type ls0-lr0 router > +check ovn-nbctl lsp-set-addresses ls0-lr0 router > +check ovn-nbctl lrp-add lr0 lr0-ls0 00:00:00:00:00:01 10.0.0.1 > + > +check ovn-nbctl lsp-add ls0 ls0-hv > +check ovn-nbctl lsp-set-addresses ls0-hv "00:00:00:00:00:02 10.0.0.2" > + > +check ovn-nbctl ls-add ext > +check ovn-nbctl lsp-add ext ext-lr0 > +check ovn-nbctl lsp-set-type ext-lr0 router > +check ovn-nbctl lsp-set-addresses ext-lr0 router > +check ovn-nbctl lrp-add lr0 lr0-ext 00:00:00:00:01:01 20.0.0.1 > + > +check ovn-nbctl lrp-set-gateway-chassis lr0-ext hv1 > + > +check ovn-nbctl lr-nat-add lr0 snat 172.16.0.2 10.0.0.0/24 > +check ovn-nbctl lr-nat-add lr0 dnat 172.16.0.2 10.0.0.2 > + > +check ovn-nbctl --wait=hv sync > + > +# Use constants so that if tables or registers change, this test can > +# be updated easily. > +DNAT_TABLE=14 > +SNAT_TABLE=43 > +DNAT_ZONE_REG="NXM_NX_REG11[[0..15]]" > +SNAT_ZONE_REG="NXM_NX_REG12[[0..15]]" > + > +dnat_zone=$(ovs-ofctl dump-flows br-int | grep table=$DNAT_TABLE | grep > -o zone=.*, | cut -d '=' -f 2) > +dnat_zone=${dnat_zone::-1} > +snat_zone=$(ovs-ofctl dump-flows br-int | grep table=$SNAT_TABLE | grep > priority=153 | grep -o zone=.*, | cut -d '=' -f 2) > +snat_zone=${snat_zone::-1} > + > +# For now, we expect that the common zone is the dnat zone > + > +check test "$dnat_zone" = "$DNAT_ZONE_REG" > +check test "$snat_zone" = "$DNAT_ZONE_REG" > + > +check ovn-nbctl --wait=hv set logical_router lr0 options:snat-ct-zone=666 > + > +dnat_zone=$(ovs-ofctl dump-flows br-int | grep table=$DNAT_TABLE | grep > -o zone=.*, | cut -d '=' -f 2) > +dnat_zone=${dnat_zone::-1} > +snat_zone=$(ovs-ofctl dump-flows br-int | grep table=$SNAT_TABLE | grep > priority=153 | grep -o zone=.*, | cut -d '=' -f 2) > +snat_zone=${snat_zone::-1} > + > +# Now with an snat-ct-zone set, the common zone is the snat zone > + > +check test "$dnat_zone" = "$SNAT_ZONE_REG" > +check test "$snat_zone" = "$SNAT_ZONE_REG" > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index d79c6a5bc..6704f612b 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -1351,6 +1351,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx > OVS_UNUSED) > .ct_snat_vip_ptable = OFTABLE_CT_SNAT_HAIRPIN, > .fdb_ptable = OFTABLE_GET_FDB, > .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB, > + .common_nat_ct_zone = MFF_LOG_DNAT_ZONE, > }; > struct ofpbuf ofpacts; > ofpbuf_init(&ofpacts, 0); > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
