Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't json-encode array option fields #133

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Don't json-encode array option fields #133

merged 1 commit into from
Sep 7, 2021

Conversation

robertgzr
Copy link
Contributor

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 apocas/dockerode#605

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 apocas/dockerode#605
@apocas apocas merged commit 55c6faf into apocas:master Sep 7, 2021
@apocas
Copy link
Owner

apocas commented Sep 7, 2021

Published

@robertgzr robertgzr deleted the rgz/fix-build-querystring branch September 7, 2021 12:42
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 7, 2021
This requires docker-modem >= 3.0.1:
apocas/docker-modem#133

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 7, 2021
This requires docker-modem >= 3.0.1:
apocas/docker-modem#133

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 8, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 8, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 29, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 29, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Sep 30, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Oct 22, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Oct 22, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
robertgzr added a commit to balena-io-modules/balena-multibuild that referenced this pull request Oct 26, 2021
This requires docker-modem >= 3.0.2:
apocas/docker-modem#133
apocas/docker-modem#134

Signed-off-by: Robert Günzler <robertg@balena.io>
@pdcastro
Copy link

@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
Copy link
Contributor Author

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

@pdcastro
Copy link

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

dfunckt added a commit to balena-io-modules/docker-modem that referenced this pull request May 27, 2022
Most Docker API parameters take JSON values but some array parameters must be passed as multiple querystring parameters. Two known such parameters are `t` and `extrahosts` for the `/build` endpoint. There may be others.

This changes Modem to behave in a similar way: encode values to JSON by default, except the known special cases.

This properly fixes apocas/dockerode#605 and effectively reverts apocas#133 and apocas#134.
dfunckt added a commit to balena-io-modules/docker-modem that referenced this pull request May 27, 2022
Most Docker API parameters take JSON values but some array parameters must be passed as multiple querystring parameters. Two known such parameters are `t` and `extrahosts` for the `/build` endpoint. There may be others.

This changes Modem to behave in a similar way: encode values to JSON by default, except the known special cases.

This properly fixes apocas/dockerode#605 and effectively reverts apocas#133 and apocas#134.
dfunckt added a commit to balena-io-modules/docker-modem that referenced this pull request May 27, 2022
Most Docker API parameters take JSON values but some array parameters must be passed as multiple querystring parameters. Two known such parameters are `t` and `extrahosts` for the `/build` endpoint. There may be others.

This changes Modem to behave in a similar way: encode values to JSON by default, except the known special cases.

This properly fixes apocas/dockerode#605 and apocas#139 and effectively reverts apocas#133 and apocas#134.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use extrahosts propery in image build
3 participants