[ovs-dev] [PATCH ovn] ovn-trace: Set ct_state if not already set when tracing ct(nat).

Numan Siddique numans at ovn.org
Tue Nov 9 19:07:56 UTC 2021


On Mon, Nov 1, 2021 at 9:36 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> Since 0038579d1928 ("northd: Optimize ct nat for load balancer
> traffic.") calls to 'ct()' and 'ct(nat)' are merged because 'ct(nat)'
> implies 'ct()'.  However ovn-trace was not setting ct_state when
> processing 'ct(nat)'.  This was yielding unexpected results.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2017540
> Reported-by: Surya Seetharaman <surya at redhat.com>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>

Thanks for fixing this.

I applied this patch to the main branch and backported to branch-21.09
and branch-21.06.

Numan

> ---
>  tests/ovn-northd.at   | 39 +++++++++++++++++++++++++++++++++++++++
>  utilities/ovn-trace.c | 31 +++++++++++++++++++++----------
>  2 files changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index e2b9924b6..635c028d2 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -5466,3 +5466,42 @@ wait_row_count Datapath_Binding 1
>
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([conntrack nat implies conntrack])
> +ovn_start
> +
> +check ovn-nbctl lr-add rtr \
> +  -- set logical_router rtr options:chassis=hv \
> +  -- lrp-add rtr rtr-ls 00:00:00:00:01:00 42.42.42.1/24 \
> +  -- lb-add lb-test 43.43.43.43:4343 42.42.42.2:4242 tcp \
> +  -- lr-lb-add rtr lb-test
> +check ovn-nbctl --wait=sb sync
> +
> +flow="eth.dst == 00:00:00:00:01:00 && inport == \"rtr-ls\" && ip4.src == 42.42.42.42 && ip4.dst == 43.43.43.43 && ip.ttl == 64 && tcp && tcp.dst == 4343"
> +
> +AT_CHECK_UNQUOTED([ovn-trace --ct new --minimal "${flow}" --lb-dst 42.42.42.42:4242], [0], [dnl
> +# tcp,reg14=0x1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:01:00,nw_src=42.42.42.42,nw_dst=43.43.43.43,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=4343,tcp_flags=0
> +ct_dnat /* assuming no un-dnat entry, so no change */ {
> +    ct_lb {
> +        ip.ttl--;
> +        eth.src = 00:00:00:00:01:00;
> +        eth.dst = 00:00:00:00:00:00;
> +        arp {
> +            eth.dst = ff:ff:ff:ff:ff:ff;
> +            arp.spa = 0x2a2a2a01;
> +            arp.tpa = 0x2a2a2a02;
> +            arp.op = 1;
> +            output("rtr-ls");
> +        };
> +    };
> +};
> +])
> +
> +AT_CHECK_UNQUOTED([ovn-trace --minimal "${flow}" --lb-dst 42.42.42.42:4242], [0], [dnl
> +# tcp,reg14=0x1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=00:00:00:00:01:00,nw_src=42.42.42.42,nw_dst=43.43.43.43,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=4343,tcp_flags=0
> +ct_dnat /* assuming no un-dnat entry, so no change */ /* default (use --ct to customize) */;
> +])
> +
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 5d016b757..65a1822ea 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -199,6 +199,17 @@ parse_ct_option(const char *state_s_)
>      ct_states[n_ct_states++] = state;
>  }
>
> +static uint32_t
> +next_ct_state(struct ds *out_comment)
> +{
> +    if (ct_state_idx < n_ct_states) {
> +        return ct_states[ct_state_idx++];
> +    } else {
> +        ds_put_format(out_comment, " /* default (use --ct to customize) */");
> +        return CS_ESTABLISHED | CS_TRACKED;
> +    }
> +}
> +
>  static void
>  parse_lb_option(const char *s)
>  {
> @@ -2248,23 +2259,17 @@ execute_ct_next(const struct ovnact_ct_next *ct_next,
>                  enum ovnact_pipeline pipeline, struct ovs_list *super)
>  {
>      /* Figure out ct_state. */
> -    uint32_t state;
> -    const char *comment;
> -    if (ct_state_idx < n_ct_states) {
> -        state = ct_states[ct_state_idx++];
> -        comment = "";
> -    } else {
> -        state = CS_ESTABLISHED | CS_TRACKED;
> -        comment = " /* default (use --ct to customize) */";
> -    }
> +    struct ds comment = DS_EMPTY_INITIALIZER;
> +    uint32_t state = next_ct_state(&comment);
>
>      /* Make a sub-node for attaching the next table. */
>      struct ds s = DS_EMPTY_INITIALIZER;
>      format_flags(&s, ct_state_to_string, state, '|');
>      struct ovntrace_node *node = ovntrace_node_append(
>          super, OVNTRACE_NODE_TRANSFORMATION, "ct_next(ct_state=%s%s)",
> -        ds_cstr(&s), comment);
> +        ds_cstr(&s), ds_cstr(&comment));
>      ds_destroy(&s);
> +    ds_destroy(&comment);
>
>      /* Trace the actions in the next table. */
>      struct flow ct_flow = *uflow;
> @@ -2319,6 +2324,12 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
>          ds_put_format(&s, " /* assuming no un-%cnat entry, so no change */",
>                        direction[0]);
>      }
> +
> +    /* ct(nat) implies ct(). */
> +    if (!(ct_flow.ct_state & CS_TRACKED)) {
> +        ct_flow.ct_state |= next_ct_state(&s);
> +    }
> +
>      struct ovntrace_node *node = ovntrace_node_append(
>          super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(&s));
>      ds_destroy(&s);
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list