New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix network name masking network ID on delete #34509

Merged
merged 1 commit into from Oct 13, 2017

Conversation

@thaJeztah
Member

thaJeztah commented Aug 14, 2017

fixes #32689

If a network is created with a name that matches another network's ID, the network with that name was masking the other network's ID.

As a result, it was not possible to remove the network with a given ID.

This patch changes the order in which networks are matched to be what we use for other cases;

  1. Match on full ID
  2. Match on full Name
  3. Match on Partial ID

Before this patch:

$ docker network create foo
336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b

$ docker network create 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b
4a698333f1197f20224583abce14876d7f25fdfe416a8545927006c315915a2a

$ docker network ls
NETWORK ID          NAME                                                               DRIVER              SCOPE
4a698333f119        336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b   bridge              local
d1e40d43a2c0        bridge                                                             bridge              local
336717eac9ea        foo                                                                bridge              local
13cf280a1bbf        host                                                               host                local
d9e4c03728a0        none                                                               null                local

$ docker network rm 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b
4a698333f1197f20224583abce14876d7f25fdfe416a8545927006c315915a2a

$ docker network ls
NETWORK ID          NAME                DRIVER              SCOPE
d1e40d43a2c0        bridge              bridge              local
336717eac9ea        foo                 bridge              local
13cf280a1bbf        host                host                local
d9e4c03728a0        none                null                local

After this patch:

$ docker network create foo
2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835

$ docker network create 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835
6cbc749a529cd2d9d3b10566c84e56c4203dd88b67417437b5fc7a6e955dd48f

$ docker network ls
NETWORK ID          NAME                                                               DRIVER              SCOPE
6cbc749a529c        2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835   bridge              local
166c943dbeb5        bridge                                                             bridge              local
2d1791a7def4        foo                                                                bridge              local
6c45b8aa6d8e        host                                                               host                local
b11c96b51ea7        none                                                               null                local

$ docker network rm 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835
2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835

$ docker network ls
NETWORK ID          NAME                                                               DRIVER              SCOPE
6cbc749a529c        2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835   bridge              local
166c943dbeb5        bridge                                                             bridge              local
6c45b8aa6d8e        host                                                               host                local
b11c96b51ea7        none                                                               null                local

Signed-off-by: Sebastiaan van Stijn github@gone.nl

- What I did

- How I did it

- How to verify it

See steps above

- Description for the changelog

Fix network names being able to mask another network's ID

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

corn snake baby - imgur

Picture: Corn snake baby by isoldael

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Aug 14, 2017

