apocas / docker-modem

Docker Remote API network stack driver.
Apache License 2.0
228 stars 109 forks source link

Don't json-encode array option fields #133

Closed robertgzr closed 3 years ago

robertgzr commented 3 years ago

Previously there was a check if the options field has the key t to avoid mistransforming the tags list. On build extra_hosts are passed as a JSON string and wouldn't be applied.

We resolve this through checking for an array.

Fixes https://github.com/apocas/dockerode/issues/605

apocas commented 3 years ago

Published

pdcastro commented 2 years ago

@robertgzr, it looks like this PR causes a Docker (balenaEngine) build error in some cases. If opts has the value:

{
  t: 'balena-secrets:latest',
  volumes: [ '/var/lib/docker/tmp:/var/lib/docker/tmp:rw' ],
  forcerm: true
}

Then the query string produced by this buildQuerystring function is:

Before this PR:

't=balena-secrets%3Alatest&volumes=%5B%22%2Fvar%2Flib%2Fdocker%2Ftmp%3A%2Fvar%2Flib%2Fdocker%2Ftmp%3Arw%22%5D&forcerm=true'

i.e. the volumes array was stringified as '["/var/lib/docker/tmp:/var/lib/docker/tmp:rw"]' prior to conversion to a query string, and the image build would succeed.

After this PR:

't=balena-secrets%3Alatest&volumes=%2Fvar%2Flib%2Fdocker%2Ftmp%3A%2Fvar%2Flib%2Fdocker%2Ftmp%3Arw&forcerm=true'

i.e. the volumes array is not stringified, and this causes Docker (balenaEngine) to produce the following image build error:

(HTTP code 500) server error - invalid character '/' looking for beginning of value

This issue was originally reported by a balena CLI end user in balena-io/balena-cli/issues/2440 . The workaround is to downgrade and pin docker-modem to v3.0.0.

Of course, there was a good reason for the changes in this PR, related to the extra_hosts option. It is not clear to me what the right fix would be here, for it to work correctly in all situations. Thoughts?

Note: You'll be thinking: Why didn't he add this comment to the relevant line of code under the the PR's "Files changed" tab? Well, I tried, but somehow GitHub did not allow me (no error, the comment button would just do nothing).

robertgzr commented 2 years ago

hmm @pdcastro did you see #134 , sounds like this is a similar case?

pdcastro commented 2 years ago

@robertgzr, docker-modem versions 3.0.3, 3.0.2 and 3.0.1 all fail with error (HTTP code 500) server error - invalid character '/' looking for beginning of value, while version 3.0.0 succeeds, so I focused on the diff between 3.0.0 and 3.0.1. It sounds like the changes in #134 would have to be amended.