Message ID | 20211101133508.17735-1-dceara@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev] ovn-trace: Set ct_state if not already set when tracing ct(nat). | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Mon, Nov 1, 2021 at 9:36 AM Dumitru Ceara <dceara@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@redhat.com> > Signed-off-by: Dumitru Ceara <dceara@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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);
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@redhat.com> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- tests/ovn-northd.at | 39 +++++++++++++++++++++++++++++++++++++++ utilities/ovn-trace.c | 31 +++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 10 deletions(-)