> On Mar 21, 2016, at 7:54 AM, Russell Bryant <[email protected]> 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"?
> + <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?
> - 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]".
> + 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.
> </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]".
> 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.
> + 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).
> --- 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]".
> + * 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.
> /* 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.
> @@ -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.
> + } 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".
> + * 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".
> + * 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.
> + } 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.
> + 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/
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
