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

Support legacy RabbitMQ configuration format #1692

Merged
merged 5 commits into from Aug 26, 2019

Conversation

twillouer
Copy link
Contributor

see #1690

@rnorth
Copy link
Member

rnorth commented Aug 2, 2019

Thanks @twillouer. I have to say, I'm a bit confused that this should make a difference! Is RabbitMQ appending .config on its own when the file extension is just .conf?

It's also slightly worrying that we must not have test coverage for this, otherwise the bug would never have happened! Would you mind adding a test case that we can use to reproduce the problem and see that it's resolved?

@twillouer
Copy link
Contributor Author

@rnorth ok, but I was not able to run graddle check on my computer 👍

./gradlew --no-build-cache  check
13 tests completed, 4 failed, 1 skipped

so... I'm not very confident about adding a new test :)

I try and I update the PR soon

@twillouer
Copy link
Contributor Author

@rnorth PTAL

@rnorth
Copy link
Member

rnorth commented Aug 10, 2019

Interestingly according to the docs:

RABBITMQ_CONFIG_FILE | The path to the configuration file, without the .config extension.

Perhaps there's an undocumented behaviour whereby it automatically strips .config as an extension - which would explain why the .conf extension wouldn't have been working.

Will have a quick play to confirm my suspicions, but perhaps if we're doing it 'by the book' we should not have the .config extension at all...

@rnorth rnorth added this to the next milestone Aug 10, 2019
@twillouer
Copy link
Contributor Author

@rnorth I try (without .config in the end), and it didn't works.

tell me if you're more lucky

@rnorth
Copy link
Member

rnorth commented Aug 10, 2019

Ohoh, this is a bit of a rabbit hole. I tried reverting the change to RabbitMQContainer, expecting the shouldMountConfigurationFile test to fail. It still passed.

Then I found more information in the RabbitMQ docs:

While some settings in RabbitMQ can be tuned using environment variables, most are configured using a configuration file, usually named rabbitmq.conf. This includes configuration for the core server as well as plugins. The sections below cover the syntax, location, how to configure things to which the format isn't well-suited, where to find examples, and so on.

Prior to RabbitMQ 3.7.0, RabbitMQ config file was named rabbitmq.config and used an Erlang term configuration format. That format is still supported for backwards compatibility. Those running 3.7.0 or later are recommended to consider the new sysctl format.

I then tried changing the shouldMountConfigurationFile test to use the rabbitmq:3.6. Then the test failed with the following among the log output:

config file(s) : /etc/rabbitmq/rabbitmq-custom.conf.config (not found)

It looks like 3.6 appends .config automatically, whereas 3.7 does not. Additionally:

  • 3.6 only supports .config files, expressed in Erlang term format
  • 3.7 supports .conf files as well, expressed in sysctl format

Would I be correct in thinking that you are using a <3.7.0 RabbitMQ image for your tests? I was wondering why you had also modified the example config file to use the Erlang format, but it would make sense.

I think that if we want to make this backwards compatible, our only option is to retain the extension of the file provided by the user, and let RabbitMQ figure it out:

    public RabbitMQContainer withRabbitMQConfig(MountableFile rabbitMQConf) {
        // DON'T provide the extension, and allow RabbitMQ to look for config/conf/both
        withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom");
        return withCopyFileToContainer(rabbitMQConf, "/etc/rabbitmq/rabbitmq-custom" + whateverExtensionTheUserProvided); // extract from rabbitMQConf object?
    }

or something like this, anyway.

WDYT?

@twillouer
Copy link
Contributor Author

yes, you're perfectly right, I use a 3.6.15 rabbitmq..

I think your idea might works, but when I try

        withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom");

rabbitmq 3.6 doesn't work correctly...

