8000 Take more care over HostConfig on start request in proxy by bboreham · Pull Request #1534 · weaveworks/weave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

Take more care over HostConfig on start request in proxy #1534

Merged
merged 7 commits into from
Oct 20, 2015
Merged

Take more care over HostConfig on start request in proxy #1534

merged 7 commits into from
Oct 20, 2015

Conversation

bboreham
Copy link
Contributor

"There are more things in heaven and earth, HostConfig, than are dreamt of in your philosophy"

Fixes #1531 and #1532

rade
rade reviewed Oct 14, 2015
View reviewed changes
test/666_abuse_of_start_test.sh
# Start c5 with a differently sneaky HostConfig
proxy docker_on $HOST1 create --name=c5 $SMALL_IMAGE $CHECK_ETHWE_UP
proxy docker_api_on $HOST1 POST /containers/c5/start '{"HostConfig": {"NetworkMode": "container:c1"}}'
assert "docker_on $HOST1 inspect -f '{{.HostConfig.NetworkMode}} {{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' c5" "container:c1 false 0 [$docker_bridge_ip]"

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@paulbellamy
Copy link
Contributor

If the user sends both a top-level HostConfig, and a named sub-HostConfig, it looks like we only modify the inner one. Will that override the outer?

Per f2f: it looks like it will, but should test cpuset handling.

@rade
Copy link
Member
rade commented Oct 14, 2015

needs rebasing - the new attach should presumably go into the check_... function.

@paulbellamy
Copy link
Contributor

Maybe add a test for cpuset handling, and the case where there are both HostConfigs, but other than that, LGTM.

@rade rade assigned bboreham and unassigned paulbellamy Oct 15, 2015
@bboreham bboreham force-pushed the 1531-check-hostconfig branch from b5c5d11 to c335d5e Compare October 15, 2015 14:21
@bboreham
Copy link
Contributor Author

I looked into Cpuset. Docker only uses that value if you send both Cpuset and HostConfig. So, for any caller sending just Cpuset, we change the behaviour: previously docker would just ignore the Cpuset and now it will use it, and additionally any HostConfig parameters supplied in the original create will be blanked out.

However, this undesirable behaviour will happen for any valid JSON that Docker was previously ignoring, and we cannot detect all such requests. So I propose not to detect any of them.

@bboreham bboreham assigned paulbellamy and unassigned bboreham Oct 15, 2015
rade
rade reviewed Oct 16, 2015
View reviewed changes
test/666_abuse_of_start_test.sh
@@ -4,24 +4,38 @@

start_suite "Abuse of 'start' operation"

weave_on $HOST1 launch
weave_on $HOST1 launch --log-level=debug

This comment was marked as abuse.

Sign in to view

@rade
Copy link
Member
rade commented Oct 16, 2015

@bboreham needs rebase, apparently.

@bboreham bboreham force-pushed the 1531-check-hostconfig branch from 16b3268 to 3c91860 Compare October 17, 2015 13:08
@bboreham
Copy link
Contributor Author

Rebased and squashed into fewer commits

@paulbellamy paulbellamy assigned bboreham and unassigned paulbellamy Oct 19, 2015
@bboreham bboreham force-pushed the 1531-check-hostconfig branch from bf099fb to 526c9c3 Compare October 20, 2015 10:27
@bboreham bboreham assigned paulbellamy and unassigned bboreham Oct 20, 2015
paulbellamy added a commit that referenced this pull request Oct 20, 2015
@paulbellamy
Merge pull request #1534 from weaveworks/1531-check-hostconfig
baf92da 
Take more care over HostConfig on start request in proxy
@paulbellamy paulbellamy merged commit baf92da into master Oct 20, 2015
@paulbellamy paulbellamy deleted the 1531-check-hostconfig branch October 20, 2015 11:18
@rade rade modified the milestone: 1.2.0 Oct 20, 2015
@rade rade mentioned this pull request Oct 20, 2015
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@paulbellamy paulbellamy

Labels
None yet
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bboreham @paulbellamy @rade
0