vmware / vic

vSphere Integrated Containers Engine is a container runtime for vSphere.
http://vmware.github.io/vic
Other
640 stars 173 forks source link

Docker run [cmd] [arg] cannot handle special characters in [arg] #3510

Open chengwang86 opened 7 years ago

chengwang86 commented 7 years ago

VIC version: 0.8

Deployment details:

What was the vic-machine create command used to deploy the VCH?

bin/vic-machine-linux create --name vch-test --target=root:pwd@192.168.60.162 --no-tlsverify --no-tls --appliance-memory 4096 --thumbprint=…

Steps to reproduce:

I got this issue when I tried to deploy a memcached container and a mcrouter container. There is a special character "|" in the command line argument of the mcrouter container.

1. deploy the VCH  (succeed)
2. docker -H 192.168.60.131:2375 run --name mcd1 -d memcached:latest -u root   (succeed)
3. docker -H 192.168.60.131:2375 run -p 5000:5000 --name mcrouter studiosol/mcrouter:latest mcrouter --config-str='{"pools":{"A":{"servers":["mcd1:11211"]}},"route":"PoolRoute|A"}' -p 5000   (fail)

Actual behavior: After step 3, the output is:

E1208 04:07:28.383450   230 main.cpp:303] Expected only one non-option argument
I1208 04:07:28.402492   230 main.cpp:382] mcrouter --config-str={"pools":{"A":{"servers":["mcd1:11211"]}},"route":"PoolRoute A"} –p 
E1208 04:07:28.402642   230 main.cpp:317] invalid ports

Expected behavior:

I1214 17:00:37.619557     1 main.cpp:382] mcrouter --config-str={"pools":{"A":{"servers":["mcd1:11211"]}},"route":"PoolRoute|A"} -p 5000 
I1214 17:00:37.620349     1 main.cpp:481] 19.0.0-master mcrouter startup (1)

Logs:

container-logs.zip

Additional comment:

An easier way to reproduce it is to run docker run -it busybox echo "A|B" On vic it outputs "A" whereas it outputs "A|B" on regular docker.

chengwang86 commented 7 years ago

For the container "mcrouter", if I do "docker inspect mcrouter" on regular docker, it shows:

"Path": "mcrouter",
        "Args": [
            "--config-str={\"pools\":{\"A\":{\"servers\":[\"mcd1:11211\"]}},\"route\":\"PoolRoute|A\"}",
            "-p",
            "5000"
        ],
... ...
"Config": {          
            ... ...
            "Cmd": [
                "mcrouter",
                "--config-str={\"pools\":{\"A\":{\"servers\":[\"mcd1:11211\"]}},\"route\":\"PoolRoute|A\"}",
                "-p",
                "5000"
            ],

However, on vic, it shows:

"Path": "mcrouter",
        "Args": [
            "--config-str={\"pools\":{\"A\":{\"servers\":[\"mcd1:11211\"]}},\"route\":\"PoolRoute",
            "A\"}",
            "-p"
        ],
... ...
        "Config": {
            ... ...
            "Cmd": [
                "mcrouter",
                "--config-str={\"pools\":{\"A\":{\"servers\":[\"mcd1:11211\"]}},\"route\":\"PoolRoute|A\"}",
                "-p",
                "5000"
            ],

So the "Args" is not resolved correctly on vic due to the special character "|".

hickeng commented 7 years ago

Agree with @hmahmood that this is a bug - it derives from the usage of clear text arguments in guestinfo and the need for a separator character. I believe @caglar10ur had tried using other characters for argument separation but don't recall the end result. Regardless we need to be able to handle almost arbitrary arguments so switching to an encoded form may be needed - would degrade diagnostics ability a little, but args are sensitive so are no logged except at high debug levels anyway.

Bumping to medium as it might be rare, but it's hard to justify and going to cause consternation in use given it's leaking a very internal implementation detail.

caglar10ur commented 7 years ago

I tried using unicode characters (like unicode dot, pi sign etc.) thinking that they won't collide with daily usage but learned that guestinfo keys can be ASCII only.

hickeng commented 7 years ago

Suggest adding a mime type prefix or similar with text/plain being the default, and adding support for an encoding such as application/base64 for strings that guestinfo cannot support.

corrieb commented 7 years ago

It's hard to justify this as a "must fix" for 1.2, but given that there's no workaround, this could be the cause of a significant limitation for some customers. @hickeng is it time we revisited this?