ARTEMIS-5871 make it illegal to provide non xml reloadable config and a properties source when reload is enabled

Author: gtully

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java

@@ -308,7 +308,7 @@ public class ActiveMQServerImpl implements ActiveMQServer {
    protected volatile ExecutorFactory ioExecutorFactory;
 
    /**
-    * This is a thread pool for page only tasks only. This is because we have to limit parallel reads on paging.
+    * This is a thread pool for page tasks only. This is because we have to limit parallel reads on paging.
     */
    protected volatile ExecutorFactory pageExecutorFactory;
 
@@ -679,8 +679,18 @@ public CriticalAnalyzer getCriticalAnalyzer() {
    }
 
    @Override
-   public void setProperties(String fileUrltoBrokerProperties) {
-      propertiesFileUrl = fileUrltoBrokerProperties;
+   public void setProperties(String fileUrlToBrokerProperties) {
+      throwIfReloadableConfigProvidedWithoutFileAndBrokerPropertiesUrlNonNullAndReload(configuration, fileUrlToBrokerProperties);
+      propertiesFileUrl = fileUrlToBrokerProperties;
+   }
+
+   private void throwIfReloadableConfigProvidedWithoutFileAndBrokerPropertiesUrlNonNullAndReload(Configuration configuration, String propertiesFileUrl) {
+      if (configuration.getConfigurationUrl() == null && configuration.getConfigurationFileRefreshPeriod() > 0 && configuration.resolvePropertiesSources(propertiesFileUrl) != null) {
+         // if any non xml (programmatic) provided config is reloadable, on the first properties source reload it will whack that config as the reload has the source of truth for reloadable attributes
+         if (hasReloadableConfig(configuration)) {
+            throw new IllegalStateException(String.format("a properties source (%s) is illegal, programmatic config contains reloadable elements and configurationFileRefreshPeriod > 0; your programmatic config will be replaced on reload of the properties source", propertiesFileUrl));
+         }
+      }
    }
 
    @Override
@@ -4689,8 +4699,16 @@ private void reloadConfigurationFile(URL xmlConfigUri) throws Exception {
          legacyJMSConfiguration.parseConfiguration(xmlConfigUri.openStream());
       }
       config.parseProperties(propertiesFileUrl);
-      configuration.setStatus(config.getStatus());
+      updateReloadableConfigurationFrom(config);
+      updateStatus(ServerStatus.CONFIGURATION_COMPONENT, configuration.getStatus());
+      configurationReloadDeployed.set(false);
+      if (isActive()) {
+         deployReloadableConfigFromConfiguration();
+      }
+   }
 
+   private void updateReloadableConfigurationFrom(Configuration config) {
+      configuration.setStatus(config.getStatus());
       configuration.setSecurityRoles(config.getSecurityRoles());
       configuration.setAddressSettings(config.getAddressSettings());
       configuration.setDivertConfigurations(config.getDivertConfigurations());
@@ -4701,14 +4719,22 @@ private void reloadConfigurationFile(URL xmlConfigUri) throws Exception {
       configuration.setAcceptorConfigurations(config.getAcceptorConfigurations());
       configuration.setAMQPConnectionConfigurations(config.getAMQPConnection());
       configuration.setPurgePageFolders(config.isPurgePageFolders());
-      configuration.setConnectionRouters(config.getConnectionRouters());   // needs reload logic
+      configuration.setConnectionRouters(config.getConnectionRouters());
       configuration.setJaasConfigs(config.getJaasConfigs());
+   }
 
-      updateStatus(ServerStatus.CONFIGURATION_COMPONENT, configuration.getStatus());
-      configurationReloadDeployed.set(false);
-      if (isActive()) {
-         deployReloadableConfigFromConfiguration();
-      }
+   private static boolean hasReloadableConfig(Configuration configuration) {
+      return !configuration.getSecurityRoles().isEmpty() ||
+            !configuration.getAddressSettings().isEmpty() ||
+            !configuration.getDivertConfigurations().isEmpty() ||
+            !configuration.getAddressConfigurations().isEmpty() ||
+            !configuration.getQueueConfigs().isEmpty() ||
+            !configuration.getBridgeConfigurations().isEmpty() ||
+            !configuration.getConnectorConfigurations().isEmpty() ||
+            !configuration.getAcceptorConfigurations().isEmpty() ||
+            !configuration.getAMQPConnection().isEmpty() ||
+            !configuration.getConnectionRouters().isEmpty() ||
+            !configuration.getJaasConfigs().isEmpty();
    }
 
    private void deployReloadableConfigFromConfiguration() throws Exception {

docs/user-manual/config-reload.adoc

@@ -544,4 +544,7 @@ Adding, updating and removing an `<acceptor>` is supported, updating or removing
 === `<queues>` _(Deprecated)_
 
 == Broker Properties
-The location of xref:configuration-index.adoc#broker-properties[brokerProperties] files will be tracked for reload. Any property values that relfect reloadable parameters will take effect after the `configuration-file-refresh-period`.
+The location of xref:configuration-index.adoc#broker-properties[brokerProperties] files will be tracked for reload. Any property values that reflect reloadable parameters will take effect after the `configuration-file-refresh-period`.
+
+[NOTE]
+If programmatic configuration is in play, any configuration object passed to the broker must not contain reloadable config *and* have a broker properties source. The broker does not have any config reload callbacks at this time to protect the programmatic configuration. An IllegalStateException will be thrown if this arises.

tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/routing/ElasticQueueTest.java

@@ -427,6 +427,7 @@ private void prepareNodesAndStartCombinedHeadTail() throws Exception {
          .setAutoDeleteQueues(false).setAutoDeleteAddresses(false); // so slow consumer can kick in!
 
       Configuration baseConfig = new ConfigurationImpl();
+      baseConfig.setConfigurationFileRefreshPeriod(-1);  // the classpath has broker.properties we don't want to reload
       baseConfig.getAddressSettings().put(qName, blockingQueue);
 
       ConnectionRouterConfiguration connectionRouterConfiguration = new ConnectionRouterConfiguration();

tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/ConfigurationTest.java

@@ -45,6 +45,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ConfigurationTest extends ActiveMQTestBase {
@@ -195,20 +196,21 @@ public void testPropertiesOnlyConfigReload() throws Exception {
          properties.store(outStream, null);
       }
       assertTrue(propsFile.exists());
-      properties.clear();
 
       FileConfiguration fc = new FileConfiguration();
       ActiveMQJAASSecurityManager sm = new ActiveMQJAASSecurityManager(InVMLoginModule.class.getName(), new SecurityConfiguration());
       ActiveMQServer server = addServer(new ActiveMQServerImpl(fc, sm));
       server.setProperties(propsFile.getAbsolutePath());    // no xml config
       try {
-
          server.start();
 
          assertEquals(1, server.getConfiguration().getConnectionRouters().size());
          assertEquals("LF", server.getConfiguration().getConnectionRouters().get(0).getLocalTargetFilter());
          assertEquals(1, server.getActiveMQServerControl().getAcceptors().length);
-         // verify update
+         assertEquals(100, server.getConfiguration().getConfigurationFileRefreshPeriod());
+
+         // verify update and remove of tcp acceptor
+         properties.clear();
          properties.put("configurationFileRefreshPeriod", "100");
          properties.put("persistenceEnabled", "false");
          properties.put("connectionRouters.joe.localTargetFilter", "UPDATED");
@@ -222,6 +224,8 @@ public void testPropertiesOnlyConfigReload() throws Exception {
             return "UPDATED".equals(server.getConfiguration().getConnectionRouters().get(0).getLocalTargetFilter());
          });
 
+         // no change
+         assertEquals(100, server.getConfiguration().getConfigurationFileRefreshPeriod());
          // verify remove
          assertEquals(0, server.getActiveMQServerControl().getAcceptors().length);
 
@@ -373,6 +377,35 @@ public void testPropertiesDirWithFilterConfigReloadOnNewFileAfterGettingJournalL
       }
    }
 
+   @Test
+   public void testPropertiesAndProgrammaticReloadableConfigArg() throws Exception {
+
+      File propsFile = new File(getTestDirfile(), "somemore.props");
+      propsFile.createNewFile();
+
+
+      Properties properties = new ConfigurationImpl.InsertionOrderedProperties();
+      properties.put("configurationFileRefreshPeriod", "100");
+      properties.put("persistenceEnabled", "false");
+      properties.put("acceptorConfigurations.reloadable.factoryClassName", NETTY_ACCEPTOR_FACTORY);
+      properties.put("acceptorConfigurations.reloadable.params.HOST", "LOCALHOST");
+      properties.put("acceptorConfigurations.reloadable.params.PORT", "61616");
+
+      try (FileOutputStream outStream = new FileOutputStream(propsFile)) {
+         properties.store(outStream, null);
+      }
+      assertTrue(propsFile.exists());
+
+      ConfigurationImpl programmatic = new ConfigurationImpl();
+      programmatic.addAcceptorConfiguration("tcp", "tcp://localhost:62618");
+      ActiveMQJAASSecurityManager sm = new ActiveMQJAASSecurityManager(InVMLoginModule.class.getName(), new SecurityConfiguration());
+      ActiveMQServer server = addServer(new ActiveMQServerImpl(programmatic, sm));
+
+      assertThrows(IllegalStateException.class, () -> {
+         server.setProperties(propsFile.getAbsolutePath());
+      });
+   }
+
    protected ActiveMQServer getActiveMQServer(String brokerConfig) throws Exception {
       FileConfiguration fc = new FileConfiguration();
       FileJMSConfiguration fileConfiguration = new FileJMSConfiguration();