github-actions[bot] commented on code in PR #67851:
URL: https://github.com/apache/doris/pull/67851#discussion_r3989090778
##########
fs_brokers/cdc_client/src/main/java/org/apache/doris/cdcclient/sink/DorisBatchStreamLoad.java:
##########
@@ -404,6 +405,7 @@ public void load(String label, BatchRecordBuffer buffer)
throws IOException {
.setUrl(loadUrl)
.addProperties(loadProps)
.addTokenAuth(token)
+ .baseAuth(dorisUser, "")
Review Comment:
[P1] Enforce the creator's LOAD privilege for token loads
This still sends the cluster token along with the new Basic username. BE
forwards both fields, but `FrontendServiceImpl.loadTxnBeginImpl` takes the
token branch whenever `request.token` is set and therefore never calls
`checkSingleTablePasswordAndPrivs(..., LOAD)`; the later token commit paths
behave the same. A user can create/start the job with LOAD, have LOAD revoked
(or be dropped), and subsequent CDC rows still begin and commit because the
username is only attribution. Please bind the token request to the job/task,
resolve the persisted creator identity on FE, and check its current LOAD
privilege for each target table; add a revoke-LOAD regression because the
changed test keeps LOAD throughout.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/StreamingJobAction.java:
##########
@@ -50,14 +80,69 @@ public Object reportTaskFailure(@RequestBody
TaskFailureRequest failureRequest,
return failTask(failureRequest);
}
+ @RequestMapping(path = "/api/streaming/schema_change", method =
RequestMethod.POST)
+ public Object executeSchemaChange(@RequestBody Map<String, String> body,
HttpServletRequest request) {
+ checkAuth(request);
+ if (!Env.getCurrentEnv().isMaster()) {
+ return ResponseEntityBuilder.okWithCommonError("Schema change must
be executed on the master FE");
+ }
+ String stmt = body.get("stmt");
+ if (Strings.isNullOrEmpty(stmt)) {
+ return ResponseEntityBuilder.badRequest("Missing statement request
body");
+ }
+
+ ConnectContext ctx = createJobContext(request);
+ try (AutoCloseConnectContext ignored = new
AutoCloseConnectContext(ctx)) {
+ StmtExecutor executor = new StmtExecutor(ctx, stmt);
+ executor.execute();
+ if (ctx.getState().getStateType() ==
QueryState.MysqlStateType.ERR) {
+ return
ResponseEntityBuilder.okWithCommonError(ctx.getState().getErrorMessage());
+ }
+ return ResponseEntityBuilder.ok();
+ } catch (Exception e) {
+ LOG.warn("Failed to execute schema change", e);
+ return ResponseEntityBuilder.okWithCommonError(e.getMessage());
+ }
+ }
+
private void checkAuth(HttpServletRequest request) {
String authToken = request.getHeader("token");
if (Strings.isNullOrEmpty(authToken)) {
throw new UnauthorizedException("Miss token");
}
if (!checkClusterToken(authToken)) {
- throw new UnauthorizedException("Invalid token: " + authToken);
+ throw new UnauthorizedException("Invalid token");
+ }
+ }
+
+ // Call only after validating the internal token. The caller owns the
context's scope.
+ private static ConnectContext createJobContext(HttpServletRequest request)
{
+ String jobIdHeader = request.getHeader("jobId");
+ if (Strings.isNullOrEmpty(jobIdHeader)) {
+ throw new BadRequestException("Missing jobId header; CDC client
must send the streaming job ID");
+ }
+ long jobId;
+ try {
+ jobId = Long.parseLong(jobIdHeader);
+ } catch (NumberFormatException e) {
+ throw new BadRequestException("Invalid jobId header: " +
jobIdHeader);
+ }
+ AbstractJob job = Env.getCurrentEnv().getJobManager().getJob(jobId);
+ if (!(job instanceof StreamingInsertJob)) {
+ throw new BadRequestException("Job " + jobId + " is not a
streaming job or does not exist");
+ }
+ if (job.getCreateUser() == null) {
+ throw new BadRequestException("Streaming job " + jobId + " has no
creator identity");
+ }
+ ConnectContext ctx = new ConnectContext();
+ ctx.setEnv(Env.getCurrentEnv());
+ ctx.setRemoteIP(request.getRemoteAddr());
+ ctx.setCurrentUserIdentity(job.getCreateUser());
Review Comment:
[P1] Preserve authentication-integration authorization
Job creation checks privileges against the live login context, where
`AuthenticatorManager` installs authentication-integration roles. Those roles
are consulted by `Auth.getRolesByUserWithLdap`, but the job persists only
`UserIdentity` and this reconstructed context starts with no
`authenticatedRoles`. An integration/JIT user whose LOAD/SHOW/ALTER rights come
from mapped session roles can therefore create and run the job, then have the
first schema event fail even though no grant changed. Define a reconstructable
owner-authorization contract (or reject non-reconstructable job owners) and
cover this case; persisting an unchecked role snapshot would also defeat later
revocation.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/StreamingJobAction.java:
##########
@@ -50,14 +80,69 @@ public Object reportTaskFailure(@RequestBody
TaskFailureRequest failureRequest,
return failTask(failureRequest);
}
+ @RequestMapping(path = "/api/streaming/schema_change", method =
RequestMethod.POST)
+ public Object executeSchemaChange(@RequestBody Map<String, String> body,
HttpServletRequest request) {
+ checkAuth(request);
+ if (!Env.getCurrentEnv().isMaster()) {
+ return ResponseEntityBuilder.okWithCommonError("Schema change must
be executed on the master FE");
+ }
+ String stmt = body.get("stmt");
+ if (Strings.isNullOrEmpty(stmt)) {
+ return ResponseEntityBuilder.badRequest("Missing statement request
body");
+ }
+
+ ConnectContext ctx = createJobContext(request);
+ try (AutoCloseConnectContext ignored = new
AutoCloseConnectContext(ctx)) {
+ StmtExecutor executor = new StmtExecutor(ctx, stmt);
+ executor.execute();
+ if (ctx.getState().getStateType() ==
QueryState.MysqlStateType.ERR) {
+ return
ResponseEntityBuilder.okWithCommonError(ctx.getState().getErrorMessage());
+ }
+ return ResponseEntityBuilder.ok();
+ } catch (Exception e) {
+ LOG.warn("Failed to execute schema change", e);
+ return ResponseEntityBuilder.okWithCommonError(e.getMessage());
+ }
+ }
+
private void checkAuth(HttpServletRequest request) {
String authToken = request.getHeader("token");
if (Strings.isNullOrEmpty(authToken)) {
throw new UnauthorizedException("Miss token");
}
if (!checkClusterToken(authToken)) {
- throw new UnauthorizedException("Invalid token: " + authToken);
+ throw new UnauthorizedException("Invalid token");
+ }
+ }
+
+ // Call only after validating the internal token. The caller owns the
context's scope.
+ private static ConnectContext createJobContext(HttpServletRequest request)
{
+ String jobIdHeader = request.getHeader("jobId");
+ if (Strings.isNullOrEmpty(jobIdHeader)) {
+ throw new BadRequestException("Missing jobId header; CDC client
must send the streaming job ID");
+ }
+ long jobId;
+ try {
+ jobId = Long.parseLong(jobIdHeader);
+ } catch (NumberFormatException e) {
+ throw new BadRequestException("Invalid jobId header: " +
jobIdHeader);
+ }
+ AbstractJob job = Env.getCurrentEnv().getJobManager().getJob(jobId);
+ if (!(job instanceof StreamingInsertJob)) {
+ throw new BadRequestException("Job " + jobId + " is not a
streaming job or does not exist");
+ }
+ if (job.getCreateUser() == null) {
+ throw new BadRequestException("Streaming job " + jobId + " has no
creator identity");
+ }
+ ConnectContext ctx = new ConnectContext();
+ ctx.setEnv(Env.getCurrentEnv());
+ ctx.setRemoteIP(request.getRemoteAddr());
+ ctx.setCurrentUserIdentity(job.getCreateUser());
+ if (!Strings.isNullOrEmpty(job.getCurrentDbName())) {
+ ctx.setDatabase(job.getCurrentDbName());
}
+ ctx.getState().setInternal(true);
Review Comment:
[P1] Restore the job's compute group in this context
`StreamingInsertJob` persists the validated `cloudCluster`, and
`StreamingInsertTask.before` explicitly restores it after merging session
state. This context never does. For a table that cannot use light schema
change, `CloudSchemaChangeJobV2` captures
`ConnectContext.get().getCloudCluster()` when the ALTER is created, so a job
explicitly bound to `cg_a` can instead capture the creator's current
default/policy-selected `cg_b` and fail if that unrelated group is unavailable.
The old JDBC route selected the admin account's group, so this patch newly
makes the result depend on the creator's default; neither route honors the job
binding. Set the persisted job group on this context and cover an explicit job
group that differs from the creator's default.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/StreamingJobAction.java:
##########
@@ -50,14 +80,69 @@ public Object reportTaskFailure(@RequestBody
TaskFailureRequest failureRequest,
return failTask(failureRequest);
}
+ @RequestMapping(path = "/api/streaming/schema_change", method =
RequestMethod.POST)
+ public Object executeSchemaChange(@RequestBody Map<String, String> body,
HttpServletRequest request) {
+ checkAuth(request);
+ if (!Env.getCurrentEnv().isMaster()) {
+ return ResponseEntityBuilder.okWithCommonError("Schema change must
be executed on the master FE");
+ }
+ String stmt = body.get("stmt");
+ if (Strings.isNullOrEmpty(stmt)) {
+ return ResponseEntityBuilder.badRequest("Missing statement request
body");
+ }
+
+ ConnectContext ctx = createJobContext(request);
+ try (AutoCloseConnectContext ignored = new
AutoCloseConnectContext(ctx)) {
+ StmtExecutor executor = new StmtExecutor(ctx, stmt);
Review Comment:
[P2] Keep CDC ALTERs in the SQL audit trail
The `/api/query` path this replaces opened a MySQL session, so
`ConnectProcessor.auditAfterExec` recorded both successful and failed ALTERs. A
direct `StmtExecutor.execute()` does not perform that audit (the call near
`StmtExecutor:2370` is only in the separate internal-result collection path),
and this handler has no audit `finally`; these CDC schema mutations therefore
disappear from the audit trail just as the PR starts attributing them to the
creator. Initialize the audit state and emit `AuditLogHelper.logAuditLog` on
both success and failure, with a regression for each outcome.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/StreamingJobAction.java:
##########
@@ -50,14 +80,69 @@ public Object reportTaskFailure(@RequestBody
TaskFailureRequest failureRequest,
return failTask(failureRequest);
}
+ @RequestMapping(path = "/api/streaming/schema_change", method =
RequestMethod.POST)
+ public Object executeSchemaChange(@RequestBody Map<String, String> body,
HttpServletRequest request) {
+ checkAuth(request);
+ if (!Env.getCurrentEnv().isMaster()) {
+ return ResponseEntityBuilder.okWithCommonError("Schema change must
be executed on the master FE");
+ }
+ String stmt = body.get("stmt");
+ if (Strings.isNullOrEmpty(stmt)) {
+ return ResponseEntityBuilder.badRequest("Missing statement request
body");
+ }
+
+ ConnectContext ctx = createJobContext(request);
Review Comment:
[P1] Restore the overdue-instance fence before DDL
The old `/api/query` route opened a MySQL session and reached
`ConnectProcessor.handleQuery`, which rejects non-root SQL while the cloud
instance is `OVERDUE`. `/api/**` bypasses `AuthInterceptor`, and this
replacement validates only the cluster token before calling `StmtExecutor`
directly, so automatic ALTER mutations continue after the warehouse becomes
overdue. Apply `checkInstanceOverdueIfCloud` (or the shared SQL-dispatch
equivalent) to the resolved job creator before executing DDL, and add a
non-root cloud overdue test.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/StreamingJobAction.java:
##########
@@ -18,26 +18,56 @@
package org.apache.doris.httpv2.rest;
import org.apache.doris.catalog.Env;
+import org.apache.doris.datasource.InternalCatalog;
import org.apache.doris.httpv2.entity.ResponseEntityBuilder;
+import org.apache.doris.httpv2.exception.BadRequestException;
import org.apache.doris.httpv2.exception.UnauthorizedException;
import org.apache.doris.job.base.AbstractJob;
import org.apache.doris.job.cdc.request.CommitOffsetRequest;
import org.apache.doris.job.cdc.request.TaskFailureRequest;
import org.apache.doris.job.extensions.insert.streaming.StreamingInsertJob;
+import org.apache.doris.mysql.privilege.PrivPredicate;
+import org.apache.doris.qe.AutoCloseConnectContext;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.QueryState;
+import org.apache.doris.qe.StmtExecutor;
import com.google.common.base.Strings;
import jakarta.servlet.http.HttpServletRequest;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RestController;
+import java.util.Map;
+
@RestController
public class StreamingJobAction extends RestBaseController {
private static final Logger LOG =
LogManager.getLogger(StreamingJobAction.class);
+ private final TableSchemaAction tableSchemaAction;
+
+ public StreamingJobAction(TableSchemaAction tableSchemaAction) {
+ this.tableSchemaAction = tableSchemaAction;
+ }
+
+ @RequestMapping(path = "/api/streaming/{" + DB_KEY + "}/{" + TABLE_KEY +
"}/_schema",
Review Comment:
[P1] Avoid shadowing the schema API for catalog streaming
`TableSchemaAction` already maps `/api/{catalog}/{db}/{table}/_schema`, and
`streaming` is a valid catalog name. With the configured `AntPathMatcher`, this
literal mapping wins for `/api/streaming/db/tbl/_schema`, so an existing
Basic-auth request is routed here and rejected for missing token/jobId; even an
internal request is forced to the internal catalog. That breaks the existing
API the PR says remains unchanged. Use a non-overlapping route shape (or
another dispatch constraint) and add an MVC routing case for a catalog named
`streaming`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
