On Tue, Apr 21, 2015 at 03:20:33PM -0700, Justin Pettit wrote:
> > On Apr 20, 2015, at 3:29 PM, Ben Pfaff <b...@nicira.com> wrote:
> > --- a/ovn/northd/automake.mk
> > +++ b/ovn/northd/automake.mk
> > @@ -1,4 +1,8 @@
> > # ovn-northd
> > bin_PROGRAMS += ovn/northd/ovn-northd
> > ovn_northd_ovn_northd_SOURCES = ovn/northd/ovn-northd.c
> > -ovn_northd_ovn_northd_LDADD = ovn/libovn.la ovsdb/libovsdb.la
> > lib/libopenvswitch.la
> > +ovn_northd_ovn_northd_LDADD = \
> > + ovn/libovn.la \
> > + ovn/lib/libovn.la \
>
> Should we combine these two "libovn.la"s?
Fixed.
> It might be time to write up a DESIGN document like we have for OVS.
> The structure of the pipeline would be nice to have so that people
> don't have to go to the code.
I think that's worthwhile, but not yet. Right now it's both very simple
and of interest to very few people.
> I suspect this is going to become quite a bit larger. Do you think
> it's worth breaking into a separate pipeline.c right now?
I don't think it's worthwhile yet.
> > +/* Parses a member of the port_security column 'ps' into 'c'. Returns
> > true if
> > + * successful, false on syntax error. */
> > +static bool
> > +parse_port_security(const char *ps, struct ps_constraint *c)
> > +{
> > + c->eth.type = LEX_T_END;
> > + c->ip4.type = LEX_T_END;
> > + c->ip6.type = LEX_T_END;
> > +
> > + struct lexer lexer;
> > + lexer_init(&lexer, ps);
> > + do {
> > + if (lexer.token.type == LEX_T_INTEGER ||
> > + lexer.token.type == LEX_T_MASKED_INTEGER) {
>
> Does a masked integer make sense? If so, we should definitely document that
> in the ovn-nb man page.
>
> > + struct lex_token *t;
> > +
> > + t = (lexer.token.format == LEX_F_IPV4 ? &c->ip4
> > + : lexer.token.format == LEX_F_IPV6 ? &c->ip6
>
> Is it worth parsing IPv4 and IPv6 now, since the syntax for it isn't even
> fully baked yet?
>
> > + : lexer.token.format == LEX_F_ETHERNET ? &c->eth
> > + : NULL);
> > + if (t) {
> > + if (t->type == LEX_T_END) {
> > + *t = lexer.token;
> > + } else {
> > + VLOG_INFO("%s: port_security has duplicate %s address",
> > + ps,
> > lex_format_to_string(lexer.token.format));
> > + }
> > + lexer_get(&lexer);
> > + lexer_match(&lexer, LEX_T_COMMA);
> > + continue;
> > + }
> > + }
> > +
> > + VLOG_INFO("%s: syntax error in port_security", ps);
> > + lexer_destroy(&lexer);
> > + return false;
> > + } while (lexer.token.type != LEX_T_END);
>
> I don't see how this works, since lexer_get() is not initially called.
I'm getting the impression you're less comfortable than me with
partially implementing a vague specification. I sent a fix:
http://openvswitch.org/pipermail/dev/2015-April/054441.html
> > + /* Table 0: Admission control framework. */
> > + const struct nbrec_logical_switch *lswitch;
> > + NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > + /* Logical VLANs not supported. */
> > + pipeline_add(&pc, lswitch, 0, 100, "vlan.present", "drop");
>
> [This is historical, since I already committed a patch to address
> this, but I think it's still worth noting.]
>
> In the definition of actions from the ovn-sb man page, it looks like
> semicolons may be required. It's actually inconsistent, since only
> the ones listed as not well thought out have semicolons.
I sent a fix:
http://openvswitch.org/pipermail/dev/2015-April/054444.html
> > + /* Table 1: Destination lookup, unicast handling (priority 50), */
> > + struct ds unknown_actions = DS_EMPTY_INITIALIZER;
> > + NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > + for (size_t i = 0; i < lport->n_macs; i++) {
> > + uint8_t mac[ETH_ADDR_LEN];
> > +
> > + if (eth_addr_from_string(lport->macs[i], mac)) {
> > + struct ds match, actions;
> > +
> > + ds_init(&match);
> > + ds_put_format(&match, "eth.dst == %s", lport->macs[i]);
> > +
> > + ds_init(&actions);
> > + ds_put_cstr(&actions, "outport = ");
>
> This is not listed as a supported action syntax. Speaking to you off
> line, it sounds like you'd like to replace "set()" with this syntax.
> It would be good to update the documentation.
I sent a documentation update:
http://openvswitch.org/pipermail/dev/2015-April/054444.html
> > +
> > + /* Table 2: ACLs. */
> > + const struct nbrec_acl *acl;
> > + NBREC_ACL_FOR_EACH (acl, ctx->ovnnb_idl) {
> > + const char *action;
> > +
> > + action = (!strcmp(acl->action, "allow") ||
> > + !strcmp(acl->action, "allow-related")) ? "resubmit" :
> > "drop";
> > + pipeline_add(&pc, acl->lswitch, 2, acl->priority, acl->match,
> > action);
> > + }
>
> It may be worth noting in the ovn-nb man page that we don't really
> support "allow-related" or the "log" flag yet.
OK, I sent a documentation update:
http://openvswitch.org/pipermail/dev/2015-April/054442.html
(You realize that we could write this practically everywhere in that
manpage? ;-)
> > + /* Table 3: Egress port security. */
> > + NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > + struct ds match, actions;
> > +
> > + ds_init(&match);
> > + ds_put_cstr(&match, "outport == ");
> > + json_string_escape(lport->name, &match);
> > + build_port_security("eth.dst",
> > + lport->port_security, lport->n_port_security,
> > + &match);
>
> I imagine this is going to break things like ARP, since we're not
> allowing broadcast when port security is enabled. If people do have
> to call them out specifically, it's going to add a bunch of eth_src
> rules that match on broadcasts that were explicitly dropped from the
> higher priority eth_src[40] rule, which will look weird. Plus, it's
> going to be annoying to have to specify them all. I assume in most
> other product, there's an implied allow for any destination with the
> multicast/broadcast bit set, but I'm not sure.
Sorry, I overlooked multicast/broadcast. I sent a fix:
http://openvswitch.org/pipermail/dev/2015-April/054443.html
> > --- a/ovn/ovn-nb.xml
> > +++ b/ovn/ovn-nb.xml
> > @@ -144,12 +144,14 @@
> > </p>
> >
> > <p>
> > - Exact syntax is TBD. One could simply use comma- or
> > - space-separated L2 and L3 addresses in each set member, or
> > - replace this by a subset of the general-purpose expression
> > - language used for the <ref column="match" table="Pipeline"
> > - db="OVN_Southbound"/> column in the OVN Southbound database's
> > - <ref table="Pipeline" db="OVN_Southbound"/> table.
> > + Each member of the set is a comma- or space-separated list. A single
> > + set member may have an Ethernet address, an IPv4 address, and an IPv6
> > + address, or any subset. Order is not significant.
>
> I touch on this above, and it's not directly related to this patch,
> but the description of port_security states that the supplied
> addresses will also limit the packets that are sent to it. I assume
> that there are exceptions that don't need to be explicitly called out,
> such as broadcast addresses, so it may be nice to list what those are.
> If not, then we'd want to document that, too.
I documented it:
http://openvswitch.org/pipermail/dev/2015-April/054443.html
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
