Hi Dumitru Thanks for the patch.
On Tue, Jan 23, 2024 at 10:44 PM Mark Michelson <mmich...@redhat.com> wrote: > Hi Dumitru, > > Acked-by: Mark Michelson <mmich...@redhat.com> > > I think this change is good. I looked through the documentation for the > "external_port_range" column in the NAT table. This change appears to > actually make the documentation *more* accurate rather than to introduce > any potentially undesirable behavior changes. > > What I find more telling is that the only tests you had to update were > the actions tests. We apparently don't have any system tests that use > NAT with an external port range. That's not great :( > > On 1/22/24 09:54, Dumitru Ceara wrote: > > This is to avoid unexpected behavior changes due to the underlying > > datapath (e.g., kernel) changing defaults. If we don't explicitly > > request a port selection algorithm, OVS leaves it up to the > > datapath to decide how to do the port selection. Currently that means > > that source port allocation is not random if the original source port > > fits in the requested range. > This looks good to me. In most cases, a source port within range was not translated w/ existing behavior ovn-nbctl --portrange lr-nat-add rtr snat 66.66.66.66 42.42.42.0/24 10000-20000 43.43.43.2:15000 -> 42.42.42.2:8080 => 66.66.66.66:15000 -> 42.42.42.2:80 # no PAT as within range But it was still translated in some cases: ovn-nbctl --portrange lr-nat-add rtr snat 66.66.66.66 42.42.42.0/24 10000-20000 43.43.43.2:40000 -> 42.42.42.2:8080 => 66.66.66.66:15000 -> 42.42.42.2:80 # PAT. dst port happens to be 15000 43.43.43.2:15000 -> 42.42.42.2:8080 => 66.66.66.66:XXX -> 42.42.42.2:80 # PAT to XXX != 15000 as 15000 already used. In other words, I do not see how one could rely on existing (pre-patch) behavior (no PAT if the source port is within the range) as it is not guaranteed. > > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2024-January/410847.html > > Reported-at: https://issues.redhat.com/browse/FDP-301 > > Fixes: 60bdc8ea78d7 ("NAT: Provide port range in input") > > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > > --- > > lib/actions.c | 16 ++++++++++++++-- > > tests/ovn.at | 8 ++++---- > > 2 files changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/lib/actions.c b/lib/actions.c > > index 38cf4642d6..fdc0529de6 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -1131,8 +1131,20 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > > } > > > > if (cn->port_range.exists) { > > - nat->range.proto.min = cn->port_range.port_lo; > > - nat->range.proto.max = cn->port_range.port_hi; > > + nat->range.proto.min = cn->port_range.port_lo; > > + nat->range.proto.max = cn->port_range.port_hi; > > + > > + /* Explicitly set the port selection algorithm to "random". > Otherwise > > + * it's up to the datapath to choose how to select the port and > that > > + * might create unexpected behavior changes when the datapath > defaults > > + * change. > > + * > > + * NOTE: for the userspace datapath the "random" function > doesn't > > + * really generate random ports, it uses "hash" under the hood: > > + * https://issues.redhat.com/browse/FDP-269. */ > > + if (nat->range.proto.min && nat->range.proto.max) { > > + nat->flags |= NX_NAT_F_PROTO_RANDOM; > > + } > > } > > > > ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 8cc4c311c1..e8cef83cb5 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -1368,7 +1368,7 @@ ct_dnat(fd11::2); > > has prereqs ip > > ct_dnat(192.168.1.2, 1-3000); > > formats as ct_dnat(192.168.1.2,1-3000); > > - encodes as > ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000)) > > + encodes as > ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1 > -3000,random)) > > has prereqs ip > > > > ct_dnat(192.168.1.2, 192.168.1.3); > > @@ -1402,7 +1402,7 @@ ct_dnat_in_czone(fd11::2); > > has prereqs ip > > ct_dnat_in_czone(192.168.1.2, 1-3000); > > formats as ct_dnat_in_czone(192.168.1.2,1-3000); > > - encodes as > ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000)) > > + encodes as > ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1 > -3000,random)) > > has prereqs ip > > > > ct_dnat_in_czone(192.168.1.2, 192.168.1.3); > > @@ -1436,7 +1436,7 @@ ct_snat(fd11::2); > > has prereqs ip > > ct_snat(192.168.1.2, 1-3000); > > formats as ct_snat(192.168.1.2,1-3000); > > - encodes as > ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000)) > > + encodes as > ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1 > -3000,random)) > > has prereqs ip > > > > ct_snat(192.168.1.2, 192.168.1.3); > > @@ -1470,7 +1470,7 @@ ct_snat_in_czone(fd11::2); > > has prereqs ip > > ct_snat_in_czone(192.168.1.2, 1-3000); > > formats as ct_snat_in_czone(192.168.1.2,1-3000); > > - encodes as > ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(src=192.168.1.2:1-3000)) > > + encodes as > ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(src=192.168.1.2:1 > -3000,random)) > > has prereqs ip > > > > ct_snat_in_czone(192.168.1.2, 192.168.1.3); > > Acked-by: Xavier Simonart <xsimo...@redhat.com> Thanks Xavier > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
