-----------------------------------------------------------
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
> 
>

Reply via email to