diff --git a/datadog_sync/model/downtime_schedules.py b/datadog_sync/model/downtime_schedules.py index 63c6906e..3cc20e67 100644 --- a/datadog_sync/model/downtime_schedules.py +++ b/datadog_sync/model/downtime_schedules.py @@ -4,7 +4,7 @@ # Copyright 2019 Datadog, Inc. from __future__ import annotations from typing import TYPE_CHECKING, Optional, List, Dict, Tuple -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from dateutil.parser import parse from datadog_sync.utils.base_resource import BaseResource, ResourceConfig @@ -63,19 +63,48 @@ async def import_resource(self, _id: Optional[str] = None, resource: Optional[Di return str(resource["id"]), resource + @staticmethod + def _parse_utc(value): + """Parse an ISO timestamp and return a UTC-aware datetime. Naive input + is assumed UTC (the destination stores schedules in UTC).""" + parsed = parse(value) + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed + + @staticmethod + def _iso_utc(dt) -> str: + return dt.isoformat().replace("+00:00", "Z") + async def pre_resource_action_hook(self, _id, resource: Dict) -> None: if _id not in self.config.state.destination[self.resource_type]: schedule = resource["attributes"].get("schedule") - if schedule and "start" in schedule: - current_time = datetime.utcnow() - t = parse(schedule["start"]) - if t.timestamp() <= current_time.timestamp(): - current_time = current_time + timedelta(seconds=60) - if getattr(current_time, "tzinfo", None) is not None: - new_time = current_time.isoformat() - else: - new_time = "{}Z".format(current_time.strftime("%Y-%m-%dT%H:%M:%S.%f")[:-3]) - schedule["start"] = new_time + if not schedule: + return + now = datetime.now(timezone.utc) + + # Past `end` means the maintenance window has already closed on the + # source. Replicating it to the destination would either invent a + # new customer-visible maintenance (if we shifted `end` forward) or + # 400 with "Downtime cannot be scheduled in the past". Skip: an + # ended downtime has nothing left to silence. + end_raw = schedule.get("end") + if end_raw: + end_dt = self._parse_utc(end_raw) + if end_dt <= now: + raise SkipResource( + str(_id), self.resource_type, + "Downtime end is in the past.", + ) + + # Rewrite past `start` forward to now+60s. `end` (if present) is + # left as-is per customer intent — the window may shrink but its + # original end time is preserved. + start_raw = schedule.get("start") + if start_raw: + start_dt = self._parse_utc(start_raw) + if start_dt <= now: + schedule["start"] = self._iso_utc(now + timedelta(seconds=60)) else: # If start or end times of the resource are in the past, we set to the current destination `start` and `end` # this is to avoid unnecessary diff outputs diff --git a/tests/unit/test_downtime_schedules.py b/tests/unit/test_downtime_schedules.py new file mode 100644 index 00000000..ab593c1e --- /dev/null +++ b/tests/unit/test_downtime_schedules.py @@ -0,0 +1,187 @@ +# Unless explicitly stated otherwise all files in this repository are licensed +# under the 3-clause BSD style license (see LICENSE). +# This product includes software developed at Datadog (https://www.datadoghq.com/). +# Copyright 2019 Datadog, Inc. + +""" +Unit tests for downtime_schedules create-path schedule normalization. + +Prior behavior only rewrote past `schedule.start` forward. Downtimes with +a past `end` (one-off maintenance windows that already closed on the +source) still hit the destination API and 400'd with "Downtime cannot be +scheduled in the past". + +New behavior: +- Past `schedule.end` → SkipResource (ended downtimes are not replicated). +- Past `schedule.start` with future/absent `end` → bump `start` to now+60s + and leave `end` as-is (window may shrink, original end time preserved). +""" + +import asyncio +from datetime import datetime, timedelta, timezone + +import pytest +from dateutil.parser import parse + +from datadog_sync.model.downtime_schedules import DowntimeSchedules +from datadog_sync.utils.resource_utils import SkipResource + + +def _run(coro): + # Fresh loop per call: pytest-asyncio strict mode closes the ambient loop + # between tests, so asyncio.get_event_loop() may raise "no current event + # loop" when this helper runs after unrelated async tests in the suite. + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + +def _now_ts() -> float: + return datetime.now(timezone.utc).timestamp() + + +def _past_iso(seconds_ago: int = 3600) -> str: + return (datetime.now(timezone.utc) - timedelta(seconds=seconds_ago)).isoformat().replace("+00:00", "Z") + + +def _future_iso(seconds_ahead: int = 3600) -> str: + return (datetime.now(timezone.utc) + timedelta(seconds=seconds_ahead)).isoformat().replace("+00:00", "Z") + + +def _make_resource(schedule): + return {"attributes": {"schedule": schedule}} + + +def test_past_start_bumped_forward(mock_config): + """Baseline invariant: past schedule.start with no `end` (open-ended + downtime) is rewritten to ~now+60s. Regression guard so the refactor + didn't change the pre-existing contract.""" + downtime = DowntimeSchedules(mock_config) + past = _past_iso(3600) + resource = _make_resource({"start": past}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + rewritten = resource["attributes"]["schedule"]["start"] + now_ts = _now_ts() + assert rewritten != past + assert now_ts - 5 < parse(rewritten).timestamp() < now_ts + 120 + + +def test_past_start_future_end_preserves_end(mock_config): + """Past `start` with a future `end`: bump `start` forward, leave `end` + alone. The customer's maintenance still ends at their intended time — + the window may be shorter than the source's, but the end boundary is + honored.""" + downtime = DowntimeSchedules(mock_config) + past_start = _past_iso(3600) + future_end = _future_iso(3600) + resource = _make_resource({"start": past_start, "end": future_end}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + schedule = resource["attributes"]["schedule"] + assert schedule["start"] != past_start + assert schedule["end"] == future_end, "future end must be untouched" + # `end > start` invariant still holds because start is now ~now+60s and + # end is ~+3600s. + assert parse(schedule["end"]).timestamp() > parse(schedule["start"]).timestamp() + + +def test_past_end_raises_skip(mock_config): + """New behavior: past `end` means the downtime has already ended on the + source. Skip the resource — replicating an expired maintenance to the + destination either produces a 400 or invents a phantom window.""" + downtime = DowntimeSchedules(mock_config) + resource = _make_resource({"start": _past_iso(7200), "end": _past_iso(3600)}) + + with pytest.raises(SkipResource) as excinfo: + _run(downtime.pre_resource_action_hook("skip-id", resource)) + + assert "past" in str(excinfo.value).lower() + + +def test_past_end_raises_skip_even_with_future_start(mock_config): + """Degenerate but possible source shape: `end` in the past AND `start` + in the future (source is a broken record). Still skip — the window + doesn't make sense to replicate.""" + downtime = DowntimeSchedules(mock_config) + resource = _make_resource({"start": _future_iso(3600), "end": _past_iso(3600)}) + + with pytest.raises(SkipResource): + _run(downtime.pre_resource_action_hook("skip-id", resource)) + + +def test_future_start_and_end_untouched(mock_config): + """Values already in the future must NOT be rewritten. Rewriting would + change the customer's intended window and produce a spurious diff on + subsequent syncs.""" + downtime = DowntimeSchedules(mock_config) + start_future = _future_iso(3600) + end_future = _future_iso(7200) + resource = _make_resource({"start": start_future, "end": end_future}) + + _run(downtime.pre_resource_action_hook("new-id", resource)) + + schedule = resource["attributes"]["schedule"] + assert schedule["start"] == start_future + assert schedule["end"] == end_future + + +def test_missing_or_null_schedule_no_op(mock_config): + """Edge cases: schedule may be absent, empty, or None. The hook must + tolerate all three without raising.""" + downtime = DowntimeSchedules(mock_config) + + # empty schedule + r1 = _make_resource({}) + _run(downtime.pre_resource_action_hook("id-1", r1)) + assert r1["attributes"]["schedule"] == {} + + # schedule is None + r2 = {"attributes": {"schedule": None}} + _run(downtime.pre_resource_action_hook("id-2", r2)) + assert r2["attributes"]["schedule"] is None + + # attributes.schedule key absent + r3 = {"attributes": {}} + _run(downtime.pre_resource_action_hook("id-3", r3)) + assert r3 == {"attributes": {}} + + +def test_start_only_no_end_field(mock_config): + """Open-ended downtime (no `end` key at all): past `start` is rewritten, + the missing-`end` shape is preserved (not injected).""" + downtime = DowntimeSchedules(mock_config) + resource = _make_resource({"start": _past_iso()}) + + _run(downtime.pre_resource_action_hook("id", resource)) + + schedule = resource["attributes"]["schedule"] + assert "end" not in schedule + assert parse(schedule["start"]).timestamp() > _now_ts() - 5 + + +def test_update_path_untouched_by_this_fix(mock_config): + """Explicit boundary: this change only touches the create branch. The + update-path branch that clamps source start/end backwards to + destination's stored values is intentionally out of scope; a follow-up + is planned to address the update-path case.""" + downtime = DowntimeSchedules(mock_config) + _id = "existing-id" + mock_config.state.destination["downtime_schedules"][_id] = { + "attributes": {"schedule": {"start": _past_iso(1800), "end": _past_iso(900)}} + } + + resource = _make_resource({"start": _past_iso(3600), "end": _past_iso(1200)}) + + # No SkipResource on update path even though end is past — update-path + # semantics are intentionally out of scope for this PR. + _run(downtime.pre_resource_action_hook(_id, resource)) + + schedule = resource["attributes"]["schedule"] + dest = mock_config.state.destination["downtime_schedules"][_id]["attributes"]["schedule"] + assert schedule["start"] == dest["start"] + assert schedule["end"] == dest["end"]