Add a network option to disable default gateway #778

chenchun · 2015-11-26T02:41:42Z

Signed-off-by: Chun Chen ramichen@tencent.com

aboch · 2015-11-26T03:09:58Z

network.go

@@ -521,6 +527,13 @@ func NetworkOptionDeferIPv6Alloc(enable bool) NetworkOption {
 	}
 }

+// NetworkOptionDefaultGW returns an option setter to set if a network needs default gateway
+func NetworkOptionDefaultGW(defaultGW bool) NetworkOption {


Can we call it something like NetworkOptionIsolatedNetwork

As a network property, we do not need to talk about default gw network, that is an implementation detail.
I think I read it somewhere people asking for this feature to refer to it as isolated network.

I think I may have mentioned it a few times at DockerCon EU 😀
And +1 on calling it "Isolated"

ok, updated

chenchun · 2015-11-26T06:23:18Z

@sanimej @aboch updated

aboch · 2015-11-26T14:23:16Z

controller.go

@@ -347,6 +347,7 @@ func (c *controller) NewNetwork(networkType, name string, options ...NetworkOpti
 		ctrlr:       c,
 		persist:     true,
 		drvOnce:     &sync.Once{},
+		isolated:    true,


Default behavior is that the network has or will be provided outside connectivity.
This line can be removed

Sorry for the mistake.

aboch · 2015-11-26T14:28:30Z

@chenchun I think the PR needs more code changes.

If we now allow UI to specify a network to be isolated, I am assuming drivers which naturally provide external connectivity (like bridge) need more changes to block the external connectivity if flag is set, or to reject the network creation if they cannot guarantee no external connectivity

chenchun · 2015-11-26T15:24:01Z

@aboch Should we add an SupportIsolation() bool func to Driver interface or simply do it as my latest update?

aboch · 2015-11-26T15:35:01Z

error.go

@@ -35,6 +35,17 @@ func (ind ErrInvalidNetworkDriver) Error() string {
 // BadRequest denotes the type of this error
 func (ind ErrInvalidNetworkDriver) BadRequest() {}

+// ErrNetworkNotSupportIsolation is returned if attempts to create an isolated
+// network, but the driver of that cannot guarantee no external connectivity.
+type ErrNetworkNotSupportIsolation string


Let's not define new errors.
Please see #269

chenchun · 2015-11-27T05:57:23Z

Then there is a requirement for not adding the default gw network along to remote driver network
Covering the second requirement via isolated network may not be the cleanest way.

How about adding an Isolation bool to driverapi.Capability? This way, remote/bridge drivers can declare that they don't support isolation at registration time. And we can throw ErrNotImplemented if attempts to create a isolated network for those drivers in NewNetwork. And NetworkOptionIsolatedNetwork continues to be useful for overlay drivers. No driver code needs to be changed any more.

sanimej · 2015-11-27T20:56:45Z

@chenchun @aboch The change being discussed in this PR is required if someone wants to isolate the containers on a overlay network because overlay driver by default doesn't set the ep's Gateway thus triggering libnetwork to provide an endpoint in the default gateway network. But I am not sure if this the right fix for #775. I left a comment for @jc-m to try setting the Gateway in the JoinResponse

aboch · 2015-11-27T21:55:08Z

@sanimej My understanding from #775 example is that remote driver may not want to use the default gateway, if it is applying static routes instead.

aboch · 2015-11-27T22:34:47Z

@chenchun, @sanimej

I can see the current libnetwork default behavior/expectation: By default any libnetwork network provides its containers with external connectivity. Either natively (ex. bridge network), or with the help of libnetwork default gw network (for ex. overlay network).

NetworkOptionIsolatedNetwork is a per network option, not a driver option.
For ex., User may want to create one isolated bridge network and one non-isolated (regular) bridge network.

As of now, libnetwork infer whether or not to connect the container to the default gw network from whether the network driver set or does not set the default gateway in the Join JoinInfo interface.
But this is not completely correct as a remote driver might use different means like static routes to provide external connectivity to containers.

Libnetwork could be changed to also check for static route set in JoinInfo when deciding whether to plug the container to the default gw network.

But I don't know if that is enough for any sort of remote drivers.

If not, then we may need the network driver to tell libnetwork if it needs libnetwork's support to provide external connectivity to the containers on its networks.

If the requireExternalConnectivitySupport capability is not set, libnetwork will not bother to connect the container to the default gw network if no default gw (or static route) was set in the JoinInfo interface.

dave-tucker · 2015-11-28T00:45:30Z

@aboch @sanimej

Consider this...

A plugin that is doing VLAN only (plain linux bridge) and uses a noop IPAM driver.
IP addresses are to be assigned automatically via DHCP running inside the container.
This feels to me like a valid use case.

So IMO, we need both:

A way for a user to specify, "don't allow this network access to the internet"
This property should also be sent to the driver so it also knows how to handle this
i.e this PR
A way for a driver to specify, "hey libnetwork, this is a special type of network that doesn't need a default gateway.. so i'm not providing one"

jc-m · 2015-11-28T01:23:34Z

In addition to point 2, I would argue that all networking setup should be delegated to the remote driver, and perhaps provide it with enough information to completely setup the network connectivity. This in my mind is the most flexible way and should enable most use cases. Is there a specific use case to have this partial delegation without full control ?

chenchun · 2015-11-30T06:32:28Z

Updated PR to add ExternalConnectivity capability

jc-m · 2015-11-30T16:16:14Z

I am not sure that have a flag name ExternalConnectivity reflects what is being done. Why not simply name it "NeedGateway" ? I'm sure that external connectivity would require more than just a default gateway.

aboch · 2015-11-30T16:49:52Z

default_gateway.go

@@ -97,11 +97,14 @@ func (sb *sandbox) needDefaultGW() bool {
 	var needGW bool

 	for _, ep := range sb.getConnectedEndpoints() {
+		if !ep.getNetwork().ctrlr.drivers[ep.getNetwork().Type()].capability.ExternalConnectivity {


Please use
ep.getNetwork().driver().capability.ExternalConnectivity instead

aboch · 2015-11-30T16:54:38Z

@jc-m Sure we can think of better names for the flag.
I am actually thinking it should not be a flag, rather a string with some const values (a C enum), so that in future the same field can convey more modes. Like it is done today for the DataScope capability.

aboch · 2015-11-30T21:46:49Z

@chenchun
Discussed offline with @mrjana and @sanimej, we agreed on:

Split this in two PRs: one for the isolated network option (this is an enhancement, lower priority) and one to fix Proposal: Add an option to disable default gateway added by libnetwork #759 needDefaultGW should not return true for remote drivers #775.
In the second PR, we need to fix 2 things:

a) Libnetwork fix for (#775): Libnetwork should not connect the container to the default gw network when the network driver has added the default static route (0.0.0.0/0) in the JoinInfo

b) Network driver does not want default gateway network for its container (#759): Libnetwork will not connect the container to the default gw network when the network driver sets 0.0.0.0 as default gateway in the JoinInfo

2.b) is preferable to the driver capability option because fits in the current protocol and allows a per-container granularity option.

jc-m · 2015-11-30T22:32:30Z

With regard to the capability to have a per container choice, I agree this is a good call. However, it seems that deriving the intent from returned values is likely less optimal. Why not have options for network drivers like there is for storage drivers and have this be an option (label) passed to the driver at run time ? It would give more flexibility to the user and the driver without having side effect being implemented by the daemon.
This would also help solve other issues like passing IP addresses (see moby/moby#6743)

aboch · 2015-11-30T23:14:41Z

@jc-m

This behavior cannot be left to driver specific options. There is a protocol the network driver has to adhere to during container Join. Libnetwork defines and control this protocol in order to provide the same user experience regardless of the network driver in use.

Per container IP allocation is a first class configuration that will be provided via IPAM configuration during endpoint creation (See #707).

jc-m · 2015-12-01T03:37:38Z

@aboch I don't understand how this is breaking the protocol. Are you saying that you cannot have per driver options ? I did not see anywhere the specification/documentation that libnetwork will create a default route or a an interface and route on bridged network in addition to what is specified in the response. Can you point out where this is documented as I don't see it here.

In addition, leaving the ip allocation to the IPAM driver is fine, but there is no way using the IPAM driver to accept or deny an IP address requested by at container creation time. IP allocation is not always dynamic, and not always initiated by a host at the time of instantiation, but maybe implemented by a scheduler requesting the launch of a container allocating the address based on global state.

jc-m · 2015-12-01T03:51:57Z

@aboch with regard to the IP allocation, your text confused me and I'll have to look a bit more closely at #707. It's not clear to me yet how it will be provided to libnetwork.

chenchun · 2015-12-01T08:24:57Z

b) Network driver does not want default gateway network for its container (#759): Libnetwork will not connect the container to the default gw network when the network driver sets 0.0.0.0 as default gateway in the JoinInfo

2.b) is preferable to the driver capability option because fits in the current protocol and allows a per-container granularity option.

@aboch I'm confused. Currently needDefaultGW func already did this by

if len(ep.Gateway()) > 0 {
    return false
}

And how can it be a per-container granularity option? How did drivers know which container needs to set 0.0.0.0 gateway? Does it indicates that docker network connect need to support the gateway flag?

sanimej · 2015-12-01T08:53:46Z

@chenchun

And how can it be a per-container granularity option? How did drivers know which container needs to set 0.0.0.0 gateway? Does it indicates that docker network connect need to support the gateway flag?

Yes, the driver can't set a valid gateway for some containers and set 0.0.0.0 for some other containers on a given network. But during the network create one can pass an option using -opt to tell the driver if Default GW should be provided by libnetwork for containers on that network. So using on the Join response gives the driver granularity at a per network level than the capability exchange, which is fixed.

sanimej · 2015-12-01T09:09:23Z

@jc-m

With regard to the capability to have a per container choice, I agree this is a good call. However, it seems that deriving the intent from returned values is likely less optimal. Why not have options for network drivers like there is for storage drivers and have this be an option (label) passed to the driver at run time ? It would give more flexibility to the user and the driver without having side effect being implemented by the daemon.

Network create has the provision to pass driver specific options using -opt. But its not interpreted or used by libnetwork all. That is why the protocol between libnetwork and driver has to be used to decide when libnetwork should provide Default GW for the container. Please see my previous reply to @chenchun on this.

Instead of using the Gateway IP 0.0.0.0 in the Join response we can add a new boolean (something like NeedDefaultGW) if that makes the interaction more explicit/clear.

chenchun · 2015-12-01T09:24:03Z

Instead of using the Gateway IP 0.0.0.0 in the Join response we can add a new boolean (something like NeedDefaultGW) if that makes the interaction more explicit/clear

I agree with this. setting gateway ip as 0.0.0.0 is very confusing. Actually, I googled it and it means _that the network is locally connected on that interface and no more hops are needed to get to it_. http://unix.stackexchange.com/questions/94018/what-is-the-meaning-of-0-0-0-0-as-a-gateway

sanimej · 2015-12-01T09:40:12Z

The suggestion was to use 0.0.0.0 in the Joininfo only as an indication to libnetwork that the driver isn't providing a valid Gateway IP and libnetwork has to provide the Default gw.

I think its probably better to add a new boolean. @aboch WDYT ?

mavenugo · 2015-12-02T21:14:24Z

@aboch @chenchun @jc-m @sanimej I agree that having an explicit flag makes it cleaner and easier for drivers to make a more explicit request. But, the flag must be backward compatible to make sure any existing drivers (that may not even use this flag) should work unaffected. Hence, the flag should be named and behave something like : DisableGatewayService which will make libnetwork not to provide the Gateway Service implicitly.

Also, I agree with @aboch & @mrjana that we should pull another PR to address the case of user controlling the functionality. There are more things to consider & we dont this PR to be blocked because of unrelated issues.

mavenugo · 2015-12-02T21:47:07Z

@chenchun i just observed that you already have 2 commits for that :) just split them into 2 different PRs. Also, if you agree with my suggestion of the flag name as DisableGatewayService (and ofcourse move it from a capability to join specific flag), then you can remove the changes from bridge and overlay drivers. I would infact prefer that so that we guarantee no compatibility issues to other plugins that were written in 1.9.0 without any additional changes required.

Also, any changes to the driver api needs a doc update for the remote drivers.

chenchun · 2015-12-03T01:51:56Z

Yes, I'll update PR shortly.

mavenugo · 2015-12-03T03:04:48Z

cmd/ovrouter/ovrouter.go

@@ -92,6 +92,8 @@ func (ep *endpoint) AddStaticRoute(destination *net.IPNet, routeType int,
 	return nil
 }

+func (ep *endpoint) DisableGatewayService(diableGatewayService bool) {}


minor spell check nit : diableGatewayService

… to disable default gateway Signed-off-by: Chun Chen <ramichen@tencent.com>

mavenugo · 2015-12-03T05:13:56Z

Thanks @chenchun for making all the changes.

LGTM

chenchun · 2015-12-03T05:20:30Z

Docs needs to be changed. I'll update docs after #796 getting merged.

sanimej · 2015-12-03T07:51:53Z

default_gateway.go

@@ -103,6 +103,9 @@ func (sb *sandbox) needDefaultGW() bool {
 		if ep.getNetwork().Type() == "null" || ep.getNetwork().Type() == "host" {
 			continue
 		}
+		if ep.joinInfo.disableGatewayService {
+			return false
+		}


If a container has endpoints on two networks with one driver setting disableGatewayService to true and the other one setting to false (default case) what should be the behavior ? If we want to provide the default gateway service in that case it has to be a 'continue' here to loop through the other endpoints.

disableGatewayService is more like a per sandbox property. If you enable such service during joining one of its endpoints, others will be infected. Since disableGatewayService is false by default and it becomes true if drivers explicitly setting it to true, I suggest we return false here.

When a driver sets disableGatewayService, i assume that the driver is capable of providing the default route for that container. Hence even if 1 endpoint suggests it can provide the default route for the sandbox, then this function must return false as it is implemented now.
Hence, the current change of returning false looks appropriate.

When a driver sets disableGatewayService, i assume that the driver is capable of providing the default route for that container.

Not necessarily. Earlier this assumption was ok because the check was specifically for Gateway IP. Now a driver can set disableGatewayService and doesn't provide gateway IP or static default route.

I am not against this change. But want to make sure there is agreement on the behavior with this change.

aboch · 2015-12-03T19:41:25Z

@sanimej @chenchun
I think both of you are right.

The disable flag does not fit in the needDefaultGw() function logic: being a endpoint property, it should cause a continue.
But this is because the flag is in fact a sandbox property, like @chenchun said.
If the flag is set, we should not even get into the endpoint loop.

@chenchun @jc-m If you agree this is a sandbox property, if that was the original intent, WDYT about moving the flag to sandbox structure ? Does it make sense ?

Not as part of this PR, this one can go in as is to me. It would be an incremental change.

aboch · 2015-12-03T21:50:54Z

LGTM

Add a network option to disable default gateway

* Update kuryr-libnetwork from branch 'master' - Merge "Handle subnet without gateway" - Handle subnet without gateway * When libnetwork requests the IP address of the gateway (via /IpamDriver.RequestAddress) and the neutron subnet has gateway as None, return '0.0.0.0/0' as a placeholder. * Disable default docker gateway via the 'DisableGatewayService' flag (see moby/libnetwork#778). Change-Id: I3033d28eb268a01de8cf038b1ed20110ca9a31ea Closes-Bug: #1881910

* When libnetwork requests the IP address of the gateway (via /IpamDriver.RequestAddress) and the neutron subnet has gateway as None, return '0.0.0.0/0' as a placeholder. * Disable default docker gateway via the 'DisableGatewayService' flag (see moby/libnetwork#778). Change-Id: I3033d28eb268a01de8cf038b1ed20110ca9a31ea Closes-Bug: #1881910

* When libnetwork requests the IP address of the gateway (via /IpamDriver.RequestAddress) and the neutron subnet has gateway as None, return '0.0.0.0/0' as a placeholder. * Disable default docker gateway via the 'DisableGatewayService' flag (see moby/libnetwork#778). Change-Id: I3033d28eb268a01de8cf038b1ed20110ca9a31ea Closes-Bug: #1881910 (cherry picked from commit 9609936)

aboch reviewed Nov 26, 2015
View reviewed changes

aboch reviewed Nov 30, 2015
View reviewed changes

mavenugo mentioned this pull request Dec 3, 2015

Don't connect sbx to default gw nw if default static route is provided #796

Merged

mavenugo reviewed Dec 3, 2015
View reviewed changes

Add DisableGatewayService in JoinInfo to offering drivers the ability…

b1e94ca

… to disable default gateway Signed-off-by: Chun Chen <ramichen@tencent.com>

chenchun mentioned this pull request Dec 3, 2015

via remote driver creates one network: admin, start a container with network admin, but got two IP, one is from admin, other is from a bridge which is automatically created #797

Closed

sanimej reviewed Dec 3, 2015
View reviewed changes

aboch added a commit that referenced this pull request Dec 3, 2015

Merge pull request #778 from chenchun/disable-default-gateway

f942c0e

Add a network option to disable default gateway

aboch merged commit f942c0e into moby:master Dec 3, 2015

chenchun deleted the disable-default-gateway branch December 4, 2015 01:32

This was referenced Dec 4, 2015

needDefaultGW should not return true for remote drivers #775

Closed

Question on eth1 of container even with other Network Driver specified #781

Closed

aboch mentioned this pull request Dec 17, 2015

Add disable default gateway option for overlay network #812

Closed

Add a network option to disable default gateway #778

Add a network option to disable default gateway #778

Conversation

chenchun commented Nov 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenchun commented Nov 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboch commented Nov 26, 2015

chenchun commented Nov 26, 2015

Choose a reason for hiding this comment

chenchun commented Nov 27, 2015

sanimej commented Nov 27, 2015

aboch commented Nov 27, 2015

aboch commented Nov 27, 2015

dave-tucker commented Nov 28, 2015

jc-m commented Nov 28, 2015

chenchun commented Nov 30, 2015

jc-m commented Nov 30, 2015

Choose a reason for hiding this comment

aboch commented Nov 30, 2015

aboch commented Nov 30, 2015

jc-m commented Nov 30, 2015

aboch commented Nov 30, 2015

jc-m commented Dec 1, 2015

jc-m commented Dec 1, 2015

chenchun commented Dec 1, 2015

sanimej commented Dec 1, 2015

sanimej commented Dec 1, 2015

chenchun commented Dec 1, 2015

sanimej commented Dec 1, 2015

mavenugo commented Dec 2, 2015

mavenugo commented Dec 2, 2015

chenchun commented Dec 3, 2015

Choose a reason for hiding this comment

mavenugo commented Dec 3, 2015

chenchun commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboch commented Dec 3, 2015

aboch commented Dec 3, 2015