Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@

### Bugs Fixed

- Fixed a resource leak where replica clients that were no longer part of the auto-failover set were not closed during client refresh.
- Fixed auto-failover replica discovery so that a DNS SRV lookup timeout (for either the origin or replica records) is distinguished from an empty replica list. A timeout now correctly triggers the longer fallback refresh interval, while an empty result refreshes at the normal interval.
- Fixed a thread-safety issue by publishing refreshed secret values through a new configuration mapping instead of mutating the existing mapping while readers may be iterating.
- Fixed `refresh_on` handling so that a single-string watched setting is treated as a key with the default (no) label instead of being incorrectly unpacked character-by-character.
- Fixed a `KeyError` when loading with an endpoint and credential (no connection string).

### Other Changes

- Bumped minimum dependency on `azure-core` to `>=1.31.0`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def __init__(self, **kwargs: Any) -> None:
self._startup_timeout: int = kwargs.pop("startup_timeout", DEFAULT_STARTUP_TIMEOUT)

self._replica_client_manager = ConfigurationClientManager(
connection_string=kwargs.pop("connection_string"),
connection_string=kwargs.pop("connection_string", None),
endpoint=kwargs.pop("endpoint"),
credential=kwargs.pop("credential", None),
user_agent=user_agent,
Expand Down Expand Up @@ -207,7 +207,12 @@ def refresh(self, **kwargs) -> None:
self._secret_provider.secret_refresh_timer
and self._secret_provider.secret_refresh_timer.needs_refresh()
):
self._dict.update(self._secret_provider.refresh_secrets())
# Publish a new dict rather than mutating in place. Readers may hold a live view or
# iterator over self._dict, and an in-place update could raise "dictionary changed
# size during iteration". Every write to self._dict must rebind to a new object.
updated_settings = dict(self._dict)
updated_settings.update(self._secret_provider.refresh_secrets())
self._dict = updated_settings
Comment thread
mrm9084 marked this conversation as resolved.
self._replica_client_manager.refresh_clients()
self._replica_client_manager.find_active_clients()
replica_count = self._replica_client_manager.get_client_count() - 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,13 @@ def is_json_content_type(content_type: str) -> bool:


def _build_watched_setting(setting: Union[str, Tuple[str, str]]) -> Tuple[str, str]:
try:
key, label = setting # type:ignore
except (IndexError, ValueError):
key = str(setting) # Ensure key is a string
label = NULL_CHAR
if isinstance(setting, str):
key, label = setting, NULL_CHAR
Comment thread
mrm9084 marked this conversation as resolved.
else:
try:
key, label = setting
except (TypeError, ValueError):
key, label = str(setting), NULL_CHAR
Comment thread
mrm9084 marked this conversation as resolved.
if "*" in key or "*" in label:
raise ValueError("Wildcard key or label filters are not supported for refresh.")
return key, label
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def _check_configuration_setting(
self.LOGGER.debug("Refresh all triggered by key: %s label %s.", key, label)
return True, None
else:
raise e
raise
return False, None

@distributed_trace
Expand Down Expand Up @@ -343,7 +343,7 @@ def _validate_snapshot(self, snapshot_name: str) -> bool:
if e.status_code == 404:
self.LOGGER.warning("Snapshot '%s' not found when resolving snapshot.", snapshot_name)
return False
raise e
raise
if snapshot.composition_type != SnapshotComposition.KEY:
raise ValueError(f"Composition type for '{snapshot_name}' must be 'key'.")
return True
Expand Down Expand Up @@ -484,18 +484,13 @@ def refresh_clients(self):
if self._next_update_time and self._next_update_time > time.time():
return

failover_endpoints = find_auto_failover_endpoints(self._original_endpoint, self._replica_discovery_enabled)

if failover_endpoints is None:
# SRV record not found, so we should refresh after a longer interval
try:
failover_endpoints = find_auto_failover_endpoints(self._original_endpoint, self._replica_discovery_enabled)
except TimeoutError:
# SRV record resolution timed out, so we should refresh after a longer interval
self._next_update_time = time.time() + FALLBACK_CLIENT_REFRESH_EXPIRED_INTERVAL
return

if len(failover_endpoints) == 0:
# No failover endpoints in SRV record.
self._next_update_time = time.time() + MINIMAL_CLIENT_REFRESH_INTERVAL
return

discovered_clients = []
for failover_endpoint in failover_endpoints:
found_client = False
Expand Down Expand Up @@ -531,6 +526,12 @@ def refresh_clients(self):
)
)
self._next_update_time = time.time() + MINIMAL_CLIENT_REFRESH_INTERVAL
# Close any replica clients that are no longer part of the failover.
retained_endpoints = {self._original_client.endpoint}
retained_endpoints.update(client.endpoint for client in discovered_clients)
for client in self._replica_clients:
if client.endpoint not in retained_endpoints:
client.close()
Comment thread
mrm9084 marked this conversation as resolved.
if not self._load_balancing_enabled:
random.shuffle(discovered_clients)
self._replica_clients = [self._original_client] + discovered_clients
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ def find_auto_failover_endpoints(endpoint: str, replica_discovery_enabled: bool)

replicas = _find_replicas(origin.target)

if not replicas:
return None # Timeout
if replicas is None:
raise TimeoutError("Timed out while resolving auto-failover replica endpoints.")
Comment thread
mrm9084 marked this conversation as resolved.
Comment thread
mrm9084 marked this conversation as resolved.

srv_records = [origin] + replicas
endpoints = []
Expand All @@ -57,6 +57,8 @@ def _find_origin(endpoint):
uri = urlparse(endpoint).hostname
request = f"_origin._tcp.{uri}"
srv_records = _request_record(request)
if srv_records is None:
raise TimeoutError("Timed out while resolving auto-failover origin endpoint.")
if not srv_records:
return None
return SRVRecord(srv_records[0])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def __init__(self, **kwargs: Any) -> None:
)

if kwargs.get("secret_refresh_interval", 60) < 1:
raise ValueError("Secret refresh interval must be greater than 1 second.")
raise ValueError("Secret refresh interval must be at least 1 second.")

self.secret_refresh_timer: Optional[_RefreshTimer] = (
_RefreshTimer(refresh_interval=kwargs.pop("secret_refresh_interval", 60))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ async def _check_configuration_setting(
self.LOGGER.debug("Refresh all triggered by key: %s label %s.", key, label)
return True, None
else:
raise e
raise
return False, None

@distributed_trace
Expand Down Expand Up @@ -345,7 +345,7 @@ async def _validate_snapshot(self, snapshot_name: str) -> bool:
if e.status_code == 404:
self.LOGGER.warning("Snapshot '%s' not found when resolving snapshot.", snapshot_name)
return False
raise e
raise
if snapshot.composition_type != SnapshotComposition.KEY:
raise ValueError(f"Composition type for '{snapshot_name}' must be 'key'.")
return True
Expand Down Expand Up @@ -486,20 +486,15 @@ async def refresh_clients(self):
if self._next_update_time and self._next_update_time > time.time():
return

failover_endpoints = await find_auto_failover_endpoints(
self._original_endpoint, self._replica_discovery_enabled
)

if failover_endpoints is None:
# SRV record not found, so we should refresh after a longer interval
try:
failover_endpoints = await find_auto_failover_endpoints(
self._original_endpoint, self._replica_discovery_enabled
)
except TimeoutError:
# SRV record resolution timed out, so we should refresh after a longer interval
self._next_update_time = time.time() + FALLBACK_CLIENT_REFRESH_EXPIRED_INTERVAL
return

if len(failover_endpoints) == 0:
# No failover endpoints in SRV record.
self._next_update_time = time.time() + MINIMAL_CLIENT_REFRESH_INTERVAL
return

discovered_clients = []
for failover_endpoint in failover_endpoints:
found_client = False
Expand Down Expand Up @@ -535,6 +530,12 @@ async def refresh_clients(self):
)
)
self._next_update_time = time.time() + MINIMAL_CLIENT_REFRESH_INTERVAL
# Close any replica clients that are no longer part of the failover.
Comment thread
mrm9084 marked this conversation as resolved.
retained_endpoints = {self._original_client.endpoint}
retained_endpoints.update(client.endpoint for client in discovered_clients)
for client in self._replica_clients:
if client.endpoint not in retained_endpoints:
await client.close()
Comment thread
mrm9084 marked this conversation as resolved.
if not self._load_balancing_enabled:
random.shuffle(discovered_clients)
self._replica_clients = [self._original_client] + discovered_clients
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ async def find_auto_failover_endpoints(endpoint: str, replica_discovery_enabled:

replicas = await _find_replicas(origin.target)

if not replicas:
return None # Timeout
if replicas is None:
raise TimeoutError("Timed out while resolving auto-failover replica endpoints.")
Comment thread
mrm9084 marked this conversation as resolved.

srv_records = [origin] + replicas
endpoints = []
Expand All @@ -42,6 +42,8 @@ async def _find_origin(endpoint):
uri = urlparse(endpoint).hostname
request = f"_origin._tcp.{uri}"
srv_records = await _request_record(request)
if srv_records is None:
raise TimeoutError("Timed out while resolving auto-failover origin endpoint.")
if not srv_records:
return None
return SRVRecord(srv_records[0])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,12 @@ async def refresh(self, **kwargs) -> None:
self._secret_provider.secret_refresh_timer
and self._secret_provider.secret_refresh_timer.needs_refresh()
):
self._dict.update(await self._secret_provider.refresh_secrets())
# Publish a new dict rather than mutating in place. Readers may hold a live view or
# iterator over self._dict, and an in-place update could raise "dictionary changed
# size during iteration". Every write to self._dict must rebind to a new object.
updated_settings = dict(self._dict)
updated_settings.update(await self._secret_provider.refresh_secrets())
self._dict = updated_settings
Comment thread
mrm9084 marked this conversation as resolved.
await self._replica_client_manager.refresh_clients()
self._replica_client_manager.find_active_clients()
replica_count = self._replica_client_manager.get_client_count() - 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ async def test_find_replicas(self, mock_request_record):
async def test_find_origin(self, mock_request_record):
endpoint = "https://fake.endpoint"
mock_request_record.return_value = None
assert not await _find_origin(endpoint)
with pytest.raises(TimeoutError):
await _find_origin(endpoint)
mock_request_record.assert_called_once_with("_origin._tcp.fake.endpoint")

mock_request_record.reset_mock()
Expand Down Expand Up @@ -202,3 +203,22 @@ async def test_find_auto_failover_endpoints(self, mock_find_replicas, mock_find_
assert result[1] == "https://fake2.appconfig.io"
mock_find_origin.assert_called_once_with("https://fake1.appconfig.io")
mock_find_replicas.assert_called_once_with("fake.appconfig.io")

# A timeout while resolving replicas is surfaced as a TimeoutError
mock_find_origin.reset_mock()
mock_find_replicas.reset_mock()
mock_find_origin.return_value = FakeAnswer(1, 99, 5000, "fake.appconfig.io")
mock_find_replicas.return_value = None
with pytest.raises(TimeoutError):
await find_auto_failover_endpoints(endpoint, True)
mock_find_origin.assert_called_once_with(endpoint)
mock_find_replicas.assert_called_once_with("fake.appconfig.io")

# A timeout while resolving the origin is surfaced as a TimeoutError
mock_find_origin.reset_mock()
mock_find_replicas.reset_mock()
mock_find_origin.side_effect = TimeoutError("timeout")
with pytest.raises(TimeoutError):
await find_auto_failover_endpoints(endpoint, True)
mock_find_origin.assert_called_once_with(endpoint)
mock_find_replicas.assert_not_called()
Loading
Loading