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.
---