[PATCH] Implement std.ip() to simplify ACL checking in VCL

[Lasse Karstensen]

On Thu, Aug 01, 2013 at 02:03:14AM +0100, Federico Schwindt wrote:
> On Wed, Jul 31, 2013 at 2:46 PM, Lasse Karstensen <
> lkarsten at varnish-software.com> wrote:
> > I've extended the std vmod to include an ip() method, which
> > converts a string into VCL IP. The result can be used for
> > matching against a VCL ACL.
> A few comments:
> - rp leaks if WS_Reserve() fails.
> - WS_Reserve() is cheaper that getaddrinfo(), so I'd check first if there
> is space and then do the getaddrinfo() call. That'd simplify the error path
> too.
> - Missing CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC).
> - You could just check for getaddrinfo() != 0 instead of using s = .. since
> it's not used anywhere else.

Hi Federico.
Thank you for taking the time to review this.

I've implemented the changes you proposed. Updated (full) patch is attached.


> - Using VCL_IP for the fallback parameter restricts what you can use to
> client.ip or server.ip. This might or might not be a problem.
> I wrote a similar function a while ago that was using a STRING parameter as
> suggested by Tollef. Not sure if this is still required.

Yes, we discussed this for a bit in the office.
I don't have strong opinions on either side.

You can of course nest them to get an arbitrary fallback:
	std.ip(req.http.X-Forwarded-For, std.ip("127.255.255.255"));

The error handling when the fallback conversion fails doesn't seem to have
any obvious solution. If the user gets to pick a fallback by him/herself,
then at least they know clearly what to check for.


-- 
With regards,
Lasse Karstensen
Varnish Software AS
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-ip-std.2013-08-01.patch
Type: text/x-diff
Size: 3545 bytes
Desc: not available
URL: <https://www.varnish-cache.org/lists/pipermail/varnish-dev/attachments/20130801/91465361/attachment.patch>