ranger-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Madhan Neethiraj <mad...@apache.org>
Subject Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin
Date Mon, 12 Mar 2018 07:21:17 GMT

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




tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 69 (patched)
<https://reviews.apache.org/r/65920/#comment279282>

    - consider moving 'static' field '_LOCK' ahead of instance members (to line #60 above)
    - also, consider marking this field as 'final'



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Line 179 (original), 195 (patched)
<https://reviews.apache.org/r/65920/#comment279283>

    'webResource' is used only inside the 'else' block at line #198. Consider moving this
line (#195) to #199.



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 228 (patched)
<https://reviews.apache.org/r/65920/#comment279284>

    "isValidRangerCookie == true" ==> "isValidRangerCookie"



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 276 (patched)
<https://reviews.apache.org/r/65920/#comment279290>

    Given this entire method implementation is under this synchronized() block, it might be
easier to mark this method as 'synchroized' (and get rid of '_LOCK' object)



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 320 (patched)
<https://reviews.apache.org/r/65920/#comment279285>

    Instead of initializing with 'null' here and assigning below, consider initializing as
shown below:
    
      WebResource         webResource = createCachedWebResource(REST_URL_IMPORT_SERVICETAGS_RESOURCE);
      WebResource.Builder br          = webResource.getRequestBuilder().cookie(sessionId);
      ClientResponse      response    = br.accept(REST_MIME_TYPE_JSON).type(REST_MIME_TYPE_JSON).put(ClientResponse.class,
tagRESTClient.toJson(serviceTags));



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 328 (patched)
<https://reviews.apache.org/r/65920/#comment279286>

    when would 'response' not contain REST_URL_IMPORT_SERVICETAGS_RESOURCE? If there is such
a case, can it not be covered by testing for response.getStatus()?
    
    Please review and update other such usage in this file.



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 367 (patched)
<https://reviews.apache.org/r/65920/#comment279287>

    It is not clean why this is a 'cachedWebResource'. Please review and rename appropriately.



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 369 (patched)
<https://reviews.apache.org/r/65920/#comment279289>

    'tagRESTClient.getClient()' can return null, if tagRESTClient.resetClient() was called.
Please review and update to handle this case.


- Madhan Neethiraj


On March 9, 2018, 4:35 p.m., Nikhil P wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> -----------------------------------------------------------
> 
> (Updated March 9, 2018, 4:35 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Don Bosco Durai, Gautam Borad, Abhay Kulkarni,
Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja Polavarapu,
and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2010
>     https://issues.apache.org/jira/browse/RANGER-2010
> 
> 
> Repository: ranger
> 
> 
> Description
> -------
> 
> Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger
admin.
> 
> 
> Diffs
> -----
> 
>   agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTClient.java 0d94edc

>   tagsync/src/main/java/org/apache/ranger/tagsync/process/TagSyncConfig.java 697c7cc

>   tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
a1dc8f5 
>   tagsync/src/main/resources/ranger-tagsync-site.xml 3542ae2 
> 
> 
> Diff: https://reviews.apache.org/r/65920/diff/2/
> 
> 
> Testing
> -------
> 
> 1)verified if ranger tagsync works as expected.
> 2)verified if tagsync is not re-login for every notification if "ranger.tagsync.cookie.enabled"
is set as true.
> 
> 
> Thanks,
> 
> Nikhil P
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message