----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51617/#review152897 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 125) <https://reviews.apache.org/r/51617/#comment222075> Can this be `const net::IP& ip`? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 144) <https://reviews.apache.org/r/51617/#comment222074> Can you make this non optional? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 242 - 252) <https://reviews.apache.org/r/51617/#comment222090> This sounds not necessary if we pass in cniContainerID as a required field. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 309) <https://reviews.apache.org/r/51617/#comment222119> `s/_command/script/` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 314) <https://reviews.apache.org/r/51617/#comment222118> you probably want to print the command executed. Probably use `set -x` here? Can we dump all the output of this script to stderr (redirect stdout to stderr). src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 328 - 329) <https://reviews.apache.org/r/51617/#comment222115> Avoid DNAT 127.0.0.0/8 traffic here? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 366) <https://reviews.apache.org/r/51617/#comment222121> `s/_command/script/` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 367) <https://reviews.apache.org/r/51617/#comment222120> Let's make this a script as well and add `set -x` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 398) <https://reviews.apache.org/r/51617/#comment222077> `s/_networkInfo/delegateResult/` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 402) <https://reviews.apache.org/r/51617/#comment222088> Can you also print ADD in error message src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 406) <https://reviews.apache.org/r/51617/#comment222080> quota before and after 'delegatePlugin' src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 408) <https://reviews.apache.org/r/51617/#comment222078> stringify is not needed. Remove space before `:` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 414 - 415) <https://reviews.apache.org/r/51617/#comment222079> Delegate CNI plugin 'xxx' did not return an IPv4 address: ... src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 417) <https://reviews.apache.org/r/51617/#comment222081> This should be delegate failure? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 429) <https://reviews.apache.org/r/51617/#comment222082> This should be delegate failure? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 437) <https://reviews.apache.org/r/51617/#comment222083> kill this line. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 466) <https://reviews.apache.org/r/51617/#comment222084> `s/_networkInfo/delegateResult/` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 469) <https://reviews.apache.org/r/51617/#comment222087> Can you also print "DEL" in the error message? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 483 - 497) <https://reviews.apache.org/r/51617/#comment222089> I'd prefer the following flow: ``` if (cniCommand == spec::CNI_CMD_ADD) { ... } else if (cniCommand == spec::CNI_CMD_DEL) { ... } else { return Error(""); } ``` - Jie Yu On Oct. 17, 2016, 7:12 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51617/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2016, 7:12 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 logic for installing and removing DNAT rules. > > > 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. > > > Apart from these tests ran a single node cluster and did an end-to-end test > with a modified `mesos-execute` binary that can setup port-mapping. > > > Thanks, > > Avinash sridharan > >
