syntax ${ENV:VARIABLE} doesn't work in event-listener.properties

[shawnzhu]

This from the release 358.

Problem

I've followed the doc Secrets to inject secrets into etc/event-listener.properties but found the injected environment variables are not resolved at all. (such syntax works well for config.properties and configuration for any connector).

Impact

So it ended up managing event-listener.properties as a secret instead of a configmap when deploying to k8s, which could work but bring addtional effort.

Expectation

It would be great to be able to use the syntax ${ENV:VARIABLE} for event-listener.properties.

Diagnose

I discovered that it doesn't replace any environment variables when loading the event-listener.properties:

https://github.com/trinodb/trino/blob/760fce85ef1a518e31696d930f7231ff3977ffba/core/trino-main/src/main/java/io/trino/eventlistener/EventListenerManager.java#L124-L132

It would be good to use method io.airlift.boostrap.Bootstrap.replaceEnvironmentVariables():

https://github.com/airlift/airlift/blob/3546f4b12884828372d30a8ae821327ad87441e8/bootstrap/src/main/java/io/airlift/bootstrap/Bootstrap.java#L198

[findinpath]

I've analyzed what is happening behind the scenes when referencing an environment property within a .properties file corresponding to a catalog. While creating the connector in https://github.com/trinodb/trino/blob/master/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnectorFactory.java#L80-L84 an airlift method call will be made where the environment properties are being substituted.

When loading the event listeners however, there is no substitution of the environment variables taking place:

https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/server/Server.java#L134

In this context, i find it reasonable to upgrade the visibility of the method io.airlift.bootstrap.Bootstrap#replaceEnvironmentVariables to public (from package-private). @electrum (since you implemented the functionality in airlift library) do you see anything against changing the visibility of the above mentioned method and adjusting correspondingly to the docs the functionality of the method io.trino.eventlistener.EventListenerManager#loadEventListenerProperties ?

[electrum]

This same issue exists for connectors and all other plugin services.

The secrets documentation shows an example for a connector like the MySQL connector. It works because the connector itself uses Bootstrap which does the replacement.

All of the plugins that we ship use Bootstrap, so it works as expected from an end user perspective. However, if you write your own plugin and don’t use Bootstrap, it won’t work.

It seems reasonable to move this into the engine so it works consistently, regardless of how the plugin is implemented.

[elferherrera]

Is there a way to bypass this issue?

[findepi]

@elferherrera currently, it works as designed -- it's a plugin responsibility. A plugin should use io.airlift.configuration.ConfigurationUtils#replaceEnvironmentVariables or equivalent

We keep this issue open because of this further improvement idea:

It seems reasonable to move this into the engine so it works consistently, regardless of how the plugin is implemented.

[elferherrera]

I see how it should be a plugin responsibility to implement the same configuration utils. However, I think that it should be mentioned in the secrets page that some plugins may not follow this approach. It took some time to discover that the plugin wasn't reading the password because of this reason and it also makes it more troublesome to share passwords with the plugin. We will be monitoring when this functionality gets moved into the engine.

Thanks a lot for the reply and for Trino.

[findepi]

I think that it should be mentioned in the secrets page that some plugins may not follow this approach

The docs are primarily for Trino users and cover Trino provided plugins, and I think these work "correctly". Maybe this is something to be mentioned in the developer section of the docs.

cc @mosabua @bitsondatadev @nineinchnick for docs

Anyway, i think we should move the responsibility to the engine for these placeholders.

[nineinchnick]

We do mention it now in the developer guide: https://trino.io/docs/current/develop/connectors.html#configuration

[mosabua]

Can we somehow make sure this works in all properties files loaded by a Trino plugin (connector or not..)

[martint]

Not consistently. Whether a connector uses properties files, yaml, xml or anything else is an implementation detail of the connector that the engine knows nothing about.

[findepi]

@mosabua we come to concusion it would be better to handle this on the server side (not leave up to connector to be or not be consistent), so that ENV placeholders is really Trino server's feature., cc @electrum However, this requires a code change in all the places where these are loaded.

[mosabua]

Yeah @findepi .. I was implying that there is some properties loading tool in core or even in airlift that is used everywhere and includes the env variable resolving .. this would also be good for any other improvements of that loading over time (such as validation for invalid properties)

[findepi]

I was implying that there is some properties loading tool in core or even in airlift

yes

and includes the env variable resolving

no, these two are separate pieces of code

[mosabua]

but they could/should be pulled together?

[kirkhansen]

It wasn't clear to me when creating a custom group provider that needed some secrets in the group-provider.properties file that I would have to implement my own version of environment variable substitution.

It'd be nice if the config passed to my plugin had already resolved the env vars.

I could also see exposing Trino's way of resolving environment variables that I could call into from my plugin or at least documenting a good/common way that plugins can read a *.properties file and resolve the env vars in the same way Trino is doing it.

I'm quite new/green to Java, but I would never have never found io.airlift.configuration.ConfigurationUtils#replaceEnvironmentVariables without this issue. At the very least, we could add a note to the docs to mention that env vars will not be substituted.

[kar0t]

In case of group provider, you can refer FileGroupProviderFactory which substitute environment variables correctly.

    @Override
    public GroupProvider create(Map<String, String> config)
    {
        Bootstrap app = new Bootstrap(
                binder -> {
                    configBinder(binder).bindConfig(FileGroupConfig.class);
                    binder.bind(FileGroupProvider.class).in(Scopes.SINGLETON);
                });

        Injector injector = app
                .doNotInitializeLogging()
                .setRequiredConfigurationProperties(config)
                .initialize();

        return injector.getInstance(FileGroupProvider.class);
    }