func (daemon *Daemon) GetNetworkByID(id string) (libnetwork.Network, error) {
c := daemon.netController
if c == nil {
return nil, libnetwork.ErrNoSuchNetwork(id)

This comment has been minimized.

@fcrisciani

fcrisciani Aug 14, 2017

Contributor

not sure if this is a possible case, but maybe should we return something more appropriate than NoSuchNetwork?

@fcrisciani

fcrisciani Aug 14, 2017

Contributor

not sure if this is a possible case, but maybe should we return something more appropriate than NoSuchNetwork?

This comment has been minimized.

@thaJeztah

thaJeztah Aug 14, 2017

Member

I kept what was already there; other than on Windows, would there be another situation where daemon.netController isn't there?

@thaJeztah

thaJeztah Aug 14, 2017

Member

I kept what was already there; other than on Windows, would there be another situation where daemon.netController isn't there?

This comment has been minimized.

@thaJeztah

thaJeztah Aug 14, 2017

Member

Oh, sorry, no this one wasn't there. Hm, I think I took it from somewhere, let me have a look

@thaJeztah

thaJeztah Aug 14, 2017

Member

Oh, sorry, no this one wasn't there. Hm, I think I took it from somewhere, let me have a look

This comment has been minimized.

@thaJeztah

thaJeztah Aug 14, 2017

Member

Ah, yes, took it from GetNetworkByName below

@thaJeztah

thaJeztah Aug 14, 2017

Member

Ah, yes, took it from GetNetworkByName below

This comment has been minimized.

@fcrisciani

fcrisciani Aug 15, 2017

Contributor

I see why you have to pass it as ErrNoSuchNetwork, so that it goes to the next check. you said on windows is nil the controller, any idea then how it works the GetNetwork there?

@fcrisciani

fcrisciani Aug 15, 2017

Contributor

I see why you have to pass it as ErrNoSuchNetwork, so that it goes to the next check. you said on windows is nil the controller, any idea then how it works the GetNetwork there?

This comment has been minimized.

@thaJeztah

thaJeztah Aug 15, 2017

Member

Looks like in that situation the network endpoints are not created;

moby/cmd/dockerd/daemon.go

Lines 524 to 526 in cbbc283

if opts.daemon.NetworkControllerEnabled() {
routers = append(routers, network.NewRouter(opts.daemon, opts.cluster))
}

@thaJeztah

thaJeztah Aug 15, 2017

Member

Looks like in that situation the network endpoints are not created;

moby/cmd/dockerd/daemon.go

Lines 524 to 526 in cbbc283

if opts.daemon.NetworkControllerEnabled() {
routers = append(routers, network.NewRouter(opts.daemon, opts.cluster))
}

This comment has been minimized.

@fcrisciani

fcrisciani Aug 15, 2017

Contributor

ok so basically there is no way to call this method

@fcrisciani

fcrisciani Aug 15, 2017

Contributor

ok so basically there is no way to call this method

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Aug 15, 2017

Contributor

LGTM

Contributor

fcrisciani commented Aug 15, 2017

LGTM

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Aug 16, 2017

Contributor

@thaJeztah I think you need to rebase

Contributor

boaz1337 commented Aug 16, 2017

@thaJeztah I think you need to rebase

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 16, 2017

Member

Thanks for the ping! I'll update when I get back home later tonight

Member

thaJeztah commented Aug 16, 2017

Thanks for the ping! I'll update when I get back home later tonight

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 17, 2017

Member

rebased

Member

thaJeztah commented Aug 17, 2017

rebased

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Aug 17, 2017

Contributor

LGTM

Contributor

fcrisciani commented Aug 17, 2017

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 17, 2017

Member

ping @vdemeester PTAL 😇

Member

thaJeztah commented Aug 17, 2017

ping @vdemeester PTAL 😇

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 9, 2017

Member

ping @dnephin updated the test to use the API, PTAL

Member

thaJeztah commented Oct 9, 2017

ping @dnephin updated the test to use the API, PTAL

Show outdated Hide outdated daemon/network.go Outdated
Show outdated Hide outdated daemon/network.go Outdated
Show outdated Hide outdated integration/network/delete_test.go Outdated
Show outdated Hide outdated integration/network/delete_test.go Outdated
Show outdated Hide outdated integration/network/delete_test.go Outdated
Fix network name masking network ID on delete
If a network is created with a name that matches another
network's ID, the network with that name was masking the
other network's ID.

As a result, it was not possible to remove the network
with a given ID.

This patch changes the order in which networks are
matched to be what we use for other cases;

1. Match on full ID
2. Match on full Name
3. Match on Partial ID

Before this patch:

    $ docker network create foo
    336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b

    $ docker network create 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b
    4a698333f1197f20224583abce14876d7f25fdfe416a8545927006c315915a2a

    $ docker network ls
    NETWORK ID          NAME                                                               DRIVER              SCOPE
    4a698333f119        336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b   bridge              local
    d1e40d43a2c0        bridge                                                             bridge              local
    336717eac9ea        foo                                                                bridge              local
    13cf280a1bbf        host                                                               host                local
    d9e4c03728a0        none                                                               null                local

    $ docker network rm 336717eac9eaa3da6557042a04efc803f7e8862ce6cf96f6b9565265ba5c618b
    4a698333f1197f20224583abce14876d7f25fdfe416a8545927006c315915a2a

    $ docker network ls
    NETWORK ID          NAME                DRIVER              SCOPE
    d1e40d43a2c0        bridge              bridge              local
    336717eac9ea        foo                 bridge              local
    13cf280a1bbf        host                host                local
    d9e4c03728a0        none                null                local

After this patch:

    $ docker network create foo
    2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835

    $ docker network create 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835
    6cbc749a529cd2d9d3b10566c84e56c4203dd88b67417437b5fc7a6e955dd48f

    $ docker network ls
    NETWORK ID          NAME                                                               DRIVER              SCOPE
    6cbc749a529c        2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835   bridge              local
    166c943dbeb5        bridge                                                             bridge              local
    2d1791a7def4        foo                                                                bridge              local
    6c45b8aa6d8e        host                                                               host                local
    b11c96b51ea7        none                                                               null                local

    $ docker network rm 2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835
    2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835

    $ docker network ls
    NETWORK ID          NAME                                                               DRIVER              SCOPE
    6cbc749a529c        2d1791a7def4e2a1ef0f6b83c6add333df0bb4ced2f196c584cb64e6bd94b835   bridge              local
    166c943dbeb5        bridge                                                             bridge              local
    6c45b8aa6d8e        host                                                               host                local
    b11c96b51ea7        none                                                               null                local

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 11, 2017

Member

ping @dnephin addressed your comments 😅

Member

thaJeztah commented Oct 11, 2017

ping @dnephin addressed your comments 😅

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 11, 2017

Member

hmm, odd one on z https://jenkins.dockerproject.org/job/Docker-PRs-s390x/6235/console;

20:13:09 === RUN   TestDockerNetworkDeletePreferID
20:13:40 --- FAIL: TestDockerNetworkDeletePreferID (31.03s)
20:13:40 	protect.go:127: Failed to load frozen images: ApplyLayer exit status 1 stdout:  stderr: unexpected EOF
Member

thaJeztah commented Oct 11, 2017

hmm, odd one on z https://jenkins.dockerproject.org/job/Docker-PRs-s390x/6235/console;

20:13:09 === RUN   TestDockerNetworkDeletePreferID
20:13:40 --- FAIL: TestDockerNetworkDeletePreferID (31.03s)
20:13:40 	protect.go:127: Failed to load frozen images: ApplyLayer exit status 1 stdout:  stderr: unexpected EOF
@dnephin

LGTM

@vdemeester

LGTM 👼

@yongtang yongtang merged commit 0181eb8 into moby:master Oct 13, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37363 has succeeded
Details
janky Jenkins build Docker-PRs 46042 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6431 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17655 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6240 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:fix-network-delete branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment