281 lines
18 KiB
Markdown
281 lines
18 KiB
Markdown
# TODO
|
||
|
||
## Completed
|
||
+ скип лист коллекторов в конфиге
|
||
+ кэш для метрик
|
||
+ fetch + build
|
||
+ wrapper cache
|
||
+ клиент кинетика поддерживает сессию
|
||
+ **Механизм остановки background updaters**: Context-based cancellation с graceful shutdown
|
||
+ **Background updater для всех метрик**: Реализован generic updater для всех коллекторов
|
||
- Работает для system, hotspot, internet, interface, interface_stats, process
|
||
- Учитывает `skip_collectors` для каждого устройства
|
||
- Graceful shutdown через context cancellation и WaitGroup
|
||
|
||
## High Priority
|
||
|
||
### Critical Issues
|
||
*(Пусто - все критические проблемы решены или перенесены в планируемые фичи)*
|
||
|
||
### Configuration & Architecture
|
||
- **Хардкод пути к конфигу**: Нет CLI флагов для указания файла конфигурации
|
||
- **Device timeout не учитывается**: HTTP клиент использует хардкод 5s таймаут
|
||
- **Dockerfile path mismatch**: Dockerfile ссылается на `./cmd/exporter`, но реальная точка входа — это `main.go` в корне
|
||
|
||
## Medium Priority
|
||
|
||
### Features
|
||
- Привести в порядок логгирование, добавить разные уровни (Error, Info, Debug), добавить в конфиг параметр отображения логов.
|
||
- медиа коллектор
|
||
- обработка ошибок для offline устройств (улучшенная)
|
||
- **Метрики ошибок экспортера**: Экспортировать метрики о работе самого экспортера
|
||
- `keenetic_scrape_success{device, collector}` - успешность последнего scrape
|
||
- `keenetic_scrape_duration_seconds{device, collector}` - длительность сбора
|
||
- `keenetic_scrape_errors_total{device, collector}` - счетчик ошибок
|
||
- Позволит настраивать алерты на проблемы со сбором метрик
|
||
- Улучшит observability и диагностику проблем
|
||
- **Строго типизированный кеш с дженериками**: Улучшение type safety
|
||
- ✅ Текущее состояние: Добавлены runtime проверки type assertions с обработкой ошибок
|
||
- 🎯 Цель: Переделать на `TypedCache` с `CacheEntry[T any]` для compile-time type safety
|
||
- Уберет необходимость runtime проверок и сделает код более безопасным
|
||
- См. вариант 1 из предыдущих обсуждений
|
||
|
||
### Security & Production Readiness
|
||
- **Credentials в git**: config.yaml содержит тестовые пароли
|
||
- Для production: использовать environment variables или secrets manager
|
||
- Добавить config.yaml.example без реальных credentials
|
||
- ⚠️ Текущее состояние допустимо только для development с тестовыми учетками
|
||
|
||
### Testing
|
||
- **Нет тестов**: Нулевое покрытие тестами
|
||
- Unit tests для коллекторов
|
||
- Integration tests для Keenetic клиента
|
||
- Tests для кеширования
|
||
|
||
## Low Priority
|
||
|
||
### Keenetic Client Code Quality Issues
|
||
|
||
Найдено при анализе `internal/keenetic/client.go` (2025-10-16)
|
||
|
||
#### Critical Issues (безопасность/корректность)
|
||
|
||
1. **Race condition при повторной аутентификации** (`client.go:249-261`)
|
||
- **Проблема**: Если несколько горутин одновременно получат HTTP 401, все они параллельно вызовут `authenticate()`. Это создаст множественные конкурирующие запросы к `/auth` endpoint.
|
||
- **Локация**: Метод `doGet()`, блок обработки `StatusUnauthorized`
|
||
- **Последствия**: Непредсказуемое поведение, лишние запросы к API, возможные конфликты сессий
|
||
- **Решение**: Использовать `sync.Mutex` или `sync.Once` для синхронизации процесса re-authentication
|
||
- **Пример**:
|
||
```go
|
||
var authMutex sync.Mutex
|
||
authMutex.Lock()
|
||
defer authMutex.Unlock()
|
||
if err := c.authenticate(ctx, c.login, c.password); err != nil { ... }
|
||
```
|
||
|
||
2. **Race condition на полях Client** (`client.go:127-133`)
|
||
- **Проблема**: Поля `login`, `password`, `Hostname` записываются в методах (`authenticate()`, `fetchHostname()`), которые могут вызываться из разных горутин без синхронизации
|
||
- **Локация**: Структура `Client` и методы, изменяющие её поля
|
||
- **Последствия**: Data races, неопределенное поведение, возможные паники
|
||
- **Решение**: Добавить `sync.RWMutex` для защиты полей или сделать их write-once (инициализация только в конструкторе)
|
||
- **Детекция**: `go test -race` покажет эти гонки
|
||
|
||
3. **Отсутствие ограничения размера тела ответа при ошибках** (`client.go:264`)
|
||
- **Проблема**: `io.ReadAll(resp.Body)` читает всё тело без ограничений при HTTP ошибках
|
||
- **Локация**: Блок обработки `resp.StatusCode >= 400` в методе `doGet()`
|
||
- **Последствия**:
|
||
- Потенциальная DoS атака через огромное тело ответа
|
||
- OOM если сервер вернет гигабайты данных
|
||
- Чувствительные данные могут попасть в логи
|
||
- **Решение**: Использовать `io.LimitReader(resp.Body, maxBodySize)` (например, 1MB)
|
||
- **Пример**:
|
||
```go
|
||
limitedBody := io.LimitReader(resp.Body, 1024*1024) // 1MB max
|
||
data, err := io.ReadAll(limitedBody)
|
||
if err != nil {
|
||
return fmt.Errorf("failed to read error body: %w", err)
|
||
}
|
||
```
|
||
|
||
4. **Недостаточная валидация входных данных**
|
||
- **Проблема в `NewClient()`** (`client.go:135-145`): Не проверяется что URL имеет scheme (http/https) и host
|
||
- **Проблема в `Init()` и `authenticate()`**: Нет проверки что `login` и `password` не пустые
|
||
- **Локация**: Конструктор и методы инициализации
|
||
- **Последствия**: Невалидные клиенты могут быть созданы, приводя к ошибкам позже
|
||
- **Решение**: Добавить валидацию в `NewClient()`:
|
||
```go
|
||
if parsed.Scheme != "http" && parsed.Scheme != "https" {
|
||
return nil, fmt.Errorf("URL must have http or https scheme")
|
||
}
|
||
if parsed.Host == "" {
|
||
return nil, fmt.Errorf("URL must have a host")
|
||
}
|
||
```
|
||
- **Решение для credentials**: Проверять в `Init()` перед вызовом `authenticate()`
|
||
|
||
#### Serious Issues (надежность)
|
||
|
||
5. **Игнорируется timeout из конфигурации** (`client.go:139`)
|
||
- **Проблема**: HTTP клиент всегда использует хардкод `Timeout: 5 * time.Second`, несмотря на то что в config.yaml есть поле `timeout` для каждого устройства
|
||
- **Локация**: Инициализация `httpClient` в `NewClient()`
|
||
- **Последствия**: Невозможно настроить timeout для медленных устройств или быстрых сетей
|
||
- **Решение**: Принимать timeout как параметр в `NewClient()` или добавить метод `SetTimeout()`
|
||
- **Связано**: High Priority issue "Device timeout не учитывается"
|
||
|
||
6. **Игнорирование ошибки создания cookiejar** (`client.go:136`)
|
||
- **Проблема**: `jar, _ := cookiejar.New(nil)` игнорирует потенциальную ошибку
|
||
- **Локация**: Инициализация в `NewClient()`
|
||
- **Последствия**: Нарушение best practice, теоретически может скрыть проблему
|
||
- **Решение**: Обрабатывать ошибку:
|
||
```go
|
||
jar, err := cookiejar.New(nil)
|
||
if err != nil {
|
||
return nil, fmt.Errorf("failed to create cookie jar: %w", err)
|
||
}
|
||
```
|
||
|
||
7. **GetConnectedInterfaceStats() молча игнорирует ошибки** (`client.go:355-358`)
|
||
- **Проблема**: При ошибке получения статистики интерфейса просто `continue`, без логирования
|
||
- **Локация**: Цикл по интерфейсам в `GetConnectedInterfaceStats()`
|
||
- **Последствия**: Невозможно отличить "интерфейс disconnected" от "ошибка API"
|
||
- **Решение**: Логировать ошибки или возвращать частичные результаты с map ошибок:
|
||
```go
|
||
type InterfaceStatsResult struct {
|
||
Stats map[string]InterfaceStats
|
||
Errors map[string]error
|
||
}
|
||
```
|
||
|
||
8. **GetProcessInfo() молча пропускает невалидные записи** (`client.go:298-300`)
|
||
- **Проблема**: При ошибке `json.Unmarshal` просто `continue`, нет информации о количестве пропущенных процессов
|
||
- **Локация**: Цикл парсинга процессов в `GetProcessInfo()`
|
||
- **Последствия**: Тихая потеря данных, сложно диагностировать проблемы с API
|
||
- **Решение**: Добавить счетчик ошибок или логирование
|
||
|
||
9. **Контекст не проверяется внутри длительных операций** (`client.go:343-363`)
|
||
- **Проблема**: В `GetConnectedInterfaceStats()` делается N запросов в цикле, но `ctx.Done()` не проверяется между итерациями
|
||
- **Локация**: Цикл по интерфейсам
|
||
- **Последствия**: Невозможно отменить долгую операцию, если context был cancelled
|
||
- **Решение**: Добавить проверку:
|
||
```go
|
||
for _, iface := range interfaces {
|
||
select {
|
||
case <-ctx.Done():
|
||
return nil, ctx.Err()
|
||
default:
|
||
}
|
||
// ... continue processing
|
||
}
|
||
```
|
||
|
||
#### Design Issues (проектирование)
|
||
|
||
10. **Неэффективная повторная аутентификация**
|
||
- **Проблема**: При каждом 401 делается полный цикл (GET /auth для получения challenge + POST /auth), даже если challenge еще валиден
|
||
- **Локация**: Логика re-authentication в `doGet()`
|
||
- **Последствия**: Лишние запросы к API, увеличенная latency
|
||
- **Решение**: Кешировать challenge с разумным TTL или использовать только POST при re-auth
|
||
|
||
11. **Отсутствие retry логики**
|
||
- **Проблема**: При временных сетевых ошибках (connection reset, timeout) сразу возвращается ошибка
|
||
- **Локация**: Все API методы
|
||
- **Последствия**: Низкая устойчивость к временным проблемам сети
|
||
- **Решение**: Добавить configurable retry с exponential backoff для idempotent операций
|
||
- **Библиотеки**: `github.com/cenkalti/backoff/v4` или встроенный механизм
|
||
|
||
12. **Нет метрик и observability**
|
||
- **Проблема**: Невозможно отследить важные события:
|
||
- Количество re-authentication попыток
|
||
- Время выполнения запросов (latency по endpoint)
|
||
- Частота ошибок по типам и endpoint
|
||
- Размер ответов API
|
||
- **Локация**: Весь клиент
|
||
- **Последствия**: Сложно диагностировать проблемы в production, нет visibility
|
||
- **Решение**: Интегрировать с Prometheus metrics или добавить structured logging
|
||
- **Связано**: Medium Priority issue "Метрики ошибок экспортера"
|
||
|
||
13. **Отсутствие метода Cleanup/Close**
|
||
- **Проблема**: `Client` владеет `httpClient` с connections pool и cookiejar, но нет способа явно освободить ресурсы
|
||
- **Локация**: Структура `Client`
|
||
- **Последствия**: Goroutine leaks в тестах, невозможность graceful cleanup
|
||
- **Решение**: Добавить метод:
|
||
```go
|
||
func (c *Client) Close() error {
|
||
c.httpClient.CloseIdleConnections()
|
||
return nil
|
||
}
|
||
```
|
||
|
||
#### Code Quality Issues (качество кода)
|
||
|
||
14. **Утечка ресурсов при двойном defer** (`client.go:247, 260`)
|
||
- **Проблема**:
|
||
```go
|
||
resp, err := doRequest()
|
||
defer resp.Body.Close() // defer на первый resp
|
||
if resp.StatusCode == http.StatusUnauthorized {
|
||
_ = resp.Body.Close() // явное закрытие
|
||
resp, err = doRequest() // переприсваивание переменной
|
||
defer resp.Body.Close() // defer на второй resp
|
||
}
|
||
```
|
||
- **Локация**: Метод `doGet()`
|
||
- **Последствия**: Путаница, потенциальное двойное закрытие, плохая читаемость
|
||
- **Решение**: Использовать именованные переменные (`resp1`, `resp2`) или рефакторить логику
|
||
|
||
15. **Смешанный язык в комментариях** (`client.go:250`)
|
||
- **Проблема**: Комментарий `// закрываем перед повтором` на русском языке
|
||
- **Локация**: Различные места в файле
|
||
- **Нарушение**: CLAUDE.md требует все комментарии на английском
|
||
- **Решение**: Перевести все комментарии на английский
|
||
|
||
16. **Потенциальная утечка credentials в логах** (`client.go:264`)
|
||
- **Проблема**: В error message включается `fullURL.String()`, который может содержать query параметры с токенами
|
||
- **Локация**: Обработка ошибок в `doGet()`
|
||
- **Последствия**: Чувствительные данные могут попасть в логи
|
||
- **Решение**: Sanitize URL перед логированием или использовать `fullURL.Redacted()`
|
||
|
||
17. **Недостаточная типизация для статусов**
|
||
- **Проблема**: Поля типа string для статусов легко опечатать:
|
||
```go
|
||
Connected string `json:"connected"` // "yes"/"no"
|
||
State string `json:"state"`
|
||
Link string `json:"link"`
|
||
```
|
||
- **Локация**: Структуры моделей данных
|
||
- **Последствия**: Легко сделать ошибку при сравнении ("Yes" vs "yes")
|
||
- **Решение**: Использовать custom types с константами:
|
||
```go
|
||
type ConnectionStatus string
|
||
const (
|
||
ConnectionStatusYes ConnectionStatus = "yes"
|
||
ConnectionStatusNo ConnectionStatus = "no"
|
||
)
|
||
```
|
||
|
||
18. **Неиспользуемые поля в структурах**
|
||
- **Проблема**: `InterfaceStats.LastOverflow` (`client.go:118`) не используется нигде
|
||
- **Локация**: Определения структур
|
||
- **Последствия**: Мертвый код, увеличивает cognitive load
|
||
- **Решение**: Удалить или задокументировать зачем поле сохранено
|
||
|
||
19. **Магические числа**
|
||
- **Проблема**: `5 * time.Second` (`client.go:139`), `400` (`client.go:263`)
|
||
- **Локация**: Различные места
|
||
- **Последствия**: Плохая читаемость и maintainability
|
||
- **Решение**: Использовать именованные константы:
|
||
```go
|
||
const (
|
||
defaultHTTPTimeout = 5 * time.Second
|
||
httpErrorStatusCode = 400
|
||
)
|
||
```
|
||
|
||
#### Общие рекомендации
|
||
|
||
- **Добавить линтеры**: `golangci-lint` с конфигом покажет большинство этих проблем
|
||
- **Race detector**: `go test -race` для всех тестов
|
||
- **Structured logging**: Заменить `fmt.Errorf` на proper logging с levels
|
||
- **Error wrapping**: Везде используется, это хорошо (Go 1.13+ style)
|
||
- **Context usage**: В целом правильно, но не проверяется в циклах
|