[ovs-dev] [PATCH ovn 1/5] northd: Use 'enum ovn_stage' for the table value in the 'next' OVN action.
Numan Siddique
numans at ovn.org
Tue Oct 13 07:12:26 UTC 2020
On Tue, Oct 13, 2020 at 12:38 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 10/5/20 7:49 PM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > Multiple places in ovn-northd.c hard codes the table value in the next() OVN action.
> > This patch changes those occurrences to use ovn_stage_get_table('enum ovn_stage' value).
> >
> > Hard coding of the table number can result in errors if new stages are added (like
> > the patch [1] which added new stages - ls_in_acl_hint and ls_out_acl_hint). After the patch [1],
> > the table number was wrong for reject ACLs associated in ingress logical switch pipeline stage.
> > Although this didn't result in any packet drops. This patch avoids such cases in the future.
> >
> > This patch also adds a new test case in ovn-northd.at for reject ACL flows.
> >
> > [1] - 209ea46bbf9d("ovn-northd: Reduce number of flows generated for stateful ACLs.")
> >
> > Signed-off-by: Numan Siddique <numans at ovn.org>
>
> Hi Numan,
>
> This change looks OK to me, just a few minor comments inline.
>
> > ---
> > northd/ovn-northd.c | 36 ++++---
> > tests/ovn-northd.at | 247 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 266 insertions(+), 17 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 91da319415..d5fd7da03a 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -5411,6 +5411,12 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
> > struct ds actions = DS_EMPTY_INITIALIZER;
> > bool ingress = (stage == S_SWITCH_IN_ACL);
> >
> > + char *next_action =
> > + xasprintf("next(pipeline=%s,table=%d);",
> > + ingress ? "egress": "ingress",
> > + ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
> > + : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP));
> > +
> > /* TCP */
> > build_acl_log(&actions, acl);
> > if (extra_match->length > 0) {
> > @@ -5419,9 +5425,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
> > ds_put_format(&match, "ip4 && tcp && (%s)", acl->match);
> > ds_put_format(&actions, "reg0 = 0; "
> > "eth.dst <-> eth.src; ip4.dst <-> ip4.src; "
> > - "tcp_reset { outport <-> inport; %s };",
> > - ingress ? "next(pipeline=egress,table=5);"
> > - : "next(pipeline=ingress,table=20);");
> > + "tcp_reset { outport <-> inport; %s };", next_action);
> > ovn_lflow_add_with_hint(lflows, od, stage,
> > acl->priority + OVN_ACL_PRI_OFFSET + 10,
> > ds_cstr(&match), ds_cstr(&actions), stage_hint);
> > @@ -5434,9 +5438,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
> > ds_put_format(&match, "ip6 && tcp && (%s)", acl->match);
> > ds_put_format(&actions, "reg0 = 0; "
> > "eth.dst <-> eth.src; ip6.dst <-> ip6.src; "
> > - "tcp_reset { outport <-> inport; %s };",
> > - ingress ? "next(pipeline=egress,table=5);"
> > - : "next(pipeline=ingress,table=20);");
> > + "tcp_reset { outport <-> inport; %s };", next_action);
> > ovn_lflow_add_with_hint(lflows, od, stage,
> > acl->priority + OVN_ACL_PRI_OFFSET + 10,
> > ds_cstr(&match), ds_cstr(&actions), stage_hint);
> > @@ -5454,9 +5456,7 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
> > }
> > ds_put_format(&actions, "reg0 = 0; "
> > "icmp4 { eth.dst <-> eth.src; ip4.dst <-> ip4.src; "
> > - "outport <-> inport; %s };",
> > - ingress ? "next(pipeline=egress,table=5);"
> > - : "next(pipeline=ingress,table=20);");
> > + "outport <-> inport; %s };", next_action);
> > ovn_lflow_add_with_hint(lflows, od, stage,
> > acl->priority + OVN_ACL_PRI_OFFSET,
> > ds_cstr(&match), ds_cstr(&actions), stage_hint);
> > @@ -5472,13 +5472,12 @@ build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
> > }
> > ds_put_format(&actions, "reg0 = 0; icmp6 { "
> > "eth.dst <-> eth.src; ip6.dst <-> ip6.src; "
> > - "outport <-> inport; %s };",
> > - ingress ? "next(pipeline=egress,table=5);"
> > - : "next(pipeline=ingress,table=20);");
> > + "outport <-> inport; %s };", next_action);
> > ovn_lflow_add_with_hint(lflows, od, stage,
> > acl->priority + OVN_ACL_PRI_OFFSET,
> > ds_cstr(&match), ds_cstr(&actions), stage_hint);
> >
> > + free(next_action);
> > ds_destroy(&match);
> > ds_destroy(&actions);
> > }
> > @@ -9820,7 +9819,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> > ds_put_format(&actions, "reg%d = 0; ", j);
> > }
> > ds_put_format(&actions, REGBIT_EGRESS_LOOPBACK" = 1; "
> > - "next(pipeline=ingress, table=0); };");
> > + "next(pipeline=ingress, table=%d); };",
> > + ovn_stage_get_table(S_SWITCH_IN_PORT_SEC_L2));
>
> Technically it works but this should be S_ROUTER_IN_ADMISSION. We're in
> the router pipeline.
>
> > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_EGR_LOOP, 100,
> > ds_cstr(&match), ds_cstr(&actions),
> > &nat->header_);
> > @@ -11006,10 +11006,11 @@ build_check_pkt_len_flows_for_lrouter(
> > "icmp4.type = 3; /* Destination Unreachable. */ "
> > "icmp4.code = 4; /* Frag Needed and DF was Set. */ "
> > "icmp4.frag_mtu = %d; "
> > - "next(pipeline=ingress, table=0); };",
> > + "next(pipeline=ingress, table=%d); };",
> > rp->lrp_networks.ea_s,
> > rp->lrp_networks.ipv4_addrs[0].addr_s,
> > - gw_mtu);
> > + gw_mtu,
> > + ovn_stage_get_table(S_SWITCH_IN_PORT_SEC_L2));
>
> Same here.
>
> > ovn_lflow_add_with_hint(lflows, od,
> > S_ROUTER_IN_LARGER_PKTS, 50,
> > ds_cstr(match), ds_cstr(actions),
> > @@ -11034,10 +11035,11 @@ build_check_pkt_len_flows_for_lrouter(
> > "icmp6.type = 2; /* Packet Too Big. */ "
> > "icmp6.code = 0; "
> > "icmp6.frag_mtu = %d; "
> > - "next(pipeline=ingress, table=0); };",
> > + "next(pipeline=ingress, table=%d); };",
> > rp->lrp_networks.ea_s,
> > rp->lrp_networks.ipv6_addrs[0].addr_s,
> > - gw_mtu);
> > + gw_mtu,
> > + ovn_stage_get_table(S_SWITCH_IN_PORT_SEC_L2));
>
> Here too.
>
> With these addressed:
> Acked-by: Dumitru Ceara <dceara at redhat.com>
>
Thanks Dumitru. Nice catch. Oops from my side.
I addressed your comments and applied this and the 2nd patch of this
series to master and branch-20.09.
Thanks
Numan
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list