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

fix(modules/rabbitmq): update container image to 3.12.11 to allow connections without passing admin credentials #2051

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

abemedia
Copy link
Contributor

@abemedia abemedia commented Dec 30, 2023

What does this PR do?

Updates rabbitmq image to 3.12.11-management-alpine.

Why is it important?

The RabbitMQ module was broken if used without passing in an admin username that is not guest and would fail with the error user 'guest' can only connect via localhost (see #1702). It appears this issue no longer happens on 3.12.

Related issues

@abemedia abemedia requested a review from a team as a code owner December 30, 2023 14:08
Copy link

netlify bot commented Dec 30, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 263fa38
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65a07b06b5307500084e1029
😎 Deploy Preview https://deploy-preview-2051--testcontainers-go.netlify.app/modules/rabbitmq
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@abemedia abemedia changed the title chore(modules/rabbitmq): update image to 3.12.22 chore(modules/rabbitmq): update image to 3.12.11 Dec 30, 2023
@abemedia abemedia force-pushed the chore/update-rabbitmq branch from 8b48258 to ae6a3e7 Compare December 30, 2023 14:15
@abemedia abemedia changed the title chore(modules/rabbitmq): update image to 3.12.11 chore(modules/rabbitmq): update container image to 3.12.11 Dec 30, 2023
@mdelapenya mdelapenya self-assigned this Jan 8, 2024
@abemedia
Copy link
Contributor Author

abemedia commented Jan 9, 2024

@mdelapenya any chance of getting this merged?

@mdelapenya
Copy link
Member

Hey @abemedia thanks for pointing this out. We usually do not bump the default images unless it's necessary for a critical/security bug, making users responsible of setting the right image with testcontainers.WithImage.

In this particular case, where you express the concerns of using the current default version, I'd add a warning in the docs explaining it: "if you use version lower than 3.12, please set the admin username, otherwise you'll get the error". Or even perform those checks in the RunContainer function.

Wdyt?

On the other hand, @eddumelendez and I have discussed a lot about default images, and we'll probably change the current model in the future. Options are: a) forcing users to pass the image b) defining a minimum supported version, among others.

This is important because the current default establishes a version that is tested (verified) by our own tests, but it does not test lower versions, or higher versions, only those defined in the tests. And there are modules where the features change depending on the versions (see Elasticsearch module and how we extract the SSL certificate for versions >= 8.x). What if a user uses a very old or very new version for a given technology, and the technology offers different features than the ones provided by the module? Should we fail/panic/error?

So for us it's not easy decision to simply bump the version. Well it's simple to merge this 😅 as it's basically correct, but to calculate all the implications of bumping the module is more complex than that.

In any case, let me double check with @eddumelendez and @kiview regarding this topic one more time. Hope I can come with a good answer 🙏 . Real soon 🤞

@abemedia
Copy link
Contributor Author

Makes sense, however since using the current module with defaults is essentially broken I'd argue that updating the default image makes sense here.

What tests do you think would be necessary to consider this image fully supported? I'm happy to help with that in the evenings once I get off work.

@mdelapenya
Copy link
Member

since using the current module with defaults is essentially broken I'd argue that updating the default image makes sense here.

One thing I'm double checking ATM is the existing tests that still use the rabbitmq:3.7.25-management-alpine version:

  • ExampleRunContainer -> passes admin username and password
  • ExampleRunContainer_connectUsingAmqp -> passes admin username and password
  • ExampleRunContainer_withSSL -> only passes SSL settings
  • ExampleRunContainer_withPlugins -> only passes two startup commands
  • ExampleRunContainer_withCustomConfigFile -> no options passed
  • TestRunContainer_withAllSettings -> does not pass admin credentials

Why they are passing correctly then?

@abemedia
Copy link
Contributor Author

abemedia commented Jan 11, 2024

If you comment out the rabbitmq.WithAdminUsername and rabbitmq.WithAdminPassword in ExampleRunContainer_connectUsingAmqp and run the example you'll see what I mean. You get the following logs in the container:

2024-01-11 23:20:07.043 [info] <0.696.0> accepting AMQP connection <0.696.0> (172.17.0.1:35938 -> 172.17.0.5:5672)
2024-01-11 23:20:07.053 [error] <0.696.0> Error on AMQP connection <0.696.0> (172.17.0.1:35938 -> 172.17.0.5:5672, state: starting):
PLAIN login refused: user 'guest' can only connect via localhost
2024-01-11 23:20:10.054 [info] <0.696.0> closing AMQP connection <0.696.0> (172.17.0.1:35938 -> 172.17.0.5:5672)

If you then also comment out testcontainers.WithImage it works.

@abemedia
Copy link
Contributor Author

I think we should probably add a test that runs the container without passing options and tries to connect with github.com/rabbitmq/amqp091-go to run in CI.

@abemedia
Copy link
Contributor Author

Have just updated the image tags in the tests and added a test to connect with AMQP using just defaults (no options).

@mdelapenya
Copy link
Member

If you comment out the rabbitmq.WithAdminUsername and rabbitmq.WithAdminPassword in ExampleRunContainer_connectUsingAmqp and run the example you'll see what I mean.

So the issue is not the container not being able to start but the connections to it not being done, right? If that's the case, then I'd mark this as a bug instead. Wdyt?

@abemedia
Copy link
Contributor Author

That's a good point!

@mdelapenya mdelapenya changed the title chore(modules/rabbitmq): update container image to 3.12.11 fix(modules/rabbitmq): update container image to 3.12.11 to allow connections without passing admin credentials Jan 12, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Jan 12, 2024
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the discussion. It helped to realise and consider this as a bug instead of a simple bump.

I've changed the title to explain it better in the release notes.

@mdelapenya mdelapenya merged commit 7b06a62 into testcontainers:main Jan 12, 2024
115 of 116 checks passed
@abemedia abemedia deleted the chore/update-rabbitmq branch January 12, 2024 20:51
@abemedia
Copy link
Contributor Author

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot connect to RabbitMQ (module)
2 participants