Skip to content

Commit 3478fc9

Browse files
committed
[KYUUBI #5717] Infer the proxy user automatically for delete batch operation
# 🔍 Description Infer the batch user from session or metadata, user do not need to specify the proxy user anymore. This pr also align the behavior of BatchesResource with that of SessionsResource and OperationsResource(no proxy user parameter). For Kyuubi Batch, Session and Operation, these resources have the user attiribute. So we only need to check whether the authentication user has the permission to access the resource. ## Issue References 🔗 This pull request fixes # ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [x] Pull request title is okay. - [x] No license issues. - [x] Milestone correctly set? - [ ] Test coverage is ok - [x] Assignees are selected. - [x] Minimum number of approvals - [x] No changes are requested **Be nice. Be informative.** Closes #5717 from turboFei/hive_server2_proxy_user. Closes #5717 70ad7e7 [fwang12] comment c721a75 [fwang12] ignore da92bd5 [fwang12] fix ut 9a197d0 [fwang12] doc c8ed5f9 [fwang12] ut cef9e32 [fwang12] do not use proxy user Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: fwang12 <fwang12@ebay.com>
1 parent 0bcd107 commit 3478fc9

File tree

10 files changed

+34
-69
lines changed

10 files changed

+34
-69
lines changed

docs/client/rest/rest_api.md

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -410,16 +410,6 @@ The [Batch](#batch).
410410

411411
Kill the batch if it is still running.
412412

413-
#### Request Parameters
414-
415-
| Name | Description | Type |
416-
|:------------------------|:------------------------------|:-----------------|
417-
| proxyUser | the proxy user to impersonate | String(optional) |
418-
| hive.server2.proxy.user | the proxy user to impersonate | String(optional) |
419-
420-
`proxyUser` is an alternative to `hive.server2.proxy.user`, and the current behavior is consistent with
421-
`hive.server2.proxy.user`. When both parameters are set, `proxyUser` takes precedence.
422-
423413
#### Response Body
424414

425415
| Name | Description | Type |

docs/deployment/migration-guide.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@
2121

2222
* Since Kyuubi 1.9.0, `kyuubi.session.conf.advisor` can be set as a sequence, Kyuubi supported chaining SessionConfAdvisors.
2323

24+
## Upgrading from Kyuubi 1.8.0 to 1.8.1
25+
26+
* Since Kyuubi 1.8.1, for `DELETE /batches/${batchId}`, `hive.server2.proxy.user` is not needed in the request parameters.
27+
2428
## Upgrading from Kyuubi 1.7 to 1.8
2529

2630
* Since Kyuubi 1.8, SQLite is added and becomes the default database type of Kyuubi metastore, as Derby has been deprecated.

kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/cmd/delete/DeleteBatchCommand.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ class DeleteBatchCommand(cliConfig: CliConfig) extends Command[Batch](cliConfig)
3636
val batchRestApi: BatchRestApi = new BatchRestApi(kyuubiRestClient)
3737
val batchId = normalizedCliConfig.batchOpts.batchId
3838

39-
val result = batchRestApi.deleteBatch(batchId, normalizedCliConfig.commonOpts.hs2ProxyUser)
39+
val result = batchRestApi.deleteBatch(batchId)
4040

4141
info(JsonUtils.toJson(result))
4242

kyuubi-ctl/src/test/scala/org/apache/kyuubi/ctl/BatchCliArgumentsSuite.scala

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -119,18 +119,6 @@ class BatchCliArgumentsSuite extends KyuubiFunSuite with TestPrematureExit {
119119
}
120120
}
121121

122-
test("delete batch with hs2ProxyUser") {
123-
val args = Array(
124-
"delete",
125-
"batch",
126-
"f7fd702c-e54e-11ec-8fea-0242ac120002",
127-
"--hs2ProxyUser",
128-
"b_user")
129-
val opArgs = new ControlCliArguments(args)
130-
assert(opArgs.cliConfig.batchOpts.batchId == "f7fd702c-e54e-11ec-8fea-0242ac120002")
131-
assert(opArgs.cliConfig.commonOpts.hs2ProxyUser == "b_user")
132-
}
133-
134122
test("test list batch option") {
135123
val args = Array(
136124
"list",

kyuubi-rest-client/src/main/java/org/apache/kyuubi/client/BatchRestApi.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,11 @@
2323
import org.apache.kyuubi.client.api.v1.dto.*;
2424
import org.apache.kyuubi.client.util.JsonUtils;
2525
import org.apache.kyuubi.client.util.VersionUtils;
26+
import org.slf4j.Logger;
27+
import org.slf4j.LoggerFactory;
2628

2729
public class BatchRestApi {
30+
static final Logger LOG = LoggerFactory.getLogger(BatchRestApi.class);
2831

2932
private KyuubiRestClient client;
3033

@@ -101,14 +104,27 @@ public OperationLog getBatchLocalLog(String batchId, int from, int size) {
101104
return this.getClient().get(path, params, OperationLog.class, client.getAuthHeader());
102105
}
103106

107+
/**
108+
* hs2ProxyUser for delete batch is deprecated since 1.8.1, please use {@link
109+
* #deleteBatch(String)} instead.
110+
*/
111+
@Deprecated
104112
public CloseBatchResponse deleteBatch(String batchId, String hs2ProxyUser) {
113+
LOG.warn(
114+
"The method `deleteBatch(batchId, hs2ProxyUser)` is deprecated since 1.8.1, "
115+
+ "using `deleteBatch(batchId)` instead.");
105116
Map<String, Object> params = new HashMap<>();
106117
params.put("hive.server2.proxy.user", hs2ProxyUser);
107118

108119
String path = String.format("%s/%s", API_BASE_PATH, batchId);
109120
return this.getClient().delete(path, params, CloseBatchResponse.class, client.getAuthHeader());
110121
}
111122

123+
public CloseBatchResponse deleteBatch(String batchId) {
124+
String path = String.format("%s/%s", API_BASE_PATH, batchId);
125+
return this.getClient().delete(path, null, CloseBatchResponse.class, client.getAuthHeader());
126+
}
127+
112128
private IRestClient getClient() {
113129
return this.client.getHttpClient();
114130
}

kyuubi-rest-client/src/test/java/org/apache/kyuubi/client/BatchRestClientTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,13 +267,13 @@ public void getOperationLogTest() {
267267
public void deleteBatchTest() {
268268
// test spnego auth
269269
BatchTestServlet.setAuthSchema(NEGOTIATE_AUTH);
270-
CloseBatchResponse response = spnegoBatchRestApi.deleteBatch("71535", "b_test");
270+
CloseBatchResponse response = spnegoBatchRestApi.deleteBatch("71535");
271271
assertTrue(response.isSuccess());
272272

273273
// test basic auth
274274
BatchTestServlet.setAuthSchema(BASIC_AUTH);
275275
BatchTestServlet.allowAnonymous(false);
276-
response = basicBatchRestApi.deleteBatch("71535", "b_test");
276+
response = basicBatchRestApi.deleteBatch("71535");
277277
assertTrue(response.isSuccess());
278278
}
279279
}

kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import java.util.{Collections, Locale, UUID}
2323
import java.util.concurrent.ConcurrentHashMap
2424
import javax.ws.rs._
2525
import javax.ws.rs.core.MediaType
26-
import javax.ws.rs.core.Response.Status
2726

2827
import scala.collection.JavaConverters._
2928
import scala.util.{Failure, Success, Try}
@@ -448,19 +447,7 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
448447
description = "close and cancel a batch session")
449448
@DELETE
450449
@Path("{batchId}")
451-
def closeBatchSession(
452-
@PathParam("batchId") batchId: String,
453-
@QueryParam("proxyUser") kyuubiProxyUser: String,
454-
@QueryParam("hive.server2.proxy.user") hs2ProxyUser: String): CloseBatchResponse = {
455-
456-
def checkPermission(operator: String, owner: String): Unit = {
457-
if (operator != owner) {
458-
throw new WebApplicationException(
459-
s"$operator is not allowed to close the session belong to $owner",
460-
Status.METHOD_NOT_ALLOWED)
461-
}
462-
}
463-
450+
def closeBatchSession(@PathParam("batchId") batchId: String): CloseBatchResponse = {
464451
def forceKill(
465452
appMgrInfo: ApplicationManagerInfo,
466453
batchId: String,
@@ -472,18 +459,15 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
472459
(killed, message)
473460
}
474461

475-
val activeProxyUser = Option(kyuubiProxyUser).getOrElse(hs2ProxyUser)
476-
val userName = fe.getSessionUser(activeProxyUser)
477-
478462
val sessionHandle = formatSessionHandle(batchId)
479463
sessionManager.getBatchSession(sessionHandle).map { batchSession =>
480-
checkPermission(userName, batchSession.user)
464+
fe.getSessionUser(batchSession.user)
481465
sessionManager.closeSession(batchSession.handle)
482466
val (killed, msg) = batchSession.batchJobSubmissionOp.getKillMessage
483467
new CloseBatchResponse(killed, msg)
484468
}.getOrElse {
485469
sessionManager.getBatchMetadata(batchId).map { metadata =>
486-
checkPermission(userName, metadata.username)
470+
fe.getSessionUser(metadata.username)
487471
if (OperationState.isTerminal(OperationState.withName(metadata.state))) {
488472
new CloseBatchResponse(false, s"The batch[$metadata] has been terminated.")
489473
} else if (batchV2Enabled(metadata.requestConf) && metadata.state == "INITIALIZED" &&
@@ -494,21 +478,21 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
494478
} else if (batchV2Enabled(metadata.requestConf) && metadata.kyuubiInstance == null) {
495479
// code goes here indicates metadata is outdated, recursively calls itself to refresh
496480
// the metadata
497-
closeBatchSession(batchId, kyuubiProxyUser, hs2ProxyUser)
481+
closeBatchSession(batchId)
498482
} else if (metadata.kyuubiInstance != fe.connectionUrl) {
499483
info(s"Redirecting delete batch[$batchId] to ${metadata.kyuubiInstance}")
500484
val internalRestClient = getInternalRestClient(metadata.kyuubiInstance)
501485
try {
502-
internalRestClient.deleteBatch(userName, batchId)
486+
internalRestClient.deleteBatch(metadata.username, batchId)
503487
} catch {
504488
case e: KyuubiRestException =>
505489
error(s"Error redirecting delete batch[$batchId] to ${metadata.kyuubiInstance}", e)
506-
val (killed, msg) = forceKill(metadata.appMgrInfo, batchId, userName)
490+
val (killed, msg) = forceKill(metadata.appMgrInfo, batchId, metadata.username)
507491
new CloseBatchResponse(killed, if (killed) msg else Utils.stringifyException(e))
508492
}
509493
} else { // should not happen, but handle this for safe
510494
warn(s"Something wrong on deleting batch[$batchId], try forcibly killing application")
511-
val (killed, msg) = forceKill(metadata.appMgrInfo, batchId, userName)
495+
val (killed, msg) = forceKill(metadata.appMgrInfo, batchId, metadata.username)
512496
new CloseBatchResponse(killed, msg)
513497
}
514498
}.getOrElse {

kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/InternalRestClient.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ class InternalRestClient(
6060

6161
def deleteBatch(user: String, batchId: String): CloseBatchResponse = {
6262
withAuthUser(user) {
63-
internalBatchRestApi.deleteBatch(batchId, null)
63+
internalBatchRestApi.deleteBatch(batchId)
6464
}
6565
}
6666

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ abstract class BatchesResourceSuiteBase extends KyuubiFunSuite
211211
.header(AUTHORIZATION_HEADER, basicAuthorizationHeader(batch.getId))
212212
.delete()
213213
assert(deleteBatchResponse.getStatus === 405)
214-
errorMessage = s"${batch.getId} is not allowed to close the session belong to anonymous"
214+
errorMessage = s"Failed to validate proxy privilege of ${batch.getId} for anonymous"
215215
assert(deleteBatchResponse.readEntity(classOf[String]).contains(errorMessage))
216216

217217
// invalid batchId
@@ -228,16 +228,6 @@ abstract class BatchesResourceSuiteBase extends KyuubiFunSuite
228228
.delete()
229229
assert(deleteBatchResponse.getStatus === 404)
230230

231-
// invalid proxy user
232-
deleteBatchResponse = webTarget.path(s"api/v1/batches/${batch.getId}")
233-
.queryParam("hive.server2.proxy.user", "invalidProxy")
234-
.request(MediaType.APPLICATION_JSON_TYPE)
235-
.header(AUTHORIZATION_HEADER, basicAuthorizationHeader("anonymous"))
236-
.delete()
237-
assert(deleteBatchResponse.getStatus === 405)
238-
errorMessage = "Failed to validate proxy privilege of anonymous for invalidProxy"
239-
assert(deleteBatchResponse.readEntity(classOf[String]).contains(errorMessage))
240-
241231
// check close batch session
242232
deleteBatchResponse = webTarget.path(s"api/v1/batches/${batch.getId}")
243233
.request(MediaType.APPLICATION_JSON_TYPE)

kyuubi-server/src/test/scala/org/apache/kyuubi/server/rest/client/BatchRestApiSuite.scala

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,16 +74,9 @@ class BatchRestApiSuite extends RestClientTestHelper with BatchTestHelper {
7474
}
7575

7676
// delete batch
77-
val closeResp = batchRestApi.deleteBatch(batch.getId(), null)
77+
val closeResp = batchRestApi.deleteBatch(batch.getId())
7878
assert(closeResp.getMsg.nonEmpty)
7979

80-
// delete batch - error
81-
val e = intercept[KyuubiRestException] {
82-
batchRestApi.deleteBatch(batch.getId(), "fake")
83-
}
84-
assert(e.getCause.toString.contains(
85-
s"Failed to validate proxy privilege of ${ldapUser} for fake"))
86-
8780
basicKyuubiRestClient.close()
8881
}
8982

@@ -170,7 +163,7 @@ class BatchRestApiSuite extends RestClientTestHelper with BatchTestHelper {
170163
}
171164

172165
// delete batch
173-
val closeResp = batchRestApi.deleteBatch(batch.getId(), proxyUser)
166+
val closeResp = batchRestApi.deleteBatch(batch.getId())
174167
assert(closeResp.getMsg.nonEmpty)
175168

176169
// list batches

0 commit comments

Comments
 (0)