Skip to content

Fix --link to a container which net mode is container#11369

Merged
icecrime merged 1 commit intomoby:masterfrom
coolljt0725:fix_link
Mar 17, 2015
Merged

Fix --link to a container which net mode is container#11369
icecrime merged 1 commit intomoby:masterfrom
coolljt0725:fix_link

Conversation

@coolljt0725
Copy link
Contributor

Signed-off-by: Lei Jitang leijitang@huawei.com

If docker run --link link to a container which net mode is container mode, it will failed with
FATA[0000] Error response from daemon: Cannot start container 95c255a241c05afeb489ce2abafb650968b786adbe6ad7ef9c0571a757b96636: Child IP '' is invalid
because if the net mode of a container is container mode, it share with net namespace with other container and the network configuration is all nil.

there are two way to fix this, one is this PR, another way is to prevent user from
linking to a container which net mode is container mode.
which way is the right way? keep me know and I will update this.

ping @LK4D4 @estesp @crosbymichael @jfrazelle

@icecrime
Copy link
Contributor

Nice catch, thanks! I think this way is the proper one.

@coolljt0725
Copy link
Contributor Author

ping , any suggestion for this?

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 16, 2015

@coolljt0725 What if those container --net=container: too? :)

@coolljt0725 coolljt0725 force-pushed the fix_link branch 2 times, most recently from ea532d5 to 3fc9fcd Compare March 17, 2015 06:25
@coolljt0725
Copy link
Contributor Author

@LK4D4 you are right, those container also could be --net=container: . update the fix and the test

Signed-off-by: Lei Jitang <leijitang@huawei.com>
@coolljt0725
Copy link
Contributor Author

@LK4D4 Just in case those container could be --net=host, so do if child.hostConfig.NetworkMode.IsHost() after for child.hostConfig.NetworkMode.IsContainer(), the previous fix is reverse

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 17, 2015

@coolljt0725 Super cool solution.

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 17, 2015

LGTM

@icecrime
Copy link
Contributor

Nice and clean! LGTM

icecrime pushed a commit that referenced this pull request Mar 17, 2015
Fix --link to a container which net mode is container
@icecrime icecrime merged commit cb77ad4 into moby:master Mar 17, 2015
@estesp
Copy link
Contributor

estesp commented Mar 17, 2015

Excellent! Thanks @coolljt0725

LGTM

@estesp
Copy link
Contributor

estesp commented Mar 17, 2015

@icecrime beat me to the punch!! :)

coolljt0725 added a commit to coolljt0725/docker that referenced this pull request Mar 20, 2015
with --link, since moby#11369 add support --net=container with --link
option, there is no need to do conflict checking for --net=container
with --link and actually this conflict checking never work due to the
wrong if expression, it's a useless code.

Signed-off-by: Lei Jitang <leijitang@huawei.com>
@coolljt0725
Copy link
Contributor Author

ping @icecrime @estesp @LK4D4 Seems prevent user from using --link with --net=container
is a desired way, see(https://github.com/docker/docker/blob/master/runconfig/parse.go#L20), but this line never work because if *flNetMode == "container" && flLinks.Len() > 0 is wrong. (really sorry for
didn't realize this at that time). So what should we do ? keep this or revert this?
one of commits in PR #11516 is assume we will keep this and do some clean up

@gdm85
Copy link
Contributor

gdm85 commented Mar 24, 2015

I was not aware of this PR, I think it nicely fixes #9340

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.

7 participants