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

Address range support in ClientIpActivationStrategy #172

Merged
merged 4 commits into from Jul 12, 2016

Conversation

Projects
None yet
2 participants
@ractive
Contributor

ractive commented Jun 28, 2016

No description provided.

ips.add(InetAddress.getByName(part));
}
} catch (Exception e) {
log.warn("Ignoring illegal IP address or CIDR range " + part);

This comment has been minimized.

@ractive

ractive Jun 28, 2016

Contributor

Not sure if this exception should be catched here. How would the togglz-console UI handle it?

This comment has been minimized.

@chkal

chkal Jun 29, 2016

Member

First you should catch UnknownHostException instead of Exception here.

And we should perhaps use the ParameterBuilder to create a validation rule for parameter values. The builder supports this using regular expressions. But you could also provider you own Parameter implementation which provides custom validation code. See ScriptEngineActivationStrategy for an example. This way we could catch invalid strategy parameters before they are saved.

}
}
} catch (UnknownHostException e) {
log.error("Illegal address " + request.getRemoteAddr());

This comment has been minimized.

@ractive

ractive Jun 28, 2016

Contributor

Not sure if this exception should be catched here. How would the togglz-console UI handle it?

}
try {
if (ips.contains(InetAddress.getByName(request.getRemoteAddr()))) {

This comment has been minimized.

@chkal

chkal Jun 29, 2016

Member

Does InetAddress.getByName() perform DNS lookups? If so, we should not use it because it will be a performance problem.

This comment has been minimized.

@ractive

ractive Jun 29, 2016

Contributor

Just checked the implementation. If the given address is an IPv4 or IPv6 address, no DNS lookup is done.

List<String> parts = Strings.splitAndTrim(featureState.getParameter(PARAM_IPS), "[\\s,]+");
List<InetAddress> ips = new ArrayList<>();

This comment has been minimized.

@chkal

chkal Jun 29, 2016

Member

I don't think that you have to build these lists first and validate in a separate code block. Why not doing this in one step. This would even be more efficient, because you have a result as soon as the first entry matches and therefore don't have parse the following addresses.

@chkal

This comment has been minimized.

Member

chkal commented Jun 29, 2016

Thanks a lot for providing help with this. I added some comments to your code.

@ractive

This comment has been minimized.

Contributor

ractive commented Jun 29, 2016

I introduced a Parameter implementation that does the validation. WDYT?

@chkal

This comment has been minimized.

Member

chkal commented Jun 30, 2016

Awesome! Thanks a lot! I'll have a depper look at this ASAP. I'm currently packing my stuff for a two week trip. So it may take some time. Sorry about that.

if (cidrUtil.isInRange(remoteAddr)) {
return true;
}
} else if (InetAddress.getByName(remoteAddr).equals(InetAddress.getByName(part))) {

This comment has been minimized.

@chkal

chkal Jul 9, 2016

Member

I think InetAddress.getByName(remoteAddr) should be moved out of the loop for performance...

} else if (InetAddress.getByName(remoteAddr).equals(InetAddress.getByName(part))) {
return true;
}
} catch (Exception e) {

This comment has been minimized.

@chkal

chkal Jul 9, 2016

Member

Catching Exception is bad practice. Could you catch the corresponding exceptions instead. Catching Exception will also catch stuff like NPEs, which is not very nice.

@chkal

This comment has been minimized.

Member

chkal commented Jul 9, 2016

Hey. Sorry for the delay. As I wrote earlier I'm currently on vacation and I promised my wife to not use the notebook too much. ;-)

Your code looks great! I just added two comments. Thank you very much for your contribution. This is awesome!

@chkal

This comment has been minimized.

Member

chkal commented Jul 9, 2016

And BTW: The Parameter implementation looks great. This way the parameters will be validated correctly and there should never be an invalid IP / netmask, which is great!

@ractive

This comment has been minimized.

Contributor

ractive commented Jul 11, 2016

I promised my wife to not use the notebook too much. ;-)

Just hold to this promise... ;-)

I implemented your suggestions.

@chkal

This comment has been minimized.

Member

chkal commented Jul 12, 2016

Awesome! This looks great! Thank you so much.

But it looks like there are test failures... Not sure why. Actually I don't think it is related to your changes. I'll have to check that...

@chkal chkal self-assigned this Jul 12, 2016

@chkal chkal added this to the 2.3.0.Final milestone Jul 12, 2016

@chkal chkal added the new-feature label Jul 12, 2016

@chkal chkal merged commit 04ce09b into togglz:master Jul 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chkal

This comment has been minimized.

Member

chkal commented Jul 12, 2016

Ok, tests are green now. Seems like this was just a temporary failure. I just merged your changes. Thank you very much! 🍻

@ractive

This comment has been minimized.

Contributor

ractive commented Jul 12, 2016

Nice! I'm looking forward to use this new feature... :-)

@chkal

This comment has been minimized.

Member

chkal commented Jul 12, 2016

I hope to find some time at the weekend to cut a release...

@chkal

This comment has been minimized.

Member

chkal commented Jul 30, 2016

Hey @ractive,

I've a funny story to share. I just ran into weird test failures with the test you implemented on my notebook. Basically the failing test verified, that abc/24 isn't a valid IP range. It took me quite some time to figure out that the Google DNS servers which I'm using are able to resolve abc as a valid hostname. So the code actually got a valid IP and therefore the range was valid.

$ dig +short @8.8.8.8 abc
127.0.53.53

So the current code treats abc/24 as 127.0.53.53/24.

I "fixed" this like this: 1609f9b

I'm not sure if I like that the code currently resolves hostnames. Does this make sense?

@ractive

This comment has been minimized.

Contributor

ractive commented Aug 2, 2016

Of course does the alphabet resolve abc. :-)

Hm. This "abc/24" was a way to test the IllegalArgumentException that is thrown by the CIDRUtil.
But I agree. We should check that the user only uses IP addresses and not hostnames, esp. for the CIDR range queries. I quickly thought about it, but could not find an easy way to determine if a string is an IP address or not.
I'll give it a try later on...

@chkal

This comment has been minimized.

Member

chkal commented Aug 2, 2016

Awesome. Thanks. ;-)

@ractive

This comment has been minimized.

Contributor

ractive commented Aug 24, 2016

Back from holiday. I hope I can give it a shot this week...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment