On Tue, Oct 13, 2020 at 12:38 AM Dumitru Ceara <[email protected]> wrote: > > On 10/5/20 7:49 PM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 <[email protected]> > > 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 <[email protected]> >
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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
