Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RabbitMQ Stream service connection from RabbitMQContainer #42443

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eddumelendez
Copy link
Contributor

Add RabbitStreamConnectionDetails and support from RabbitMQContainer
when rabbitmq_stream plugin is enabled.

Add `RabbitStreamConnectionDetails` and support from `RabbitMQContainer`
when `rabbitmq_stream` plugin is enabled.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 25, 2024
Copy link
Contributor

@mhalbritter mhalbritter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @eddumelendez, thanks for the PR. I've left some comments for your consideration.

private static RabbitMQContainer getRabbitMqStreamContainer() {
RabbitMQContainer container = TestImage.container(RabbitMQContainer.class);
container.addExposedPorts(RABBITMQ_STREAMS_PORT);
var enabledPlugins = "[rabbitmq_stream,rabbitmq_prometheus].";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use var in the codebase.

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label Sep 25, 2024
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when rabbitmq_stream plugin is enabled

Apologies if I've missed it, but I can't see any code in the connection details factory that checks that the stream plugin is enabled. What'll happen if the factory kicks in on a container without streams enabled? I'm wondering if it may break somehow, perhaps by overriding the app's Rabbit Streams configuration with connection details that won't work.

@eddumelendez
Copy link
Contributor Author

rabbitmq_stream plugin uses a new port 5552. If the plugin is not enabled, then it will fail because can not get the mapped port. This also requires an additional bean, otherwise will fail. See https://www.rabbitmq.com/blog/2021/07/23/connecting-to-streams#using-the-stream-java-client-with-a-load-balancer

@Bean
EnvironmentBuilderCustomizer environmentBuilderCustomizer() {
	return env -> {
		Address entrypoint = new Address(rabbit.getHost(), rabbit.getMappedPort(RABBITMQ_STREAMS_PORT));
		env.addressResolver(address -> entrypoint);
	};
}

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 25, 2024
@eddumelendez
Copy link
Contributor Author

PR has been updated

import org.springframework.boot.autoconfigure.service.connection.ConnectionDetails;

/**
* Details required to establish a connection to a RabbitMQ service.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Especially given the javadoc, it looks like to me that the existing RabbitConnectionDetails should be enough, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rabbitmq stream uses a different port and in order to do that the plugin should be enabled. Having two implementations of RabbitConnectionDetails will lead to having an exception with 2 beans unless other beans or configuration are disabled when stream are enabled.

I think would be possible to have a different virtual host for stream in a single RabbitMQ instance, so, having a specific ConnectionDetails would make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a bug in the current code as we're not taking RabbitConnectionDetails into account when falling back from stream-specific configuration to general Rabbit configuration. I've opened #42489 for that. When fixed, we'll fallback from spring.rabbit.stream.* properties to RabbitConnectionDetails. With the changes here in place, we'd then fallback from RabbitStreamConnectionDetails (a replacement for some of the spring.rabbit.stream.* properties) to RabbitConnectionDetails (a replacement for some of the general spring.rabbit.* properties).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this some more, Rabbit Streams support is deceptively complicated (to me at least) as I think it's the first time where different users may want different things. One user may want RabbitConnectionDetails and RabbitStreamConnectionDetails to be sourced from the same container as their app is using the same Rabbit broker for both streaming and regular messaging. Another user may want RabbitConnectionDetails and RabbitStreamConnectionDetails to be sourced from different containers as their app is using different Rabbit brokers for streaming and regular messaging.

With only RabbitMQContainer to look at, even if we could detect in the container connection details factory that the streams plugin is enabled, we wouldn't know if the user wanted this container to be used only for streams or for both streams and regular messaging. It feels like we may need some other signal to indicate how the container should be used as a source for connection details. This reminds me a bit of the exploratory work being done for SSL and the possibility of having SSL-related annotations on the container field. I'll flag this one for a meeting so I can see what the rest of the team thinks.

Copy link
Member

@wilkinsona wilkinsona Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We think we may be able to check to see if the default streams port is exposed by the container. If it isn't, the factory should return null.

We should also think about Docker Compose as, ideally, we'd keep the supported services in sync across Compose and Testcontainers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further thought, I'm not sure that this will work for all cases. Those cases are:

  1. One container to be used for both streams and "standard" messaging
  2. Two containers: one for streams and one for standard messaging

In the two container case, RabbitContainerConnectionDetailsFactory will match a container with the streams port exposed and I think we'll end up with a duplicate connection details failure. If we made RabbitContainerConnectionDetailsFactory ignore containers with the streams port exposed it would fix the two container case but it would break the one container case.

Some experimentation's needed to see if the above's accurate.

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 30, 2024
@wilkinsona wilkinsona added for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 2, 2024
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Oct 9, 2024
@wilkinsona wilkinsona added this to the 3.x milestone Oct 9, 2024
@wilkinsona wilkinsona added the for: merge-with-amendments Needs some changes when we merge label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants