[ovs-dev] [PATCH ovn 1/5] northd: Use 'enum ovn_stage' for the table value in the 'next' OVN action.
Dumitru Ceara
dceara at redhat.com
Mon Oct 12 19:07:55 UTC 2020
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
More information about the dev
mailing list