lang/funcs: update cidrsubnet and cidrhost for 64-bit systems#25517
lang/funcs: update cidrsubnet and cidrhost for 64-bit systems#25517mildwonkey merged 3 commits intomasterfrom
Conversation
mildwonkey
changed the title
Codecov Report
|
| 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 |
There was a problem hiding this comment.
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.
lang/funcs/cidr.go
Outdated
| // (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 { |
There was a problem hiding this comment.
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.)
mildwonkey
force-pushed
the
mildwonkey/embiggen-cidr
branch
from
9acb90b to
e639d81
Compare
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
2 participants
The
cidrsubnetandcidrhostfunctions were limited to supporting 32-bit systems. This PR:"github.com/apparentlymart/go-cidr"library, which was recently extended with SubnetBig and HostBig functionsbig.Intscidrsubnetto check for bits > 64 instead of 32Fixes #25360
There's a new test, but for good measure, have some
terraform consoleoutput: