initial commit
This commit is contained in:
280
TODO.md
Normal file
280
TODO.md
Normal file
@@ -0,0 +1,280 @@
|
||||
# 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**: В целом правильно, но не проверяется в циклах
|
||||
Reference in New Issue
Block a user