On Tue, Mar 29, 2016 at 7:26 PM, Justin Pettit <jpet...@ovn.org> wrote:
>
> > On Mar 21, 2016, at 7:54 AM, Russell Bryant <russ...@ovn.org> wrote:
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 2cc9c34..c8cca54 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> >
> > + <code>allow</code> ACLs translate into logical flows with
> > + the <code>next;</code> action. If there are any stateful ACLs
> > + on this datapath, then <code>alloc</code> ACLs translate to
>
> Should that be "allow" instead of "alloc"?
That was just a test to see who read the docs very closely. You passed.
>
> > + <code>ct_commit; next;</code>.
> > + </li>
> > + <li>
> > + <code>allow-related</code> ACLs translate into logical
> > + flows with the <code>ct_commit(ct_label=0); next;</code> actions
> > + for new connections and "next;" for existing connections.
>
> Should that "next;" be surrounded by a <code> declaration instead of
> quotes?
Done, thanks.
>
> > - 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=1</code> set.
>
> I realize it has the same effect since we set the whole field, but I think
> it actually just matches on "ct_label[0]".
You're right, thanks.
>
> > + We only handle traffic in the reply direction here because
> > + we want all packets going in the request direction to still
> > + go through the flows that implement the currently defined
> > + policy based on ACLs. If a connection is no longer allowed by
> > + policy, <code>ct_label</code> will get set and packets in the
> > + reply direction will no longer be allowed, either.
>
> I think it would be good to specify which bit gets set here, since we may
> use other bits in the future, and we may forget to update this comment.
>
Done.
>
> > </li>
> >
> > <li>
> > A priority-65535 flow that allows any traffic that is considered
> > related to a committed flow in the connection tracker (e.g., an
> > - ICMP Port Unreachable from a non-listening UDP port).
> > + ICMP Port Unreachable from a non-listening UDP port), as long
> > + as the committed flow does not have <code>ct_label=1</code> set.
>
> Same comment about "ct_label[0]".
Done.
> A priority-65535 flow that drops all traffic marked by the
> > - connection tracker as invalid.
> > + connection tracker as invalid or reply packets with
> > + <code>ct_label=1</code> set meaning that the connection should
>
> Ditto.
>
Done.
>
> > + no longer be allowed due to a policy change. Packets in the
> > + request direction are skipped here to let a newly created
> > + ACL re-allow this connection.
>
> This sentence makes it sound like invalid packets will be allowed in the
> request direction, which I don't think is desired behavior, and it doesn't
> appear to be what the code does (ie, the code looks right to me).
>
Yeah, that sentence was pretty awkward. I split it into two sections to
hopefully make it more clear.
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -1384,48 +1384,77 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows, struct hmap *ports)
> > ...
> > + * We use "ct_commit" for a connection that is not already known
> > + * by the connection tracker. Once a connection is committed,
> > + * subsequent packets will hit the flow at priority 0 that just
> > + * uses "next;"
> > + *
> > + * We also check for established connections that have ct_label
> set
>
> Once again, I would mention "ct_label[0]".
>
>
Done.
> > + * on them. That's a connection that was disallowed, but is now
> > + * allowed by policy again since it hit this default-allow flow.
> > + * We need to clear the mark to let the connection continue.
>
> "mark" is correct generically, but it might be clearer to say "label".
> Also, I would mention that it's the "special" bit.
>
Done. That was probably left over from when I was using ct_mark and my
search for "ct_mark" didn't catch it.
>
> > /* Ingress and Egress ACL Table (Priority 65535).
> > *
> > - * Always allow traffic that is established to a committed
> > - * conntrack entry. This is enforced at a higher priority than
> > + * Allow reply traffic that is part of an established
> > + * conntrack entry that has not been marked for deletion
> > + * (bit 0 of ct_label). We only match traffic in the
> > + * reply direction because we want traffic in the request
> > + * direction to hit the currently defined policy from ACLs.
> > + *
> > + * This is enforced at a higher priority than
> > * ACLs can be defined. */
>
> I think this may be able to to fit on one line.
Done.
> > @@ -1435,38 +1464,102 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows, struct hmap *ports)
> > bool ingress = !strcmp(acl->direction, "from-lport") ? true
> :false;
> > enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL :
> S_SWITCH_OUT_ACL;
> >
> > - if (!strcmp(acl->action, "allow")) {
> > - /* If there are any stateful flows, we must even commit
> "allow"
> > - * actions. This is because, while the initiater's
> > - * direction may not have any stateful rules, the server's
> > - * may and then its return traffic would not have an
> > - * associated conntrack entry and would return "+invalid".
> */
> > - const char *actions = has_stateful ? "ct_commit; next;" :
> "next;";
> > - ovn_lflow_add(lflows, od, stage,
> > - acl->priority + OVN_ACL_PRI_OFFSET,
> > - acl->match, actions);
> > - } else if (!strcmp(acl->action, "allow-related")) {
> > - struct ds match = DS_EMPTY_INITIALIZER;
> > + if (!strcmp(acl->action, "allow")
> > + || !strcmp(acl->action, "allow-related")) {
> > + if (!has_stateful) {
> > + /* If there are any stateful flows, we must even commit
> "allow"
> > + * actions. This is because, while the initiater's
> > + * direction may not have any stateful rules, the
> server's
> > + * may and then its return traffic would not have an
> > + * associated conntrack entry and would return
> "+invalid". */
> > + ovn_lflow_add(lflows, od, stage,
> > + acl->priority + OVN_ACL_PRI_OFFSET,
> > + acl->match, "next;");
>
> I think that big comment should go right above the "!has_stateful" check,
> since its really describing the "else" case.
>
Done.
>
> > + } else {
> > + struct ds match = DS_EMPTY_INITIALIZER;
> > +
> > + /* Commit the connection tracking entry if its a new
> connection
>
> Since I'm already being nit-picky, I think that should be "it's".
>
Done.
>
> > + * that matches this ACL. After this commit, the reply
> traffic
> > + * is allowed by a flow we create at priority 65535,
> defined
> > + * earlier.
> > + *
> > + * It's also possible that a known connection was
> marked for
> > + * deletion after a policy was deleted, but the policy
> was
> > + * re-added while that connection is still known. We
> catch that
> > + * case here and un-set ct_label to indicate that the
> connection
>
> I'd mention the specific bit in "ct_label".
>
Done.
>
> > + * should be allowed to resume.
> > + */
> > + ds_put_format(&match, "((ct.new && !ct.est)"
> > + " || (!ct.new && ct.est &&
> !ct.rpl "
> > + "&& ct_label[0] == 1)) "
> > + "&& (%s)", acl->match);
> > + ovn_lflow_add(lflows, od, stage,
> > + acl->priority + OVN_ACL_PRI_OFFSET,
> > + ds_cstr(&match), "ct_commit(ct_label=0);
> next;");
>
> If you end up allowing bit-level setting in the previous patch, I'd use
> that here.
>
I checked with Joe and he's still working on fixes for that, so I'm holding
off for now.
>
>
> > + } else if (!strcmp(acl->action, "drop")
> > + || !strcmp(acl->action, "reject")) {
> > + struct ds match = DS_EMPTY_INITIALIZER;
> > + /* XXX Need to support "reject", treat it as "drop;" for
> now. */
> > ...
> > - } else if (!strcmp(acl->action, "reject")) {
> > - /* xxx Need to support "reject". */
> > - VLOG_INFO("reject is not a supported action");
> > - ovn_lflow_add(lflows, od, stage,
> > - acl->priority + OVN_ACL_PRI_OFFSET,
> > - acl->match, "drop;");
>
> We used to log an error for "reject" actions. I think it still may be
> worth doing.
>
Oops. I re-added the VLOG_INFO() for the "reject" case.
>
> > + 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 previous
> committed
>
> s/previous/previously/
>
Done. Thanks for the review!
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
