Skip to content
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

Check parameter --ip --ip6 --link-local-ip in docker network connect #30807

Merged
merged 1 commit into from Mar 6, 2017

Conversation

chchliang
Copy link
Contributor

@chchliang chchliang commented Feb 8, 2017

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
image

- 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

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

@chchliang
Copy link
Contributor Author

@aboch @mavenugo
This error messages are time out.

@tiborvass
Copy link
Contributor

I think this makes sense.

for _, ips := range ipam.LinkLocalIPs {
if ip := net.ParseIP(ips); ip != nil {
ipList = append(ipList, ip)
if len(ipam.LinkLocalIPs) > 0 {
Copy link
Contributor

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.

if len(ipam.LinkLocalIPs) > 0 {
for _, ips := range ipam.LinkLocalIPs {
if ip := net.ParseIP(ips); ip != nil {
ipList = append(ipList, ip)
Copy link
Member

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?

}
}
if ipam.IPv4Address != "" {

if ip := net.ParseIP(ipam.IPv4Address); ip == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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))`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aboch Has been modified,thank you!

var ipList []net.IP
var ip, ip6 net.IP
Copy link
Contributor

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

for _, ips := range ipam.LinkLocalIPs {
if ip := net.ParseIP(ips); ip != nil {
ipList = append(ipList, ip)
if linkip := net.ParseIP(ips); linkip == nil && ips != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refactoring code,thank you! @aboch

}

if ip = net.ParseIP(ipam.IPv4Address); ip == nil && ipam.IPv4Address != "" {
return nil, fmt.Errorf("Error parsing parameter ip value(%s)", ipam.IPv4Address)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

var ipList []net.IP
var ip, ip6, linkip net.IP
Copy link
Contributor

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

if ip := net.ParseIP(ips); ip != nil {
ipList = append(ipList, ip)
if linkip = net.ParseIP(ips); linkip == nil && ips != "" {
return nil, fmt.Errorf("Error parsing parameter link-local-ip value(%s):%s", ipam.LinkLocalIPs, ips)
Copy link
Contributor

@aboch aboch Feb 23, 2017

Choose a reason for hiding this comment

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

so here we should probably return

invalid link-local IP address: ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @aboch
I have modified
image

Signed-off-by: chchliang <chen.chuanliang@zte.com.cn>
@aboch
Copy link
Contributor

aboch commented Feb 23, 2017

Looks good to me.

@tiborvass
Copy link
Contributor

LGTM

@vdemeester
Copy link
Member

LGTM 💃

@chchliang chchliang deleted the networkproject branch September 16, 2017 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants