| Message ID | 20211201125608.36918-1-odivlad@gmail.com |
|---|---|
| Headers | show |
| Series | Support mixing stateless and stateful ACLs regardless of their priority | expand |
On 01.12.2021 15:56, Vladislav Odintsov wrote: > Currently if user has a stateless and statetul ACLs (allow-stateless and > allow-related) in one port group or in one logical switch simultaneously, > the stateless rules whould take precedence. > This patch series adds support for mixing all the ACLs types with the > respect to their priority. > This change requires next: > > Also, as an optimisation, traffic from HW VTEP switch in ingress datapath > is passed from ls_in_l2_sec directly to ls_in_l2_lkup, as it doesn't need > any processing in ingress pipeline except determining outport in > ls_in_l2_lkup table. > > Vladislav Odintsov (3): > Revert "northd: support HW VTEP with stateful datapath" > northd: send ingress packets from HW VTEP directly to L2_LKUP table > northd: support mix of stateless ACL with lower priority than stateful > > northd/northd.c | 113 ++++++++++++++++++++++------------------ > northd/ovn-northd.8.xml | 35 ++++--------- > northd/ovn_northd.dl | 47 +++++------------ > tests/ovn-northd.at | 50 ++++++++++-------- > 4 files changed, 114 insertions(+), 131 deletions(-) > Hi Numan, is is possible to plan this series to be included in 21.12?
On Tue, Dec 7, 2021 at 3:58 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > > On 01.12.2021 15:56, Vladislav Odintsov wrote: > > Currently if user has a stateless and statetul ACLs (allow-stateless and > > allow-related) in one port group or in one logical switch simultaneously, > > the stateless rules whould take precedence. > > This patch series adds support for mixing all the ACLs types with the > > respect to their priority. > > This change requires next: > > > > Also, as an optimisation, traffic from HW VTEP switch in ingress datapath > > is passed from ls_in_l2_sec directly to ls_in_l2_lkup, as it doesn't need > > any processing in ingress pipeline except determining outport in > > ls_in_l2_lkup table. > > > > Vladislav Odintsov (3): > > Revert "northd: support HW VTEP with stateful datapath" > > northd: send ingress packets from HW VTEP directly to L2_LKUP table > > northd: support mix of stateless ACL with lower priority than stateful > > > > northd/northd.c | 113 ++++++++++++++++++++++------------------ > > northd/ovn-northd.8.xml | 35 ++++--------- > > northd/ovn_northd.dl | 47 +++++------------ > > tests/ovn-northd.at | 50 ++++++++++-------- > > 4 files changed, 114 insertions(+), 131 deletions(-) > > > Hi Numan, > > is is possible to plan this series to be included in 21.12? Hi Vladislav, I was thinking of considering them after branching. Do you need these patches for 21.12 ? I'm trying to understand the risk factor ? Are these patches risky at this time or will not affect other users who don't use this scenario ? If it is risk free, +1 from me for 21.12. Thanks Numan > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Talking about patches 1 and 2 - they've got totally no negative impact, it's an optimization for HW VTEP scenario - I'd like them to be included as a part of 21.12. For patch #3 there is absolutely no affect for users who use either only stateless ACLs or only stateful. For users, who do mix of allow-stateless and allow-related rules it's a _possible_ affect, only if priority for allow-related rules is higher, than for allow-stateless AND these rules have overlapping meaning. It's worth to mention, that if somebody has such rules installed now, they don't work as the could be treated. Let me know your thoughts. regards, Vladislav Odintsov On 07.12.2021 19:14, Numan Siddique wrote: > On Tue, Dec 7, 2021 at 3:58 AM Vladislav Odintsov <odivlad@gmail.com> wrote: >> On 01.12.2021 15:56, Vladislav Odintsov wrote: >>> Currently if user has a stateless and statetul ACLs (allow-stateless and >>> allow-related) in one port group or in one logical switch simultaneously, >>> the stateless rules whould take precedence. >>> This patch series adds support for mixing all the ACLs types with the >>> respect to their priority. >>> This change requires next: >>> >>> Also, as an optimisation, traffic from HW VTEP switch in ingress datapath >>> is passed from ls_in_l2_sec directly to ls_in_l2_lkup, as it doesn't need >>> any processing in ingress pipeline except determining outport in >>> ls_in_l2_lkup table. >>> >>> Vladislav Odintsov (3): >>> Revert "northd: support HW VTEP with stateful datapath" >>> northd: send ingress packets from HW VTEP directly to L2_LKUP table >>> northd: support mix of stateless ACL with lower priority than stateful >>> >>> northd/northd.c | 113 ++++++++++++++++++++++------------------ >>> northd/ovn-northd.8.xml | 35 ++++--------- >>> northd/ovn_northd.dl | 47 +++++------------ >>> tests/ovn-northd.at | 50 ++++++++++-------- >>> 4 files changed, 114 insertions(+), 131 deletions(-) >>> >> Hi Numan, >> >> is is possible to plan this series to be included in 21.12? > Hi Vladislav, > > I was thinking of considering them after branching. Do you need these > patches for 21.12 ? > I'm trying to understand the risk factor ? Are these patches risky at > this time or will not affect other users who don't use this scenario ? > > If it is risk free, +1 from me for 21.12. > > Thanks > Numan > >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Tue, Dec 7, 2021 at 11:43 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > > Talking about patches 1 and 2 - they've got totally no negative impact, > it's an optimization for HW VTEP scenario - I'd like them to be included > as a part of 21.12. > > For patch #3 there is absolutely no affect for users who use either only > stateless ACLs or only stateful. > > For users, who do mix of allow-stateless and allow-related rules it's a > _possible_ affect, only if priority for allow-related rules is higher, > than for allow-stateless AND these rules have overlapping meaning. It's > worth to mention, that if somebody has such rules installed now, they > don't work as the could be treated. > > Let me know your thoughts. Ok. I applied the first 2 patches to the main branch with the below changes in patch 2. ddlog compilation was broken. --- diff --git a/northd/northd.c b/northd/northd.c index 2efc4bb1f7..5db6ff03db 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5483,7 +5483,7 @@ build_lswitch_input_port_sec_op( if (!strcmp(op->nbsp->type, "vtep")) { ds_put_format(actions, "next(pipeline=ingress, table=%d);", - S_SWITCH_IN_L2_LKUP); + ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); } else { ds_put_cstr(actions, "next;"); } diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index 530bb1e9d9..2fe73959c6 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -3469,19 +3469,18 @@ for (&SwitchPort(.lsp = lsp, .sw = sw, .json_name = json_name, .ps_eth_addresses i"inport == ${json_name} && eth.src == {${ps_eth_addresses.join(\" \")}}" } in - var actions = { - var queue = match (pbinding.options.get(i"qdisc_queue_id")) { - None -> i"next;", - Some{id} -> i"set_queue(${id}); " - }; - var ramp = if (lsp.__type == i"vtep") { - i"next(pipeline=ingress, table=${s_SWITCH_IN_L2_LKUP()});" - } else { - i"next;" - } in - }; - i"${queue}${ramp}" - } in + var actions = { + var queue = match (pbinding.options.get(i"qdisc_queue_id")) { + None -> i"", + Some{id} -> i"set_queue(${id});" + }; + var ramp = if (lsp.__type == i"vtep") { + i"next(pipeline=ingress, table=${s_SWITCH_IN_L2_LKUP().table_id});" + } else { + i"next;" + }; + i"${queue}${ramp}" + } in Flow(.logical_datapath = sw._uuid, .stage = s_SWITCH_IN_PORT_SEC_L2(), .priority = 50, ------------- I still need to take a look at patch 3. Thanks Numan > > regards, > > Vladislav Odintsov > > On 07.12.2021 19:14, Numan Siddique wrote: > > On Tue, Dec 7, 2021 at 3:58 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > >> On 01.12.2021 15:56, Vladislav Odintsov wrote: > >>> Currently if user has a stateless and statetul ACLs (allow-stateless and > >>> allow-related) in one port group or in one logical switch simultaneously, > >>> the stateless rules whould take precedence. > >>> This patch series adds support for mixing all the ACLs types with the > >>> respect to their priority. > >>> This change requires next: > >>> > >>> Also, as an optimisation, traffic from HW VTEP switch in ingress datapath > >>> is passed from ls_in_l2_sec directly to ls_in_l2_lkup, as it doesn't need > >>> any processing in ingress pipeline except determining outport in > >>> ls_in_l2_lkup table. > >>> > >>> Vladislav Odintsov (3): > >>> Revert "northd: support HW VTEP with stateful datapath" > >>> northd: send ingress packets from HW VTEP directly to L2_LKUP table > >>> northd: support mix of stateless ACL with lower priority than stateful > >>> > >>> northd/northd.c | 113 ++++++++++++++++++++++------------------ > >>> northd/ovn-northd.8.xml | 35 ++++--------- > >>> northd/ovn_northd.dl | 47 +++++------------ > >>> tests/ovn-northd.at | 50 ++++++++++-------- > >>> 4 files changed, 114 insertions(+), 131 deletions(-) > >>> > >> Hi Numan, > >> > >> is is possible to plan this series to be included in 21.12? > > Hi Vladislav, > > > > I was thinking of considering them after branching. Do you need these > > patches for 21.12 ? > > I'm trying to understand the risk factor ? Are these patches risky at > > this time or will not affect other users who don't use this scenario ? > > > > If it is risk free, +1 from me for 21.12. > > > > Thanks > > Numan > > > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks Numan, I’ll wait for your comments on #3. Regards, Vladislav Odintsov > On 8 Dec 2021, at 19:13, Numan Siddique <numans@ovn.org> wrote: > > On Tue, Dec 7, 2021 at 11:43 AM Vladislav Odintsov <odivlad@gmail.com> wrote: >> >> Talking about patches 1 and 2 - they've got totally no negative impact, >> it's an optimization for HW VTEP scenario - I'd like them to be included >> as a part of 21.12. >> >> For patch #3 there is absolutely no affect for users who use either only >> stateless ACLs or only stateful. >> >> For users, who do mix of allow-stateless and allow-related rules it's a >> _possible_ affect, only if priority for allow-related rules is higher, >> than for allow-stateless AND these rules have overlapping meaning. It's >> worth to mention, that if somebody has such rules installed now, they >> don't work as the could be treated. >> >> Let me know your thoughts. > > Ok. > > I applied the first 2 patches to the main branch with the below > changes in patch 2. > ddlog compilation was broken. > --- > > diff --git a/northd/northd.c b/northd/northd.c > index 2efc4bb1f7..5db6ff03db 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5483,7 +5483,7 @@ build_lswitch_input_port_sec_op( > > if (!strcmp(op->nbsp->type, "vtep")) { > ds_put_format(actions, "next(pipeline=ingress, table=%d);", > - S_SWITCH_IN_L2_LKUP); > + ovn_stage_get_table(S_SWITCH_IN_L2_LKUP)); > } else { > ds_put_cstr(actions, "next;"); > } > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index 530bb1e9d9..2fe73959c6 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -3469,19 +3469,18 @@ for (&SwitchPort(.lsp = lsp, .sw = sw, > .json_name = json_name, .ps_eth_addresses > i"inport == ${json_name} && eth.src == > {${ps_eth_addresses.join(\" \")}}" > } in > > - var actions = { > - var queue = match (pbinding.options.get(i"qdisc_queue_id")) { > - None -> i"next;", > - Some{id} -> i"set_queue(${id}); " > - }; > - var ramp = if (lsp.__type == i"vtep") { > - i"next(pipeline=ingress, table=${s_SWITCH_IN_L2_LKUP()});" > - } else { > - i"next;" > - } in > - }; > - i"${queue}${ramp}" > - } in > + var actions = { > + var queue = match (pbinding.options.get(i"qdisc_queue_id")) { > + None -> i"", > + Some{id} -> i"set_queue(${id});" > + }; > + var ramp = if (lsp.__type == i"vtep") { > + i"next(pipeline=ingress, > table=${s_SWITCH_IN_L2_LKUP().table_id});" > + } else { > + i"next;" > + }; > + i"${queue}${ramp}" > + } in > Flow(.logical_datapath = sw._uuid, > .stage = s_SWITCH_IN_PORT_SEC_L2(), > .priority = 50, > > > ------------- > I still need to take a look at patch 3. > > Thanks > Numan > >> >> regards, >> >> Vladislav Odintsov >> >> On 07.12.2021 19:14, Numan Siddique wrote: >>> On Tue, Dec 7, 2021 at 3:58 AM Vladislav Odintsov <odivlad@gmail.com> wrote: >>>> On 01.12.2021 15:56, Vladislav Odintsov wrote: >>>>> Currently if user has a stateless and statetul ACLs (allow-stateless and >>>>> allow-related) in one port group or in one logical switch simultaneously, >>>>> the stateless rules whould take precedence. >>>>> This patch series adds support for mixing all the ACLs types with the >>>>> respect to their priority. >>>>> This change requires next: >>>>> >>>>> Also, as an optimisation, traffic from HW VTEP switch in ingress datapath >>>>> is passed from ls_in_l2_sec directly to ls_in_l2_lkup, as it doesn't need >>>>> any processing in ingress pipeline except determining outport in >>>>> ls_in_l2_lkup table. >>>>> >>>>> Vladislav Odintsov (3): >>>>> Revert "northd: support HW VTEP with stateful datapath" >>>>> northd: send ingress packets from HW VTEP directly to L2_LKUP table >>>>> northd: support mix of stateless ACL with lower priority than stateful >>>>> >>>>> northd/northd.c | 113 ++++++++++++++++++++++------------------ >>>>> northd/ovn-northd.8.xml | 35 ++++--------- >>>>> northd/ovn_northd.dl | 47 +++++------------ >>>>> tests/ovn-northd.at | 50 ++++++++++-------- >>>>> 4 files changed, 114 insertions(+), 131 deletions(-) >>>>> >>>> Hi Numan, >>>> >>>> is is possible to plan this series to be included in 21.12? >>> Hi Vladislav, >>> >>> I was thinking of considering them after branching. Do you need these >>> patches for 21.12 ? >>> I'm trying to understand the risk factor ? Are these patches risky at >>> this time or will not affect other users who don't use this scenario ? >>> >>> If it is risk free, +1 from me for 21.12. >>> >>> Thanks >>> Numan >>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>