Check parameter --ip --ip6 --link-local-ip in `docker network connect` by chchliang · Pull Request #30807 · moby/moby

chchliang · 2017-02-08T02:45:24Z

Signed-off-by: chchliang chen.chuanliang@zte.com.cn

- What I did
Execute docker network connect --ip=aa network1 xxx but successful,i think ip address invalid,should be failure. like --ip6 --link-local-ip

- How I did it
To add check code in container/container.go

if epConfig != nil {
ipam := epConfig.IPAMConfig

	if ipam != nil {
		var (
			ipList          []net.IP
			ip, ip6, linkip net.IP
		)

		for _, ips := range ipam.LinkLocalIPs {
			if linkip = net.ParseIP(ips); linkip == nil && ips != "" {
				return nil, fmt.Errorf("Invalid link-local IP address:%s", ipam.LinkLocalIPs)
			}
			ipList = append(ipList, linkip)

		}

		if ip = net.ParseIP(ipam.IPv4Address); ip == nil && ipam.IPv4Address != "" {
			return nil, fmt.Errorf("Invalid IPv4 address:%s)", ipam.IPv4Address)
		}

		if ip6 = net.ParseIP(ipam.IPv6Address); ip6 == nil && ipam.IPv6Address != "" {
			return nil, fmt.Errorf("Invalid IPv6 address:%s)", ipam.IPv6Address)
		}

		createOptions = append(createOptions,
			libnetwork.CreateOptionIpam(ip, ip6, ipList, nil))

	}

	for _, alias := range epConfig.Aliases {
		createOptions = append(createOptions, libnetwork.CreateOptionMyAlias(alias))
	}
}

*- How to verify it
docker network connect --ip=aa network1 59bb1359f671
Error response from daemon: Invalid IPv4 address:aa

vdemeester

/cc @aboch @mavenugo

chchliang · 2017-02-15T00:41:59Z

@aboch @mavenugo
This error messages are time out.

tiborvass · 2017-02-22T19:49:24Z

I think this makes sense.

aboch · 2017-02-22T20:03:49Z

container/container.go

You do not need to check the length of the list.

thaJeztah · 2017-02-22T20:06:46Z

container/container.go

perhaps continue here, instead of else?

aboch · 2017-02-22T20:07:44Z

container/container.go

given we are parsing again later for

libnetwork.CreateOptionIpam(net.ParseIP(ipam.IPv4Address), net.ParseIP(ipam.IPv6Address), ipList, nil))

I think we should define var ip, ip6 net.IP

and change these to

if ip6 = net.ParseIP(ipam.IPv6Address); ip == nil && ipam.IPv6Address != "" { return nil, fmt.Errorf("Error parsing parameter ip value(%s)", ipam.IPv6Address) }

and later change to

`libnetwork.CreateOptionIpam(ip, ip6, ipList, nil))`

@aboch Has been modified,thank you!

aboch · 2017-02-23T02:02:08Z

container/container.go

Please create a var block

aboch · 2017-02-23T02:04:24Z

container/container.go

if you also add a llIP to the above var block, you can improve readability here dropping the else block

if llIP = net.ParseIP(ips); linkip == nil && ips != "" { return nil, fmt.Errorf("Error....) } ipList = append(ipList, llIP) }

I have refactoring code,thank you! @aboch

aboch · 2017-02-23T05:18:36Z

container/container.go

It would be nice to conform these errors to the errors docker already returns for invalid IP on docker run
As an example, I see
invalid IPv4 address: 3.3.3.333
invalid IPv6 address: 2001:db8:12345678::1

aboch · 2017-02-23T05:18:41Z

container/container.go

Thanks @chchliang
I was also suggesting to define all these vars in a

var ( )

block

aboch · 2017-02-23T05:19:23Z

container/container.go

so here we should probably return

invalid link-local IP address: ....

Thanks @aboch
I have modified

Signed-off-by: chchliang <chen.chuanliang@zte.com.cn>

aboch · 2017-02-23T06:57:24Z

Looks good to me.

tiborvass · 2017-03-06T20:05:04Z

LGTM

vdemeester · 2017-03-06T20:10:38Z

LGTM 💃

GordonTheTurtle added the status/0-triage label Feb 8, 2017

chchliang force-pushed the networkproject branch from e8f8072 to 5a3fd12 Compare February 8, 2017 06:25

vdemeester added status/1-design-review and removed status/0-triage labels Feb 8, 2017

vdemeester reviewed Feb 8, 2017

View reviewed changes

GordonTheTurtle assigned tiborvass Feb 22, 2017

aboch reviewed Feb 22, 2017

View reviewed changes

container/container.go Outdated

Copy link

Contributor

aboch Feb 22, 2017

Choose a reason for hiding this comment

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

You do not need to check the length of the list.

thaJeztah reviewed Feb 22, 2017

View reviewed changes

container/container.go Outdated

Copy link

Member

thaJeztah Feb 22, 2017

Choose a reason for hiding this comment

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

perhaps continue here, instead of else?

aboch reviewed Feb 22, 2017

View reviewed changes

tiborvass added status/2-code-review and removed status/1-design-review labels Feb 22, 2017

chchliang force-pushed the networkproject branch 2 times, most recently from e780226 to d69cb1e Compare February 23, 2017 01:58

aboch reviewed Feb 23, 2017

View reviewed changes

container/container.go Outdated

Copy link

Contributor

aboch Feb 23, 2017

Choose a reason for hiding this comment

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

Please create a var block

aboch reviewed Feb 23, 2017

View reviewed changes

chchliang force-pushed the networkproject branch from d69cb1e to 40a4457 Compare February 23, 2017 02:15

aboch reviewed Feb 23, 2017

View reviewed changes

container/container.go Outdated

Copy link

Contributor

aboch Feb 23, 2017

Choose a reason for hiding this comment

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

Thanks @chchliang
I was also suggesting to define all these vars in a

var ( )

block

aboch reviewed Feb 23, 2017

View reviewed changes

Check parameter --ip --ip6 --link-local-ip in

b7ce6ce

Signed-off-by: chchliang <chen.chuanliang@zte.com.cn>

chchliang force-pushed the networkproject branch from 40a4457 to b7ce6ce Compare February 23, 2017 06:18

vdemeester merged commit de5ca68 into moby:master Mar 6, 2017

GordonTheTurtle added this to the 17.04.0 milestone Mar 6, 2017

thaJeztah added the impact/changelog label Mar 6, 2017

yosifkit mentioned this pull request May 2, 2017

Why is ipv6 disabled in boot2docker 17.04? boot2docker/boot2docker#1257

Closed

chchliang deleted the networkproject branch September 16, 2017 02:24

Conversation

chchliang commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

chchliang commented Feb 15, 2017

Uh oh!

tiborvass commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch commented Feb 23, 2017

Uh oh!

tiborvass commented Mar 6, 2017

Uh oh!

vdemeester commented Mar 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chchliang commented Feb 8, 2017 •

edited

Loading

aboch Feb 23, 2017 •

edited

Loading