CLOUD-3621 - Correct default bindings / jms-connection factory for usage with external broker

[luck3y]

https://issues.redhat.com/browse/CLOUD-3621
Signed-off-by: Ken Wills [email protected]

[luck3y]

Note, this and #180 will need to be cherry picked to the 0.18.x branch.

[jfdenise]

@luck3y , what happens with this change when we don't have remoting in the configuration? The embedded broker is enabled? This impacts 7.3 behavior. Embedded server was not started when provisioning Galleon layers (eg: cloud-server).

[yersan]

@jfdenise there is an additional check for the presence of the remoting subsystem before adding the embedded server at messaging.sh#L1002, so it won't be configured.

@luck3y I don't understand at all the motivation of this change, Jira title is "External broker configuration requires unnecessary remoting". When there is an external broker, REMOTE_AMQ_BROKER is flagged as true, and then any embedded server configuration is bypassed messaging.sh#L129

The can_add_embedded() verifies some requirements for the embedded server only, so I don't see the relation with external brokers. What is the case this PR is fixing?

[luck3y]

@yersan See here:

wildfly-cekit-modules/jboss/container/wildfly/launch/messaging/added/launch/messaging.sh

Line 949 in 7633804

if ([ "$REMOTE_AMQ_BROKER" = "true" ] || ([ -n "${MQ_QUEUES}" ] || [ -n "${HORNETQ_QUEUES}" ] || [ -n "${MQ_TOPICS}" ] || [ -n "${HORNETQ_TOPICS}" ] || [ "x${DISABLE_EMBEDDED_JMS_BROKER}" != "xtrue" ]) && [ "x${DISABLE_EMBEDDED_JMS_BROKER}" != "xalways" ]) && [ -n "${defaultJmsConnectionFactoryJndi}" ]; then

If can_add_embedded() ends up setting DISABLE_EMBEDDED_JMS_BROKER=always then this line doesn't end up setting the default bindings, so the lookup fails when you deploy an app.

[yersan]

Then I believe the fix should be there in L949.
The use of DISABLE_EMBEDDED_JMS_BROKER=always is to prevent any configuration that could be added for the embedded server.

What do you think if we replace the complex if by:

if [ "$REMOTE_AMQ_BROKER" = "true" ] && [ -n "${defaultJmsConnectionFactoryJndi}" ]; then
  echo "/subsystem=ee/service=default-bindings:write-attribute(name=jms-connection-factory, value=\"${defaultJmsConnectionFactoryJndi}\")" >> "${CLI_SCRIPT_FILE}"
elif ([ -n "${MQ_QUEUES}" ] || [ -n "${HORNETQ_QUEUES}" ] || [ -n "${MQ_TOPICS}" ] || [ -n "${HORNETQ_TOPICS}" ] || [ "x${DISABLE_EMBEDDED_JMS_BROKER}" != "xtrue" ]) && [ "x${DISABLE_EMBEDDED_JMS_BROKER}" != "xalways" ] && [ -n "${defaultJmsConnectionFactoryJndi}" ]; then
  echo "/subsystem=ee/service=default-bindings:write-attribute(name=jms-connection-factory, value=\"${defaultJmsConnectionFactoryJndi}\")" >> "${CLI_SCRIPT_FILE}"
else
  echo "/subsystem=ee/service=default-bindings:undefine-attribute(name=jms-connection-factory)" >> "${CLI_SCRIPT_FILE}"
fi

It also duplicate one line, but I think it gets more clear the intention. Would that work?

[luck3y]

@yersan That seems fine to me, so +1. Do you want me to make the change?

[yersan]

Thanks @luck3y , yes, please, if you can, force-push the PR, I haven't executed the bats tests or test with a deployment though. Notice there is also a similar check when we are running on xml mode, see L936

[luck3y]

@yeray this should be all set -- in the case of xml config, the if should short circuit if the remote broker is in use on L936, so it seems fine to me, plus the tests pass :)

[yersan]

Hi @luck3y , I think L936 should be fixed as well, since the && is executed before || and both side of the operand will be executed first. Grouping with a parentheses would be enough, or divide the if in a if..elif..else as we did with the other one to make it clearer.

In L936, If remoting subsystem is not present but there is a remote broker, the default-bindings won't be configured because the logical operands will return false.

[luck3y]

@yersan Ah, yes, thanks. I think I'd just stared at it long enough to convince myself it was fine :) I'll add that here too.