[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. I looped master_sentry-itest 1000 times (which used UNIQUE_LOOPBACK mode) and all passed: http://dist-test.cloudera.org/job?job_id=hao.hao.1548189107.129474 [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Reviewed-on: http://gerrit.cloudera.org:8080/11956 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 532 insertions(+), 77 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 12: Verified+1 Code-Review+2 Overriding Jenkins, the failure is a known flake and unrelated to this patch. -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 22:39:32 + Gerrit-HasComments: No
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: hms_.reset(new hms::MiniHms()); > You're right; not sure how I thought this would help macOS. Done http://gerrit.cloudera.org:8080/#/c/11956/11/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11956/11/src/kudu/sentry/mini_sentry.cc@144 PS11, Line 144: // Check that the port number only changed if the original port was 0 > Nit: this is a bit unclear. Reword as "Check that the port number only chan Done -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 19:59:16 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#12). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. I looped master_sentry-itest 1000 times (which used UNIQUE_LOOPBACK mode) and all passed: http://dist-test.cloudera.org/job?job_id=hao.hao.1548189107.129474 [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 532 insertions(+), 77 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/12 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: string ktpath; > Sorry that I fail to see why. With the current approach, for macOS env we r You're right; not sure how I thought this would help macOS. It would add a bit of clarity to the UNIQUE_LOOPBACK case (in that there would be more symmetry between HMS and Sentry), but that's about it. Maybe note it in a TODO here instead, citing the upstream bug? http://gerrit.cloudera.org:8080/#/c/11956/11/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11956/11/src/kudu/sentry/mini_sentry.cc@144 PS11, Line 144: // Check that the port number hasn't changed if it has been set (not zero). Nit: this is a bit unclear. Reword as "Check that the port number only changed if the original port was 0 (i.e. if we asked to bind to an ephemeral port)". Maybe also rename 'port' to 'orig_port' to help clarify? -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 07:28:48 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 11:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257
PS2, Line 257: string ktpath;
> I think so, because it'll allow macOS to benefit from a restart-less Extern
Sorry that I fail to see why. With the current approach, for macOS env we
restart Sentry once, since assign a random port can cause port collision. I
don't see how the restart can be avoided even HMS can bind to a specific host.
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/mini-cluster/external_mini_cluster.cc@233
PS10, Line 233: we cannot choose a port at random as that can cause a port co
> Nit: "we cannot choose a port at random as that can cause a port collision.
Done
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/mini-cluster/external_mini_cluster.cc@236
PS10, Line 236: Sentry servi
> Nit: reconfigure
Done
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/sentry/mini_sentry.cc@68
PS10, Line 68: void MiniSentry::EnableHms(string hms_uris) {
> We should CHECK(!sentry_process_) here and in the other new setters.
Done
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/sentry/mini_sentry.cc@137
PS10, Line 137:
> If port_ isn't 0 before this call, we might want to check that its value ha
Done
--
To view, visit http://gerrit.cloudera.org:8080/11956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 22 Jan 2019 21:59:52 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#11). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. I looped master_sentry-itest 1000 times (which used UNIQUE_LOOPBACK mode) and all passed: http://dist-test.cloudera.org/job?job_id=hao.hao.1548189107.129474 [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 526 insertions(+), 77 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/11 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 10:
(5 comments)
Can you loop master_sentry-itest 1000 times just to make sure that the "use
port 1 on a UNIQUE_LOOPBACK address" approach is OK?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257
PS2, Line 257: RETURN_NOT_OK_PREPEND(kdc_->CreateServiceKeytab(spn,
),
> Sure, though with the new approach you suggested which can avoid restart fo
I think so, because it'll allow macOS to benefit from a restart-less
ExternalMiniCluster.
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/mini-cluster/external_mini_cluster.cc@233
PS10, Line 233: we cannot choose a random port which can cause port collision
Nit: "we cannot choose a port at random as that can cause a port collision."
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/mini-cluster/external_mini_cluster.cc@236
PS10, Line 236: reconfigured
Nit: reconfigure
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/sentry/mini_sentry.cc@68
PS10, Line 68: void MiniSentry::EnableHms(string hms_uris) {
We should CHECK(!sentry_process_) here and in the other new setters.
http://gerrit.cloudera.org:8080/#/c/11956/10/src/kudu/sentry/mini_sentry.cc@137
PS10, Line 137: Status wait = WaitForTcpBind(sentry_process_->pid(), _,
ip_,
If port_ isn't 0 before this call, we might want to check that its value hasn't
changed afterwards. The port should have only changed if the original value was
0.
--
To view, visit http://gerrit.cloudera.org:8080/11956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 14 Dec 2018 05:26:16 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], spn, ktpath, > Yeah, and looks like Hive 4 isn't even released yet. Making our own fork of Sure, though with the new approach you suggested which can avoid restart for Sentry, do you think this is still relevant? http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto@65 PS8, Line 65: // Whether or not the Sentry service should be set up. > Sure, please revert this hunk then. Done -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 14 Dec 2018 00:39:13 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#10). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 516 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/10 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99: std::string ip_{"0.0.0.0"};
> Sorry for the confusion; my comment was just surprise that "Foo f_ = ..." s
Thanks for the explanation. Yes, this is legal.
Referred ยง12.6.2/8 of C++11 draft
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf):
In a non-delegating constructor, if a given non-static data member or base
class is not designated by a mem-initializer-id (including the case
where there is no mem-initializer-list because the constructor has no
ctor-initializer) and the entity is not a virtual base class of an abstract
class (10.4), then if the entity is a non-static data member that has a
brace-or-equal-initializer, the entity is initialized as specified in 8.5;
For narrowing conversion, I agree this is not related to the usage here.
I will switch it back.
--
To view, visit http://gerrit.cloudera.org:8080/11956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 02:24:11 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99: std::string ip_{"0.0.0.0"};
> After accommodated your comment I thought list initialization is more commo
Sorry for the confusion; my comment was just surprise that "Foo f_ = ..." style
member initialization was legal. If it is, great. I think it's more
approachable than "Foo f_{ ... }" so all else being equal I'd revert this back
to how it was before.
What did you mean by not allowing narrowing conversion? We're not dealing with
integer types here, so I'm not seeing the connection.
--
To view, visit http://gerrit.cloudera.org:8080/11956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 01:05:21 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 9:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257
PS2, Line 257: hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"],
spn, ktpath,
> Hmm, it looks like so. Although all of our work are based Hive 2.3.x curren
Yeah, and looks like Hive 4 isn't even released yet. Making our own fork of
Hive 2.3.x to apply this one patch is probably too much work, so I guess we'll
have to live with this.
When you merge, could you file a Kudu JIRA to track the progression of this
Hive feature, so that when we do upgrade to a version of Hive that supports it,
we'll be able to simplify this code?
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/mini-cluster/external_mini_cluster.cc@269
PS9, Line 269: // Restart Sentry with the HMS address.
Even though we can't configure HMS's bind address, I think we can still avoid a
restart:
1. Generate a UNIQUE_LOOPBACK address for Sentry.
2. Statically choose a non-zero port that Sentry will use. Since Sentry will
live on its own UNIQUE_LOOPBACK, there's no danger of collision with anything
else and you could pick whatever port number you wanted.
3. Start HMS configured to talk to Sentry at the address you got in #1 and port
you got in #2.
4. Start Sentry configured at the address/port from #1/#2, and configured to
talk to the HMS from #3.
Granted, this only works well for UNIQUE_LOOPBACK, so macOS will need to do the
restart thing. Would the code get too messy with a UNIQUE_LOOPBACK case and a
non-UNIQUE_LOOPBACK case? FWIW I'd have a pretty high tolerance for messy code
in this case because the payoff is so significant (faster test run time on
Linux).
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:
http://gerrit.cloudera.org:8080/#/c/11956/9/src/kudu/sentry/mini_sentry.h@99
PS9, Line 99: std::string ip_{"0.0.0.0"};
Now I'm confused; why did you switch from copy init to value init? Copy init is
more familiar to most people.
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto@65
PS8, Line 65: // Whether or not the Sentry service should be set up.
> Would you mind I do this in a follow up patch? This reminds me to add a tes
Sure, please revert this hunk then.
--
To view, visit http://gerrit.cloudera.org:8080/11956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59
Gerrit-Change-Number: 11956
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 13 Dec 2018 00:03:23 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 9: (17 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322 PS2, Line 322: Configures the HMS to use the Sentry post-event listener, which : // synchronizes the HMS events with t > Ah, I think my confusion is in separating out the rules for the HMS vs the Discussed with Andrew offline. Updated it based on his suggestion. Although note that config is not specific to Kudu instead is for entire HMS operations. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@79 PS8, Line 79: sentry_service_principal > nit: perhaps DCHECK this isn't empty? Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@229 PS8, Line 229: : static const string kHiveFileTemplate = R"( : nit: should be a part of kHiveSentryFileTemplate? Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@310 PS8, Line 310: : // - hive.metastore.filter.hook : // Configures the HMS to use the Sentry plugin for filtering : // out information user has no priv > Does this mean that we'll only see HMS notifications that the `kudu` user h This is only for SHOWTABLES and SHOWDATABASES. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@321 PS8, Line 321: ve.metastore.event.listeners : // Configures the HMS to use the Sentry post-event listener, which : // synchronizes the HMS events with the Sentry service. The Sentry : // service will be made aware of events like table renames and : // update itself accor > nit reword a bit: It is not. That is for HDFS. Reword based on your recommendation, although it is synchronizing the HMS events with the Sentry service not the other way around. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@46 PS8, Line 46: using std::string; > warning: using decl 'ExternalMiniCluster' is unused [misc-unused-using-decl Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@51 PS8, Line 51: // integration enabled. > warning: using decl 'vector' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@95 PS8, Line 95: > nit: kTableIdentifier Done http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], spn, ktpath, > Search for THRIFT_BIND_HOST in HIVE-20794. Does that address HIVE-18998? It Hmm, it looks like so. Although all of our work are based Hive 2.3.x currently. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc@231 PS8, Line 231: 2. Start the HMS, configured to talk to the Sentry service. Find out : //which port it's on > It's been a while since we talked about it, but is it not possible that ano Done http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc@64 PS8, Line 64: case EXTERNAL_SERVER: : idx = kServersMaxNum / 3 + index; > Somewhat of a nit: this is giving the larger partition to the masters, when Given the kServerMaxNum is 62, that means TSERVER can range from [1,20) I think this is enough for our test cases. But I can switch the calculation between master and tserver. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc File src/kudu/sentry/mini_sentry.cc: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@114 PS8, Line 114: 127.0.0.1 > Why doesn't this need to be the HMS URIs? This 'hack' works, because we don't use other IP address for the HMS. And I would like to keep it this way as it is simple. And we can always change it if it no longer applies.
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#9). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/tools/tool.proto M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 16 files changed, 516 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/9 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@257 PS2, Line 257: } > Unfortunately, this is not an option. As surprisedly, HMS always does wildc Search for THRIFT_BIND_HOST in HIVE-20794. Does that address HIVE-18998? It was merged with the fix version 4.0.0. Is that an HMS artifact we can use for our own testing? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/sentry/mini_sentry.h File src/kudu/sentry/mini_sentry.h: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/sentry/mini_sentry.h@93 PS2, Line 93: > Yeah, in class member initialization is allowed in C++11, https://isocpp.or Ah, I thought you had to use value initialization (i.e. surround the value with braces). Didn't realize copy initialization works too. http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/tools/tool.proto@65 PS8, Line 65: // Whether or not the Sentry service should be set up. This isn't sufficient; you also have to update tool_action_test.cc (I think) to configure the ExternalMiniCluster the right way if this is set. And a new test in kudu-tool-test would be nice too. -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 11 Dec 2018 05:42:48 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 8:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322
PS2, Line 322: which synchronizes the notification logs with the Sentry
: // service. So that Sentry service is
> Yeah.
Ah, I think my confusion is in separating out the rules for the HMS vs the
rules for Kudu. Maybe it's worth stating explicitly that it's verifying that
changes to Kudu metadata entries in the HMS are performed by `kudu` (or
whatever user it actually is).
"authorization metadata operations" is a bit ambiguous IMO since it can refer
to authorization for DDL/DML that starts in Kudu and ends up in the HMS, and
those DDL/DML operations do need to be authorized, but are referring to a
different user.
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@79
PS8, Line 79: sentry_service_principal
nit: perhaps DCHECK this isn't empty?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@229
PS8, Line 229: - hive.sentry.conf.url
: // Configuration URL of the Sentry authorization plugin in
the HMS.
: //
nit: should be a part of kHiveSentryFileTemplate?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@310
PS8, Line 310: - hive.metastore.filter.hook
: // Configures the HMS to use the Sentry plugin for
filtering
: // out information user has no privileges to access for
operations
: // as SHOWTABLES and SHOWDATABASES.
Does this mean that we'll only see HMS notifications that the `kudu` user has
Sentry privileges for? Or is this specific to SHOWTABLES and SHOWDATABASES
(SHOW TABLES and SHOW DATABASES?) requests to the HMS?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/hms/mini_hms.cc@321
PS8, Line 321: Configures the HMS to use the Sentry post-event listener
: // which synchronizes the notification logs with the
Sentry
: // service. So that Sentry service is aware of events
such
: // as table rename or the table location update, and
accordingly
: // updates internally.
nit reword a bit:
"Configures the HMS to use the Sentry post-event listener, which synchronizes
the Sentry service with the HMS via Sentry's notification log listener. The
Sentry service will be made aware of events like table renames and update
itself accordingly."
Also, what is a "table location update"? It's not the location of a Kudu table,
is it?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/integration-tests/master_sentry-itest.cc@95
PS8, Line 95: Substitute("$0.$1", kDatabaseName, kTableName)
nit: kTableIdentifier
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/external_mini_cluster.cc@231
PS8, Line 231: 3. Restart the Sentry service on the same port, reconfigured to
talk
: //to the HMS's port.
It's been a while since we talked about it, but is it not possible that another
process will claim this port? If so, why not? Mind putting the answer in a
comment either here or around GetBindIpForExternalServer?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/mini-cluster/mini_cluster.cc@64
PS8, Line 64: case EXTERNAL_SERVER:
: idx = kServersMaxNum / 3 + index;
Somewhat of a nit: this is giving the larger partition to the masters, when
really, it's the tablet servers that we would expect to dominate the idx-space
in a larger EMC deployment (if we wanted to test such large clusters).
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@114
PS8, Line 114: 127.0.0.1
Why doesn't this need to be the HMS URIs?
http://gerrit.cloudera.org:8080/#/c/11956/8/src/kudu/sentry/mini_sentry.cc@264
PS8, Line 264: kudu,hive
Should this depend on IsHmsEnabled()?
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/CMakeLists.txt
File src/kudu/integration-tests/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/CMakeLists.txt@91
PS3, Line 91: ADD_KUDU_TEST(master_sentry-itest RUN_SERIAL true PROCESSORS 4)
> How did you arrive at the PROCESSORS value? And why use RUN_SERIAL here?
I think this is similar to the test in master_hms-itest, so I used its config
as a reference.
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@55
PS3, Line 55: const ExternalMiniCluster::BindMode bind_mode;
> Looks like you're not using this param in SetUp(). Nor do I think you need
Done
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@71
PS3, Line 71: opts.enable_sentry = true;
> What happens if enable_sentry=true but enable_kerberos=false? The MiniSentr
Yes. In the follow up patch I removed the situation when enable_kerberos=false
since it doesn't make much sense for Sentry integration test.
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@74
PS3, Line 74: opts.num_masters = 1;
: opts.num_tablet_servers = 1;
> These are the defaults; you can omit this.
Done
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@93
PS3, Line 93: TestFoo
> Rename to something more useful.
I revised this test to have a meaningful test in
https://gerrit.cloudera.org/#/c/11797/3/src/kudu/integration-tests/master_sentry-itest.cc
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@111
PS3, Line 111: exist
> Nit: exists
Done
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@116
PS3, Line 116: const auto& params = GetParam();
> Only used in one place below, so just inline GetParam().
Done
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/mini-cluster/mini_cluster.cc@52
PS3, Line 52: int idx;
: switch (type) {
: case MASTER:
: idx = kServersMaxNum - index;
: break;
: case TSERVER:
: idx = index + 1;
: break;
: case EXTERNAL_SERVER:
: idx = kServersMaxNum / 3 + index;
: break;
> Not really your fault, but could you add a comment here walking through wha
Done
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@a421
PS3, Line 421:
:
> This structure of the pairs is lost in the new comment. Mind keeping it aro
Done
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@402
PS3, Line 402: // The '-Ffn' flag gets lsof to output something like:
: // p5801
: // f548
: // n127.0.0.1:43954->127.0.0.1:43617
: // f549
: // n*:8038
> Hrm, I think I'm missing something. The command didn't change, right? Why d
The output format didn't change, but the way we parse the output is only good
for wildcard binding (n*).
http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@425
PS3, Line 425: Status s = Subprocess::Call(cmd, "", _out).AndThen([&]
() {
: StripTrailingNewline(_out);
: vector lines = strings::Split(lsof_out, "\n");
: for (int index = 2; index < lines.size(); index += 2) {
: StringPiece cur_line(lines[index]);
: if (HasPrefixString(cur_line.ToString(), addr_pattern) &&
: !cur_line.contains("->")) {
: if
(!safe_strto32(lines[index].substr(addr_pattern.size()), )) {
: return Status::RuntimeError("unexpected lsof
output", lsof_out);
: }
:
: return Status::OK();
: }
: }
:
: return Status::RuntimeError("unexpected lsof output",
lsof_out);
: });
> nit: merge these `if`s into one conditional block? Also mind adding a comme
The explanation is added in the previous
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#8). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/tools/tool.proto M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 16 files changed, 517 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/8 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/security/test/mini_kdc.cc File src/kudu/security/test/mini_kdc.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/security/test/mini_kdc.cc@157 PS2, Line 157: kdc_ > Doc with inline comment. Done -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 10 Dec 2018 02:16:27 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 7:
(42 comments)
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@13
PS2, Line 13: one appro
> nit: this reads a bit like this is what's implemented. Mind changing it to
Done
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@15
PS2, Line 15: 1. Start the Sentry service. Find out which port it's on. At
this
> Good explanation, but I'd make two small tweaks to improve readability:
Done
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@21
PS2, Line 21:
> Nit: where
Done
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@22
PS2, Line 22: ver, there could be
> Nit: "same port during the restart in step 3."
Done
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@24
PS2, Line 24: SO_REUSEPORT socket option, which permits multiple sockets to be
bound
: to an identical socket address
> Nit: I'd rewrite as "Although both Sentry and HMS are written in Java, only
Done
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@26
PS2, Line 26: [1]. Meth
> Nit: "they are hacky"
Done
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@33
PS2, Line 33: collision.
> Wrong link here; didn't you mean the SO_REUSEPORT anchor?
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.h@54
PS2, Line 54: d EnableSentry(const HostPort& sentry_address,
> nit: doc what 'sentry_service_principal' is? What happens when it's boost::
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@81
PS2, Line 81: ess_ = sentry_address.ToSt
> nit: maybe add the sentry address?
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@84
PS2, Line 84:
> If you're going to go through the trouble to wrap sentry_service_principal
Sorry for the confusion. I found it is more straightforward to always pass the
service_principal and use keytab_file as an indicator of if Kerberos is
enabled, the same as all other places are doing.
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@164
PS2, Line 164: if (!wait.ok()) {
> nit: Can you add a comment about why `none` is appropriate here?
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@164
PS2, Line 164: if (!wait.ok()) {
> Should use /*foo=*/ inline comments to help explain what 'none' means.
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@306
PS2, Line 306:
: string sentry_properties;
: if (IsAuthorizationEnabled()) {
:
> Shouldn't this also be conditioned on IsAuthorizationEnabled? Otherwise the
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@318
PS2, Line 318: authorization validation.
: //
> I'm not sure I understand this. What is a "read result" in this context?
I am not super clear on this config as well, I cannot find any formal
documentations on this. By reading the code, it only adds filtering for
SHOWDATABASES and SHOWTABLES. I will make it more clear based on my
understanding.
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322
PS2, Line 322: which synchronizes the notification logs with the Sentry
: // service. So that Sentry service is
> Just making sure I understand this, this is configured to ensure the HMS wi
Yeah.
For 'What is considered an authorization metadata operation?', do you mean what
kinds of operation are considered to consult for authorization metadata?
Similar to what we do in Kudu, mostly are DDL and DML operations. A complete
list can be found here:
https://www.cloudera.com/documentation/enterprise/latest/topics/sentry_privileges_hive_impala.html.
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@327
PS2, Line 327:
: hive.sentr
> Just making sure I understand this, this is synchronizing authz metadata en
This is to sync events in the HMS with Sentry service, so that Sentry knows
about the tables has been renamed, or the location of the table has changed.
And it updates these changes accordingly in Sentry. Update the comment to
explain in more detail.
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@328
PS2, Line 328: ve.sentr
> nit: missing space
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@328
PS2, Line 328: hive.sentry.conf.url
> Nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@355
PS2, Line 355:
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#7). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, one approach would be to: 1. Start the Sentry service. Find out which port it's on. At this stage, the Sentry service has no knowledge of any HMS service. 2. Start the HMS, configured to talk to Sentry's port. Find out which port it's on. 3. Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition where another program binds to the same port during the restart in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but they are hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEPORT Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/tools/tool.proto M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 16 files changed, 537 insertions(+), 69 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/7 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#5). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, we can do: * Start the Sentry service. Find out which port it's on. * Start the HMS, configured to talk to Sentry's port. Find out which port it's on. * Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition that another program binds to the same port in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Though both Sentry and HMS are written in Java, and only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but is hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEADDR Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 488 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/5 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#4). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, we can do: * Start the Sentry service. Find out which port it's on. * Start the HMS, configured to talk to Sentry's port. Find out which port it's on. * Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition that another program binds to the same port in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Though both Sentry and HMS are written in Java, and only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but is hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEADDR Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 488 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/4 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster
..
Patch Set 3:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@13
PS2, Line 13: we can do
nit: this reads a bit like this is what's implemented. Mind changing it to "one
approach would be to:"
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.h@54
PS2, Line 54: Configures the mini HMS to enable the Sentry plugin.
nit: doc what 'sentry_service_principal' is? What happens when it's boost::none?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@81
PS2, Line 81: "Enabling Sentry for HMS";
nit: maybe add the sentry address?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@164
PS2, Line 164: Status wait = WaitForTcpBind(hms_process_->pid(), _, none,
nit: Can you add a comment about why `none` is appropriate here?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@318
PS2, Line 318: Configures the HMS to use the Sentry plugin for filtering
: // read results.
I'm not sure I understand this. What is a "read result" in this context?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322
PS2, Line 322: Configures the HMS to use the Sentry event listener for
: // authorization metadata operations.
Just making sure I understand this, this is configured to ensure the HMS will
consult Sentry for authorization metadata? What is considered an authorization
metadata operation?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@328
PS2, Line 328: service.
nit: missing space
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@327
PS2, Line 327: synchronizes the notification logs with the Sentry
: //service.
Just making sure I understand this, this is synchronizing authz metadata
entries in the HMS with changes in Sentry? I'm unsure of where the notification
logs come into play.
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@359
PS2, Line 359: HNS
HMS
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@55
PS2, Line 55: const ExternalMiniCluster::BindMode bind_mode;
Hrm, where is this used?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@86
PS2, Line 86: { ExternalMiniCluster::BindMode::LOOPBACK, true, },
: { ExternalMiniCluster::BindMode::LOOPBACK, false, },
Mind commenting what the behavior for this test is for mac?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@103
PS2, Line 103: Substitute("$0.$1", kDatabaseName, kTableName)
nit: maybe pull this out into a kTableIdentifier?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@117
PS2, Line 117: if (params.enable_kerberos) {
: // Create a principal 'kudu' and configure to use it.
: ASSERT_OK(cluster_->kdc()->CreateUserPrincipal("kudu"));
: ASSERT_OK(cluster_->kdc()->Kinit("kudu"));
: ASSERT_OK(cluster_->kdc()->SetKrb5Environment());
: hms_client_opts.enable_kerberos = true;
: hms_client_opts.service_principal = "hive";
: }
: hms::HmsClient hms_client(cluster_->hms()->address(),
hms_client_opts);
: ASSERT_OK(hms_client.Start());
:
: hive::Table hms_table;
: ASSERT_OK(hms_client.GetTable(kDatabaseName, kTableName,
_table));
Are there authz metadata entries we should be checking here?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.h@159
PS2, Line 159: // If true, set up a Sentry service as part of this
ExternalMiniCluster.
nit: Is there a default worth mentioning here?
http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.h@419
PS2, Line 419: // Index to identify the number of external servers being
started.
I'm a little confused about this variable. Usually an index is a pointer into
some list, but this comment makes it out to be some kind of count? Mind
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 3: (34 comments) http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@15 PS2, Line 15: * Start the Sentry service. Find out which port it's on. Good explanation, but I'd make two small tweaks to improve readability: 1. Make it a numbered list (i.e 1, 2, 3) in order to emphasize the order of events 2. For the first step, point out that the Sentry service is configured without any knowledge of an HMS service. It'll start up but will be useless otherwise. http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@21 PS2, Line 21: that Nit: where http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@22 PS2, Line 22: same port in step 3 Nit: "same port during the restart in step 3." http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@24 PS2, Line 24: Though both Sentry and HMS are written in Java, and only JDK9 (and above) : supports this socket option[1] Nit: I'd rewrite as "Although both Sentry and HMS are written in Java, only JDK9 (and above) supports this socket option[1]". http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@26 PS2, Line 26: is hacky. Nit: "they are hacky" http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@33 PS2, Line 33: [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEADDR Wrong link here; didn't you mean the SO_REUSEPORT anchor? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@84 PS2, Line 84: sentry_service_principal_ = *sentry_service_principal; If you're going to go through the trouble to wrap sentry_service_principal in an optional to reflect its optionality in the EnableSentry() function, shouldn't that optionality carry over into the sentry_service_principal_ state as well, so that methods like IsAuthorizationEnabled() are more clear? Then you could also make this a simpler optional and std::move it into place. http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@164 PS2, Line 164: Status wait = WaitForTcpBind(hms_process_->pid(), _, none, Should use /*foo=*/ inline comments to help explain what 'none' means. http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@306 PS2, Line 306: : hive.sentry.conf.url : file://$1/hive-sentry-site.xml : Shouldn't this also be conditioned on IsAuthorizationEnabled? Otherwise there will have been no hive-sentry-site.xml file. http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@328 PS2, Line 328: //service. Nit: indentation http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@355 PS2, Line 355: sentry_address_); Where is this substituted? I don't see a $7 in kHiveFileTemplate. http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@359 PS2, Line 359: HNS HMS http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@363 PS2, Line 363: // Set of service users which is safe to be excluded from the : // Sentry authorization checks. Nit: I'd make this a bit more explicit "Set of service users whose access will be excluded from Sentry authorization checks." http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@366 PS2, Line 366: The indentation here is pretty confusing w.r.t. the IsAuthorizationEnabled condition. Maybe shift this block all the way left? http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/CMakeLists.txt@91 PS3, Line 91: ADD_KUDU_TEST(master_sentry-itest RUN_SERIAL true PROCESSORS 4) How did you arrive at the PROCESSORS value? And why use RUN_SERIAL here? http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@55 PS3, Line 55: const ExternalMiniCluster::BindMode bind_mode; Looks like you're not using this param in SetUp(). Nor do I think you need to; the default BindMode is already taken care of in mini_cluster.h. http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/integration-tests/master_sentry-itest.cc@71 PS3, Line 71: opts.enable_sentry = true; What happens if enable_sentry=true but enable_kerberos=false? The MiniSentry is started but not configured to be used?
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#3). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, we can do: * Start the Sentry service. Find out which port it's on. * Start the HMS, configured to talk to Sentry's port. Find out which port it's on. * Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition that another program binds to the same port in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Though both Sentry and HMS are written in Java, and only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but is hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEADDR Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 488 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/3 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has removed a vote on this change. Change subject: [sentry] add mini Sentry to the external mini cluster .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 ) Change subject: [sentry] add mini Sentry to the external mini cluster .. Patch Set 2: Verified+1 Unrelated flaky TraceEventCallbackTest. -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 19 Nov 2018 21:21:08 + Gerrit-HasComments: No
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11956 to look at the new patch set (#2). Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, we can do: * Start the Sentry service. Find out which port it's on. * Start the HMS, configured to talk to Sentry's port. Find out which port it's on. * Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition that another program binds to the same port in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Though both Sentry and HMS are written in Java, and only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but is hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEADDR Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/sentry/sentry-test-base.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 15 files changed, 488 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/2 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add mini Sentry to the external mini cluster
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11956 Change subject: [sentry] add mini Sentry to the external mini cluster .. [sentry] add mini Sentry to the external mini cluster This commit enables external mini cluster to be able to start with both mini sentry and mini hms. A major challenge is the circular dependency between the HMS and Sentry in terms of configuring each with the other's address, including port which is currently discovered by polling lsof. To work around it, we can do: * Start the Sentry service. Find out which port it's on. * Start the HMS, configured to talk to Sentry's port. Find out which port it's on. * Restart the Sentry service on the same port, reconfigured to talk to the HMS's port. However, there could be a race condition that another program binds to the same port in step 3. One option is to use SO_REUSEPORT socket option, which permits multiple sockets to be bound to an identical socket address. Though both Sentry and HMS are written in Java, and only JDK9 (and above) supports this socket option[1]. Methods such as Java reflection can be used to invoke private methods to set up this option, but is hacky. This patch extends the UNIQUE_LOOPBACK usage in mini cluster, so that external servers such as Sentry can pick up a unique bind address, knowing that in doing so there'll never be any danger of a port collision. [1]: https://docs.oracle.com/javase/9/docs/api/java/net/StandardSocketOptions.html#SO_REUSEADDR Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 --- M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/security/test/mini_kdc.cc M src/kudu/sentry/mini_sentry.cc M src/kudu/sentry/mini_sentry.h M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 14 files changed, 484 insertions(+), 62 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11956/1 -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
