Details

    • Sub-task
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 2.1.0
    • 2.2.0
    • Sentry
    • None

    Description

      Thrift interface between sentry server and hdfs sentry client should be updated to send privileges granted to users.

      Attachments

        1. SENTRY-2170.001.patch
          105 kB
          Krishna Kalyan
        2. SENTRY-2170.002.patch
          62 kB
          Krishna Kalyan
        3. SENTRY-2170.003.patch
          63 kB
          Krishna Kalyan
        4. SENTRY-2170.003.patch
          63 kB
          Krishna Kalyan

        Issue Links

          Activity

            kkalyan Krishna Kalyan added a comment - - edited

            This include making changes to TPrivilegeChanges class.

             

            enum TPrivilegeEntityType

            { ROLE, USER, OBJECT }

            New Definitions

            struct TPrivilegeEntity {
             # Type of the privilege entity
             1: required TPrivilegeEntityType type;
            
             # Value of entity
             2: required string value;
             }
            
            

             

            Here is the an initial thought on changes to TPrivilegeChanges.

            struct TPrivilegeChanges {

            1. The authorizable object that needs to be updated.
              1: required string authzObj;
            1. The privileges that needs to be added to
            2. the authorizable object.
              2: required map<TPrivilegeEntity, string> addPrivileges;
            1. The privileges that needs to be deleted to
            2. the authorizable object.
              3: required map<TPrivilegeEntity, string> delPrivileges;
              }
            kkalyan Krishna Kalyan added a comment - - edited This include making changes to TPrivilegeChanges class.   enum TPrivilegeEntityType { ROLE, USER, OBJECT } New Definitions struct TPrivilegeEntity { # Type of the privilege entity 1: required TPrivilegeEntityType type; # Value of entity 2: required string value; }   Here is the an initial thought on changes to TPrivilegeChanges. struct TPrivilegeChanges { The authorizable object that needs to be updated. 1: required string authzObj; The privileges that needs to be added to the authorizable object. 2: required map< TPrivilegeEntity , string> addPrivileges; The privileges that needs to be deleted to the authorizable object. 3: required map< TPrivilegeEntity , string> delPrivileges; }
            hadoopqa Hadoop QA added a comment -

            Here are the results of testing the latest attachment
            https://issues.apache.org/jira/secure/attachment/12915785/SENTRY-2170.001.patch against master.

            Overall: +1 all checks pass

            SUCCESS: all tests passed

            Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3710/console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12915785/SENTRY-2170.001.patch against master. Overall: +1 all checks pass SUCCESS: all tests passed Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3710/console This message is automatically generated.
            kkalyan Krishna Kalyan added a comment -

            Here is the proposal

             

            Currently TPrivilegeChanges is created to hold all the privileges that are granted to an object.

             

            Case-1: When a snapshot is taken.

            All the privileges granted to a object are added to addPrivileges.

            Case-2: When a new privileges is granted

            New privileges granted it added to addPrivileges

            Case-3: When a privileges is revoked

            Privileges that is revoked is added to delPrivileges

            Both addPrivileges/delPrivileges have role to permission mapping. We could extend this mapping to send user permissions as well.

             
            New Definitions

            enum TPrivilegeEntityType
            { ROLE, USER, OBJECT }
            
            struct TPrivilegeEntity {
             # Type of the privilege entity
             1: required TPrivilegeEntityType type;
            
             # Value of entity
             2: required string value;
             }
            
            

             

            struct TPrivilegeChanges {

            1. The authorizable object that needs to be updated.
              1: required string authzObj;
            1. The privileges that needs to be added to
            2. the authorizable object.
              2: required map<TPrivilegeEntity, string> addPrivileges;
            1. The privileges that needs to be deleted to
            2. the authorizable object.
              3: required map<TPrivilegeEntity, string> delPrivileges;
              }

             

            kkalyan Krishna Kalyan added a comment - Here is the proposal   Currently TPrivilegeChanges is created to hold all the privileges that are granted to an object.   Case-1: When a snapshot is taken. All the privileges granted to a object are added to addPrivileges. Case-2: When a new privileges is granted New privileges granted it added to addPrivileges Case-3 : When a privileges is revoked Privileges that is revoked is added to delPrivileges Both addPrivileges/delPrivileges have role to permission mapping. We could extend this mapping to send user permissions as well.   New Definitions enum TPrivilegeEntityType { ROLE, USER, OBJECT } struct TPrivilegeEntity { # Type of the privilege entity 1: required TPrivilegeEntityType type; # Value of entity 2: required string value; }   struct TPrivilegeChanges { The authorizable object that needs to be updated. 1: required string authzObj; The privileges that needs to be added to the authorizable object. 2: required map< TPrivilegeEntity , string> addPrivileges; The privileges that needs to be deleted to the authorizable object. 3: required map< TPrivilegeEntity , string> delPrivileges; }  
            kkalyan Krishna Kalyan added a comment -

            Tagging LinaAtAustin and spena to comment.

            kkalyan Krishna Kalyan added a comment - Tagging LinaAtAustin and spena to comment.
            linaataustin Na Li added a comment -

            why does " TPrivilegeEntityType { ROLE, USER, OBJECT }" include "Object"? If it is for future use, we can add it in the future.

            linaataustin Na Li added a comment - why does " TPrivilegeEntityType { ROLE, USER, OBJECT }" include "Object"? If it is for future use, we can add it in the future.
            kkalyan Krishna Kalyan added a comment -

            LinaAtAustin it's not for future purpose. we need it currently.

            we handle rename bit differently when there isa privilege that is renamed, TPrivilegeChanges is updated differently.

            1. authzObj will be initialized with a constant string "_RENAME_PRIV_"
            2. addPrivileges will have (newAuthz, newAuthz)
            3. delPrivileges will have (oldAuthz, oldAuthz)

            This can be done in a better way. I will update this in different commit.

            In short to your question, we need that to handle renaming of privileges.

            kkalyan Krishna Kalyan added a comment - LinaAtAustin it's not for future purpose. we need it currently. we handle rename bit differently when there isa privilege that is renamed, TPrivilegeChanges is updated differently. authzObj will be initialized with a constant string "_ RENAME_PRIV _" addPrivileges will have (newAuthz, newAuthz) delPrivileges will have (oldAuthz, oldAuthz) This can be done in a better way. I will update this in different commit. In short to your question, we need that to handle renaming of privileges.
            kkalyan Krishna Kalyan added a comment -

            spena and LinaAtAustin

            (Full Snapshot) PermissionsUpdate -> TPermissionsUpdate -> Collection<TPrivilegeChanges>
            (Del Update) List<PermissionsUpdate> -> PermissionsUpdate -> TPermissionsUpdate -> TPrivilegeChanges
            One instance of TPrivilegeChanges has the permission information about a HMS object
            
            1. Retrieving full/Delta snapshot from persistent store. Construct 
                1. (Full Snapshot)PermissionsImage
                    1. Role Image
                    2. Object ->Role,permission mapping
                    3. Create thrift version to send stover to NN pug-in
                        1. PermissionsUpdate
                            1. TPermissionsUpdate is updated with permissions and role information from PermissionsImage
                                1. TPrivilegeChanges (one instance per HMS object)
                                2. TRoleChanges(one Entry for each role which has role -> group Mapping)
                2. Retrieving Delta Update from persistent store. 
                    1. deltaRetriever#retrieveDelta
                    2. Get all the PermissionsUpdate entries from MSentryPermChange table
                        1. Each PermissionsUpdate object corresponds to one permission change.
            
            kkalyan Krishna Kalyan added a comment - spena and LinaAtAustin (Full Snapshot) PermissionsUpdate -> TPermissionsUpdate -> Collection<TPrivilegeChanges> (Del Update) List<PermissionsUpdate> -> PermissionsUpdate -> TPermissionsUpdate -> TPrivilegeChanges One instance of TPrivilegeChanges has the permission information about a HMS object 1. Retrieving full/Delta snapshot from persistent store. Construct 1. (Full Snapshot)PermissionsImage 1. Role Image 2. Object ->Role,permission mapping 3. Create thrift version to send stover to NN pug-in 1. PermissionsUpdate 1. TPermissionsUpdate is updated with permissions and role information from PermissionsImage 1. TPrivilegeChanges (one instance per HMS object) 2. TRoleChanges(one Entry for each role which has role -> group Mapping) 2. Retrieving Delta Update from persistent store. 1. deltaRetriever#retrieveDelta 2. Get all the PermissionsUpdate entries from MSentryPermChange table 1. Each PermissionsUpdate object corresponds to one permission change.
            linaataustin Na Li added a comment -

            kkalyan I agree with your design. The only thing to change is for TPrivilegeEntityType. Instead of

            { ROLE, USER, OBJECT }

            , how about

            { ROLE, USER, AUTHZ_OBJECT }

            or something that is more specific than OBJECT?

            linaataustin Na Li added a comment - kkalyan I agree with your design. The only thing to change is for TPrivilegeEntityType. Instead of { ROLE, USER, OBJECT } , how about { ROLE, USER, AUTHZ_OBJECT } or something that is more specific than OBJECT?
            hadoopqa Hadoop QA added a comment -

            Here are the results of testing the latest attachment
            https://issues.apache.org/jira/secure/attachment/12920887/SENTRY-2170.002.patch against master.

            Overall: +1 all checks pass

            SUCCESS: all tests passed

            Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3746/console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12920887/SENTRY-2170.002.patch against master. Overall: +1 all checks pass SUCCESS: all tests passed Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3746/console This message is automatically generated.
            akolb Alex Kolbasov added a comment -

            Do we need to preserve on the wire compatibility between old protocol and the new one?

            akolb Alex Kolbasov added a comment - Do we need to preserve on the wire compatibility between old protocol and the new one?
            kkalyan Krishna Kalyan added a comment -

            akolb I don't see a reason to maintain that compatibility unless sentry supports the use of different version of of sentry jars in client and server side. I'm not sure if it's valid use case.

            kkalyan Krishna Kalyan added a comment - akolb I don't see a reason to maintain that compatibility unless sentry supports the use of different version of of sentry jars in client and server side. I'm not sure if it's valid use case.
            spena Sergio Peña added a comment -

            The next version of Sentry will be 2.1.0, so being a minor version, I think we should think of compatibility when making changes on the API.  

            spena Sergio Peña added a comment - The next version of Sentry will be 2.1.0, so being a minor version, I think we should think of compatibility when making changes on the API.  
            kkalyan Krishna Kalyan added a comment -

            spena I understand that. Compatibility should be maintained while changing the API's that are exposed for other components to use. This is not the case now. Changing the thrift definitions does not effect the sentry client API's that are exposed to other components.

            kkalyan Krishna Kalyan added a comment - spena I understand that. Compatibility should be maintained while changing the API's that are exposed for other components to use. This is not the case now. Changing the thrift definitions does not effect the sentry client API's that are exposed to other components.
            hadoopqa Hadoop QA added a comment -

            Here are the results of testing the latest attachment
            https://issues.apache.org/jira/secure/attachment/12921623/SENTRY-2170.003.patch against master.

            Overall: -1 due to 2 errors

            ERROR: mvn test exited 1
            ERROR: Failed: org.apache.sentry.tests.e2e.dbprovider.TestDbPrivilegeCleanupOnDrop

            Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3757/console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12921623/SENTRY-2170.003.patch against master. Overall: -1 due to 2 errors ERROR: mvn test exited 1 ERROR: Failed: org.apache.sentry.tests.e2e.dbprovider.TestDbPrivilegeCleanupOnDrop Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3757/console This message is automatically generated.
            kkalyan Krishna Kalyan added a comment -

            See test failure. Test failure looks un-related. Will resubmit the patch again.

            kkalyan Krishna Kalyan added a comment - See test failure. Test failure looks un-related. Will resubmit the patch again.
            hadoopqa Hadoop QA added a comment -

            Here are the results of testing the latest attachment
            https://issues.apache.org/jira/secure/attachment/12922117/SENTRY-2170.003.patch against master.

            Overall: +1 all checks pass

            SUCCESS: all tests passed

            Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3766/console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12922117/SENTRY-2170.003.patch against master. Overall: +1 all checks pass SUCCESS: all tests passed Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/3766/console This message is automatically generated.

            People

              kkalyan Krishna Kalyan
              kkalyan Krishna Kalyan
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: