[ovs-dev] [service controllers 3/3] ofproto: Add support for remote "service controllers".

[Ben Pfaff]

On Fri, Aug 06, 2010 at 04:56:54PM -0700, Justin Pettit wrote:
> On Aug 6, 2010, at 11:26 AM, Ben Pfaff wrote:
> 
> > static void
> > bridge_reconfigure_remotes(struct bridge *br,
> >                            const struct sockaddr_in *managers,
> > @@ -1539,99 +1610,62 @@ bridge_reconfigure_remotes(struct bridge *br,
> > {
> > ...
> > +            VLOG_ERR_RL(&rl, "%s: not adding Unix domain socket controller "
> > +                        "\"%s\" due to possibility for remote exploit",
> > +                        dpif_name(br->dpif), c->target);
> 
> It may be worth commenting on this security vulnerability as it wasn't
> immediately obvious to me that the concern was that a remote manager
> with config access could overwrite files on the switch.

OK, I added a comment.

> > +    free(ocs[0].target);
> 
> It might be nice to document that this is the special mgmt controller.

OK, I added a comment.

> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > 
> > +          Open vSwitch permits a bridge to have any number of primary
> > +          controllers.  When multiple controllers are configured, Open vSwitch
> > +          connects to all of them simultaneously.  Because OpenFlow 1.0 does
> > +          not specify how multiple controllers coordinate in interacting with a
> > +          single switch, so more than one primary controller should be
> > +          specified only if the controllers are themselves designed to
> > +          coordinate with each other.
> 
> I had a hard time processing this last sentence.  It's a bit easier to
> read if the "so" is dropped.  Also, it may be good to reference our
> vendor extension as an option for working with multiple controllers.

Thanks, fixed both.

> >               used only for bootstrapping the OpenFlow PKI at initial switch
> >               setup; <code>ovs-vswitchd</code> does not use it at all.</p>
> >           </dd>
> > -          <dt><code>none</code></dt>
> > -          <dd>Disables the controller.</dd>
> 
> Do we not support "none" anymore?

We support it, but don't document it.  "none" was only useful when the
Open_vSwitch table configured a default controller but a particular
bridge didn't want to have any controller at all.  We don't have default
controllers anymore so there's no need for "none" (and really we could
drop the support).

> > +              The <ref table="Open_vSwitch" column="ssl"/> column in the <ref
> > +              table="Open_vSwitch"/> must point to a valid SSL configuration
> > +              when this form is used.
> 
> Did you want to add table before "must" or drop "the" before the table?

Thanks, did the former.

> This was more involved than I expected.  Thanks for getting it done.

Me too.  You're welcome.