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

Posted to dev@flume.apache.org by Ted Malaska <te...@cloudera.com> on 2013/09/17 21:58:50 UTC

Review Request 14176: Add support for IP filtering on AvroSource

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

Review request for Flume.


Bugs: Flume-2189
    https://issues.apache.org/jira/browse/Flume-2189


Repository: flume-git


Description
-------

This patch does the following
adds support for ipFiltering on the avroSource
adds unit testing for configuring ipFiltering on the avroSource
adds documentation in the user's guide


Diffs
-----

  flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java f23cd93 
  flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 2667a6f 
  flume-ng-doc/sphinx/FlumeUserGuide.rst c614991 
  pom.xml 25ea4e7 

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


Testing
-------


Thanks,

Ted Malaska

Re: Review Request 14176: Add support for IP filtering on AvroSource

Posted by Hari Shreedharan <hs...@cloudera.com>.

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


Thanks for the patch, Ted! Looks pretty good. I have some comments below, especially about the configuration. 


flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51262>

    Can we make this ipFilterRules? All new config params are being camel cased.



flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51263>

    You need a check here to ensure that patternRuleConfigDefinition is not null. If it is, then the config is invalid



flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51264>

    All of these can be final.



flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51265>

    Perhaps it is time to use a builder here?



flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51266>

    Nit: patternRuleConfigDefinition.isEmpty() == false can be written as
    !patternRuleConfigDefinition.isEmpty()
    
    This check should happen in the configure method, since having ip filter enabled with no rule is a misconfiguration and should fail the component - else the result would be that the component behaves in an unexpected way



flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/14176/#comment51268>

    I understand that this is being done so we can have multiple rules, but I'd prefer the rules to be made more explicit. A typo in the config (a vs d) should not cause issues which turns the rules upside down. I think the rule format should be more like:
    
    allow/deny:ip/name:pattern.
    
    Agreed that it makes the rules more verbose, but since it is compiled only once initially, I don't see a performance issue either.



flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java
<https://reviews.apache.org/r/14176/#comment51269>

    Could you add a test where a deny is happening where the accepted random host address is a random IP address. Something like:
    
    a:i:45.3.2.4 - this should be denied since the connection is from local host (of course you must also ensure that that random IP is not the local host - maybe generate using random method, not hardcode)



flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/14176/#comment51270>

    Trailing whitespace



flume-ng-doc/sphinx/FlumeUserGuide.rst
<https://reviews.apache.org/r/14176/#comment51271>

    trailing white space. Also please do not mention details about the underlying implementation - we could change that later without changing the parameters. So the user should not depend on the implementation.


- Hari Shreedharan


On Sept. 17, 2013, 7:58 p.m., Ted Malaska wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14176/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2013, 7:58 p.m.)
> 
> 
> Review request for Flume.
> 
> 
> Bugs: Flume-2189
>     https://issues.apache.org/jira/browse/Flume-2189
> 
> 
> Repository: flume-git
> 
> 
> Description
> -------
> 
> This patch does the following
> adds support for ipFiltering on the avroSource
> adds unit testing for configuring ipFiltering on the avroSource
> adds documentation in the user's guide
> 
> 
> Diffs
> -----
> 
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java f23cd93 
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 2667a6f 
>   flume-ng-doc/sphinx/FlumeUserGuide.rst c614991 
>   pom.xml 25ea4e7 
> 
> Diff: https://reviews.apache.org/r/14176/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ted Malaska
> 
>

Re: Review Request 14176: Add support for IP filtering on AvroSource

Posted by Hari Shreedharan <hs...@cloudera.com>.

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


Thanks for the patch. This looks good. I have one request though - please add a test where filtering and SSL are enabled on the same source to ensure that combination works.

- Hari Shreedharan


On Sept. 19, 2013, 7:13 p.m., Ted Malaska wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14176/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2013, 7:13 p.m.)
> 
> 
> Review request for Flume.
> 
> 
> Bugs: Flume-2189
>     https://issues.apache.org/jira/browse/Flume-2189
> 
> 
> Repository: flume-git
> 
> 
> Description
> -------
> 
> This patch does the following
> adds support for ipFiltering on the avroSource
> adds unit testing for configuring ipFiltering on the avroSource
> adds documentation in the user's guide
> 
> 
> Diffs
> -----
> 
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java f23cd93 
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 2667a6f 
>   flume-ng-doc/sphinx/FlumeUserGuide.rst c614991 
>   pom.xml 25ea4e7 
> 
> Diff: https://reviews.apache.org/r/14176/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ted Malaska
> 
>