apocas / docker-modem

Docker Remote API network stack driver.
Apache License 2.0
234 stars 112 forks source link

arrays within query parameters #91

Open twifty opened 6 years ago

twifty commented 6 years ago

The API states that some of the query parameters can be duplicated. for example the 'extrahosts' and 't' parameters of ImageBuild.

I see this library makes use of querystrings, and correctly converts objects to JSON strings. But, it doesn't handle arrays. So, for example, given an object like:

{
   extrahosts: [
     "somehost:162.242.195.82",
     "otherhost:50.31.209.229",
   ]
}

querystrings will covert to 'extrahosts[0]=somehost:162.242.195.82&extrahosts[1]=otherhost:50.31.209.229' (escaped of course).

docker-py uses the requests package which simply repeats the name and is consistent with the API docs: 'extrahosts=somehost:162.242.195.82&extrahosts=otherhost:50.31.209.229' .

While the array notation may work on some systems, it all depends on the binary the daemon uses. It may very well break on some OS's or daemon versions.

ghost commented 6 years ago

@twifty arrays aren't being querystringify'd properly due to buildQuerystring (https://github.com/apocas/docker-modem/blob/master/lib/modem.js#L350). Arrays are being stringify'd before being passed to querystring.

/cc @apocas

apocas commented 5 years ago

Interesting, will check this one.

apocas commented 5 years ago

I can't replicated this, there's even a test for this.

https://github.com/apocas/docker-modem/blob/master/test/modem_test.js#L109

INPUT:

 var opts = {
      "limit": 12,
      "filters": {
        "label": ["staging", "env=green"]
      },
      "t": ["repo:latest", "repo:1.0.0"]
    };

OUTPUT: limit=12&filters={"label":["staging","env=green"]}&t=repo:latest&t=repo:1.0.0

The array "t" was properly converted.

ghost commented 5 years ago

@apocas the test case works because of key !== 't' https://github.com/apocas/docker-modem/blob/01cac7434c1927a5ccd41dbe493ae661b1274815/lib/modem.js#L351

apocas commented 5 years ago

Missed that. Can't remember why that was done. I suspect that endpoints didn't behave the same way, regarding input.

Will check if removing the stringify doesn't break anything.

apocas commented 5 years ago

Passes: https://github.com/apocas/dockerode/blob/master/test/docker.js#L648 Fails: https://github.com/apocas/dockerode/blob/master/test/docker.js#L658

So we can't just remove the stringify. Add that parameter as an exception?

ghost commented 5 years ago

@apocas looks like it was introduced with this commit: https://github.com/apocas/docker-modem/commit/8565c3ffb9158329ae98c0b3ae666f163ed09f51. Regarding my use case, as far as I remember it was related to passing an array of environment variables when creating/starting (not sure) a Docker container using dockerode. It was not working and debug'd down to this part of the code in docker-modem.

apocas commented 5 years ago

But we can't just remove this condition because the are endpoints expecting the data "stringifed". Like https://github.com/apocas/dockerode/blob/master/test/docker.js#L658

On the other hand there are parameters that are expected by Docker to be repeated like "t" in imageBuild. https://docs.docker.com/engine/api/v1.40/#operation/ImageBuild "A name and optional tag to apply to the image in the name:tag format. If you omit the tag the default latest value is assumed. You can provide several t parameters."

If "extrahosts" is one of this cases we can add it to the exception list, like "t". (Docker documentation does not detail this enough in this use case)

roddc commented 2 years ago

@apocas I tried to get multiple images, I need to pass names as array to query, but it doesn't work. I think you need to check the opts[key] is Array or not

lstocchi commented 8 months ago

Same here, i was working with the getImages function from dockerode and i discovered the query string is not created as expected. My object

{
names: ["name1", "name2"]
}

gets changed to

{
names: "[\"name1\", \"name2\"]"
}

in the cloned object and the querystring created by it it's incorrect. /images/get?names=%5B%22name1%22%2C%22name2%22%5D instead of /images/get?names=name1&names=name2

Happy to help if you want!!