[KYUUBI #1176] InvalidACL appears in the engine when zookeeper acl is turned on #1177

cxzl25 · 2021-09-28T17:06:14Z

Why are the changes needed?

#1176
When kyuubi.ha.zookeeper.acl.enabled=true, both service and engine will use zookeeper acl to create znode, but engine has no keytab information and cannot write information to zookeeper, throwing an exception.

Caused by: org.apache.zookeeper.KeeperException$InvalidACLException: KeeperErrorCode = InvalidACL for /kyuubi_USER/XXXX
	at org.apache.zookeeper.KeeperException.create(KeeperException.java:124)
	at org.apache.zookeeper.KeeperException.create(KeeperException.java:54)
	at org.apache.zookeeper.ZooKeeper.create(ZooKeeper.java:792)
	at org.apache.kyuubi.shade.org.apache.curator.framework.imps.CreateBuilderImpl$11.call(CreateBuilderImpl.java:740)

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

cxzl25 · 2021-09-28T17:09:10Z

Current

Fix

cxzl25 · 2021-09-28T17:10:18Z

https://github.com/apache/incubator-kyuubi/blob/4f74aba50e683fc7d58e793f92ff6db75e09f7b8/kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/ZooKeeperClientProvider.scala#L91-L104

codecov-commenter · 2021-09-28T18:18:39Z

Codecov Report

Merging #1177 (4a28756) into master (191d30e) will increase coverage by 0.04%.
The diff coverage is 78.75%.

❗ Current head 4a28756 differs from pull request most recent head 0b7cc2e. Consider uploading reports for the commit 0b7cc2e to get more accurate results

@@             Coverage Diff              @@
##             master    #1177      +/-   ##
============================================
+ Coverage     78.79%   78.84%   +0.04%     
  Complexity       93       93              
============================================
  Files           182      182              
  Lines          6654     6683      +29     
  Branches        785      784       -1     
============================================
+ Hits           5243     5269      +26     
- Misses          961      966       +5     
+ Partials        450      448       -2

Impacted Files	Coverage Δ
...ubi/engine/spark/operation/PlanOnlyStatement.scala	`0.00% <0.00%> (ø)`
...la/org/apache/kyuubi/session/AbstractSession.scala	`95.40% <0.00%> (ø)`
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala	`63.88% <46.15%> (-1.39%)`	⬇️
...g/apache/kyuubi/session/KyuubiSessionManager.scala	`94.73% <80.00%> (-2.33%)`	⬇️
...uubi/engine/spark/events/EventLoggingService.scala	`79.16% <100.00%> (-7.20%)`	⬇️
...uubi/engine/spark/operation/ExecuteStatement.scala	`88.75% <100.00%> (-0.14%)`	⬇️
.../engine/spark/session/SparkSQLSessionManager.scala	`92.10% <100.00%> (+0.43%)`	⬆️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`95.33% <100.00%> (+0.08%)`	⬆️
...org/apache/kyuubi/operation/log/OperationLog.scala	`97.82% <100.00%> (+2.17%)`	⬆️
...ala/org/apache/kyuubi/session/SessionManager.scala	`56.36% <100.00%> (+3.42%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 191d30e...0b7cc2e. Read the comment docs.

yaooqinn · 2021-09-29T03:05:31Z

kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionManager.scala

@@ -48,14 +49,18 @@ class KyuubiSessionManager private (name: String) extends SessionManager(name) {

    val username = Option(user).filter(_.nonEmpty).getOrElse("anonymous")

+    val sessionConf = this.getConf.getUserDefaults(user)
+    if (!sessionConf.get(HA_ZK_ACL_ENGINE_ENABLED)) {


It‘s a bit hacky, can we use HA_ZK_ACL_ENGINE_ENABLED at the caller side?

Move to ZooKeeperACLProvider

cxzl25 · 2021-09-29T09:36:29Z

kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/ServiceDiscoverySuite.scala

-    val acl1 = new ZooKeeperACLProvider(conf1).getDefaultAcl
-    assert(acl1.size() === 2)
-    val expected = ZooDefs.Ids.READ_ACL_UNSAFE
-    expected.addAll(ZooDefs.Ids.CREATOR_ALL_ACL)


This (expected.addAll) will modify the original list

pan3793 · 2021-09-30T06:09:33Z

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/ZooKeeperACLProvider.scala

+    }
+
+    if (conf.get(HighAvailabilityConf.HA_ZK_ACL_ENABLED) &&
+      conf.get(HighAvailabilityConf.HA_ZK_ENGINE_REF_ID).isEmpty) {


@ulysses-you does this sufficient to distinguish between the server side and the engine side?

I don’t seem to find other ways to distinguish server and engine, or use main class to distinguish.

… turned on ### _Why are the changes needed?_ #1176 When `kyuubi.ha.zookeeper.acl.enabled=true`, both service and engine will use zookeeper acl to create znode, but engine has no keytab information and cannot write information to zookeeper, throwing an exception. ```java Caused by: org.apache.zookeeper.KeeperException$InvalidACLException: KeeperErrorCode = InvalidACL for /kyuubi_USER/XXXX at org.apache.zookeeper.KeeperException.create(KeeperException.java:124) at org.apache.zookeeper.KeeperException.create(KeeperException.java:54) at org.apache.zookeeper.ZooKeeper.create(ZooKeeper.java:792) at org.apache.kyuubi.shade.org.apache.curator.framework.imps.CreateBuilderImpl$11.call(CreateBuilderImpl.java:740) ``` ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [x] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.readthedocs.io/en/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #1177 from cxzl25/KYUUBI-1176. Closes #1176 ecc08fa [sychen] fix engine acl 0b7cc2e [sychen] fix InvalidACL Authored-by: sychen <sychen@trip.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit d332534) Signed-off-by: Kent Yao <yao@apache.org>

yaooqinn · 2021-10-08T02:09:21Z

thanks, merged to master/1.3

ulysses-you · 2021-10-08T08:15:20Z

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala

+  val HA_ZK_ACL_ENGINE_ENABLED: ConfigEntry[Boolean] =
+    buildConf("ha.zookeeper.acl.engine.enabled")
+      .doc("Set to true if the zookeeper ensemble is kerberized at engine side.")
+      .version("1.4.0")


@cxzl25 sorry for the late, can we send a followup change the version to 1.3.1 ?

branch-1.3 lacks HA_ZK_ENGINE_REF_ID configuration to distinguish between service and engine.
It should be compilation failure now?

good point, seems branch-1.3 would fail. Can you send a PR target to branch-1.3 ?

Do you mean to merge #1032 into branch-1.3? And change the version of ha.engine.ref.id to 1.3.1

….enabled) version #1177 ### _Why are the changes needed?_ change configuration(ha.zookeeper.acl.engine.enabled) version to `1.3.1` ### _How was this patch tested?_ - [] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [] Add screenshots for manual tests if appropriate - [] [Run test](https://kyuubi.readthedocs.io/en/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #1193 from cxzl25/KYUUBI-1176-followup. Closes #1176 72753ae [sychen] change config version Authored-by: sychen <sychen@trip.com> Signed-off-by: ulysses-you <ulyssesyou@apache.org>

….enabled) version #1177 ### _Why are the changes needed?_ change configuration(ha.zookeeper.acl.engine.enabled) version to `1.3.1` ### _How was this patch tested?_ - [] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [] Add screenshots for manual tests if appropriate - [] [Run test](https://kyuubi.readthedocs.io/en/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #1193 from cxzl25/KYUUBI-1176-followup. Closes #1176 72753ae [sychen] change config version Authored-by: sychen <sychen@trip.com> Signed-off-by: ulysses-you <ulyssesyou@apache.org> (cherry picked from commit 464fdf4) Signed-off-by: ulysses-you <ulyssesyou@apache.org>

pan3793 · 2021-10-12T13:39:11Z

Change milestone to 1.4.0, see #1221

fix InvalidACL

0b7cc2e

yaooqinn reviewed Sep 29, 2021

View reviewed changes

fix engine acl

ecc08fa

cxzl25 commented Sep 29, 2021

View reviewed changes

pan3793 approved these changes Sep 30, 2021

View reviewed changes

pan3793 reviewed Sep 30, 2021

View reviewed changes

yaooqinn approved these changes Sep 30, 2021

View reviewed changes

yaooqinn closed this in d332534 Oct 8, 2021

yaooqinn added this to the v1.3.1 milestone Oct 8, 2021

ulysses-you assigned cxzl25 Oct 8, 2021

ulysses-you modified the milestones: v1.3.1, v1.4.0 Oct 8, 2021

ulysses-you reviewed Oct 8, 2021

View reviewed changes

cxzl25 mentioned this pull request Oct 8, 2021

[KYUUBI #1176][FOLLOWUP] change configuration(ha.zookeeper.acl.engine.enabled) version #1193

Closed

pan3793 modified the milestones: v1.3.1, v1.4.0 Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #1176] InvalidACL appears in the engine when zookeeper acl is turned on #1177

[KYUUBI #1176] InvalidACL appears in the engine when zookeeper acl is turned on #1177

cxzl25 commented Sep 28, 2021 •

edited

Loading

cxzl25 commented Sep 28, 2021

cxzl25 commented Sep 28, 2021

codecov-commenter commented Sep 28, 2021

yaooqinn Sep 29, 2021

cxzl25 Sep 29, 2021

cxzl25 Sep 29, 2021 •

edited

Loading

pan3793 Sep 30, 2021

cxzl25 Sep 30, 2021

yaooqinn commented Oct 8, 2021

ulysses-you Oct 8, 2021

cxzl25 Oct 8, 2021

cxzl25 Oct 8, 2021

cxzl25 Oct 8, 2021

ulysses-you Oct 8, 2021

cxzl25 Oct 8, 2021

pan3793 commented Oct 12, 2021

[KYUUBI #1176] InvalidACL appears in the engine when zookeeper acl is turned on #1177

[KYUUBI #1176] InvalidACL appears in the engine when zookeeper acl is turned on #1177

Conversation

cxzl25 commented Sep 28, 2021 • edited Loading

Why are the changes needed?

How was this patch tested?

cxzl25 commented Sep 28, 2021

Current

Fix

cxzl25 commented Sep 28, 2021

codecov-commenter commented Sep 28, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cxzl25 Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 commented Oct 12, 2021

cxzl25 commented Sep 28, 2021 •

edited

Loading

cxzl25 Sep 29, 2021 •

edited

Loading