| Message ID | 20211201125608.36918-4-odivlad@gmail.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | Support mixing stateless and stateful ACLs regardless of their priority | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
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 <odivlad@gmail.com> > --- Hi Vladislav, We also need to update the documentation here: 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 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
Hi Dumitru, 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? That’s very interesting situation as for me, I have to think how we can solve it. 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? Regards, Vladislav Odintsov > On 9 Dec 2021, at 13:45, Dumitru Ceara <dceara@redhat.com> 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 <odivlad@gmail.com <mailto:odivlad@gmail.com>> >> --- > > 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 > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
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. :) 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. > > Regards, > Vladislav Odintsov Regards, Dumitru > >> On 9 Dec 2021, at 13:45, Dumitru Ceara <dceara@redhat.com> 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 <odivlad@gmail.com <mailto:odivlad@gmail.com>> >>> --- >> >> 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 >> dev@openvswitch.org <mailto:dev@openvswitch.org> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >
Thanks Dumitru. Regards, Vladislav Odintsov > On 10 Dec 2021, at 14:13, Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> 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 <odivlad@gmail.com <mailto:odivlad@gmail.com>> >>>> --- >>> >>> 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 >>> dev@openvswitch.org <mailto:dev@openvswitch.org> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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; + } } } } @@ -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; + } } } } @@ -5647,45 +5669,42 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op, } static void -build_stateless_filter(struct ovn_datapath *od, - const struct nbrec_acl *acl, - struct hmap *lflows) +build_acl_filter(struct ovn_datapath *od, const struct nbrec_acl *acl, + struct hmap *lflows) { + int direction; + char *actions; + if (!strcmp(acl->direction, "from-lport")) { - ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL, - acl->priority + OVN_ACL_PRI_OFFSET, - acl->match, - "next;", - &acl->header_); + direction = S_SWITCH_IN_PRE_ACL; } else { - ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL, - acl->priority + OVN_ACL_PRI_OFFSET, - acl->match, - "next;", - &acl->header_); + direction = S_SWITCH_OUT_PRE_ACL; + } + + if (!strcmp(acl->action, "allow-stateless")) { + actions = "next;"; + } else { + actions = REGBIT_CONNTRACK_DEFRAG" = 1; next;"; } + + ovn_lflow_add_with_hint(lflows, od, direction, + acl->priority + OVN_ACL_PRI_OFFSET, acl->match, + actions, &acl->header_); } static void -build_stateless_filters(struct ovn_datapath *od, - const struct hmap *port_groups, - struct hmap *lflows) +build_acl_filters(struct ovn_datapath *od, const struct hmap *port_groups, + struct hmap *lflows) { for (size_t i = 0; i < od->nbs->n_acls; i++) { - const struct nbrec_acl *acl = od->nbs->acls[i]; - if (!strcmp(acl->action, "allow-stateless")) { - build_stateless_filter(od, acl, lflows); - } + build_acl_filter(od, od->nbs->acls[i], lflows); } struct ovn_port_group *pg; HMAP_FOR_EACH (pg, key_node, port_groups) { if (ovn_port_group_ls_find(pg, &od->nbs->header_.uuid)) { for (size_t i = 0; i < pg->nb_pg->n_acls; i++) { - const struct nbrec_acl *acl = pg->nb_pg->acls[i]; - if (!strcmp(acl->action, "allow-stateless")) { - build_stateless_filter(od, acl, lflows); - } + build_acl_filter(od, pg->nb_pg->acls[i], lflows); } } } @@ -5700,10 +5719,10 @@ build_pre_acls(struct ovn_datapath *od, const struct hmap *port_groups, ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 0, "1", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 0, "1", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, + ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 65535, "eth.dst == $svc_monitor_mac", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, + ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 65535, "eth.src == $svc_monitor_mac", "next;"); /* If there are any stateful ACL rules in this datapath, we may @@ -5713,25 +5732,26 @@ build_pre_acls(struct ovn_datapath *od, const struct hmap *port_groups, for (size_t i = 0; i < od->n_router_ports; i++) { skip_port_from_conntrack(od, od->router_ports[i], S_SWITCH_IN_PRE_ACL, S_SWITCH_OUT_PRE_ACL, - 110, lflows); + 65535, lflows); } for (size_t i = 0; i < od->n_localnet_ports; i++) { skip_port_from_conntrack(od, od->localnet_ports[i], S_SWITCH_IN_PRE_ACL, S_SWITCH_OUT_PRE_ACL, - 110, lflows); + 65535, lflows); } - /* stateless filters always take precedence over stateful ACLs. */ - build_stateless_filters(od, port_groups, lflows); + if (od->has_stateless_acl) { + build_acl_filters(od, port_groups, lflows); + } /* Ingress and Egress Pre-ACL Table (Priority 110). * * Not to do conntrack on ND and ICMP destination * unreachable packets. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, + ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 65535, "nd || nd_rs || nd_ra || mldv1 || mldv2 || " "(udp && udp.src == 546 && udp.dst == 547)", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, + ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 65535, "nd || nd_rs || nd_ra || mldv1 || mldv2 || " "(udp && udp.src == 546 && udp.dst == 547)", "next;"); diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index bd3c3aa26..f92906e5b 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -459,7 +459,9 @@ priority-110 flow is added to skip over stateful ACLs. IPv6 Neighbor Discovery and MLD traffic also skips stateful ACLs. For "allow-stateless" ACLs, a flow is added to bypass setting the hint for connection tracker - processing. + processing. If datapath has "allow-stateless" and "allow-related" ACLs, + a flow for each ACL would be added with priority value calculated as 1000 + plus ACL's priority and action <code>reg0[0] = 1; next;</code>. </p> <p> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index c4424ab14..b168b79c9 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -3100,7 +3100,7 @@ AT_CLEANUP ]) OVN_FOR_EACH_NORTHD([ -AT_SETUP([ACL allow-stateless overrides stateful rules with higher priority - Logical_Switch]) +AT_SETUP([ACL allow-stateless doesn't override stateful rules with higher priority - Logical_Switch]) ovn_start ovn-nbctl ls-add ls @@ -3110,22 +3110,29 @@ ovn-nbctl lsp-add ls lsp2 ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02 for direction in from to; do - ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related - ovn-nbctl acl-add ls ${direction}-lport 3 "udp" allow + ovn-nbctl acl-add ls ${direction}-lport 3 "ip4.src == 10.0.0.0/24 && tcp" allow-related + ovn-nbctl acl-add ls ${direction}-lport 3 "ip4.src == 10.0.0.0/24 && udp" allow +done + +# Allow stateless with *lower* priority. It should not override stateful rules. +for direction in from to; do + ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add ls ${direction}-lport 1 udp allow-stateless done ovn-nbctl --wait=sb sync flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' -flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' +flow_ip_10='ip.ttl==64 && ip4.src == 10.0.0.1 && ip4.dst == 66.66.66.66' +flow_ip_20='ip.ttl==64 && ip4.src == 20.0.0.1 && ip4.dst == 66.66.66.66' flow_tcp='tcp && tcp.dst == 80' flow_udp='udp && udp.dst == 80' lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) -# TCP packets should go to conntrack. -flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" +# TCP and UDP packets should go to conntrack. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip_10} && ${flow_tcp}" AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=10.0.0.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 ct_next(ct_state=new|trk) { ct_next(ct_state=new|trk) { output("lsp2"); @@ -3133,24 +3140,25 @@ ct_next(ct_state=new|trk) { }; ]) -# Allow stateless with *lower* priority. It always beats stateful rules. -for direction in from to; do - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless - ovn-nbctl acl-add ls ${direction}-lport 1 udp allow-stateless -done -ovn-nbctl --wait=sb sync +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip_10} && ${flow_udp}" +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl +# udp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=10.0.0.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80 +ct_next(ct_state=new|trk) { + ct_next(ct_state=new|trk) { + output("lsp2"); + }; +}; +]) -# TCP packets should not go to conntrack anymore. -flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" +# TCP and UDP packets from 20.0.0.1 should go to conntrack. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip_20} && ${flow_tcp}" AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl -# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=20.0.0.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 output("lsp2"); ]) - -# UDP packets should not go to conntrack anymore. -flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip_20} && ${flow_udp}" AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl -# udp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80 +# udp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=20.0.0.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80 output("lsp2"); ])
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 <odivlad@gmail.com> --- northd/northd.c | 92 +++++++++++++++++++++++++---------------- northd/ovn-northd.8.xml | 4 +- tests/ovn-northd.at | 48 ++++++++++++--------- 3 files changed, 87 insertions(+), 57 deletions(-)