You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2016/12/06 23:05:26 UTC
Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET upsets
Sentry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I have made changes assuming that grant option is either true/false removing unset.
Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
Diffs
-----
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java c056bcc
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 742798d
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 7, 2016, 12:56 a.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java, line 16
> > <https://reviews.apache.org/r/54454/diff/1/?file=1578184#file1578184line16>
> >
> > This file is auto-generated by Thrift, I don't think we should touch these.
>
> kalyan kumar kalvagadda wrote:
> Sasha,
>
> Are you sure that all of the file is auto generated? I thought creates a template file and let lets update it.
Here is what is says at the top:
/**
* Autogenerated by Thrift Compiler (0.9.3)
*
* DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
* @generated
*/
Do you have any reasons to think otherwise?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review158278
-----------------------------------------------------------
On Dec. 14, 2016, 10:38 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:38 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I have made changes assuming that grant option is either true/false removing unset.
> Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Dec. 7, 2016, 12:56 a.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1431
> > <https://reviews.apache.org/r/54454/diff/1/?file=1578186#file1578186line1431>
> >
> > Why do you want to leave this line here?
I will remove the line
> On Dec. 7, 2016, 12:56 a.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1432
> > <https://reviews.apache.org/r/54454/diff/1/?file=1578186#file1578186line1432>
> >
> > Why privilege.setActionIsSet(false) is called here?
It was a type. It should have been privilege.setGrantOptionIsSet(false)
> On Dec. 7, 2016, 12:56 a.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1453
> > <https://reviews.apache.org/r/54454/diff/1/?file=1578186#file1578186line1453>
> >
> > Why do you want to leave the old code behind?
There is no reason. Just thought of to removing it after the review is done.
> On Dec. 7, 2016, 12:56 a.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java, line 16
> > <https://reviews.apache.org/r/54454/diff/1/?file=1578184#file1578184line16>
> >
> > This file is auto-generated by Thrift, I don't think we should touch these.
Sasha,
Are you sure that all of the file is auto generated? I thought creates a template file and let lets update it.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review158278
-----------------------------------------------------------
On Dec. 6, 2016, 11:05 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 11:05 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I have made changes assuming that grant option is either true/false removing unset.
> Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java c056bcc
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 742798d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review158278
-----------------------------------------------------------
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java (line 16)
<https://reviews.apache.org/r/54454/#comment229050>
This file is auto-generated by Thrift, I don't think we should touch these.
sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java (line 981)
<https://reviews.apache.org/r/54454/#comment229051>
This is also auto-generated file
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1431)
<https://reviews.apache.org/r/54454/#comment229052>
Why do you want to leave this line here?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1432)
<https://reviews.apache.org/r/54454/#comment229053>
Why privilege.setActionIsSet(false) is called here?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1453)
<https://reviews.apache.org/r/54454/#comment229054>
Why do you want to leave the old code behind?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1720)
<https://reviews.apache.org/r/54454/#comment229056>
MSentryPrivilege is generated by Sentry code itself so it is weird to complain about invalid privilege here. I guess the check should be for the original privilege instead.
And the message probably should be something like
"grant option is not specified"
- Alexander Kolbasov
On Dec. 6, 2016, 11:05 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 11:05 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I have made changes assuming that grant option is either true/false removing unset.
> Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java c056bcc
> sentry-service/sentry-service-common/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java 15b339f
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 742798d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 1052
> > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1052>
> >
> > I normally follow a convention of having a single return at the end. Do you see any performence issue here?
It isn't about performance but baout readability and simplicity. With return you know that you exit the function, otherwise you need to mentally follow what happens next. What benefit do you get from a single return at the end?
> On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 1059
> > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1059>
> >
> > Purpose of both the functions is same. They just differ in the arguments provided.
> >
> > This is a perfect place to use overloaded functions. Why do you think it would be confusing?
Purpose is quite diffetent - one validates a single privilege, another validates multiple privileges. In this case using the same name for both is just confusing.
> On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 1060
> > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1060>
> >
> > I wanted this vaidation functions as modular as possible so that they can be re-used. That was the intent behind having seperate functions for TSentryPrivilege and Set<TSentryPrivilege>.
Can you clarify how do you envision such reuse? In what context it would be reused? Anyway, if you think that it is better to split this into two functions, this is ok as well.
> On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 1081
> > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1081>
> >
> > I agree, this code can be inlined in above function. I have seperated it for reason. This way this utility function are more re-usable. If some part of the code wants to check the mandatory feilds in a single privilege they can just call this function.
> >
> > If is not modular people might come up other functions to vaidate privilege if the validations are to be enchanced in future.
This is Ok to have it in separate functions if you think that it is useful - but what code do envision will be using it?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review161876
-----------------------------------------------------------
On Jan. 17, 2017, 7:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 7:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632ddef6ac5eec7b0e240c596f57f427fda9d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Jan. 17, 2017, 6:06 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 1052
> > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1052>
> >
> > I normally follow a convention of having a single return at the end. Do you see any performence issue here?
>
> Alexander Kolbasov wrote:
> It isn't about performance but baout readability and simplicity. With return you know that you exit the function, otherwise you need to mentally follow what happens next. What benefit do you get from a single return at the end?
Having it this ways helps in debugging as there is only one place where the function exists. I have anyway taken a different approach for the issue in the new patch.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review161876
-----------------------------------------------------------
On Feb. 28, 2017, 12:58 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:58 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review161876
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 290)
<https://reviews.apache.org/r/54454/#comment233166>
Code assumes that there will be atleast from privilege. We have some validations added to make sure of it.
I will raise another jira for that.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1045)
<https://reviews.apache.org/r/54454/#comment233158>
There are no tests using this function. I initially thought of adding tests for them so I mde them visible for tests.
I later dropped that idea. I will remove it.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1047)
<https://reviews.apache.org/r/54454/#comment233159>
There will be alteast one privilage in the set.
Any way, this function is called after valid performing below check.
if(request.isSetPrivileges() && (!request.getPrivileges().isEmpty()))
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1050)
<https://reviews.apache.org/r/54454/#comment233168>
I normally follow a convention of having a single return at the end. Do you see any performence issue here?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1057)
<https://reviews.apache.org/r/54454/#comment233169>
Purpose of both the functions is same. They just differ in the arguments provided.
This is a perfect place to use overloaded functions. Why do you think it would be confusing?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1058)
<https://reviews.apache.org/r/54454/#comment233165>
I wanted this vaidation functions as modular as possible so that they can be re-used. That was the intent behind having seperate functions for TSentryPrivilege and Set<TSentryPrivilege>.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1059)
<https://reviews.apache.org/r/54454/#comment233161>
privileges set will have al least one element.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1079)
<https://reviews.apache.org/r/54454/#comment233155>
I agree, this code can be inlined in above function. I have seperated it for reason. This way this utility function are more re-usable. If some part of the code wants to check the mandatory feilds in a single privilege they can just call this function.
If is not modular people might come up other functions to vaidate privilege if the validations are to be enchanced in future.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1083)
<https://reviews.apache.org/r/54454/#comment233156>
Scope of these changes are limited to SENTRY-1547 and SENTRY-1548.
We are not really validating the actions here.
We have seperate jira to addresss that.
- kalyan kumar kalvagadda
On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 4:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Jan. 5, 2017, 6:59 p.m., Vamsee Yarlagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, lines 1082-1085
> > <https://reviews.apache.org/r/54454/diff/4/?file=1589127#file1589127line1082>
> >
> > Can't we leverage SentryStore.isNull() ? Or Strings.isNullOrEmpty()?
isNullOrEmpty can be used if we were not considering leading and trailing whitespace characters.
Current code added would handle a case where the server name OR action feilds are just whitespace characters it would return failure.
This can not be done with isNullOrEmpty.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review160612
-----------------------------------------------------------
On Feb. 28, 2017, 12:58 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:58 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review160612
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1065)
<https://reviews.apache.org/r/54454/#comment231744>
nit. typo
Feilds -> Fields
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1077)
<https://reviews.apache.org/r/54454/#comment231745>
nit. Same typo as stated above.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (lines 1080 - 1083)
<https://reviews.apache.org/r/54454/#comment231748>
Can't we leverage SentryStore.isNull() ? Or Strings.isNullOrEmpty()?
- Vamsee Yarlagadda
On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 4:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review161897
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (lines 1080 - 1083)
<https://reviews.apache.org/r/54454/#comment233174>
isNullOrEmpty can be used if we were not considering leading and trailing whitespace characters.
Current code added would handle a case where the server name OR action feilds are just whitespace characters it would return failure.
This can not be done with isNullOrEmpty.
- kalyan kumar kalvagadda
On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 4:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review167621
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 36 (patched)
<https://reviews.apache.org/r/54454/#comment239527>
There is no reason this should be a singleton. In fact, it should only have static methods. Same goes for other validator classes.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java
Lines 32 (patched)
<https://reviews.apache.org/r/54454/#comment239525>
Looking at the way it is used and defined I don't see any value in creating sucb abstract class. The validate() method can't be generalized since you can't subclass thrift methods, so it isn't very useful. The only thing you need to share is the validateVersion method, so you can just have a class that implements that as a static method - and there is no need for inheritance at all.
- Alexander Kolbasov
On Feb. 28, 2017, 2:21 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 2:21 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1548
> https://issues.apache.org/jira/browse/SENTRY-1548
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/54454/diff/6/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review167653
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
Lines 335 (patched)
<https://reviews.apache.org/r/54454/#comment239562>
Here is an example - you call specific validator's getInstance() and then validate. It is simpler to just call static validation routine of this validator here - the abstraction doesn't buy you anything here.
- Alexander Kolbasov
On Feb. 28, 2017, 2:21 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 2:21 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1548
> https://issues.apache.org/jira/browse/SENTRY-1548
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/54454/diff/6/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review167795
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 29 (patched)
<https://reviews.apache.org/r/54454/#comment239765>
The comment reflects the old code, not the current one
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 32 (patched)
<https://reviews.apache.org/r/54454/#comment239766>
can be final
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 36 (patched)
<https://reviews.apache.org/r/54454/#comment239767>
Remove "this method".
"Validate privileges within the input request"
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 37 (patched)
<https://reviews.apache.org/r/54454/#comment239768>
s/invoked/invokes/
"Each privilege should have a non-empty server name and action."
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 39 (patched)
<https://reviews.apache.org/r/54454/#comment239769>
There is no need to specify type in the javadoc - it is available in the code.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 40 (patched)
<https://reviews.apache.org/r/54454/#comment239770>
@throws SentryInvalidInputException if one of the rivileges is missing mandatory fields or the grant option of the privilege is invalid
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 44 (patched)
<https://reviews.apache.org/r/54454/#comment239771>
extra empty line
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 45 (patched)
<https://reviews.apache.org/r/54454/#comment239772>
It was already documented above - is there a need to repeat?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 47 (patched)
<https://reviews.apache.org/r/54454/#comment239773>
space after if
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 52 (patched)
<https://reviews.apache.org/r/54454/#comment239774>
missing newline
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 53 (patched)
<https://reviews.apache.org/r/54454/#comment239775>
Remove "This function"
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 56 (patched)
<https://reviews.apache.org/r/54454/#comment239776>
@throws
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 58 (patched)
<https://reviews.apache.org/r/54454/#comment239777>
This function seems to be the same as the one in RevokePrivilegeRequestValidator() - is it possible to just call it?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 71 (patched)
<https://reviews.apache.org/r/54454/#comment239778>
new line
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 72 (patched)
<https://reviews.apache.org/r/54454/#comment239779>
Remove "This function"
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 76 (patched)
<https://reviews.apache.org/r/54454/#comment239780>
Add @throws javadoc
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 78 (patched)
<https://reviews.apache.org/r/54454/#comment239781>
space before {
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 80 (patched)
<https://reviews.apache.org/r/54454/#comment239782>
space after if
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 82 (patched)
<https://reviews.apache.org/r/54454/#comment239783>
s/grant/GRANT/
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java
Lines 27 (patched)
<https://reviews.apache.org/r/54454/#comment239784>
Stale comment
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java
Lines 31 (patched)
<https://reviews.apache.org/r/54454/#comment239785>
can be final
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java
Lines 35 (patched)
<https://reviews.apache.org/r/54454/#comment239786>
See comments for the GrantPrivilegeRequestValidator - most of them apply here.
- Alexander Kolbasov
On March 2, 2017, 2:44 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 2:44 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1548
> https://issues.apache.org/jira/browse/SENTRY-1548
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/54454/diff/7/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated March 3, 2017, 9:04 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed final comments.
Bugs: SENTRY-1548
https://issues.apache.org/jira/browse/SENTRY-1548
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
Diff: https://reviews.apache.org/r/54454/diff/9/
Changes: https://reviews.apache.org/r/54454/diff/8-9/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review167848
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 42 (patched)
<https://reviews.apache.org/r/54454/#comment239816>
Usual convention is to use "If" rather then "When"
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 66 (patched)
<https://reviews.apache.org/r/54454/#comment239817>
it may be better to replace ',' with ':'
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java
Lines 70 (patched)
<https://reviews.apache.org/r/54454/#comment239818>
replace ',' with ':'
- Alexander Kolbasov
On March 3, 2017, 4:41 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 4:41 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1548
> https://issues.apache.org/jira/browse/SENTRY-1548
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/54454/diff/8/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated March 3, 2017, 4:41 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed reciew comments.
Bugs: SENTRY-1548
https://issues.apache.org/jira/browse/SENTRY-1548
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
Diff: https://reviews.apache.org/r/54454/diff/8/
Changes: https://reviews.apache.org/r/54454/diff/7-8/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated March 2, 2017, 2:44 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed sasha's review comments.
Bugs: SENTRY-1548
https://issues.apache.org/jira/browse/SENTRY-1548
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
Diff: https://reviews.apache.org/r/54454/diff/7/
Changes: https://reviews.apache.org/r/54454/diff/6-7/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 2:21 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Changed th branch and Bug fields.
Bugs: SENTRY-1548
https://issues.apache.org/jira/browse/SENTRY-1548
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 12:58 a.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
I have taken a different approach this time. I have added validator class for validating requests.
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Jan. 22, 2017, 6:46 a.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 288
> > <https://reviews.apache.org/r/54454/diff/5/?file=1606701#file1606701line288>
> >
> > How do you know that the value is UNSET? May be it was 42?
TSentryGrantOption is enum. I will can hold one of these values (1, 0, -1).
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review162568
-----------------------------------------------------------
On Feb. 28, 2017, 12:58 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:58 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review162568
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 281)
<https://reviews.apache.org/r/54454/#comment233880>
Please add a space between // and a comment
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 283)
<https://reviews.apache.org/r/54454/#comment233881>
Other code here uses convention
if (condition) {
...
}
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 286)
<https://reviews.apache.org/r/54454/#comment233882>
The code knows what exactly is missing - so why say that server name [OR] Action are missing? Why not just say what exactly is missing?
ALso this line is too long
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 288)
<https://reviews.apache.org/r/54454/#comment233884>
How do you know that the value is UNSET? May be it was 42?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 289)
<https://reviews.apache.org/r/54454/#comment233883>
This line is too long.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 359)
<https://reviews.apache.org/r/54454/#comment233886>
Same comments as above.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1050)
<https://reviews.apache.org/r/54454/#comment233888>
Would it be useful to allow any iterable collection rather than a set?
Please document what is 'validation'
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1052)
<https://reviews.apache.org/r/54454/#comment233889>
A boolean value is, by definition, true/false.
You can just say
@return true if the validation is successfull, false otherwise
That said, this interface makes it difficult to tell exactly what is wrong - you can't pass this information fro the validator.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1056)
<https://reviews.apache.org/r/54454/#comment233890>
All these validators can be private
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1057)
<https://reviews.apache.org/r/54454/#comment233891>
It will be shorter and cleaner if you just remove the ret boolean and return the needed values directly
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1073)
<https://reviews.apache.org/r/54454/#comment233892>
As we discussed earlier, use of overloading here just creates confusion
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1075)
<https://reviews.apache.org/r/54454/#comment233893>
See http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-137265.html#333
A better way to write this is
return condition
Will Thrift validate that the grant option can't have a value like 42?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1082)
<https://reviews.apache.org/r/54454/#comment233894>
Please describe which fields are mandatory
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1084)
<https://reviews.apache.org/r/54454/#comment233895>
See the comment above about boolean return values
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1089)
<https://reviews.apache.org/r/54454/#comment233896>
See comment above about direct returns
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1102)
<https://reviews.apache.org/r/54454/#comment233897>
This interface makes it difficult to tell what exact field is missing
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1108)
<https://reviews.apache.org/r/54454/#comment233898>
This is better written as
return condition
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1110)
<https://reviews.apache.org/r/54454/#comment233900>
It is usuallu better to use isEmpty instead of length()
- Alexander Kolbasov
On Jan. 17, 2017, 7:28 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 7:28 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632ddef6ac5eec7b0e240c596f57f427fda9d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 7:28 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Added code changes to address the review commants.
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632ddef6ac5eec7b0e240c596f57f427fda9d
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review160628
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (lines 1080 - 1083)
<https://reviews.apache.org/r/54454/#comment231767>
There is no string funtion that can perform a null check for you. Best way is to do perform a null check.
- kalyan kumar kalvagadda
On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 4:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Jan. 6, 2017, 12:27 a.m., Alexander Kolbasov wrote:
> > The change seems to check for other privilege correctness in addition to UNSET grant option - are you adding a fix for another problem here as well?
This change set has fix for SENTRY-1548 as well.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review160654
-----------------------------------------------------------
On Feb. 28, 2017, 12:58 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:58 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review160654
-----------------------------------------------------------
The change seems to check for other privilege correctness in addition to UNSET grant option - are you adding a fix for another problem here as well?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 285)
<https://reviews.apache.org/r/54454/#comment231813>
It would be helpful to report what exactly is missing
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 286)
<https://reviews.apache.org/r/54454/#comment231814>
You don't need else clause here since the throw above will terminate, so you can just write this as two ifs.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 290)
<https://reviews.apache.org/r/54454/#comment231815>
Do we need to do anything here if the privilege set is empty?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1045)
<https://reviews.apache.org/r/54454/#comment231805>
Is there a test that uses it? Why is it visible for testing?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1046)
<https://reviews.apache.org/r/54454/#comment231804>
Please add javadoc for this function
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1047)
<https://reviews.apache.org/r/54454/#comment231807>
Can privileges be null here?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1050)
<https://reviews.apache.org/r/54454/#comment231803>
Why not just "return false"?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1057)
<https://reviews.apache.org/r/54454/#comment231800>
Use of overloading here is confusing - can you choose different names for these functions?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1058)
<https://reviews.apache.org/r/54454/#comment231801>
A simple
return privilege.getGrantOption() != TSentryGrantOption.UNSET
works just fine. And given that it is now a one-liner - do we really need a function for it? This will remove the problem with overloaded names as well.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1059)
<https://reviews.apache.org/r/54454/#comment231808>
Can privilege be null here?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1064)
<https://reviews.apache.org/r/54454/#comment231806>
Same comments as for validateGrantOption above.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1079)
<https://reviews.apache.org/r/54454/#comment231809>
Can it really be null?
This can be inlined directly in checkforMandatoryPrivilegeFeilds() above as
privilege.getServerName() != null && privilege.getServerName().trim().length() > 0 &&
privilege.getAction() != null &&
privilege.getAction().trim().length() > 0)
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1083)
<https://reviews.apache.org/r/54454/#comment231811>
Do we allow any action as long as it is non-empty? Other code makes certain assumption about valid set of actions.
- Alexander Kolbasov
On Jan. 5, 2017, 4:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 4:50 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Jan. 5, 2017, 4:50 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
This diff address both SENTRY-1547 and SENTRY-1548, making sure all the mandatory feilds are present in the requests and also validates the UNSET option.
Repository: sentry
Description (updated)
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Dec. 20, 2016, 12:07 a.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Optimized previous patch.
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I have made changes assuming that grant option is either true/false removing unset.
Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Dec. 19, 2016, 11:25 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
I'm working on SENTRY-1547 and SENTRY-1548. Fixes for both of them are in place. Last patch was bit confusing as weel as I had to remove the changes done for SENTRY-1547.
This diff has changes for both of them.
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I have made changes assuming that grant option is either true/false removing unset.
Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On Dec. 14, 2016, 11:21 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 280
> > <https://reviews.apache.org/r/54454/diff/2/?file=1585183#file1585183line280>
> >
> > can getPrivileges() return null?
alter_sentry_role_grant_privilege should have atleast one privilege. I was not able to run grant with privilage from CLI. Let me check the code to me sure of it.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review159250
-----------------------------------------------------------
On Feb. 28, 2017, 12:58 a.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 12:58 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I'm working on SENTRY-1547 and SENTRY-1548. Fixe for both the issues should be addressed together. Last patch was bit confusing as I had to remove the changes done for SENTRY-1547. This diff has changes for both of them.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b10c2f2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/GrantPrivilegeRequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RequestValidator.java PRE-CREATION
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/validator/RevokePrivilegeRequestValidator.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review159250
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 280)
<https://reviews.apache.org/r/54454/#comment230195>
can getPrivileges() return null?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 281)
<https://reviews.apache.org/r/54454/#comment230196>
This is the same code as below - can it be handled by a common func?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 282)
<https://reviews.apache.org/r/54454/#comment230197>
if (!validateGrantOption(request.getPrivilege()) {
...
}
There is no point in comparing booleans to true/false
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 283)
<https://reviews.apache.org/r/54454/#comment230198>
can this be moe specific like "grant option is unset" or something like this?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 286)
<https://reviews.apache.org/r/54454/#comment230199>
else if {
}
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 1066)
<https://reviews.apache.org/r/54454/#comment230194>
It is a bit confusing for these two functions to share the same name.
Also, given that this can be simply written as
return privilege.getGrantOption() != TSentryGrantOption.UNSET
do we need a function for that at all?
- Alexander Kolbasov
On Dec. 14, 2016, 10:38 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:38 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I have made changes assuming that grant option is either true/false removing unset.
> Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/#review159494
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 281)
<https://reviews.apache.org/r/54454/#comment230503>
I'm working on SENTRY-1547 and SENTRY-1548. Fixes for both of them are in place. I had to manually split them to create seperate diff files. I messed it. One of them would be request.getPrivileges().
I will probably publish complete diff and appropriately docuement just for review.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 282)
<https://reviews.apache.org/r/54454/#comment230504>
Agree. I will change it
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 283)
<https://reviews.apache.org/r/54454/#comment230505>
Will change the description.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 286)
<https://reviews.apache.org/r/54454/#comment230506>
I'm working on SENTRY-1547 and SENTRY-1548. Fixes for both of them are in place.
I will probably publish complete diff and appropriately docuement just for review.
- kalyan kumar kalvagadda
On Dec. 14, 2016, 10:38 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54454/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 10:38 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
>
> I have made changes assuming that grant option is either true/false removing unset.
> Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
>
> Diff: https://reviews.apache.org/r/54454/diff/
>
>
> Testing
> -------
>
> Verfied the changes using sentry thrift client.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 54454: SENTRY-1548 Setting GrantOption to UNSET
upsets Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54454/
-----------------------------------------------------------
(Updated Dec. 14, 2016, 10:38 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
I had to address the review comments so I had to come up with a differnt fix.
UNSET option for grant is valid is only valid while revoking the privileges.
Added code changes to Sentry Policy Processor to handle the same.
Repository: sentry
Description
-------
SENTRY-1548 Setting GrantOption to UNSET upsets Sentry
I have made changes assuming that grant option is either true/false removing unset.
Also, added code so that sentry server could validate the TSentryPrivilege object constructed from the Thrift message received client. If the validation is failed exception is raised and appropriate error is message is sent.
Diffs (updated)
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 898632d
Diff: https://reviews.apache.org/r/54454/diff/
Testing
-------
Verfied the changes using sentry thrift client.
Thanks,
kalyan kumar kalvagadda