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

net: why not check valid when parse CIDR #30834

Closed
daixiang0 opened this issue Mar 14, 2019 · 8 comments

Comments

Projects
None yet
3 participants
@daixiang0
Copy link

commented Mar 14, 2019

What version of Go are you using (go version)?

$ go version
go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

I setup an invalid CIDR address by mistake in k8s CNI then all pods in different nodes can not communicate each other, then i find code never check a CIDR address is valid or not.

Based on example in https://golang.org/pkg/net/#ParseCIDR, test some invalid CIDR address like 10.97.0.0/12, 192.167.0.0/12, all can work. They are invalid based on my knowledge, aren't they?

What did you expect to see?

Check validation when parse.

@daixiang0

This comment has been minimized.

Copy link
Author

commented Mar 14, 2019

@mikioh mikioh changed the title why not check valid when parse CIDR net: why not check valid when parse CIDR Mar 14, 2019

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

They are invalid based on my knowledge, aren't they?

Please make sure what's valid and what's invalid from what point of view.

@daixiang0

This comment has been minimized.

Copy link
Author

commented Mar 14, 2019

take 10.97.0.0/12 as example:
CIDR bases on dotted decimal, 12 mean it uses 12 bits, the first num uses whole 8 bits and the second num should use first 4 bit of whole 8 bits, 97 is invalid since it is 01100001 while 96 is valid since it is 01100000.

there is a check way using bash: https://stackoverflow.com/questions/50084229/bash-check-if-cidr-address-is-valid.

For some case, the invalidation make network not communicate.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

CIDR bases on dotted decimal

Oh, please don't forget Pv6 addresses.

12 mean it uses 12 bits

It's the length of the address prefix, no more no less.

10.97.0.0/12

We usually think that "okay, the attached address is 10.97.0.0/32, the address prefix is 10.96.0.0/12; presumably, the subnet address is 10.96.0.0/32 and the directed broadcast address is 10.111.255.255/32; what a big subnet! which RIR (Regional Internet Registry) allocates this for which organization?"

@daixiang0

This comment has been minimized.

Copy link
Author

commented Mar 14, 2019

take k8s network as example:
kubeadm and CNI like calico use CIDR to setup cluster network, specify 10.96.0.0/12 as pod ip range is common. If specify 10.97.0.0/12, network not works as expected.

@daixiang0

This comment has been minimized.

Copy link
Author

commented Mar 14, 2019

CIDR format as ip/mask, now ParseCIDR() only check the validation of ip part not mask, so if use invalid CIDR, in this range, must have many subnets can not communicate.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

kubeadm and and CNI like ... network not works as expected.

Sounds like that is not an issue of ParseCIDR. If your issue is that ParseCIDR accepts a valid CIDR notation and the behavior is not what you want, changing the behavior of ParseCIDR for your application is not an ideal solution (moreover the change will break Kubernetes and Docker).

Instead, you may use ParseCIDR more appropriately like https://play.golang.org/p/z3cYDbR21ws Closing.

@mikioh mikioh closed this Mar 14, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Mar 25, 2019

Change https://golang.org/cl/168860 mentions this issue: net: update examples for ParseCIDR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.