ARTEMIS-5871 restrict reload of properties config to the set of reloa…

artemis-server/src/main/java/org/apache/activemq/artemis/core/config/Configuration.java

@@ -1545,4 +1545,5 @@ default boolean isUsingDatabasePersistence() {
    }
 
    Map<String, JaasAppConfiguration> getJaasConfigs();
+   Configuration setJaasConfigs(Map<String, JaasAppConfiguration> configs);
 }

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

@@ -1040,6 +1040,15 @@ public void addJaasConfig(JaasAppConfiguration config) {
       jaasConfigs.put(config.getName(), config);
    }
 
+   @Override
+   public Configuration setJaasConfigs(Map<String, JaasAppConfiguration> configs) {
+      jaasConfigs.putAll(configs);
+      // prune removed entries after update to retain existing entries, this is live config referenced by jaas
+      // see org.apache.activemq.artemis.core.config.impl.ConfigurationImpl.getAppConfigurationEntry
+      jaasConfigs.keySet().retainAll(configs.keySet());
+      return this;
+   }
+
    private void writeProperties(FileWriter writer) throws Exception {
       final BeanUtilsBean beanUtilsBean = new BeanUtilsBean();
       beanUtilsBean.getPropertyUtils().addBeanIntrospector(new FluentPropertyBeanIntrospectorWithIgnores());

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
@@ -4682,29 +4692,51 @@ public void reloadConfigurationFile() throws Exception {
    }
 
    private void reloadConfigurationFile(URL xmlConfigUri) throws Exception {
+      Configuration config = new ConfigurationImpl();
       if (xmlConfigUri != null) {
-         Configuration config = new FileConfigurationParser().parseMainConfig(xmlConfigUri.openStream());
+         config = new FileConfigurationParser().parseMainConfig(xmlConfigUri.openStream());
          LegacyJMSConfiguration legacyJMSConfiguration = new LegacyJMSConfiguration(config);
          legacyJMSConfiguration.parseConfiguration(xmlConfigUri.openStream());
-         configuration.setSecurityRoles(config.getSecurityRoles());
-         configuration.setAddressSettings(config.getAddressSettings());
-         configuration.setDivertConfigurations(config.getDivertConfigurations());
-         configuration.setAddressConfigurations(config.getAddressConfigurations());
-         configuration.setQueueConfigs(config.getQueueConfigs());
-         configuration.setBridgeConfigurations(config.getBridgeConfigurations());
-         configuration.setConnectorConfigurations(config.getConnectorConfigurations());
-         configuration.setAcceptorConfigurations(config.getAcceptorConfigurations());
-         configuration.setAMQPConnectionConfigurations(config.getAMQPConnection());
-         configuration.setPurgePageFolders(config.isPurgePageFolders());
       }
-      configuration.parseProperties(propertiesFileUrl);
+      config.parseProperties(propertiesFileUrl);
+      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());
+      configuration.setAddressConfigurations(config.getAddressConfigurations());
+      configuration.setQueueConfigs(config.getQueueConfigs());
+      configuration.setBridgeConfigurations(config.getBridgeConfigurations());
+      configuration.setConnectorConfigurations(config.getConnectorConfigurations());
+      configuration.setAcceptorConfigurations(config.getAcceptorConfigurations());
+      configuration.setAMQPConnectionConfigurations(config.getAMQPConnection());
+      configuration.setPurgePageFolders(config.isPurgePageFolders());
+      configuration.setConnectionRouters(config.getConnectionRouters());
+      configuration.setJaasConfigs(config.getJaasConfigs());
+   }
+
+   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 {
       if (configurationReloadDeployed.compareAndSet(false, true)) {
          ActiveMQServerLogger.LOGGER.reloadingConfiguration("security settings");

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

@@ -16,12 +16,9 @@
  */
 package org.apache.activemq.artemis.tests.integration.server;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.File;
 import java.io.FileOutputStream;
+import java.io.StringReader;
 import java.util.Properties;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
@@ -36,13 +33,21 @@
 import org.apache.activemq.artemis.core.server.ActiveMQServer;
 import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl;
 import org.apache.activemq.artemis.jms.server.config.impl.FileJMSConfiguration;
+import org.apache.activemq.artemis.json.JsonObject;
 import org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager;
 import org.apache.activemq.artemis.spi.core.security.jaas.InVMLoginModule;
 import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
+import org.apache.activemq.artemis.utils.JsonLoader;
 import org.apache.activemq.artemis.utils.RandomUtil;
 import org.apache.activemq.artemis.tests.util.Wait;
 import org.junit.jupiter.api.Test;
 
+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 {
 
    @Test
@@ -183,6 +188,9 @@ public void testPropertiesOnlyConfigReload() throws Exception {
       properties.put("configurationFileRefreshPeriod", "100");
       properties.put("persistenceEnabled", "false");
       properties.put("connectionRouters.joe.localTargetFilter", "LF");
+      properties.put("acceptorConfigurations.tcp.factoryClassName", NETTY_ACCEPTOR_FACTORY);
+      properties.put("acceptorConfigurations.tcp.params.HOST", "LOCALHOST");
+      properties.put("acceptorConfigurations.tcp.params.PORT", "61616");
 
       try (FileOutputStream outStream = new FileOutputStream(propsFile)) {
          properties.store(outStream, null);
@@ -194,17 +202,20 @@ public void testPropertiesOnlyConfigReload() throws Exception {
       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);
+         assertEquals(100, server.getConfiguration().getConfigurationFileRefreshPeriod());
 
-         properties.put("persistenceEnabled", "false");
+         // verify update and remove of tcp acceptor
+         properties.clear();
          properties.put("configurationFileRefreshPeriod", "100");
-
-         // verify update
+         properties.put("persistenceEnabled", "false");
          properties.put("connectionRouters.joe.localTargetFilter", "UPDATED");
+
+         String startedStatus = server.getStatus();
          try (FileOutputStream outStream = new FileOutputStream(propsFile)) {
             properties.store(outStream, null);
          }
@@ -213,6 +224,22 @@ 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);
+
+         // verify status json reflects update
+         String updatedStatus = server.getStatus();
+         assertNotEquals(startedStatus, updatedStatus);
+         assertTrue(startedStatus.contains(propsFile.getName()));
+         assertTrue(updatedStatus.contains(propsFile.getName()));
+         JsonObject jsonStarted = JsonLoader.readObject(new StringReader(startedStatus));
+         JsonObject jsonUpdated = JsonLoader.readObject(new StringReader(updatedStatus));
+         String alder32Used = jsonStarted.getJsonObject("configuration").getJsonObject("properties").getJsonObject(propsFile.getName()).getString("fileAlder32");
+         String alder32Updated = jsonUpdated.getJsonObject("configuration").getJsonObject("properties").getJsonObject(propsFile.getName()).getString("fileAlder32");
+         assertNotEquals(alder32Used, alder32Updated);
+
       } finally {
          try {
             server.stop();
@@ -350,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();