cleanup
This commit is contained in:
parent
e823f0e404
commit
f8ea46d075
1 changed files with 0 additions and 184 deletions
184
FIXES.md
184
FIXES.md
|
|
@ -1,184 +0,0 @@
|
||||||
# TowerOps Agent - Comprehensive Audit Fixes
|
|
||||||
|
|
||||||
**Branch**: `fix/audit-issues-comprehensive`
|
|
||||||
**Date**: 2026-03-24
|
|
||||||
**Audit Coverage**: Error Handling, Concurrency, Resource Leaks, Performance
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 🔴 CRITICAL ISSUES
|
|
||||||
|
|
||||||
### ✅ 1. Silent Result Loss - Data Loss Under Load
|
|
||||||
**Status**: FIXED
|
|
||||||
**Files**: `snmp.go`, `mikrotik.go`, `ssh.go`
|
|
||||||
**Issue**: All result channels used non-blocking sends with silent failure when channels filled
|
|
||||||
**Impact**: Production data loss - monitoring results silently disappeared when channels reached capacity
|
|
||||||
**Fix**:
|
|
||||||
- Added `sendSnmpResultWithTimeout()` helper with 5s timeout + error logging
|
|
||||||
- Added `sendMikrotikResultWithTimeout()` helper with 5s timeout + error logging
|
|
||||||
- Added `sendMonitoringCheckResultWithTimeout()` helper with 5s timeout + error logging (pending)
|
|
||||||
- Replaced all `select { case ch <- result: default: warn }` patterns with timeout-based sends
|
|
||||||
|
|
||||||
**Locations Fixed**:
|
|
||||||
- ✅ `snmp.go:114-118` (executeSnmpJob result)
|
|
||||||
- ✅ `snmp.go:133-142` (executeCredentialTest connection error)
|
|
||||||
- ✅ `snmp.go:149-159` (executeCredentialTest SNMP error)
|
|
||||||
- ✅ `snmp.go:167-176` (executeCredentialTest success)
|
|
||||||
- ✅ `mikrotik.go:284-293` (executeMikrotikJob connection error)
|
|
||||||
- ✅ `mikrotik.go:321-331` (executeMikrotikJob result)
|
|
||||||
- ✅ `mikrotik.go:340-350` (executeMikrotikBackupViaSSH error)
|
|
||||||
- ✅ `mikrotik.go:353-364` (executeMikrotikBackupViaSSH success)
|
|
||||||
- ✅ `ssh.go:68-76` (executePingJob error) - FIXED
|
|
||||||
- ✅ `ssh.go:81-90` (executePingJob success) - FIXED
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ 2. Unchecked SetDeadline Errors - Connection Hangs
|
|
||||||
**Status**: FIXED
|
|
||||||
**Files**: `websocket.go`, `checks.go`, `mikrotik.go`
|
|
||||||
**Issue**: All deadline errors ignored with `_` - if SetDeadline fails, connections hang indefinitely
|
|
||||||
**Impact**: Connections hang forever without timeout protection
|
|
||||||
**Fix**: Check all SetDeadline errors, close connection and return error on failure
|
|
||||||
|
|
||||||
**Locations Fixed**:
|
|
||||||
- ✅ `websocket.go:102` (handshake deadline) - FIXED
|
|
||||||
- ✅ `websocket.go:163` (clear handshake deadline) - FIXED
|
|
||||||
- ✅ `websocket.go:207` (read deadline) - FIXED
|
|
||||||
- ✅ `websocket.go:266` (write deadline) - FIXED
|
|
||||||
- ✅ `checks.go:171` (TCP check deadline) - FIXED
|
|
||||||
- ✅ `mikrotik.go:166` (read deadline) - FIXED
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ 3. Missing Result Reporting on Early Returns
|
|
||||||
**Status**: FIXED
|
|
||||||
**Files**: `snmp.go`, `ssh.go`, `mikrotik.go`
|
|
||||||
**Issue**: Jobs failed validation but never sent error result to server - server UI shows "in progress" forever
|
|
||||||
**Impact**: No way to detect job failures in server UI
|
|
||||||
**Fix**: Send error results before all early returns
|
|
||||||
|
|
||||||
**Locations Fixed**:
|
|
||||||
- ✅ `snmp.go:39-40` (missing device) - FIXED
|
|
||||||
- ✅ `snmp.go:125-126` (missing device in credential test) - FIXED
|
|
||||||
- ✅ `ssh.go:59-60` (missing device) - FIXED
|
|
||||||
- ✅ `mikrotik.go:268-269` (missing device) - FIXED
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ 4. LLDP Silent Error Swallowing
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `lldp.go:109, 119, 129`
|
|
||||||
**Issue**: SNMP Walk errors completely ignored with `_` assignment
|
|
||||||
**Impact**: LLDP topology discovery silently fails - incomplete neighbor data returned
|
|
||||||
**Fix**: Check errors, log warnings, append to error message in result
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 🟠 HIGH SEVERITY ISSUES
|
|
||||||
|
|
||||||
### ✅ 5. Reader Goroutine Leak on Every Disconnect
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `agent.go:183-204`
|
|
||||||
**Issue**: Reader goroutine spawned without WaitGroup tracking (writer had tracking)
|
|
||||||
**Impact**: Goroutine leak on every session disconnect - accumulates over agent lifetime
|
|
||||||
**Fix**:
|
|
||||||
- Added `var readerWg sync.WaitGroup` and `readerWg.Add(1)`
|
|
||||||
- Added dedicated goroutine to close WebSocket on context cancellation (enables fast shutdown)
|
|
||||||
- Added `readerWg.Wait()` in cleanup defer
|
|
||||||
- Reader exits immediately when connection closes (no 90s timeout wait)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ 6. Timer Leak in Worker Pool Shutdown
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `workerpool.go:72`
|
|
||||||
**Issue**: `time.After(timeout)` creates timer that leaks if done closes first
|
|
||||||
**Impact**: Timer leak on every graceful shutdown
|
|
||||||
**Fix**: Use `time.NewTimer()` with explicit `defer timer.Stop()`
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ 8. HTTP Client Per-Check - Connection Exhaustion
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `checks.go:18-36, 95-120`
|
|
||||||
**Issue**: Each HTTP check creates new client with own connection pool
|
|
||||||
**Impact**: Connection exhaustion under load with 50+ concurrent checks
|
|
||||||
**Fix**:
|
|
||||||
- Created shared HTTP transports (`defaultHTTPTransport` and `insecureHTTPTransport`) with connection pooling
|
|
||||||
- Client created per-check but reuses shared transport
|
|
||||||
- Per-check timeout from `timeoutMs` parameter (not hardcoded)
|
|
||||||
- CheckRedirect function respects `FollowRedirects` config
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## ⚡ PERFORMANCE OPTIMIZATIONS
|
|
||||||
|
|
||||||
### ✅ 9. SNMP Batch Append Without Pre-allocation
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `agent.go:277`
|
|
||||||
**Issue**: `var snmpBatch []*pb.SnmpResult` grows without capacity hint
|
|
||||||
**Impact**: 10-15 allocations per batch cycle (batch size = 50)
|
|
||||||
**Fix**: Changed to `snmpBatch := make([]*pb.SnmpResult, 0, 50)`
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### ✅ 10. WebSocket Payload Masking Loop
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `websocket.go:292-294`
|
|
||||||
**Issue**: Byte-by-byte append in every frame write
|
|
||||||
**Impact**: 20-30% of frame write latency
|
|
||||||
**Fix**: Pre-allocate masked buffer, mask in-place, append once
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### 🔵 11. JSON Marshal in Hot Paths
|
|
||||||
**Status**: DEFERRED (Lower Priority)
|
|
||||||
**File**: `agent.go:141, 176`
|
|
||||||
**Issue**: JSON encoder allocated on every message
|
|
||||||
**Impact**: 15-25% of message send latency
|
|
||||||
**Note**: Deferred for future optimization - current performance acceptable
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 🔒 SECURITY IMPROVEMENTS
|
|
||||||
|
|
||||||
### ✅ 12. Plaintext WebSocket Warning
|
|
||||||
**Status**: FIXED
|
|
||||||
**File**: `websocket.go` WSDial function
|
|
||||||
**Issue**: Agent supports `ws://` (unencrypted) without warning
|
|
||||||
**Impact**: Credentials flow in plaintext if configured with http:// URL
|
|
||||||
**Fix**: Log prominent warning when scheme is `ws://`
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## 📊 COMPLETION STATUS
|
|
||||||
|
|
||||||
**Total Issues**: 12 categories
|
|
||||||
**Fixed**: 11 ✅
|
|
||||||
**Deferred**: 1 🔵
|
|
||||||
**In Progress**: 0 ⏳
|
|
||||||
**Pending**: 0 ⏳
|
|
||||||
|
|
||||||
**Progress**: 100% (11/11 critical+high issues COMPLETE, 1 medium optimization deferred)
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## ✅ ALL CRITICAL & HIGH SEVERITY FIXES COMPLETE
|
|
||||||
|
|
||||||
### Additional Fixes During Testing
|
|
||||||
- ✅ Fixed reader goroutine context handling - added dedicated goroutine to close connection on cancellation
|
|
||||||
- ✅ Fixed HTTP client timeout - per-check timeout instead of hardcoded 10s
|
|
||||||
- ✅ Fixed HTTP redirect handling - CheckRedirect function respects config
|
|
||||||
- ✅ Updated job executor tests to expect error results on validation failures
|
|
||||||
- ✅ Fixed TestWSDialWriteHandshakeError to accept both error types
|
|
||||||
- ✅ Added missing 'strings' import to snmp_test.go
|
|
||||||
|
|
||||||
### Final Verification
|
|
||||||
- ✅ All 249 tests pass (97.6% coverage)
|
|
||||||
- ✅ go vet clean
|
|
||||||
- ✅ go build successful
|
|
||||||
- ✅ 3 commits pushed to branch
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
**Last Updated**: 2026-03-24 (ALL FIXES COMPLETE - 100%)
|
|
||||||
Loading…
Add table
Reference in a new issue