> On Apr 20, 2015, at 3:29 PM, Ben Pfaff <b...@nicira.com> wrote:
I know this got pushed this morning, but I had been part way through a review,
so here it is now.
> --- 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?
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 suspect this is going to become quite a bit larger. Do you think it's worth
breaking into a separate pipeline.c right now?
> +/* 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.
> + /* 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.
> + /* Table 0: Ingress port security. */
> + const struct nbrec_logical_port *lport;
> + NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> + struct ds match = DS_EMPTY_INITIALIZER;
> + ds_put_cstr(&match, "inport == ");
> + json_string_escape(lport->name, &match);
> + build_port_security("eth.src",
> + lport->port_security, lport->n_port_security,
> + &match);
This isn't how port security is currently described as working in the ovn-sb
man page. It would be good to make them jibe.
> + /* 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.
> +
> + /* 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.
> + /* 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.
> --- 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.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
