Github user denalex commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1379#discussion_r202134716
--- Diff:
pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java
---
@@ -52,69 +62,90 @@
*/
@Override
public void init(FilterConfig filterConfig) throws ServletException {
+ //TODO: initialize cache here
}
/**
* If user impersonation is configured, examines the request for the
presense of the expected security headers
* and create a proxy user to execute further request chain. Responds
with an HTTP error if the header is missing
* or the chain processing throws an exception.
*
- * @param request http request
+ * @param request http request
* @param response http response
- * @param chain filter chain
+ * @param chain filter chain
*/
@Override
- public void doFilter(final ServletRequest request, final
ServletResponse response, final FilterChain chain) throws IOException,
ServletException {
+ public void doFilter(final ServletRequest request, final
ServletResponse response, final FilterChain chain)
+ throws IOException, ServletException {
if (SecureLogin.isUserImpersonationEnabled()) {
// retrieve user header and make sure header is present and is
not empty
- final String user = ((HttpServletRequest)
request).getHeader(USER_HEADER);
- if (user == null) {
- throw new IllegalArgumentException(MISSING_HEADER_ERROR);
- } else if (user.trim().isEmpty()) {
- throw new IllegalArgumentException(EMPTY_HEADER_ERROR);
+ final String gpdbUser = getHeaderValue(request, USER_HEADER);
+ String transactionId = getHeaderValue(request,
TRANSACTION_ID_HEADER);
+ Integer segmentId = getHeaderValueInt(request,
SEGMENT_ID_HEADER, true);
+ Integer fragmentCount = getHeaderValueInt(request,
FRAGMENT_COUNT_HEADER, false);
+ Integer fragmentIndex = getHeaderValueInt(request,
FRAGMENT_INDEX_HEADER, false);
+
+ SessionId session = new SessionId(segmentId, transactionId,
gpdbUser);
+ if (LOG.isDebugEnabled() && fragmentCount != null) {
+ LOG.debug(session.toString() + " Fragment = " +
fragmentIndex + " of " + fragmentCount);
}
// TODO refresh Kerberos token when security is enabled
- // prepare pivileged action to run on behalf of proxy user
+ // prepare privileged action to run on behalf of proxy user
PrivilegedExceptionAction<Boolean> action = new
PrivilegedExceptionAction<Boolean>() {
@Override
public Boolean run() throws IOException, ServletException {
- LOG.debug("Performing request chain call for proxy
user = " + user);
+ LOG.debug("Performing request chain call for proxy
user = " + gpdbUser);
chain.doFilter(request, response);
return true;
}
};
// create proxy user UGI from the UGI of the logged in user
and execute the servlet chain as that user
- UserGroupInformation proxyUGI = null;
+ UserGroupInformation ugi =
cache.getUserGroupInformation(session);
try {
- LOG.debug("Creating proxy user = " + user);
- proxyUGI = UserGroupInformation.createProxyUser(user,
UserGroupInformation.getLoginUser());
- proxyUGI.doAs(action);
+ ugi.doAs(action);
} catch (UndeclaredThrowableException ute) {
// unwrap the real exception thrown by the action
throw new ServletException(ute.getCause());
} catch (InterruptedException ie) {
throw new ServletException(ie);
} finally {
- try {
- if (proxyUGI != null) {
- LOG.debug("Closing FileSystem for proxy user = " +
proxyUGI.getUserName());
- FileSystem.closeAllForUGI(proxyUGI);
- }
- } catch (Throwable t) {
- LOG.warn("Error closing FileSystem for proxy user = "
+ proxyUGI.getUserName());
- }
+ // Optimization to cleanup the cache if it is the last
fragment
+ boolean forceClean = (fragmentIndex != null &&
fragmentIndex.equals(fragmentCount));
+ cache.release(session, forceClean);
--- End diff --
here you're now relying on release() to not throw any exceptions, even
runtime ones, which is a lot of trust and by no means is guaranteed (we should
treat it as a black box). I'd suggest to restore the previous logic.
---
