Re: [ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
On Wed, Jun 2, 2021 at 12:22 AM Han Zhou wrote:
>
>
>
> On Tue, Jun 1, 2021 at 12:28 PM Ihar Hrachyshka wrote:
> >
> > On Thu, May 20, 2021 at 9:55 PM Han Zhou wrote:
> > >
> > >
> > >
> > > On Thu, May 20, 2021 at 3:22 PM Han Zhou wrote:
> > > >
> > > >
> > > >
> > > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka
> > > > wrote:
> > > > >
> > > > > While we *should not* circumvent conntrack when a stateful ACL of
> > > > > higher
> > > > > priority is present on the switch, we should do so only when
> > > > > allow-stateless and allow-stateful directions are the same, otherwise
> > > > > we
> > > > > should still skip conntrack for allow-stateless ACLs.
> > > > >
> > > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL
> > > > > verb")
> > > >
> > > > Is this patch a "fix" or an "optimization"?
> > > >
> > > > >
> > > > > Signed-off-by: Ihar Hrachyshka
> > > > > ---
> > > > > northd/lswitch.dl| 88 ---
> > > > > northd/ovn-northd.c | 89
> > > > > ++--
> > > > > northd/ovn_northd.dl | 32
> > > > > tests/ovn-northd.at | 72 +++
> > > > > 4 files changed, 213 insertions(+), 68 deletions(-)
> > > > >
> > > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> > > > > index b73cfd047..8a4c9154c 100644
> > > > > --- a/northd/lswitch.dl
> > > > > +++ b/northd/lswitch.dl
> > > > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> > > > > LogicalSwitchACL(ls, acl),
> > > > > nb::ACL(._uuid = acl, .action = "allow-stateless").
> > > > >
> > > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> > > > > +
> > > > > +LogicalSwitchStatelessFromACL(ls, acl) :-
> > > > > +LogicalSwitchStatelessACL(ls, acl),
> > > > > +nb::ACL(._uuid = acl, .direction = "from-lport").
> > > > > +
> > > > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why
> > > > > this
> > > > > +// is an output relation:
> > > > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
> > > > > has_stateless_from_acl: bool)
> > > > > +
> > > > > +LogicalSwitchHasStatelessFromACL(ls, true) :-
> > > > > +LogicalSwitchStatelessFromACL(ls, _).
> > > > > +
> > > > > +LogicalSwitchHasStatelessFromACL(ls, false) :-
> > > > > +nb::Logical_Switch(._uuid = ls),
> > > > > +not LogicalSwitchStatelessFromACL(ls, _).
> > > > > +
> > > > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> > > > > +
> > > > > +LogicalSwitchStatelessToACL(ls, acl) :-
> > > > > +LogicalSwitchStatelessACL(ls, acl),
> > > > > +nb::ACL(._uuid = acl, .direction = "to-lport").
> > > > > +
> > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why
> > > > > this
> > > > > // is an output relation:
> > > > > -output relation LogicalSwitchHasStatelessACL(ls: uuid,
> > > > > has_stateless_acl: bool)
> > > > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
> > > > > has_stateless_to_acl: bool)
> > > > >
> > > > > -LogicalSwitchHasStatelessACL(ls, true) :-
> > > > > -LogicalSwitchStatelessACL(ls, _).
> > > > > +LogicalSwitchHasStatelessToACL(ls, true) :-
> > > > > +LogicalSwitchStatelessToACL(ls, _).
> > > > >
> > > > > -LogicalSwitchHasStatelessACL(ls, false) :-
> > > > > +LogicalSwitchHasStatelessToACL(ls, false) :-
> > > > > nb::Logical_Switch(._uuid = ls),
> > > > > -not LogicalSwitchStatelessACL(ls, _).
> > > > > +not LogicalSwitchStatelessToACL(ls, _).
> > > > >
> > > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why
> > > > > this
> > > > > // is an output relation:
> > > > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> > > > > /* Switch relation collects all attributes of a logical switch */
> > > > >
> > > > > relation &Switch(
> > > > > -ls:nb::Logical_Switch,
> > > > > -has_stateful_acl: bool,
> > > > > -has_stateless_acl: bool,
> > > > > -has_acls: bool,
> > > > > -has_lb_vip:bool,
> > > > > -has_dns_records: bool,
> > > > > -has_unknown_ports: bool,
> > > > > -localnet_ports:Vec<(uuid, string)>, // UUID and name of
> > > > > each localnet port.
> > > > > -subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
> > > > > bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > > > -ipv6_prefix: Option,
> > > > > -mcast_cfg: Ref,
> > > > > -is_vlan_transparent: bool,
> > > > > +ls: nb::Logical_Switch,
> > > > > +has_stateful_acl: bool,
> > > > > +has_stateless_from_acl: bool,
> > > > > +has_stateless_to_acl: bool,
> > > > > +has_acls: bool,
> > > > > +has_lb_vip: bool,
> > > > > +has_dns_records:bool,
> > > > > +has_unknown_ports: bool,
> > > > > +localnet_ports: Vec<(uuid, string)>, // UUID and nam
Re: [ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
On Tue, Jun 1, 2021 at 12:28 PM Ihar Hrachyshka wrote:
>
> On Thu, May 20, 2021 at 9:55 PM Han Zhou wrote:
> >
> >
> >
> > On Thu, May 20, 2021 at 3:22 PM Han Zhou wrote:
> > >
> > >
> > >
> > > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka
wrote:
> > > >
> > > > While we *should not* circumvent conntrack when a stateful ACL of
higher
> > > > priority is present on the switch, we should do so only when
> > > > allow-stateless and allow-stateful directions are the same,
otherwise we
> > > > should still skip conntrack for allow-stateless ACLs.
> > > >
> > > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL
verb")
> > >
> > > Is this patch a "fix" or an "optimization"?
> > >
> > > >
> > > > Signed-off-by: Ihar Hrachyshka
> > > > ---
> > > > northd/lswitch.dl| 88
---
> > > > northd/ovn-northd.c | 89
++--
> > > > northd/ovn_northd.dl | 32
> > > > tests/ovn-northd.at | 72 +++
> > > > 4 files changed, 213 insertions(+), 68 deletions(-)
> > > >
> > > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> > > > index b73cfd047..8a4c9154c 100644
> > > > --- a/northd/lswitch.dl
> > > > +++ b/northd/lswitch.dl
> > > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> > > > LogicalSwitchACL(ls, acl),
> > > > nb::ACL(._uuid = acl, .action = "allow-stateless").
> > > >
> > > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> > > > +
> > > > +LogicalSwitchStatelessFromACL(ls, acl) :-
> > > > +LogicalSwitchStatelessACL(ls, acl),
> > > > +nb::ACL(._uuid = acl, .direction = "from-lport").
> > > > +
> > > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why
this
> > > > +// is an output relation:
> > > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
has_stateless_from_acl: bool)
> > > > +
> > > > +LogicalSwitchHasStatelessFromACL(ls, true) :-
> > > > +LogicalSwitchStatelessFromACL(ls, _).
> > > > +
> > > > +LogicalSwitchHasStatelessFromACL(ls, false) :-
> > > > +nb::Logical_Switch(._uuid = ls),
> > > > +not LogicalSwitchStatelessFromACL(ls, _).
> > > > +
> > > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> > > > +
> > > > +LogicalSwitchStatelessToACL(ls, acl) :-
> > > > +LogicalSwitchStatelessACL(ls, acl),
> > > > +nb::ACL(._uuid = acl, .direction = "to-lport").
> > > > +
> > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why
this
> > > > // is an output relation:
> > > > -output relation LogicalSwitchHasStatelessACL(ls: uuid,
has_stateless_acl: bool)
> > > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
has_stateless_to_acl: bool)
> > > >
> > > > -LogicalSwitchHasStatelessACL(ls, true) :-
> > > > -LogicalSwitchStatelessACL(ls, _).
> > > > +LogicalSwitchHasStatelessToACL(ls, true) :-
> > > > +LogicalSwitchStatelessToACL(ls, _).
> > > >
> > > > -LogicalSwitchHasStatelessACL(ls, false) :-
> > > > +LogicalSwitchHasStatelessToACL(ls, false) :-
> > > > nb::Logical_Switch(._uuid = ls),
> > > > -not LogicalSwitchStatelessACL(ls, _).
> > > > +not LogicalSwitchStatelessToACL(ls, _).
> > > >
> > > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why
this
> > > > // is an output relation:
> > > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> > > > /* Switch relation collects all attributes of a logical switch */
> > > >
> > > > relation &Switch(
> > > > -ls:nb::Logical_Switch,
> > > > -has_stateful_acl: bool,
> > > > -has_stateless_acl: bool,
> > > > -has_acls: bool,
> > > > -has_lb_vip:bool,
> > > > -has_dns_records: bool,
> > > > -has_unknown_ports: bool,
> > > > -localnet_ports:Vec<(uuid, string)>, // UUID and name of
each localnet port.
> > > > -subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > > -ipv6_prefix: Option,
> > > > -mcast_cfg: Ref,
> > > > -is_vlan_transparent: bool,
> > > > +ls: nb::Logical_Switch,
> > > > +has_stateful_acl: bool,
> > > > +has_stateless_from_acl: bool,
> > > > +has_stateless_to_acl: bool,
> > > > +has_acls: bool,
> > > > +has_lb_vip: bool,
> > > > +has_dns_records:bool,
> > > > +has_unknown_ports: bool,
> > > > +localnet_ports: Vec<(uuid, string)>, // UUID and name
of each localnet port.
> > > > +subnet: Option<(in_addr/*subnet*/,
in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > > +ipv6_prefix:Option,
> > > > +mcast_cfg: Ref,
> > > > +is_vlan_transparent:bool,
> > > >
> > > > /* Does this switch have at least one port with type !=
"router"? */
> > > > has_non_router_p
Re: [ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
On Thu, May 20, 2021 at 6:22 PM Han Zhou wrote:
>
>
>
> On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka wrote:
> >
> > While we *should not* circumvent conntrack when a stateful ACL of higher
> > priority is present on the switch, we should do so only when
> > allow-stateless and allow-stateful directions are the same, otherwise we
> > should still skip conntrack for allow-stateless ACLs.
> >
> > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
>
> Is this patch a "fix" or an "optimization"?
>
It can be argued both ways. On one hand, indeed, it reduces the number
of flows in some scenarios. On the other hand, sending traffic to
conntrack when it, considering direction, should not be sent there,
may be considered a bug. I tried to split the series into parts but
here there are intersecting in such a way that the first patch in the
series introduces incorrect behavior in a corner case that is then
adjusted/fixed by this patch under review.
> >
> > Signed-off-by: Ihar Hrachyshka
> > ---
> > northd/lswitch.dl| 88 ---
> > northd/ovn-northd.c | 89 ++--
> > northd/ovn_northd.dl | 32
> > tests/ovn-northd.at | 72 +++
> > 4 files changed, 213 insertions(+), 68 deletions(-)
> >
> > diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> > index b73cfd047..8a4c9154c 100644
> > --- a/northd/lswitch.dl
> > +++ b/northd/lswitch.dl
> > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> > LogicalSwitchACL(ls, acl),
> > nb::ACL(._uuid = acl, .action = "allow-stateless").
> >
> > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> > +
> > +LogicalSwitchStatelessFromACL(ls, acl) :-
> > +LogicalSwitchStatelessACL(ls, acl),
> > +nb::ACL(._uuid = acl, .direction = "from-lport").
> > +
> > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > +// is an output relation:
> > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
> > has_stateless_from_acl: bool)
> > +
> > +LogicalSwitchHasStatelessFromACL(ls, true) :-
> > +LogicalSwitchStatelessFromACL(ls, _).
> > +
> > +LogicalSwitchHasStatelessFromACL(ls, false) :-
> > +nb::Logical_Switch(._uuid = ls),
> > +not LogicalSwitchStatelessFromACL(ls, _).
> > +
> > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> > +
> > +LogicalSwitchStatelessToACL(ls, acl) :-
> > +LogicalSwitchStatelessACL(ls, acl),
> > +nb::ACL(._uuid = acl, .direction = "to-lport").
> > +
> > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > // is an output relation:
> > -output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl:
> > bool)
> > +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
> > has_stateless_to_acl: bool)
> >
> > -LogicalSwitchHasStatelessACL(ls, true) :-
> > -LogicalSwitchStatelessACL(ls, _).
> > +LogicalSwitchHasStatelessToACL(ls, true) :-
> > +LogicalSwitchStatelessToACL(ls, _).
> >
> > -LogicalSwitchHasStatelessACL(ls, false) :-
> > +LogicalSwitchHasStatelessToACL(ls, false) :-
> > nb::Logical_Switch(._uuid = ls),
> > -not LogicalSwitchStatelessACL(ls, _).
> > +not LogicalSwitchStatelessToACL(ls, _).
> >
> > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > // is an output relation:
> > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> > /* Switch relation collects all attributes of a logical switch */
> >
> > relation &Switch(
> > -ls:nb::Logical_Switch,
> > -has_stateful_acl: bool,
> > -has_stateless_acl: bool,
> > -has_acls: bool,
> > -has_lb_vip:bool,
> > -has_dns_records: bool,
> > -has_unknown_ports: bool,
> > -localnet_ports:Vec<(uuid, string)>, // UUID and name of each
> > localnet port.
> > -subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
> > bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > -ipv6_prefix: Option,
> > -mcast_cfg: Ref,
> > -is_vlan_transparent: bool,
> > +ls: nb::Logical_Switch,
> > +has_stateful_acl: bool,
> > +has_stateless_from_acl: bool,
> > +has_stateless_to_acl: bool,
> > +has_acls: bool,
> > +has_lb_vip: bool,
> > +has_dns_records:bool,
> > +has_unknown_ports: bool,
> > +localnet_ports: Vec<(uuid, string)>, // UUID and name of each
> > localnet port.
> > +subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/,
> > bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > +ipv6_prefix:Option,
> > +mcast_cfg: Ref,
> > +is_vlan_transparent:bool,
> >
> > /* Does this switch have at least one port with type != "router"? */
> > has_non_router_port: bool
> > @@ -251,22 +275,24 @@ function ipv6_p
Re: [ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
On Thu, May 20, 2021 at 9:55 PM Han Zhou wrote:
>
>
>
> On Thu, May 20, 2021 at 3:22 PM Han Zhou wrote:
> >
> >
> >
> > On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka wrote:
> > >
> > > While we *should not* circumvent conntrack when a stateful ACL of higher
> > > priority is present on the switch, we should do so only when
> > > allow-stateless and allow-stateful directions are the same, otherwise we
> > > should still skip conntrack for allow-stateless ACLs.
> > >
> > > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
> >
> > Is this patch a "fix" or an "optimization"?
> >
> > >
> > > Signed-off-by: Ihar Hrachyshka
> > > ---
> > > northd/lswitch.dl| 88 ---
> > > northd/ovn-northd.c | 89 ++--
> > > northd/ovn_northd.dl | 32
> > > tests/ovn-northd.at | 72 +++
> > > 4 files changed, 213 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> > > index b73cfd047..8a4c9154c 100644
> > > --- a/northd/lswitch.dl
> > > +++ b/northd/lswitch.dl
> > > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> > > LogicalSwitchACL(ls, acl),
> > > nb::ACL(._uuid = acl, .action = "allow-stateless").
> > >
> > > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> > > +
> > > +LogicalSwitchStatelessFromACL(ls, acl) :-
> > > +LogicalSwitchStatelessACL(ls, acl),
> > > +nb::ACL(._uuid = acl, .direction = "from-lport").
> > > +
> > > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > > +// is an output relation:
> > > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
> > > has_stateless_from_acl: bool)
> > > +
> > > +LogicalSwitchHasStatelessFromACL(ls, true) :-
> > > +LogicalSwitchStatelessFromACL(ls, _).
> > > +
> > > +LogicalSwitchHasStatelessFromACL(ls, false) :-
> > > +nb::Logical_Switch(._uuid = ls),
> > > +not LogicalSwitchStatelessFromACL(ls, _).
> > > +
> > > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> > > +
> > > +LogicalSwitchStatelessToACL(ls, acl) :-
> > > +LogicalSwitchStatelessACL(ls, acl),
> > > +nb::ACL(._uuid = acl, .direction = "to-lport").
> > > +
> > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > > // is an output relation:
> > > -output relation LogicalSwitchHasStatelessACL(ls: uuid,
> > > has_stateless_acl: bool)
> > > +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
> > > has_stateless_to_acl: bool)
> > >
> > > -LogicalSwitchHasStatelessACL(ls, true) :-
> > > -LogicalSwitchStatelessACL(ls, _).
> > > +LogicalSwitchHasStatelessToACL(ls, true) :-
> > > +LogicalSwitchStatelessToACL(ls, _).
> > >
> > > -LogicalSwitchHasStatelessACL(ls, false) :-
> > > +LogicalSwitchHasStatelessToACL(ls, false) :-
> > > nb::Logical_Switch(._uuid = ls),
> > > -not LogicalSwitchStatelessACL(ls, _).
> > > +not LogicalSwitchStatelessToACL(ls, _).
> > >
> > > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > > // is an output relation:
> > > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> > > /* Switch relation collects all attributes of a logical switch */
> > >
> > > relation &Switch(
> > > -ls:nb::Logical_Switch,
> > > -has_stateful_acl: bool,
> > > -has_stateless_acl: bool,
> > > -has_acls: bool,
> > > -has_lb_vip:bool,
> > > -has_dns_records: bool,
> > > -has_unknown_ports: bool,
> > > -localnet_ports:Vec<(uuid, string)>, // UUID and name of each
> > > localnet port.
> > > -subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
> > > bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > -ipv6_prefix: Option,
> > > -mcast_cfg: Ref,
> > > -is_vlan_transparent: bool,
> > > +ls: nb::Logical_Switch,
> > > +has_stateful_acl: bool,
> > > +has_stateless_from_acl: bool,
> > > +has_stateless_to_acl: bool,
> > > +has_acls: bool,
> > > +has_lb_vip: bool,
> > > +has_dns_records:bool,
> > > +has_unknown_ports: bool,
> > > +localnet_ports: Vec<(uuid, string)>, // UUID and name of
> > > each localnet port.
> > > +subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/,
> > > bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > > +ipv6_prefix:Option,
> > > +mcast_cfg: Ref,
> > > +is_vlan_transparent:bool,
> > >
> > > /* Does this switch have at least one port with type != "router"? */
> > > has_non_router_port: bool
> > > @@ -251,22 +275,24 @@ function ipv6_parse_prefix(s: string):
> > > Option {
> > > }
> > > }
> > >
> > > -&Switch(.ls= ls,
> > > -.has_stateful_acl = has_stateful_acl,
> > > -
Re: [ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
On Thu, May 20, 2021 at 3:22 PM Han Zhou wrote:
>
>
>
> On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka
wrote:
> >
> > While we *should not* circumvent conntrack when a stateful ACL of higher
> > priority is present on the switch, we should do so only when
> > allow-stateless and allow-stateful directions are the same, otherwise we
> > should still skip conntrack for allow-stateless ACLs.
> >
> > Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
>
> Is this patch a "fix" or an "optimization"?
>
> >
> > Signed-off-by: Ihar Hrachyshka
> > ---
> > northd/lswitch.dl| 88 ---
> > northd/ovn-northd.c | 89 ++--
> > northd/ovn_northd.dl | 32
> > tests/ovn-northd.at | 72 +++
> > 4 files changed, 213 insertions(+), 68 deletions(-)
> >
> > diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> > index b73cfd047..8a4c9154c 100644
> > --- a/northd/lswitch.dl
> > +++ b/northd/lswitch.dl
> > @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> > LogicalSwitchACL(ls, acl),
> > nb::ACL(._uuid = acl, .action = "allow-stateless").
> >
> > +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> > +
> > +LogicalSwitchStatelessFromACL(ls, acl) :-
> > +LogicalSwitchStatelessACL(ls, acl),
> > +nb::ACL(._uuid = acl, .direction = "from-lport").
> > +
> > +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > +// is an output relation:
> > +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
has_stateless_from_acl: bool)
> > +
> > +LogicalSwitchHasStatelessFromACL(ls, true) :-
> > +LogicalSwitchStatelessFromACL(ls, _).
> > +
> > +LogicalSwitchHasStatelessFromACL(ls, false) :-
> > +nb::Logical_Switch(._uuid = ls),
> > +not LogicalSwitchStatelessFromACL(ls, _).
> > +
> > +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> > +
> > +LogicalSwitchStatelessToACL(ls, acl) :-
> > +LogicalSwitchStatelessACL(ls, acl),
> > +nb::ACL(._uuid = acl, .direction = "to-lport").
> > +
> > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > // is an output relation:
> > -output relation LogicalSwitchHasStatelessACL(ls: uuid,
has_stateless_acl: bool)
> > +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
has_stateless_to_acl: bool)
> >
> > -LogicalSwitchHasStatelessACL(ls, true) :-
> > -LogicalSwitchStatelessACL(ls, _).
> > +LogicalSwitchHasStatelessToACL(ls, true) :-
> > +LogicalSwitchStatelessToACL(ls, _).
> >
> > -LogicalSwitchHasStatelessACL(ls, false) :-
> > +LogicalSwitchHasStatelessToACL(ls, false) :-
> > nb::Logical_Switch(._uuid = ls),
> > -not LogicalSwitchStatelessACL(ls, _).
> > +not LogicalSwitchStatelessToACL(ls, _).
> >
> > // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> > // is an output relation:
> > @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> > /* Switch relation collects all attributes of a logical switch */
> >
> > relation &Switch(
> > -ls:nb::Logical_Switch,
> > -has_stateful_acl: bool,
> > -has_stateless_acl: bool,
> > -has_acls: bool,
> > -has_lb_vip:bool,
> > -has_dns_records: bool,
> > -has_unknown_ports: bool,
> > -localnet_ports:Vec<(uuid, string)>, // UUID and name of each
localnet port.
> > -subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > -ipv6_prefix: Option,
> > -mcast_cfg: Ref,
> > -is_vlan_transparent: bool,
> > +ls: nb::Logical_Switch,
> > +has_stateful_acl: bool,
> > +has_stateless_from_acl: bool,
> > +has_stateless_to_acl: bool,
> > +has_acls: bool,
> > +has_lb_vip: bool,
> > +has_dns_records:bool,
> > +has_unknown_ports: bool,
> > +localnet_ports: Vec<(uuid, string)>, // UUID and name of
each localnet port.
> > +subnet: Option<(in_addr/*subnet*/,
in_addr/*mask*/, bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> > +ipv6_prefix:Option,
> > +mcast_cfg: Ref,
> > +is_vlan_transparent:bool,
> >
> > /* Does this switch have at least one port with type != "router"?
*/
> > has_non_router_port: bool
> > @@ -251,22 +275,24 @@ function ipv6_parse_prefix(s: string):
Option {
> > }
> > }
> >
> > -&Switch(.ls= ls,
> > -.has_stateful_acl = has_stateful_acl,
> > -.has_stateless_acl = has_stateless_acl,
> > -.has_acls = has_acls,
> > -.has_lb_vip= has_lb_vip,
> > -.has_dns_records = has_dns_records,
> > -.has_unknown_ports = has_unknown_ports,
> > -.localnet_ports= localnet_ports,
> > -.subnet= subnet,
> > -
Re: [ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
On Mon, May 17, 2021 at 2:47 PM Ihar Hrachyshka wrote:
>
> While we *should not* circumvent conntrack when a stateful ACL of higher
> priority is present on the switch, we should do so only when
> allow-stateless and allow-stateful directions are the same, otherwise we
> should still skip conntrack for allow-stateless ACLs.
>
> Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
Is this patch a "fix" or an "optimization"?
>
> Signed-off-by: Ihar Hrachyshka
> ---
> northd/lswitch.dl| 88 ---
> northd/ovn-northd.c | 89 ++--
> northd/ovn_northd.dl | 32
> tests/ovn-northd.at | 72 +++
> 4 files changed, 213 insertions(+), 68 deletions(-)
>
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index b73cfd047..8a4c9154c 100644
> --- a/northd/lswitch.dl
> +++ b/northd/lswitch.dl
> @@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
> LogicalSwitchACL(ls, acl),
> nb::ACL(._uuid = acl, .action = "allow-stateless").
>
> +relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
> +
> +LogicalSwitchStatelessFromACL(ls, acl) :-
> +LogicalSwitchStatelessACL(ls, acl),
> +nb::ACL(._uuid = acl, .direction = "from-lport").
> +
> +// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> +// is an output relation:
> +output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
has_stateless_from_acl: bool)
> +
> +LogicalSwitchHasStatelessFromACL(ls, true) :-
> +LogicalSwitchStatelessFromACL(ls, _).
> +
> +LogicalSwitchHasStatelessFromACL(ls, false) :-
> +nb::Logical_Switch(._uuid = ls),
> +not LogicalSwitchStatelessFromACL(ls, _).
> +
> +relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
> +
> +LogicalSwitchStatelessToACL(ls, acl) :-
> +LogicalSwitchStatelessACL(ls, acl),
> +nb::ACL(._uuid = acl, .direction = "to-lport").
> +
> // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> // is an output relation:
> -output relation LogicalSwitchHasStatelessACL(ls: uuid,
has_stateless_acl: bool)
> +output relation LogicalSwitchHasStatelessToACL(ls: uuid,
has_stateless_to_acl: bool)
>
> -LogicalSwitchHasStatelessACL(ls, true) :-
> -LogicalSwitchStatelessACL(ls, _).
> +LogicalSwitchHasStatelessToACL(ls, true) :-
> +LogicalSwitchStatelessToACL(ls, _).
>
> -LogicalSwitchHasStatelessACL(ls, false) :-
> +LogicalSwitchHasStatelessToACL(ls, false) :-
> nb::Logical_Switch(._uuid = ls),
> -not LogicalSwitchStatelessACL(ls, _).
> +not LogicalSwitchStatelessToACL(ls, _).
>
> // "Pitfalls of projections" in ddlog-new-feature.rst explains why this
> // is an output relation:
> @@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> /* Switch relation collects all attributes of a logical switch */
>
> relation &Switch(
> -ls:nb::Logical_Switch,
> -has_stateful_acl: bool,
> -has_stateless_acl: bool,
> -has_acls: bool,
> -has_lb_vip:bool,
> -has_dns_records: bool,
> -has_unknown_ports: bool,
> -localnet_ports:Vec<(uuid, string)>, // UUID and name of each
localnet port.
> -subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> -ipv6_prefix: Option,
> -mcast_cfg: Ref,
> -is_vlan_transparent: bool,
> +ls: nb::Logical_Switch,
> +has_stateful_acl: bool,
> +has_stateless_from_acl: bool,
> +has_stateless_to_acl: bool,
> +has_acls: bool,
> +has_lb_vip: bool,
> +has_dns_records:bool,
> +has_unknown_ports: bool,
> +localnet_ports: Vec<(uuid, string)>, // UUID and name of
each localnet port.
> +subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
> +ipv6_prefix:Option,
> +mcast_cfg: Ref,
> +is_vlan_transparent:bool,
>
> /* Does this switch have at least one port with type != "router"? */
> has_non_router_port: bool
> @@ -251,22 +275,24 @@ function ipv6_parse_prefix(s: string):
Option {
> }
> }
>
> -&Switch(.ls= ls,
> -.has_stateful_acl = has_stateful_acl,
> -.has_stateless_acl = has_stateless_acl,
> -.has_acls = has_acls,
> -.has_lb_vip= has_lb_vip,
> -.has_dns_records = has_dns_records,
> -.has_unknown_ports = has_unknown_ports,
> -.localnet_ports= localnet_ports,
> -.subnet= subnet,
> -.ipv6_prefix = ipv6_prefix,
> -.mcast_cfg = mcast_cfg,
> -.has_non_router_port = has_non_router_port,
> -.is_vlan_transparent = is_vlan_transparent) :-
> +&Switch(.ls = ls,
> +.has_stateful_acl = has_stat
[ovs-dev] [PATCH ovn 3/3] Honor ACL direction when omitting ct for stateless
While we *should not* circumvent conntrack when a stateful ACL of higher
priority is present on the switch, we should do so only when
allow-stateless and allow-stateful directions are the same, otherwise we
should still skip conntrack for allow-stateless ACLs.
Fixes: 3187b9fef1 ("ovn-northd: introduce new allow-stateless ACL verb")
Signed-off-by: Ihar Hrachyshka
---
northd/lswitch.dl| 88 ---
northd/ovn-northd.c | 89 ++--
northd/ovn_northd.dl | 32
tests/ovn-northd.at | 72 +++
4 files changed, 213 insertions(+), 68 deletions(-)
diff --git a/northd/lswitch.dl b/northd/lswitch.dl
index b73cfd047..8a4c9154c 100644
--- a/northd/lswitch.dl
+++ b/northd/lswitch.dl
@@ -135,16 +135,39 @@ LogicalSwitchStatelessACL(ls, acl) :-
LogicalSwitchACL(ls, acl),
nb::ACL(._uuid = acl, .action = "allow-stateless").
+relation LogicalSwitchStatelessFromACL(ls: uuid, acl: uuid)
+
+LogicalSwitchStatelessFromACL(ls, acl) :-
+LogicalSwitchStatelessACL(ls, acl),
+nb::ACL(._uuid = acl, .direction = "from-lport").
+
+// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
+// is an output relation:
+output relation LogicalSwitchHasStatelessFromACL(ls: uuid,
has_stateless_from_acl: bool)
+
+LogicalSwitchHasStatelessFromACL(ls, true) :-
+LogicalSwitchStatelessFromACL(ls, _).
+
+LogicalSwitchHasStatelessFromACL(ls, false) :-
+nb::Logical_Switch(._uuid = ls),
+not LogicalSwitchStatelessFromACL(ls, _).
+
+relation LogicalSwitchStatelessToACL(ls: uuid, acl: uuid)
+
+LogicalSwitchStatelessToACL(ls, acl) :-
+LogicalSwitchStatelessACL(ls, acl),
+nb::ACL(._uuid = acl, .direction = "to-lport").
+
// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
// is an output relation:
-output relation LogicalSwitchHasStatelessACL(ls: uuid, has_stateless_acl: bool)
+output relation LogicalSwitchHasStatelessToACL(ls: uuid, has_stateless_to_acl:
bool)
-LogicalSwitchHasStatelessACL(ls, true) :-
-LogicalSwitchStatelessACL(ls, _).
+LogicalSwitchHasStatelessToACL(ls, true) :-
+LogicalSwitchStatelessToACL(ls, _).
-LogicalSwitchHasStatelessACL(ls, false) :-
+LogicalSwitchHasStatelessToACL(ls, false) :-
nb::Logical_Switch(._uuid = ls),
-not LogicalSwitchStatelessACL(ls, _).
+not LogicalSwitchStatelessToACL(ls, _).
// "Pitfalls of projections" in ddlog-new-feature.rst explains why this
// is an output relation:
@@ -223,18 +246,19 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
/* Switch relation collects all attributes of a logical switch */
relation &Switch(
-ls:nb::Logical_Switch,
-has_stateful_acl: bool,
-has_stateless_acl: bool,
-has_acls: bool,
-has_lb_vip:bool,
-has_dns_records: bool,
-has_unknown_ports: bool,
-localnet_ports:Vec<(uuid, string)>, // UUID and name of each localnet
port.
-subnet:Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
-ipv6_prefix: Option,
-mcast_cfg: Ref,
-is_vlan_transparent: bool,
+ls: nb::Logical_Switch,
+has_stateful_acl: bool,
+has_stateless_from_acl: bool,
+has_stateless_to_acl: bool,
+has_acls: bool,
+has_lb_vip: bool,
+has_dns_records:bool,
+has_unknown_ports: bool,
+localnet_ports: Vec<(uuid, string)>, // UUID and name of each
localnet port.
+subnet: Option<(in_addr/*subnet*/, in_addr/*mask*/,
bit<32>/*start_ipv4*/, bit<32>/*total_ipv4s*/)>,
+ipv6_prefix:Option,
+mcast_cfg: Ref,
+is_vlan_transparent:bool,
/* Does this switch have at least one port with type != "router"? */
has_non_router_port: bool
@@ -251,22 +275,24 @@ function ipv6_parse_prefix(s: string): Option {
}
}
-&Switch(.ls= ls,
-.has_stateful_acl = has_stateful_acl,
-.has_stateless_acl = has_stateless_acl,
-.has_acls = has_acls,
-.has_lb_vip= has_lb_vip,
-.has_dns_records = has_dns_records,
-.has_unknown_ports = has_unknown_ports,
-.localnet_ports= localnet_ports,
-.subnet= subnet,
-.ipv6_prefix = ipv6_prefix,
-.mcast_cfg = mcast_cfg,
-.has_non_router_port = has_non_router_port,
-.is_vlan_transparent = is_vlan_transparent) :-
+&Switch(.ls = ls,
+.has_stateful_acl = has_stateful_acl,
+.has_stateless_from_acl = has_stateless_from_acl,
+.has_stateless_to_acl = has_stateless_to_acl,
+.has_acls = has_acls,
+.has_lb_vip = has_lb_vip,
+.has_dns_records= has_dns_records,
+.has_unknown_ports = has_un
