GGraziadei commented on code in PR #9183:
URL: https://github.com/apache/storm/pull/9183#discussion_r4353098733
##########
storm-webapp/src/main/java/org/apache/storm/daemon/ui/filters/AuthorizedUserFilter.java:
##########
@@ -147,6 +147,18 @@ public void filter(ContainerRequestContext
containerRequestContext) {
ReqContext reqContext = ReqContext.context();
if (reqContext.isImpersonating()) {
+ String realUserName = reqContext.realPrincipal().getName();
Review Comment:
Minor, no need to block on it. This check only looks at the request context,
but for `needsTopoId()` endpoints it runs after the `getTopologyConf` call to
Nimbus above, which it does not need. So a request that is going to be refused
with 401 anyway first costs a Thrift round trip to Nimbus and leaves a
`getTopologyConf` line in the Nimbus access log. Moving this `if` block above
the `needsTopoId()` lookup would reject such requests right away, with no
Nimbus call.
##########
storm-webapp/src/main/java/org/apache/storm/daemon/drpc/webapp/ReqContextFilter.java:
##########
@@ -48,8 +50,23 @@ public ReqContextFilter(IHttpCredentialsPlugin
httpCredsHandler) {
* @param request the request to populate
*/
public void populateContext(HttpServletRequest request) {
+ ReqContext context = ReqContext.context();
if (httpCredsHandler != null) {
- httpCredsHandler.populateContext(ReqContext.context(), request);
+ httpCredsHandler.populateContext(context, request);
+ }
+ context.setRemoteAddress(getRemoteAddress(request));
+ }
+
+ private static InetAddress getRemoteAddress(HttpServletRequest request) {
+ String remoteAddr = request.getRemoteAddr();
+ if (remoteAddr == null || remoteAddr.isEmpty()) {
+ return null;
+ }
+ try {
+ // The servlet container reports an IP address literal, so no name
lookup happens here.
+ return InetAddress.getByName(remoteAddr);
Review Comment:
Nit: the comment is true today but `getByName` would silently start doing a
DNS lookup per request if that ever changes. Instead
`InetAddress.ofLiteral(remoteAddr)` gives the no-lookup guarantee explicitly
(it throws `IllegalArgumentException` for anything that is not a literal).
Verified with java doc:
https://cr.openjdk.org/~prr/8357176/api/java.base/java/net/InetAddress.html#ofLiteral(java.lang.String)
--
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]
