Fix provider review findings

2026-08-31 20:19:31 +03:00
parent 75c868e0b5
commit 86871498a2
13 changed files with 327 additions and 141 deletions
@@ -0,0 +1,173 @@
+# Code Review провайдера — Opus — 2026-08-31
+
+**Источник:** анализ и код-ревью через VS Code Copilot Chat
+**Статус:** анализ завершён; часть исправлений внесена 2026-08-31
+
+## Область анализа
+
+Проверены:
+
+- рукописное ядро провайдера в `provider/internal/core` и `provider/internal/resources_core`;
+- CRUD, state management и валидация;
+- HTTP-слой и `client.go`;
+- регистрация провайдера и TLS-настройки;
+- генераторы Go-ресурсов, YAML и build-пайплайн;
+- Python- и shell-скрипты;
+- gateway.
+
+## Критичные находки
+
+### 1. Отладочный лог с данными инстансов пишется в `/tmp` безусловно
+
+В `provider/internal/core/client.go:629-637` замыкание `debug()` в `FindInstanceByDisplayName` всегда пишет в `/tmp/nubes_find_debug.log` с правами `0644`. В лог попадают `instanceUid`, `displayName` и `serviceId`.
+
+Файл не защищён условием `NUBES_DEBUG_HTTP`, не ротируется и не очищается. Это создаёт риск раскрытия данных и неконтролируемого роста файла.
+
+**Рекомендация:** убрать постоянную запись либо включать её только через явный debug-флаг; использовать безопасный путь и контролируемую ротацию.
+
+### 2. Bearer-токен попадает в stderr при HTTP-отладке
+
+В `provider/internal/core/client.go:1100-1101` вызов `httputil.DumpRequestOut(req, ...)` выводит полный исходящий запрос вместе с заголовком `Authorization: Bearer <token>` при `NUBES_DEBUG_HTTP=1`.
+
+Токен может попасть в логи CI/CD или окружения выполнения.
+
+**Рекомендация:** перед дампом удалять или маскировать `Authorization`; не выводить секреты ни в одном режиме.
+
+### 3. В Python-скрипте сетевые вызовы выполняются без таймаутов
+
+В `scripts/check_cloud_instances.py:87-88` вызовы `self.session.get(...)` не передают `timeout=`. При зависании API процесс может ожидать ответ бесконечно.
+
+**Рекомендация:** добавить явные таймауты ко всем HTTP-вызовам и определить единое значение или конфигурационный параметр.
+
+## Существенные находки
+
+### 4. Retry сетевых ошибок применяется к POST-запросам
+
+В `provider/internal/core/client.go:1113-1120` при сетевой ошибке повторяется любой HTTP-метод, включая POST к `/instances` и `/instanceOperations`.
+
+Если сервер принял запрос, но ответ потерян, повтор может создать дубликат инстанса или операции. Идемпотентность POST не гарантирована.
+
+**Рекомендация:** ограничить retry идемпотентными методами либо использовать идемпотency key и явную серверную поддержку повторов.
+
+### 5. Ответ `401 Unauthorized` включён в retryable
+
+В `provider/internal/core/client.go:1150-1156` статус `401` считается повторяемым. Протухший или неверный токен приводит к трём попыткам с задержкой, маскируя исходную ошибку авторизации и увеличивая время отказа.
+
+**Рекомендация:** исключить `401` из retryable; возвращать ошибку авторизации сразу.
+
+### 6. Gateway раскрывает внутренние upstream-адреса
+
+В `gateway/server.js:60-71` корневой endpoint `/` и обработчик 404 возвращают наружу адреса `upstream` для маршрутов.
+
+Публичный ответ раскрывает внутреннюю топологию сервисов.
+
+**Рекомендация:** убрать `upstream` из публичных ответов; внутренние адреса оставлять только в серверных логах с необходимой санацией.
+
+### 7. Некорректное определение неуспешной операции в Python
+
+В `scripts/check_cloud_instances.py:187-189` используется сравнение `last_op.get("isSuccessful") == False`. При отсутствии поля возвращается `None`, поэтому состояние `OPERATION_FAILED` не определяется.
+
+**Рекомендация:** использовать проверку `is False` либо явно обрабатывать отсутствие ключа согласно контракту API.
+
+## Умеренные находки
+
+### 8. Retry-логика дублируется в трёх местах
+
+В `provider/internal/core/client.go:777-905` похожие циклы retry присутствуют в `doRequest`, `GetInstanceState` и `GetInstanceStateRaw`.
+
+Дублирование увеличивает риск расхождения поведения и повторного появления ошибок безопасности.
+
+**Рекомендация:** вынести общую retry-логику в единый внутренний helper с параметрами метода, таймаутов и политики повторов.
+
+### 9. Пагинация имеет тихий предел 10 000 инстансов
+
+В fallback-ветке `FindInstanceByDisplayName` (`provider/internal/core/client.go:747-749`) поиск прекращается после `page > 100` при размере страницы `100`.
+
+При большем количестве инстансов совпадение может не быть найдено без предупреждения.
+
+**Рекомендация:** убрать произвольный предел либо возвращать диагностируемую ошибку/предупреждение при достижении лимита.
+
+### 10. Ошибка `gofmt` не останавливает генерацию
+
+`FormatSourceOrWarn` в `TOOLS/resource-generator/writers.go:61` при ошибке форматирования только выводит предупреждение и записывает исходник.
+
+В результате pipeline может сохранить неформатированный или потенциально некомпилируемый Go-код.
+
+**Рекомендация:** считать ошибку форматирования фатальной для генерации либо выполнять последующую обязательную компиляционную проверку.
+
+### 11. Секрет передаётся в командной строке shell-скрипта
+
+В `TOOLS/s3_notification_example.sh:74` значение `SECRET_KEY` передаётся аргументом в `mc alias set`.
+
+Секрет может быть виден через `ps` или аналогичный список процессов.
+
+**Рекомендация:** использовать механизм передачи секрета через stdin, переменную окружения, конфигурационный файл с безопасными правами или другой поддерживаемый секретный канал.
+
+## Дополнительные замечания
+
+- В `provider/internal/core/client.go` ссылка на `tools/gen_v2/generate_resources_v2.go` обновлена на актуальный путь `TOOLS/resource-generator/internal/templates/instance.go`.
+- В исходниках генератора (`TOOLS/resource-generator/internal/templates/*`, `TOOLS/resource-generator/internal/writers/writers.go`) метка `Code generated by tools/gen_v2` обновлена на `Code generated by TOOLS/resource-generator`.
+- Текущий `provider/internal/resources_gen/registry.go` обновлён на новую метку генератора.
+- `TOOLS/resource-generator/main.go` переведён на `run()` с корректным `exit code=1` и агрегированным отчётом по ошибкам записи ресурсов (instance/subresource/action).
+- Пути debug-логов в `provider/internal/core/client.go` переведены на `os.TempDir()` с override через `NUBES_DEBUG_DIR` (без хардкода `/tmp`).
+
+## Что выглядит хорошо
+
+- Сериализация операций на инстансе через `instanceMutexes` в `client.go` защищает от параллельных операций API.
+- TLS настроен с `MinVersion: TLS 1.2`; `InsecureSkipVerify` по умолчанию равен `false`.
+- `api_token` отмечен как `Sensitive: true` в схеме провайдера.
+- Канонизация JSON для сравнения state устраняет ложные различия из-за порядка ключей.
+
+## Итоговый статус
+
+| Находка | Статус |
+|---|---|
+| Безусловная запись данных инстансов в `/tmp` | Исправлено: debug gated + права `0600` |
+| Bearer-токен в HTTP debug dump | Исправлено: `Authorization` маскируется |
+| Python HTTP-вызовы без таймаутов | Исправлено: добавлен `REQUEST_TIMEOUT` |
+| Retry POST-запросов | Исправлено: retry сетевых ошибок только для GET |
+| `401` в retryable | Исправлено: исключён из retryable |
+| Раскрытие upstream в gateway | Исправлено: `upstream` удалён из root-ответа |
+| Ошибка определения `OPERATION_FAILED` | Исправлено: сравнение через `is False` |
+| Дублирование retry-логики | Исправлено: общий helper для чтения состояния |
+| Тихий предел пагинации | Частично исправлено: добавлена явная ошибка при достижении лимита |
+| Некритичная ошибка `gofmt` в генераторе | Исправлено: fail-fast при ошибке форматирования |
+| Секрет в аргументах shell-команды | Исправлено: исключена передача в argv |
+
+## Выполненные изменения (2026-08-31)
+
+- `provider/internal/core/client.go`:
+	- debug-лог `FindInstanceByDisplayName` теперь пишется только при `NUBES_DEBUG_HTTP=1`;
+	- права debug-логов снижены до `0600`;
+	- в stderr-дампе HTTP-запроса маскируется заголовок `Authorization`;
+	- retry сетевых ошибок ограничен методом `GET`;
+	- `401 Unauthorized` удалён из `isRetryable`;
+	- при достижении лимита fallback-пагинации возвращается явная ошибка.
+	- `GetInstanceState` и `GetInstanceStateRaw` переведены на общий helper `getInstanceStateWithRetry` с единым retry/HTTP-поведением.
+- `scripts/check_cloud_instances.py`:
+	- добавлен `REQUEST_TIMEOUT = 30` и применён ко всем `session.get(...)`;
+	- проверка failed-операции изменена на `is False`.
+- `gateway/server.js`:
+	- удалено поле `upstream` из публичного ответа `GET /`.
+- `TOOLS/resource-generator/internal/helpers/helpers.go`:
+	- `FormatSourceOrWarn` переведён на fail-fast: возвращает ошибку при сбое `gofmt`.
+- `TOOLS/resource-generator/internal/writers/writers.go`:
+	- все вызовы форматирования обрабатывают ошибку и прерывают генерацию.
+- `TOOLS/resource-generator/main.go`:
+	- убраны `panic` на первом сбое записи ресурса;
+	- добавлена агрегация ошибок генерации с отчётом по каждому ресурсу;
+	- завершение с `exit code=1` и человекочитаемым сообщением в stderr.
+- `TOOLS/resource-generator/internal/templates/instance.go`:
+	- обновлён marker генерации на `Code generated by TOOLS/resource-generator`.
+- `TOOLS/resource-generator/internal/templates/subresource.go`:
+	- обновлён marker генерации на `Code generated by TOOLS/resource-generator`.
+- `TOOLS/resource-generator/internal/templates/action.go`:
+	- обновлён marker генерации на `Code generated by TOOLS/resource-generator`.
+- `provider/internal/resources_gen/registry.go`:
+	- обновлён marker генерации на `Code generated by TOOLS/resource-generator`.
+- `scripts/s3_notification_example.sh`:
+	- убрана передача секрета в аргументах процесса;
+	- для `mc` используется временный `--config-dir` и переменная `MC_HOST_<alias>`.
+- `provider/internal/core/client.go`:
+	- debug log path переведён на `os.TempDir()`;
+	- добавлен override директории через `NUBES_DEBUG_DIR`.