SIENTIAPDE-1974: fix(read): code-review fixes for the slot v2 read path
- config: GatewayConfig.certs_from_env() reads only CERT_BUCKET/CERT_CACHE_DIR; IngestorManager uses it, so a malformed write/gRPC env var (GRPC_PORT, WRITE_MIN_PAUSE_S, IDLE_CONN_MAX_S) can no longer stop the reads. - certs: CertProvider raises CertificateNotConfiguredError with an actionable message when no platform_certificates document exists; downloads once under an asyncio.Lock, caches the paths, writes atomically (tmp + os.replace); invalidate() forces a fresh download. - pool: server_id normalized to str (1 and '1' share a session); warns when a get() asks for other url/server_uri/use_security than the pooled client was opened with (client reused, the slot config wins); invalidates the cert cache when a secured connect fails. - SubscriptionEngine: tracks nodes per subscription and forgets them on unsubscribe (no more cycle alarms / tag count for removed tags), keeping addresses still monitored by another slot. - ServerReader.shutdown(evict) / IngestorManager.remove_server(evict): a server that only left this pod's slots keeps the shared connection for NATS writes/gRPC; lost servers and config changes still evict. - IngestorManager warns once per server when a slot still sets server_cert_path (server certificate pinning is no longer applied). - slots: CONNECTION_FIELDS exported and reused by ServerReader. Accepted, not changed: read metrics label server_name with the server_id (same as the write path, which only knows the id); a malformed slot is notified each lease attempt (alerts are deduplicated by notification_id). Tests: 236 unit tests pass (+18); ruff clean; local e2e against an asyncua server incl. slot moved (connection kept and reused by the write path) and lost-server eviction/re-initialization. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -70,3 +70,79 @@ def test_build_cert_provider_is_lazy_and_uses_config(monkeypatch):
|
||||
assert provider.cache_dir == 'certs-cache'
|
||||
assert isinstance(provider.minio, certs.LazyMinio)
|
||||
minio_cls.assert_not_called() # MinIO is only reached when a cert is needed
|
||||
|
||||
|
||||
def _provider(tmp_path, doc=None, data=b'pem'):
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
mongo = MagicMock()
|
||||
mongo.find_one = AsyncMock(
|
||||
return_value=doc
|
||||
if doc is not None
|
||||
else {
|
||||
'client_cert': {'minio_object_name': 'platform_certificates/c.pem'},
|
||||
'private_key': {'minio_object_name': 'platform_certificates/k.pem'},
|
||||
}
|
||||
)
|
||||
minio = MagicMock()
|
||||
minio.download_file = AsyncMock(return_value=data)
|
||||
return CertProvider(mongo=mongo, minio=minio, cache_dir=str(tmp_path), bucket='b'), minio
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
'doc',
|
||||
[
|
||||
None,
|
||||
{},
|
||||
{'client_cert': {'minio_object_name': 'c.pem'}},
|
||||
{'client_cert': None, 'private_key': {}},
|
||||
],
|
||||
)
|
||||
async def test_fetch_without_platform_certificate_raises_clear_error(tmp_path, doc):
|
||||
from ingestor.connection.certs import CertificateNotConfiguredError
|
||||
|
||||
provider, minio = _provider(tmp_path)
|
||||
provider.mongo.find_one.return_value = doc
|
||||
|
||||
with pytest.raises(CertificateNotConfiguredError, match='platform_certificates'):
|
||||
await provider.fetch()
|
||||
minio.download_file.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fetch_is_cached_until_invalidated(tmp_path):
|
||||
provider, minio = _provider(tmp_path)
|
||||
|
||||
first = await provider.fetch()
|
||||
second = await provider.fetch()
|
||||
assert first == second
|
||||
assert minio.download_file.await_count == 2 # cert + key, once
|
||||
|
||||
provider.invalidate()
|
||||
await provider.fetch()
|
||||
assert minio.download_file.await_count == 4
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_concurrent_fetches_download_once(tmp_path):
|
||||
import asyncio
|
||||
|
||||
provider, minio = _provider(tmp_path)
|
||||
|
||||
results = await asyncio.gather(*(provider.fetch() for _ in range(5)))
|
||||
|
||||
assert len(set(results)) == 1
|
||||
assert minio.download_file.await_count == 2
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_download_replaces_file_atomically(tmp_path):
|
||||
provider, minio = _provider(tmp_path, data=b'new')
|
||||
(tmp_path / 'c.pem').write_bytes(b'old')
|
||||
|
||||
cert_path, _ = await provider.fetch()
|
||||
|
||||
assert (tmp_path / 'c.pem').read_bytes() == b'new'
|
||||
assert cert_path == str(tmp_path / 'c.pem')
|
||||
assert not list(tmp_path.glob('*.tmp'))
|
||||
|
||||
@@ -248,3 +248,63 @@ async def test_evict_unknown_server_is_noop():
|
||||
await pool.evict('nope')
|
||||
|
||||
pool.emit_metric.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_normalizes_server_id_to_str():
|
||||
pool = make_pool()
|
||||
fake_client = AsyncMock()
|
||||
with patch('ingestor.connection.pool.Client', return_value=fake_client):
|
||||
c1 = await pool.get(1, {'url': 'opc.tcp://h:4840', 'use_security': False})
|
||||
c2 = await pool.get('1', {'url': 'opc.tcp://h:4840', 'use_security': False})
|
||||
|
||||
assert c1 is c2
|
||||
assert fake_client.connect.await_count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_with_different_cfg_reuses_client_and_warns():
|
||||
pool = make_pool()
|
||||
fake_client = AsyncMock()
|
||||
with patch('ingestor.connection.pool.Client', return_value=fake_client):
|
||||
await pool.get('srv', {'url': 'opc.tcp://h:4840', 'use_security': False})
|
||||
client = await pool.get('srv', {'url': 'opc.tcp://10.0.0.5:4840', 'use_security': False})
|
||||
|
||||
assert client is fake_client
|
||||
assert fake_client.connect.await_count == 1
|
||||
pool.logger.warning.assert_called_once()
|
||||
assert 'url' in pool.logger.warning.call_args.args[0]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_secured_connect_failure_invalidates_cert_cache():
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
cert_provider = MagicMock()
|
||||
cert_provider.fetch = AsyncMock(return_value=('cert.pem', 'key.pem'))
|
||||
pool = make_pool(cert_provider=cert_provider)
|
||||
fake_client = AsyncMock()
|
||||
fake_client.connect.side_effect = RuntimeError('BadCertificateUntrusted')
|
||||
with (
|
||||
patch('ingestor.connection.pool.Client', return_value=fake_client),
|
||||
patch('ingestor.connection.pool.apply_security', AsyncMock()),
|
||||
):
|
||||
with pytest.raises(RuntimeError):
|
||||
await pool.get('srv', {'url': 'opc.tcp://h:4840', 'use_security': True})
|
||||
|
||||
cert_provider.invalidate.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_insecure_connect_failure_keeps_cert_cache():
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
cert_provider = MagicMock()
|
||||
pool = make_pool(cert_provider=cert_provider)
|
||||
fake_client = AsyncMock()
|
||||
fake_client.connect.side_effect = RuntimeError('unreachable')
|
||||
with patch('ingestor.connection.pool.Client', return_value=fake_client):
|
||||
with pytest.raises(RuntimeError):
|
||||
await pool.get('srv', {'url': 'opc.tcp://h:4840', 'use_security': False})
|
||||
|
||||
cert_provider.invalidate.assert_not_called()
|
||||
|
||||
Reference in New Issue
Block a user