You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sravya Tirukkovalur <sr...@cloudera.com> on 2016/02/05 08:08:33 UTC
Review Request 43242: SENTRY-1035: Generic service does not handle
group name casing correctly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/
-----------------------------------------------------------
Review request for sentry and Gregory Chanan.
Repository: sentry
Description
-------
Group names are case insensitive, handle them as such in list apis.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
Diff: https://reviews.apache.org/r/43242/diff/
Testing
-------
Test already present, tweaked it to use mixed case.
Thanks,
Sravya Tirukkovalur
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review118207
-----------------------------------------------------------
Ship it!
- Lenni Kuff
On Feb. 5, 2016, 6:21 p.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 6:21 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case insensitive, handle them as such in list apis.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Test already present, tweaked it to use mixed case.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review118496
-----------------------------------------------------------
Ship it!
Only one minor issue, LGTM +1.
- Hao Hao
On Feb. 5, 2016, 6:21 p.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 6:21 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case insensitive, handle them as such in list apis.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Test already present, tweaked it to use mixed case.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review118508
-----------------------------------------------------------
Fix it, then Ship it!
Fix the few comments and ship it.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java (line 219)
<https://reviews.apache.org/r/43242/#comment179764>
Comment on what is the expected results.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java (line 223)
<https://reviews.apache.org/r/43242/#comment179762>
Remove the space.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java (line 225)
<https://reviews.apache.org/r/43242/#comment179761>
Alignment.
- Hao Hao
On Feb. 9, 2016, 11:59 p.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2016, 11:59 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case sensitive, handle them as such Generic model
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java e1c15fa5304b553f69ef4d7e5053d587efb92ae5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Existing tests pass, and also added a new test.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/
-----------------------------------------------------------
(Updated Feb. 10, 2016, 12:22 a.m.)
Review request for sentry and Gregory Chanan.
Changes
-------
Updated based on comments
Repository: sentry
Description
-------
Group names are case sensitive, handle them as such Generic model
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java e1c15fa5304b553f69ef4d7e5053d587efb92ae5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
Diff: https://reviews.apache.org/r/43242/diff/
Testing
-------
Existing tests pass, and also added a new test.
Thanks,
Sravya Tirukkovalur
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/
-----------------------------------------------------------
(Updated Feb. 9, 2016, 11:59 p.m.)
Review request for sentry and Gregory Chanan.
Changes
-------
As per the comment on jira, updated groups to be case sensitive. Looks like HDFS enforces case sensitivity for group names, seems like it is best to be consistent here. In future, we might want to consider providing a way to configure this if there is a use case.
Repository: sentry
Description (updated)
-------
Group names are case sensitive, handle them as such Generic model
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java e1c15fa5304b553f69ef4d7e5053d587efb92ae5
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
Diff: https://reviews.apache.org/r/43242/diff/
Testing (updated)
-------
Existing tests pass, and also added a new test.
Thanks,
Sravya Tirukkovalur
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review118495
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java (line 131)
<https://reviews.apache.org/r/43242/#comment179748>
Can we just remove this line, since the flag is no longer needed.
- Hao Hao
On Feb. 5, 2016, 6:21 p.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 6:21 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case insensitive, handle them as such in list apis.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Test already present, tweaked it to use mixed case.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 6:21 p.m.)
Review request for sentry and Gregory Chanan.
Changes
-------
Changed trimed -> trimmed
Repository: sentry
Description
-------
Group names are case insensitive, handle them as such in list apis.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
Diff: https://reviews.apache.org/r/43242/diff/
Testing
-------
Test already present, tweaked it to use mixed case.
Thanks,
Sravya Tirukkovalur
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On Feb. 5, 2016, 8:36 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java, line 481
> > <https://reviews.apache.org/r/43242/diff/1/?file=1235971#file1235971line481>
> >
> > Are group names really case-insensitive? Linux groups are usually case-insensitive, AD groups are as well.
> >
> > While you are here can you fix toTrimedLower to spell "trimmed" properly?
I did some reading about it, and it looks like AD and OpenLap are by default case insensitive, but can be configured otherwise. But looks like Hadoop is case sensitive :-). So not sure which standard we should follow. Thoughts?
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review117990
-----------------------------------------------------------
On Feb. 5, 2016, 6:21 p.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 6:21 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case insensitive, handle them as such in list apis.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Test already present, tweaked it to use mixed case.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Lenni Kuff <ls...@cloudera.com>.
> On Feb. 5, 2016, 8:36 a.m., Lenni Kuff wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java, line 481
> > <https://reviews.apache.org/r/43242/diff/1/?file=1235971#file1235971line481>
> >
> > Are group names really case-insensitive? Linux groups are usually case-insensitive, AD groups are as well.
> >
> > While you are here can you fix toTrimedLower to spell "trimmed" properly?
>
> Sravya Tirukkovalur wrote:
> I did some reading about it, and it looks like AD and OpenLap are by default case insensitive, but can be configured otherwise. But looks like Hadoop is case sensitive :-). So not sure which standard we should follow. Thoughts?
Given that Sentry treats them as case-insensitive in other places (db model), we can probably do the same here. Should add a TODO and file a follow on JIRA though. What do you think?
- Lenni
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review117990
-----------------------------------------------------------
On Feb. 5, 2016, 6:21 p.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 6:21 p.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case insensitive, handle them as such in list apis.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Test already present, tweaked it to use mixed case.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>
Re: Review Request 43242: SENTRY-1035: Generic service does not
handle group name casing correctly
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43242/#review117990
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java (line 481)
<https://reviews.apache.org/r/43242/#comment179294>
Are group names really case-insensitive? Linux groups are usually case-insensitive, AD groups are as well.
While you are here can you fix toTrimedLower to spell "trimmed" properly?
- Lenni Kuff
On Feb. 5, 2016, 7:08 a.m., Sravya Tirukkovalur wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43242/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 7:08 a.m.)
>
>
> Review request for sentry and Gregory Chanan.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Group names are case insensitive, handle them as such in list apis.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericPolicyProcessor.java 78d38473f938b1aa2b357dd858b00f833147ee5d
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java f1a87a83a97f3e2d7d06462abaaf4b983ab17677
>
> Diff: https://reviews.apache.org/r/43242/diff/
>
>
> Testing
> -------
>
> Test already present, tweaked it to use mixed case.
>
>
> Thanks,
>
> Sravya Tirukkovalur
>
>