Skip to content

Commit

Permalink
ovn-trace: Set ct_state if not already set when tracing ct(nat).
Browse files Browse the repository at this point in the history
Since 0038579 ("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@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from commit c117ce1)
  • Loading branch information
dceara authored and numansiddique committed Nov 9, 2021
1 parent a43aa02 commit 18f7199
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 10 deletions.
39 changes: 39 additions & 0 deletions tests/ovn-northd.at
Expand Up @@ -3732,3 +3732,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
])
31 changes: 21 additions & 10 deletions utilities/ovn-trace.c
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2309,6 +2314,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);
Expand Down

0 comments on commit 18f7199

Please sign in to comment.