read ranges from env v2 #1
Conversation
|
I asusme @pslldq is the right person to have a look at this. For better collaboration this project should probably be moved to the ffbs-github namespace? |
grische
force-pushed
the
read-ranges-from-env
branch
from
5da6579
to
17368fc
Compare
|
@SmithChart @pslldq I noticed that the original code is not able to handle othe sizes but exactly I hope the new code is also a bit more readable and has less binary operations in comparison to v1. |
| const CLIENT_V4_RANGE_SIZE uint = 22 | ||
| const CLIENT_V6_RANGE_SIZE uint = 64 |
There was a problem hiding this comment.
Might as well make those configurable as well, at least the v4 subnet.
Otherwise LGTM.
v2 fixes some bugs that were present in both the original code and v1 - combine IPv4 and IPv6 calculations into one - the new version can handle abitrary IPv4 or IPv6 subnet sizes and is not bound to specific sizes
grische
force-pushed
the
read-ranges-from-env
branch
from
17368fc
to
a2cfc9b
Compare
There was a problem hiding this comment.
Some of my thoughts. I agree with @SmithChart to move the project to Github to avoid cherry picking commits back and fourth.
| const CLIENT_V6_RANGE_SIZE uint = 64 | ||
|
|
||
| // IPv4 handling | ||
| v4RangeStr := os.Getenv("PARKER_V4_RANGE") |
There was a problem hiding this comment.
Personally would tend to use command line options (maybe in combination with a configuration file) for all the moving parts. Mostly because of the builtin documentation of the options via --help (and showing the default option values).
| v6ClientRangeStr := fmt.Sprintf("%s/%d", v6ClientSubnet, CLIENT_V6_RANGE_SIZE) | ||
| v6BigAddr := new(big.Int).SetBytes(v6ClientSubnet) | ||
| v6BigAddr.Add(v6BigAddr, big.NewInt(1)) // use the next free IPv6 | ||
| v6ClientAddrStr := net.IP(v6BigAddr.Bytes()).String() |
There was a problem hiding this comment.
having a function, I would prefer to integrate these variables into it, as they are equal for the V4 and V6 case. Probably as additional return values, e.g. v6ClientSubnet, v6ClientRangeString, v6ClientAddrStr, err := getNthSubnet(v6RangeStr, num, CLIENT_V6_RANGE_SIZE)
|
@grische We've moved the etcd-tools repo to Github for collaboration purposes: https://github.com/ffbs/etcd-tools . I've also added support for configuration via command line arguments (ffbs/etcd-tools@88dcf67) and a basic test for the ip range and address generation (ffbs/etcd-tools@018c0d4). Feel free to add your refactored subnet calculation and to set the subnet and it's range. |
Looking for upstream feedback (apart from the module rename ofc).
ping @SmithChart