csaf-poc / csaf_distribution

Tools to download or provide CSAF (Common Security Advisory Framework) documents.
https://csaf.io
38 stars 22 forks source link

Improve logging for `csaf_aggregator` when no config file is present #538

Open lebogg opened 4 months ago

lebogg commented 4 months ago

What happened?

I tried to test the csaf_aggregator by building and just running it without any parameter, in particular without a path to a config file. Then, of course, the aggregator was looking into the default paths but couldn't find any config file. I got the following error message: error: no providers given in configuration This is due a check in the prepare function of config.go where the providers of the passed config is checked: https://github.com/csaf-poc/csaf_distribution/blob/086c4ab48b292b05b2382bf7b26d99ed8b346a0d/cmd/csaf_aggregator/config.go#L456-L458

What would I expect?

For me, the error code is misleading since it suggests that just the providers are missing in the config file, but the whole config file is missing in the first place. Therefore, having, e.g., a typo in the passed path to the config file lets the user in the unknown that the path or the file was actually wrong. I would expect an error or additional warning message indicating sth. like

WARN: config file is not parsed: there is no config file in the provided or default paths
ERROR: no providers given in configuration

Suggestion

A simple solution would be to print a warning log message when the config file couldn't be found in the generic Parse() function if it doesn't clash with the usage in other places (it is still just a warning message though). https://github.com/csaf-poc/csaf_distribution/blob/7a8cdb6d19271ac1b9d5a0a93891056a9f3994cc/internal/options/options.go#L47

For example:

@@ -12,6 +12,7 @@ package options
 import (
        "fmt"
        "log"
+       "log/slog"
        "os"

        "github.com/BurntSushi/toml"
@@ -81,6 +82,7 @@ func (p *Parser[C]) Parse() ([]string, *C, error) {

        // No config file -> We are good.
        if path == "" {
+               slog.Warn("No config file found. Maybe you want to specify one or store it in a respective default location")
                return args, &cmdLineOpts, nil
        }
bernhardreiter commented 4 months ago

Hi @lebogg thanks for your suggestion and for testing the aggregator!

If we can improve the messaging here, that looks like a good thing, we'd accept a pull request for this.

Note that in many cases it is fine to have no config file and just use the defaults. However for the aggregator that maybe different from the general case.

What about logging the tried config file path in general. And possible add a hint to the aggregator error message to also check the place of the configuration file.