Jackie-Jiang commented on code in PR #11136:
URL: https://github.com/apache/pinot/pull/11136#discussion_r1268436162
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -390,7 +396,9 @@ public String sendPostRaw(String urlStr, String requestStr,
Map<String, String>
/*if (LOG.isInfoEnabled()){
LOGGER.info("The http response code is " + responseCode);
}*/
- if (responseCode != HttpURLConnection.HTTP_OK) {
+ if (responseCode == HttpURLConnection.HTTP_FORBIDDEN) {
Review Comment:
Should we first check response code before getting the output stream?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -385,8 +387,7 @@ private BrokerResponseNative handleRequest(long requestId,
String query,
if (!hasTableAccess) {
_brokerMetrics.addMeteredTableValue(tableName,
BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
LOGGER.info("Access denied for request {}: {}, table: {}", requestId,
query, tableName);
- requestContext.setErrorCode(QueryException.ACCESS_DENIED_ERROR_CODE);
Review Comment:
Please check all usage of `hasAccess()`, seems there are other places also
need to be handled
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -121,6 +121,8 @@ public void processSqlQueryGet(@ApiParam(value = "Query",
required = true) @Quer
}
BrokerResponse brokerResponse = executeSqlQuery(requestJson,
makeHttpIdentity(requestContext), true);
asyncResponse.resume(brokerResponse.toJsonString());
+ } catch (WebApplicationException wae) {
Review Comment:
Put some error log? Also do we have a metric tracking the auth failure?
Same for other places
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -385,8 +387,7 @@ private BrokerResponseNative handleRequest(long requestId,
String query,
if (!hasTableAccess) {
_brokerMetrics.addMeteredTableValue(tableName,
BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1);
LOGGER.info("Access denied for request {}: {}, table: {}", requestId,
query, tableName);
- requestContext.setErrorCode(QueryException.ACCESS_DENIED_ERROR_CODE);
Review Comment:
We should still set the request context
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
