Skip to content

lang/funcs: update cidrsubnet and cidrhost for 64-bit systems #25517

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.

Sign up for GitHub

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

Merged
mildwonkey merged 3 commits into from
Aug 11, 2020
Merged

lang/funcs: update cidrsubnet and cidrhost for 64-bit systems #25517

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
642e359
upgrade go-cidr to v1.1.0
mildwonkey Jul 8, 2020
384cfaa
lang/funcs: refactor Subnet and Host functions to support 64-bit systems
mildwonkey Jul 8, 2020
e639d81
add test cases and remove no-longer-needed validation
mildwonkey Jul 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/aliyun/alibaba-cloud-sdk-go v0.0.0-20190329064014-6e358769c32a
github.com/aliyun/aliyun-oss-go-sdk v0.0.0-20190103054945-8205d1f41e70
github.com/aliyun/aliyun-tablestore-go-sdk v4.1.2+incompatible
github.com/apparentlymart/go-cidr v1.0.1
github.com/apparentlymart/go-cidr v1.1.0
github.com/apparentlymart/go-dump v0.0.0-20190214190832-042adf3cf4a0
github.com/apparentlymart/go-userdirs v0.0.0-20190512014041-4a23807e62b9
github.com/apparentlymart/go-versions v1.0.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ github.com/antchfx/xquery v0.0.0-20180515051857-ad5b8c7a47b0 h1:JaCC8jz0zdMLk2m+
github.com/antchfx/xquery v0.0.0-20180515051857-ad5b8c7a47b0/go.mod h1:LzD22aAzDP8/dyiCKFp31He4m2GPjl0AFyzDtZzUu9M=
github.com/apparentlymart/go-cidr v1.0.1 h1:NmIwLZ/KdsjIUlhf+/Np40atNXm/+lZ5txfTJ/SpF+U=
github.com/apparentlymart/go-cidr v1.0.1/go.mod h1:EBcsNrHc3zQeuaeCeCtQruQm+n9/YjEn/vI25Lg7Gwc=
github.com/apparentlymart/go-cidr v1.1.0 h1:2mAhrMoF+nhXqxTzSZMUzDHkLjmIHC+Zzn4tdgBZjnU=
github.com/apparentlymart/go-cidr v1.1.0/go.mod h1:EBcsNrHc3zQeuaeCeCtQruQm+n9/YjEn/vI25Lg7Gwc=
github.com/apparentlymart/go-dump v0.0.0-20180507223929-23540a00eaa3/go.mod h1:oL81AME2rN47vu18xqj1S1jPIPuN7afo62yKTNn3XMM=
github.com/apparentlymart/go-dump v0.0.0-20190214190832-042adf3cf4a0 h1:MzVXffFUye+ZcSR6opIgz9Co7WcDx6ZcY+RjfFHoA0I=
github.com/apparentlymart/go-dump v0.0.0-20190214190832-042adf3cf4a0/go.mod h1:oL81AME2rN47vu18xqj1S1jPIPuN7afo62yKTNn3XMM=
Expand Down
17 changes: 7 additions & 10 deletions lang/funcs/cidr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package funcs

import (
"fmt"
"math/big"
"net"

"github.com/apparentlymart/go-cidr/cidr"
Expand All @@ -25,7 +26,7 @@ var CidrHostFunc = function.New(&function.Spec{
},
Type: function.StaticReturnType(cty.String),
Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) {
var hostNum int
var hostNum *big.Int
Copy link
Copy Markdown
Contributor

@apparentlymart apparentlymart Jul 8, 2020

Choose a reason for hiding this comment

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

I had honestly completely forgotten that gocty knows how to decode into big.Int! I'm glad to remember now that it does, cause I was originally expecting this change to be a bit more invasive.

One thing I notice looking back at the test cases for these functions is that they aren't testing the situation where the newbits, hostnum, netnum arguments are fractional numbers. I'm expecting that all of those would be properly rejected as invalid by the gocty conversions here (it should say something like "value must be a whole number") but might be worth adding some "expected error" test cases as insurance to future changes that might inadvertently make those panic otherwise.

Copy link
Copy Markdown
Contributor Author

@mildwonkey mildwonkey Jul 8, 2020

Choose a reason for hiding this comment

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

Done and done!

if err := gocty.FromCtyValue(args[1], &hostNum); err != nil {
return cty.UnknownVal(cty.String), err
}
Expand All @@ -34,7 +35,7 @@ var CidrHostFunc = function.New(&function.Spec{
return cty.UnknownVal(cty.String), fmt.Errorf("invalid CIDR expression: %s", err)
}

ip, err := cidr.Host(network, hostNum)
ip, err := cidr.HostBig(network, hostNum)
if err != nil {
return cty.UnknownVal(cty.String), err
}
Expand Down Expand Up @@ -86,7 +87,7 @@ var CidrSubnetFunc = function.New(&function.Spec{
if err := gocty.FromCtyValue(args[1], &newbits); err != nil {
return cty.UnknownVal(cty.String), err
}
var netnum int
var netnum *big.Int
if err := gocty.FromCtyValue(args[2], &netnum); err != nil {
return cty.UnknownVal(cty.String), err
}
Expand All @@ -96,15 +97,11 @@ var CidrSubnetFunc = function.New(&function.Spec{
return cty.UnknownVal(cty.String), fmt.Errorf("invalid CIDR expression: %s", err)
}

// For portability with 32-bit systems where the subnet number
// will be a 32-bit int, we only allow extension of 32 bits in
// one call even if we're running on a 64-bit machine.
// (Of course, this is significant only for IPv6.)
if newbits > 32 {
return cty.UnknownVal(cty.String), fmt.Errorf("may not extend prefix by more than 32 bits")
if newbits > 64 {
Copy link
Copy Markdown
Contributor

@apparentlymart apparentlymart Jul 8, 2020

Choose a reason for hiding this comment

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

I don't think we need to have this check at all anymore, because the underlying CIDR library will already check to make sure that there's enough room to extend by newbits bits. I had originally put the > 32 check here just to make sure this function wouldn't behave differently on 32-bit vs. 64-bit builds of Terraform, but using *big.Int now means that the behavior of SubnetBig should be the same for all architectures and so we shouldn't need to apply additional constraints in the Terraform layer here.

(When using IPv6, it would also be "valid" to start off with a very short prefix like e.g. 1::/1 and extend that by up to 127 new bits, which SubnetBig should accept just fine now and might be worth including a test for just to exercise the boundary cases a bit more.)

return cty.UnknownVal(cty.String), fmt.Errorf("may not extend prefix by more than 64 bits")
}

newNetwork, err := cidr.Subnet(network, newbits, netnum)
newNetwork, err := cidr.SubnetBig(network, newbits, netnum)
if err != nil {
return cty.UnknownVal(cty.String), err
}
Expand Down
7 changes: 7 additions & 0 deletions lang/funcs/cidr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@ func TestCidrSubnet(t *testing.T) {
cty.StringVal("192.168.6.0/24"),
false,
},
{
cty.StringVal("fe80::/48"),
cty.NumberIntVal(33),
cty.NumberIntVal(6),
cty.StringVal("fe80::3:0:0:0/81"),
false,
},
{ // not enough bits left
cty.StringVal("192.168.0.0/30"),
cty.NumberIntVal(4),
Expand Down
38 changes: 28 additions & 10 deletions vendor/github.com/apparentlymart/go-cidr/cidr/cidr.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ github.com/aliyun/aliyun-tablestore-go-sdk/tablestore/search
github.com/antchfx/xpath
# github.com/antchfx/xquery v0.0.0-20180515051857-ad5b8c7a47b0
github.com/antchfx/xquery/xml
# github.com/apparentlymart/go-cidr v1.0.1
# github.com/apparentlymart/go-cidr v1.1.0
## explicit
github.com/apparentlymart/go-cidr/cidr
# github.com/apparentlymart/go-dump v0.0.0-20190214190832-042adf3cf4a0
Expand Down