Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attach user identity to router request logs #15126

Merged
merged 4 commits into from
Oct 19, 2023
Merged

Conversation

a2l007
Copy link
Contributor

@a2l007 a2l007 commented Oct 10, 2023

Description

This PR attaches user identity to the query request logs on the router. Having the user identity present in the router request logs is useful when druid security is enabled only on the routers.
Sample query request logs on the router would now look like this:
{"query/time":94,"success":false,"identity":"a2l007"} {"query":"select * from \"testdata\"","context":{"sqlQueryId":"sqlQuerytest1","sqlOuterLimit":1001,"queryId":"sqlQuerytest1"}}


Key changed/added classes in this PR
  • AsyncQueryForwardingServlet

This PR has:

  • been self-reviewed.
  • been tested in a test Druid cluster.

@@ -744,6 +749,8 @@ public void onComplete(Result result)
}
emitQueryTime(requestTimeNs, success, sqlQueryId, queryId);

AuthenticationResult authenticationResult = AuthorizationUtils.authenticationResultFromRequest(req);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would throw an exception if DRUID_AUTHENTICATION_RESULT attribute is not set. can we not get the request directly and avoid possibility of that exception being thrown?

Copy link
Contributor Author

@a2l007 a2l007 Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are attaching an authenticator even if the authenticators are null, I'd expect the request would have an auth result every time. The QueryLifecycle also uses the same method and so if there were a failure, it would fail before the request comes back to the router. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I did check that, too. Just being more risk averse, I guess. I am going to approve. Feel free to merge once CI is green. Those failures are likely code coverage issues.

@a2l007
Copy link
Contributor Author

a2l007 commented Oct 19, 2023

Thanks for the review @abhishekagarwal87

@a2l007 a2l007 merged commit 7802078 into apache:master Oct 19, 2023
81 checks passed
CaseyPan pushed a commit to CaseyPan/druid that referenced this pull request Nov 17, 2023
* Attach user identity to router request logs

* Add test

* More tests
ratishr pushed a commit to confluentinc/druid that referenced this pull request Jan 4, 2024
* Attach user identity to router request logs

* Add test

* More tests

(cherry picked from commit 7802078)
ratishr added a commit to confluentinc/druid that referenced this pull request Jan 4, 2024
* Attach user identity to router request logs

* Add test

* More tests

(cherry picked from commit 7802078)

Co-authored-by: Atul Mohan <[email protected]>
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants