Icinga DB: don't write negative Downtime durations into Redis

[Al2Klimov]

via std::max(0, x) not to crash the Go daemon which can't handle such.

fixes #9774

Test cases (which don't crash anymore now)

• curl -fvku root:123456 -X POST -H 'Accept: application/json' 'https://127.0.0.1:5665/v1/actions/schedule-downtime?type=Host&filter=true&start_time=14000&end_time=2000000000&author=ak&comment=-&fixed=0&duration=-14'
• curl -fvku root:123456 -X POST -H 'Accept: application/json' 'https://127.0.0.1:5665/v1/actions/schedule-downtime?type=Host&filter=true&start_time=2010000000&end_time=2000000000&author=ak&comment=-'

[julianbrost]

What about any kind of input validation for new downtimes? end_time <= start_time and fixed && duration <= 0 is guaranteed to result in broken/useless downtimes, so it doesn't make sense to accept that.

[Al2Klimov]

Sure. I have this on my to do list. But this PR doesn’t depend on that, does it?

[julianbrost]

This PR on its own doesn't make clear what exactly your plan is. Also you said nothing about why you're changing this in the Icinga DB specific code instead of earlier in the downtime code so that no other code has to have special cases for strange values. Is there something you expect to work better this way for later changes you're planning?

[Al2Klimov]

This code on its own just behaves like the other stuff which mitigates Icinga DB crashes.

Regarding preventing bad user behaviour in the future: the newly discovered existing ability to create end < start changes the whole picture of what's necessary for this (and for what you suggested). Whatever my plan was, it's now in the trash bin.

[julianbrost]

Sure. I have this on my to do list.

Nobody besides you know that list, so please share what you're planning so that others can follow. This PR started as an alternative to #9773, so it would be much more helpful if the description included a summary what is different, what's in here and what isn't and what the plan is for things that aren't in here.

But this PR doesn’t depend on that, does it?

When following #9774 to the word, yes, this PR probably fixes it. But #9773 started with some better validation which would still be useful, but probably in a more compatible way. If you want to treat that separately, please create an issue for this so that we don't forget it.

[julianbrost]

As the return value of GetDuration() isn't restricted to non-negative values, the following computation could result in a negative value (the same is also done in other places in that file):

icinga2/lib/icingadb/icingadb-objects.cpp

Line 1432 in c3ff2fb

attributes->Set("end_time", TimestampToMilliseconds(downtime->GetFixed() ? downtime->GetEndTime() : (downtime->GetTriggerTime() + downtime->GetDuration())));

In Go, the end time is parsed into a float64 and converted to a time.Time, both of which can handle pre-1970 values (i.e. negative Unix timestamps), the database columns are unsigned though.

However, I wasn't able to actually trigger that bug due to #9726 immediately removing the downtime when it triggers (as it's ending before its trigger time) so it's gone again before it makes its way to Icinga DB. Not sure if that's guaranteed though, maybe with enough load the timer might be delayed for long enough so that it's actually written to Redis and it might be a good idea to add a safeguard for this nonetheless.