| Message ID | B4B350CE-CC74-41A4-B541-02B33271CD95@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Sep 10, 2015 at 02:07:22PM -0700, Justin Pettit wrote: > > > On Sep 5, 2015, at 4:40 PM, Ben Pfaff <blp@nicira.com> wrote: > > > > On Fri, Sep 04, 2015 at 05:39:01PM -0700, Justin Pettit wrote: > >> Introduce a new "direction" column to the ACL table that accepts the > >> values "to-lport" and "from-lport". Also reserve the ACL priority 65535 > >> for return traffic associated with the "allow-related" action. > >> > >> Signed-off-by: Justin Pettit <jpettit@nicira.com> > > > > I'd prefer to squash this with the commit (presumably patch 2/2) that > > actually implements the newly documented behavior. > > As you noted in the follow-up message, the implementation is actually in the patch that adds "allow-related" support. There's no reason not to add support now; it just didn't occur to me based on the order I was working on these patches. I've appended an incremental that add the support. Does it look reasonable? > > > Here: > > > > <ref table="Logical_Flow" db="OVN_Southbound"/> table. The > > <code>outport</code> logical port is only available in the > > <code>to-lport</code> direction. > > > > I'd consider adding a parenthetical to make it perfectly clear, e.g.: > > > > <ref table="Logical_Flow" db="OVN_Southbound"/> table. The > > <code>outport</code> logical port is only available in the > > <code>to-lport</code> direction (the <code>inport</code> is > > available in both directions). > > Okay, I updated it. > > > Acked-by: Ben Pfaff <blp@nicira.com> > > Thanks. If the incremental looks good, I'll push the series. The incremental looks good (and smaller than I expected).
> On Sep 10, 2015, at 2:30 PM, Ben Pfaff <blp@nicira.com> wrote: > > On Thu, Sep 10, 2015 at 02:07:22PM -0700, Justin Pettit wrote: >> >>> On Sep 5, 2015, at 4:40 PM, Ben Pfaff <blp@nicira.com> wrote: >>> >>> On Fri, Sep 04, 2015 at 05:39:01PM -0700, Justin Pettit wrote: >>>> Introduce a new "direction" column to the ACL table that accepts the >>>> values "to-lport" and "from-lport". Also reserve the ACL priority 65535 >>>> for return traffic associated with the "allow-related" action. >>>> >>>> Signed-off-by: Justin Pettit <jpettit@nicira.com> >>> >>> I'd prefer to squash this with the commit (presumably patch 2/2) that >>> actually implements the newly documented behavior. >> >> As you noted in the follow-up message, the implementation is actually in the patch that adds "allow-related" support. There's no reason not to add support now; it just didn't occur to me based on the order I was working on these patches. I've appended an incremental that add the support. Does it look reasonable? >> >>> Here: >>> >>> <ref table="Logical_Flow" db="OVN_Southbound"/> table. The >>> <code>outport</code> logical port is only available in the >>> <code>to-lport</code> direction. >>> >>> I'd consider adding a parenthetical to make it perfectly clear, e.g.: >>> >>> <ref table="Logical_Flow" db="OVN_Southbound"/> table. The >>> <code>outport</code> logical port is only available in the >>> <code>to-lport</code> direction (the <code>inport</code> is >>> available in both directions). >> >> Okay, I updated it. >> >>> Acked-by: Ben Pfaff <blp@nicira.com> >> >> Thanks. If the incremental looks good, I'll push the series. > > The incremental looks good (and smaller than I expected). I'm that good. Thanks for the reviews. I pushed the series. --Justin
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index f7004f7..253ee59 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -60,6 +60,7 @@ static const char *default_db(void); * These must be listed in the order that the stages will be executed. */ #define INGRESS_STAGES \ INGRESS_STAGE(PORT_SEC, port_sec) \ + INGRESS_STAGE(ACL, acl) \ INGRESS_STAGE(L2_LKUP, l2_lkup) enum ingress_stage { @@ -762,7 +763,28 @@ build_lflows(struct northd_context *ctx, struct hmap *datap ds_destroy(&match); } - /* Ingress table 1: Destination lookup, broadcast and multicast handling + /* Ingress table 1: ACLs (any priority). */ + HMAP_FOR_EACH (od, key_node, datapaths) { + for (size_t i = 0; i < od->nb->n_acls; i++) { + const struct nbrec_acl *acl = od->nb->acls[i]; + const char *action; + + if (strcmp(acl->direction, "from-lport")) { + continue; + } + + action = (!strcmp(acl->action, "allow") || + !strcmp(acl->action, "allow-related")) + ? "next;" : "drop;"; + ovn_lflow_add(&lflows, od, P_IN, S_IN_ACL, acl->priority, + acl->match, action); + } + } + HMAP_FOR_EACH (od, key_node, datapaths) { + ovn_lflow_add(&lflows, od, P_IN, S_IN_ACL, 0, "1", "next;"); + } + + /* Ingress table 2: Destination lookup, broadcast and multicast handling * (priority 100). */ HMAP_FOR_EACH (op, key_node, ports) { if (lport_is_enabled(op->nb)) { @@ -774,7 +796,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapa "outport = \""MC_FLOOD"\"; output;"); } - /* Ingress table 1: Destination lookup, unicast handling (priority 50), */ + /* Ingress table 2: Destination lookup, unicast handling (priority 50), */ HMAP_FOR_EACH (op, key_node, ports) { for (size_t i = 0; i < op->nb->n_macs; i++) { struct eth_addr mac; @@ -805,7 +827,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapa } } - /* Ingress table 1: Destination lookup for unknown MACs (priority 0). */ + /* Ingress table 2: Destination lookup for unknown MACs (priority 0). */ HMAP_FOR_EACH (od, key_node, datapaths) { if (od->has_unknown) { ovn_lflow_add(&lflows, od, P_IN, S_IN_L2_LKUP, 0, "1", @@ -819,6 +841,10 @@ build_lflows(struct northd_context *ctx, struct hmap *datap const struct nbrec_acl *acl = od->nb->acls[i]; const char *action; + if (strcmp(acl->direction, "to-lport")) { + continue; + } + action = (!strcmp(acl->action, "allow") || !strcmp(acl->action, "allow-related")) ? "next;" : "drop;";