add support --net=container with --mac-address, --add-host error out by coolljt0725 · Pull Request #11516 · moby/moby

coolljt0725 · 2015-03-20T08:56:11Z

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

There are two commits in this PR, the first one is to do clean up
for --net=container with --link. The previous code suppose to
prevent user from using --link with --net=container,
but the code if *flNetMode == "container" never work
because the value of *flNetMode should
be like this container:xxxx, so both of us think --net=container
with --link is a desired behavior,
so I created PR #11369 (sorry for didn't realize it wasn't a
desired behavior at that time). What should
we do? revert #11369 and prevent user from using --link
with --net=container or keep #11369 and clean up the
invalid conflict checking? ping @LK4D4 @estesp @icecrime

The second commits is to fix --net=container with --dns no error out,
it's the same problem, if *flNetMode == "container" && flDns.Len() > 0
never work. Since a container with --net=container
means it have the same dns, hosts, mac address with its parent,
--add-host, --mac-address is invalid on creation,
so we should error out when user use these option on creation with --net=container

estesp · 2015-03-20T18:16:31Z

For reference, the "conflict" was added in #7780. Looking at the referenced bug, I'm not sure anyone consciously decided that --link and --net=container were absolutely in conflict, not to mention your prior/merged PR #11369 has made that a "working configuration". I guess we need others to weigh in on whether that seems reasonable to remove that conflict now that it works; not to mention as you note it isn't actually stopping users because of the logic bug :)

Adding your other "conflicts" (dns and mac addr. when --net=container) make sense to me.

@LK4D4 can you take a look?

coolljt0725 · 2015-03-23T11:48:29Z

ping @estesp @LK4D4 @jfrazelle

coolljt0725 · 2015-03-25T01:07:39Z

ping @estesp @cpuguy83 @jfrazelle @LK4D4 WDYT?

coolljt0725 · 2015-03-31T13:33:00Z

hi all, WDYT?

cpuguy83 · 2015-04-02T17:57:05Z

daemon/container.go

This one is hurting my head.

