[pillar] Revamp pillar file structure & allow arbitrary keys in datadog.yaml #35
Conversation
…og.yaml Separates the root pillar object 'datadog' into two sub-objects: - "config", which describes the contents of the datadog.yaml config file that will be installed on the minions, - "install_settings", which contains various customizable parameters for the Agent install, such as the Agent version. With this new structure, any arbitrary key can be put in "config", and will be reflected in the minions' config files.
KSerrania
force-pushed
the
kserrania/revamp-pillar
branch
from
3fbd43b to
be3339c
Compare
| 'install_settings': { | ||
| 'agent_version': 'latest', | ||
| 'pkg_name': 'datadog-agent', | ||
| 'service_name': 'datadog-agent', |
There was a problem hiding this comment.
Does it make sense for pkg_name and service_name to be overridable by the user? I would remove them from here and just hardcode them (otherwise, we should document them?)
There was a problem hiding this comment.
They were here when I started maintaining the repo, but I agree that they should be hardcoded. But I didn't want to mix the cleanup of the repo with this change ; I'll do the cleanup in another PR
| {%- endif %} | ||
| {% endif %} | ||
|
|
||
| {% if 'checks_confd' not in datadog_settings %} | ||
| {% if 'checks_confd' not in datadog_install_settings %} |
There was a problem hiding this comment.
For config_folder and config_file I think the same as above: I don't see why the user would need to override them.
For checks_confd, this field overlaps with confd_path in datadog_config (which is read by the agent), so I wonder if we should merge them (otherwise, the user needs to change both)
There was a problem hiding this comment.
Same as above: I'll get rid of config_folder and config_file in a separate cleanup PR (they should not be used: if you use them, SaltStack will put the datadog.yaml file there, and the Agent will never find it, failing Agent starts).
For checks_confd, I have another planned change (after this PR is done) to make it use confd_path
KSerrania
force-pushed
the
kserrania/revamp-pillar
branch
5 times, most recently
from
6be58c7 to
9217690
Compare
KSerrania
force-pushed
the
kserrania/revamp-pillar
branch
from
9217690 to
2102ff4
Compare
| {% set config_file_path = '%s/%s'|format(datadog_install_settings.config_folder, datadog_install_settings.config_file) -%} | ||
|
|
||
| {%- if not latest_agent_version and parsed_version[1] == '5' %} |
There was a problem hiding this comment.
Shouldn't this be parsed_version[0]?
There was a problem hiding this comment.
parsed_version[0] contains the full match of the regex defined here: https://github.com/DataDog/datadog-formula/pull/35/files#diff-541c639658b352ec59d46a0868aa02d2R35
parsed_version[1] contains the first capture group, which is the major version number
| @@ -0,0 +1,10 @@ | |||
| {% from "datadog/map.jinja" import datadog_config with context -%} | |||
|
|
|||
| [Main] | |||
There was a problem hiding this comment.
How was agent 5 working before without this file? :o
There was a problem hiding this comment.
We took the example file that the package provides (or the datadog.conf file directly if it exists on the host already), and replaced the fields we wanted to change using regexes. But this is error-prone, we relied on fields being present on the config file (which may not be the case if it was modified by the user)
What does this PR do?
Separates the root pillar object
datadoginto two sub-objects:config, which describes the contents of thedatadog.yamlconfig file that will be installed on the minions,install_settings, which contains various customisable parameters for the Agent install, such as the Agent version.With this new structure, any arbitrary key can be put in
config.For now, what's in config is simply written in the
datadog.yamlfile, but making modifications before writing the file could also be possible in the future (if we want to check fields, for example).Note: Agent 5 is using a .ini config file, and there's no ini renderer available easily, so we're using the old way with explicitly supported options (except a little cleaner: we're not matching regexes in a file we don't have control of). To comply with what was previously supported on Agent 5 on SaltStack, the
api_keyoption is supported for now.Motivation
Right now, to add a new option to the config, we need to explicitly add it to the
datadog/config.slsstate file to support it. Moreover, the way we're doing it is not satisfying: we take the file that's present on the minion (which can possibly be anything), and we match regexes to find the option and replace the corresponding line in the file (which can fail if the user manually changed the file).Additional Notes
This is a breaking change, as the current pillar structure will not work anymore.