You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Jeff Holoman <je...@gmail.com> on 2015/01/08 18:01:46 UTC

Review Request 29714: Patch for KAFKA-1810

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29714/
-----------------------------------------------------------

Review request for kafka.


Bugs: KAFKA-1810
    https://issues.apache.org/jira/browse/KAFKA-1810


Repository: kafka


Description
-------

KAFKA-1810


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
-------


Thanks,

Jeff Holoman

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Gwen Shapira <gs...@cloudera.com>.

-----------------------------------------------------------
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
> 
>

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.


> On Jan. 16, 2015, 1:55 p.m., Eric Olander wrote:
> > core/src/test/scala/unit/kafka/network/IPFilterTest.scala, line 71
> > <https://reviews.apache.org/r/29714/diff/2/?file=823240#file823240line71>
> >
> >     This is more an FYI as it may go against established practices for tests in this project.  Kafka includes scalatest which has a nice mechanism for this kind of testing:
> >     
> >     intercept[IPFilterConfigException] {
> >       new IPFilter(range, ruleType)
> >     }
> >     
> >     That does the same as the try/catch with the fail() in the try block.

Agreed. Based on some research I did when looking into KAFKA-1782 the testing guidleines are perhaps a bit in flux. I'm open to change this to whatever makes sense.


> On Jan. 16, 2015, 1:55 p.m., Eric Olander wrote:
> > core/src/main/scala/kafka/network/IPFilter.scala, line 38
> > <https://reviews.apache.org/r/29714/diff/2/?file=823235#file823235line38>
> >
> >     Pattern matching is unnecessary here.  Instead it is more performant/idiomatic to use the API calls on Option:
> >     
> >     if (rgx.findFirstIn(ruleType).isDefined) {
> >       ruleType
> >     } else {
> >       throw new IPFilterConfigException("Invalid rule type specified: " + ruleType)
> >     }

Thanks for the Review! Will incorporate this.


- Jeff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29714/#review68424
-----------------------------------------------------------


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
> 
>

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Eric Olander <ol...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29714/#review68424
-----------------------------------------------------------



core/src/main/scala/kafka/network/IPFilter.scala
<https://reviews.apache.org/r/29714/#comment112638>

    Pattern matching is unnecessary here.  Instead it is more performant/idiomatic to use the API calls on Option:
    
    if (rgx.findFirstIn(ruleType).isDefined) {
      ruleType
    } else {
      throw new IPFilterConfigException("Invalid rule type specified: " + ruleType)
    }



core/src/test/scala/unit/kafka/network/IPFilterTest.scala
<https://reviews.apache.org/r/29714/#comment112639>

    This is more an FYI as it may go against established practices for tests in this project.  Kafka includes scalatest which has a nice mechanism for this kind of testing:
    
    intercept[IPFilterConfigException] {
      new IPFilter(range, ruleType)
    }
    
    That does the same as the try/catch with the fail() in the try block.


- Eric Olander


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
> 
>

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29714/#review76500
-----------------------------------------------------------



core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/29714/#comment124033>

    Need to delete


- Jeff Holoman


