Support providing configuration from YAML files #123
Conversation
proxy/pkg/config/config.go
Outdated
| defer file.Close() | ||
|
|
||
| dec := yaml.NewDecoder(file) | ||
| if err = dec.Decode(c); err != nil { |
There was a problem hiding this comment.
what happens if the same config settings are defined in multiple files? As it stands it seems that they will be overwritten which is not a very clear behavior. We should probably return an error and fail the launch if that happens.
There was a problem hiding this comment.
It was intentional to allow to override. At the end we already print all applied configuration variables, so there should be no confusion.
There was a problem hiding this comment.
I heavily disagree with this, I don't see any benefit to it and it might just lead to user error, users never check the configuration that is printed in the logs, it's mostly for us devs to troubleshoot issues.
There was a problem hiding this comment.
I hadn't thought about the possibility of the same value being set in multiple files, but it is a good point and definitely a risk.
I would see the value in supporting multiple files so that users can have one with stricter governance and another with easier access. For example, they may want to have one file containing credentials / SCB path / contact points, with restricted access, and another with things like read mode, primary cluster, log level etc etc which can be more easily accessed.
This could also be managed by having clearly structured and named files, from which we extract only the expected properties. This would not leave it up to the user to define any property in any file, and would ensure that each property is defined and considered exactly once. The downside is that we need to parse each file specifically for the expected content, but the configuration properties themselves do not tend to change, so I wouldn't see it as a big problem.
There was a problem hiding this comment.
I wonder if we are overengeneering this a bit... In general most tools just allow you to provide a config file and that's it. In the case of DSE, DSE allows you to set a config setting that is a path to a key file so that DSE can decrypt the credentials.
Can we take a step back and actually think if we need multiple config files? I feel like the DSE approach is a simple one but effective no?
lukasz-antoniak
requested review from
grighetto,
absurdfarce and
weideng1
as code owners
There was a problem hiding this comment.
Looks good, added a few comments.
We should add a "reference" yaml file so that we can point users when they ask about its structure. It wouldn't be used by the proxy like the java driver does it but it would sit on the repo so that we can use it for documentation and example purposes. It should contain all settings (except the performance tuning ones) including the default values (or commented if they don't have a default value)
docs/assets/zdm-config-reference.yml
Outdated
|
|
||
| # Control connection failure threshold. If threshold is exceeded, | ||
| # readiness probe of ZDM will report failure and pod will be recreated. | ||
| heartbeat_failure_threshold: 1 |
There was a problem hiding this comment.
Maybe we can comment out all of the optional settings (but have the commented line here anyway just so that it is easy for someone to uncomment and set a specific value).
The required settings should be left uncommented like they are now 👍
There was a problem hiding this comment.
I left all required and sensitive properties uncommented, but commented majority.
docs/assets/zdm-config-reference.yml
Outdated
| # origin_tls_client_key_path: | ||
|
|
||
| # Comma separated ist of target cluster contact points. | ||
| target_contact_points: target.datastax.com |
There was a problem hiding this comment.
maybe 127.0.0.2 as target and 127.0.0.1 as origin? feels a bit weird to see a valid hostname here (even if it doesn't exist, at least I don't think it does? but the datastax.com domain definitely exists 😅 )
There was a problem hiding this comment.
Good point, fixed.
No description provided.