rdhabalia closed pull request #885: Cleanup unsused ApiVersionFilter URL: https://github.com/apache/incubator-pulsar/pull/885
This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/conf/broker.conf b/conf/broker.conf index e5a0fd1e4..396272349 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -99,9 +99,6 @@ defaultNumberOfNamespaceBundles=4 # Enable check for minimum allowed client library version clientLibraryVersionCheckEnabled=false -# Allow client libraries with no version information -clientLibraryVersionCheckAllowUnversioned=true - # Path for the file used to determine the rotation status for the broker when responding # to service discovery health checks statusFilePath= diff --git a/conf/standalone.conf b/conf/standalone.conf index 1da8e9a7a..797b07db4 100644 --- a/conf/standalone.conf +++ b/conf/standalone.conf @@ -92,9 +92,6 @@ defaultNumberOfNamespaceBundles=4 # Enable check for minimum allowed client library version clientLibraryVersionCheckEnabled=false -# Allow client libraries with no version information -clientLibraryVersionCheckAllowUnversioned=true - # Path for the file used to determine the rotation status for the broker when responding # to service discovery health checks statusFilePath=/usr/local/apache/htdocs diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java index e917d7e3b..307190ba5 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java @@ -106,8 +106,6 @@ // Enable check for minimum allowed client library version private boolean clientLibraryVersionCheckEnabled = false; - // Allow client libraries with no version information - private boolean clientLibraryVersionCheckAllowUnversioned = true; // Path for the file used to determine the rotation status for the broker // when responding to service discovery health checks private String statusFilePath; @@ -561,14 +559,6 @@ public void setClientLibraryVersionCheckEnabled(boolean clientLibraryVersionChec this.clientLibraryVersionCheckEnabled = clientLibraryVersionCheckEnabled; } - public boolean isClientLibraryVersionCheckAllowUnversioned() { - return clientLibraryVersionCheckAllowUnversioned; - } - - public void setClientLibraryVersionCheckAllowUnversioned(boolean clientLibraryVersionCheckAllowUnversioned) { - this.clientLibraryVersionCheckAllowUnversioned = clientLibraryVersionCheckAllowUnversioned; - } - public String getStatusFilePath() { return statusFilePath; } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/ApiVersionFilter.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/ApiVersionFilter.java deleted file mode 100644 index 9af6d5b09..000000000 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/ApiVersionFilter.java +++ /dev/null @@ -1,144 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.pulsar.broker.web; - -import java.io.IOException; - -import javax.servlet.Filter; -import javax.servlet.FilterChain; -import javax.servlet.FilterConfig; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.http.HttpServletResponseWrapper; - -import org.apache.pulsar.broker.PulsarService; -import org.apache.pulsar.zookeeper.Deserializers; -import org.apache.zookeeper.KeeperException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * Implementation of a servlet {@code Filter} which rejects requests from clients older than the configured minimum - * version. - * - */ -public class ApiVersionFilter implements Filter { - // Needed constants. - private static final String CLIENT_VERSION_PARAM = "pulsar-client-version"; - private static final String MIN_API_VERSION_PATH = "/minApiVersion"; - private static final Logger LOG = LoggerFactory.getLogger(ApiVersionFilter.class); - - /** - * The PulsarService instance holding the Local ZooKeeper cache. We use this rather than the underlying Zk cache - * directly as the cache is not initialized at the time the ApiVersionFilter is constructed. - */ - private final PulsarService pulsar; - - /** If true, clients which do not report a version will be allowed. */ - private final boolean allowUnversionedClients; - - public ApiVersionFilter(PulsarService pulsar, boolean allowUnversionedClients) { - this.pulsar = pulsar; - this.allowUnversionedClients = allowUnversionedClients; - } - - @Override - public void doFilter(ServletRequest req, ServletResponse resp, FilterChain chain) - throws IOException, ServletException { - try { - String minApiVersion = pulsar.getLocalZkCache().getData(MIN_API_VERSION_PATH, - Deserializers.STRING_DESERIALIZER).orElseThrow(() -> new KeeperException.NoNodeException()); - String requestApiVersion = getRequestApiVersion(req); - if (shouldAllowRequest(req.getRemoteAddr(), minApiVersion, requestApiVersion)) { - // Allow the request to continue by invoking the next filter in - // the chain. - chain.doFilter(req, resp); - } else { - // The client's API version is less than the min supported, - // reject the request. - HttpServletResponse httpResponse = (HttpServletResponse) resp; - HttpServletResponseWrapper respWrapper = new HttpServletResponseWrapper(httpResponse); - respWrapper.sendError(HttpServletResponse.SC_BAD_REQUEST, "Unsuported Client version"); - } - } catch (Exception ex) { - LOG.warn("[{}] Unable to safely determine client version eligibility. Allowing request", - req.getRemoteAddr()); - chain.doFilter(req, resp); - } - } - - @Override - public void init(FilterConfig arg0) throws ServletException { - // No init necessary. - } - - @Override - public void destroy() { - // No state to clean up. - } - - /** - * Checks to see if {@code requestApiVersion} is greater than {@code minApiVersion}. Does that by converting both - * {@code minApiVersion} and {@code requestApiVersion} to a floating point number. Assumes that version are properly - * formatted floating point numbers. - * - * Note that this scheme implies that version numbers cannot be of the format x.y.z or any other format which is not - * a valid floating point number. - * - * @param minApiVersion - * @param requestApiVersion - * @return true if requestApiVersion is greater than or equal to minApiVersion - */ - private boolean shouldAllowRequest(String clientAddress, String minApiVersion, String requestApiVersion) { - if (requestApiVersion == null) { - // The client has not sent a version, allow the request if - // configured to do so. - if (LOG.isDebugEnabled()) { - LOG.debug("[{}] Checking client version: req: {} -- min: {} -- Allow unversioned: {}", clientAddress, - requestApiVersion, minApiVersion, allowUnversionedClients); - } - return allowUnversionedClients; - } - try { - float minVersion = Float.parseFloat(minApiVersion); - float requestVersion = Float.parseFloat(requestApiVersion); - - if (LOG.isDebugEnabled()) { - LOG.debug("[{}] Checking client version: req: {} -- min: {}", clientAddress, requestApiVersion, - minApiVersion); - } - return minVersion <= requestVersion; - } catch (NumberFormatException ex) { - LOG.warn("[{}] Unable to convert version info to floats. " + "minVersion = {}, requestVersion = {}", - clientAddress, minApiVersion, requestApiVersion); - throw new IllegalArgumentException("Invalid Number in min or request API version"); - } - } - - private String getRequestApiVersion(ServletRequest req) { - // Implementation assumes that the client version is in an HTTP header - // named client_version. - // TODO (agh) Ensure that this is the case. - HttpServletRequest httpReq = (HttpServletRequest) req; - return httpReq.getHeader(CLIENT_VERSION_PARAM); - } -} diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java index cf5e1eaf2..3b9d81949 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java @@ -144,18 +144,6 @@ public void addServlet(String path, ServletHolder servletHolder, boolean require context.addFilter(filter, MATCH_ALL, EnumSet.allOf(DispatcherType.class)); } - log.info("Servlet path: '{}' -- Enable client version check: {} -- shouldCheckApiVersionOnPath: {}", path, - pulsar.getConfiguration().isClientLibraryVersionCheckEnabled(), - shouldCheckApiVersionOnPath(path)); - if (pulsar.getConfiguration().isClientLibraryVersionCheckEnabled() && shouldCheckApiVersionOnPath(path)) { - // Add the ApiVersionFilter to reject request from deprecated - // clients. - FilterHolder holder = new FilterHolder( - new ApiVersionFilter(pulsar, pulsar.getConfiguration().isClientLibraryVersionCheckAllowUnversioned())); - context.addFilter(holder, MATCH_ALL, EnumSet.allOf(DispatcherType.class)); - log.info("Enabling ApiVersionFilter"); - } - FilterHolder responseFilter = new FilterHolder(new ResponseHandlerFilter(pulsar)); context.addFilter(responseFilter, MATCH_ALL, EnumSet.allOf(DispatcherType.class)); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/PulsarBrokerStarterTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/PulsarBrokerStarterTest.java index 1bb1d1f06..e10d5bade 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/PulsarBrokerStarterTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/PulsarBrokerStarterTest.java @@ -106,7 +106,6 @@ public void testLoadConfig() throws SecurityException, NoSuchMethodException, IO Assert.assertEquals(serviceConfig.getManagedLedgerCursorRolloverTimeInSeconds(), 3000); Assert.assertEquals(serviceConfig.getManagedLedgerMaxEntriesPerLedger(), 25); Assert.assertEquals(serviceConfig.getManagedLedgerCursorMaxEntriesPerLedger(), 50); - Assert.assertTrue(serviceConfig.isClientLibraryVersionCheckAllowUnversioned()); Assert.assertTrue(serviceConfig.isClientLibraryVersionCheckEnabled()); Assert.assertEquals(serviceConfig.getManagedLedgerMinLedgerRolloverTimeMinutes(), 34); Assert.assertEquals(serviceConfig.isBacklogQuotaCheckEnabled(), true); @@ -255,7 +254,6 @@ public void testGlobalZooKeeperConfig() throws SecurityException, NoSuchMethodEx Assert.assertEquals(serviceConfig.getManagedLedgerCursorRolloverTimeInSeconds(), 3000); Assert.assertEquals(serviceConfig.getManagedLedgerMaxEntriesPerLedger(), 25); Assert.assertEquals(serviceConfig.getManagedLedgerCursorMaxEntriesPerLedger(), 50); - Assert.assertTrue(serviceConfig.isClientLibraryVersionCheckAllowUnversioned()); Assert.assertTrue(serviceConfig.isClientLibraryVersionCheckEnabled()); Assert.assertEquals(serviceConfig.getReplicationConnectionsPerBroker(), 12); } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/ApiVersionFilterTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/ApiVersionFilterTest.java deleted file mode 100644 index d6592b563..000000000 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/ApiVersionFilterTest.java +++ /dev/null @@ -1,185 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.pulsar.broker.web; - -import static org.mockito.Matchers.anyObject; -import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import static org.testng.Assert.fail; - -import java.util.Optional; - -import javax.servlet.FilterChain; -import javax.servlet.ServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.ws.rs.WebApplicationException; - -import org.apache.pulsar.broker.PulsarService; -import org.apache.pulsar.broker.authentication.AuthenticationService; -import org.apache.pulsar.broker.service.BrokerService; -import org.apache.pulsar.broker.web.ApiVersionFilter; -import org.apache.pulsar.broker.web.AuthenticationFilter; -import org.apache.pulsar.broker.web.VipStatus; -import org.apache.pulsar.zookeeper.ZooKeeperCache; -import org.mockito.Matchers; -import org.testng.annotations.BeforeMethod; -import org.testng.annotations.Test; - -public class ApiVersionFilterTest { - - private PulsarService pulsar; - private ZooKeeperCache localCache; - private HttpServletRequest req; - private HttpServletResponse resp; - private FilterChain chain; - - @BeforeMethod - public void perTestSetup() { - pulsar = mock(PulsarService.class); - localCache = mock(ZooKeeperCache.class); - req = mock(HttpServletRequest.class); - resp = mock(HttpServletResponse.class); - chain = mock(FilterChain.class); - } - - /** - * Test that if the client reports a valid version, the request is allowed. - */ - @Test - public void testClientWithValidVersion() throws Exception { - ApiVersionFilter filter = new ApiVersionFilter(pulsar, false); - - // Set up mock behaviour for the test. - when(pulsar.getLocalZkCache()).thenReturn(localCache); - when(localCache.getData(eq("/minApiVersion"), Matchers.anyObject())).thenReturn(Optional.of("1.0")); - when(req.getHeader("pulsar-client-version")).thenReturn("1.0"); - - filter.doFilter(req, resp, chain); - verify(chain).doFilter(req, resp); - } - - /** - * Test that if the client reports a version lower than the minApiVersion, the request is rejected. - */ - @Test - public void testClientWithLowVersion() throws Exception { - ApiVersionFilter filter = new ApiVersionFilter(pulsar, false); - - // Set up mock behaviour for the test. - when(pulsar.getLocalZkCache()).thenReturn(localCache); - when(localCache.getData(eq("/minApiVersion"), Matchers.anyObject())).thenReturn(Optional.of("1.0")); - when(req.getHeader("pulsar-client-version")).thenReturn("0.9"); - - filter.doFilter(req, resp, chain); - verify(resp).sendError(HttpServletResponse.SC_BAD_REQUEST, "Unsuported Client version"); - verify(chain, never()).doFilter(req, resp); - } - - /** - * Test that if the min client version cannot be determined from ZooKeeper, the request is allowed. - */ - @Test - public void testZKReadFailureAllowsAccess() throws Exception { - ApiVersionFilter filter = new ApiVersionFilter(pulsar, false); - - // Set up mock behaviour for the test. - when(pulsar.getLocalZkCache()).thenReturn(localCache); - doThrow(new RuntimeException()).when(localCache).getData(eq("/minApiVersion"), Matchers.anyObject()); - when(req.getHeader("pulsar-client-version")).thenReturn("0.9"); - - filter.doFilter(req, resp, chain); - verify(chain).doFilter(req, resp); - } - - /** - * Test that if the either the min client version or the reported version is incorrectly formatted, the request is - * allowed. - */ - @Test - public void testBadVersionFormatRequestIsAllowed() throws Exception { - ApiVersionFilter filter = new ApiVersionFilter(pulsar, false); - - // Set up mock behaviour for the test. - when(pulsar.getLocalZkCache()).thenReturn(localCache); - when(localCache.getData(eq("/minApiVersion"), Matchers.anyObject())).thenReturn(Optional.of("foo")); - when(req.getHeader("pulsar-client-version")).thenReturn("0.9"); - - filter.doFilter(req, resp, chain); - verify(chain).doFilter(req, resp); - } - - /** - * Test that if the client version is not reported, the allowUnversionedClients field is used to determine the - * response. - */ - @Test - public void testDefaultVersionAssumedOnUnreportedVersion() throws Exception { - ApiVersionFilter filter1 = new ApiVersionFilter(pulsar, true); - ApiVersionFilter filter2 = new ApiVersionFilter(pulsar, false); - - // Set up mock behaviour for the test. - when(pulsar.getLocalZkCache()).thenReturn(localCache); - when(localCache.getData(eq("/minApiVersion"), Matchers.anyObject())).thenReturn(Optional.of("1.0")); - - // Returning null here will simulate the client not sending a version, - // which should cause the response to be determined by the - // allowUnversionedClients field. - when(req.getHeader("pulsar-client-version")).thenReturn(null); - - filter1.doFilter(req, resp, chain); - filter2.doFilter(req, resp, chain); - verify(chain).doFilter(req, resp); - } - - @Test - public void testVipStatus() { - VipStatus vipStatus = spy(new VipStatus()); - vipStatus.setPulsar(pulsar); - when(pulsar.getStatusFilePath()).thenReturn("testFile.html"); - try { - vipStatus.checkStatus(); - fail(); - } catch (WebApplicationException e) { - // Ok - } - } - - @Test - public void testAuthFilterTest() throws Exception { - BrokerService nativeService = mock(BrokerService.class); - AuthenticationService authService = mock(AuthenticationService.class); - doReturn(nativeService).when(pulsar).getBrokerService(); - doReturn(authService).when(nativeService).getAuthenticationService(); - doReturn("test-role").when(authService).authenticateHttpRequest(mock(HttpServletRequest.class)); - AuthenticationFilter filter = new AuthenticationFilter(pulsar); - HttpServletRequest request = mock(HttpServletRequest.class); - ServletResponse response = null; - doNothing().when(request).setAttribute(anyString(), anyObject()); - filter.doFilter(request, response, chain); - } -} diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java index f5a6887e9..ebe361cfc 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/WebServiceTest.java @@ -86,43 +86,6 @@ private static final String TLS_CLIENT_KEY_FILE_PATH = "./src/test/resources/certificate/client.key"; /** - * Test that if the enableClientVersionCheck option is enabled, the {@code ApiVersionFilter} is added to the filter - * chain. We test this indirectly by creating live PulsarService and making an http call to it. - */ - @Test - public void testFilterEnabled() throws Exception { - setupEnv(true, "1.0", false, false, false, false); - - // Make an HTTP request to lookup a namespace. The request should fail - // with a 400 error. - try { - makeHttpRequest(false, false); - Assert.fail("Request should have failed."); // We should have gotten an exception on the previous - // line. - } catch (IOException ex) { - Assert.assertTrue(ex.getMessage().contains("HTTP response code: 400")); - } - } - - /** - * Test that if the enableClientVersionCheck option is disabled, the {@code ApiVersionFilter} is not added to the - * filter chain. We test this indirectly by creating live PulsarService and making an http call to it. - * - */ - @Test - public void testFilterDisabled() throws Exception { - setupEnv(false, "1.0", false, false, false, false); - - try { - // Make an HTTP request to lookup a namespace. The request should - // succeed - makeHttpRequest(false, false); - } catch (Exception e) { - Assert.fail("HTTP request to lookup a namespace shouldn't fail ", e); - } - } - - /** * Test that the {@WebService} class properly passes the allowUnversionedClients value. We do this by setting * allowUnversionedClients to true, then making a request with no version, which should go through. * @@ -286,7 +249,6 @@ private void setupEnv(boolean enableFilter, String minApiVersion, boolean allowU config.setAuthenticationEnabled(enableAuth); config.setAuthenticationProviders(providers); config.setAuthorizationEnabled(false); - config.setClientLibraryVersionCheckAllowUnversioned(allowUnversionedClients); config.setSuperUserRoles(roles); config.setTlsEnabled(enableTls); config.setTlsCertificateFilePath(TLS_SERVER_CERT_FILE_PATH); ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services