refactor: DNS backend #876
Conversation
…agement and error handling
…andling in PowerDNSManager
…nt and authorization rules
There was a problem hiding this comment.
Pull request overview
This PR refactors the DNS backend from Bind9 to PowerDNS, implementing a two-component architecture with PowerDNS Authoritative Server and Recursor. The refactoring involves significant changes to the DNS management layer, API endpoints, and configuration.
Key changes:
- Migration from Bind9 to PowerDNS with custom Dockerfile for LMDB backend support
- Removal of self-hosted Bind9 manager and custom DNS API
- Introduction of new DTOs and enums aligned with PowerDNS data structures
- API restructuring with zone_id as path parameters and separate endpoints for forward zones
- Removal of DNS server-level configuration endpoints (DNSSEC now zone-specific)
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pdns.conf | PowerDNS Authoritative Server configuration with LMDB backend |
| recursor.conf | PowerDNS Recursor configuration with API access |
| .docker/pdns_auth.Dockerfile | Alpine-based build for PowerDNS with LMDB support |
| docker-compose.yml | Replaced bind_dns service with pdns_auth and pdns_recursor services |
| app/ldap_protocol/dns/powerdns.py | New PowerDNS manager implementing AbstractDNSManager interface |
| app/ldap_protocol/dns/dto.py | Extended DTOs for PowerDNS zones, records, and RRSets |
| app/ldap_protocol/dns/enums.py | New enums for DNS record types, zone types, and change types |
| app/api/main/dns_router.py | Restructured routes with zone_id path params and forward zone endpoints |
| app/api/main/adapters/dns.py | Adapter updated to construct DTOs from request schemas |
| app/ldap_protocol/dns/use_cases.py | Simplified use cases with removed server options functionality |
| app/config.py | New config for PDNS hosts and API key |
| app/ioc.py | Updated DI container for PowerDNS client provisioning |
| tests/test_api/test_main/test_dns.py | Comprehensive test updates for new API structure |
Comments suppressed due to low confidence (1)
app/ldap_protocol/dns/stub.py:77
- This method requires 2 positional arguments, whereas overridden AbstractDNSManager.check_forward_dns_server requires 3.
@logger_wraps(is_stub=True)
async def check_forward_dns_server(
self,
dns_server_ip: str,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def delete_zones(self, zone_ids: list[str]) -> None: | ||
| """Delete DNS zones.""" | ||
| for zone_id in zone_ids: | ||
| await self._dns_manager.delete_zone(zone_id) |
There was a problem hiding this comment.
The use_cases.py delete_zones method iterates through zone_ids and calls delete_zone for each one. If any of these calls fail, the remaining zones won't be deleted. Consider implementing error handling to either collect errors and raise them at the end, or use a transaction-like mechanism to ensure all-or-nothing deletion.
| @@ -0,0 +1,10 @@ | |||
| local-address=0.0.0.0 | |||
| webserver-allow-from=0.0.0.0/0 | |||
There was a problem hiding this comment.
The webserver is configured to allow connections from any IP address (0.0.0.0/0). This is a security risk in production environments. Consider restricting access to specific IP ranges or implementing additional authentication mechanisms.
| webserver-allow-from=0.0.0.0/0 | |
| webserver-allow-from=127.0.0.1/32 |
There was a problem hiding this comment.
Пока сделано так, в дальнейшем, при добавлении PowerDNS DNSdist будет переделано.
| local-port=53 | ||
| api=yes | ||
| api-key=supersecretapikey | ||
| webserver-allow-from=0.0.0.0/0 |
There was a problem hiding this comment.
The webserver is configured to allow connections from any IP address (0.0.0.0/0). This is a security risk in production environments. Consider restricting access to specific IP ranges or implementing additional authentication mechanisms.
| webserver-allow-from=0.0.0.0/0 | |
| webserver-allow-from=127.0.0.1,::1 |
There was a problem hiding this comment.
Здесь то же самое
| local-address=0.0.0.0 | ||
| local-port=53 | ||
| api=yes | ||
| api-key=supersecretapikey |
There was a problem hiding this comment.
The API key "supersecretapikey" is hardcoded in the configuration file. This is a security risk as it exposes the API key in version control. The API key should be passed as an environment variable or mounted from a secrets management system.
| api-key=supersecretapikey | |
| api-key=${PDNS_API_KEY} |
There was a problem hiding this comment.
API не торчит наружу из сети докера. Сделано так, потому что без ключа API не работает.
recursor.conf
Outdated
| webserver=yes | ||
| webserver-address=0.0.0.0 | ||
| webserver-port=8083 | ||
| api-key=supersecretapikey No newline at end of file |
There was a problem hiding this comment.
The API key "supersecretapikey" is hardcoded in the configuration file. This is a security risk as it exposes the API key in version control. The API key should be passed as an environment variable or mounted from a secrets management system.
| api-key=supersecretapikey | |
| api-key=changeme # real value must be injected from environment/secrets at deploy time |
There was a problem hiding this comment.
То же самое, что и выше.
| async def delete_forward_zones(self, zone_ids: list[str]) -> None: | ||
| """Delete DNS forward zones.""" | ||
| for zone_id in zone_ids: | ||
| await self._dns_manager.delete_forward_zone(zone_id) |
There was a problem hiding this comment.
The same issue exists in delete_forward_zones - if any deletion fails, the remaining zones won't be deleted. Consider implementing error handling to either collect errors and raise them at the end, or use a transaction-like mechanism to ensure all-or-nothing deletion.
| async def create(self, data: CatalogueSetting) -> None: | ||
| """Create DNS.""" | ||
| self._session.add(data) | ||
| await self._session.commit() |
There was a problem hiding this comment.
на уровне gateway мы можем делать commit? по моему на таком низком уровне мы договаривались без коммита, а только flush
т.к. если тут сделать коммит, то транзакция закроется и мы не сможем собрать пайплайн из двух небольших методов гейтвея (если в каждом из них будет по коммиту), тк первый метод закроет транзакцию
There was a problem hiding this comment.
Нужна помощь второго ревьювера, не берусь утверждать
There was a problem hiding this comment.
в manager/use_case/service есть коммит
гейтвей как и dao как и репозиторий примерно одно и то же с нюансами, но все инкапсулирует доступ к данным, поэтому там коммита нет, чтобы можно было легко собрать любой пайплайн из этих методов, как из кирпичиков и транзакция не закрылась по середине
There was a problem hiding this comment.
Дима прав. Егор говорил, что мы управляем сессией на уровне use_case не gateway. Т.к. задача gateway работать с данными, а когда их отнести до БД, решается выше уровнем
…by_zone_kind method
…d updating DTO import
| code = ErrorCodes.DNS_NOT_IMPLEMENTED_ERROR | ||
|
|
||
|
|
||
| class DNSUnavailableError(DNSError): |
There was a problem hiding this comment.
если какая-то из ошибок идет на фронт, нужно добавить код в enum выше. так же добавить в error_map ее, чтобы она обрабатывалась в роуте
There was a problem hiding this comment.
Эта ошибка не идет на фронт, но добавил другие
| base_retort = Retort() | ||
|
|
||
|
|
||
| class PowerDNSManager(AbstractDNSManager): |
There was a problem hiding this comment.
как по мне, получился уберкомбайнер, который шлет\валидирует запросы(работа с http), логика самого днс, вычисление и агрегация данных. ИМХО слишком много всего, я бы декомпозировал
There was a problem hiding this comment.
Согласен, но предлагаю отложить пока
|
а мы от адаптикса отказались, почему все дто вручную? |
Потому что пытался по минимуму менять схемы фронтовые, чтобы меньше фронтам переделывать |
так адаптикс просто код перевода объектов убирает из логики, схему фронтовые можно не менять |
… in workflows and docker-compose
Задачи: 1057, 1058
Произведена замена DNS сервера с Bind9 на PowerDNS, состоящий из двух компонентов: Authoritative Server и Recursor. Произведен рефакторинг компонентов DNS сервера. Вся логика, относящаяся к Bind9, была удалена, за исключением некоторым датаклассов по просьбе Егора. Вся логика была воспроизведена в соответствии с новым DNS сервером за исключением пары фич: включение/отключение DNSSEC опции DNS сервера - теперь это опция относится к зоне, соответственно, временно удален функционал получения и изменения параметров DNS сервера, удалена возможность перезагружать DNS сервер и отдельную зону, так как в этом больше нет необходимости и на фронт этот функционал так и не был выведен.
Сделано: