Export `RemoteIp.Block` as its own package?

[halostatue]

I’m getting ready to release a new IP access control plug and I had been using InetCidr when I looked at the latest 1.0 update to remote_ip. At this point, I threw out the parsing and implementation that I had and explicitly refer to RemoteIp.Block for my IP address inclusion test, which means that instead of remote_ip being an optional dependency of my plug, it is now a required dependency of my plug (its use is recommended whenever someone is behind a proxy server).

This is compounded by the fact that RemoteIp.Block is an implementation detail for the RemoteIp plug (that is, you’ve set it @moduledoc false). I don’t see you changing the API in a way that would break my use of it, but I’m not 100% comfortable relying on such a detail.

This leaves three options:

1. I copy the code from RemoteIp.Block into IpAccessControl.Block. I’m not fond of this approach, but it is the least amount of work for you.
2. I convince you to document RemoteIp.Block so that I can treat it as an official API that I can depend on.
3. I convince you to document RemoteIp.Block and make it its own Hex package similar to InetCidr so that it is usable without depending on internal details of a tangentially-related package.

I’d rather not do 1; the implementation for RemoteIp.Block is really well done and it feels wrong to duplicate this code in my package. I haven’t yet released the package to Hex, but I’d like to do so in the next week or so (I need to test this on a local package).

Let me know what you’d prefer; the current implementation can be found here: https://github.com/KineticCafe/ip_access_control

[ajvondrak]

Sure, I'd love to split it off into a separate package! Makes me happy to know someone noticed my yak shaving. 😛

It shouldn't be that much work - really, just the plumbing and docs and all, but that's mostly a matter of copy/paste/rearrange. I can make a little weekend project of it. I guess the hardest part would be coming up with a good name. 😄 Any suggestions? IpBlock?

[ajvondrak]

Looking at it, I think I'd like a name that would make a good namespace for both individual IPs and ranges. RemoteIp.Block.encode/1 actually encodes a single IP, so it reads kind of awkwardly when it's in the same module as the IP block functions. Would rather an Address.encode/1 and a Block.contains?/2 sort of distinction. 🤔

[halostatue]

I think that IpBlock is a good name, which would allow for IpBlock.Address.encode/1 and IpBlock.contains?/2.

From a usability perspective, it would probably be good if IpBlock.contains?/2 handled both encoded and unencoded IPs and parsed and unparsed ranges. This would make it (reasonably) possible to use the library correctly without making silly mistakes like I did and you pointed out. I’d say maybe something like this for a spec:

@type t :: %__MODULE__{}
@type encoded_range :: t | list(t)
@type unencoded_range :: binary() | list(binary())
@type range :: encoded_range | unencoded_range
@type unencoded_ip :: binary()
@type encoded_ip :: {:v4 | :v6, integer()}
@type ip :: encoded_ip | unencoded_ip

@spec contains?(range, ip) :: boolean()

If it would be preferable to have the list handling separately, then any_contains?/2 might be viable (even though that’s pretty much the same as Enum.any?(ranges, &contains?(&1, encoded_ip). I am thinking that the implementation of these would make it harder to use the library wrong, but it would still be worth documenting the advantages of pre-encoding/pre-parsing for long-term performance in tight loops.

[ajvondrak]

Yes, all good points. I was thinking many of the same things in my brain's background threads. 😄 If it's going to be a first-class library, it's probably better to provide a more flexible & complete API. Plus a "safe" API for doing the all-too-common Enum.any?(&contains?) check. Just have to document all the performance caveats.

I'll see what I can put together this weekend, even if it's just a v0.1.0 with the exact same interface as the current RemoteIp.Block. :shipit:

[halostatue]

Thanks! I’ll wait to release my package until you’ve got the 0.1.0 released.

[ajvondrak]

Okay, I have the repo live at https://github.com/ajvondrak/bitwise_ip. Tests all pass, the API seems stable enough, and the benchmarks show it to be on par with remote_ip. I also went on some side quests doing fun things like implementing the Enumerable protocol on BitwiseIp.Block. 🙃

To do:

• get all the packaging figured out so I can publish v0.1.0 to hex.pm; I'm going to try to get that done today
• docs; I doubt I'll get them done today, so I'll throw some generic "under construction" stuff up for the development release
• after docs are ready, it's probably okay to ship it as v1.0.0 (i.e., the first not-development release)
• any other feature requests? feedback?

Edit: Oh yeah, and the future goal of moving RemoteIp onto using BitwiseIp, of course.

[halostatue]

I’ll look at this a bit later and give you some feedback.

[ajvondrak]

Closed via ajvondrak/bitwise_ip#1. 🎉