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
