Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix get federation status of destination if no error occured #11593

Merged
merged 9 commits into from
Jan 5, 2022
1 change: 1 addition & 0 deletions changelog.d/11593.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix an error to get federation status of a destination server even if no error has occurred.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include when the bug was introduced and note somewhere that this only applies to an admin API? Thanks!

22 changes: 17 additions & 5 deletions synapse/rest/admin/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,26 @@ async def on_GET(
) -> Tuple[int, JsonDict]:
await assert_requester_is_admin(self._auth, request)

if not await self._store.is_destination_known(destination):
raise NotFoundError("Unknown destination")

destination_retry_timings = await self._store.get_destination_retry_timings(
destination
)

if not destination_retry_timings:
raise NotFoundError("Unknown destination")
retry_timing_response: JsonDict = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be clearer to create the dictionary initially with destination and then add more keys to it, but I don't feel strongly either way!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion is response (GET /_synapse/admin/v1/federation/destinations/<destination>) with

{
  "destination": {
     "destination": "matrix.org",
     "retry_last_ts": 1557332397936,
     "retry_interval": 3000000,
     "failure_ts": 1557329397936,
     "last_successful_stream_ordering": null
  }
}

instead of:

{
   "destination": "matrix.org",
   "retry_last_ts": 1557332397936,
   "retry_interval": 3000000,
   "failure_ts": 1557329397936,
   "last_successful_stream_ordering": null
}

?

This would be different to rooms and users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, my suggestion is to build the response as:

retry_timing_response: JsonDict = {
    "destination": destination,
}
retry_timing_response[...] = ...

if destination_retry_timings:
retry_timing_response = {
"failure_ts": destination_retry_timings.failure_ts,
"retry_last_ts": destination_retry_timings.retry_last_ts,
"retry_interval": destination_retry_timings.retry_interval,
}
else:
retry_timing_response = {
"failure_ts": None,
"retry_last_ts": 0,
"retry_interval": 0,
}

last_successful_stream_ordering = (
clokep marked this conversation as resolved.
Show resolved Hide resolved
await self._store.get_destination_last_successful_stream_ordering(
Expand All @@ -126,10 +140,8 @@ async def on_GET(

response = {
"destination": destination,
"failure_ts": destination_retry_timings.failure_ts,
"retry_last_ts": destination_retry_timings.retry_last_ts,
"retry_interval": destination_retry_timings.retry_interval,
"last_successful_stream_ordering": last_successful_stream_ordering,
**retry_timing_response,
}

return HTTPStatus.OK, response
11 changes: 11 additions & 0 deletions synapse/storage/databases/main/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -560,3 +560,14 @@ def get_destinations_paginate_txn(
return await self.db_pool.runInteraction(
"get_destinations_paginate_txn", get_destinations_paginate_txn
)

async def is_destination_known(self, destination: str) -> bool:
"""Check if a destination is known to the server."""
result = await self.db_pool.simple_select_one_onecol(
table="destinations",
keyvalues={"destination": destination},
retcol="1",
allow_none=True,
desc="is_destination_known",
)
return bool(result)
75 changes: 57 additions & 18 deletions tests/rest/admin/test_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,15 +314,12 @@ def _order_test(
retry_interval,
last_successful_stream_ordering,
) in dest:
self.get_success(
self.store.set_destination_retry_timings(
destination, failure_ts, retry_last_ts, retry_interval
)
)
self.get_success(
self.store.set_destination_last_successful_stream_ordering(
destination, last_successful_stream_ordering
)
self._create_destination(
destination,
failure_ts,
retry_last_ts,
retry_interval,
last_successful_stream_ordering,
)

# order by default (destination)
Expand Down Expand Up @@ -413,11 +410,9 @@ def _search_test(
_search_test(None, "foo")
_search_test(None, "bar")

def test_get_single_destination(self) -> None:
"""
Get one specific destinations.
"""
self._create_destinations(5)
def test_get_single_destination_with_retry_timings(self) -> None:
"""Get one specific destination which has retry timings."""
self._create_destinations(1)

channel = self.make_request(
"GET",
Expand All @@ -432,6 +427,53 @@ def test_get_single_destination(self) -> None:
# convert channel.json_body into a List
self._check_fields([channel.json_body])

def test_get_single_destination_no_retry_timings(self) -> None:
"""Get one specific destination which has no retry timings."""
self._create_destination("sub0.example.com")

channel = self.make_request(
"GET",
self.url + "/sub0.example.com",
access_token=self.admin_user_tok,
)

self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body)
self.assertEqual("sub0.example.com", channel.json_body["destination"])
self.assertEqual(0, channel.json_body["retry_last_ts"])
self.assertEqual(0, channel.json_body["retry_interval"])
self.assertIsNone(channel.json_body["failure_ts"])
self.assertIsNone(channel.json_body["last_successful_stream_ordering"])

def _create_destination(
self,
destination: str,
failure_ts: Optional[int] = None,
retry_last_ts: int = 0,
retry_interval: int = 0,
last_successful_stream_ordering: Optional[int] = None,
) -> None:
"""Create one specific destination

Args:
destination: the destination we have successfully sent to
failure_ts: when the server started failing (ms since epoch)
retry_last_ts: time of last retry attempt in unix epoch ms
retry_interval: how long until next retry in ms
last_successful_stream_ordering: the stream_ordering of the most
recent successfully-sent PDU
"""
self.get_success(
self.store.set_destination_retry_timings(
destination, failure_ts, retry_last_ts, retry_interval
)
)
if last_successful_stream_ordering is not None:
self.get_success(
self.store.set_destination_last_successful_stream_ordering(
destination, last_successful_stream_ordering
)
)

def _create_destinations(self, number_destinations: int) -> None:
"""Create a number of destinations

Expand All @@ -440,10 +482,7 @@ def _create_destinations(self, number_destinations: int) -> None:
"""
for i in range(0, number_destinations):
dest = f"sub{i}.example.com"
self.get_success(self.store.set_destination_retry_timings(dest, 50, 50, 50))
self.get_success(
self.store.set_destination_last_successful_stream_ordering(dest, 100)
)
self._create_destination(dest, 50, 50, 50, 100)

def _check_fields(self, content: List[JsonDict]) -> None:
"""Checks that the expected destination attributes are present in content
Expand Down