You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2019/02/21 00:27:03 UTC
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12542
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_provider and creates some
factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
43 files changed, 776 insertions(+), 214 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12542/2
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_factory_class and creates
some factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
The --authorization_factory_class is used to specify which authorization
provider ipmlementation to use.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
A fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
49 files changed, 1,060 insertions(+), 258 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12542/7
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 12:
> Patch Set 12: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3887/
Looks like thrift-server-test crashed:
:25:55 ] 56/103 Test #56: thrift-server-test ...............***Exception: Other 1.11 sec
06:25:55 ] Turning perftools heap leak checking off
06:25:55 ] Loading random data
06:25:55 ] Initializing database 'f2b2-8af6-9c5c-7874/krb5kdc/principal' for realm 'KRBTEST.COM',
06:25:55 ] master key name 'K/M@KRBTEST.COM'
06:25:55 ] Mar 07 03:02:53 ip-172-31-45-54 krb5kdc[80473](info): setting up network...
06:25:55 ] krb5kdc: Address already in use - Cannot bind server socket on 0.0.0.0.51293
06:25:55 ] krb5kdc: setsockopt(9,IPV6_V6ONLY,1) worked
06:25:55 ] Mar 07 03:02:53 ip-172-31-45-54 krb5kdc[80473](info): listening on fd 9: udp ::.51293 (pktinfo)
06:25:55 ] Mar 07 03:02:53 ip-172-31-45-54 krb5kdc[80473](info): listening on fd 10: udp 172.31.45.54.51293
06:25:55 ] Mar 07 03:02:53 ip-172-31-45-54 krb5kdc[80473](info): set up 2 sockets
06:25:55 ] Mar 07 03:02:53 ip-172-31-45-54 krb5kdc[80473](info): commencing operation
06:25:55 ] krb5kdc: starting...
06:25:55 ] *** Check failure stack trace: ***
06:25:55 ] @ 0x484217c google::LogMessage::Fail()
06:25:55 ] @ 0x4843a21 google::LogMessage::SendToLog()
06:25:55 ] @ 0x4841b56 google::LogMessage::Flush()
06:25:55 ] @ 0x484511d google::LogMessageFatal::~LogMessageFatal()
06:25:55 ] @ 0x1a510dd main
06:25:55 ] @ 0x7f70726a482f __libc_start_main
06:25:55 ] @ 0x1a44a68 _start
06:25:55 ] Wrote minidump to /home/ubuntu/Impala/logs/be_tests/minidumps/thrift-server-test/d5f8d324-a674-4331-52673c89-d7e11863.dmp
06:25:55 ] Wrote minidump to /home/ubuntu/Impala/logs/be_tests/minidumps/thrift-server-test/d5f8d324-a674-4331-52673c89-d7e11863.dmp
I don't think this change has anything to do with it. Restarting the GVO.
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 06:40:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_provider and creates some
factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
43 files changed, 806 insertions(+), 229 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12542/3
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3885/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 19:54:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 13: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 13
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 10:46:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2247/ : 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/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Feb 2019 03:57:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2352/ : 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/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 05:54:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "radford nguyen (Code Review)" <ge...@cloudera.org>.
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 12:
(1 comment)
hi @fredy, just adding a teeny little comment
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@121
PS2, Line 121: analyzer.registerPrivReq(builder ->
> I like the last one. Will update.
I also prefer the last one because it makes it easy to see which methods are called on `builder` at-a-glance. With the one-liner, method calls on other objects (e.g. arguments) produce noise
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 03:48:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2243/ : 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/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Feb 2019 01:31:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 19:54:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3887/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 01:36:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 12: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3887/
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 06:25:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_factory_class and creates
some factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
The --authorization_factory_class is used to specify which authorization
provider implementation to use.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Reviewed-on: http://gerrit.cloudera.org:8080/12542
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
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/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
A fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
52 files changed, 1,113 insertions(+), 298 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 14
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 11: Code-Review+2
Very nicely done!
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 19:44:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 13
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 06:41:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward (450) has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2516
PS2, Line 2516: if (privilege == Privilege.ANY) {
I think you can shorten this by moving the if statement inside the lambda
registerPrivReq(builder -> {
if (requireGrantOption) {
builder.grantOption();
}
return (privelege == Privelege.ANY) ? build.any().onAnyColumn(dbName).build() :
builder.allOf(privilege).onDb(dbName).build();
}
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2694
PS2, Line 2694: .allOf(priv);
nit: The style is inconsistent with the rest of the file
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@121
PS2, Line 121: analyzer.registerPrivReq(builder ->
nit: There are a few different lambda styles in different files. Wondering what the goto should be.
Personally, I prefer the last one even if it is possible for everything to be on a single line.
registerPrivReq(builder -> builder
.allOf(priv).onAnyColumn(fqTableName.getDb(), fqTableName.getTbl()).build());
vs
analyzer.registerPrivReq(builder ->
builder.onFunction(fn_.dbName(), fn_.signatureString())
.allOf(Privilege.CREATE)
.build());
vs
analyzer.registerPrivReq(builder -> builder
.onTable(table.getDb().getName(), table.getName())
.allOf(Privilege.VIEW_METADATA)
.build());
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java@27
PS2, Line 27: serverName_ = serverName == null ? "server" : serverName;
Any reason the `null` is handled here instead of being checked in the `AuthorizableFactory` like `AuthorizableDatabase`, `AuthorizableTable`, etc.
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Feb 2019 20:15:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2516
PS2, Line 2516: * the specified privilege.
> I think you can shorten this by moving the if statement inside the lambda
Done
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2694
PS2, Line 2694: builder.grantOption();
> nit: The style is inconsistent with the rest of the file
Done
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@121
PS2, Line 121: analyzer.registerPrivReq(builder ->
> nit: There are a few different lambda styles in different files. Wondering
I like the last one. Will update.
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java@27
PS2, Line 27: serverName_ = serverName == null ? "server" : serverName;
> Any reason the `null` is handled here instead of being checked in the `Auth
Yeah that's how it works AFAIK. Basically if the name is not specified, it will use the default name. For example:
"grant select on server to role foo"
"grant select on server my_server to role foo"
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Feb 2019 20:59:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_factory_class and creates
some factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
The --authorization_factory_class is used to specify which authorization
provider implementation to use.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
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/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
A fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
53 files changed, 1,114 insertions(+), 299 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12542/8
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 6:
(7 comments)
I redesigned the CR to be reduce the leaky abstraction. Please take a look at it again.
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java@46
PS2, Line 46: Authorizable newTable(String dbName, String tableName);
> I wonder if there is another way to do this? This class would appear to nee
Done
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java@26
PS2, Line 26: public class AuthorizableTable extends Authorizable {
> Looks like there are a series of these classes. Each takes a series of name
Each implementation represents the objects supported by Impala authorization, e.g. server, database, table, column, URI, function, etc. They all require different information. We can revisit this design again, though. A lot of stuff is remnant from the old implementation.
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@60
PS2, Line 60:
> Similar to prior comments: should the sentry-specific binding be in the gen
Done
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java@499
PS2, Line 499: Set<String> groupNames = ((SentryAuthorizationChecker) fe.getAuthzChecker())
> Temporary code? Or, should this be handled by sentry-specific code?
Yeah temporary code. The second part of this patch only focuses on the checker. I will need to revisit how to deal with authorization management (update, add, remove) in later patches.
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/Frontend.java@321
PS2, Line 321: // In the case that it was a non-delta update, the catalog might have reloaded
> Similar to earlier: should Sentry-specific stuff leak out of the abstractio
Done
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java@111
PS2, Line 111: try {
> Should this be factored to into a an auth factory rather than inline here i
Done
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@143
PS2, Line 143: AuthorizationFactory authzFactory;
> Ditto
Done
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Feb 2019 01:11:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 11: Code-Review+1
(2 comments)
Carry Bharath's +1.
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java:
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java@55
PS8, Line 55: analyzer.isAuthzEnabled() && owner_.getOwnerType()
> I think we can retain the getAuthzConfig() helper in the Analyzer since the
Good idea. Done.
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java@119
PS8, Line 119: e)
> nit: space
Done
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 07:04:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 13:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3888/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 13
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: radford nguyen <ra...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 06:41:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 3:
(7 comments)
First-pass review. Sorry, was started on PS 2; I see you're now on PS 3.
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java@46
PS2, Line 46: case SENTRY:
I wonder if there is another way to do this? This class would appear to need to enumerate all the auth providers. As such, it breaks abstraction. Can these methods be defined within each auth plugin itself? Sentry, say, knows to create its own authorizables?
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java@26
PS2, Line 26: public class AuthorizableTable extends Authorizable {
Looks like there are a series of these classes. Each takes a series of names. I wonder, does this generalize? Or, should these be built via composition? An auth table takes an auth DB and a table name? An auth column takes an auth table and a column name?
I'm not proposing we make the change if it's not needed, just trying to reason out the data model.
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java@60
PS2, Line 60: static SentryAuthorizationConfig sentry(String serverName,
Similar to prior comments: should the sentry-specific binding be in the generic interface, or should this reside in some sentry-specific driver?
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java@499
PS2, Line 499: Set<String> groupNames = ((SentryAuthorizationChecker) fe.getAuthzChecker())
Temporary code? Or, should this be handled by sentry-specific code?
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/Frontend.java@321
PS2, Line 321: if (authzConfig_.getProvider() == AuthorizationProvider.SENTRY) {
Similar to earlier: should Sentry-specific stuff leak out of the abstraction, or should this stuff be handled through a generic interface?
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniCatalog.java@111
PS2, Line 111: if (BackendConfig.INSTANCE.useSentry()) {
Should this be factored to into a an auth factory rather than inline here in this top-level class?
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/12542/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@143
PS2, Line 143: if (BackendConfig.INSTANCE.useSentry()) {
Ditto
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Sat, 23 Feb 2019 04:38:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2205/ : 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/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Feb 2019 21:32:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_factory_class and creates
some factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
The --authorization_factory_class is used to specify which authorization
provider ipmlementation to use.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
A fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
49 files changed, 1,056 insertions(+), 258 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12542/6
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/12542/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:
http://gerrit.cloudera.org:8080/#/c/12542/7/be/src/service/frontend.cc@43
PS7, Line 43: DEFINE_string(server_name, "", "The name to use for securing this impalad "
: "server during authorization. Set to enable authorization. By default, the "
: "authorization policy will be loaded from the catalog server (via the statestore)."
: "To use a file based authorization policy, set --authorization_policy_file.");
: DEFINE_string(authorization_policy_file, "", "HDFS path to the authorization policy "
: "file. If set, authorization will be enabled and the authorization policy will be "
: "read from a file.");
: DEFINE_string(authorization_po
> Aren't these also Sentry specific? If so, do we need to call them out?
Yes, except server_name. The authorization_policy_file will be removed in https://gerrit.cloudera.org/c/12637/
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@383
PS7, Line 383: er_ = new User(TSessionStateUti
> Do you need this too? should probably be inferred from the factory.
Done. I refactored AuthorizationFafctory to hold the instance of authzConfig.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@29
PS7, Line 29: /**
> Any reason to call them (this and below) getBlahBlah() instead of newBlahBl
We need both newAuthzConfig() and getAuthzConfig(). The newAuthzConfig() will be called by the constructor and getAuthzConfig() will reuse the instance of authzConfig created in the constructor. That way we can make the implementation of this class to be immutable.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@40
PS7, Line 40: ker}.
> Should this be in control of the AuthorizationFactoryClass? It looks like t
Makes sense. Done.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
File fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java@25
PS7, Line 25: DefaultAuthorizableFactory
> Whats the use of this class?
The idea is to build the Authorizable classes without having Sentry-specific classes. For example: https://gerrit.cloudera.org/c/12632/3/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java#57.
The SentryAuthorizableFactory is a bit special since we wrap all the Sentry specific classes inside. We could use DefaultAuthorizableFactory with Sentry. However, that would require converting those Authorizable classes into org.apache.sentry.core.model.db.* classes.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@28
PS7, Line 28: /
> Make this private and use the interface method AuthorizationFactor#getAuthz
Done.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@116
PS7, Line 116: String msg = String.format("Unable to instantiate authorization provider: %s",
> Log the error trace here?
Done
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@149
PS7, Line 149: String msg = String.format("Unable to instantiate authorization provider: %s",
> nit: Can you also log the error here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 05:10:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 8: Code-Review+1
(2 comments)
Will let Paul +2 this.
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java:
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java@55
PS8, Line 55: analyzer.getAuthzFactory().getAuthorizationConfig()
I think we can retain the getAuthzConfig() helper in the Analyzer since there are so many callers for it. We can also add isAuthzEnabled() helper if needed, to simplify the caller code.
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12542/8/fe/src/main/java/org/apache/impala/service/JniCatalog.java@119
PS8, Line 119: e);
nit: space
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 06:45:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2187/ : 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/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Feb 2019 00:52:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
IMPALA-7917 (Part 2): Decouple Sentry from Impala
This patch adds a new flag --authorization_factory_class and creates
some factories to make it easier to switch to a different authorization
provider. For backward compatability the default authorization provider
is still set to Sentry.
The --authorization_factory_class is used to specify which authorization
provider implementation to use.
This patch has no functional change.
Testing:
- Ran all FE tests
- Ran all authorization E2E tests
Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
---
M be/src/service/frontend.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java
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/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/AuthorizationStmt.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
M fe/src/main/java/org/apache/impala/analysis/HdfsUri.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableDb.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableFn.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableServer.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableTable.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizableUri.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java
A fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
A fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizableFactory.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationConfig.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.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/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/common/FrontendFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/common/QueryFixture.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
52 files changed, 1,113 insertions(+), 298 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/12542/11
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 7:
(9 comments)
Nice clean separation. The patch looks pretty good to me. Some minor comments, would like to know your thoughts on them.
http://gerrit.cloudera.org:8080/#/c/12542/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12542/7//COMMIT_MSG@15
PS7, Line 15: ipmlementation
nit: typo
http://gerrit.cloudera.org:8080/#/c/12542/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:
http://gerrit.cloudera.org:8080/#/c/12542/7/be/src/service/frontend.cc@43
PS7, Line 43: DEFINE_string(server_name, "", "The name to use for securing this impalad "
: "server during authorization. Set to enable authorization. By default, the "
: "authorization policy will be loaded from the catalog server (via the statestore)."
: "To use a file based authorization policy, set --authorization_policy_file.");
: DEFINE_string(authorization_policy_file, "", "HDFS path to the authorization policy "
: "file. If set, authorization will be enabled and the authorization policy will be "
: "read from a file.");
: DEFINE_string(authorization_po
Aren't these also Sentry specific? If so, do we need to call them out?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@383
PS7, Line 383: AuthorizationConfig authzConfig
Do you need this too? should probably be inferred from the factory.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@29
PS7, Line 29: AuthorizationConfig newAuthorizationConfig(BackendConfig backendConfig);
Any reason to call them (this and below) getBlahBlah() instead of newBlahBlah()? For ex: getAuthzConfig() instead of newAuthorizationCofnig()?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@40
PS7, Line 40: AuthorizationConfig authzConfig
Should this be in control of the AuthorizationFactoryClass? It looks like the current flow is like,
1. create AuthzConfig
2. createAuthznFactor(AuthzConfig).
The latter step could also build the AuthzConfig, no? It looks to me like the clients shouldn't worry about AuthznConfig anymore. They just pass the BackendConfig and totally rely on the created AuthznFactory from there on.
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java
File fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java@25
PS7, Line 25: DefaultAuthorizableFactory
Whats the use of this class?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@28
PS7, Line 28: public
Make this private and use the interface method AuthorizationFactor#getAuthzConfig() at callers?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@116
PS7, Line 116: String.format("Unable to instantiate authorization provider: %s",
Log the error trace here?
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:
http://gerrit.cloudera.org:8080/#/c/12542/7/fe/src/main/java/org/apache/impala/service/JniFrontend.java@149
PS7, Line 149: String.format("Unable to instantiate authorization provider: %s",
nit: Can you also log the error here?
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Mar 2019 21:48:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2354/ : 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/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 07:45:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 2): Decouple Sentry from Impala
Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12542 )
Change subject: IMPALA-7917 (Part 2): Decouple Sentry from Impala
......................................................................
Patch Set 7:
Took a quick spin over the files. I really like how you've completely abstracted out Sentry into the auth factory. Very clean. Adding Ranger (or anything else) should be quite clean without the (if sentry to this, if ranger do that, else do nothing) code of old. Nicely done!
--
To view, visit http://gerrit.cloudera.org:8080/12542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I051d06451fb14ba2ffb3f7e0d4aae37dee55ab86
Gerrit-Change-Number: 12542
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis (450)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Feb 2019 03:53:54 +0000
Gerrit-HasComments: No