ConsulSubsitutor does not seem to handle default values

[sleberknight]

Summary

When using the ConsulSubsitutor to retrieve configuration values from the Consul KV store, there seems to be a problem when there are default values.

I used the consul-example application (before removing it from this project, and it's obviously still in git history) to test getting values from Consul's KV store. Here is the YAML config corresponding to the template and defaultName properties in the application's Configuration class:

template: ${helloworld/template:-Hello, %s!}
defaultName: ${helloworld/defaultName:-Stranger}

This is a snippet from the hello-world.yml file that is in the example application.

In this configuration, helloworld/template and helloworld/defaultName are the keys to look up in Consul, and Hello, %s! and Stranger are the default values when not found in Consul.

What should happen:

When the application starts, it should find the values for helloworld/template and helloworld/defaultName in Consul's KV store, and they should be stored in the application's Configuration object at runtime.

What actually happens

The default values are in the Configuration object, not the values from Consul.

This only happens when there are default values specified in the YAML. If the configuration is:

template: ${helloworld/template}
defaultName: ${helloworld/defaultName}

then the values stored in Consul (I used Hola %s for helloworld/template and Cowgirl for helloworld/defaultName) are stored in the Configuration object at runtime.

Steps to reproduce:

1. Start Consul
2. Add values for helloworld/template and helloworld/defaultName in Consul's KV store (e.g. Hola, %s and Cowgirl)
3. Start the example application
4. Make a GET request to the /hello-world endpoint

The output will be: Hello, Stranger! instead of the expected Hola, Cowgirl.

Notes

I searched through the issues in the original dropwizard-consul project and found this issue: working with default values failing. So, I was not the first person to find this problem. That issue was reported on Jan 2, 2018 and was automatically closed as stale by a GitHub action.

[sleberknight]

I changed this to an investigation because it might be caused by the way the example application is implemented.

It explicitly adds an environment variable StringSubstitutor in the initialize of the Application class before adding the ConsulBundle, which then adds the Consul substitutor.

So, it might be that whenever there is a default value in a template variable, it uses that when it fails to find a value and doesn't consult any other substitutors. This needs more investigation to determine the exact behavior.

[sleberknight]

I performed additional investigation using seven different configuration scenarios, and two application runtime configurations.

The two application configurations are:

1. CS - The Application class only registers a ConsulSubstitutor (indirectly when adding the ConsulBundle)
2. ECS - The Application class registers an EnvironmentVariableSubstitutor and then registers a ConsulSubstitutor (indirectly when adding the ConsulBundle)

The seven configuration scenarios are:

1. Value in Configuration class (i.e. a field in the Configuration class that has a default value)
2. Value in Configuration class, literal value in YAML config file
3. Value in Configuration class, template value in YAML config, value Consul KV store
4. Value in Configuration class, template value in YAML config with default, value Consul KV store
5. Value in Configuration class, template value in YAML config with default
6. Value in Configuration class, template value in YAML config with default, value in environment variable
7. Value in Configuration class, template value in YAML config with no default, value in environment variable

A value in the Configuration class looks like:

private String template = "Hello there, %s!";
private String defaultName = "Stranger";

The literal value in YAML looks like:

template: Hello there, %s!
defaultValue: Stranger

A template value in YAML looks like:

template: ${helloworld/template}
defaultName: ${helloworld/defaultName}

And, template value in YAML with default value looks like:

template: ${helloworld/template:-Hello, %s!}
defaultName: ${helloworld/defaultName:-Stranger}

The table below shows the results of attempting each of the seven configuration scenarios for the two application configurations.

Scenario	CS	ECS	Final value
1	✔️	✔️	Value of field in Configuration object
2	✔️	✔️	Literal value in YAML
3	✔️	✔️	Value in Consul KV
4	✔️	✔️	Value in Consul KV
5	✔️	✔️	Default value in YAML
6	❌	❌	Default value in YAML (expected to be value of environment variable)
7	✔️	✔️	Value of environment variable

As is shown in the table, only one scenario fails, which occurs when there is a field in the Configuration class with a default value, and there is a template value in the YAML config with a default value, and there is an environment variable, but there is no value in Consul KV. The final value is from the default value in the YAML config.

I am fairly sure this occurs because the ConsulSubstitutor attempts a Consul KV lookup, fails, sees that there is a default value from the YAML config, and just uses that instead of attempting to see if there is an environment variable before falling back to the default value in the YAML. This makes a certain amount of sense, but I would logically expect a "chain" of look-ups to try the first one, then the second one, and so on. For example, you might expect this to behave like:

flowchart LR
    ckv(Consul KV) --> ev(Environment Value) --> dvy(Default value in YAML) --> dvcc(Default value in Configuration class)

or maybe you would want to check for the environment variable first and the Consul KV value second:

flowchart LR
    ev(Environment Value) --> ckv(Consul KV) --> dvy(Default value in YAML) --> dvcc(Default value in Configuration class)

This kind of chained fallback behavior can be implemented, but not without creating additional code and changing the existing ConsulSubstitutor. In fact, the chained fallback can be implemented as a StringLookup e.g.

public class StringLookupChain implements StringLookup {

    private final List<StringLookup> lookups;

    public StringLookupChain(List<StringLookup> lookups) {
        checkArgument(nonNull(lookups) && !lookups.isEmpty(), "lookups must not be null or empty");
        this.lookups = lookups;
    }

    @Override
    public String lookup(String key) {
        return lookups.stream()
                .map(nextLookup -> nextLookup.lookup(key))
                .map(Strings::nullToEmpty)
                .filter(str -> !str.isEmpty())
                .findFirst()
                .orElse(null);
    }
}

And then the ConsulSubstitutor constructor can be changed as follows to set a chained variable resolver:

StringLookup envLookup = System::getenv;
var consulLookup = new ConsulLookup(consul, strict);

// In this implementation, environment variables take precedence over Consul KV values
var lookupChain = new StringLookupChain(List.of(envLookup, consulLookup));
this.setVariableResolver(lookupChain);