> On Jun 30, 2016, at 10:14 PM, Russell Bryant <russ...@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 <jpet...@ovn.org>
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
