trino: syntax ${ENV:VARIABLE} doesn't work in event-listener.properties
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:
It would be good to use method io.airlift.boostrap.Bootstrap.replaceEnvironmentVariables():
About this issue
- Original URL
- State: open
- Created 3 years ago
- Reactions: 2
- Comments: 15 (11 by maintainers)
We do mention it now in the developer guide: https://trino.io/docs/current/develop/connectors.html#configuration
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.
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#replaceEnvironmentVariablesto 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 methodio.trino.eventlistener.EventListenerManager#loadEventListenerProperties?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.