----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29714/#review68440 -----------------------------------------------------------
Looks awesome. Great tests. Two things: 1. The patch no longer applies. You may need to rebase. 2. As mentioned below, it may make more sense to initialize IPFilter in KafkaConfig rather than in SocketServer since we are validating the configs there. core/src/main/scala/kafka/network/IPFilter.scala <https://reviews.apache.org/r/29714/#comment112657> This may make more sense as an Enumeration? core/src/main/scala/kafka/network/IPFilter.scala <https://reviews.apache.org/r/29714/#comment112665> Can you explain the algorithm you are using in high level? Or if you are using a well-known conversion, just link to it. Also, perhaps mention that the goal of the exercise is to convert a range from the format of x.x.x.x/y to the format of (low_ip, high_ip) core/src/main/scala/kafka/network/IPFilter.scala <https://reviews.apache.org/r/29714/#comment112667> Error message can be better. I bet that seeing: "Rejected connection from 1.1.1.1 (allow)" will confuse the hell out of someone :) core/src/main/scala/kafka/network/SocketServer.scala <https://reviews.apache.org/r/29714/#comment112669> We have the IPFilter class that contains both rule type and list of strings, doesn't it make more sense to pass it here? core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/29714/#comment112674> You can actually have a single "val ipFilter" that will call a function that will get both props and initialize the IPFilter object. This will put the validation into the config too, where I think it belongs. Similar to what we have for getLogRetentionTimeMillis. core/src/main/scala/kafka/utils/VerifiableProperties.scala <https://reviews.apache.org/r/29714/#comment112676> Nice! core/src/test/scala/unit/kafka/network/IPFilterTest.scala <https://reviews.apache.org/r/29714/#comment112677> Very comprehensive tests. Thank you :) - Gwen Shapira On Jan. 16, 2015, 12:48 a.m., Jeff Holoman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29714/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2015, 12:48 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1810 > https://issues.apache.org/jira/browse/KAFKA-1810 > > > Repository: kafka > > > Description > ------- > > Put back in the 1512 changes and moved the initial BigInt out of the check > CIDRRange contains method > > > Diffs > ----- > > core/src/main/scala/kafka/network/IPFilter.scala PRE-CREATION > core/src/main/scala/kafka/network/SocketServer.scala > 39b1651b680b2995cedfde95d74c086d9c6219ef > core/src/main/scala/kafka/server/KafkaConfig.scala > 6e26c5436feb4629d17f199011f3ebb674aa767f > core/src/main/scala/kafka/server/KafkaServer.scala > 1691ad7fc80ca0b112f68e3ea0cbab265c75b26b > core/src/main/scala/kafka/utils/VerifiableProperties.scala > 2ffc7f452dc7a1b6a06ca7a509ed49e1ab3d1e68 > core/src/test/scala/unit/kafka/network/IPFilterTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 78b431f9c88cca1bc5e430ffd41083d0e92b7e75 > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 2377abe4933e065d037828a214c3a87e1773a8ef > > Diff: https://reviews.apache.org/r/29714/diff/ > > > Testing > ------- > > This code centers around a new class, CIDRRange in IPFilter.scala. The > IPFilter class is created and holds two fields, the ruleType > (allow|deny|none) and a list of CIDRRange objects. This is used in the Socket > Server acceptor thread. The check does an exists on the value in the list if > the rule type is allow or deny. On object creation, we pre-calculate the > lower and upper range values and store those as a BigInt. The overhead of the > check should be fairly minimal as it involves converting the incoming IP > Address to a BigInt and then just doing a compare to the low/high values. In > writing this review up I realized that I can optimize this further to convert > to bigint first and move that conversion out of the range check, which I can > address. > > Testing covers the CIDRRange and IPFilter classes and validation of IPV6, > IPV4, and configurations. Additionally the functionality is tested in > SocketServerTest. Other changes are just to assist in configuration. > > I modified the SocketServerTest to use a method for grabbing the Socket > server to make the code a bit more concise. > > One key point is that, if there is an error in configuration, we halt the > startup of the broker. The thinking there is that if you screw up > security-related configs, you want to know about it right away rather than > silently accept connections. (thanks Joe Stein for the input). > > There are two new exceptions realted to this functionality, one to handle > configuration errors, and one to handle blocking the request. Currently the > level is set to INFO. Does it make sense to move this to WARN ? > > > Thanks, > > Jeff Holoman > >
