Thanks Dumitru. Regards, Vladislav Odintsov
> On 10 Dec 2021, at 14:13, Dumitru Ceara <[email protected]> wrote: > > On 12/9/21 17:12, Vladislav Odintsov wrote: >> Hi Dumitru, >> > > Hi Vladislav, > >> thanks for review & testing and pointing for the broken behaviour. >> I’ve missed such case, and I think this usecase should be of course >> supported. Maybe, it should be listed in a ovn testcases for not loose it >> one day? > > Well, the existing test case, "ACL allow-stateless overrides stateful > rules with higher priority - Logical_Switch", does that already to some > extent. > > It's just that your patch changes exactly that test to match the new > behavior you were trying to implement. :) Huh, that’s true. :) > > If I revert the test change then the test will fail with your patch > applied due to: > > -output("lsp2"); > +ct_next(ct_state=est|trk /* default (use --ct to customize) */) { > + ct_next(ct_state=est|trk /* default (use --ct to customize) */) { > + output("lsp2"); > + }; > +}; > > That's because the lower priority stateless ACL is not matched anymore > and we go to conntrack. > > Maybe we should just add a better comment and description to the > existing test case. > >> That’s very interesting situation as for me, I have to think how we can >> solve it. > > I'm not sure we can and I'm not sure we really need to. Maybe we should > just improve the documentation [0] and explain that it's relevant for > the case when allow-related and allow-stateless ACLs have overlapping > matches. > > [0] https://github.com/ovn-org/ovn/blob/main/ovn-nb.xml#L1934 > >> >> Anyway, I’ve managed to resolve my personal task, which initially was the >> reason for this patch, and if it's not needed for the community, I’m okay to >> close it for now. >> If somebody is interested in it, please let me know. >> >> What do you think? > > +1 for leaving it as it is now. Deal. > >> >> Regards, >> Vladislav Odintsov > > Regards, > Dumitru > >> >>> On 9 Dec 2021, at 13:45, Dumitru Ceara <[email protected]> wrote: >>> >>> On 12/1/21 13:56, Vladislav Odintsov wrote: >>>> It was unsupported - to have a mix of stateless ACLs with lower than >>>> stateful priority at the same time with stateful (related) ACLs. >>>> >>>> This patch changes stateless ACLs behaviour, but this change should not >>>> be harmful for users. Likely nobody mixed rules in the described manner, >>>> so this change should be okay. >>>> >>>> Signed-off-by: Vladislav Odintsov <[email protected] >>>> <mailto:[email protected]>> >>>> --- >>> >>> Hi Vladislav, >>> >>> We also need to update the documentation here: >>> https://github.com/ovn-org/ovn/blob/a2e7f69d6684766dadfb0a1eb455e123f6bd200a/ovn-nb.xml#L1934 >>> >>> <https://github.com/ovn-org/ovn/blob/a2e7f69d6684766dadfb0a1eb455e123f6bd200a/ovn-nb.xml#L1934> >>> >>> Should this be a "Fixes: 3187b9fef1 ("ovn-northd: introduce new >>> allow-stateless ACL verb")"? CC-ing Ihar and Han for more input on this >>> matter. >>> >>> I guess this should give more context on the original implementation: >>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383513.html >>> <https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383513.html> >>> >>> However, I think there are some issues with your approach; using the >>> example Han gave there (adding it here for clarity): >>> >>> All traffic from A to B (egress) is stateless: >>> ACL1: from-lport 100 'inport == "A" && ip.dst == B' allow-stateless >>> >>> So does the return traffic: >>> ACL2: to-lport 100 'outport == "A" && ip.src == B' allow-stateless >>> >>> Now we need the traffic initiated from A to B's TCP port 80 to be >>> stateful: >>> ACL3: from-lport 200 'inport == "A" && ip.dst == B && tcp.dst == 80' >>> allow-related >>> >>> My topology consists of one logical switch and two ports: >>> >>> ovn-nbctl ls-add ls >>> ovn-nbctl lsp-add ls vm1 >>> ovn-nbctl lsp-set-addresses vm1 00:00:00:00:00:01 >>> >>> ip netns add vm1 >>> ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal >>> ip link set vm1 netns vm1 >>> ip netns exec vm1 ip link set vm1 address 00:00:00:00:00:01 >>> ip netns exec vm1 ip addr add 42.42.42.2/24 dev vm1 >>> ip netns exec vm1 ip link set vm1 up >>> ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 >>> >>> ovn-nbctl lsp-add ls vm2 >>> ovn-nbctl lsp-set-addresses vm2 00:00:00:00:00:02 >>> >>> ip netns add vm2 >>> ovs-vsctl add-port br-int vm2 -- set interface vm2 type=internal >>> ip link set vm2 netns vm2 >>> ip netns exec vm2 ip link set vm2 address 00:00:00:00:00:02 >>> ip netns exec vm2 ip addr add 42.42.42.3/24 dev vm2 >>> ip netns exec vm2 ip link set vm2 up >>> ovs-vsctl set Interface vm2 external_ids:iface-id=vm2 >>> >>> ovn-nbctl acl-add ls from-lport 100 'inport == "vm1" && ip4.dst == >>> 42.42.42.3' allow-stateless >>> ovn-nbctl acl-add ls to-lport 100 'outport == "vm1" && ip4.src == >>> 42.42.42.3' allow-stateless >>> ovn-nbctl acl-add ls from-lport 200 'inport == "vm1" && ip4.dst == >>> 42.42.42.3 && tcp.dst == 80' allow-related >>> >>> I'd expect TCP sessions to always be established with this config and >>> that is the case without your patch, sessions can be established fine >>> between A:X -> B:80: >>> tcp 6 431998 ESTABLISHED src=42.42.42.2 dst=42.42.42.3 sport=52994 >>> dport=80 src=42.42.42.3 dst=42.42.42.2 sport=80 dport=52994 [ASSURED] >>> mark=0 secctx=system_u:object_r:unlabeled_t:s0 zone=1 use=1 >>> >>> While, when applying your patch, the connection is not fully established: >>> tcp 6 19 SYN_RECV src=42.42.42.2 dst=42.42.42.3 sport=60730 dport=8080 >>> src=42.42.42.3 dst=42.42.42.2 sport=8080 dport=60730 mark=0 >>> secctx=system_u:object_r:unlabeled_t:s0 zone=1 use=1 >>> tcp 6 79 SYN_SENT src=42.42.42.2 dst=42.42.42.3 sport=60730 dport=8080 >>> [UNREPLIED] src=42.42.42.3 dst=42.42.42.2 sport=8080 dport=60730 mark=0 >>> secctx=system_u:object_r:unlabeled_t:s0 zone=3 use=1 >>> >>> Zone 1 is vm1's conntrack zone and zone 3 corresponds to vm2. >>> >>>> northd/northd.c | 92 +++++++++++++++++++++++++---------------- >>>> northd/ovn-northd.8.xml | 4 +- >>>> tests/ovn-northd.at | 48 ++++++++++++--------- >>>> 3 files changed, 87 insertions(+), 57 deletions(-) >>>> >>>> diff --git a/northd/northd.c b/northd/northd.c >>>> index 2efc4bb1f..1831e6e79 100644 >>>> --- a/northd/northd.c >>>> +++ b/northd/northd.c >>>> @@ -592,6 +592,7 @@ struct ovn_datapath { >>>> uint32_t port_key_hint; >>>> >>>> bool has_stateful_acl; >>>> + bool has_stateless_acl; >>>> bool has_lb_vip; >>>> bool has_unknown; >>>> bool has_acls; >>>> @@ -5416,15 +5417,26 @@ ls_get_acl_flags(struct ovn_datapath *od) >>>> { >>>> od->has_acls = false; >>>> od->has_stateful_acl = false; >>>> + od->has_stateless_acl = false; >>>> >>>> if (od->nbs->n_acls) { >>>> od->has_acls = true; >>>> >>>> for (size_t i = 0; i < od->nbs->n_acls; i++) { >>>> struct nbrec_acl *acl = od->nbs->acls[i]; >>>> - if (!strcmp(acl->action, "allow-related")) { >>>> + if (!od->has_stateful_acl && >>>> + !strcmp(acl->action, "allow-related")) { >>>> od->has_stateful_acl = true; >>>> - return; >>>> + if (od->has_stateless_acl) { >>>> + return; >>>> + } >>>> + } >>>> + if (!od->has_stateless_acl && >>>> + !strcmp(acl->action, "allow-stateless")) { >>>> + od->has_stateless_acl = true; >>>> + if (od->has_stateful_acl) { >>>> + return; >>>> + } >>>> } >>> >>> I think it might be easier to read the code if we remove the "return" >>> statements above and just add a check here: >>> >>> if (od->has_stateless_acl && od->has_stateful_acl) { >>> return; >>> } >>> >>>> } >>>> } >>>> @@ -5436,9 +5448,19 @@ ls_get_acl_flags(struct ovn_datapath *od) >>>> >>>> for (size_t i = 0; i < ls_pg->nb_pg->n_acls; i++) { >>>> struct nbrec_acl *acl = ls_pg->nb_pg->acls[i]; >>>> - if (!strcmp(acl->action, "allow-related")) { >>>> + if (!od->has_stateful_acl && >>>> + !strcmp(acl->action, "allow-related")) { >>>> od->has_stateful_acl = true; >>>> - return; >>>> + if (od->has_stateless_acl) { >>>> + return; >>>> + } >>>> + } >>>> + if (!od->has_stateless_acl && >>>> + !strcmp(acl->action, "allow-stateless")) { >>>> + od->has_stateless_acl = true; >>>> + if (od->has_stateful_acl) { >>>> + return; >>>> + } >>>> } >>> >>> Same here. >>> >>> Regards, >>> Dumitru >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] <mailto:[email protected]> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > > _______________________________________________ > 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
