[ovs-dev] [PATCH v2] Add Docker integration for OVN.

Gurucharan Shetty shettyg at nicira.com
Tue Nov 10 22:45:05 UTC 2015


> Thanks for doing this!  I have a few comments, see below.

Thank you for the review!

>
> I guess that $IP should be $HOST_IP here.
True. I corrected it.


> I think that the paragraph above means that you have to do this on one
> machine selected from your hypervisors.  But "on any machine...where you
> have installed and started Open vSwitch" could be interpreted to mean
> that you run this on *every* hypervisor.  Maybe s/on any machine/on one
> of your machines/?

That is better. I changed the words.

>
>> +Start ovn_northd daemon.  This daemon translates networking intent from Docker
>> +stored in OVN_Northbound databse to logical flows in OVN_Southbound database.
>
> It might be worth s/ovn_northd/ovn-northd/ above since that's the
> correct name of the daemon.

corrected it.

>
>> +```
>> +/usr/share/openvswitch/scripts/ovn-ctl start_northd
>> +```
>> +
>> +* One time setup.
>> +
>> +On each host, where you plan to spawn your containers, you will need to
>> +run the following commands once.
>
> I wonder whether it's worth adding some additional clarification, e.g.:
>
>     (You need to run it again if your OVS database gets cleared.  It is
>     harmless to run it again in any case.)

I added the suggested sentences.

>
>> +```
>> +ovs-vsctl set Open_vSwitch . external_ids:ovn-remote="tcp:$CENTRAL_IP:6640 \
>
> There's a missing " near the end of the line above.

Right, I corrected it. I also changed the paragraph to now read:

$ENCAP_TYPE is the type of tunnel that you would like to use for overlay
networking.  The options are "geneve" or "stt".  (Please note that your
kernel should have support for your chosen $ENCAP_TYPE.  Both geneve
and stt are part of the Open vSwitch kernel module that is compiled from this
repo.  If you use the Open vSwitch kernel module from upstream Linux,
you will need a minumum kernel version of 3.18 for geneve.  There is no stt
support in upstream Linux.  You can verify whether you have the support in your
kernel by doing a "lsmod | grep $ENCAP_TYPE".)

```
ovs-vsctl set Open_vSwitch . external_ids:ovn-remote="tcp:$CENTRAL_IP:6640" \
  external_ids:ovn-encap-ip=$LOCAL_IP external_ids:ovn-encap-type="$ENCAP_TYPE"
```

>> +```
>> +
>> +And finally, start the ovn-controller.
>> +
>> +```
>> +/usr/share/openvswitch/scripts/ovn-ctl start_controller
>> +```
>
> I don't know whether that's really "one time" since it needs to happen
> on each boot.

I changed the line to now read:
And finally, start the ovn-controller.  (You need to run the below command
on every boot)


>
>> +Source the openrc file. e.g.:
>> +````
>> +source openrc.sh
>>  ```
>
> The "source" command name isn't portable, since POSIX does not require
> it.  You can use "." instead.
>
> Are you expecting openrc.sh to be in the current directory?  POSIX says
> that ". ./openrc.sh" is required to source a file in the current
> directory (unless . is in $PATH).

I changed it to now read:
". ./openrc.sh

>
> In the Python code, I wonder whether there are any concerns about
> malicious input.  I mean, what if someone names a subnet "--
> emer-reset", for example (or similar)?  Would that delete basically the
> whole OVS database?  Or does everything show up as a UUID and therefore
> make it safe?  I didn't investigate enough to figure that out.

Thanks for the above warning. Though one could not send malicious
input via docker api (as they check for the validity there), once
could still send a TCP request directly to the driver to carefully
insert " -- $database_command --" as arguments for ovs-vsctl and
ovn-nbctl commands. To handle that I was thinking of doing something
like this:


+def vet_inputs(*args):
+    for arg in args:
+        if arg.find(" -- ") != -1:
+            raise RuntimeError("Input contains invalid characters")
+

     try:
+        vet_inputs(network, subnet, gateway_ip)
         ovn_nbctl("lswitch-add %s -- set Logical_Switch %s "
                   "external_ids:subnet=%s external_ids:gateway_ip=%s"
                   % (network, network, subnet, gateway_ip))


And everywhere else where we pass the user input to ovn_nbctl or
ovs_vsctl calls.
What do you think?

>
> Assuming there's some kind of security story that way,
> Acked-by: Ben Pfaff <blp at ovn.org>



More information about the dev mailing list