On March 15, 2015, 5:13 a.m., Jeff Holoman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29714/
> -----------------------------------------------------------
> 
> (Updated March 15, 2015, 5:13 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1810
>     https://issues.apache.org/jira/browse/KAFKA-1810
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1810 ConfigDef Refactor
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/network/IPFilter.scala PRE-CREATION 
>   core/src/main/scala/kafka/network/SocketServer.scala 76ce41aed6e04ac5ba88395c4d5008aca17f9a73 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 46d21c73f1feb3410751899380b35da0c37c975c 
>   core/src/main/scala/kafka/server/KafkaServer.scala dddef938fabae157ed8644536eb1a2f329fb42b7 
>   core/src/test/scala/unit/kafka/network/IpFilterTest.scala PRE-CREATION 
>   core/src/test/scala/unit/kafka/network/SocketServerTest.scala 0af23abf146d99e3d6cf31e5d6b95a9e63318ddb 
>   core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 191251d1340b5e5b2d649b37af3c6c1896d07e6e 
>   core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 7f47e6f9a74314ed9e9f19d0c97931f3f2e49259 
> 
> 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
> 
>

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29714/
-----------------------------------------------------------

(Updated March 15, 2015, 5:26 a.m.)

Review request for kafka.

Bugs: KAFKA-1810
https://issues.apache.org/jira/browse/KAFKA-1810

Repository: kafka

Description
-------

KAFKA-1810 ConfigDef Refactor

Diffs
-----

core/src/main/scala/kafka/network/IPFilter.scala PRE-CREATION
core/src/main/scala/kafka/network/SocketServer.scala 76ce41aed6e04ac5ba88395c4d5008aca17f9a73
core/src/main/scala/kafka/server/KafkaConfig.scala 46d21c73f1feb3410751899380b35da0c37c975c
core/src/main/scala/kafka/server/KafkaServer.scala dddef938fabae157ed8644536eb1a2f329fb42b7
core/src/test/scala/unit/kafka/network/IpFilterTest.scala PRE-CREATION
core/src/test/scala/unit/kafka/network/SocketServerTest.scala 0af23abf146d99e3d6cf31e5d6b95a9e63318ddb
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 191251d1340b5e5b2d649b37af3c6c1896d07e6e
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 7f47e6f9a74314ed9e9f19d0c97931f3f2e49259

Diff: https://reviews.apache.org/r/29714/diff/

Testing (updated)
-------

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.

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 is one new exceptions realted to this functionality, to handle blocking the request. Currently the level is set to WARN.

More details are in KIP-7
https://cwiki.apache.org/confluence/display/KAFKA/KIP-7+-+Security+-+IP+Filtering

Thanks,

Jeff Holoman

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

(Updated March 15, 2015, 5:13 a.m.)

Review request for kafka.

Bugs: KAFKA-1810
https://issues.apache.org/jira/browse/KAFKA-1810

Repository: kafka

Description (updated)
-------

KAFKA-1810 ConfigDef Refactor

Diffs (updated)
-----

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.

I modified the SocketServerTest to use a method for grabbing the Socket server to make the code a bit more concise.

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

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

(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 (updated)
-------

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
-------

I modified the SocketServerTest to use a method for grabbing the Socket server to make the code a bit more concise.

Thanks,

Jeff Holoman

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

(Updated Jan. 16, 2015, 12:47 a.m.)

Review request for kafka.

Bugs: KAFKA-1810
https://issues.apache.org/jira/browse/KAFKA-1810

Repository: kafka

Description (updated)
-------

KAFKA-1810 Refactor

KAFKA-1810 Refactor 2

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/29714/diff/

Testing
-------

I modified the SocketServerTest to use a method for grabbing the Socket server to make the code a bit more concise.

Thanks,

Jeff Holoman

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29714/#review67573
-----------------------------------------------------------



core/src/main/scala/kafka/network/SocketServer.scala
<https://reviews.apache.org/r/29714/#comment111609>

    This needs to be addressed as I backed out the previous fix for 1512



core/src/main/scala/kafka/server/KafkaServer.scala
<https://reviews.apache.org/r/29714/#comment111610>

    Also needs to be fixed to include previous fix for 1512


- Jeff Holoman


On Jan. 8, 2015, 7:14 p.m., Jeff Holoman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29714/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2015, 7:14 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1810
>     https://issues.apache.org/jira/browse/KAFKA-1810
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1810
> 
> 
> 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
> 
>

Re: Review Request 29714: Patch for KAFKA-1810

Posted by Jeff Holoman <je...@gmail.com>.

(Updated Jan. 8, 2015, 7:14 p.m.)

Review request for kafka.

Bugs: KAFKA-1810
https://issues.apache.org/jira/browse/KAFKA-1810

Repository: kafka

Description
-------

KAFKA-1810

Diffs
-----

Diff: https://reviews.apache.org/r/29714/diff/

Testing (updated)
-------

I modified the SocketServerTest to use a method for grabbing the Socket server to make the code a bit more concise.

Thanks,

Jeff Holoman