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 <[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
