You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "soumitra-st (via GitHub)" <gi...@apache.org> on 2023/07/19 16:37:03 UTC
[GitHub] [pinot] soumitra-st opened a new pull request, #11136: WIP: Returning 403 status code in case of authorization failures
soumitra-st opened a new pull request, #11136:
URL: https://github.com/apache/pinot/pull/11136
Currently, if SQL query execution has authorization issues, the return code is 200 and the errorCode 180 is set in the response JSON. This happens for both controller and broker endpoints.
SQL execution using Controller:
% curl -v -X POST -H "accept: application/json" -H "Content-Type: application/json" -d '{"sql":"select * from transcript limit 10","trace":false,"queryOptions":""}' http://localhost:9000/sql ; echo
Note: Unnecessary use of -X or --request, POST is already inferred.
* Trying 127.0.0.1:9000...
* Connected to localhost (127.0.0.1) port 9000 (#0)
> POST /sql HTTP/1.1
> Host: localhost:9000
> User-Agent: curl/7.88.1
> accept: application/json
> Content-Type: application/json
> Content-Length: 75
>
< HTTP/1.1 200 OK
< Pinot-Controller-Host: soumitras-mbp.attlocal.net
< Pinot-Controller-Version: 1.0.0-sk-bf66c10b03399e8682ed69cf7c087127ca986c36
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Methods: GET, POST, PUT, OPTIONS, DELETE
< Access-Control-Allow-Headers: *
< Content-Type: application/json
< Content-Length: 1023
<
* Connection #0 to host localhost left intact
{"requestId":"1262666501000000001","exceptions":[{"message":null,"errorCode":180}],"numServersQueried":0,"numServersResponded":0,"numSegmentsQueried":0,"numSegmentsProcessed":0,"numSegmentsMatched":0,"numConsumingSegmentsQueried":0,"numConsumingSegmentsProcessed":0,"numConsumingSegmentsMatched":0,"numDocsScanned":0,"numEntriesScannedInFilter":0,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":0,"timeUsedMs":0,"offlineThreadCpuTimeNs":0,"realtimeThreadCpuTimeNs":0,"offlineSystemActivitiesCpuTimeNs":0,"realtimeSystemActivitiesCpuTimeNs":0,"offlineResponseSerializationCpuTimeNs":0,"realtimeResponseSerializationCpuTimeNs":0,"offlineTotalCpuTimeNs":0,"realtimeTotalCpuTimeNs":0,"segmentStatistics":[],"traceInfo":{},"numRowsResultSet":0,"minConsumingFreshnessTimeMs":0,"numSegmentsPrunedByBroker":0,"numSegmentsPrunedByServer":0,"numSegmentsPrunedInvalid":0,"numSegmentsPrunedByLimit":0,"numSegmentsPrunedByValue":0,"explainPlanNumEmptyFilterSegments":0,"explainPlanN
umMatchAllFilterSegments":0}
SQL execution using Broker:
% curl -v -X POST -H "accept: application/json" -H "Content-Type: application/json" -d '{"sql":"select * from transcript limit 10","trace":false,"queryOptions":""}' http://localhost:8000/query/sql ; echo
Note: Unnecessary use of -X or --request, POST is already inferred.
* Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> POST /query/sql HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.88.1
> accept: application/json
> Content-Type: application/json
> Content-Length: 75
>
< HTTP/1.1 200 OK
< Content-Type: application/json
< Content-Length: 1023
<
* Connection #0 to host localhost left intact
{"requestId":"1262666501000000000","exceptions":[{"message":null,"errorCode":180}],"numServersQueried":0,"numServersResponded":0,"numSegmentsQueried":0,"numSegmentsProcessed":0,"numSegmentsMatched":0,"numConsumingSegmentsQueried":0,"numConsumingSegmentsProcessed":0,"numConsumingSegmentsMatched":0,"numDocsScanned":0,"numEntriesScannedInFilter":0,"numEntriesScannedPostFilter":0,"numGroupsLimitReached":false,"totalDocs":0,"timeUsedMs":0,"offlineThreadCpuTimeNs":0,"realtimeThreadCpuTimeNs":0,"offlineSystemActivitiesCpuTimeNs":0,"realtimeSystemActivitiesCpuTimeNs":0,"offlineResponseSerializationCpuTimeNs":0,"realtimeResponseSerializationCpuTimeNs":0,"offlineTotalCpuTimeNs":0,"realtimeTotalCpuTimeNs":0,"segmentStatistics":[],"traceInfo":{},"numRowsResultSet":0,"minConsumingFreshnessTimeMs":0,"numSegmentsPrunedByBroker":0,"numSegmentsPrunedByServer":0,"numSegmentsPrunedInvalid":0,"numSegmentsPrunedByLimit":0,"numSegmentsPrunedByValue":0,"explainPlanNumEmptyFilterSegments":0,"explainPlanN
umMatchAllFilterSegments":0}
This PR changes the return code to 403, sample runs below:
% curl -v -X POST -H "accept: application/json" -H "Content-Type: application/json" -d '{"sql":"select * from transcript limit 10","trace":false,"queryOptions":""}' http://localhost:9000/sql ; echo
Note: Unnecessary use of -X or --request, POST is already inferred.
* Trying 127.0.0.1:9000...
* Connected to localhost (127.0.0.1) port 9000 (#0)
> POST /sql HTTP/1.1
> Host: localhost:9000
> User-Agent: curl/7.88.1
> accept: application/json
> Content-Type: application/json
> Content-Length: 75
>
< HTTP/1.1 403 Forbidden
% curl -v -X POST -H "accept: application/json" -H "Content-Type: application/json" -d '{"sql":"select * from transcript limit 10","trace":false,"queryOptions":""}' http://localhost:8000/query/sql ; echo
Note: Unnecessary use of -X or --request, POST is already inferred.
* Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> POST /query/sql HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.88.1
> accept: application/json
> Content-Type: application/json
> Content-Length: 75
>
< HTTP/1.1 403 Forbidden
label is 'bugfix'.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11136: WIP: Returning 403 status code in case of authorization failures
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11136:
URL: https://github.com/apache/pinot/pull/11136#discussion_r1270378174
##########
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:
Ah, sorry I misread the code, nvm
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] soumitra-st commented on a diff in pull request #11136: WIP: Returning 403 status code in case of authorization failures
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #11136:
URL: https://github.com/apache/pinot/pull/11136#discussion_r1268606733
##########
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:
Checked and handled other calls of hasAccess.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #11136: Returning 403 status code in case of authorization failures
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11136:
URL: https://github.com/apache/pinot/pull/11136
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] soumitra-st commented on a diff in pull request #11136: WIP: Returning 403 status code in case of authorization failures
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #11136:
URL: https://github.com/apache/pinot/pull/11136#discussion_r1268613693
##########
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:
Added log. There is a Meter metric REQUEST_DROPPED_DUE_TO_ACCESS_ERROR("requestsDropped") which is tracking at several places, but let me file a task to review and add at the missing places, such as request filter.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11136: WIP: Returning 403 status code in case of authorization failures
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #11136: WIP: Returning 403 status code in case of authorization failures
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11136:
URL: https://github.com/apache/pinot/pull/11136#issuecomment-1642461675
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11136?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11136](https://app.codecov.io/gh/apache/pinot/pull/11136?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (eef256d) into [master](https://app.codecov.io/gh/apache/pinot/commit/abc749774485405a75c1ec06dc6b75a1b89e68ce?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (abc7497) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11136 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2203 2149 -54
Lines 118196 115809 -2387
Branches 17887 17606 -281
=========================================
Hits 137 137
+ Misses 118039 115652 -2387
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...pinot/broker/api/resources/PinotClientRequest.java](https://app.codecov.io/gh/apache/pinot/pull/11136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...t/controller/api/resources/PinotQueryResource.java](https://app.codecov.io/gh/apache/pinot/pull/11136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90UXVlcnlSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (ø)` | |
... and [70 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11136/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] soumitra-st commented on a diff in pull request #11136: WIP: Returning 403 status code in case of authorization failures
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #11136:
URL: https://github.com/apache/pinot/pull/11136#discussion_r1268608762
##########
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:
output stream is used to send the payload, input and error streams are read after checking the status code. The logic looks correct.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org