You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2020/05/11 18:41:35 UTC
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15833
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
IMPALA-9708: Remove Sentry support
Impala 4 decided to drop Sentry support in favor of Ranger. This
removes Sentry support and related tests. It retires startup
flags related to Sentry and removes as much code as possible.
Documentation will still need to be adjusted to remove
references to Sentry.
Some issues came up when implementing this. Here is a summary
of how this patch resolves them:
1. authorization_provider currently defaults to "sentry", but
"ranger" requires extra parameters to be set. This changes the
default value of authorization_provider to "", which translates
internally to the noop policy that does no authorization.
2. These flags are Sentry specific and are now retired:
- authorization_policy_provider_class
- sentry_catalog_polling_frequency_s
- sentry_config
3. The authorization_factory_class may be obsolete now that
there is only one authorization policy, but this leaves it
in place.
4. Sentry is the last component using CDH_COMPONENTS_HOME, so
that is removed. There are still Maven dependencies coming
from the CDH_BUILD_NUMBER repository, so that is not removed.
5. To make the transition easier, testdata/bin/kill-sentry-service.sh
is not removed and it is still called from testdata/bin/kill-all.sh.
Testing:
- Core job passes
Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
---
M README-build.md
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/transport/TSasl.cpp
M be/src/util/backend-gflag-util.cc
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M buildall.sh
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaActionFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaPrivilegeModel.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableColumn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableServer.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableTable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableUri.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyReaderException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUnavailableException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUtil.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
D fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
D fe/src/test/java/org/apache/impala/authorization/sentry/ImpalaActionFactoryTest.java
D fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
D fe/src/test/java/org/apache/impala/testutil/SentryServicePinger.java
D fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java
M fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
M fe/src/test/resources/hive-site.xml.py
D fe/src/test/resources/sentry-site.xml.py
M impala-parent/pom.xml
M infra/deploy/deploy.py
M testdata/bin/run-all.sh
M testdata/bin/run-hive-server.sh
D testdata/bin/run-sentry-service.sh
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
D testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_authorization.py
M tests/authorization/test_authorized_proxy.py
D tests/authorization/test_grant_revoke.py
D tests/authorization/test_sentry.py
M tests/common/custom_cluster_test_suite.py
D tests/common/sentry_cache_test_suite.py
M tests/common/skip.py
81 files changed, 94 insertions(+), 9,078 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/15833/4
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5:
> (1 comment)
>
> Thanks for doing the cleanup! Is this the part-1 and there will be
> other patches for this JIRA?
>
> I think at least we can remove the catalog cache for
> users/principals/privileges since they are only used for Sentry.
> It's ok to do this in a separate patch since maybe a lot of codes
> can be removed as well.
>
> Some works (IMPALA-9002, IMPALA-9195, IMPALA-9242, IMPALA-9222,
> etc.) in the SHOW TABLES/DATABASES code path are done for Sentry,
> they may can be removed/reverted as well.
Let's plan on having follow-up code changes for removing further obsolete code. I'm open to doing that with followup JIRAs that we link to IMPALA-9708 or with further changes using IMPALA-9708 itself. For example, I know we will need to update docs, and Fang-Yu pointed out that we should consider removing "show roles".
I updated the commit comment to indicate that this is the first round of removing code and that there will be further removal of obsolete code.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 23:33:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5: Code-Review+1
> Patch Set 5:
>
> > Hi Joe, thank you for the effort to remove Sentry support!
> >
> > I do not have any major comment but the following very minor one.
> >
> > After removing Sentry support, there is no role-based authorization
> > provider in Impala, meaning that some statement like "SHOW ROLES"
> > is no longer supported. In this regard, I was wondering whether we
> > also need to remove the related code.
> >
> > For instance, in the case of "SHOW ROLES" described above, we may
> > remove the method of getRoles() at
> > 1) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/JniFrontend.java#L531-L541,
> > 2) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java#L54,
> > and
> > 3) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java#L99-L107.
> >
> > Also, some production rule like show_roles_stmt at
> > https://github.com/apache/impala/blob/master/fe/src/main/cup/sql-parser.cup#L944-L951
> > and ShowRoleStmt at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
> > may have to be removed as well if we decide to completely remove
> > support for role-based authorization.
> >
> > But on the other hand, it seems better or more flexible to keep
> > those statements for role-based authorization in case we would like
> > to add support for role-based authorization (other than Sentry)
> > some day and I guess that is the reason why you chose not to remove
> > these statements for role-based authorization.
>
> This is something I didn't consider. I think if we don't have any authorization that supports roles and Ranger isn't planning on supporting roles, then we should consider removing the roles functionality. I think we can turn that into a separate change. I will file a JIRA for tracking removing the show roles functionality. We can have a discussion on that JIRA. I imagine there will be several followup cleanups as we find out what is no longer needed.
Thanks Joe for tracking this! Will try to provide some ideas about the followup cleanups in that JIRA. I do not have any other comment.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 15:03:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 May 2020 04:19:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
IMPALA-9708: Remove Sentry support
Impala 4 decided to drop Sentry support in favor of Ranger. This
removes Sentry support and related tests. It retires startup
flags related to Sentry and removes as much code as possible.
Documentation will still need to be adjusted to remove
references to Sentry.
Some issues came up when implementing this. Here is a summary
of how this patch resolves them:
1. authorization_provider currently defaults to "sentry", but
"ranger" requires extra parameters to be set. This changes the
default value of authorization_provider to "", which translates
internally to the noop policy that does no authorization.
2. These flags are Sentry specific and are now retired:
- authorization_policy_provider_class
- sentry_catalog_polling_frequency_s
- sentry_config
3. The authorization_factory_class may be obsolete now that
there is only one authorization policy, but this leaves it
in place.
4. Sentry is the last component using CDH_COMPONENTS_HOME, so
that is removed. There are still Maven dependencies coming
from the CDH_BUILD_NUMBER repository, so that is not removed.
5. To make the transition easier, testdata/bin/kill-sentry-service.sh
is not removed and it is still called from testdata/bin/kill-all.sh.
Testing:
- Core job passes
Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
---
M README-build.md
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/transport/TSasl.cpp
M be/src/util/backend-gflag-util.cc
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M buildall.sh
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaActionFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaPrivilegeModel.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableColumn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableServer.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableTable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableUri.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyReaderException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUnavailableException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUtil.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
D fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
D fe/src/test/java/org/apache/impala/authorization/sentry/ImpalaActionFactoryTest.java
D fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
D fe/src/test/java/org/apache/impala/testutil/SentryServicePinger.java
D fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java
M fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
M fe/src/test/resources/hive-site.xml.py
D fe/src/test/resources/sentry-site.xml.py
M impala-parent/pom.xml
M infra/deploy/deploy.py
M testdata/bin/run-all.sh
M testdata/bin/run-hive-server.sh
D testdata/bin/run-sentry-service.sh
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
D testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_authorization.py
M tests/authorization/test_authorized_proxy.py
D tests/authorization/test_grant_revoke.py
D tests/authorization/test_sentry.py
M tests/common/custom_cluster_test_suite.py
D tests/common/sentry_cache_test_suite.py
M tests/common/skip.py
81 files changed, 94 insertions(+), 9,078 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/15833/5
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6028/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 May 2020 19:28:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
Thanks for doing the cleanup! Is this the part-1 and there will be other patches for this JIRA?
I think at least we can remove the catalog cache for users/principals/privileges since they are only used for Sentry. It's ok to do this in a separate patch since maybe a lot of codes can be removed as well.
Some works (IMPALA-9002, IMPALA-9195, IMPALA-9242, IMPALA-9222, etc.) in the SHOW TABLES/DATABASES code path are done for Sentry, they may can be removed/reverted as well.
http://gerrit.cloudera.org:8080/#/c/15833/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/15833/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2336
PS5, Line 2336: /**
: * Adds a new role with the given name and grant groups to the AuthorizationPolicy.
: * If a role with the same name already exists it will be overwritten.
: */
: public Role addRole(String roleName, Set<String> grantGroups) {
: Principal role = addPrincipal(roleName, grantGroups, TPrincipalType.ROLE);
: Preconditions.checkState(role instanceof Role);
: return (Role) role;
: }
:
: /**
: * Adds a new user with the given name to the AuthorizationPolicy.
: * If a user with the same name already exists it will be overwritten.
: */
: public User addUser(String userName) {
: Principal user = addPrincipal(userName, new HashSet<>(),
: TPrincipalType.USER);
: Preconditions.checkState(user instanceof User);
: return (User) user;
: }
:
: /**
: * Add a user to the catalog if it doesn't exist. This is necessary so privileges
: * can be added for a user. example: owner privileges.
: */
: public User addUserIfNotExists(String owner, Reference<Boolean> existingUser) {
: versionLock_.writeLock().lock();
: try {
: User user = getAuthPolicy().getUser(owner);
: existingUser.setRef(Boolean.TRUE);
: if (user == null) {
: user = addUser(owner);
: existingUser.setRef(Boolean.FALSE);
: }
: return user;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: private Principal addPrincipal(String principalName, Set<String> grantGroups,
: TPrincipalType type) {
: versionLock_.writeLock().lock();
: try {
: Principal principal = Principal.newInstance(principalName, type, grantGroups);
: principal.setCatalogVersion(incrementAndGetCatalogVersion());
: authPolicy_.addPrincipal(principal);
: return principal;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: /**
: * Removes the role with the given name from the AuthorizationPolicy. Returns the
: * removed role with an incremented catalog version, or null if no role with this name
: * exists.
: */
: public Role removeRole(String roleName) {
: Principal role = removePrincipal(roleName, TPrincipalType.ROLE);
: if (role == null) return null;
: Preconditions.checkState(role instanceof Role);
: return (Role) role;
: }
:
: /**
: * Removes the user with the given name from the AuthorizationPolicy. Returns the
: * removed user with an incremented catalog version, or null if no user with this name
: * exists.
: */
: public User removeUser(String userName) {
: Principal user = removePrincipal(userName, TPrincipalType.USER);
: if (user == null) return null;
: Preconditions.checkState(user instanceof User);
: return (User) user;
: }
:
: private Principal removePrincipal(String principalName, TPrincipalType type) {
: versionLock_.writeLock().lock();
: try {
: Principal principal = authPolicy_.removePrincipal(principalName, type);
: // TODO(todd): does this end up leaking the privileges associated
: // with this principal into the CatalogObjectVersionSet on the catalogd?
: if (principal == null) return null;
: for (PrincipalPrivilege priv: principal.getPrivileges()) {
: priv.setCatalogVersion(incrementAndGetCatalogVersion());
: deleteLog_.addRemovedObject(priv.toTCatalogObject());
: }
: principal.setCatalogVersion(incrementAndGetCatalogVersion());
: deleteLog_.addRemovedObject(principal.toTCatalogObject());
: return principal;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: /**
: * Adds a grant group to the given role name and returns the modified Role with
: * an updated catalog version. If the role does not exist a CatalogException is thrown.
: */
: public Role addRoleGrantGroup(String roleName, String groupName)
: throws CatalogException {
: versionLock_.writeLock().lock();
: try {
: Role role = authPolicy_.addRoleGrantGroup(roleName, groupName);
: Preconditions.checkNotNull(role);
: role.setCatalogVersion(incrementAndGetCatalogVersion());
: return role;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: /**
: * Removes a grant group from the given role name and returns the modified Role with
: * an updated catalog version. If the role does not exist a CatalogException is thrown.
: */
: public Role removeRoleGrantGroup(String roleName, String groupName)
: throws CatalogException {
: versionLock_.writeLock().lock();
: try {
: Role role = authPolicy_.removeRoleGrantGroup(roleName, groupName);
: Preconditions.checkNotNull(role);
: role.setCatalogVersion(incrementAndGetCatalogVersion());
: return role;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: /**
: * Adds a privilege to the given role name. Returns the new PrincipalPrivilege and
: * increments the catalog version. If the parent role does not exist a CatalogException
: * is thrown.
: */
: public PrincipalPrivilege addRolePrivilege(String roleName, TPrivilege thriftPriv)
: throws CatalogException {
: Preconditions.checkArgument(thriftPriv.getPrincipal_type() == TPrincipalType.ROLE);
: return addPrincipalPrivilege(roleName, thriftPriv, TPrincipalType.ROLE);
: }
:
: /**
: * Adds a privilege to the given user name. Returns the new PrincipalPrivilege and
: * increments the catalog version. If the user does not exist a CatalogException is
: * thrown.
: */
: public PrincipalPrivilege addUserPrivilege(String userName, TPrivilege thriftPriv)
: throws CatalogException {
: Preconditions.checkArgument(thriftPriv.getPrincipal_type() == TPrincipalType.USER);
: return addPrincipalPrivilege(userName, thriftPriv, TPrincipalType.USER);
: }
:
: private PrincipalPrivilege addPrincipalPrivilege(String principalName,
: TPrivilege thriftPriv, TPrincipalType type) throws CatalogException {
: versionLock_.writeLock().lock();
: try {
: Principal principal = authPolicy_.getPrincipal(principalName, type);
: if (principal == null) {
: throw new CatalogException(String.format("%s does not exist: %s",
: Principal.toString(type), principalName));
: }
: PrincipalPrivilege priv = PrincipalPrivilege.fromThrift(thriftPriv);
: priv.setCatalogVersion(incrementAndGetCatalogVersion());
: authPolicy_.addPrivilege(priv);
: return priv;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: /**
: * Removes a PrincipalPrivilege from the given role name and privilege name. Returns
: * the removed PrincipalPrivilege with an incremented catalog version or null if no
: * matching privilege was found. Throws a CatalogException if no role exists with this
: * name.
: */
: public PrincipalPrivilege removeRolePrivilege(String roleName, String privilegeName)
: throws CatalogException {
: return removePrincipalPrivilege(roleName, privilegeName, TPrincipalType.ROLE);
: }
:
: /**
: * Removes a PrincipalPrivilege from the given user name and privilege name. Returns
: * the removed PrincipalPrivilege with an incremented catalog version or null if no
: * matching privilege was found. Throws a CatalogException if no user exists with this
: * name.
: */
: public PrincipalPrivilege removeUserPrivilege(String userName, String privilegeName)
: throws CatalogException {
: return removePrincipalPrivilege(userName, privilegeName, TPrincipalType.USER);
: }
:
: private PrincipalPrivilege removePrincipalPrivilege(String principalName,
: String privilegeName, TPrincipalType type) throws CatalogException {
: versionLock_.writeLock().lock();
: try {
: Principal principal = authPolicy_.getPrincipal(principalName, type);
: if (principal == null) {
: throw new CatalogException(String.format("%s does not exist: %s",
: Principal.toString(type), principalName));
: }
: PrincipalPrivilege principalPrivilege = principal.removePrivilege(privilegeName);
: if (principalPrivilege == null) return null;
: principalPrivilege.setCatalogVersion(incrementAndGetCatalogVersion());
: deleteLog_.addRemovedObject(principalPrivilege.toTCatalogObject());
: return principalPrivilege;
: } finally {
: versionLock_.writeLock().unlock();
: }
: }
:
: /**
: * Gets a PrincipalPrivilege from the given principal name. Returns the privilege
: * if it exists, or null if no privilege matching the privilege spec exist.
: * Throws a CatalogException if the principal does not exist.
: */
: public PrincipalPrivilege getPrincipalPrivilege(String principalName,
: TPrivilege privSpec) throws CatalogException {
: String privilegeName = PrincipalPrivilege.buildPrivilegeName(privSpec);
: versionLock_.readLock().lock();
: try {
: Principal principal = authPolicy_.getPrincipal(principalName,
: privSpec.getPrincipal_type());
: if (principal == null) {
: throw new CatalogException(Principal.toString(privSpec.getPrincipal_type()) +
: " does not exist: " + principalName);
: }
: return principal.getPrivilege(privilegeName);
: } finally {
: versionLock_.readLock().unlock();
: }
: }
These can be removed as well. They are only used for Sentry. For Ranger, the authz stuffs (user/privileges/tags/policies) are cached by the ranger plugin. We don't need to maintain users/principals/privileges in our catalog cache (both catalogd side and impalad side).
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 02:01:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Fang-Yu Rao (Code Review)" <ge...@cloudera.org>.
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5:
Hi Joe, thank you for the effort to remove Sentry support!
I do not have any major comment but the following very minor one.
After removing Sentry support, there is no role-based authorization provider in Impala, meaning that some statement like "SHOW ROLES" is no longer supported. In this regard, I was wondering whether we also need to remove the related code.
For instance, in the case of "SHOW ROLES" described above, we may remove the method of getRoles() at
1) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/JniFrontend.java#L531-L541,
2) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java#L54, and
3) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java#L99-L107.
Also, some production rule like show_roles_stmt at https://github.com/apache/impala/blob/master/fe/src/main/cup/sql-parser.cup#L944-L951 and ShowRoleStmt at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java may have to be removed as well if we decide to completely remove support for role-based authorization.
But on the other hand, it seems better or more flexible to keep those statements for role-based authorization in case we would like to add support for role-based authorization (other than Sentry) some day and I guess that is the reason why you chose not to remove these statements for role-based authorization.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Mon, 18 May 2020 16:18:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6105/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 20 May 2020 00:24:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5: Code-Review+2
Thanks for the cleanup!
About removing more code that is only relevant in Sentry: I vote for merging this as it is and continue the cleanup in another patch. I understand if someone prefers to do this in one big patch, but removing Sentry tests and CDH dependencies would make test jobs faster and avoid some potential build issues/flakiness, so I think it would be better to merge these changes earlier.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 14:41:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 7: Code-Review+2
The way that I'm thinking about this change is that this should leave us in a place with coherent behavior for developers and users. There's still obsolete code to remove, but visible behavior should be reasonable. So, I plan to go ahead with this change and then plan to remove more code in further changes.
I'm carrying Csaba's +2, but let me know if there is any concern.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 23:47:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 20 May 2020 04:59:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5:
> Hi Joe, thank you for the effort to remove Sentry support!
>
> I do not have any major comment but the following very minor one.
>
> After removing Sentry support, there is no role-based authorization
> provider in Impala, meaning that some statement like "SHOW ROLES"
> is no longer supported. In this regard, I was wondering whether we
> also need to remove the related code.
>
> For instance, in the case of "SHOW ROLES" described above, we may
> remove the method of getRoles() at
> 1) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/service/JniFrontend.java#L531-L541,
> 2) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java#L54,
> and
> 3) https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java#L99-L107.
>
> Also, some production rule like show_roles_stmt at
> https://github.com/apache/impala/blob/master/fe/src/main/cup/sql-parser.cup#L944-L951
> and ShowRoleStmt at https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
> may have to be removed as well if we decide to completely remove
> support for role-based authorization.
>
> But on the other hand, it seems better or more flexible to keep
> those statements for role-based authorization in case we would like
> to add support for role-based authorization (other than Sentry)
> some day and I guess that is the reason why you chose not to remove
> these statements for role-based authorization.
This is something I didn't consider. I think if we don't have any authorization that supports roles and Ranger isn't planning on supporting roles, then we should consider removing the roles functionality. I think we can turn that into a separate change. I will file a JIRA for tracking removing the show roles functionality. We can have a discussion on that JIRA. I imagine there will be several followup cleanups as we find out what is no longer needed.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 00:04:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6071/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 May 2020 01:26:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5861/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 19 May 2020 23:38:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Fang-Yu Rao, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15833
to look at the new patch set (#6).
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
IMPALA-9708: Remove Sentry support
Impala 4 decided to drop Sentry support in favor of Ranger. This
removes Sentry support and related tests. It retires startup
flags related to Sentry and does the first round of removing
obsolete code. This does not adjust documentation to remove
references to Sentry, and other dead code will be removed
separately.
Some issues came up when implementing this. Here is a summary
of how this patch resolves them:
1. authorization_provider currently defaults to "sentry", but
"ranger" requires extra parameters to be set. This changes the
default value of authorization_provider to "", which translates
internally to the noop policy that does no authorization.
2. These flags are Sentry specific and are now retired:
- authorization_policy_provider_class
- sentry_catalog_polling_frequency_s
- sentry_config
3. The authorization_factory_class may be obsolete now that
there is only one authorization policy, but this leaves it
in place.
4. Sentry is the last component using CDH_COMPONENTS_HOME, so
that is removed. There are still Maven dependencies coming
from the CDH_BUILD_NUMBER repository, so that is not removed.
5. To make the transition easier, testdata/bin/kill-sentry-service.sh
is not removed and it is still called from testdata/bin/kill-all.sh.
Testing:
- Core job passes
Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
---
M README-build.md
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/transport/TSasl.cpp
M be/src/util/backend-gflag-util.cc
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M buildall.sh
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaActionFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaPrivilegeModel.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableColumn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableServer.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableTable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableUri.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyReaderException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUnavailableException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUtil.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
D fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
D fe/src/test/java/org/apache/impala/authorization/sentry/ImpalaActionFactoryTest.java
D fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
D fe/src/test/java/org/apache/impala/testutil/SentryServicePinger.java
D fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java
M fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
M fe/src/test/resources/hive-site.xml.py
D fe/src/test/resources/sentry-site.xml.py
M impala-parent/pom.xml
M infra/deploy/deploy.py
M testdata/bin/run-all.sh
M testdata/bin/run-hive-server.sh
D testdata/bin/run-sentry-service.sh
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
D testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_authorization.py
M tests/authorization/test_authorized_proxy.py
D tests/authorization/test_grant_revoke.py
D tests/authorization/test_sentry.py
M tests/common/custom_cluster_test_suite.py
D tests/common/sentry_cache_test_suite.py
M tests/common/skip.py
81 files changed, 94 insertions(+), 9,078 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/15833/6
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-9708: Remove Sentry support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15833 )
Change subject: IMPALA-9708: Remove Sentry support
......................................................................
IMPALA-9708: Remove Sentry support
Impala 4 decided to drop Sentry support in favor of Ranger. This
removes Sentry support and related tests. It retires startup
flags related to Sentry and does the first round of removing
obsolete code. This does not adjust documentation to remove
references to Sentry, and other dead code will be removed
separately.
Some issues came up when implementing this. Here is a summary
of how this patch resolves them:
1. authorization_provider currently defaults to "sentry", but
"ranger" requires extra parameters to be set. This changes the
default value of authorization_provider to "", which translates
internally to the noop policy that does no authorization.
2. These flags are Sentry specific and are now retired:
- authorization_policy_provider_class
- sentry_catalog_polling_frequency_s
- sentry_config
3. The authorization_factory_class may be obsolete now that
there is only one authorization policy, but this leaves it
in place.
4. Sentry is the last component using CDH_COMPONENTS_HOME, so
that is removed. There are still Maven dependencies coming
from the CDH_BUILD_NUMBER repository, so that is not removed.
5. To make the transition easier, testdata/bin/kill-sentry-service.sh
is not removed and it is still called from testdata/bin/kill-all.sh.
Testing:
- Core job passes
Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Reviewed-on: http://gerrit.cloudera.org:8080/15833
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M README-build.md
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/transport/TSasl.cpp
M be/src/util/backend-gflag-util.cc
M bin/bootstrap_toolchain.py
M bin/create-test-configuration.sh
M bin/impala-config.sh
M buildall.sh
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/analysis/AlterDbSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaAction.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaActionFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/ImpalaPrivilegeModel.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableColumn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableDb.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFn.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableServer.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableTable.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableUri.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryConfig.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyReaderException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryPolicyService.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUnavailableException.java
D fe/src/main/java/org/apache/impala/authorization/sentry/SentryUtil.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/util/AuthorizationUtil.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
D fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
D fe/src/test/java/org/apache/impala/authorization/sentry/ImpalaActionFactoryTest.java
D fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
D fe/src/test/java/org/apache/impala/testutil/SentryServicePinger.java
D fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java
M fe/src/test/java/org/apache/impala/util/AuthorizationUtilTest.java
M fe/src/test/resources/hive-site.xml.py
D fe/src/test/resources/sentry-site.xml.py
M impala-parent/pom.xml
M infra/deploy/deploy.py
M testdata/bin/run-all.sh
M testdata/bin/run-hive-server.sh
D testdata/bin/run-sentry-service.sh
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
D testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
D testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_authorization.py
M tests/authorization/test_authorized_proxy.py
D tests/authorization/test_grant_revoke.py
D tests/authorization/test_sentry.py
M tests/common/custom_cluster_test_suite.py
D tests/common/sentry_cache_test_suite.py
M tests/common/skip.py
81 files changed, 94 insertions(+), 9,078 deletions(-)
Approvals:
Joe McDonnell: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8e99c15936d6d250cf258e3a1dcba11d3eb4661e
Gerrit-Change-Number: 15833
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fang-Yu Rao <fa...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>