----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51617/#review152569 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 130) <https://reviews.apache.org/r/51617/#comment221615> space after `a` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 262 - 284) <https://reviews.apache.org/r/51617/#comment221619> Is this necessary? I think this should be the job of the container runtime? In Mesos, if attach fails, 'detach' will be called in cleanup. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 329) <https://reviews.apache.org/r/51617/#comment221633> s/portMap/portMapping/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 331 - 334) <https://reviews.apache.org/r/51617/#comment221631> Let's split this into `addPortMapping` and `delPortMapping` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 336) <https://reviews.apache.org/r/51617/#comment221632> s/proto/protocol/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 340 - 350) <https://reviews.apache.org/r/51617/#comment221658> I suggest introduce a helper for this: ``` string getIptablesDNATRule( const IP& ip, const PortMapping& portMapping); ``` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 373 - 378) <https://reviews.apache.org/r/51617/#comment221653> The logic here is a bit complicated. I'd suggest we use C++ string literal and a script here. I would probably introduce a helper `ensureChain` to install the chain first. ``` strings::format( R"~( #!/bin/sh # COMMENTS HERE. iptables -t nat --list %s if [ $? -ne 0 ]; then iptables -t nat -N %s fi # COMMENTS HERE. (Q: what if this rule already exists?) iptables -t nat -A PREROUTING --dst-type LOCAL -j %s )~"); ``` And then, you can install the DNAT rule in the chain. Also, I suggest we calculate `exclude` devices list here in this function, rather than in the top level caller. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 425 - 457) <https://reviews.apache.org/r/51617/#comment221659> Let's don't do this for simplicity for now. I think it's ok to leave the chain there. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 482 - 494) <https://reviews.apache.org/r/51617/#comment221622> Yeah, I understand that we want to do best effort cleanup. I think the logic of this method becomes a bit confusing now. I'd suggest we split `execute` into two: `handleCommandAdd` and `handleCommandDel` - Jie Yu On Oct. 13, 2016, 8:33 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51617/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2016, 8:33 p.m.) > > > Review request for mesos, Jie Yu and Qian Zhang. > > > Bugs: MESOS-6023 > https://issues.apache.org/jira/browse/MESOS-6023 > > > Repository: mesos > > > Description > ------- > > Added the `remove` and `insert` methods. > > > Diffs > ----- > > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp > 7fad707a240234e35828917aea1bc79f42fe130e > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 > > Diff: https://reviews.apache.org/r/51617/diff/ > > > Testing > ------- > > Ran the CNI plugin against a network namespace with the following JSON input: > ``` > { > "name": "mynet", > "type": "port-mapper", > "chain": "MESOS-TEST", > "excludeDevices": ["mesos-cni0"], > "delegate": { > "type" : "bridge", > "bridge": "cni0", > "isGateway": true, > "ipMasq": true, > "ipam": { > "type": "host-local", > "subnet": "192.168.37.0/24", > "routes": [ > { "dst": "0.0.0.0/0" } > ] > } > }, > "args" : { > "org.apache.mesos" : { > "network_info" : { > "port_mappings": { > "host_port" : 8080, > "container_port" : 9000 > } > } > } > } > } > ``` > > Used the ADD command to test that the CNI plugin correctly invokes the > delegate plugin (a CNI bridge plugin in this case) and also inserts the > correct iptable entries for the given port mapping. After running this > plugin, this was the output of the `iptables -t nat -S MESOS-TEST` command: > ``` > sudo iptables -t nat -S MESOS-TEST > -N MESOS-TEST > -A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT > --to-destination 192.168.37.21:9000 > ``` > > Ran a python HTTP server in this network namespace and verified that DNAT > works from outside the box. Was able to connect to port 9000 of this server, > by connecting to port 8080 on the host. > > Used the DEL command to test the CNI plugin correctly deletes the DNAT rule > and chain, if there are no DNAT rules exist in the chain. After running the > DEL command (by injecting `NetworkInfo` into the above JSON schema) verified > the chain and the DNAT rule is deleted from iptables. > > > Thanks, > > Avinash sridharan > >
