[ovs-dev,RFC,v1,1/3] OVN ACL: Replace the usage of ct_label with ct_mark

Message ID 1547165793-14659-2-git-send-email-ankur.sharma@nutanix.com
State Changes Requested
Headers show
Series
  • Associate identifier with OVN ACL connection tracking entry
Related show

Commit Message

Ankur Sharma Jan. 11, 2019, 12:16 a.m.
OVN ACL implementation used ct_label to indicate if a previosuly
allowed connection shoudl not be allowed anymore and vice versa.

However, ct_label is a 128 bit value and we should rather leverage
on ct_mark which is a 32 bit value.

Using ct_mark for this purpose, allows us to use ct_label for storing
other values like, identifier for corresponidng OVN ACL/Security group etc.

signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com>
---
 ovn/lib/logical-fields.c    |  1 +
 ovn/northd/ovn-northd.8.xml | 14 ++++++-------
 ovn/northd/ovn-northd.c     | 48 ++++++++++++++++++++++-----------------------
 tests/ovn.at                |  9 +++++----
 4 files changed, 37 insertions(+), 35 deletions(-)

Comments

Ben Pfaff Feb. 5, 2019, 9:21 p.m. | #1
On Fri, Jan 11, 2019 at 12:16:35AM +0000, Ankur Sharma wrote:
> OVN ACL implementation used ct_label to indicate if a previosuly
> allowed connection shoudl not be allowed anymore and vice versa.
> 
> However, ct_label is a 128 bit value and we should rather leverage
> on ct_mark which is a 32 bit value.
> 
> Using ct_mark for this purpose, allows us to use ct_label for storing
> other values like, identifier for corresponidng OVN ACL/Security group etc.
> 
> signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com>

Thanks for the patch.

I think that the idea here is to retain the existing ct_label.blocked
for compatibility with older ovn-northd, so that during an upgrade the
older logical flows continue to work.  That is a good idea.  I think
that there should be a comment in logical-fields.c explaining why
ct_label.blocked is still there.  Then, someday in the future, when we
think that upgrades from such an old version is no longer important, we
will have an idea why it is there and that we can now to remove it.

I find myself wondering, though, why we have ct_label.blocked at all.
In some other cases where ovn-northd uses a bit specifically, it has a
macro for it, e.g. REGBIT_CONNTRACK_COMMIT.  Another option would be to
have better abstraction, i.e. to name the bit "ct.blocked" instead of
ct_mark.blocked or ct_label.blocked.

Thanks,

Ben.
Ankur Sharma Feb. 6, 2019, 9:29 p.m. | #2
Hi Ben,

Thanks a lot for the review.
Sure, I will add comments in logical-fields.c explaining the reason for retaining ct_label.blocked.
I will rename ct_mark.blocked to ct.blocked as well.

V2 will have all these changes.

Thanks again for review.

Regards,
Ankur

-----Original Message-----
From: Ben Pfaff <blp@ovn.org> 
Sent: Tuesday, February 5, 2019 1:21 PM
To: Ankur Sharma <ankur.sharma@nutanix.com>
Cc: ovs-dev@openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 1/3] OVN ACL: Replace the usage of ct_label with ct_mark

On Fri, Jan 11, 2019 at 12:16:35AM +0000, Ankur Sharma wrote:
> OVN ACL implementation used ct_label to indicate if a previosuly 
> allowed connection shoudl not be allowed anymore and vice versa.
> 
> However, ct_label is a 128 bit value and we should rather leverage on 
> ct_mark which is a 32 bit value.
> 
> Using ct_mark for this purpose, allows us to use ct_label for storing 
> other values like, identifier for corresponidng OVN ACL/Security group etc.
> 
> signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com>

Thanks for the patch.

I think that the idea here is to retain the existing ct_label.blocked for compatibility with older ovn-northd, so that during an upgrade the older logical flows continue to work.  That is a good idea.  I think that there should be a comment in logical-fields.c explaining why ct_label.blocked is still there.  Then, someday in the future, when we think that upgrades from such an old version is no longer important, we will have an idea why it is there and that we can now to remove it.

I find myself wondering, though, why we have ct_label.blocked at all.
In some other cases where ovn-northd uses a bit specifically, it has a macro for it, e.g. REGBIT_CONNTRACK_COMMIT.  Another option would be to have better abstraction, i.e. to name the bit "ct.blocked" instead of ct_mark.blocked or ct_label.blocked.

Thanks,

Ben.

Patch

diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
index a8b5e3c..5165a3a 100644
--- a/ovn/lib/logical-fields.c
+++ b/ovn/lib/logical-fields.c
@@ -108,6 +108,7 @@  ovn_init_symtab(struct shash *symtab)
 
     /* Connection tracking state. */
     expr_symtab_add_field(symtab, "ct_mark", MFF_CT_MARK, NULL, false);
