Conversation
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.
app/ldap_protocol/dns/use_cases.py
Outdated
| 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.
Здесь то же самое
recursor.conf
Outdated
| webserver=yes | ||
| webserver-address=0.0.0.0 | ||
| webserver-port=8083 | ||
| 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=changeme # real value must be injected from environment/secrets at deploy time |
There was a problem hiding this comment.
То же самое, что и выше.
app/ldap_protocol/dns/use_cases.py
Outdated
| 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 работать с данными, а когда их отнести до БД, решается выше уровнем
| 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.
Согласен, но предлагаю отложить пока
|
а мы от адаптикса отказались, почему все дто вручную? |
Потому что пытался по минимуму менять схемы фронтовые, чтобы меньше фронтам переделывать |
так адаптикс просто код перевода объектов убирает из логики, схему фронтовые можно не менять |
iyashnov
force-pushed
the
refactor_dns_backend
branch
from
49664af to
8bd9a0e
Compare
milov-dmitriy
left a comment
milov-dmitriy
left a comment
There was a problem hiding this comment.
ничего существенного только нейминг
я бы в будущем посмотрел в сторону рефакторинга app/ldap_protocol/dns, хотелось бы сгруппировать логические блоки кода между собой, например всех менеджеров в одну кучу, все Enum положить в одно место, нужно уменьшить кол-во разных сущностей в base, по итогу получится в лучших традициях high cohesion/low coupling
| - ./recursor.conf:/etc/powerdns/recursor.conf | ||
| - forward_zones:/etc/powerdns/recursor.d/ | ||
|
|
||
| pdnsdist: |
There was a problem hiding this comment.
pdns_dist ? стоит в едином стиле делать?
There was a problem hiding this comment.
Официально оно просто dnsdist называется
| base_retort = Retort() | ||
|
|
||
|
|
||
| class PowerDNSDistClient: |
There was a problem hiding this comment.
-
предлагаю согласовать названия файла и класса:
power_dns_manager.py< - >PowerDNSDistClient -
а что значит Dist?
There was a problem hiding this comment.
- думаю, что было бы яснее, если мы бы смогли более явно указать, что значит remote_dns_manager. указать его отличие от других dns менеджеров если они есть (судя по названию вроде как должны быть)
т.е. предполагается что
- есть не только remote? но и local?
или - remote client и local client?
There was a problem hiding this comment.
Remote DNS manager - унифицированный менеджер для DNS'a клиента, который может только записями управлять
Хз, как его еще обозвать
| PDNS_AUTH_SERVER_HOST: str = "pdns_auth" | ||
| PDNS_AUTH_SERVER_IP: str = "172.20.0.4" | ||
| PDNS_AUTH_SERVER_PORT: int = 8082 | ||
| PDNS_RECURSOR_SERVER_HOST: str = "pdns_recursor" | ||
| PDNS_RECURSOR_SERVER_IP: str = "172.20.0.2" | ||
| PDNS_RECURSOR_SERVER_PORT: int = 8083 | ||
| PDNS_DIST_HOST: str = "172.20.0.3" | ||
| PDNS_DIST_PORT: int = 8084 | ||
| PDNS_DIST_CONFIG_PATH: str = "/dnsdist/delta.conf" | ||
| PDNS_DIST_KEY: str | ||
| PDNS_API_KEY: str | ||
| DEFAULT_NAMESERVER: str |
There was a problem hiding this comment.
добавить описание в вики
…agement and error handling
…te resolv.conf file
iyashnov
force-pushed
the
refactor_dns_backend
branch
from
7198477 to
2c2afb5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 63 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log = logger.bind(name="DNSManager") | ||
|
|
||
| log.add( | ||
| "logs/dnsmanager_{time:DD-MM-YYYY}.log", | ||
| filter=lambda rec: rec["extra"].get("name") == "dnsmanager", | ||
| retention="10 days", | ||
| rotation="1d", | ||
| colorize=False, | ||
| ) |
There was a problem hiding this comment.
The log sink filter checks rec["extra"].get("name") == "dnsmanager", but the logger is bound with name="DNSManager". With the current mismatch, this sink will never receive records. Align the bound name and filter value (and consider ensuring the logs/ directory exists before adding the sink, otherwise Loguru will fail to open the file).
milov-dmitriy
left a comment
milov-dmitriy
left a comment
There was a problem hiding this comment.
[опционально]
по-хорошему проверить у всех затронутых файлов шапку: докстринг + год копирайта.
| command = f""" | ||
| newServer({{ | ||
| address = "{recursor_ip}:53", | ||
| pool = "recursor" | ||
| }}) | ||
| """ |
There was a problem hiding this comment.
смотрю и хочется эти команды унести в отдельный класс, например: DNSdistCommands
сейчас команды выглядят как магические переменные
There was a problem hiding this comment.
Я тоже думал об этом, но не придумал, как сделать их шаблонизированными
There was a problem hiding this comment.
вопрос не в шаблоне а в "инкапсуляции" их "куда-нибудь"
буквально так:
class _DNSdistCommands(flag):
ADD_SERVER = "newServer({{ . . ."
GET_ALL_RULES = "showRules()"
. . . def _get_all_rules(self) -> DNSdistRulesTable:
"""Get list of all rules."""
return self._send_command(
command=_DNSdistCommands.GET_ALL_RULES ,
expected=DNSdistCommandType.SHOW_RULES,
)ИЛИ второй вариант:
называть команды в _DNSdistCommands так же как и в DNSdistCommandType
т.е. в явном виде будет прослежваться мапинг между самой командой и ее типом:
def _get_all_rules(self) -> DNSdistRulesTable:
"""Get list of all rules."""
return self._send_command(
command=_DNSdistCommands.SHOW_RULES,
expected=DNSdistCommandType.SHOW_RULES,
)There was a problem hiding this comment.
мне больше второй вариант нравится
| """Set up DNS server and DNS manager.""" | ||
| records = [] | ||
| if dns_settings.power_dns_settings is None: | ||
| raise DNSError("PowerDNS settings is not set.") |
There was a problem hiding this comment.
добавь более конкретный exception
4 participants
Задачи: 1057, 1058
Произведена замена DNS сервера с Bind9 на PowerDNS, состоящий из двух компонентов: Authoritative Server и Recursor. Произведен рефакторинг компонентов DNS сервера. Вся логика, относящаяся к Bind9, была удалена, за исключением некоторым датаклассов по просьбе Егора. Вся логика была воспроизведена в соответствии с новым DNS сервером за исключением пары фич: включение/отключение DNSSEC опции DNS сервера - теперь это опция относится к зоне, соответственно, временно удален функционал получения и изменения параметров DNS сервера, удалена возможность перезагружать DNS сервер и отдельную зону, так как в этом больше нет необходимости и на фронт этот функционал так и не был выведен.
Сделано:
UPD: Добавлен dnsdist