Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-14 Thread Velmurugan Periasamy 

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


Ship it!




Ship It!

- Velmurugan Periasamy


On March 14, 2018, 6:31 a.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 14, 2018, 6:31 a.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/5/
> 
> 
> 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.
> 
> 
> File Attachments
> 
> 
> 0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 0005-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/14/e8193a99-70e9-4124-b70a-bd9335665455__0005-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 
> 
> Thanks,
> 
> Nikhil P
> 
>

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-14 Thread Madhan Neethiraj 

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


Ship it!




Ship It!

- Madhan Neethiraj


On March 14, 2018, 6:31 a.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 14, 2018, 6:31 a.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/5/
> 
> 
> 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.
> 
> 
> File Attachments
> 
> 
> 0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 0005-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/14/e8193a99-70e9-4124-b70a-bd9335665455__0005-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 
> 
> Thanks,
> 
> Nikhil P
> 
>

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-14 Thread Nikhil P 

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

(Updated March 14, 2018, 12:01 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 (updated)
-

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

Changes: https://reviews.apache.org/r/65920/diff/4-5/


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.


File Attachments (updated)


0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
  
https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
0005-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
  
https://reviews.apache.org/media/uploaded/files/2018/03/14/e8193a99-70e9-4124-b70a-bd9335665455__0005-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch


Thanks,

Nikhil P

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-13 Thread Madhan Neethiraj 

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


Fix it, then Ship it!





tagsync/src/main/java/org/apache/ranger/tagsync/process/TagSyncConfig.java
Line 208 (original), 209 (patched)


This would cause NPE, if 'val' is null i.e. when property 
TAGSYNC_ENABLED_PROP is not set.

Consider replacing this with:
 return val == null || Boolean.valueOf(val.trim());

Same applies for line #214 as well.


- Madhan Neethiraj


On March 12, 2018, 10:16 a.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 12, 2018, 10:16 a.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/4/
> 
> 
> 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.
> 
> 
> File Attachments
> 
> 
> 0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 
> 
> Thanks,
> 
> Nikhil P
> 
>

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-12 Thread Ramesh Mani 

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




tagsync/src/main/java/org/apache/ranger/tagsync/process/TagSyncConfig.java
Lines 214 (patched)


please consider doing ("false").equalsIgnoreCase(val.trim())


- Ramesh Mani


On March 12, 2018, 10:16 a.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 12, 2018, 10:16 a.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/3/
> 
> 
> 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.
> 
> 
> File Attachments
> 
> 
> 0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 
> 
> Thanks,
> 
> Nikhil P
> 
>

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-12 Thread Nikhil P 


> On March 12, 2018, 12:51 p.m., Madhan Neethiraj wrote:
> > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
> > Lines 328 (patched)
> > 
> >
> > 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.

this could happen in case of redirection to a different page such as login page 
for some reason and status code(response.getStatus) could still be 200/204 for 
that page (as that page is successfully loaded) ,therefore added a check that 
request URL in response should be same as requested.


> On March 12, 2018, 12:51 p.m., Madhan Neethiraj wrote:
> > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
> > Lines 367 (patched)
> > 
> >
> > It is not clean why this is a 'cachedWebResource'. Please review and 
> > rename appropriately.

unlike createWebResource() method, createCachedWebResource() creates a 
webresource without credentials in order to use that webresource for cookie 
based authentication.renaming createCachedWebResource() to 
createWebResourceForCookieAuth().


> On March 12, 2018, 12:51 p.m., Madhan Neethiraj wrote:
> > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
> > Lines 369 (patched)
> > 
> >
> > 'tagRESTClient.getClient()' can return null, if 
> > tagRESTClient.resetClient() was called. Please review and update to handle 
> > this case.

If tagRESTClient.resetClient() was called tagRESTClient.getClient() will return 
new object of Client,it wont return null.Also, we are calling 
tagRESTClient.resetClient() only when we want a completely new session(during 
first login or at the time when cookie expired/invalidated).

Following is the tagRESTClient.getClient() method for reference.
public Client getClient() {
// result saves on access time when client is built at the time of the 
call
Client result = client;
if(result == null) {
synchronized(this) {
result = client;
if(result == null) {
client = result = buildClient();
}
}
}

return result;
}


- Nikhil


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


On March 12, 2018, 3:46 p.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 12, 2018, 3:46 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/3/
> 
> 
> 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.
> 
> 
> File Attachments
> 
> 
> 0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
> 
> 
> Thanks,
> 
> Nikhil P
> 
>

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-12 Thread Nikhil P 

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

(Updated March 12, 2018, 3:46 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 (updated)
-

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

Changes: https://reviews.apache.org/r/65920/diff/2-3/


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.


File Attachments (updated)


0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch
  
https://reviews.apache.org/media/uploaded/files/2018/03/12/3f7133ab-72f9-413e-997c-4d9265f88cdc__0003-RANGER-2010-Ranger-Tagsync-should-use-cookie-based-a.patch


Thanks,

Nikhil P

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-12 Thread Madhan Neethiraj 

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


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


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


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



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 276 (patched)


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)


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)


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)


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)


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

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-12 Thread Gautam Borad 

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


Ship it!




Ship It!

- Gautam Borad


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

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-09 Thread Nikhil P 

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

(Updated March 9, 2018, 10:05 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 (updated)
-

  
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/

Changes: https://reviews.apache.org/r/65920/diff/1-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

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-09 Thread Nikhil P 


> On March 6, 2018, 9:49 p.m., Zsombor Gegesy wrote:
> > tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
> > Lines 267 (patched)
> > 
> >
> > Instead of relying on an external projects toString method, could you 
> > just use response.getLocation() ?

no zsombor, we can not use response.getLocation() as it does not return 
expected result.


- Nikhil


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


On March 6, 2018, 4:46 p.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 6, 2018, 4:46 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/1/
> 
> 
> 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
> 
>

Re: Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-06 Thread Zsombor Gegesy 

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



And one question, why there is a uploadTagsWithCredUnSync ? It's not clear from 
the code, why the play with sync/unsync was needed ?


tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 63 (patched)


RANGERADMINSESSIONID is not a final static constant, 'sessionId' could be a 
perfectly valid name



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 67 (patched)


cookieList



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 267 (patched)


Instead of relying on an external projects toString method, could you just 
use response.getLocation() ?



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 286 (patched)


The 3 if all contains (response != null) - this could be moved to an outer 
if :

if (response != null) {
 if (a) ... 
 if (b) ... 
 if (c) ...
}

Or isn't it true that response is not null?



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 324 (patched)


Assigning value twice to CookieList - null, and response.getCookies(). The 
first is not needed.

CookieList is a variable, not a type, why the upper case ?



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 326 (patched)


Cookie should be lowercased



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 343 (patched)


response.toString()



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 362 (patched)


response.toString()


- Zsombor Gegesy


On March 6, 2018, 11:16 a.m., Nikhil P wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> ---
> 
> (Updated March 6, 2018, 11:16 a.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/1/
> 
> 
> 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
> 
>

Review Request 65920: Ranger Tagsync should use cookie based authentication for subsequent requests to Ranger admin

2018-03-06 Thread Nikhil P 

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

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


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