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

Support loading configuration from both YAML files and env vars #831

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lippserd
Copy link
Member

@lippserd lippserd commented Oct 26, 2024

I will keep the original PR description as a reference, but the implementation now uses config.Load() from our Go libs.

With this PR Icinga DB supports configuration loading in three scenarios:

  1. Load configuration solely from YAML files when no relevant environment variables are set.
  2. Combine YAML file and environment variable configurations, allowing environment variables to supplement or override possible incomplete YAML configurations.
  3. Load entirely from environment variables if the default YAML config file is absent and no specific config path is provided by the user.

config.FromYAMLFile() is still called first but continuation with config.FromEnv() is allowed by handling:

  • ErrInvalidConfiguration: The configuration may be incomplete and will be revalidated in config.FromEnv().
  • Non-existent file errors: If no explicit config path is set, fallback to environment variables is allowed.

config.FromEnv() is called regardless of the outcome from config.FromYAMLFile(). If no environment variables are set, configuration relies entirely on YAML. Otherwise, environment variables can supplement, override YAML settings, or serve as the sole source. config.FromEnv() also includes validation, ensuring completeness after considering both sources.

Possible alternative implementations:

  • If the config path is the default, os.Stat() could be used before calling config.FromYAMLFile(), rather than handling non-existent file errors. This approach would split the logic into two sections and add an additional if block.
  • Don't call Validate() in the config package automatically. Instead, require it to be called manually. I appreciate that library-wise, both config.FromYAMLFile() and config.FromEnv() include validation allowing them to be used without needing an additional function call on their own. When combining them, I think it's straightforward to use errors.Is() to check for ErrInvalidConfiguration, i.e. errors from Validate().

Requires:

closes #756

@cla-bot cla-bot bot added the cla/signed label Oct 26, 2024
@lippserd lippserd marked this pull request as ready for review October 28, 2024 11:52
@lippserd lippserd force-pushed the config-from-environment-variables branch 2 times, most recently from 65232f7 to f73a444 Compare October 31, 2024 10:58
@lippserd lippserd added this to the 1.3.0 milestone Dec 5, 2024
@lippserd lippserd force-pushed the config-from-environment-variables branch from f73a444 to 614d015 Compare January 14, 2025 11:20
@lippserd lippserd requested a review from oxzi January 15, 2025 05:52
@lippserd lippserd force-pushed the config-from-environment-variables branch from 614d015 to 8f5e40e Compare January 15, 2025 07:33
@lippserd lippserd force-pushed the config-from-environment-variables branch from 8f5e40e to b18e594 Compare January 15, 2025 15:11
@lippserd lippserd requested a review from oxzi January 15, 2025 15:11
@oxzi oxzi modified the milestones: 1.3.0, 1.2.2 Mar 5, 2025
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having documentation (like human readable docs) for this PR would be nice. Even just stating that this is possible in the 03-Configuration section.

@lippserd lippserd force-pushed the config-from-environment-variables branch from b18e594 to 11d7698 Compare March 5, 2025 12:59
oxzi added a commit that referenced this pull request Mar 5, 2025
New ways to configure Icinga DB were introduced in [icingadb-831] and
[igl-113]. The first change allows configuring Icinga DB using
environment variables instead of or next to the YAML configuration file.
In addition, the second change allows setting certificates and keys as
PEM-encoded strings next to referencing files. This was now documented.

As a structural change, the order of the Database and Redis sections
were changed to reflect the order in the example configuration file.

Some words about the Logging Components were written, as the
documentation previously that they exist.

[icingadb-831]: #831
[igl-113]: Icinga/icinga-go-library#113
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have taken another look at why the CI fails and after some digging, it became quite obvious. The Icinga DB container bailed out as it cannot parse its config.

can't parse YAML file /icingadb.yml: does not implemented Unmarshaler

However, the generated config looked fine.

database:
  type: pgsql
  host: 172.20.0.4
  port: 5432
  database: d1
  user: u1
  password: 7f96cc1ac9d3ad03
redis:
  host: 172.20.0.2
  port: 6379
logging:
  level: debug
  interval: 1s
retention:
  history-days: 7
  sla-days: 30
  options:
    acknowledgement: 0
    comment: 1
    downtime: 2

So I went down the YAML parsing and ended up at a faulty YAML parsing for the RetentionOptions.

Turns out, the custom encoding.TextUnmarshaler is not enough on its own, but a yaml.InterfaceUnmarshaleris also required for custom types.

I pushed a hopefully fix to the just created config-from-environment-variables-fix-alvar branch in fe94784. At least on my machine all tests went through.
Edit: Pushed commit in this PR's branch after talking about with @lippserd and deleted my other branch.

It would be good to have unit test for this, similar to RetentionOptions to catch this earlier on.

@lippserd
Copy link
Member Author

lippserd commented Mar 17, 2025

@oxzi Thanks for the correction. We already had a similar issue in our Go library and I'll link it here for reference:

Icinga/icinga-go-library#84

From my point of view, the changes from the PR can be merged now.

Edit:

I'll push another set of tests:

It would be good to have unit test for this, similar to RetentionOptions to catch this earlier on.

lippserd and others added 6 commits March 17, 2025 09:32
For YAML decoding, the yaml.InterfaceUnmarshaler interface needs to be
implemented. To not mix value and pointer receiving methods for
RetentionOptions, the signature of Validate was updated.

It is important to set the internally type to map[string]uint16 instead
of RetentionOptions, as otherwise the method would be called in an
endless loop until the stack is full.
@lippserd lippserd force-pushed the config-from-environment-variables branch from 0572d6d to d9fdf99 Compare March 17, 2025 08:42
oxzi added a commit that referenced this pull request Mar 17, 2025
New ways to configure Icinga DB were introduced in [icingadb-831] and
[igl-113]. The first change allows configuring Icinga DB using
environment variables instead of or next to the YAML configuration file.
In addition, the second change allows setting certificates and keys as
PEM-encoded strings next to referencing files. This was now documented.

As a structural change, the order of the Database and Redis sections
were changed to reflect the order in the example configuration file.

Some words about the Logging Components were written, as the
documentation previously that they exist.

[icingadb-831]: #831
[igl-113]: Icinga/icinga-go-library#113
oxzi added a commit that referenced this pull request Mar 17, 2025
New ways to configure Icinga DB were introduced in [icingadb-831] and
[igl-113]. The first change allows configuring Icinga DB using
environment variables instead of or next to the YAML configuration file.
In addition, the second change allows setting certificates and keys as
PEM-encoded strings next to referencing files. This was now documented.

As a structural change, the order of the Database and Redis sections
were changed to reflect the order in the example configuration file.

Some words about the Logging Components were written, as the
documentation previously that they exist.

[icingadb-831]: #831
[igl-113]: Icinga/icinga-go-library#113
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

I have just canceled the docker CI job since it seemed stalled since over two hours and reran it. Unless it fails, feel free to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants