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.

Sign up for GitHub

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

Attach user identity to router request logs #15126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04b7b1a
Attach user identity to router request logs
a2l007 Oct 10, 2023
41c031b
Merge branch 'master' of github.com:apache/druid into routeridentity
a2l007 Oct 18, 2023
20f4661
Add test
a2l007 Oct 18, 2023
06151f7
More tests
a2l007 Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import org.apache.druid.server.security.AuthenticationResult;
import org.apache.druid.server.security.Authenticator;
import org.apache.druid.server.security.AuthenticatorMapper;
import org.apache.druid.server.security.AuthorizationUtils;
import org.apache.druid.sql.http.SqlQuery;
import org.apache.druid.sql.http.SqlResource;
import org.eclipse.jetty.client.HttpClient;
Expand Down Expand Up @@ -303,6 +304,7 @@ protected void service(HttpServletRequest request, HttpServletResponse response)

/**
* Rebuilds the {@link SqlQuery} object with sqlQueryId and queryId context parameters if not present
*
* @param sqlQuery the original SqlQuery
* @return an updated sqlQuery object with sqlQueryId and queryId context parameters
*/
Expand Down Expand Up @@ -367,13 +369,16 @@ void handleQueryParseException(
// Log the error message
final String errorMessage = exceptionToReport.getMessage() == null
? "no error message" : exceptionToReport.getMessage();

AuthenticationResult authenticationResult = AuthorizationUtils.authenticationResultFromRequest(request);

if (isNativeQuery) {
requestLogger.logNativeQuery(
RequestLogLine.forNative(
null,
DateTimes.nowUtc(),
request.getRemoteAddr(),
new QueryStats(ImmutableMap.of("success", false, "exception", errorMessage))
new QueryStats(ImmutableMap.of("success", false, "exception", errorMessage, "identity", authenticationResult.getIdentity()))
)
);
} else {
Expand All @@ -383,7 +388,7 @@ void handleQueryParseException(
null,
DateTimes.nowUtc(),
request.getRemoteAddr(),
new QueryStats(ImmutableMap.of("success", false, "exception", errorMessage))
new QueryStats(ImmutableMap.of("success", false, "exception", errorMessage, "identity", authenticationResult.getIdentity()))
)
);
}
Expand Down Expand Up @@ -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 •
edited
Loading

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.


//noinspection VariableNotUsedInsideIf
if (sqlQueryId != null) {
// SQL query doesn't have a native query translation in router. Hence, not logging the native query.
Expand All @@ -761,7 +768,9 @@ public void onComplete(Result result)
TimeUnit.NANOSECONDS.toMillis(requestTimeNs),
"success",
success
&& result.getResponse().getStatus() == Status.OK.getStatusCode()
&& result.getResponse().getStatus() == Status.OK.getStatusCode(),
"identity",
authenticationResult.getIdentity()
)
)
)
Expand All @@ -787,7 +796,9 @@ public void onComplete(Result result)
TimeUnit.NANOSECONDS.toMillis(requestTimeNs),
"success",
success
&& result.getResponse().getStatus() == Status.OK.getStatusCode()
&& result.getResponse().getStatus() == Status.OK.getStatusCode(),
"identity",
authenticationResult.getIdentity()
)
)
)
Expand Down Expand Up @@ -824,6 +835,7 @@ public void onFailure(Response response, Throwable failure)

failedQueryCount.incrementAndGet();
emitQueryTime(requestTimeNs, false, sqlQueryId, queryId);
AuthenticationResult authenticationResult = AuthorizationUtils.authenticationResultFromRequest(req);

//noinspection VariableNotUsedInsideIf
if (sqlQueryId != null) {
Expand All @@ -841,7 +853,9 @@ public void onFailure(Response response, Throwable failure)
"success",
false,
"exception",
errorMessage == null ? "no message" : errorMessage
errorMessage == null ? "no message" : errorMessage,
"identity",
authenticationResult.getIdentity()
)
)
)
Expand Down Expand Up @@ -871,7 +885,9 @@ public void onFailure(Response response, Throwable failure)
"success",
false,
"exception",
errorMessage == null ? "no message" : errorMessage
errorMessage == null ? "no message" : errorMessage,
"identity",
authenticationResult.getIdentity()
)
)
)
Expand All @@ -890,7 +906,12 @@ public void onFailure(Response response, Throwable failure)
super.onFailure(response, failure);
}

private void emitQueryTime(long requestTimeNs, boolean success, @Nullable String sqlQueryId, @Nullable String queryId)
private void emitQueryTime(
long requestTimeNs,
boolean success,
@Nullable String sqlQueryId,
@Nullable String queryId
)
{
QueryMetrics queryMetrics;
if (sqlQueryId != null) {
Expand Down
6 changes: 6 additions & 0 deletions services/src/test/java/org/apache/druid/server/AsyncQueryForwardingServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@
import org.apache.druid.server.router.QueryHostFinder;
import org.apache.druid.server.router.RendezvousHashAvaticaConnectionBalancer;
import org.apache.druid.server.security.AllowAllAuthorizer;
import org.apache.druid.server.security.AuthConfig;
import org.apache.druid.server.security.AuthenticationResult;
import org.apache.druid.server.security.AuthenticatorMapper;
import org.apache.druid.server.security.Authorizer;
import org.apache.druid.server.security.AuthorizerMapper;
Expand Down Expand Up @@ -408,6 +410,7 @@ public void testHandleQueryParseExceptionWithFilterDisabled() throws Exception
new Properties(),
new ServerConfig()
);
Mockito.when(request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)).thenReturn(new AuthenticationResult("userA", "basic", "basic", null));
IOException testException = new IOException(errorMessage);
servlet.handleQueryParseException(request, response, mockMapper, testException, false);
ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class);
Expand Down Expand Up @@ -454,6 +457,7 @@ public ErrorResponseTransformStrategy getErrorResponseTransformStrategy()
}
}
);
Mockito.when(request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)).thenReturn(new AuthenticationResult("userA", "basic", "basic", null));
IOException testException = new IOException(errorMessage);
servlet.handleQueryParseException(request, response, mockMapper, testException, false);
ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class);
Expand Down Expand Up @@ -501,6 +505,7 @@ public ErrorResponseTransformStrategy getErrorResponseTransformStrategy()
}
}
);
Mockito.when(request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)).thenReturn(new AuthenticationResult("userA", "basic", "basic", null));
IOException testException = new IOException(errorMessage);
servlet.handleQueryParseException(request, response, mockMapper, testException, false);
ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class);
Expand Down Expand Up @@ -587,6 +592,7 @@ public int read()
EasyMock.expectLastCall();
requestMock.setAttribute("org.apache.druid.proxy.to.host.scheme", "http");
EasyMock.expectLastCall();
EasyMock.expect(requestMock.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)).andReturn(new AuthenticationResult("userA", "basic", "basic", null));
EasyMock.replay(requestMock);

final AtomicLong didService = new AtomicLong();
Expand Down