+    expr_symtab_add_subfield(symtab, "ct_mark.blocked", NULL, "ct_mark[0]");
 
     expr_symtab_add_field(symtab, "ct_label", MFF_CT_LABEL, NULL, false);
     expr_symtab_add_subfield(symtab, "ct_label.blocked", NULL, "ct_label[0]");
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 392a5ef..4fef4c4 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -286,14 +286,14 @@ 
       </li>
       <li>
         <code>allow-related</code> ACLs translate into logical
-        flows with the <code>ct_commit(ct_label=0/1); next;</code> actions
+        flows with the <code>ct_commit(ct_mark=0/1); next;</code> actions
         for new connections and <code>reg0[1] = 1; next;</code> for existing
         connections.
       </li>
       <li>
         Other ACLs translate to <code>drop;</code> for new or untracked
-        connections and <code>ct_commit(ct_label=1/1);</code> for known
-        connections.  Setting <code>ct_label</code> marks a connection
+        connections and <code>ct_commit(ct_mark=1/1);</code> for known
+        connections.  Setting <code>ct_mark</code> marks a connection
         as one that was previously allowed, but should no longer be
         allowed due to a policy change.
       </li>
@@ -319,12 +319,12 @@ 
         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.blocked</code> set.
+        the committed flow does not have <code>ct_mark.blocked</code> set.
         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.blocked</code> will get set and packets in the
+        policy, <code>ct_mark.blocked</code> will get set and packets in the
         reply direction will no longer be allowed, either.
       </li>
 
@@ -332,7 +332,7 @@ 
         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), as long
-        as the committed flow does not have <code>ct_label.blocked</code> set.
+        as the committed flow does not have <code>ct_mark.blocked</code> set.
       </li>
 
       <li>
@@ -342,7 +342,7 @@ 
 
       <li>
         A priority-65535 flow that drops all trafic in the reply direction
-        with <code>ct_label.blocked</code> set meaning that the connection
+        with <code>ct_mark.blocked</code> set meaning that the connection
         should 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.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 3fd8a87..a3249c4 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -3546,13 +3546,13 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
              * 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.blocked (which will be done
+             * that case here and un-set ct_mark.blocked (which will be done
              * by ct_commit in the "stateful" stage) to indicate that the
              * connection should be allowed to resume.
              */
             ds_put_format(&match, "((ct.new && !ct.est)"
                                   " || (!ct.new && ct.est && !ct.rpl "
-                                       "&& ct_label.blocked == 1)) "
+                                       "&& ct_mark.blocked == 1)) "
                                   "&& (%s)", acl->match);
             ds_put_cstr(&actions, REGBIT_CONNTRACK_COMMIT" = 1; ");
             build_acl_log(&actions, acl);
@@ -3573,7 +3573,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
             ds_clear(&actions);
             ds_put_format(&match,
                           "!ct.new && ct.est && !ct.rpl"
-                          " && ct_label.blocked == 0 && (%s)",
+                          " && ct_mark.blocked == 0 && (%s)",
                           acl->match);
 
             build_acl_log(&actions, acl);
@@ -3599,7 +3599,7 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
             /* If the packet is not part of an established connection, then
              * we can simply reject/drop it. */
             ds_put_cstr(&match,
-                        "(!ct.est || (ct.est && ct_label.blocked == 1))");
+                        "(!ct.est || (ct.est && ct_mark.blocked == 1))");
             if (!strcmp(acl->action, "reject")) {
                 build_reject_acl_rules(od, lflows, stage, acl, &match,
                                        &actions);
@@ -3611,11 +3611,11 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
                               acl->priority + OVN_ACL_PRI_OFFSET,
                               ds_cstr(&match), ds_cstr(&actions));
             }
-            /* For an existing connection without ct_label set, we've
+            /* For an existing connection without ct_mark 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
+             * connection.  First, we set bit 0 of ct_mark to indicate
              * that this connection is set for deletion.  By not
              * specifying "next;", we implicitly drop the packet after
              * updating conntrack state.  We would normally defer
@@ -3624,8 +3624,8 @@  consider_acl(struct hmap *lflows, struct ovn_datapath *od,
              */
             ds_clear(&match);
             ds_clear(&actions);
-            ds_put_cstr(&match, "ct.est && ct_label.blocked == 0");
-            ds_put_cstr(&actions, "ct_commit(ct_label=1/1); ");
+            ds_put_cstr(&match, "ct.est && ct_mark.blocked == 0");
+            ds_put_cstr(&actions, "ct_commit(ct_mark=1/1); ");
             if (!strcmp(acl->action, "reject")) {
                 build_reject_acl_rules(od, lflows, stage, acl, &match,
                                        &actions);
@@ -3748,56 +3748,56 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows,
          * subsequent packets will hit the flow at priority 0 that just
          * uses "next;"
          *
-         * We also check for established connections that have ct_label.blocked
+         * We also check for established connections that have ct_mark.blocked
          * set 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 set ct_label.blocked=0 to let the connection continue,
+         * We need to set ct_mark.blocked=0 to let the connection continue,
          * which will be done by ct_commit() in the "stateful" stage.
          * Subsequent packets will hit the flow at priority 0 that just
          * uses "next;". */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1,
-                      "ip && (!ct.est || (ct.est && ct_label.blocked == 1))",
+                      "ip && (!ct.est || (ct.est && ct_mark.blocked == 1))",
                        REGBIT_CONNTRACK_COMMIT" = 1; next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1,
-                      "ip && (!ct.est || (ct.est && ct_label.blocked == 1))",
+                      "ip && (!ct.est || (ct.est && ct_mark.blocked == 1))",
                        REGBIT_CONNTRACK_COMMIT" = 1; next;");
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
          * Always drop traffic that's in an invalid state.  Also drop
          * reply direction packets for connections that have been marked
-         * for deletion (bit 0 of ct_label is set).
+         * for deletion (bit 0 of ct_mark is set).
          *
          * This is enforced at a higher priority than ACLs can be defined. */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
-                      "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
+                      "ct.inv || (ct.est && ct.rpl && ct_mark.blocked == 1)",
                       "drop;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
-                      "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
+                      "ct.inv || (ct.est && ct.rpl && ct_mark.blocked == 1)",
                       "drop;");
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
          * 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
+         * (bit 0 of ct_mark).  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. */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
                       "ct.est && !ct.rel && !ct.new && !ct.inv "
-                      "&& ct.rpl && ct_label.blocked == 0",
+                      "&& ct.rpl && ct_mark.blocked == 0",
                       "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
                       "ct.est && !ct.rel && !ct.new && !ct.inv "
-                      "&& ct.rpl && ct_label.blocked == 0",
+                      "&& ct.rpl && ct_mark.blocked == 0",
                       "next;");
 
         /* Ingress and Egress ACL Table (Priority 65535).
          *
          * Allow traffic that is related to an existing conntrack entry that
-         * has not been marked for deletion (bit 0 of ct_label).
+         * has not been marked for deletion (bit 0 of ct_mark).
          *
          * This is enforced at a higher priority than ACLs can be defined.
          *
@@ -3807,11 +3807,11 @@  build_acls(struct ovn_datapath *od, struct hmap *lflows,
          * that's generated from a non-listening UDP port.  */
         ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
                       "!ct.est && ct.rel && !ct.new && !ct.inv "
-                      "&& ct_label.blocked == 0",
+                      "&& ct_mark.blocked == 0",
                       "next;");
         ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
                       "!ct.est && ct.rel && !ct.new && !ct.inv "
-                      "&& ct_label.blocked == 0",
+                      "&& ct_mark.blocked == 0",
                       "next;");
 
         /* Ingress and Egress ACL Table (Priority 65535).
@@ -3989,13 +3989,13 @@  build_stateful(struct ovn_datapath *od, struct hmap *lflows)
     ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;");
 
     /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be
-     * committed to conntrack. We always set ct_label.blocked to 0 here as
+     * committed to conntrack. We always set ct_mark.blocked to 0 here as
      * any packet that makes it this far is part of a connection we
      * want to allow to continue. */
     ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
-                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit(ct_label=0/1); next;");
+                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit(ct_mark=0/1); next;");
     ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100,
-                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit(ct_label=0/1); next;");
+                  REGBIT_CONNTRACK_COMMIT" == 1", "ct_commit(ct_mark=0/1); next;");
 
     /* If REGBIT_CONNTRACK_NAT is set as 1, then packets should just be sent
      * through nat (without committing).
diff --git a/tests/ovn.at b/tests/ovn.at
index 8bada32..506c979 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -171,6 +171,7 @@  ct.trk = ct_state[5]
 ct_label = NXM_NX_CT_LABEL
 ct_label.blocked = ct_label[0]
 ct_mark = NXM_NX_CT_MARK
+ct_mark.blocked = ct_mark[0]
 ct_state = NXM_NX_CT_STATE
 ]])
 AT_CLEANUP
@@ -356,7 +357,7 @@  eth.src == {$set3, badmac, 00:00:00:00:00:01} => Syntax error at `badmac' expect
 
 ((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((())))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))) => Parentheses nested too deeply.
 
-ct_label > $set4 => Only == and != operators may be used to compare a field against an empty value set.
+ct_mark > $set4 => Only == and != operators may be used to compare a field against an empty value set.
 ]])
 sed 's/ =>.*//' test-cases.txt > input.txt
 sed 's/.* => //' test-cases.txt > expout
@@ -703,10 +704,10 @@  ip,nw_src=10.0.0.2: conjunction(1, 1/2)
 ip,nw_src=10.0.0.3: conjunction(1, 1/2)
 ])
 
-lflow="ip && (!ct.est || (ct.est && ct_label.blocked == 1))"
+lflow="ip && (!ct.est || (ct.est && ct_mark.blocked == 1))"
 AT_CHECK([expr_to_flow "$lflow"], [0], [dnl
-ct_state=+est+trk,ct_label=0x1/0x1,ip
-ct_state=+est+trk,ct_label=0x1/0x1,ipv6
+ct_state=+est+trk,ct_mark=0x1/0x1,ip
+ct_state=+est+trk,ct_mark=0x1/0x1,ipv6
 ct_state=-est+trk,ip
 ct_state=-est+trk,ipv6
 ])