perhaps we can try to figure out the type of the file (erlang or sysctl format), or try to check version from image name (but not robust in my opinion, if you have a custom image without this name), or, more basicaly, provide two function

    /**
     * Overwrites the default RabbitMQ configuration file with the supplied one.
     *
     * @param rabbitMQConf The rabbitmq.conf file to use (in sysctl format)
     * @return This container.
     */
    public RabbitMQContainer withRabbitMQConfig(MountableFile rabbitMQConf) {
        
        return withRabbitMQConfigSysctl(rabbitMQConf);
    }

    /**
     * Overwrites the default RabbitMQ configuration file with the supplied one.
     *
     * This function doesn't work with RabbitMQ < 3.7.
     *
     * This function and the Sysctl format is recommended for RabbitMQ >= 3.7
     *
     * @param rabbitMQConf The rabbitmq.config file to use (in sysctl format)
     * @return This container.
     */
    public RabbitMQContainer withRabbitMQConfigSysctl(MountableFile rabbitMQConf) {
        withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom");
        return withCopyFileToContainer(rabbitMQConf, "/etc/rabbitmq/rabbitmq-custom.conf");
    }

    /**
     * Overwrites the default RabbitMQ configuration file with the supplied one.
     *
     * @param rabbitMQConf The rabbitmq.config file to use (in erlang format)
     * @return This container.
     */
    public RabbitMQContainer withRabbitMQConfigErlang(MountableFile rabbitMQConf) {
        withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom.config");
        return withCopyFileToContainer(rabbitMQConf, "/etc/rabbitmq/rabbitmq-custom.config");
    }

@rnorth
Copy link
Member

rnorth commented Aug 11, 2019

Hmm, my quick test has it the other way round:

  • 3.6 is OK with withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom");
  • 3.7 is OK with withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom"); but only if rabbitmq-custom.conf exists in sysctl format
  • 3.7 is OK with withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom.config"); if rabbitmq-custom.config exists in Erlang format
  • 3.7 is NOT OK with withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom"); if rabbitmq-custom.config exists in Erlang format

Still, I think your suggested additions to the API make sense really.

@twillouer
Copy link
Contributor Author

ok, I update the PR.

Some remarks:

        withEnv("RABBITMQ_CONFIG_FILE", "/etc/rabbitmq/rabbitmq-custom.config");

works with 3.7 alpine, and if I omit the suffix ".config", it doesn't work.

The file in sysctl format MUST have an empty line in the end, unless you will have:

14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: BOOT FAILED 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: 12:35:13.077 [error] Error transforming datatype for: listeners.tcp.default 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: errorContext: child_terminated 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: reason: killed 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: [gr_lager_default_tracer_counters]}}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {child_type,worker}] 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: reason: killed 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: offender: [{pid,<0.95.0>}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {child_type,worker}] 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {shutdown,brutal_kill}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {restart_type,transient}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {mfargs,{gr_param,start_link,[gr_lager_default_tracer_params]}}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {id,gr_lager_default_tracer_params}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: errorContext: child_terminated 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: supervisor: {local,gr_param_sup} 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: =SUPERVISOR REPORT==== 11-Aug-2019::12:35:13.079505 === 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {shutdown,brutal_kill}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {restart_type,transient}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {mfargs,{gr_counter,start_link, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: {id,gr_lager_default_tracer_counters}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: offender: [{pid,<0.96.0>}, 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: supervisor: {local,gr_counter_sup} 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: =SUPERVISOR REPORT==== 11-Aug-2019::12:35:13.079298 === 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: 12:35:13.077 [error] "5672default_pass = guest" cannot be converted to a(n) IP 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: 12:35:13.077 [error] "5672default_pass = guest" cannot be converted to a(n) integer 14:35:14.188 [Test worker] DEBUG org.testcontainers.containers.output.WaitingConsumer - STDERR: 12:35:13.077 [error] Error generating configuration in phase transform_datatypes

in error.

PTAL

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @twillouer - I'm happy with this!

@rnorth rnorth changed the title use right pattern for rabbit custom config Support legacy RabbitMQ configuration format Aug 26, 2019
@rnorth rnorth merged commit a0f5d00 into testcontainers:master Aug 26, 2019
@twillouer twillouer deleted the patch-2 branch September 8, 2019 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants