Add support for templating in configs #6251
Conversation
Amongst other use cases, this avoids the need to use dockerize in container deployments, making it much easier to build more secure images which do not need write access to the filesystem.
|
If preferred to avoid the slight risk we can add a flag that enables templating. |
There was a problem hiding this comment.
I wonder how many people use the templates outside of our docker compose setup to justify putting this dependency and additional functionality in the code.
Also wondering if this would break if users store templates in their config files, not sure there's a use case for that.
|
I misremembered during our call. Our kubernetes (including Helm) installs also need this functionality, its required to make use of passwords stored in kubernetes secrets. |
There was a problem hiding this comment.
Considering we need this templating in all of the container based deployment setups, and this allows images to be immutable, I'm fine with this approach.
I think server configs might need an overhaul though, ideally we wouldn't require multiple files and can simplify some of the structure. That's a bigger effort and we'd need to think about it some more.
common/config/loader.go
Outdated
| for _, f := range files { | ||
| // This is tagged nosec because the file names being read are for config files that are not user supplied | ||
| // #nosec | ||
| data, err := os.ReadFile(f) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = yaml.Unmarshal(data, config) | ||
|
|
||
| tpl, err := template.New("config").Funcs(templateFuncs).Parse(string(data)) |
There was a problem hiding this comment.
is this compatible with the existing config_template.yaml? it looks like sprig provides a function used like env "VAR" but the template is written with things like .Env.VAR. do we have to migrate the template?
There was a problem hiding this comment.
The template would need adjusting. Sprig's default argument ordering is reversed from the dockerize version. There are probably a few other gotchas. I would suggest leaving the existing template as it is and creating a new template under config/docker.yaml with the updated syntax so that containers using dockerize will continue to work until we deprecate that. Newer containers can use this new functionality with the updated config file. I'll add a working config file to this PR.
Takes the opportunity to simplify the template a little and add some errors for common issues.
alexshtin
changed the title
common/config/loader.go
Outdated
| return err | ||
| } | ||
|
|
||
| err = yaml.Unmarshal(rendered.Bytes(), config) |
There was a problem hiding this comment.
It also would be very useful for debugging purpose to check what is in rendered. And not only in case of template error. Template syntax might be fine, but produce different output. Write it to stdout? I am ok, if you do it in separate PR.
There was a problem hiding this comment.
Agreed to put this into a tdbg command for now, and later move into the server binary once we're happy (due to backwards compat requirements).
Another simple approach would be to look for a string like
What multiple files do you mean? Only one static config file is required. Dynamic config requires a second file only if you want to change anything from defaults, but it would be simple to add a "static dynamic config" section in the static config file if you wanted to override some things but not actually have them be dynamic. |
There was a problem hiding this comment.
I'm fine with this. I don't like the extra dependency but if it lets us get rid of dockerize that's nice.
I have a little concern about the two copies of the config getting out of sync, but it's changed pretty rarely and we should be able to deprecate the other one soon, right?
Yeah, it's less about requiring multiple files but that multiple files are supported and there's a concept of an environment that's not ideal to expose in the interface IMHO. |
Oh, yeah, the environment stuff should go, agreed. Supporting embedding dynamic config in the static config file would be like 20 lines of code. It would be a pretty easy change to even make it dynamic too. Is there a proposal for what the specific UX should be? I can whip up something but maybe it's better to design it first. |
Off the top of my head, to improve DX, we need to accept a single file, drop the directory and environment concept and allow embedding dynamic config in this single file. We'd better design this though before making changes, this will require some thought to be done in a non disruptive way. |
|
I've added a comment that enables the templating, and a tdbg command to render the template (as YAML and Config{}) for debugging. |
| func newConfigCommands() []*cli.Command { | ||
| return []*cli.Command{ | ||
| { | ||
| Name: "render", |
There was a problem hiding this comment.
Why not just add a render-config command to the server binary itself? It's a trivial amount of code, and then you don't need to duplicate anything into tdbg (which is admittedly also trivial). As a side-effect it'll better replicate the file lookup that the server does, which can be confusing
There was a problem hiding this comment.
I agree about the file paths being a bonus, but also it seems helpful to be able to point the command at a config file which isn't (yet) in the right place, for testing. So I'd want to be able to over-ride the path if we did move this over to temporal.
There was a problem hiding this comment.
There should definitely be a way to point the server to a single file, the env/zone stuff is confusing. That's what #6561 is about.
It seems like it may not be that helpful, though, since the config will depend on a lot of env vars, so it'd be easiest to run it with docker or k8s anyway to get an interesting result. In that case the config will already be in the right place and the easiest thing would be to override the subcommand from start to render-config?
Co-authored-by: David Reiss <[email protected]>
What changed?
Support for text/template was added to the config loading system. Sprig is used for some useful templating functions, as used by dockerize and helm.
Why?
This avoids the need to use dockerize to render config templates when deploying in containers. Without the need to write the intermediate config to disk, the image can be immutable. Any volumes used to copy over an alternative template can be read-only also, enabling more secure deployment.
How did you test it?
Built a
FROM scratchimage using the updated temporal-server binary and confirmed that it would work when deployed using (slightly adjusted) version of our helm charts.Potential risks
There is a very slim chance that some configuration files might accidentally have some text that looks like go template, which the code would now try and interpret. This seems very unlikely.
Documentation
Documentation should be updated to explain this new feature, but given it's so close to the standard deployment setup with dockerize, this doesn't need to block release of the feature imho.
Is hotfix candidate?
No