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

Add Terraform CIDR based functions #1342

Merged
merged 5 commits into from
May 7, 2022
Merged

Add Terraform CIDR based functions #1342

merged 5 commits into from
May 7, 2022

Conversation

bartoszj
Copy link
Contributor

@bartoszj bartoszj commented Mar 19, 2022

I've added a few more functions to the net namespace:

  • net.CidrHost to calculate host IP address from CIDR and host number, based on Terraform cidrhost function
  • net.CidrNetmask to return CIDR netmask in dotted-decimal IPv4 address syntax, based on Terraform cidrnetmask function
  • net.CidrSubnets to return a list of subnets from give CIDR and new netmask bits, based on Terraform cidrsubnet function
  • net.CidrSubnetSizes to calculate a sequence of consecutive IP address ranges within a particular CIDR prefix, based on Terraform cidrsubnets function

@hairyhenderson
Copy link
Owner

Thanks for taking the time to submit this, @bartoszj!

For the future, it's a good idea to file an issue before spending time sending a PR (see CONTRIBUTING.md for details)

I'm curious about why the current net.Parse* functions not adequate for your needs? I had actually been planning to swap them out for the new net/netip functions that were added to the stdlib in Go 1.18 - my understanding is the new package is almost the same as the inet.af/netaddr package - some API changed, and some is missing, though I'm not sure how much impact there is there.

@bartoszj
Copy link
Contributor Author

Hello. Sorry for that missing issue ticket :(

Regarding net.Parse*, I'm using github.com/apparentlymart/go-cidr/cidr package, which works with net types. But after writing this PR I think I can remove net.StdParse* methods, I don't need them. For me only the net.Cidr* methods are important and it is possible to reuse inet.af/netaddr types and probably the netip types.

If you want I can remove net.StdParse* methods and reimplement parsing in net.Cidr* using the methods which you will choose.

@hairyhenderson
Copy link
Owner

Thanks for the extra detail.

Can you explain in concrete terms what you're planning on using the net.Cidr* functions for?

A bit of a tangent:

It's a bit unfortunate that neither inet.af/netaddr nor net/netip have better support for netmasks... It seems to me that something definitely missing is a function that can take a prefix length and format it as an IPv4 netmask. There's a bit of ambiguity as I might want a v6 mask, but I've never seen a v6 mask written as anything other than prefix length...

Also - I looked a bit more at the difference between inet.af/netaddr and net/netip, and there are methods missing from net/netip - there's no equivalent for inet.af/netaddr#IPPrefix.IPNet, for example. So if I were to simply migrate the package over, that would technically be a breaking change, which I'd like to reserve for gomplate v4. Not a big deal, but adds a bit of an extra complicating factor...

@bartoszj
Copy link
Contributor Author

Hello. Regarding net.Cidr* methods. I need to prepare network configuration from a network like 10.0.0.0/23. I can use net.CidrSubnetSizes to make a basic network setup:

$ gomplate -i '{{ net.CidrSubnetSizes 1 2 3 3 "10.0.0.0/23" -}}'
[10.0.0.0/24 10.0.1.0/25 10.0.1.128/26 10.0.1.192/26]

The first and second subnet is later divided into 4 or 8 smaller subnets. net.CidrSubnets can be used to make those networks:

$ gomplate -i '{{ net.CidrSubnets 3 "10.0.0.0/24" -}}'
[10.0.0.0/27 10.0.0.32/27 10.0.0.64/27 10.0.0.96/27 10.0.0.128/27 10.0.0.160/27 10.0.0.192/27 10.0.0.224/27]

$ gomplate -i '{{ net.CidrSubnets 2 "10.1.0.0/25" -}}'
[10.1.0.0/27 10.1.0.32/27 10.1.0.64/27 10.1.0.96/27]

Sometimes there is also a need to get the 10th host IP from a given subnet like:

$ gomplate -i '{{ net.CidrHost 10 "10.0.0.32/27" -}}'
10.0.0.42

I don't have yet usage for net.CidrNetmask but I've added it since it was the last method from Terraform. Maybe later it will be useful.

I'll try to find a method to convert inet.af/netaddr#IPPerfix and net/netip#Prefix to net#IPNet.

@bartoszj
Copy link
Contributor Author

bartoszj commented Mar 20, 2022

I've removed net.StdParse* methods, since I'm not using them. Also I've converted all return types to use inet.af/netaddr types. Added support for parsing net/netip types and two helper methods to convert from net.IP and net.IPNet to net/netip.Addr and net/netip.Prefix types. So once, you will move to net/netip the net.Cidr* methods will work without any problems.

@bartoszj bartoszj changed the title Add net package IP parsing and Terraform CIDR based functions Add Terraform CIDR based functions Mar 21, 2022
@hairyhenderson
Copy link
Owner

Thanks @bartoszj, and sorry for the delay... I'm going to merge this, though I think the names should be net.CIDR* rather than net.Cidr* - because CIDR is an acronym (compare with net.LookupCNAME or conv.URL).

I'll take care of changing these though - I might do this in this PR, or I might merge it first and then make the change in a followup...

bartoszj added 4 commits May 6, 2022 22:50
…functions which are working similar to Terraform IP network functions
…ype parsing. Convert return types to use `inet.af/netaddr` types.
@hairyhenderson
Copy link
Owner

I've reworked this a bit to move to using net/netip instead on these new functions. Also, I've marked them as experimental for now - as I'm fairly certain I'm going to want to change the names to fit better with some new net/netip-based functions I plan on adding before v4. But I want to get this merged sooner rather than later so it can go into an upcoming v3.11 release.

@hairyhenderson hairyhenderson enabled auto-merge May 7, 2022 02:51
Signed-off-by: Dave Henderson <[email protected]>
@hairyhenderson hairyhenderson merged commit 33f886a into hairyhenderson:master May 7, 2022
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.

2 participants