Uploaded image for project: 'Sentry'
  1. Sentry
  2. SENTRY-1119

Allow data engines to specify the ActionFactory from configuration

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.0
    • Fix Version/s: 1.7.0
    • Component/s: Service
    • Labels:
      None

      Description

      PrivilegeOperatePersistence uses a hard-coded map of component to ActionFactory to figure out the ActionFactory for a given component. This prevents external data engines from integrating easily with Sentry without modifying Sentry code.

      From mailing list discussion with Colin Ma

      I think the hardcoded map you pointed should be improved, make this to be configurable or remove this.
      
      1. SENTRY-1119.001.patch
        17 kB
        Bhooshan Mogal
      2. SENTRY-1119.002.patch
        19 kB
        Bhooshan Mogal
      3. SENTRY-1119.003.patch
        18 kB
        Bhooshan Mogal

        Issue Links

          Activity

          Hide
          bhooshan Bhooshan Mogal added a comment -

          Adding a patch to try and get the ActionFactory from configuration if it is not available in the map.

          Show
          bhooshan Bhooshan Mogal added a comment - Adding a patch to try and get the ActionFactory from configuration if it is not available in the map.
          Hide
          hadoopqa Hadoop QA added a comment -

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

          Overall: -1 due to an error

          ERROR: failed to build with patch (exit code 1)

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

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12791912/SENTRY-1119.001.patch against master. Overall: -1 due to an error ERROR: failed to build with patch (exit code 1) Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/1311/console This message is automatically generated.
          Hide
          colinma Colin Ma added a comment -

          Bhooshan Mogal, thanks for the patch, can you add the link for the review board?
          For this improvement, I prefer to add it to branch SENTRY-999 first.

          Show
          colinma Colin Ma added a comment - Bhooshan Mogal , thanks for the patch, can you add the link for the review board? For this improvement, I prefer to add it to branch SENTRY-999 first.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Colin Ma Thanks, created https://reviews.apache.org/r/44502/. I'm fine with this going into branch SENTRY-999. However, I would really like it to be part of 1.7.0.

          Show
          bhooshan Bhooshan Mogal added a comment - Colin Ma Thanks, created https://reviews.apache.org/r/44502/ . I'm fine with this going into branch SENTRY-999 . However, I would really like it to be part of 1.7.0.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Colin Ma could you please review when you have some time?

          Show
          bhooshan Bhooshan Mogal added a comment - Colin Ma could you please review when you have some time?
          Hide
          sravya Sravya Tirukkovalur added a comment -

          Left minor comments on the RB. Uber comment: I do think using component specific class in the sentry core is something we should definitely change. Tight coupling here is bad. I am not super familiar with this part of the code but are all other hooks configurable? We should make sure they are consistent in they way they are configured.

          Show
          sravya Sravya Tirukkovalur added a comment - Left minor comments on the RB. Uber comment: I do think using component specific class in the sentry core is something we should definitely change. Tight coupling here is bad. I am not super familiar with this part of the code but are all other hooks configurable? We should make sure they are consistent in they way they are configured.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Thanks for the review Sravya Tirukkovalur. I agree 100%. There shouldn't be any Solr, Kafka, Sqoop classes in the PrivilegeOperatorPersistence. I left the existing classes in that map as-is and did not enforce them to be configured via sentry-site.xml primarily to avoid having to fix existing tests in this PR. If we're ok with this strategy, I could create a follow up jira/patch to remove those as well, so at least action factories are not tightly coupled. The eventual goal should be that the sentry-provider-db module should not depend on sentry-core-model-search and sentry-core-model-sqoop (and in future sentry-core-model-kafka.

          I'm not familiar with the other hooks that you are talking about. Could you please elaborate?

          Show
          bhooshan Bhooshan Mogal added a comment - Thanks for the review Sravya Tirukkovalur . I agree 100%. There shouldn't be any Solr, Kafka, Sqoop classes in the PrivilegeOperatorPersistence . I left the existing classes in that map as-is and did not enforce them to be configured via sentry-site.xml primarily to avoid having to fix existing tests in this PR. If we're ok with this strategy, I could create a follow up jira/patch to remove those as well, so at least action factories are not tightly coupled. The eventual goal should be that the sentry-provider-db module should not depend on sentry-core-model-search and sentry-core-model-sqoop (and in future sentry-core-model-kafka . I'm not familiar with the other hooks that you are talking about. Could you please elaborate?
          Hide
          sravya Sravya Tirukkovalur added a comment -

          Moving all unresolved jiras with fix version 1.7.0 to 1.8.0. Please change the fix version if you intend to make it into 1.7.0 release.

          Show
          sravya Sravya Tirukkovalur added a comment - Moving all unresolved jiras with fix version 1.7.0 to 1.8.0. Please change the fix version if you intend to make it into 1.7.0 release.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Addressed comments from review board

          Show
          bhooshan Bhooshan Mogal added a comment - Addressed comments from review board
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Sravya Tirukkovalur I would really like to get this into 1.7.0.

          Show
          bhooshan Bhooshan Mogal added a comment - Sravya Tirukkovalur I would really like to get this into 1.7.0.
          Hide
          hadoopqa Hadoop QA added a comment -

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

          Overall: -1 due to an error

          ERROR: failed to build with patch (exit code 1)

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

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - Here are the results of testing the latest attachment https://issues.apache.org/jira/secure/attachment/12792344/SENTRY-1119.002.patch against master. Overall: -1 due to an error ERROR: failed to build with patch (exit code 1) Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/1324/console This message is automatically generated.
          Hide
          colinma Colin Ma added a comment -

          Bhooshan Mogal, left comment in the RB. The patch has some problems and can't pass the pre-commit.
          I think this shouldn't be a sub-task for SENTRY-999, change it to an isolated JIRA.

          Show
          colinma Colin Ma added a comment - Bhooshan Mogal , left comment in the RB. The patch has some problems and can't pass the pre-commit. I think this shouldn't be a sub-task for SENTRY-999 , change it to an isolated JIRA.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Colin Ma I think the problems are because per our earlier discussion, this is based off of the SENTRY-999 branch. I will rebase it off of master if it needs to go into master. After that, it should pass pre-commit.

          Show
          bhooshan Bhooshan Mogal added a comment - Colin Ma I think the problems are because per our earlier discussion, this is based off of the SENTRY-999 branch. I will rebase it off of master if it needs to go into master. After that, it should pass pre-commit.
          Hide
          colinma Colin Ma added a comment -

          Bhooshan Mogal, got it, let's make it based on master.

          Show
          colinma Colin Ma added a comment - Bhooshan Mogal , got it, let's make it based on master.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Updated patch after rebasing with master

          Show
          bhooshan Bhooshan Mogal added a comment - Updated patch after rebasing with master
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Colin Ma Added a new patch rebased with master. Also fixed your RB comment. Please review again.

          Show
          bhooshan Bhooshan Mogal added a comment - Colin Ma Added a new patch rebased with master. Also fixed your RB comment. Please review again.
          Hide
          hadoopqa Hadoop QA added a comment -

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

          Overall: +1 all checks pass

          SUCCESS: all tests passed

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

          This message is automatically generated.

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

          +1

          Show
          colinma Colin Ma added a comment - +1
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Thanks for the review Sravya Tirukkovalur and Colin Ma. Is there anything else I need to do to get this merged?

          Show
          bhooshan Bhooshan Mogal added a comment - Thanks for the review Sravya Tirukkovalur and Colin Ma . Is there anything else I need to do to get this merged?
          Hide
          colinma Colin Ma added a comment -

          Bhooshan Mogal, thanks for the contribute, the patch will be committed later if there is no other comments.

          Show
          colinma Colin Ma added a comment - Bhooshan Mogal , thanks for the contribute, the patch will be committed later if there is no other comments.
          Hide
          bhooshan Bhooshan Mogal added a comment -

          Colin Ma Sravya Tirukkovalur, thanks for committing this. This patch added a configuration parameter for Sentry service in sentry-site.xml. Should this be documented somewhere?

          Also, I have reported SENTRY-1133 as a follow-up task for this. I'll get to that a little later - perhaps 1.8.0, since it does not block anything right now.

          Show
          bhooshan Bhooshan Mogal added a comment - Colin Ma Sravya Tirukkovalur , thanks for committing this. This patch added a configuration parameter for Sentry service in sentry-site.xml . Should this be documented somewhere? Also, I have reported SENTRY-1133 as a follow-up task for this. I'll get to that a little later - perhaps 1.8.0, since it does not block anything right now.

            People

            • Assignee:
              bdmogal Bhooshan Mogal
              Reporter:
              bhooshan Bhooshan Mogal
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development