| Message ID | 20240122145453.1925147-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] actions: Use random port selection for SNAT with external_port_range. | expand |
On Mon, Jan 22, 2024 at 3:55 PM Dumitru Ceara <dceara@redhat.com> 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. > > 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 <dceara@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); > -- > 2.39.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
Hi Dumitru, Acked-by: Mark Michelson <mmichels@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. > > 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 <dceara@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);
Hi Dumitru Thanks for the patch. On Tue, Jan 23, 2024 at 10:44 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Dumitru, > > Acked-by: Mark Michelson <mmichels@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 <dceara@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 <xsimonar@redhat.com> Thanks Xavier > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 1/29/24 15:33, Xavier Simonart wrote: > Hi Dumitru > > Thanks for the patch. > > On Tue, Jan 23, 2024 at 10:44 PM Mark Michelson <mmichels@redhat.com> wrote: > >> Hi Dumitru, >> >> Acked-by: Mark Michelson <mmichels@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 :( >> That's not great indeed, we should try to improve that in the future. >> 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. > > That makes sense, thanks for trying this out! Thanks for the reviews, Mark, Ales and Xavier! I pushed this to main and backported it to all stable branches down to 22.03. Regards, Dumitru
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);
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. 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 <dceara@redhat.com> --- lib/actions.c | 16 ++++++++++++++-- tests/ovn.at | 8 ++++---- 2 files changed, 18 insertions(+), 6 deletions(-)