You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Nikhil P <ni...@gmail.com> on 2018/03/06 11:16:15 UTC
Review Request 65920: Ranger Tagsync should use cookie based
authentication for subsequent requests to Ranger admin
-----------------------------------------------------------
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
Re: Review Request 65920: Ranger Tagsync should use cookie based
authentication for subsequent requests to Ranger admin
Posted by Gautam Borad <gb...@gmail.com>.
-----------------------------------------------------------
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
Posted by Velmurugan Periasamy <vp...@hortonworks.com>.
-----------------------------------------------------------
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
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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
Posted by Nikhil P <ni...@gmail.com>.
-----------------------------------------------------------
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
Posted by Ramesh Mani <rm...@hortonworks.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65920/#comment279314>
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
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65920/#comment279460>
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
Posted by Nikhil P <ni...@gmail.com>.
-----------------------------------------------------------
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
Posted by Nikhil P <ni...@gmail.com>.
> 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)
> > <https://reviews.apache.org/r/65920/diff/2/?file=1973123#file1973123line329>
> >
> > 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)
> > <https://reviews.apache.org/r/65920/diff/2/?file=1973123#file1973123line368>
> >
> > 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)
> > <https://reviews.apache.org/r/65920/diff/2/?file=1973123#file1973123line370>
> >
> > '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
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 65920: Ranger Tagsync should use cookie based
authentication for subsequent requests to Ranger admin
Posted by Nikhil P <ni...@gmail.com>.
-----------------------------------------------------------
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
Posted by Nikhil P <ni...@gmail.com>.
> 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)
> > <https://reviews.apache.org/r/65920/diff/1/?file=1971742#file1971742line268>
> >
> > 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
Posted by Zsombor Gegesy <zs...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65920/#comment278936>
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)
<https://reviews.apache.org/r/65920/#comment278937>
cookieList
tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 267 (patched)
<https://reviews.apache.org/r/65920/#comment278930>
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)
<https://reviews.apache.org/r/65920/#comment278931>
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)
<https://reviews.apache.org/r/65920/#comment278932>
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)
<https://reviews.apache.org/r/65920/#comment278933>
Cookie should be lowercased
tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 343 (patched)
<https://reviews.apache.org/r/65920/#comment278934>
response.toString()
tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 362 (patched)
<https://reviews.apache.org/r/65920/#comment278935>
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
>
>