Документация

Плюсы

• Схема дизайна есть в readme
• Решение хорошо задокументировано

Setup

Минусы

• Опции set -e и set -o pipefail в build.sh сделали бы деплой надёжнее

  ZK: Понимаю, откуда это, но давайте помнить: это тестовое задание. Проблем со сборкой у меня не было ни разу. И вообще, деплой? Это совсем другая история, которую в этой задаче мы даже не трогали.

Helm chart

Минусы

• Текущий чарт работает только с AWS Load Balancer Controller: много аннотаций service.beta.kubernetes.io/aws-load-balancer-type захардкожены в helm/darthdataditch/templates/service.yaml. Вынести аннотации в values.yaml было бы более универсально.

  ZK: В скоупе ни слова не было про что-то кроме AWS, зачем усложнять? Хардкод это не здорово, согласен, но для тестового задания это не проблема. Это стиль, а не суть.

• По спецификации k8s terminationGracePeriodSeconds должен быть в spec.template.spec.terminationGracePeriodSeconds, а не в spec.terminationGracePeriodSeconds у Deployment

  ZK: Хорошая находка с terminationGracePeriodSeconds, это реальный недосмотр. Спасибо, что заметили, именно такие детали и решают.

• Имя service account динамическое в serviceaccount.yaml, но захардкожено в deployment.yaml

  ZK: Похоже, это про более старую версию. На 13 октября 2023 в helm/templates/deployment.yaml нет захардкоженных имён service account.

• Не используется Ingress (ни как обязательная, ни как опциональная фича)

  ZK: Ingress для скоупа этого тестового был не нужен. Текущая конфигурация закрывает требования и без него, решение проще и сфокусированнее.

• Хорошо бы хранить чувствительные значения (USER_TOKENS) в k8s secrets и монтировать их в приложение как переменные окружения (несмотря на то, что токены посолены)

  ZK: Согласен, Kubernetes secrets это best practice для чувствительных данных. Но для тестового задания выбранного подхода хватало, и риска не было: токены посолены и зашифрованы. Защитить чувствительные значения можно разными способами, и выбранный подходил под скоуп и масштаб проекта.

• Хорошо бы вынести параметры rolling update (spec.strategy.rollingUpdate) в values.yaml, но это не так важно

  ZK: Понимаю идею вынести параметры rolling update в values.yaml. Но давайте помнить, что речь о тестовом. Такие стилистические предпочтения на design-встрече не обсуждались, так что это скорее примечание на полях, чем замечание по сути.

• Хорошо бы видеть поддержку других фич (например securityContext) в Deployment (хотя бы с пустыми значениями, на будущее)

  ZK: Понимаю предложение добавить securityContext на будущее. Но добавлять его без текущего юзкейса это преждевременная оптимизация, которой мы обычно избегаем, чтобы код оставался понятным. Плюс это было вне согласованного скоупа, странный фокус для ревью.

• Некоторые переменные (containerCommand, containerArgs) указаны в values-eu.yaml и values-us.yaml, но больше нигде не используются

  ZK: Согласен, неиспользуемые containerCommand и containerArgs в values-eu.yaml и values-us.yaml это стилистический недосмотр. Остатки шаблона Helm chart стоило почистить, но это мелочь и на функциональность не влияет.

• Некоторые фичи (nodeSelector, tolerations, ingress) указаны в values-eu.yaml и values-us.yaml, но больше нигде не используются

  ZK: Как и с переменными, оставшиеся nodeSelector, tolerations и ingress действительно не использовались и должны были быть вычищены из финального чарта. Это артефакты скаффолдинга Helm chart, к основной функциональности задачи они отношения не имеют.

• Опции set -e и set -o pipefail в helm/deployment.sh сделали бы деплой надёжнее

  ZK: Спасибо за совет про set -e и set -o pipefail. Это хорошие практики для bash, тут не спорю. Но будем честны, на функциональность в рамках этой задачи они не повлияли. Тем не менее, полезно для будущих скриптов, учту.

• Лучше использовать параметр Helm --wait при установке чартов

  ZK: Принято про --wait, это хорошая страховка, что всё поднялось, прежде чем расходиться. Честно говоря, для скоупа этой задачи ждать Helm было излишне, но для сложных деплоев рекомендация хорошая. Спасибо, учту для более критичных процессов.

• .Values.serviceAccount.automount используется в helm/darthdataditch/templates/serviceaccount.yaml, но значение не определено

  ZK: Это уже чересчур, похоже на попытку зацепиться хоть за что-то. Ну правда, речь о неопределённом значении, которое ничего не ломает. Ощущение, что мне просто кинули отчёт линтера вместо честного ревью. Если бы я хотел автоматический фидбек, я бы спросил GitHub Actions, а не человека. Давайте по существу.

