Retention Count config uint64 overflow

[yhabteab]

Describe the bug

The config.Retention.Count is user configurable via the YAML file and thus should not be of type uint64, otherwise it can easily be overflowed.

can't parse YAML file config.example.yml: cannot unmarshal -10 into Go value of type uint64 ( overflow )
exit status 1

[julianbrost]

Note that it's not that uint64 in inherently wrong here, it's just that the YAML parser we use seems to return a more helpful error message with mismatching types (parsing string into int) than with integer values out out range:

$ go run ./cmd/icingadb --config <(echo 'retention: {count: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint64 ( overflow )
exit status 1
$ go run ./cmd/icingadb --config <(echo 'retention: {count: "str"}')
can't parse YAML file /proc/self/fd/11: [1:20] cannot unmarshal string into Go struct field Config.Retention of type uint64
>  1 | retention: {count: "str"}
                          ^

exit status 1

Using a signed int could be a workaround to provide a more helpful error message, at least in some more common error cases (something like trying -1 to disable something).

[oxzi]

This issue is not restricted to the Count struct field, but to every uint64 configuration field.

$ ./icingadb --config <(echo 'retention: {history-days: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint64 ( overflow )
$ ./icingadb --config <(echo 'retention: {sla-days: -1}')
can't parse YAML file /proc/self/fd/11: cannot unmarshal -1 into Go value of type uint64 ( overflow )

A quick fix could be to change the type to int64 and add a greater-zero check in Validate(), but this feels a bit like throwing the type system under the bus. Thus, as @julianbrost wrote, I would say that this is something to be fixed in the YAML library.

[oxzi]

This would be a quick fix addressing this goccy/go-yaml#470.

[oxzi]

The mentioned PR got merged and a new go-yaml release was drafted, #832. In the meantime, the retention size was also modified in #824. @yhabteab, would you please take another look at this issue?