CLI does not fill NetworkAttachmentConfig.Target correctly

[aaronlehmann]

The CLI used to put whatever reference to a network the user entered directly into NetworkAttachmentConfig.Target. This field is documented to only contain a network ID, so this was the wrong behavior. It turned out that the daemon was automatically rewriting these names into IDs, which is bad because the daemon shouldn't be modifying the spec supplied by the user. The CLI behavior was fixed as part of moby/moby#32062, and there was a lot of discussion about this in the PR:

moby/moby#32062 (comment)
moby/moby#32062 (comment)
moby/moby#32062 (comment)

The daemon still rewrites names into IDs for backward compatibility, but this code path isn't meant to be used going forward, and ideally should only be enabled for older API versions.

The CLI's behavior appears to have regressed in #62. It's no longer using the ID as it should. The problem is hidden by the magic rewriting in the daemon.

[thaJeztah]

ping @abhinandanpb @mavenugo

[thaJeztah]

ping @abhi can you have a look; is this still an issue?

$ docker service create --network=bridge nginx:alpine
0aajbat10xptfsqvkty27bked

$ docker service inspect --format '{{json .Spec.TaskTemplate.Networks}}' 0aajbat10xptfsqvkty27bked
[{"Target":"i2262bjzwohcd94p2o5ttrbxa"}]


$ docker network create --driver=overlay foo
ncvighu47mg2gqpz9kcyejr8r

$ docker service create --network=foo nginx:alpine
o33rila7nd2zgzbbyeowiky65

$ docker service inspect --format '{{json .Spec.TaskTemplate.Networks}}' o33rila7nd2zgzbbyeowiky65
[{"Target":"ncvighu47mg2gqpz9kcyejr8r"}]

[thaJeztah]

Oh, nevermind; looks like it's still an issue;

time="2017-11-01T12:27:09.196416819Z" level=debug msg="Calling GET /v1.33/networks/foo?scope=swarm"
time="2017-11-01T12:27:09.202046203Z" level=debug msg="Calling GET /v1.33/distribution/nginx:alpine/json"
time="2017-11-01T12:27:11.074130093Z" level=debug msg="Calling POST /v1.33/services/create"
time="2017-11-01T12:27:11.074368573Z" level=debug msg="form data: {\"EndpointSpec\":{\"Mode\":\"vip\"},\"Labels\":{},\"Mode\":{\"Replicated\":{}},\"TaskTemplate\":{\"ContainerSpec\":{\"DNSConfig\":{},\"Image\":\"nginx:alpine@sha256:18f83c1759551c173dba982e5fe6f43d62412888e6f431c9c20b8164661592e3\"},\"ForceUpdate\":0,\"Networks\":[{\"Target\":\"foo\"}],\"Placement\":{\"Platforms\":[{\"Architecture\":\"amd64\",\"OS\":\"linux\"}]},\"Resources\":{\"Limits\":{},\"Reservations\":{}}}}"

JSON request to create the service:

{
  "EndpointSpec": {
    "Mode": "vip"
  },
  "Labels": {},
  "Mode": {
    "Replicated": {}
  },
  "TaskTemplate": {
    "ContainerSpec": {
      "DNSConfig": {},
      "Image": "nginx:alpine@sha256:18f83c1759551c173dba982e5fe6f43d62412888e6f431c9c20b8164661592e3"
    },
    "ForceUpdate": 0,
    "Networks": [
      {
        "Target": "foo"
      }
    ],
    "Placement": {
      "Platforms": [
        {
          "Architecture": "amd64",
          "OS": "linux"
        }
      ]
    },
    "Resources": {
      "Limits": {},
      "Reservations": {}
    }
  }
}

[wmluke]

Hi @robmry, is this still an issue or has it been addressed?

[robmry]

Noting my understanding of the issue, as it took me a while to unpack ...

In moby/moby#32062, at opts.go line 283, in the new CLI code Target is set to the network id instead of whatever name/short-id the user specified.

That meant the daemon didn't need to replace a name/short-id in Target with the full id.

Then #62 changed the CLI back, allowing any name to be sent in Target.

That worked because the daemon still migrated the field anyway for backward compatibility. That migration has never been limited to older API versions.

In the current CLI, whatever name the user gave is still put straight in to Target ...

cli/cli/command/service/opts.go

Line 415 in f81816e

Target: net.Target,

So, the behaviours of CLI and daemon haven't changed since this issue was reported.

Changing the CLI to send an id in Target rather than a name/short-id wouldn't have any visible effect for CLI users. They'd still be able use any valid name/id for the network, and the inspect output would still show the full id rather than that name.

But, to meet the objective of reported config not being mutated by the daemon, the daemon would need to refuse to accept anything apart from a full id in Target ... and that'd potentially be a breaking change for any other API client.

The daemon is guilty of mixing config and running state on the network side. That's caused issues - for example, it was impossible to tell whether a MAC address was specifically configured or assigned by the daemon, making it difficult to re-create containers without ending up with duplicate addresses. But I don't think this one's quite in the same category, the difference between given config and reported state is only in the way the network is identified - and a CLI user sees the mutated name wherever it's mutated.

Given all that - after all this time, a breaking change on the daemon side seems hard to justify in terms of the benefit. I think we should close this issue.