Would it be better to have a if config.NetworkMode.IsBridge() {, and then it's only one check, and it's a positive check?

If/when we have something more complicated than that we can always adjust (not that as it currently is wouldn't need to be adjusted anyway).

Also I think this would need to be &&, not ||, since if anytime it's not host it wouldn't run the 2nd check anyway.

And maybe instead of even that, we can do an IsShared(), assuming anything that is sharing can't change existing network options, and get rid of the need for multiple checks in runconfig.Parse

my mistake here, This should be !config.NetworkMode.IsHost() && !config.NetworkMode.IsContainer() or config.NetworkMode.IsBridge || config.NetworkMode.IsNone, since only --net=bridge and --net=none has its
own private network stack at this time.
Do the checking in runconfig.Parse is to info the user
the setting for shared network mode is invalid or we have some
docs to tell the user what they can set in shared net mode.

cpuguy83 · 2015-04-02T18:16:06Z

Some comments, also needs a rebase.

coolljt0725 · 2015-04-03T03:19:32Z

updated ping @cpuguy83

cpuguy83 · 2015-04-04T03:33:57Z

runconfig/parse.go

prefer positive checks instead of negative ones.
So if netMode.IsHost() || netMode.IsContainer()

cpuguy83 · 2015-04-04T03:35:28Z

Maybe in a separate PR we can refactor this code so we aren't branching quite so much.

estesp · 2015-04-06T21:12:11Z

@coolljt0725 this needs a quick rebase due to merging the --link/--net=container test PR #11686. Also, I think this PR should not remove that conflict, as your other PR (#11369) fixed linking to a --net=container:other but it didn't fix using both flags together on the same container run command.

To be clear, I can get an error when I start 2 containers "foo" and "bar", and then try to link to one and set network to the other:
docker run -ti --rm --link=foo:foo --net=container:bar busybox /bin/sh

This produces a "can't find parent IP" error on container start. Of course, now that #11686 is merged, you can't produce this error on master as the use of the flags together now is correctly producing a conflicts error on parse of runconfig.

However, I agree with your added and corrected "conflicts" in this PR for --dns, --mac-address, and --add-host

coolljt0725 · 2015-04-07T09:03:54Z

@estesp I see, I was a bit of confused at that time. the PR is updated. The first commit is to refactor the code of checking conflict option with --net, use netMode.IsHost instead of *flNetMode == "host" and so on. I think this is better.
The second commit is to add support --net=container with --mac-address, --add-host error out
and add some docs. Thanks

cpuguy83 · 2015-04-07T13:19:09Z

These checks also need to happen in ContainerHostConfigFromJob, so the validations should probably be extracted out into their own function so they can be run in both places.

dmcgowan · 2015-04-21T21:58:55Z

runconfig/parse.go

Typo in name ErrConfiictNetworkHosts

dmcgowan · 2015-04-21T22:03:13Z

@cpuguy83 are you comments still valid with the job removal work?

cpuguy83 · 2015-04-21T23:41:12Z

Yes, but I think it should go in verifyHostConfig

coolljt0725 · 2015-04-22T01:00:49Z

@dmcgowan @cpuguy83 Thanks, I'll update

coolljt0725 · 2015-04-22T02:32:28Z

@cpuguy83 This is a problem to move this check to verifyHostConfig due to the Hostname is not in the HostConfig, it's in the Config, so the conflict network and hostname can't be check in verifyHostConfig. Do you have any suggestion?

icecrime · 2015-04-28T01:22:29Z

Sorry, this needs another rebase :-(

coolljt0725 · 2015-04-29T03:45:20Z

@icecrime rebased
@cpuguy83 Move these check to verifyHostConfig has a problem because Hostname is not in the HostConfig

cpuguy83 · 2015-04-29T19:58:28Z

runconfig/parse.go

This looks like it can be combined with the check above it.

cpuguy83 · 2015-04-29T20:02:32Z

One nit, but LGTM.

coolljt0725 · 2015-04-30T06:00:03Z

update as @cpuguy83 's suggestion

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

coolljt0725 · 2015-05-05T12:04:10Z

rebased because github show it contain conflict

jessfraz · 2015-05-08T00:13:24Z

LGTM

…ag_in_container_netmode add support --net=container with --mac-address, --add-host error out

jessfraz · 2015-05-08T00:13:37Z

thanks!

…e_flag_in_container_netmode add support --net=container with --mac-address, --add-host error out

GordonTheTurtle added status/0-triage dco/yes labels Mar 20, 2015

coolljt0725 mentioned this pull request Mar 20, 2015

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

Merged

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch 3 times, most recently from 87daa6f to 98d3d82 Compare March 20, 2015 11:12

cpuguy83 added status/2-code-review and removed status/0-triage labels Mar 20, 2015

estesp mentioned this pull request Mar 24, 2015

Add test for net=container and links #11686

Merged

coolljt0725 mentioned this pull request Mar 25, 2015

--link should be disallowed with --net=container #9340

Closed

icecrime removed the dco/yes label Mar 30, 2015

cpuguy83 reviewed Apr 2, 2015
View reviewed changes

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch from 98d3d82 to 9e65c93 Compare April 3, 2015 02:13

cpuguy83 reviewed Apr 4, 2015
View reviewed changes

runconfig/parse.go Outdated

Copy link

Member

cpuguy83 Apr 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer positive checks instead of negative ones.
So if netMode.IsHost() || netMode.IsContainer()

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch from 9e65c93 to 1d6d59e Compare April 7, 2015 08:55

coolljt0725 changed the title ~~Fix --net=container with --dns not error out and add support --net=container with --mac-address, --add-host error out~~ add support --net=container with --mac-address, --add-host error out Apr 7, 2015

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch 2 times, most recently from 9f47a34 to f4632b6 Compare April 7, 2015 11:26

icecrime added the Networking label Apr 21, 2015

dmcgowan reviewed Apr 21, 2015
View reviewed changes

runconfig/parse.go Outdated

Copy link

Member

dmcgowan Apr 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in name ErrConfiictNetworkHosts

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch 2 times, most recently from 9085375 to c9a80d1 Compare April 29, 2015 02:44

cpuguy83 reviewed Apr 29, 2015
View reviewed changes

runconfig/parse.go Outdated

Copy link

Member

cpuguy83 Apr 29, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it can be combined with the check above it.

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch 2 times, most recently from 9fcf02c to 90fd6c5 Compare April 30, 2015 03:36

Refactor the code of checking conflict option with netmode.

101f982

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

coolljt0725 force-pushed the add_show_error_set_some_flag_in_container_netmode branch from 90fd6c5 to 0e08e9a Compare May 5, 2015 11:27

Add support --net=container with --mac-address,--add-host error out

0e08e9a

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

jessfraz pushed a commit that referenced this pull request May 8, 2015

Merge pull request #11516 from coolljt0725/add_show_error_set_some_fl…

6b6a26c

…ag_in_container_netmode add support --net=container with --mac-address, --add-host error out

jessfraz merged commit 6b6a26c into moby:master May 8, 2015

lowenna pushed a commit to microsoft/docker that referenced this pull request May 8, 2015

Merge pull request moby#11516 from coolljt0725/add_show_error_set_som…

25049b6

…e_flag_in_container_netmode add support --net=container with --mac-address, --add-host error out

coolljt0725 mentioned this pull request Oct 19, 2015

1.9: Conflicting options: -h and the network mode (--net) #17147

Closed

Conversation

coolljt0725 commented Mar 20, 2015

Uh oh!

estesp commented Mar 20, 2015

Uh oh!

coolljt0725 commented Mar 23, 2015

Uh oh!

coolljt0725 commented Mar 25, 2015

Uh oh!

coolljt0725 commented Mar 31, 2015

Uh oh!

cpuguy83 Apr 2, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Apr 2, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Apr 2, 2015

Choose a reason for hiding this comment

Uh oh!

coolljt0725 Apr 3, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 2, 2015

Uh oh!

coolljt0725 commented Apr 3, 2015

Uh oh!

cpuguy83 Apr 4, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 4, 2015

Uh oh!

estesp commented Apr 6, 2015

Uh oh!

coolljt0725 commented Apr 7, 2015

Uh oh!

cpuguy83 commented Apr 7, 2015

Uh oh!

dmcgowan Apr 21, 2015

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Apr 21, 2015

Uh oh!

cpuguy83 commented Apr 21, 2015

Uh oh!

coolljt0725 commented Apr 22, 2015

Uh oh!

coolljt0725 commented Apr 22, 2015

Uh oh!

icecrime commented Apr 28, 2015

Uh oh!

coolljt0725 commented Apr 29, 2015

Uh oh!

cpuguy83 Apr 29, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 29, 2015

Uh oh!

coolljt0725 commented Apr 30, 2015

Uh oh!

coolljt0725 commented May 5, 2015

Uh oh!

jessfraz commented May 8, 2015

Uh oh!

jessfraz commented May 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants