On Wed, Mar 6, 2024 at 8:07 AM Ales Musil <amu...@redhat.com> wrote:

>
>
> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok <martin.kal...@canonical.com>
> wrote:
>
>> This change fixes bug that breaks ability of machines from external
>> networks to communicate with machines in SNATed networks (specifically
>> when using a Distributed router).
>>
>> Currently when a machine (S1) on an external network tries to talk
>> over TCP with a machine (A1) in a network that has enabled SNAT, the
>> connection is established successfully. However after the three-way
>> handshake, any packets that come from the A1 machine will have their
>> source address translated by the Distributed router, breaking the
>> communication.
>>
>> Existing rule in `build_lrouter_out_snat_flow` that decides which
>> packets should be SNATed already tries to avoid SNATing packets in
>> reply direction with `(!ct.trk || !ct.rpl)`. However, previous stages
>> in the distributed LR egress pipeline do not initiate the CT state.
>>
>> Additionally we need to commit new connections that originate from
>> external networks into CT, so that the packets in the reply direction
>> can be properly identified.
>>
>> Rationale:
>>
>> In my original RFC [0], there were questions about the motivation for
>> fixing this issue. I'll try to summarize why I think this is a bug
>> that should be fixed.
>>
>> 1. Current implementation for Distributed router already tries to
>>    avoid SNATing packets in the reply direction, it's just missing
>>    initialized CT states to make proper decisions.
>>
>> 2. This same scenario works with Gateway Router. I tested with
>>    following setup:
>>
>>     foo -- R1 -- join - R3 -- alice
>>                   |
>>     bar ----------R2
>>
>>     R1 is a Distributed router with SNAT for foo. R2 is a Gateway
>>     router with SNAT for bar. R3 is a Gateway router with no SNAT.
>>     Using 'alice1' as a client I was able to talk over TCP with
>>     'bar1' but connection with 'foo1' failed.
>>
>> 3. Regarding security and "leaking" of internal IPs. Reading through
>>    RFC 4787 [1], 5382 [2] and their update in 7857 [3], the
>>    specifications do not seem to mandate that SNAT implementations
>>    must filter incoming traffic destined directly to the internal
>>    network. Sections like "5. Filtering Behavior" in 4787 and
>>    "4.3 Externally Initiated Connections" in 5382 describe only
>>    behavior for traffic destined to external IP/Port associated
>>    with NAT on the device that performs NAT.
>>
>>    Besides, with the current implementation, it's already possible
>>    to scan the internal network with pings and TCP syn scanning.
>>
>>    If an additional security is needed, ACLs can be used to filter
>>    out incomming traffic from external networks.
>>
>> 4. We do have customers/clouds that depend on this functionality.
>>    This is a scenario that used to work in Openstack with ML2/OVS
>>    and migrating those clouds to ML2/OVN would break it.
>>
>> [0]
>> https://mail.openvswitch.org/pipermail/ovs-dev/2024-February/411670.html
>> [1]https://datatracker.ietf.org/doc/html/rfc4787
>> [2]https://datatracker.ietf.org/doc/html/rfc5382
>> [3]https://datatracker.ietf.org/doc/html/rfc7857
>>
>> Signed-off-by: Martin Kalcok <martin.kal...@canonical.com>
>> ---
>>
>
> Hi Martin,
>
> I have a few comments down below.
>
>
>>  northd/northd.c         | 82 ++++++++++++++++++++++++++++++++++++-----
>>  northd/ovn-northd.8.xml | 29 +++++++++++++++
>>  tests/ovn-northd.at     | 15 +++++++-
>>  tests/system-ovn.at     | 69 ++++++++++++++++++++++++++++++++++
>>  4 files changed, 185 insertions(+), 10 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 2c3560ce2..4b79b357c 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -14437,21 +14437,28 @@ build_lrouter_out_is_dnat_local(struct
>> lflow_table *lflows,
>>  }
>>
>>  static void
>> -build_lrouter_out_snat_match(struct lflow_table *lflows,
>> -                             const struct ovn_datapath *od,
>> -                             const struct nbrec_nat *nat, struct ds
>> *match,
>> -                             bool distributed_nat, int cidr_bits, bool
>> is_v6,
>> -                             struct ovn_port *l3dgw_port,
>> -                             struct lflow_ref *lflow_ref)
>> +build_lrouter_out_snat_direction_match__(struct lflow_table *lflows,
>> +                                         const struct ovn_datapath *od,
>> +                                         const struct nbrec_nat *nat,
>> +                                         struct ds *match,
>> +                                         bool distributed_nat, int
>> cidr_bits,
>> +                                         bool is_v6,
>> +                                         struct ovn_port *l3dgw_port,
>> +                                         struct lflow_ref *lflow_ref,
>> +                                         bool is_reverse)
>>  {
>>      ds_clear(match);
>>
>> -    ds_put_format(match, "ip && ip%c.src == %s", is_v6 ? '6' : '4',
>> +    ds_put_format(match, "ip && ip%c.%s == %s",
>> +                  is_v6 ? '6' : '4',
>> +                  is_reverse ? "dst" : "src",
>>                    nat->logical_ip);
>>
>>      if (!od->is_gw_router) {
>>          /* Distributed router. */
>> -        ds_put_format(match, " && outport == %s", l3dgw_port->json_key);
>> +        ds_put_format(match, " && %s == %s",
>> +                      is_reverse ? "inport" : "outport",
>> +                      l3dgw_port->json_key);
>>          if (od->n_l3dgw_ports) {
>>              ds_put_format(match, " && is_chassis_resident(\"%s\")",
>>                            distributed_nat
>> @@ -14462,11 +14469,37 @@ build_lrouter_out_snat_match(struct lflow_table
>> *lflows,
>>
>>      if (nat->allowed_ext_ips || nat->exempted_ext_ips) {
>>          lrouter_nat_add_ext_ip_match(od, lflows, match, nat,
>> -                                     is_v6, false, cidr_bits,
>> +                                     is_v6, is_reverse, cidr_bits,
>>                                       lflow_ref);
>>      }
>>  }
>>
>> +static void
>> +build_lrouter_out_snat_match(struct lflow_table *lflows,
>> +                             const struct ovn_datapath *od,
>> +                             const struct nbrec_nat *nat, struct ds
>> *match,
>> +                             bool distributed_nat, int cidr_bits, bool
>> is_v6,
>> +                             struct ovn_port *l3dgw_port,
>> +                             struct lflow_ref *lflow_ref)
>> +{
>> +    build_lrouter_out_snat_direction_match__(lflows, od, nat, match,
>> +                                             distributed_nat, cidr_bits,
>> is_v6,
>> +                                             l3dgw_port, lflow_ref,
>> false);
>> +}
>> +
>> +static void
>> +build_lrouter_out_snat_reverse_match(struct lflow_table *lflows,
>> +                             const struct ovn_datapath *od,
>> +                             const struct nbrec_nat *nat, struct ds
>> *match,
>> +                             bool distributed_nat, int cidr_bits, bool
>> is_v6,
>> +                             struct ovn_port *l3dgw_port,
>> +                             struct lflow_ref *lflow_ref)
>> +{
>> +    build_lrouter_out_snat_direction_match__(lflows, od, nat, match,
>> +                                             distributed_nat, cidr_bits,
>> is_v6,
>> +                                             l3dgw_port, lflow_ref,
>> true);
>> +}
>> +
>>
>
> nit: I don't think we need two new functions when only one argument was
> added. Considering the old function was used at 3 places only anyway.
>
>
>>  static void
>>  build_lrouter_out_snat_stateless_flow(struct lflow_table *lflows,
>>                                        const struct ovn_datapath *od,
>> @@ -14591,6 +14624,7 @@ build_lrouter_out_snat_flow(struct lflow_table
>> *lflows,
>>          return;
>>      }
>>
>> +    struct ds match_all_from_snat = DS_EMPTY_INITIALIZER;
>>      ds_clear(actions);
>>
>>      /* The priority here is calculated such that the
>> @@ -14600,6 +14634,7 @@ 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);
>> +    ds_clone(&match_all_from_snat, match);
>>
>>      if (!od->is_gw_router) {
>>          /* Distributed router. */
>> @@ -14624,6 +14659,35 @@ build_lrouter_out_snat_flow(struct lflow_table
>> *lflows,
>>                              priority, ds_cstr(match),
>>                              ds_cstr(actions), &nat->header_,
>>                              lflow_ref);
>> +
>> +    /* 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 (!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.
>> +         */
>> +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 70,
>> +                      ds_cstr(&match_all_from_snat), "ct_snat; ",
>> +                      lflow_ref);
>>
> +
>> +        build_lrouter_out_snat_reverse_match(lflows, od, nat, match,
>> +                                             distributed_nat, cidr_bits,
>> is_v6,
>> +                                             l3dgw_port, lflow_ref);
>> +
>> +        /* New traffic that goes into SNAT network is committed to CT to
>> avoid
>> +         * SNAT-ing replies.*/
>> +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, priority,
>> +                      ds_cstr(match), "ct_snat;",
>> +                      lflow_ref);
>> +
>> +        ds_put_cstr(match, "&& ct.new");
>> +        ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, priority,
>> +                      ds_cstr(match), "ct_commit(snat); next; ",
>> +                      lflow_ref);
>> +    }
>> +
>> +    ds_destroy(&match_all_from_snat);
>>
>
> I suppose that the gw router works because we have ct_commit for new
> traffic [0]. I wonder if we could extend that with a check for the DGP case.
>
>
>>  }
>>
>>  static void
>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>> index 17b414144..ce6fd1270 100644
>> --- a/northd/ovn-northd.8.xml
>> +++ b/northd/ovn-northd.8.xml
>> @@ -4869,6 +4869,13 @@ nd_ns {
>>
>>      <p>
>>        <ul>
>> +        <li>
>> +          A priority-70 logical flow is added that initiates CT state for
>> +          traffic that is configured to be SNATed on Distributed routers.
>> +          This allows the next table, <code>lr_out_snat</code>, to
>> +          effectively match on various CT states.
>> +        </li>
>> +
>>          <li>
>>            A priority-50 logical flow is added that commits any untracked
>> flows
>>            from the previous table <code>lr_out_undnat</code> for Gateway
>> @@ -5061,6 +5068,18 @@ nd_ns {
>>
>>        </li>
>>
>> +      <li>
>> +        An additional flow is added for traffic that goes in opposite
>> +        direction (i.e. it enters a network with configured SNAT). Where
>> the
>> +        flow above matched on <code>ip4.src == <var>A</var> &amp;&amp;
>> outport
>> +        == <var>GW</var></code>, this flow matches on <code> ip4.dst ==
>> +        <var>A</var> &amp;&amp; inport == <var>GW</var></code>. A CT
>> state is
>> +        initiated for this traffic so that the following table, <code>
>> +        lr_out_post_snat</code>, can identify whether the traffic flow
>> was
>> +        initiated from the internal or external network.
>> +
>> +      </li>
>> +
>>        <li>
>>          A priority-0 logical flow with match <code>1</code> has actions
>>          <code>next;</code>.
>> @@ -5072,6 +5091,16 @@ nd_ns {
>>      <p>
>>        Packets reaching this table are processed according to the flows
>> below:
>>        <ul>
>> +        <li>
>> +          Traffic that goes directly into a network configured with SNAT
>> on
>> +          Distributed routers, and was initiated from an external
>> network (i.e.
>> +          it matches <code>ct.new</code>), is committed to the SNAT CT
>> zone.
>> +          This ensures that replies returning from the SNATed network do
>> not
>> +          have their source address translated. For details about match
>> rules
>> +          and priority see section "Egress Table 3: SNAT on Distributed
>> +          Routers".
>> +        </li>
>> +
>>          <li>
>>            A priority-0 logical flow that matches all packets not already
>>            handled (match <code>1</code>) and action <code>next;</code>.
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 0732486f3..a7597a975 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -1155,6 +1155,7 @@ AT_CAPTURE_FILE([crflows])
>>  AT_CHECK([grep -e "lr_out_snat" drflows | ovn_strip_lflows], [0], [dnl
>>    table=??(lr_out_snat        ), priority=0    , match=(1),
>> action=(next;)
>>    table=??(lr_out_snat        ), priority=120  , match=(nd_ns),
>> action=(next;)
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
>> ip4.src == $allowed_range), action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
>> ip4.dst == $allowed_range && (!ct.trk || !ct.rpl)),
>> action=(ct_snat(172.16.1.1);)
>>  ])
>>
>> @@ -1185,6 +1186,7 @@ AT_CAPTURE_FILE([crflows2])
>>  AT_CHECK([grep -e "lr_out_snat" drflows2 | ovn_strip_lflows], [0], [dnl
>>    table=??(lr_out_snat        ), priority=0    , match=(1),
>> action=(next;)
>>    table=??(lr_out_snat        ), priority=120  , match=(nd_ns),
>> action=(next;)
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 50.0.0.11 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1")),
>> action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
>> (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.1);)
>>    table=??(lr_out_snat        ), priority=163  , match=(ip && ip4.src ==
>> 50.0.0.11 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
>> ip4.dst == $disallowed_range), action=(next;)
>>  ])
>> @@ -1279,7 +1281,7 @@ AT_CHECK([grep -e "lr_out_snat" crflows5 |
>> ovn_strip_lflows], [0], [dnl
>>    table=??(lr_out_snat        ), priority=33   , match=(ip && ip4.src ==
>> 50.0.0.11 && ip4.dst == $allowed_range), action=(ip4.src=172.16.1.2; next;)
>>  ])
>>
>> -# Stateful FIP with DISALLOWED_IPs
>> +# Stateless FIP with DISALLOWED_IPs
>>  ovn-nbctl lr-nat-del DR dnat_and_snat  172.16.1.2
>>  ovn-nbctl lr-nat-del CR dnat_and_snat  172.16.1.2
>>
>> @@ -5590,12 +5592,16 @@ 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; )
>>  ])
>>
>>  AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl
>>    table=??(lr_out_snat        ), priority=0    , match=(1),
>> action=(next;)
>>    table=??(lr_out_snat        ), priority=120  , match=(nd_ns),
>> action=(next;)
>> +  table=??(lr_out_snat        ), priority=153  , match=(ip && ip4.dst ==
>> 10.0.0.0/24 && inport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=153  , match=(ip && ip4.src ==
>> 10.0.0.0/24 && outport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
>> action=(ct_snat(172.168.0.10);)
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 10.0.0.10 && inport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 10.0.0.10 && outport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
>> action=(ct_snat(172.168.0.30);)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
>> && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
>>  ])
>> @@ -5738,12 +5744,16 @@ 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; )
>>  ])
>>
>>  AT_CHECK([grep "lr_out_snat" lr0flows | ovn_strip_lflows], [0], [dnl
>>    table=??(lr_out_snat        ), priority=0    , match=(1),
>> action=(next;)
>>    table=??(lr_out_snat        ), priority=120  , match=(nd_ns),
>> action=(next;)
>> +  table=??(lr_out_snat        ), priority=153  , match=(ip && ip4.dst ==
>> 10.0.0.0/24 && inport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=153  , match=(ip && ip4.src ==
>> 10.0.0.0/24 && outport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
>> action=(ct_snat(172.168.0.10);)
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 10.0.0.10 && inport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public")), action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 10.0.0.10 && outport == "lr0-public" &&
>> is_chassis_resident("cr-lr0-public") && (!ct.trk || !ct.rpl)),
>> action=(ct_snat(172.168.0.30);)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 10.0.0.3 && outport == "lr0-public" && is_chassis_resident("cr-lr0-public")
>> && (!ct.trk || !ct.rpl)), action=(ct_snat(172.168.0.20);)
>>  ])
>> @@ -7643,6 +7653,9 @@ AT_CHECK([grep lr_in_unsnat lrflows | grep ct_snat
>> | ovn_strip_lflows], [0], [dn
>>  ])
>>
>>  AT_CHECK([grep lr_out_snat lrflows | grep ct_snat | ovn_strip_lflows],
>> [0], [dnl
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 20.0.0.10 && inport == "DR-S1" && is_chassis_resident("cr-DR-S1")),
>> action=(ct_snat;)
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 20.0.0.10 && inport == "DR-S2" && is_chassis_resident("cr-DR-S2")),
>> action=(ct_snat;)
>> +  table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.dst ==
>> 20.0.0.10 && inport == "DR-S3" && is_chassis_resident("cr-DR-S3")),
>> action=(ct_snat;)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 20.0.0.10 && outport == "DR-S1" && is_chassis_resident("cr-DR-S1") &&
>> (!ct.trk || !ct.rpl)), action=(ct_snat(172.16.1.10);)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 20.0.0.10 && outport == "DR-S2" && is_chassis_resident("cr-DR-S2") &&
>> (!ct.trk || !ct.rpl)), action=(ct_snat(10.0.0.10);)
>>    table=??(lr_out_snat        ), priority=161  , match=(ip && ip4.src ==
>> 20.0.0.10 && outport == "DR-S3" && is_chassis_resident("cr-DR-S3") &&
>> (!ct.trk || !ct.rpl)), action=(ct_snat(192.168.0.10);)
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index c22c7882f..80e9b7d0b 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -3572,6 +3572,40 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2
>> 10.0.0.1 | FORMAT_PING], \
>>  3 packets transmitted, 3 received, 0% packet loss, time 0ms
>>  ])
>>
>> +
>> +# test_connectivity_from_ext takes parameters 'vm' and 'ip'. It tests
>> +# icmp, udp and tcp connectivity from external network to the 'vm' on
>> +# the specified 'ip'.
>> +test_connectivity_from_ext() {
>> +    local vm=$1; shift
>> +    local ip=$1; shift
>> +
>> +    # Start listening daemons for UDP and TCP connections
>> +    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
>> +    NETNS_DAEMONIZE($vm, [nc -l -k 1235], [nc-$vm-$ip-tcp.pid])
>> +
>> +    # Ensure that vm can be pinged on the specified IP
>> +    NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2 $ip |
>> FORMAT_PING], \
>> +    [0], [dnl
>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
>> +])
>> +
>> +    # Perform two consecutive UDP connections to the specified IP
>> +    NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
>> +    NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
>> +
>> +    # Send data over TCP connection to the specified IP
>> +    NS_CHECK_EXEC([alice1], [echo "TCP test" | nc --send-only $ip 1235])
>> +}
>> +
>> +# Test access from external network to the internal IP of a VM that
>> +# has also configured DNAT
>> +test_connectivity_from_ext foo1 192.168.1.2
>> +
>> +# Test access from external network to the internal IP of a VM that
>> +# does not have DNAT
>> +test_connectivity_from_ext bar1 192.168.2.2
>> +
>>  OVS_WAIT_UNTIL([
>>      total_pkts=$(cat ext-net.pcap | wc -l)
>>      test "${total_pkts}" = "3"
>> @@ -3738,6 +3772,39 @@ sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0],
>> [dnl
>>
>>  
>> icmpv6,orig=(src=fd12::2,dst=fd20::2,id=<cleared>,type=128,code=0),reply=(src=fd20::2,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
>>  ])
>>
>> +# test_connectivity_from_ext takes parameters 'vm' and 'ip'. It tests
>> +# icmp, udp and tcp connectivity from external network to the 'vm' on
>> +# the specified 'ip'.
>> +test_connectivity_from_ext() {
>> +    local vm=$1; shift
>> +    local ip=$1; shift
>> +
>> +    # Start listening daemons for UDP and TCP connections
>> +    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
>> +    NETNS_DAEMONIZE($vm, [nc -l -k 1235], [nc-$vm-$ip-tcp.pid])
>> +
>> +    # Ensure that vm can be pinged on the specified IP
>> +    NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2 $ip |
>> FORMAT_PING], \
>> +    [0], [dnl
>> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
>> +])
>> +
>> +    # Perform two consecutive UDP connections to the specified IP
>> +    NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
>> +    NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
>> +
>> +    # Send data over TCP connection to the specified IP
>> +    NS_CHECK_EXEC([alice1], [echo "TCP test" | nc --send-only $ip 1235])
>> +}
>> +
>> +# Test access from external network to the internal IP of a VM that
>> +# has also configured DNAT
>> +test_connectivity_from_ext foo1 fd11::2
>> +
>> +# Test access from external network to the internal IP of a VM that
>> +# does not have DNAT
>> +test_connectivity_from_ext bar1 fd12::2
>> +
>>  OVS_APP_EXIT_AND_WAIT([ovn-controller])
>>
>>  as ovn-sb
>> @@ -3918,6 +3985,7 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2
>> 172.16.1.4 | FORMAT_PING], \
>>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep icmp |
>> FORMAT_CT(172.16.1.1) | \
>>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>>
>>  
>> icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>>
>> +icmp,orig=(src=172.16.1.1,dst=192.168.2.2,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>>
>>  
>> icmp,orig=(src=192.168.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>>  ])
>>
>> @@ -4086,6 +4154,7 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2
>> fd20::4 | FORMAT_PING], \
>>  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(fd20::1) | \
>>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>>
>>  
>> icmpv6,orig=(src=fd11::3,dst=fd20::4,id=<cleared>,type=128,code=0),reply=(src=fd20::4,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
>>
>> +icmpv6,orig=(src=fd20::1,dst=fd12::2,id=<cleared>,type=128,code=0),reply=(src=fd12::2,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
>>
>>  
>> icmpv6,orig=(src=fd20::1,dst=fd20::4,id=<cleared>,type=128,code=0),reply=(src=fd12::2,dst=fd20::1,id=<cleared>,type=129,code=0),zone=<cleared>
>>  ])
>>
>> --
>> 2.40.1
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> One thing that slightly worries me is the potential impact on traffic
> performance. Basically anything going towards the SNAT would go through CT
> at least once.
>
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amu...@redhat.com
> <https://red.ht/sig>
>

Forgot to add the link.

[0]
https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/northd/northd.c#L14998-L15009

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA <https://www.redhat.com>

amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to