[ovs-dev] [PATCH v6 1/1] ovn: Apply ACL changes to existing connections.

Justin Pettit jpettit at ovn.org
Thu Jul 7 17:13:43 UTC 2016


> On Jun 30, 2016, at 10:14 PM, Russell Bryant <russell at ovn.org> wrote:
> 
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 260cc14..d2bddcb 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> 

> 
> @@ -282,20 +299,37 @@
>       </li>
> 
>       <li>
> -        A priority-65535 flow that allows any traffic that has been
> -        committed to the connection tracker (i.e., established flows).
> +        A priority-65535 flow that allows any traffic in the reply
> +        direction for a connection that has been committed to the
> +        connection tracker (i.e., established flows), as long as
> +        the committed flow does not have <code>ct_label[0]=1</code> set.

There are a couple of places where "<code>ct_label[0]=1</code> set", but I think it would be clearer to just drop "=1".  Especially if you take my later suggestion to use a symbolic name for the field.

> @@ -1444,38 +1472,106 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, struct hmap *ports)
> ...
> +                ds_put_format(&match, "((ct.new && !ct.est)"
> +                                      " || (!ct.new && ct.est && !ct.rpl "
> +                                           "&& ct_label[0] == 1)) "

It might be nice to use a symbolic name, similar to "eth.mcast".  That way it's meaning is clearer, and if we ever need to change the label, it only needs to be done in one place.

> +            if (has_stateful) {
> +                /* The implementation of "drop" differs if stateful ACLs are in
> +                 * use for this datapath.  In that case, the actions differ
> +                 * depending on whether the connection was previously committed
> +                 * to the connection tracker with ct_commit.
> +                 *
> +                 * If the packet is not part of an established connection, then
> +                 * we can simply drop it. */

Minor nit: I think it might be clearer to break the two paragraphs into two comments because the first paragraph applies to the entire code block.

> +                ds_put_format(&match,
> +                              "(!ct.est || (ct.est && ct_label[0] == 1)) "
> +                              "&& (%s)",
> +                              acl->match);
> +                ovn_lflow_add(lflows, od, stage, acl->priority +
> +                        OVN_ACL_PRI_OFFSET, ds_cstr(&match), "drop;");
> +
> +                /* For an existing connection without ct_label set, we've
> +                 * encountered a policy change. ACLs previously allowed
> +                 * this connection and we committed the connection tracking
> +                 * entry.  Current policy says that we should drop this
> +                 * connection.  First, we set bit 0 of ct_label to indicate
> +                 * that this connection is set for deletion.  By not
> +                 * specifying "next;", we implicitly drop the packet after
> +                 * updating conntrack state. */
> +
> +                ds_clear(&match);

I think you can drop the blank line after the comment.

Thanks for implementing this.  I apologize for taking so long to review it.  As penance, I'd be happy to rebase the code if you'd like.

Acked-by: Justin Pettit <jpettit at ovn.org>

--Justin





More information about the dev mailing list