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]> > ---
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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
