Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

We shouldn't be modifying unintended fields when creating containers/execs #1371

Closed
paulbellamy opened this issue Aug 28, 2015 · 13 comments
Closed
Assignees
Milestone

Comments

@paulbellamy
Copy link
Contributor

The side effect of decoding/encoding the request body with go-dockerclient, is that it can encode it differently than the client had meant to. This caused a failure in #1367, and could generally cause mangling of requests (e.g. if the fields present in go-dockerclient disagree with the docker server).

We should use a less invasive way of modifying the incoming requests.

Specific instance of this bug is the failure in test_valid_no_config_specified

@rade
Copy link
Member

rade commented Sep 3, 2015

I am reluctant to make such a significant change in 1.1 so close to a release, though I see you are already half-doing that in #1376.

Would updating the go docker client to the latest version - which iirc happens anyway during the release process - ameliorate this issue?

@paulbellamy
Copy link
Contributor Author

I agree it's a bit big for 1.1.

Updating the go docker client may temporarily help, but won't solve other issues (like with volume drivers). The client makes a few simplifying assumptions about the format of the data which are approriate for a client, but not specific enough for the server.

@rade
Copy link
Member

rade commented Sep 3, 2015

won't solve other issues (like with volume drivers)

Oh. Really? The most recent go docker client ignores the volume driver settings?

@paulbellamy
Copy link
Contributor Author

Ah, no, looking again, it does handle them. It's more that longer-term when the API changes the go client (being stricter) will break more often.

@rade
Copy link
Member

rade commented Sep 3, 2015

could somebody please check that #1378 gets fixed when running with a proxy built with the latest golang docker client?

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

On it.

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

It does not get fixed; go-dockerclient does not yet know anything about the new VolumeDriver parameter.

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

So I submitted fsouza/go-dockerclient#367

@bufdev
Copy link

bufdev commented Sep 3, 2015

@bboreham let me know if there's anything else missing on the volumes side of go-dockerclient, I think we might be both trying to accomplish similar things fsouza/go-dockerclient#362

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

@peter-edge ok, but we're reacting to a user issue #1378; I eyeballed the Docker change to look for other missing fields but don't have any good basis to know for sure.

@errordeveloper
Copy link
Contributor

From testing latest change (32a82f2) on Kubernetes, one rather critical issue popped-up. Essentially what happens is that we somehow end-up marshalling integers in scientific notation and the parser on the other end fail with json: cannot unmarshal number 5.24288e+07 into Go value of type int64.

@rade
Copy link
Member

rade commented Sep 4, 2015

@errordeveloper Please open a separate bug for that!

@errordeveloper
Copy link
Contributor

Please open a separate bug for that!

#1390

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants