You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@artemis.apache.org by "KalCramer (via GitHub)" <gi...@apache.org> on 2026/08/13 17:31:28 UTC
[PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
KalCramer opened a new pull request, #6614:
URL: https://github.com/apache/artemis/pull/6614
## Description
Add a `clientFailoverAdvertisingEnabled` connector property to control whether a backup connector is advertised to clients as part of the AMQP failover topology.
Currently, Artemis can advertise the backup connector from the broker's internal cluster topology to AMQP clients. This can cause clients using different internal and external DNS names or ports to receive an unreachable failover endpoint.
For example, an external Qpid JMS client may initially connect using:
```text
amqp://primary.example.com:5672
```
but receive the internal backup connector from the broker topology:
```text
amqp://backup.internal:61616
```
The client can then attempt to use the advertised endpoint instead of the endpoint configured by the client.
This is the scenario described in [ARTEMIS-6004](https://issues.apache.org/jira/browse/ARTEMIS-6004).
### Changes
- Add the `clientFailoverAdvertisingEnabled` connector property.
- Default the property to `true` to preserve existing behavior.
- When set to `false`, the backup connector is not included in the AMQP failover information advertised to clients.
- The connector remains available for internal broker/cluster use; this change only controls client failover advertisement.
- Add unit tests covering:
- failover advertising enabled
- failover advertising disabled
- default behavior
- SSL-enabled failover URIs
### Testing
The change was tested with a Java Qpid JMS client using an Artemis HA live/backup configuration where the broker's internal cluster topology uses different hostnames/ports from those exposed to external clients.
With:
```text
clientFailoverAdvertisingEnabled=false
```
the broker no longer advertises the internal backup connector to the AMQP client.
As a result, external clients **no longer need to configure**:
```text
failover.amqpOpenServerListAction=IGNORE
```
to prevent Qpid JMS from replacing their configured failover endpoints with the broker-provided topology.
This provides a broker-side solution for clients that cannot be configured with `failover.amqpOpenServerListAction=IGNORE`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "KalCramer (via GitHub)" <gi...@apache.org>.
KalCramer commented on PR #6614:
URL: https://github.com/apache/artemis/pull/6614#issuecomment-5606933471
> While this is better than nothing I don't think having just this one option solves issues mentioned in [ARTEMIS-6004](https://issues.apache.org/jira/browse/ARTEMIS-6004) in a meaningful way. Topology is not only used for failover transport, and we still need some kind of vehicle to separate what is sent to clients instead which one is used to form a cluster. Ignoring connector endpoints is only part of it.
>
> Also, don't forget about dynamic topology discovery which doesn't have static connector configuration to attach this property to.
@ViliusS Yeah, this seems like it would be a much larger change. My original implementation was primarily intended to address the specific failure scenario described in the ticket, where an internally advertised connector is not reachable by the external client.
It sounds like the broader solution is to have a separate client-facing TransportConfiguration from the connector used for internal cluster communication, and have that information carried through topology so it can be consumed by Core and other protocols. That would require changes much deeper in the topology path, including core messages such as NodeAnnounceMessage, rather than just filtering the existing failover connector like I am doing now.
If that is the intended direction for ARTEMIS-6004, I think that would be a significantly larger change and likely better handled as a separate MR at this point, with this change serving only as a targeted fix for the specific failure case.
If you don't consider the current approach a reasonable interim fix, I'm happy to close this MR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "KalCramer (via GitHub)" <gi...@apache.org>.
KalCramer commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3937386668
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -150,6 +150,8 @@ public class TransportConstants {
public static final String TRUST_MANAGER_FACTORY_PLUGIN_PROP_NAME = "trustManagerFactoryPlugin";
Review Comment:
Thanks, that makes sense. My manual testing did not seem to need this but I'll add for completeness.
I've added the property to `ALLOWABLE_CONNECTOR_KEYS` and added a regression test in `ConnectorTransportConfigurationParserURITest` to verify that `clientFailoverAdvertisingEnabled=false` is parsed from the connector URI into the `TransportConfiguration` params.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "KalCramer (via GitHub)" <gi...@apache.org>.
KalCramer commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3806947283
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallback.java:
##########
@@ -266,7 +269,14 @@ public URI getFailoverList() {
if (clusterConnection != null) {
TopologyMemberImpl member = clusterConnection.getTopology().getMember(server.getNodeID().toString());
if (member != null) {
- return member.toBackupURI();
+ TransportConfiguration backupConnector = member.getBackup();
Review Comment:
Currently this fix is specific for AMQP. I could see making this naming AMQP specific if that make it more obvious for now. No matter what there won't be a single spot for all protocols unless an abstraction is specifically made for this but I don't think this is something all protocols support so I wouldn't think that makes sense but let me know if that would stop this getting merged. Core might be only other protocol that I know of that might use this but I could be wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3805787122
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallbackTest.java:
##########
@@ -93,4 +105,121 @@ public void testAnonymousSupportCheck() throws Exception {
assertFalse(callback.isSupportsAnonymous());
Mockito.verify(securityStore).authenticate(Mockito.any(), Mockito.any(), Mockito.same(connectionDelegate));
}
+
+ @Test
+ public void testGetFailoverListFailoverEnabled() throws Exception {
+ ActiveMQServer server = Mockito.mock(ActiveMQServer.class);
+
+ TransportConfiguration backup = createBackupTransportConfiguration();
+ backup.getParams().put(TransportConstants.CLIENT_FAILOVER_ADVERTISING_ENABLED_PROP_NAME, true);
+
+ AMQPConnectionCallback callback = createCallback(server, backup);
+
+ URI failoverList = callback.getFailoverList();
+
+ assertNotNull(failoverList);
+ assertEquals("tcp", failoverList.getScheme());
+ assertEquals("backup", failoverList.getHost());
+ assertEquals(61617, failoverList.getPort());
+ assertEquals("sslEnabled=false", failoverList.getQuery());
+ }
+
+ @Test
+ public void testGetFailoverListFailoverDisabled() throws Exception {
+ ActiveMQServer server = Mockito.mock(ActiveMQServer.class);
+
+ TransportConfiguration backup = createBackupTransportConfiguration();
+ backup.getParams().put(TransportConstants.CLIENT_FAILOVER_ADVERTISING_ENABLED_PROP_NAME, false);
+
+ AMQPConnectionCallback callback = createCallback(server, backup);
+
+ assertNull(callback.getFailoverList());
+ }
+
+ @Test
+ public void testGetFailoverListFailoverEnabledByDefault() throws Exception {
+ ActiveMQServer server = Mockito.mock(ActiveMQServer.class);
+
+ TransportConfiguration backup = createBackupTransportConfiguration();
+
+ // CLIENT_FAILOVER_ADVERTISING_ENABLED_PROP_NAME is intentionally not set.
+ // The default value is true.
+ AMQPConnectionCallback callback = createCallback(server, backup);
+
+ URI failoverList = callback.getFailoverList();
+
+ assertNotNull(failoverList);
+ assertEquals("tcp", failoverList.getScheme());
+ assertEquals("backup", failoverList.getHost());
+ assertEquals(61617, failoverList.getPort());
+ assertEquals("sslEnabled=false", failoverList.getQuery());
+ }
+ @Test
Review Comment:
The formatting in this test is off, there's extra spaces, no spaces where there should be, things need to be cleaned up.
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContextTest.java:
##########
@@ -142,4 +142,59 @@ public void testMaxTransferPerDeliveryAppliedToConnectionContext() throws Except
connectionContext.onRemoteOpen(connection);
connectionContext.close(null);
}
+
+
+ @Test
Review Comment:
I can't figure out what you are trying to test here, it appears to be a copy of the above test with no context on what the intent of the test is
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallback.java:
##########
@@ -266,7 +269,14 @@ public URI getFailoverList() {
if (clusterConnection != null) {
TopologyMemberImpl member = clusterConnection.getTopology().getMember(server.getNodeID().toString());
if (member != null) {
- return member.toBackupURI();
+ TransportConfiguration backupConnector = member.getBackup();
Review Comment:
Adding this here seems off if you intend as you've said above for this to be a more generic option applicable to other protocols besides AMQP. Having to copy this code block around for any other implementation that needs it is sub optimal and likely error prone. Seems as though this code should live elsewhere so that it can be called here or in other protocols implementations to check if the connector is to be advertised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3919019067
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -150,6 +150,8 @@ public class TransportConstants {
public static final String TRUST_MANAGER_FACTORY_PLUGIN_PROP_NAME = "trustManagerFactoryPlugin";
Review Comment:
This value should be added to the initialization of ALLOWABLE_ACCEPTOR_KEYS from what I can tell the usage is meant to be
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "KalCramer (via GitHub)" <gi...@apache.org>.
KalCramer commented on PR #6614:
URL: https://github.com/apache/artemis/pull/6614#issuecomment-5673248663
I'll wait to see if I can get some feedback on this MR if possible. I'll have limited access for the next week. There would need to be more discussion on a design before tackling another approach since it's very open ended right now. I may be able to look into an initial design and refine on Jira if I'm told this is not good and needs to be closed, but would need to get specific requirements. I do think that this MR does resolve a problem that I have seen and I wanted to provide a solution in the meantime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "ViliusS (via GitHub)" <gi...@apache.org>.
ViliusS commented on PR #6614:
URL: https://github.com/apache/artemis/pull/6614#issuecomment-5558969897
While this is better than nothing I don't think having just this one option solves issues mentioned in ARTEMIS-6004 in a meaningful way. Topology is not only used for failover transport, and we still need some kind of vehicle to separate what is sent to clients instead which one is used to form a cluster. Ignoring connector endpoints is only part of it.
Also, don't forget about dynamic topology discovery which doesn't have static connector configuration to attach this property to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "KalCramer (via GitHub)" <gi...@apache.org>.
KalCramer commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3806840433
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallbackTest.java:
##########
@@ -93,4 +105,121 @@ public void testAnonymousSupportCheck() throws Exception {
assertFalse(callback.isSupportsAnonymous());
Mockito.verify(securityStore).authenticate(Mockito.any(), Mockito.any(), Mockito.same(connectionDelegate));
}
+
+ @Test
+ public void testGetFailoverListFailoverEnabled() throws Exception {
+ ActiveMQServer server = Mockito.mock(ActiveMQServer.class);
+
+ TransportConfiguration backup = createBackupTransportConfiguration();
+ backup.getParams().put(TransportConstants.CLIENT_FAILOVER_ADVERTISING_ENABLED_PROP_NAME, true);
+
+ AMQPConnectionCallback callback = createCallback(server, backup);
+
+ URI failoverList = callback.getFailoverList();
+
+ assertNotNull(failoverList);
+ assertEquals("tcp", failoverList.getScheme());
+ assertEquals("backup", failoverList.getHost());
+ assertEquals(61617, failoverList.getPort());
+ assertEquals("sslEnabled=false", failoverList.getQuery());
+ }
+
+ @Test
+ public void testGetFailoverListFailoverDisabled() throws Exception {
+ ActiveMQServer server = Mockito.mock(ActiveMQServer.class);
+
+ TransportConfiguration backup = createBackupTransportConfiguration();
+ backup.getParams().put(TransportConstants.CLIENT_FAILOVER_ADVERTISING_ENABLED_PROP_NAME, false);
+
+ AMQPConnectionCallback callback = createCallback(server, backup);
+
+ assertNull(callback.getFailoverList());
+ }
+
+ @Test
+ public void testGetFailoverListFailoverEnabledByDefault() throws Exception {
+ ActiveMQServer server = Mockito.mock(ActiveMQServer.class);
+
+ TransportConfiguration backup = createBackupTransportConfiguration();
+
+ // CLIENT_FAILOVER_ADVERTISING_ENABLED_PROP_NAME is intentionally not set.
+ // The default value is true.
+ AMQPConnectionCallback callback = createCallback(server, backup);
+
+ URI failoverList = callback.getFailoverList();
+
+ assertNotNull(failoverList);
+ assertEquals("tcp", failoverList.getScheme());
+ assertEquals("backup", failoverList.getHost());
+ assertEquals(61617, failoverList.getPort());
+ assertEquals("sslEnabled=false", failoverList.getQuery());
+ }
+ @Test
Review Comment:
I'll look up how to run formatter. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3919019067
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java:
##########
@@ -150,6 +150,8 @@ public class TransportConstants {
public static final String TRUST_MANAGER_FACTORY_PLUGIN_PROP_NAME = "trustManagerFactoryPlugin";
Review Comment:
This value should be added to the initialization of ALLOWABLE_CONNECTOR_KEYS from what I can tell the usage is meant to be. Also a test to ensure that the XML parses out the URI options and applies them as this would show that the option is likely not applied currently in that case since it isn't added to the allowed keys set
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "KalCramer (via GitHub)" <gi...@apache.org>.
KalCramer commented on code in PR #6614:
URL: https://github.com/apache/artemis/pull/6614#discussion_r3806835593
##########
artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContextTest.java:
##########
@@ -142,4 +142,59 @@ public void testMaxTransferPerDeliveryAppliedToConnectionContext() throws Except
connectionContext.onRemoteOpen(connection);
connectionContext.close(null);
}
+
+
+ @Test
Review Comment:
Sorry, this needed to get removed... I ended up moving logic over to AMQPConnectionCallback when I started in AMQPConnectionContext. No changes make in AMQPConnectionContext anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-6004 Add clientFailoverAdvertisingEnabled configuration to not send failover server list info to AMQP clients [artemis]
Posted by "ViliusS (via GitHub)" <gi...@apache.org>.
ViliusS commented on PR #6614:
URL: https://github.com/apache/artemis/pull/6614#issuecomment-5618560979
> > While this is better than nothing I don't think having just this one option solves issues mentioned in [ARTEMIS-6004](https://issues.apache.org/jira/browse/ARTEMIS-6004) in a meaningful way. Topology is not only used for failover transport, and we still need some kind of vehicle to separate what is sent to clients instead which one is used to form a cluster. Ignoring connector endpoints is only part of it.
> > Also, don't forget about dynamic topology discovery which doesn't have static connector configuration to attach this property to.
>
> @ViliusS Yeah, this seems like it would be a much larger change. My original implementation was primarily intended to address the specific failure scenario described in the ticket, where an internally advertised connector is not reachable by the external client.
>
> It sounds like the broader solution is to have a separate client-facing TransportConfiguration from the connector used for internal cluster communication, and have that information carried through topology so it can be consumed by Core and other protocols. That would require changes much deeper in the topology path, including core messages such as NodeAnnounceMessage, rather than just filtering the existing failover connector like I am doing now.
>
> If that is the intended direction for [ARTEMIS-6004](https://issues.apache.org/jira/browse/ARTEMIS-6004), I think that would be a significantly larger change and likely better handled as a separate MR at this point, with this change serving only as a targeted fix for the specific failure case.
>
> If you don't consider the current approach a reasonable interim fix, I'm happy to close this MR.
Looking purely from the perspective of what I was implying in ARTEMIS-6004, your description is correct. I was thinking the same way, about some kind of separate "topology configuration" option which would define full topology and would cover other issues described in the ticket.
As for this pull request, I'm not against it. I'm not on Artemis team, and I don't have enough knowledge about Artemis codebase, so in the end it's up to them to decide if this interim fix is reasonable. I just wanted to clearly indicate that this doesn't cover the ticket fully. Maybe the ticket can be split per use case? I'm not sure what is the best approach here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org