knox-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pzamp...@apache.org
Subject [knox] branch master updated: KNOX-2371 - DefaultTopologyService may skip cluster config change processing of valid descriptors (#336)
Date Tue, 12 May 2020 22:25:45 GMT
This is an automated email from the ASF dual-hosted git repository.

pzampino pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/knox.git


The following commit(s) were added to refs/heads/master by this push:
     new ce0f2a9  KNOX-2371 - DefaultTopologyService may skip cluster config change processing
of valid descriptors (#336)
ce0f2a9 is described below

commit ce0f2a9d6a757610132343cb6133cc2bda9388e2
Author: Phil Zampino <pzampino@apache.org>
AuthorDate: Tue May 12 18:25:39 2020 -0400

    KNOX-2371 - DefaultTopologyService may skip cluster config change processing of valid
descriptors (#336)
---
 .../org/apache/knox/gateway/GatewayMessages.java   |   3 +-
 .../topology/impl/DefaultTopologyService.java      |  32 +++---
 .../topology/DefaultTopologyServiceTest.java       | 112 ++++++++++++++++++++-
 3 files changed, 126 insertions(+), 21 deletions(-)

diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayMessages.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayMessages.java
index e37f40a..d47583b 100644
--- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayMessages.java
+++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayMessages.java
@@ -606,9 +606,10 @@ public interface GatewayMessages {
 
 
   @Message(level = MessageLevel.ERROR,
-           text = "Encountered an error while responding to {1} @ {0} configuration change:
{2}")
+           text = "Encountered an error processing {2} in response to a {1} @ {0} configuration
change: {3}")
   void errorRespondingToConfigChange(String source,
                                      String clusterName,
+                                     String descriptor,
                                      @StackTrace(level = MessageLevel.DEBUG) Exception e);
 
   @Message(level = MessageLevel.INFO,
diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
b/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
index 66874f9..b9a24cc 100644
--- a/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
+++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
@@ -89,14 +89,14 @@ public class DefaultTopologyService extends FileAlterationListenerAdaptor
implem
 
   private static final JAXBContext jaxbContext = getJAXBContext();
 
-  private static Auditor auditor = AuditServiceFactory.getAuditService().getAuditor(
+  private static final Auditor auditor = AuditServiceFactory.getAuditService().getAuditor(
     AuditConstants.DEFAULT_AUDITOR_NAME, AuditConstants.KNOX_SERVICE_NAME,
     AuditConstants.KNOX_COMPONENT_NAME);
 
   public static final List<String> SUPPORTED_TOPOLOGY_FILE_EXTENSIONS = Collections.unmodifiableList(Arrays.asList("xml",
"conf"));
 
-  private static GatewayMessages log = MessagesFactory.get(GatewayMessages.class);
-  private Map<String, FileAlterationMonitor> monitors = new ConcurrentHashMap<>();
+  private static final GatewayMessages log = MessagesFactory.get(GatewayMessages.class);
+  private final Map<String, FileAlterationMonitor> monitors = new ConcurrentHashMap<>();
   private File topologiesDirectory;
   private File sharedProvidersDirectory;
   private File descriptorsDirectory;
@@ -193,7 +193,6 @@ public class DefaultTopologyService extends FileAlterationListenerAdaptor
implem
             } else {
               Thread.sleep(10);
               elapsed = System.currentTimeMillis() - start;
-              continue;
             }
           } else {
             auditor.audit(Action.REDEPLOY, topology.getName(), ResourceType.TOPOLOGY,
@@ -759,8 +758,8 @@ public class DefaultTopologyService extends FileAlterationListenerAdaptor
implem
    */
   private static class TopologyDiscoveryTrigger implements ClusterConfigurationMonitor.ConfigurationChangeListener
{
 
-    private TopologyService topologyService;
-    private ClusterConfigurationMonitorService ccms;
+    private final TopologyService topologyService;
+    private final ClusterConfigurationMonitorService ccms;
 
     TopologyDiscoveryTrigger(TopologyService topologyService, ClusterConfigurationMonitorService
ccms) {
       this.topologyService = topologyService;
@@ -770,10 +769,11 @@ public class DefaultTopologyService extends FileAlterationListenerAdaptor
implem
     @Override
     public void onConfigurationChange(final String source, final String clusterName) {
       log.noticedClusterConfigurationChange(source, clusterName);
-      try {
-        boolean affectedDescriptors = false;
-        // Identify any descriptors associated with the cluster configuration change
-        for (File descriptor : topologyService.getDescriptors()) {
+      boolean affectedDescriptors = false;
+
+      // Identify any descriptors associated with the cluster configuration change
+      for (File descriptor : topologyService.getDescriptors()) {
+        try {
           SimpleDescriptor sd = SimpleDescriptorFactory.parse(descriptor.getAbsolutePath());
           if (source.equals(sd.getDiscoveryAddress()) && clusterName.equals(sd.getCluster()))
{
             affectedDescriptors = true;
@@ -781,14 +781,14 @@ public class DefaultTopologyService extends FileAlterationListenerAdaptor
implem
             // 'Touch' the descriptor to trigger re-generation of the associated topology
             descriptor.setLastModified(System.currentTimeMillis());
           }
+        } catch (IOException e) {
+          log.errorRespondingToConfigChange(source, clusterName, descriptor.getName(), e);
         }
+      }
 
-        if (!affectedDescriptors) {
-          // If no descriptors are affected by this configuration, then clear the cache to
prevent future notifications
-          ccms.clearCache(source, clusterName);
-        }
-      } catch (Exception e) {
-        log.errorRespondingToConfigChange(source, clusterName, e);
+      if (!affectedDescriptors) {
+        // If no descriptors are affected by this configuration, then clear the cache to
prevent future notifications
+        ccms.clearCache(source, clusterName);
       }
     }
   }
diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java
b/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java
index b71ce27..4b0890d 100644
--- a/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java
+++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java
@@ -25,9 +25,14 @@ import org.apache.knox.gateway.GatewayServer;
 import org.apache.knox.gateway.config.GatewayConfig;
 import org.apache.knox.gateway.services.GatewayServices;
 import org.apache.knox.gateway.services.ServiceType;
+import org.apache.knox.gateway.services.topology.impl.DefaultClusterConfigurationMonitorService;
 import org.apache.knox.gateway.services.topology.impl.DefaultTopologyService;
 import org.apache.knox.gateway.services.topology.monitor.DescriptorsMonitor;
 import org.apache.knox.gateway.services.security.AliasService;
+import org.apache.knox.gateway.topology.ClusterConfigurationMonitorService;
+import org.apache.knox.gateway.topology.discovery.ClusterConfigurationMonitor;
+import org.apache.knox.gateway.topology.simple.SimpleDescriptor;
+import org.apache.knox.gateway.topology.simple.SimpleDescriptorFactory;
 import org.apache.knox.test.TestUtils;
 import org.apache.knox.gateway.topology.Param;
 import org.apache.knox.gateway.topology.Provider;
@@ -37,21 +42,25 @@ import org.apache.knox.gateway.topology.TopologyListener;
 import org.easymock.EasyMock;
 import org.junit.Test;
 
+import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 import static org.easymock.EasyMock.anyObject;
 import static org.hamcrest.CoreMatchers.is;
@@ -70,14 +79,19 @@ public class DefaultTopologyServiceTest {
   }
 
   private File createFile(File parent, String name, String resource, long timestamp) throws
IOException {
+    try(InputStream input = ClassLoader.getSystemResourceAsStream(resource)) {
+      return createFile(parent, name, input, timestamp);
+    }
+  }
+
+  private File createFile(File parent, String name, InputStream content, long timestamp)
throws IOException {
     File file = new File(parent, name);
     if (!file.exists()) {
       FileUtils.touch(file);
     }
-    try(InputStream input = ClassLoader.getSystemResourceAsStream(resource);
-        OutputStream output = FileUtils.openOutputStream(file)) {
-      assertNotNull(input);
-      IOUtils.copy(input, output);
+    try(OutputStream output = FileUtils.openOutputStream(file)) {
+      assertNotNull(content);
+      IOUtils.copy(content, output);
     }
     file.setLastModified(timestamp);
     assertTrue("Failed to create test file " + file.getAbsolutePath(), file.exists());
@@ -602,6 +616,96 @@ public class DefaultTopologyServiceTest {
     }
   }
 
+  /**
+   * KNOX-2371
+   */
+  @Test
+  public void testTopologyDiscoveryTriggerHandlesInvalidDescriptorContent() throws Exception
{
+    File dir = createDir();
+    File topologyDir = new File(dir, "topologies");
+    topologyDir.mkdirs();
+
+    File descriptorsDir = new File(dir, "descriptors");
+    descriptorsDir.mkdirs();
+
+    File sharedProvidersDir = new File(dir, "shared-providers");
+    sharedProvidersDir.mkdirs();
+
+    try {
+      GatewayConfig config = EasyMock.createNiceMock(GatewayConfig.class);
+      EasyMock.expect(config.getGatewayTopologyDir()).andReturn(topologyDir.getAbsolutePath()).anyTimes();
+      EasyMock.expect(config.getGatewayConfDir()).andReturn(descriptorsDir.getParentFile().getAbsolutePath()).anyTimes();
+      EasyMock.replay(config);
+
+      TopologyService ts = new DefaultTopologyService();
+      ts.init(config, Collections.emptyMap());
+
+      ClusterConfigurationMonitorService ccms = new DefaultClusterConfigurationMonitorService();
+      ccms.init(config, Collections.emptyMap());
+
+      // GatewayServices mock
+      GatewayServices gws = EasyMock.createNiceMock(GatewayServices.class);
+      EasyMock.expect(gws.getService(ServiceType.TOPOLOGY_SERVICE)).andReturn(ts).anyTimes();
+      EasyMock.expect(gws.getService(ServiceType.CLUSTER_CONFIGURATION_MONITOR_SERVICE)).andReturn(ccms).anyTimes();
+      EasyMock.replay(gws);
+      setGatewayServices(gws);
+
+      // Write out the referenced provider config first
+      createFile(sharedProvidersDir,
+                 "provider-config-one.xml",
+                 "org/apache/knox/gateway/topology/file/provider-config-one.xml",
+                 System.currentTimeMillis());
+
+      // Create a valid simple descriptor, which depends on provider-config-one.xml
+      File validDescriptorFile = createFile(descriptorsDir,
+                                            "valid-descriptor.json",
+                                            "org/apache/knox/gateway/topology/file/simple-descriptor-six.json",
+                                            System.currentTimeMillis() - TimeUnit.HOURS.toMillis(1));
// One hour ago
+      long initialValidTimestamp = validDescriptorFile.lastModified();
+
+      // Parse the valid test descriptor
+      final SimpleDescriptor validDescriptor = SimpleDescriptorFactory.parse(validDescriptorFile.getAbsolutePath());
+
+      // Create an invalid simple descriptor
+      final String invalidDescriptorContent = "{\"utter\" = \"nonsense\"}";
+      File invalidDescriptorFile =
+              createFile(descriptorsDir,
+                         "invalid-descriptor.json",
+                         new ByteArrayInputStream(invalidDescriptorContent.getBytes(StandardCharsets.UTF_8)),
+                         System.currentTimeMillis()- TimeUnit.MINUTES.toMillis(45)); // 45
minutes ago
+      long initialInvalidTimestamp = invalidDescriptorFile.lastModified();
+
+      // Hack the DefaultTopologyService class to access the internal TopologyDiscoveryTrigger,
+      // which is what we're actually trying to test
+      ClusterConfigurationMonitor.ConfigurationChangeListener topologyDiscoveryTrigger =
null;
+      Class[] classes = DefaultTopologyService.class.getDeclaredClasses();
+      for (Class clazz : classes) {
+        if ("TopologyDiscoveryTrigger".equals(clazz.getSimpleName())) {
+          Constructor ctor =
+                  clazz.getDeclaredConstructor(TopologyService.class, ClusterConfigurationMonitorService.class);
+          ctor.setAccessible(true);
+          topologyDiscoveryTrigger =
+                  (ClusterConfigurationMonitor.ConfigurationChangeListener) ctor.newInstance(ts,
ccms);
+          break;
+        }
+      }
+      assertNotNull("Failed to access the cluster configuration change listener under test.",
topologyDiscoveryTrigger);
+
+      // Invoke the TopologyDiscoveryTrigger
+      topologyDiscoveryTrigger.onConfigurationChange(validDescriptor.getDiscoveryAddress(),
validDescriptor.getCluster());
+
+      assertEquals("Expected the invalid descriptor file's timestamp to have remained unchanged.",
+                   initialInvalidTimestamp,
+                   invalidDescriptorFile.lastModified());
+      assertTrue("Expected the timestamp of the valid descriptor to have been updated.",
+                 (initialValidTimestamp < validDescriptorFile.lastModified()));
+
+    } finally {
+      FileUtils.deleteQuietly(dir);
+      setGatewayServices(null);
+    }
+  }
+
   private class TestTopologyListener implements TopologyListener {
     List<List<TopologyEvent>> events = new ArrayList<>();
 


Mime
View raw message