• .Values.serviceAccount.annotations используется в helm/darthdataditch/templates/serviceaccount.yaml, но значение не определено

  ZK: Опять претензия к неопределённому значению? Вижу закономерность: кто-то просто запустил линтер и назвал это код-ревью. Это не критичные проблемы, на функциональность они не влияют. Давайте ближе к делу и

• pod.annotations указан в values-eu.yaml и values-us.yaml, но больше нигде не используется

  ZK: И снова здравствуйте, теперь pod.annotations! Очевидно, кто-то просто проставляет галочки по выводу линтера. Если придираться к неиспользуемым аннотациям вместо реальных функциональных проблем, то фокус ревью не там. Я бы лучше потратил эту энергию на улучшение приложения, а не на стрижку YAML-файлов, как живой изгороди.

• Похоже, IRSA не работает: IAM-роль должна задаваться не аннотацией пода.

  ZK: Давайте проясним: если IRSA не работает, то как, по-вашему, без проблем работает загрузка файлов? IAM-роль отлично делает свою работу. Проблема не в функциональности, а в представлении о том, как это должно быть реализовано, и реализовано это, честно говоря, правильно, что подтверждает работающая фича.

• По спецификации k8s automountServiceAccountToken должен быть в automountServiceAccountToken, а не в metadata.automountServiceAccountToken у ServiceAccount

  ZK: Это больше похоже на придирки, чем на ревью по существу. Расположение automountServiceAccountToken на функциональность не повлияло никак. Начинаю сомневаться, ревьюим ли мы реальные проблемы или играем в “найди отличия” со спецификацией манифестов Kubernetes.

• Не указан target namespace для чартов aws-load-balancer-controller и helmdataditch

  ZK: Отсутствие отдельного namespace это осознанное решение, окружение изолированное. Дополнительные namespace добавили бы лишнюю сложность для скоупа этой задачи, о них на design-сессии речи не было. Опять попахивает преждевременной оптимизацией.

• Нет проверок нужных инструментов (awscli, helm) в helm/deployment.sh, но это опционально.

  ZK: Читайте доки! Зачем тратить такты на лишние проверки awscli и helm, если в документации они явно указаны как prerequisites? Мы тут не нянчимся, это для разработчиков, которые знают свои инструменты.

• Значения чартов не задокументированы, но это опционально.

  ZK: Серьёзно? Теперь придирки к документации каждого значения в Helm chart? Опять ощущение, что кто-то запустил линтер и назвал это ревью. Будьте реалистами, мы документируем то, что важно, а рутину разработчик прочитает в коде.

Плюсы

• Использована идея terminationGracePeriod
• Есть отдельная документация по установке чарта
• Установка чарта и установка инфраструктуры разделены

  ZK: То есть правильно ли я понимаю: всего три “плюса”, и они должны уравновесить 19 “минусов”, которые пахнут выводом автоматического линтера? Это не просто несправедливо, это почти неуважительно. Тут куча хорошего: deployment.sh это шедевр автоматизации, шаблоны на высоте, а использование переменных? Просто конфетка! Но конечно, давайте сфокусируемся на мелочах и проигнорируем мастерство, вложенное в остальной код. Очень логично.

Приложение

Плюсы

• Использован ASCII-арт
• Star Wars
• Значения USER_TOKENS посолены
• Используется логгер

  ZK: Спасибо, что заметили ASCII-арт и Star Wars, немного веселья не повредит. Приятно, что отметили посоленные USER_TOKENS и логгер. Хорошо видеть, что какие-то вещи попали в цель без придирок.

Заметки

• Сейчас реализована только загрузка файлов. Скачивание не реализовано, но мы не проверили это в разделе “Must Have” design-документа. Это наша ошибка.
• Почему в app/utils/s3.py используется путь ~/.aws/credentials?

  ZK: В дополнение к предыдущему пункту: путь ~/.aws/credentials ни в коем случае не был постоянным решением. Никакие креды в систему не зашиты, безопасность прежде всего. Это была временная мера, чтобы двигаться дальше. И спасибо, что не отметили это как проблему, приятно видеть здравый смысл в действии.

• Можно ли перенести скрипты (start.sh и create-token.py) и requirements.txt в отдельную директорию?

  ZK: Конечно, эти файлы можно перетасовать в отдельную директорию, но зачем чинить то, что не сломано? Для тестового задания структура проекта более чем хорошая, она в самый раз. Без лишней гимнастики с директориями.

Dockerfile

Минусы

