search

softwaremill / elasticmq

In-memory message queue with an Amazon SQS-compatible interface. Runs stand-alone or embedded.
https://softwaremill.com/open-source/
Apache License 2.0
2.55k stars 196 forks source link

Make optional config optional and/or include a config schema in the read me #584

Closed Hamzaan-Bridle closed 2 years ago

Hamzaan-Bridle commented 2 years ago

Using the example config from README does not work with new ElasticMQServerConfig(config) due to missing config values! If these values are not required, ElasticMQServerConfig should handle missing config values.

If the values missing from the README example are required, could we please have a schema added to repo showing what the full conf file should look like?

Tested on:

szymonneo commented 2 years ago

Hi can you send more info about this issue? What is your config exactly? Provide some stacktrace and environment variables please. Have you tried to build it with sbt?

Hamzaan-Bridle commented 2 years ago

Hi, The config was literally taking the values from the installation stand alone section of the readme, but filling in the queues and queues-storage from the following sections.

The full config I used (that worked) needed messages-storage added to, with a lot of the config settings added to. Looking at ElasticMQServerConfig.scala, the calls to subConfig.getString are throwing due to the implementation in java:

    /**
     * @param path
     *            path expression
     * @return the string value at the requested path
     * @throws ConfigException.Missing
     *             if value is absent or null
     * @throws ConfigException.WrongType
     *             if value is not convertible to a string
     */
    String getString(String path);

StackTrace txt file copy of the conf (github didn't like the .conf extension)

micossow commented 2 years ago

Could you please provide a sample application reproducing the issue? It seems like you are passing invalid Config object to the constructor of ElasticMQServerConfig. Such config has to include default settings that can be found in server/src/main/resources/reference.conf

Hamzaan-Bridle commented 2 years ago

Such config has to include default settings that can be found in server/src/main/resources/reference.conf

I didn't know such a file existed! It would be very helpful if it was referenced on the README.md!

Even then, it's still difficult to see what config is/is not required as there is no indication in either the README.md or the server/src/main/resources/reference.conf file. What config can be skipped? Is there any config that must match between sections? E.g.

Basically there is a fair bit missing from config documentation, but the original issue I was raising is that there is no clear documentation for what is or is not required in the config.

micossow commented 2 years ago

If you use it like in the Starting an embedded ElasticMQ server with an SQS interface section, then you should not be bothered by this. The problem is that you probably have something in your project that makes ConfigFactory.load(...) skip the reference.conf or you are doing something wrong when creating the Config object. If you provided a minimal project reproducing the issue, then we would be able to determine whether some vital information are missing in README.

Hamzaan-Bridle commented 2 years ago

What should be the contents of the elasticmq.conf file referenced in this code from Starting an embedded ElasticMQ server with an SQS interface?

val config = ConfigFactory.load("elasticmq.conf")
val server = new ElasticMQServer(new ElasticMQServerConfig(config))
server.start()

Creating a conf file based solely on the README.md won't have the details from reference.conf...

micossow commented 2 years ago

It should only contain your customizations. reference.conf will be loaded as fallback for missing keys.

Hamzaan-Bridle commented 2 years ago

I can't see a reference in ElasticMQServerConfig where it would fall back to reference.conf!

Is this done by conf file itself? Something like

# the include should be done only once, at the beginning of the custom configuration file
include classpath("application.conf")

should have reference.conf instead of application.conf?

# the include should be done only once, at the beginning of the custom configuration file
include classpath("reference.conf")

(I can't produce a test app on this machine)

micossow commented 2 years ago

It's how Lightbend Config lib, that we use, works. You can check the docs for more details.

Hamzaan-Bridle commented 2 years ago

Okay, that lib is neat. A random note from digging into it - you probably want to use the config.checkValid in ElasticMQServerConfig. In fact, looking at their example, you probably want to also add a default constructor too!

I was able to find my issue. As I was developing a Maven plugin to start the Elastic MQ before integration tests were run (and shutdown afterwards), the config file was living on a different path to the class loading the config. This meant the config was loaded using ConfigFactory.parseFile(new File(<absoluteFilePath>)), which does not load in the application config or the reference config!

I was able to fix the issue using ConfigFactory.parseFile(new File(<absoluteFilePath>)).withFallback(ConfigFactory.load()), but it's not ideal having to load in the entirety of the application config just to fill in the defaults of the ElasticMQ library.

I guess including the config.checkValid will help with errors, but in terms of this "issue", I think something in the readme pointing users to the reference.conf will be enough. This would sufficient for use cases that cannot use ConfigFactory.load(...) as they'll be pointed to a reference config file to use!

Edit: In the end I used the following

Config elasticMqReference = ConfigFactory.parseResourcesAnySyntax(ElasticMQConfig.class.getClassLoader(), "reference.conf");
Config config = ConfigFactory.parseFile(new File(configFileName)).withFallback(elasticMqReference);
micossow commented 2 years ago

but it's not ideal having to load in the entirety of the application config just to fill in the defaults of the ElasticMQ library.

it's exactly how it supposed to be used. You should always use .withFallback(ConfigFactory.load()) as there's nothing dangerous about using default configuration. I know it may be confusing if you haven't used typesafe config library before (it's common for Akka based projects).

Hamzaan-Bridle commented 2 years ago

it's exactly how it supposed to be used. You should always use .withFallback(ConfigFactory.load()) as there's nothing dangerous about using default configuration. I know it may be confusing if you haven't used typesafe config library before (it's common for Akka based projects).

It's not that it is confusing, it's that it doesn't compartmentalise config. If you're passing in a custom config file, you'll likely have multiple configs running in the same application. As such, falling back to a global application config sounds like a surefire way to get configs bleeding into each other and creating very annoying bugs or possible security/other issues.

When using ConfigFactory.load() you may encounter issues due to:

To be brutally honest, from a security point of view, global config loading isn't great. I'd prefer to have all config fixed by the application and not be pulling config globally from different libraries, especially as libraries may not have unique root config names. E.g. the akka config ssl-config is a config name that might easily be re-used by different developers, so ConfigFactory.load() may lead to SSL security issues based on the order it loads different reference.conf files.

Not the most relevant for a library meant for testing, but in general you'd ideally want to have the option to make config as isolated as possible for areas with security or data integrity considerations!

That's my reasoning behind calling it "not ideal" to load the full application config for the defaults. If it's hard to debug and hard fix to specific values, then it's not ideal 🤷

adamw commented 2 years ago

@Hamzaan-Bridle I think the idea is that reference.conf should only specify config values for the keys that are defined by a given module. Hence, you should never have a value for the same key in two different reference.conf files - as this would cause the problems you describe.

So when using custom config, you always have the same "default" set of values defined by all reference.conf files merged - but since there are no duplicates, there are no conflicts - plus whatever you specify in your config file.

Hamzaan-Bridle commented 2 years ago

You cannot guarantee that, especially when your config doesn't have a "unique root" - e.g. compare the elastic mq config like node-address (which is a name another library may use) to akka.ssl-config in com.typesafe.akka:akka-stream, the latter has the config that is more likely to conflict with another library within a root node.

On a side note: the reference file in akka-stream declares both akka.ssl.config and ssl-config as there is a name space ssl-config from an external library that it is stepping over...

adamw commented 2 years ago

You are right that we should have scoped our config to a unique namespace. Well, maybe in some future major release. In fact, maybe there should be an issue for that?

As for akka, hard to comment. Probably that's for backwards compat, but maybe it's just lenience.