Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to runtime configuration YAML #431

Closed
6 of 7 tasks
tomkerkhove opened this issue Mar 25, 2019 · 16 comments · Fixed by #690
Closed
6 of 7 tasks

Switch to runtime configuration YAML #431

tomkerkhove opened this issue Mar 25, 2019 · 16 comments · Fixed by #690
Assignees
Labels
configuration All issues related to configuration discussion runtime All issues related to our runtime specs-defined All issues where the specifications are defined and ready to be implemented
Milestone

Comments

@tomkerkhove
Copy link
Owner

tomkerkhove commented Mar 25, 2019

As of today, all the runtime configuration is done via environment variables which is fine if you have a few of them but it feels like the list is becoming too long.

We should take a step back and see if we should go to a configuration model where we define them in YAML as well and load them at runtime, similar to how Prometheus and other tools approach this.

Benefits:

  • Everything in one file which can be loaded via a volume (Docker) or config map (Kubernetes)
  • Configuration can be added to source control
  • Easier to read and get an idea what it is configured like
  • Centralized configuration

Downsides:

  • Harder to get started as you need to know how volumes/config maps work
    • I think we can provide the required guidance around this and/or provide default config, either in the container or docs, to use
  • Existing customers need to migrate to the new model

The (de)serialization should use the out-of-the-box capability, and not built our own as we do for the metrics, given it's pretty straight forward.

Checklist

  • Move all configuration to the structure below, except for auth information
  • Update all documentation on this and use an example. Location should be /config/runtime.yaml
  • Provide unit testing, where needed.
  • Update Docker Hub bot.
  • Ensure optional variables are working fine
  • Update Helm Chart
  • Change landing page to point to v1.0

Configuration Example

This configuration should include everything from our configuration docs except for authentication config which would still be via environment variables.

server:
  httpPort: 80
prometheus:
  scrapeEndpoint:
    baseUriPath: /prometheus/scrape
metricsConfiguration:
  absolutePath: /config/metrics-declaration.yaml
logging:
  applicationInsights:
    instrumentationKey: ABC
    isEnabled: true
  containerLogs:
    isEnabled: true
    verbosity: warning
featureFlags:
  metrics:
    disableTimestamps: true # false by default
@tomkerkhove tomkerkhove added configuration All issues related to configuration specs-required All issues where the specifications are still being defined and implementation should be halted discussion runtime All issues related to our runtime labels Mar 25, 2019
@tomkerkhove tomkerkhove added this to the v1.0.0 milestone Mar 25, 2019
@tomkerkhove
Copy link
Owner Author

Adding @samneirinck as he might have some good feedback/insights as well

@tomkerkhove tomkerkhove self-assigned this Mar 26, 2019
@tomkerkhove tomkerkhove changed the title Rethink our current runtime configuration approach Switch to runtime configuration YAML Mar 27, 2019
@tomkerkhove
Copy link
Owner Author

The above YAML is the way to go forward with v1.0.0.

@tomkerkhove
Copy link
Owner Author

@jsturtevant @brusMX This should be ready to be implemented and use the YAML mentioned above using the out-of-the-box deserialization.

Feel free to take this up this week, but it's not a must

@tomkerkhove tomkerkhove added specs-defined All issues where the specifications are defined and ready to be implemented and removed specs-required All issues where the specifications are still being defined and implementation should be halted labels Mar 27, 2019
@samneirinck
Copy link
Contributor

What will be the plan to support deploying to multiple environments? Is the way forward to create 1 file per environment, or still have environment variables to be able to override certain parts? Or have some kind of placeholder support in this yaml file?

@tomkerkhove
Copy link
Owner Author

The environment variables will go away but if you're using the helm chart you should be able to let that create the file for you if you want and use different parameters based on the env. Makes sense?

@samneirinck
Copy link
Contributor

Makes sense, but makes it a bit harder to use without helm.

@tomkerkhove
Copy link
Owner Author

I think it should be ok, which configurations do you think will always defer depending on the environment?

@samneirinck
Copy link
Contributor

AI instrumentation key mainly

@tomkerkhove
Copy link
Owner Author

I get your point. Would it make sense if this is moved to environment variable but still ahs to be enabled here?

Plan is to add more sinks later on which are configured here but instrumentation key is a secret so should not be here indeed.

@tomkerkhove tomkerkhove added the seeking-customer-input All issues where we are seeking input from customers label Jun 8, 2019
@tomkerkhove
Copy link
Owner Author

@datadot @ResDiaryLewis @adam-resdiary @marcinbudny @barakAtSoluto @jdevalk2 Feel free to have a look at this issue and provide feedback.

Personally, I think this is the best approach to go forward. If you are using Helm chart for deployment, this should be handled for you.

@ResDiaryLewis
Copy link

Hey Tom, this is definitely preferable for us 🙆‍♂. It's just a bit easier to view the config file than to print out each environment variable, it's also easier for us to just plug the YAML file in from our Helm values file directly.

@jdevalk2
Copy link

jdevalk2 commented Jun 10, 2019

I like the idea of the config file but consider being able to override every setting with an environment variable as well. Example: How Grafana does things.

Ex.

[server]
name=bla

Can be overridden with environment variable GF_SERVER_NAME=bla2

Also consider not making the config file more than 2 levels deep, and prefer 1 level deep, keeping things at the root. I say this because tools like Helm mainly do not allow deep-merge. You can see this in prometheus operator chart where if you want to override a setting 3 levels deep you have to specify the entire subtree from the root downwards.

Also consideration is to be able to mount passwords on filesystem. If there are any passwords in the configuration then ideally those can be mounted through a mounted file secret (instead of over an environment variable). I.e. the secret is mounted on a folder and is contained in a file in that folder. The idea here is that an environment / environment variables are relatively easy to dump when software get compromised and if that dump contains your database credentials that could be used to escalate any attack that was going on.

@datadot
Copy link

datadot commented Jun 17, 2019

@tomkerkhove sounds reasonable to me.
I'd agree with @jdevalk2 idea too with the environment variable override, however feel this could be a stretch goal for after v1.0.0.

@tomkerkhove
Copy link
Owner Author

I think we have a consensus that it's a good approach but overriding via environment variables are a good idea. Thanks for the input folks!

Overriding seems to be more of enhancement rather than a requirement for v1.0 so I tend to agree with @datadot. This improvement is tracked in #594.

Also consider not making the config file more than 2 levels deep, and prefer 1 level deep, keeping things at the root. I say this because tools like Helm mainly do not allow deep-merge. You can see this in prometheus operator chart where if you want to override a setting 3 levels deep you have to specify the entire subtree from the root downwards.

That's good feedback, thanks - I was not aware of that!

Also consideration is to be able to mount passwords on filesystem. If there are any passwords in the configuration then ideally those can be mounted through a mounted file secret (instead of over an environment variable). I.e. the secret is mounted on a folder and is contained in a file in that folder. The idea here is that an environment / environment variables are relatively easy to dump when software get compromised and if that dump contains your database credentials that could be used to escalate any attack that was going on.

Another good point, tracking this in #593

@tomkerkhove
Copy link
Owner Author

This should be done, only have to change docs to new version when shipping v1.0

@tomkerkhove
Copy link
Owner Author

PR for docs is waiting to be merged!

Closing this one already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration All issues related to configuration discussion runtime All issues related to our runtime specs-defined All issues where the specifications are defined and ready to be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants