fastly / sigsci-splunk-app

Splunk app for Fastly (Signal Sciences)
https://splunkbase.splunk.com/app/3495
MIT License
9 stars 10 forks source link

Timestamp sanitisation when tztime is outside UTC can be erroneously generated #37

Closed jeremy-cxf closed 8 months ago

jeremy-cxf commented 9 months ago

In the sigsci_helper time stamp sanitisation process:

def timestamp_sanitise(_time):
    new_time = datetime.utcfromtimestamp(_time).replace(second=0)
    new_time = int(new_time.timestamp())
    return new_time

It is being passing a UTC (aware) time already:

def get_from_and_until_times(delta, five_min_offset=False):
    if five_min_offset:
        until_time = datetime.now(timezone.utc) - timedelta(minutes=5)
    else:
        until_time = datetime.now(timezone.utc)
    from_time = until_time - timedelta(seconds=delta)

    if five_min_offset:
        until_time = until_time.replace(second=0)
        from_time = from_time.replace(second=0)

    until_time = int(until_time.timestamp())
    from_time = int(from_time.timestamp())

    return timestamp_sanitise(until_time), timestamp_sanitise(from_time)

This is fine if the server is in UTC. This is because, when you take a timestamp representing a UTC time and use utcfromtimestamp on it, then derive a timestamp back, you are effectively shifting the time based on the local timezone.

So, when we take the datetime object and convert it back to a timestamp using timestamp(), we are essentially getting the timestamp of 7:00 PM UTC. When this timestamp is treated as an MST time, it would correspond to 12:00 PM MST (noon) + 7 hours = 7:00 PM MST, which is 7 hours into the future.

For example:

Time now: 2023-10-04 06:31:27.191104 MST
=====
From time:1696450860
Until Time:1696451160
https://dashboard.signalsciences.net/api/v0/corps/<corp>/sites/<site>/feed/requests?from=1696450860&until=1696451160
400
{"message":"The from and until timestamps cannot be in the future - see https://docs.signalsciences.net/developer/extract-your-data/"}

1696450860 will convert to Wednesday October 04, 2023 20:21:00 (pm)

The fix for this would not bother converting the timestamp we already generated, as we have to convert it into a datetime object again (and in this process currently, making it naieve).

A fix for this would just be to replace replace=0 operation by truncating the timestamp using arithmetic instead. Thus avoiding handling another datetime object.

e.g:

def timestamp_sanitise(_time):
    return _time - _time % 60
jeremy-cxf commented 9 months ago

It makes more sense to just use epoch timestamps (time.time) across the board here. This should just ideally be refactored to use that instead and not rely on dt at all.