xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4252244287
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -703,7 +726,13 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
Map<String, String> headers = new HashMap<>();
httpRequesterIdentity.getHttpHeaders().entries()
.forEach(entry -> headers.put(entry.getKey(), entry.getValue()));
+ if (sqlNodeAndOptions.getSqlNode() instanceof SqlDelete) {
+ return executeDelete(sqlNodeAndOptions, headers,
httpRequesterIdentity, httpHeaders);
Review Comment:
Fixed in de5d618d11: with `getCursor=true` a non-DQL statement is now
rejected with SQL_PARSING before the cursor options are set or the cursor meter
is bumped, mirroring the `onlyDql` guard, with a test.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -133,6 +144,28 @@ public TableAuthorizationResult
authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public AuthorizationResult authorizeDeleteRows(RequesterIdentity
requesterIdentity,
+ @Nullable HttpHeaders httpHeaders, String tableName) {
+ Optional<BasicAuthPrincipal> principalOpt =
getPrincipalOpt(requesterIdentity);
+ if (principalOpt.isEmpty()) {
+ return new BasicAuthorizationResultImpl(false, "Missing or invalid
credentials");
+ }
+ BasicAuthPrincipal principal = principalOpt.get();
+ // `tables` matches the name as given, as for queries, while
`excludeTables` lists raw names: also check the
+ // exclusion list on the raw name, so that a name with a type suffix
cannot bypass an excluded table
+ if (!principal.hasTable(tableName)
+ ||
principal.isTableExcluded(TableNameBuilder.extractRawTableName(tableName))) {
Review Comment:
Done in de5d618d11, at the root as you suggest:
`BasicAuthPrincipal.isTableNotExcluded` now also checks the raw table name, so
the query path denies `t_OFFLINE` for a principal with `excludeTables=t` too,
and the DELETE-specific raw check here went away. `BasicAuthAccessControlTest`
has a case for the query path.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -713,6 +742,231 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table.
+ ///
+ /// The first-step access control runs first, as for queries, followed by
the table check without a table, so that
+ /// an access control that only authenticates the caller in that check
rejects an unauthenticated caller before the
+ /// table is looked up. The table is then resolved with the database of the
request and in the case it is defined
+ /// with, the caller is authorized to delete rows from it (see
[#authorizeDelete]), and the executor deletes rows
+ /// from that exact table. A logical table fails with
[QueryErrorCode#QUERY_VALIDATION], and a table the table cache
+ /// does not know with [QueryErrorCode#TABLE_DOES_NOT_EXIST], as a query
does, but only once the caller is
+ /// authorized for it, and every message names the table as the caller wrote
it (see [#deleteAccessDenied] for the
+ /// reason of a denial), so that neither the existence of a table nor its
name as defined is leaked to a caller who
+ /// is not authorized for it.
+ private BrokerResponse executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
Map<String, String> headers,
+ HttpRequesterIdentity requesterIdentity, @Nullable HttpHeaders
httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ // The first-step access control runs before the table is looked up, as
for queries
+ AuthorizationResult authorizationResult =
accessControl.authorize(requesterIdentity);
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(null, null, authorizationResult);
+ }
+ // Access controls that authenticate the caller in their table check
rather than in the first step (e.g. the
+ // ZooKeeper basic auth) reject an unauthenticated caller here, before the
table is looked up
+ authorizationResult = authorizeTables(accessControl, requesterIdentity,
Set.of());
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(null, null, authorizationResult);
+ }
+ if (_tableCache == null) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE is not supported by this broker: no table cache was
configured");
+ }
+ DeleteStatement parsed;
+ DeleteStatement statement;
+ try {
+ String databaseHeader = httpHeaders != null ?
httpHeaders.getHeaderString(CommonConstants.DATABASE) : null;
+ parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ statement = parsed.resolveTableName(databaseHeader, _tableCache);
+ } catch (QueryException e) {
+ // e.g. an invalid statement, or a database header that does not match
the statement
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ // The caller is authorized for the table as resolved, but the messages
name it as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
requesterIdentity, httpHeaders);
+ if (statement.isLogicalTable()) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE does not support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ return new BrokerResponseNative(QueryErrorCode.TABLE_DOES_NOT_EXIST,
+ "Table does not exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, sqlNodeAndOptions,
requesterIdentity));
+ return _sqlQueryExecutor.executeStatement(statement, headers);
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run. The
predicate is redacted with the SQL
+ /// redaction mode of the query log (`pinot.broker.query.log.sqlRedaction`),
as the queries are, since its literals
+ /// are the very values a privacy-driven delete removes: the
`literal_values` mode logs the fingerprint of the
+ /// statement (`DELETE FROM t WHERE c = ?`), which keeps its shape as the
fingerprint of a query does, in place of
+ /// the predicate. The client is only logged when the query log logs client
IPs
+ /// (`pinot.broker.request.client.ip.logging`), since it comes from request
headers the caller controls. The option
+ /// keys and the client are kept on a single line, as the query log keeps
the predicate, so that a line ending in
+ /// the statement or in a header cannot forge a log record.
+ ///
+ /// @param sqlNodeAndOptions parsed statement, which the `literal_values`
mode fingerprints
+ @VisibleForTesting
+ String deleteLogMessage(DeleteStatement statement, SqlNodeAndOptions
sqlNodeAndOptions,
+ HttpRequesterIdentity requesterIdentity) {
+ SqlRedactionMode sqlRedactionMode = SqlRedactionMode.fromString(
+
_brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_QUERY_LOG_SQL_REDACTION,
+ CommonConstants.Broker.DEFAULT_BROKER_QUERY_LOG_SQL_REDACTION));
+ QueryFingerprint fingerprint = null;
+ if (sqlRedactionMode == SqlRedactionMode.LITERAL_VALUES) {
+ try {
+ fingerprint =
QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ } catch (Exception e) {
+ // Logged as the query log logs a query it could not fingerprint
+ LOGGER.warn("Failed to generate the fingerprint of a DELETE on table:
{}", statement.getTableName(), e);
+ }
+ }
+ String predicate = QueryLogger.redactQuery(statement.getPredicate(),
sqlRedactionMode, fingerprint);
+ String options = toSingleLine(statement.getOptions().keySet().toString());
+ String client =
_brokerConf.getProperty(CommonConstants.Broker.CONFIG_OF_BROKER_REQUEST_CLIENT_IP_LOGGING,
+ CommonConstants.Broker.DEFAULT_BROKER_REQUEST_CLIENT_IP_LOGGING)
+ ? toSingleLine(requesterIdentity.getClientIp()) :
CommonConstants.UNKNOWN;
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: " + predicate + ", options: "
+ + options + ", client: " + client;
+ }
+
+ /// Escapes backslashes, CR and LF as `\\`, `\r` and `\n`, as `QueryLogger`
does to log a query on a single line.
+ private static String toSingleLine(String text) {
+ return text.replace("\\", "\\\\").replace("\r", "\\r").replace("\n",
"\\n");
+ }
+
+ /// Authorizes the caller, who passed the first-step access control, to
delete rows from the table.
+ ///
+ /// The checks run in this order, and the first one that denies answers HTTP
403:
+ /// 1. [AccessControl#authorize(RequesterIdentity, Set)] with the table as
its only element, since the WHERE clause
+ /// reads it. [AccessControl#authorize(RequesterIdentity, BrokerRequest)]
is not called, since a `DELETE` is not a
+ /// broker request: table restrictions that an access control only
applies in that overload are not consulted,
+ /// and must be applied again in [AccessControl#authorizeDeleteRows].
+ /// 2. The fine-grained [Actions.Table#QUERY] action on the table name, as
for a query.
+ /// 3. The fine-grained [Actions.Table#DELETE_ROWS] action on the table
name, then on its raw name when the name has
+ /// a type suffix, which the controller checks, so that a type suffix
cannot bypass a rule on the raw name.
+ /// 4. [AccessControl#authorizeDeleteRows], which denies by default: the
fine-grained checks allow every action by
+ /// default, so they cannot tell an access control written before
`DELETE` existed from one that allows it.
+ /// 5. No row-level security filter applies to the table (see
[AccessControl#getRowColFilters], when the broker
+ /// enables row-level security), since it would not restrict the rows the
statement deletes.
+ ///
+ /// @param tableName table as resolved, which the checks authorize
+ /// @param writtenTableName table as the caller wrote it, which a denial
names
+ /// @throws WebApplicationException with status 403 if the caller is not
authorized
+ private void authorizeDelete(AccessControl accessControl, String tableName,
String writtenTableName,
+ HttpRequesterIdentity requesterIdentity, @Nullable HttpHeaders
httpHeaders) {
+ checkDeleteAccess(authorizeTables(accessControl, requesterIdentity,
Set.of(tableName)), tableName,
+ writtenTableName);
+ checkDeleteAccess(accessControl.authorize(httpHeaders, TargetType.TABLE,
tableName, Actions.Table.QUERY),
+ tableName, writtenTableName);
+ checkDeleteAccess(accessControl.authorize(httpHeaders, TargetType.TABLE,
tableName, Actions.Table.DELETE_ROWS),
+ tableName, writtenTableName);
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ if (!rawTableName.equals(tableName)) {
+ checkDeleteAccess(
+ accessControl.authorize(httpHeaders, TargetType.TABLE, rawTableName,
Actions.Table.DELETE_ROWS), tableName,
+ writtenTableName);
+ }
+ checkDeleteAccess(accessControl.authorizeDeleteRows(requesterIdentity,
httpHeaders, tableName), tableName,
+ writtenTableName);
+ if (hasRowFilters(accessControl, requesterIdentity, tableName)) {
+ throw deleteAccessDenied(tableName, writtenTableName, new
BasicAuthorizationResultImpl(false,
+ "Row-level security applies to the table, and would not restrict the
rows the statement deletes"));
+ }
+ }
+
+ /// Throws the HTTP 403 error of a `DELETE` if the check denied it.
+ private void checkDeleteAccess(AuthorizationResult authorizationResult,
String tableName, String writtenTableName) {
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(tableName, writtenTableName,
authorizationResult);
+ }
+ }
+
+ /// Runs the table check of the access control, denying the `DELETE` when
the access control cannot run it: an
+ /// access control written for single-stage queries only, which overrides
+ /// [AccessControl#hasAccess(RequesterIdentity, BrokerRequest)] but not the
table check, throws
+ /// [UnsupportedOperationException] from the default table check (see
+ /// [AccessControl#hasAccess(RequesterIdentity, Set)]). A `DELETE` is not a
broker request, so it cannot be
+ /// authorized by such an access control, and is denied rather than failed
as an uncaught error.
+ private static AuthorizationResult authorizeTables(AccessControl
accessControl, RequesterIdentity requesterIdentity,
+ Set<String> tables) {
+ try {
+ return accessControl.authorize(requesterIdentity, tables);
+ } catch (UnsupportedOperationException e) {
+ LOGGER.warn("The access control {} cannot authorize a DELETE: it
implements the broker-request check only",
+ accessControl.getClass().getName(), e);
+ return new BasicAuthorizationResultImpl(false,
+ "The access control of the broker cannot authorize a DELETE: it
implements the broker-request check only");
+ }
+ }
+
+ /// Records a `DELETE` denied by the access control (metric, and a log line
once the table is known) and returns its
+ /// HTTP 403 error to throw. The error only names the table once the caller
passed the first-step access control,
+ /// and names it as the caller wrote it rather than as resolved, so that a
caller who is not authorized for the
+ /// table does not learn its name as defined (e.g. its case, which
`enable.case.insensitive` resolves).
+ ///
+ /// The reason of the access control is only sent to the caller when it does
not mention the table, since the
+ /// access control echoes the name it authorized, which is the resolved one
(e.g. the basic auth: "Authorization
+ /// Failed for tables: [MyTable]"). Rewriting the resolved name in the
reason would not do: it would only apply
+ /// when the table cache resolved the name, i.e. when the table exists, so
the reason would read differently for a
+ /// table that does not exist. The rule applies whether or not the resolved
name differs from the written one, so
+ /// that the message is the same for a table that exists and for one that
does not. The log line keeps the full
+ /// reason and the resolved name for the operator.
+ private WebApplicationException deleteAccessDenied(@Nullable String
tableName, @Nullable String writtenTableName,
+ AuthorizationResult authorizationResult) {
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR,
1);
+ String reason = authorizationResult.getFailureMessage();
+ String message;
+ if (tableName != null) {
+ // The name, which is the one written when the table cache does not know
it, and the reason come from the
+ // request and from the access control: escaped as the predicate of the
log line of a DELETE is, so that they
+ // cannot forge a log record
+ LOGGER.info("Access denied to delete rows from table: {}, reason: {}",
toSingleLine(tableName),
+ reason != null ? toSingleLine(reason) : null);
+ if (reason != null && (mentionsTable(reason, tableName) ||
mentionsTable(reason, writtenTableName))) {
+ reason = null;
+ }
+ message = "Permission denied to delete rows from table: " +
writtenTableName;
+ } else {
+ message = "Permission denied to delete rows";
+ }
+ if (reason != null && !reason.isBlank()) {
+ message += ". Reason: " + reason;
+ }
+ return new WebApplicationException(message, Response.Status.FORBIDDEN);
+ }
+
+ /// Whether the reason of a denial mentions the table name, or its raw name
(the fine-grained check of the right to
+ /// delete rows also runs on the raw name), ignoring case, since
`enable.case.insensitive` resolves it.
+ private static boolean mentionsTable(String reason, @Nullable String
tableName) {
Review Comment:
Changed in de5d618d11: `mentionsTable` matches the table name (and its raw
name) as a whole word, case-insensitively, so a reason mentioning "rows" is
kept for a table named `row` and only a reason that actually echoes the table
is dropped. A table literally named like a word of the reason still suppresses
it, which the Javadoc notes; I preferred that over never echoing reasons, since
"is not granted the DELETE permission" is the one callers most need.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java:
##########
@@ -715,6 +714,12 @@ private void setUpPinotController() {
// max.segment.completion.time.millis is seeded from cluster config before
serving requests.
_clusterConfigChangeHandler.registerClusterConfigChangeListener(_pinotLLCRealtimeSegmentManager);
LOGGER.info("Registered PinotLLCRealtimeSegmentManager as cluster config
change listener");
+ // Keeps the legacy OPTION(...) syntax mode cluster config in effect for
the statements the controller parses on
+ // /sql, including the DML it executes itself (a DELETE carrying legacy
options fails in IGNORE and REJECT mode).
+ // The query option validation mode is not applied here: the broker
validates every query the controller forwards,
+ // and the REJECT allowlist of registered SET keys is per JVM (broker
plugins register their keys in the broker
+ // only), so applying it here would reject options the broker accepts
+
_clusterConfigChangeHandler.registerClusterConfigChangeListener(QueryOptionConfigListener.legacySyntaxModeOnly());
Review Comment:
Noted in de5d618d11: the registration comment now says the JVM-wide legacy
syntax mode also governs the other statements the controller parses (the
recommender, `/query/tableNames`, `validateMultiStageQuery`), so a cluster that
rejects or ignores `OPTION(...)` does so on those endpoints as well, which is
what the broker would do at runtime; the description's compatibility section
carries the same note.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
+ + toSingleLine(statement.getPredicate()) + ", options: "
+ + toSingleLine(statement.getOptions().keySet().toString()) + ",
client: "
+ + toSingleLine(getClientIp(httpHeaders));
+ }
+
+ /// Client of the request for the log: the `X-Forwarded-For` header, else
the `X-Real-IP` header, else
+ /// [CommonConstants#UNKNOWN]. The addresses of a comma-separated
`X-Forwarded-For` value are joined with `;`, the
+ /// format the broker's `HttpRequesterIdentity#getClientIp` logs, so that
the field parses the same way in the logs
+ /// of both roles. Both headers are set by the caller unless a proxy
overwrites them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
Review Comment:
Done in de5d618d11: `handlePostSql` takes the Grizzly `Request`, and the
line logs `client:` (the proxy-reported address, as the broker logs it) plus a
new `remote:` field with the socket address from `getRemoteAddr()`, so the
field names mean the same on both roles and the trustworthy address is always
there for forensics. The Javadoc notes the controller has no equivalent of the
broker's client-IP logging switch, so the forwarded header is logged
unconditionally. Tests updated.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +445,116 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, and the caller
authorized to delete rows from the resolved
+ /// table. Only then is the table checked not to be a logical table and to
exist, and every error message names the
+ /// table as the caller wrote it, so that a caller who is not authorized for
a table learns neither whether it exists
+ /// nor the case it is defined with, nor whether the name is a logical
table. The executor deletes rows from the
+ /// exact resolved table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#QUERY_VALIDATION] if the table is
a logical table, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DeleteStatement statement =
parsed.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ // Authorized on the resolved table, reported with the name as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
httpHeaders);
+ if (statement.isLogicalTable()) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Formats the log line of a `DELETE` the executor is about to run: the
resolved table, the predicate, the option
+ /// keys and the client of the request, in the format of the broker's line
so that it parses the same way in the
+ /// logs of both roles. The predicate, the option keys and the client are
kept on a single line (see
+ /// [#toSingleLine]), so that a line ending in the statement, in a request
option or in a proxy header cannot forge
+ /// a log record.
+ @VisibleForTesting
+ static String deleteLogMessage(DeleteStatement statement, HttpHeaders
httpHeaders) {
+ return "Executing DELETE on table: " + statement.getTableName() + ",
predicate: "
+ + toSingleLine(statement.getPredicate()) + ", options: "
+ + toSingleLine(statement.getOptions().keySet().toString()) + ",
client: "
+ + toSingleLine(getClientIp(httpHeaders));
+ }
+
+ /// Client of the request for the log: the `X-Forwarded-For` header, else
the `X-Real-IP` header, else
+ /// [CommonConstants#UNKNOWN]. The addresses of a comma-separated
`X-Forwarded-For` value are joined with `;`, the
+ /// format the broker's `HttpRequesterIdentity#getClientIp` logs, so that
the field parses the same way in the logs
+ /// of both roles. Both headers are set by the caller unless a proxy
overwrites them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
+ if (forwardedFor != null) {
+ return forwardedFor.replace(',', ';');
+ }
+ String realIp = httpHeaders.getHeaderString("X-Real-IP");
+ return realIp != null ? realIp : CommonConstants.UNKNOWN;
+ }
+
+ /// Escapes backslashes, CR and LF as `\\`, `\r` and `\n`, as the broker's
query log does, so that a value the
+ /// caller controls (a string literal or a quoted identifier of the
predicate, an option key from `SET`, the legacy
+ /// `OPTION(...)` suffix or the request `queryOptions`, a proxy header)
cannot split a log record or append a line
+ /// that looks like a log entry.
+ private static String toSingleLine(String value) {
Review Comment:
Hoisted in de5d618d11: `LoggerUtils.toSingleLine` in pinot-common is now the
single implementation, used by `QueryLogger`, `PinotClientRequest` and
`PinotQueryResource`.
--
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]
