Fix to address regression caused by PR 30897 #36316

selansen · 2018-02-15T04:32:52Z

With the inclusion of PR #30897, creating service for host network fails in 18.02. Modified IsPreDefinedNetwork check and return NetworkNameError instead of errdefs.Forbidden to address this issue

Signed-off-by: selansen [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cpuguy83 · 2018-02-15T14:41:41Z

daemon/network.go

-		return nil, errdefs.Forbidden(err)
+	if runconfig.IsPreDefinedNetwork(create.Name) {
+		logrus.Debugf("%s is a pre-defined network and cannot be created", create.Name)
+		return nil, libnetwork.NetworkNameError(create.Name)


I still think this should be wrapped in a errdefs.Forbidden(err), but callers that need to check the error for NetworkNameError should check against errors.Cause(err)

selansen · 2018-02-15T15:17:35Z

In the same function below we do exact same logic. May I know is there any advantage are we going to get by throwing errdefs.Forbidden instead of NetworkNameError ? I can go ahead and modify but just trying to understand if I am missing anything with your suggestion. Thanks in advance.

…

On Thu, Feb 15, 2018 at 9:42 AM, Brian Goff ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In daemon/network.go <#36316 (comment)>: > @@ -267,9 +267,9 @@ func (daemon *Daemon) CreateNetwork(create types.NetworkCreateRequest) (*types.N } func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string, agent bool) (*types.NetworkCreateResponse, error) { - if runconfig.IsPreDefinedNetwork(create.Name) && !agent { - err := fmt.Errorf("%s is a pre-defined network and cannot be created", create.Name) - return nil, errdefs.Forbidden(err) + if runconfig.IsPreDefinedNetwork(create.Name) { + logrus.Debugf("%s is a pre-defined network and cannot be created", create.Name) + return nil, libnetwork.NetworkNameError(create.Name) I still think this should be wrapped in a errdefs.Forbidden(err), but callers that need to check the error for NetworkNameError should check against errors.Cause(err) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#36316 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKn92DPPcEFbVntH57NweWvS5KcrvTks5tVEJqgaJpZM4SGU62> .

cpuguy83 · 2018-02-15T15:23:50Z

The error wrapping in this case is so that we return the correct error code to the API.

cpuguy83 · 2018-02-15T15:24:08Z

04:57:16 FAIL: docker_api_network_test.go:241: DockerSuite.TestAPICreateDeletePredefinedNetworks
04:57:16 
04:57:16 docker_api_network_test.go:243:
04:57:16     createDeletePredefinedNetwork(c, "bridge")
04:57:16 docker_api_network_test.go:323:
04:57:16     c.Assert(resp.StatusCode, checker.Equals, expectedStatusCode)
04:57:16 ... obtained int = 409
04:57:16 ... expected int = 403

selansen · 2018-02-15T15:44:00Z

That make sense. will change it as per your suggestion and let the caller handle error case according to their requirement. Thanks.

…

On Thu, Feb 15, 2018 at 10:25 AM, Brian Goff ***@***.***> wrote: 04:57:16 FAIL: docker_api_network_test.go:241: DockerSuite.TestAPICreateDeletePredefinedNetworks 04:57:16 04:57:16 docker_api_network_test.go:243: 04:57:16 createDeletePredefinedNetwork(c, "bridge") 04:57:16 docker_api_network_test.go:323: 04:57:16 c.Assert(resp.StatusCode, checker.Equals, expectedStatusCode) 04:57:16 ... obtained int = 409 04:57:16 ... expected int = 403 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#36316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKnwDqCQfhNbd85pBw7MGJPXSpmPCmks5tVEx4gaJpZM4SGU62> .

thaJeztah · 2018-02-15T16:19:07Z

@selansen this fixes #36247 correct?

selansen · 2018-02-15T16:25:36Z

You mean 36247 ??? Yes.

…

On Thu, Feb 15, 2018 at 11:20 AM, Sebastiaan van Stijn < ***@***.***> wrote: @selansen <https://github.com/selansen> this fixes #36316 <#36316> correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKn-y8jexGn8FMZKLnex9w8Y_R96Dnks5tVFldgaJpZM4SGU62> .

thaJeztah · 2018-02-15T16:27:10Z

errr, copy/pasta error, yes, meant #36247

selansen · 2018-02-15T16:31:22Z

Yes. But looks like fix is causing existing test case failure. Let me dig deeper while addressing cpuguy83 suggestion.

…

On Thu, Feb 15, 2018 at 11:28 AM, Sebastiaan van Stijn < ***@***.***> wrote: errr, copy/pasta error, yes, meant #36247 <#36247> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKnzzJJa-xgWXCu5EV94sFg7Rb456Tks5tVFtEgaJpZM4SGU62> .

ddebroy · 2018-02-15T17:22:35Z

We also need a test to make sure similar regression does not happen. Maybe add a test to make sure attaching to host and other "predefined" networks work fine with both swarm services and regular containers as well as a unit test to make sure the "predefined" networks are properly rejected.

thaJeztah · 2018-02-17T12:22:24Z

daemon/cluster/executor/container/adapter.go

@@ -153,6 +154,11 @@ func (c *containerAdapter) createNetworks(ctx context.Context) error {
 			if _, ok := err.(libnetwork.NetworkNameError); ok {
 				continue
 			}
+			// We will continue if CreateManagedNetwork returns forbidden error.
+			// Other callers still can treat it as Error.
+			if errdefs.IsForbidden(err) {


I’ll have to give this more thinking (haven’t looked at what other situations return), but isn’t the case where we try to create a pre-defined network a conflict (i.e. the network already “exists”).

Forbidden sounds like “you don’t have access to this endpoint” (but you do). Other options would be “invalid argument” (passed value is not valid)

Just dumping my thinking here, so that I don’t forget 😄

Conflict would imply that if the state changed then it would be possible to perform the action. In this case it is not allowed.

Predefined networks are created when Daemon starts. When user creates service with predefined network, we know that network already exists and return saying you can't create. But service creation still can go ahead as the predefined network exists already. so we can ignore this error and continue in this scenario.

Conflict would imply that if the state changed then it would be possible to perform the action. In this case it is not allowed.

Right, yes. I was looking at the 403 because I wondered if it could cause issues in situations where a 403 could also occur due to authentication issues to the API. A generic 400 (bad request - the name is reserved/invalid) could've been an option for that.

Looking at NetworkNameError, I see that is also a "Forbidden" error; should we be more specific in this case, and check if it's actually a "forbidden" error, because it's a predefined network?

Given that we already walk different paths inside createNetwork() if the caller is agent, we could also use the agent option to just return a nil instead;

if runconfig.IsPreDefinedNetwork(create.Name) { if agent { return nil, nil } err := fmt.Errorf("%s is a pre-defined network and cannot be created", create.Name) return nil, errdefs.Forbidden(err) }

selansen · 2018-02-21T16:47:44Z

I still believe instead of returning nil in case of agent is true, we let the caller decides the error information. Because setupIngress() also calls createNetwork with agent=true option.

…

On Wed, Feb 21, 2018 at 10:38 AM, Sebastiaan van Stijn < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In daemon/cluster/executor/container/adapter.go <#36316 (comment)>: > @@ -153,6 +154,11 @@ func (c *containerAdapter) createNetworks(ctx context.Context) error { if _, ok := err.(libnetwork.NetworkNameError); ok { continue } + // We will continue if CreateManagedNetwork returns forbidden error. + // Other callers still can treat it as Error. + if errdefs.IsForbidden(err) { Conflict would imply that if the state changed then it would be possible to perform the action. In this case it is not allowed. Right, yes. I was looking at the 403 because I wondered if it could cause issues in situations where a 403 could also occur due to authentication issues to the API. A generic 400 (bad request - the name is reserved/invalid) could've been an option for that. Looking at NetworkNameError <https://github.com/docker/libnetwork/blob/cb09a3770a365d8a4ed54f66e284041daf09ddef/error.go#L101-L108>, I see that is also a "Forbidden" error; should we be more specific in this case, and check if it's actually a "forbidden" error, because it's a predefined network? Given that we already walk different paths inside createNetwork() if the caller is agent, we could also use the agent option to just return a nil instead; if runconfig.IsPreDefinedNetwork(create.Name) { if agent { return nil, nil } err := fmt.Errorf("%s is a pre-defined network and cannot be created", create.Name) return nil, errdefs.Forbidden(err) } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKn1dgfgbWifcKU0D_SJ5coI3v5QX4ks5tXDhrgaJpZM4SGU62> .

thaJeztah · 2018-02-21T17:07:31Z

I still believe instead of returning nil in case of agent is true, we let the caller decides the error information. Because setupIngress() also calls createNetwork with agent=true option.

Yeah not a big fan of the agent boolean (it's changing the behaviour of that function; perhaps that needs a refactor).

If we keep the check in the other location, should we be more specific though (as I mentioned above)?

selansen · 2018-02-21T22:13:49Z

@thaJeztah , I can add new Forbidden Error type say "PredefinedNetworkExistError" and use it in all the places accordingly. Would that be ok with you?

thaJeztah · 2018-02-26T14:58:10Z

@selansen yes, if you can add an error so that we can distinguish if the error was due to "predefined network" and not some other "forbidden" error, that would be good.

ddebroy · 2018-02-26T21:50:15Z

daemon/network.go

@@ -267,7 +267,7 @@ func (daemon *Daemon) CreateNetwork(create types.NetworkCreateRequest) (*types.N
 }

 func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string, agent bool) (*types.NetworkCreateResponse, error) {
-	if runconfig.IsPreDefinedNetwork(create.Name) && !agent {
+	if runconfig.IsPreDefinedNetwork(create.Name) {


Sorry for this late comment. Do we really need the IsPreDefinedNetwork logic here? I was discussing with Flavio a bit and it seems the logic around daemon.GetNetworkByName right below should be able to reject all PreDefined networks (besides user defined networks). This way, we will not even need IsPreDefinedNetwork above.

Just discussed a bit and we do need separate code paths for Pre-Defined vs User-defined as we do allow name conflicts for the latter category.

Thats is what I h added originally. I was reluctant to change. But we need to be able to differentiate regular network name exist and predefined network exist . Review comments were asking me to be more specific to error reports to the callers. Hence I added new type .

codecov · 2018-02-28T07:44:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3e1505e). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #36316   +/-   ##
=========================================
  Coverage          ?   34.89%           
=========================================
  Files             ?      613           
  Lines             ?    45413           
  Branches          ?        0           
=========================================
  Hits              ?    15847           
  Misses            ?    27465           
  Partials          ?     2101

selansen · 2018-03-02T20:30:48Z

@thaJeztah CI tests results looks good. only one test is failing . That is failing due to below test case. I looked into it. Its not related to above changes.

18:25:05 FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest
18:25:05
18:25:05 unmount of /tmp/docker-execroot/d9cc520883592/netns failed: invalid argument
18:25:05 unmount of /tmp/docker-execroot/d9cc520883592/netns failed: no such file or directory
18:25:05 check_test.go:371:
18:25:05 d.Stop(c)
18:25:05 daemon/daemon.go:389:
18:25:05 t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
18:25:05 ... Error: Error while stopping the daemon d58c8df026d8e : exit status 130
18:25:05

thaJeztah

one nit/question, but otherwise looks good

thaJeztah · 2018-03-05T13:55:05Z

daemon/network.go

@@ -528,6 +537,7 @@ func (daemon *Daemon) deleteNetwork(nw libnetwork.Network, dynamic bool) error {
 	if runconfig.IsPreDefinedNetwork(nw.Name()) && !dynamic {
 		err := fmt.Errorf("%s is a pre-defined network and cannot be removed", nw.Name())
 		return errdefs.Forbidden(err)
+		//return PredefinedNetworkError(nw.Name())


Can you remove this? (or should this be a PredefinedNetworkError?)

Let me remove it. I was testing it . but forgot to remove it.

thaJeztah

LGTM

thaJeztah · 2018-03-05T23:35:03Z

@selansen looks like there's a golint failure:

23:16:50 integration/network/service_test.go:67:10:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
23:16:50 Build step 'Execute shell' marked build as failure

thaJeztah · 2018-03-05T23:36:38Z

integration/network/service_test.go

+			return poll.Error(err)
+		}
+
+		if len(services) != int(instances) {


To make golint happy, change this to

if len(services) != int(instances) { return poll.Continue("Service count at %d waiting for %d", len(services), instances) } return poll.Success()

Already fixed golint issue and all test cases passed now.

With the inclusion of PR 30897, creating service for host network fails in 18.02. Modified IsPreDefinedNetwork check and return NetworkNameError instead of errdefs.Forbidden to address this issue Signed-off-by: selansen <[email protected]>

vdemeester

LGTM 🐯

dnephin · 2018-03-06T16:46:03Z

integration/network/service_test.go

+func serviceRunningCount(client client.ServiceAPIClient, serviceID string, instances uint64) func(log poll.LogT) poll.Result {
+	return func(log poll.LogT) poll.Result {
+		filter := filters.NewArgs()
+		filter.Add("service", serviceID)


This filter is not used. Was it supposed to be set in ServiceListOptions ?

You can build these inline now, which would prevent such an issue:

filters.NewArgs(filter.Arg("service", serviceID))

GordonTheTurtle added the status/0-triage label Feb 15, 2018

cpuguy83 reviewed Feb 15, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Feb 15, 2018

vdemeester requested review from vdemeester, dnephin, cpuguy83 and thaJeztah February 15, 2018 14:46

thaJeztah mentioned this pull request Feb 15, 2018

[18.02.0] Swarm service cannot attach to host network - "only one instance of "host" network is allowed" #36247

Closed

selansen force-pushed the 36247 branch from d37c022 to d45edb7 Compare February 16, 2018 13:55

thaJeztah reviewed Feb 17, 2018

View reviewed changes

thaJeztah added the area/networking label Feb 26, 2018

ddebroy reviewed Feb 26, 2018

View reviewed changes

thaJeztah mentioned this pull request Feb 28, 2018

Adding new Forbidden Error type moby/libnetwork#2091

Closed

selansen force-pushed the 36247 branch from d45edb7 to bae8139 Compare March 1, 2018 21:38

GordonTheTurtle assigned crosbymichael Mar 2, 2018

selansen force-pushed the 36247 branch from bae8139 to e4046db Compare March 2, 2018 16:45

vdemeester added the rebuild/janky label Mar 5, 2018

GordonTheTurtle removed the rebuild/janky label Mar 5, 2018

thaJeztah requested changes Mar 5, 2018

View reviewed changes

selansen force-pushed the 36247 branch from e4046db to 3fa1c48 Compare March 5, 2018 17:42

thaJeztah approved these changes Mar 5, 2018

View reviewed changes

selansen force-pushed the 36247 branch from 3fa1c48 to 0e85f9d Compare March 5, 2018 23:13

thaJeztah reviewed Mar 5, 2018

View reviewed changes

Fix to address regression caused by PR 30897

7cf8b20

With the inclusion of PR 30897, creating service for host network fails in 18.02. Modified IsPreDefinedNetwork check and return NetworkNameError instead of errdefs.Forbidden to address this issue Signed-off-by: selansen <[email protected]>

selansen force-pushed the 36247 branch from 0e85f9d to 7cf8b20 Compare March 6, 2018 00:11

vdemeester approved these changes Mar 6, 2018

View reviewed changes

thaJeztah merged commit 0b0af85 into moby:master Mar 6, 2018

This was referenced Mar 6, 2018

[18.03] Fix to address regression caused by PR 30897 docker-archive/docker-ce#452

Merged

Remove duplicate TestServiceUpdatePort #36502

Merged

dnephin reviewed Mar 6, 2018

View reviewed changes

thaJeztah mentioned this pull request Mar 9, 2018

Flaky test TestServiceWithPredefinedNetwork #36547

Closed

vdemeester mentioned this pull request Mar 19, 2018

Docker stack deploy with network host fails yaml validation on docker for windows docker/cli#952

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to address regression caused by PR 30897 #36316

Fix to address regression caused by PR 30897 #36316

selansen commented Feb 15, 2018 •

edited by thaJeztah

Loading

cpuguy83 Feb 15, 2018

selansen commented Feb 15, 2018 via email

cpuguy83 commented Feb 15, 2018

cpuguy83 commented Feb 15, 2018

selansen commented Feb 15, 2018 via email

thaJeztah commented Feb 15, 2018 •

edited

Loading

selansen commented Feb 15, 2018 via email

thaJeztah commented Feb 15, 2018

selansen commented Feb 15, 2018 via email

ddebroy commented Feb 15, 2018

thaJeztah Feb 17, 2018

cpuguy83 Feb 17, 2018

selansen Feb 17, 2018

thaJeztah Feb 21, 2018

selansen commented Feb 21, 2018 via email

thaJeztah commented Feb 21, 2018

selansen commented Feb 21, 2018

thaJeztah commented Feb 26, 2018

ddebroy Feb 26, 2018 •

edited

Loading

ddebroy Feb 26, 2018

selansen Feb 26, 2018

codecov bot commented Feb 28, 2018 •

edited

Loading

selansen commented Mar 2, 2018

thaJeztah left a comment

thaJeztah Mar 5, 2018

selansen Mar 5, 2018

thaJeztah left a comment

thaJeztah commented Mar 5, 2018

thaJeztah Mar 5, 2018

selansen Mar 6, 2018 •

edited

Loading

vdemeester left a comment

dnephin Mar 6, 2018

Fix to address regression caused by PR 30897 #36316

Fix to address regression caused by PR 30897 #36316

Conversation

selansen commented Feb 15, 2018 • edited by thaJeztah Loading

Choose a reason for hiding this comment

selansen commented Feb 15, 2018 via email

cpuguy83 commented Feb 15, 2018

cpuguy83 commented Feb 15, 2018

selansen commented Feb 15, 2018 via email

thaJeztah commented Feb 15, 2018 • edited Loading

selansen commented Feb 15, 2018 via email

thaJeztah commented Feb 15, 2018

selansen commented Feb 15, 2018 via email

ddebroy commented Feb 15, 2018

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

selansen commented Feb 21, 2018 via email

thaJeztah commented Feb 21, 2018

selansen commented Feb 21, 2018

thaJeztah commented Feb 26, 2018

ddebroy Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2018 • edited Loading

Codecov Report

selansen commented Mar 2, 2018

@thaJeztah CI tests results looks good. only one test is failing . That is failing due to below test case. I looked into it. Its not related to above changes.

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Mar 5, 2018

Choose a reason for hiding this comment

selansen Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selansen commented Feb 15, 2018 •

edited by thaJeztah

Loading

thaJeztah commented Feb 15, 2018 •

edited

Loading

ddebroy Feb 26, 2018 •

edited

Loading

codecov bot commented Feb 28, 2018 •

edited

Loading

selansen Mar 6, 2018 •

edited

Loading