only allowing one connection per IP address

[Trustin Lee]

Comments inline:

On Wed, Apr 1, 2009 at 3:15 PM, Frederic Bregier <fredbregier at free.fr> wrote:
> Sorry for the delay...

Hope you feel fine now.

> I commit on the branch located at
> http://anonsvn.jboss.org/repos/netty/branches/ipfilter
> a new proposal, trying to follow as much as possible your ideas and my understanding.
>
> I make a small resume:
>
> - channelOpenAccept / channelBoundAccept are completely removed.

Sweet.

>  Only channelConnectedAccept and channelClosedWasBlocked (not so happy with this name
>  but I couldn't figure something more acceptable rigt now) are still there.

I think we can simplify this further.  Can we remove
channelConnectedAccept and channelClosedWasBlocked, and just use
accept() and the attachment?  We can move the code in
channelConnectedAccept() to handleUpstream() and get rid of
channelConnectedAccept() and channelClosedWasBlocked().

> - handleRefusedChannel returns a ChannelFuture (may be null).
>  channelConnectedAccept therefore wait for this future to finish (except if null)
>  before closing the channel.

The following code is buggy:

            ChannelFuture future = this.handleRefusedChannel(ctx, e,
inetSocketAddress);
            if (future != null) {
                future.awaitUninterruptibly();
            }
            Channels.close(e.getChannel());

You have to to use ChannelFuture.addListener().  We can not assume IP
filter is behind an ExecutionHandler.

> - I only trap by default CONNECTED and CLOSED events.
>  However, if a channel was blocked (accept returning false), all events will not
>  be passed to the next entry in the pipeline.

This is somewhat inconsistent in that there's possibility that
channelOpen() and channelBound() can be handed off to the next
handlers while channelClosed() will never be handed off.  We should
block all events or none.

> - I create an interface IpFilterRule that one might want to implement to fit his need.
>  A default implementation IpSubnet is proposed (both standard and CIDR notations).
>  The main change is as the following.
>
>  An IpFilterRule have two kind of informations:
>  - A subnet (or whatever it is) where a InetAddress can be compared by the method
>    contains. This method returns true if the InetAddress is in this IpFilterRule.

It look OK, but I think it should work fine with IPv6 addresses, too.
Could you consider this?

>  - A DENY or ALLOW flag that says if the previous call to contains should be
>    considered as a DENY or ALLOW rule.
>  - A constructor enables to define a IpSubnet DENY/ALLOW ALL by just setting the flag.

This needs to be independent from IpSubnet, which means, IpSubnet
shouldn't decide its address should be allowed or denied.

> - I change a bit the OneIpFilterHandler (mainly HashMap to Set).

This broke compatibility with Java 1.5.  I fixed it by reverting it
back to ConcurrentHashMap.

> - I change a bit the IpBlackWhiteListHandler mainly to reflect other changes.

Looks good!  It will need some modification once IpSubnet and
allow/deny flag are split from each other.

> - I create a new handler named IpFilterRuleHandler which could replace the BlackWhite
>  one as it is more general. The BlackWhite is an handler that have only two lists:
>  Black or deny list then White or allow list in that order.
>  IpFilterRuleHandler is like a standard Firewall ruleset where you can mix in whatever
>  order you want IpFilter which are ALLOW or DENY.

Same here.  It will need some modification once IpSubnet and
allow/deny flag are split from each other.  IpSubnet is already
IpFilterRule, so we can remove IpBlackWhiteListHandler, right?  I
don't see a reason to keep IpBlackWhiteListHandler.

> I hope those changes will be more ok.
> Please take care that I didn't test them correctly, it is just a prototype!!!

It's getting better and better.  Please keep up the great work!

Cheers,

— Trustin Lee, http://gleamynode.net/