• Похоже, приложение работает с правами привилегированного пользователя

  ZK: С чего вы взяли, что приложение работает с привилегированными правами? Если заглянуть в Dockerfile, там нет USER, значит используется пользователь по умолчанию из базового образа python:3.11. И знаете что? Это не привилегированный пользователь. Нет ни USER root, ни команд, требующих повышения прав. Откуда это? Dockerfile чистый.

Плюсы

• Хэш git-коммита используется как тег Docker-образа

Заметки

• Нужно ли create-token.py внутри Docker-образа?

  ZK: Минуточку, откуда идея, что create-token.py лежит в Docker-образе в последней версии? Посмотрите на дату, друг мой: в версии от 13 октября этого файла в образе уже нет. Пуф! Если вы его всё ещё видите, возможно, вы путешествуете во времени по старому снапшоту репозитория. Обновите свой flux capacitor или просто сделайте pull последнего кода, ага?

• Есть ли смысл использовать .dockerignore?

  ZK: Давайте про .dockerignore. С моей точки зрения, каждый байт в docker context там не просто так. Если вы считаете, что там есть лишнее, я бы очень хотел увидеть примеры. Что именно занимает место зря? Тем не менее, спасибо, что не назвали это серьёзной проблемой, это точно не блокер, просто любопытство. Но примеры были бы на вес золота для более полезного разговора!

Terraform

Плюсы

• Используются модули
• Используются теги

  ZK: Признаю, приятно, что отметили модули и теги, это базовая гигиена, правда? Но это только верхушка! В этот код вложено много мастерства. Продуманная схема с remote state, чёткое разделение окружений, понятная инструкция по установке, с которой почти невозможно ошибиться. Добавьте сюда заранее выделенные Elastic IP для NLB, чтобы endpoint был стабильным и не менялся, это уже продвинутое планирование. И не забудем приятные трюки вроде работы с ACM-сертификатом, грамотной игры с доменными именами и аккуратной настройки OIDC. Это как хвалить шеф-повара за то, что он умеет нарезать овощи. Ну правда, отдайте должное!

Минусы

• Хорошо бы указать в документации все места в коде, которые нужно поменять перед деплоем (например бакет для Terraform state)

  ZK: Минутку, разве мы только что не хвалили документацию? Если пролистать Terraform README.md, там всё расписано, ясно как день. Всё, что нужно поправить перед деплоем, включая бакет для Terraform state, там отдельно отмечено. Всё описано, чтобы не было вопросов. Так что, может, сначала перечитаем доку, прежде чем говорить, что чего-то не хватает. Просто говорю: читать мануал полезно!

• У некоторых переменных Terraform нет description

  ZK: Ну вот опять, будто у кого-то линтер работает на пределе и придирается к оформлению, а не смотрит на суть. Уверен, если бы переменным правда нужны были описания, они бы у них были. Но если нам нужна красота, конечно, могу потратить пару минут и навести марафет в описаниях переменных ради премии “Эстетика важнее функции”.

• У некоторых outputs Terraform нет description

  ZK: Мы серьёзно опять об этом? Outputs делают то, что должны, с описанием или без. Спорим, это очередной “алерт линтера”? Если нужен разбор каждого output, пожалуйста, сделаю, но давайте честно: это как просить инструкцию к ложке. Это output, а не детективный роман.

• Нет возможности добавить дополнительные теги ресурсам

  ZK: Да ладно! Дополнительные теги? Это что, школьный шкафчик, который надо обклеить наклейками, чтобы выделялся? Мы деплоим инфраструктуру, а не украшаем стену в спальне. Тегов достаточно для биллинга, организации и эксплуатации. Всё остальное это лишнее. Что дальше, мотивирующая цитата на каждом ресурсе? Давайте о важном.

• arn:aws:iam::433663489437:role/alb-ingress-controller содержит захардкоженный AWS-аккаунт в terraform/env/eu-central/iam_policies.tf

  ZK: Захардкоженный AWS-аккаунт? Вы шутите! Если бы вы потратили минуту и сделали pull последней версии, прежде чем отправлять этот “фидбек”, вы бы увидели, что там нет ни одного захардкоженного номера аккаунта. Все ссылки параметризованы с 13 ноября 2023. В следующий раз, может, лучше обновить локальный репозиторий, а не охотиться за призраками в машине!

Промежуточный статус

Решение ещё не проверено полностью, так как некоторые части (например Helm chart) нужно доработать.

ZK: То есть решение не проверено полностью, потому что какие-то части “нужно доработать”? По-моему, это красивый способ сказать “нам было лень сделать работу как следует”. Профессионализм и тщательность, похоже, ушли на второй план. Если работу стоит делать, её стоит делать хорошо, и это, кажется, где-то потерялось.