test: Improve cmd/node-doctor test coverage to 76.4% #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implement comprehensive input validation to prevent command injection and path traversal attacks in disk and network remediators. Security Controls: - Path whitelisting: Only /tmp and /var/tmp allowed for cleanup - Interface name validation: Strict regex preventing shell metacharacters - Vacuum size format validation: Prevents parsing issues - Defense-in-depth: Config-time validation + safe command execution Implementation: - Created pkg/remediators/validation.go with validation functions - Integrated validation into disk.go (lines 185-187) - Integrated validation into network.go (lines 170-172) - Added 100+ security-focused test cases in validation_test.go - Documented security model in docs/security/command-validation.md Test Results: - Coverage: 81.0% (above 80% threshold) - All 108 tests passing - Linter: passing - QA review: A- Fixes #3859
Implements Task #3860 - Add ReDoS protection for log pattern regex compilation
with configurable timeout, complexity scoring, and runtime metrics tracking.
## Security Enhancements
- **Configurable Compilation Timeout**: 50-1000ms range (default: 100ms)
- Goroutine-based timeout prevents runaway regex compilation
- Clear error messages for timeout failures
- Validates timeout bounds during configuration
- **Multi-Layer ReDoS Protection**:
1. Pre-compilation safety validation (blocks dangerous patterns)
2. Complexity scoring (0-100 scale with weighted penalties)
3. Compilation timeout enforcement
4. Runtime metrics tracking
- **Pattern Safety Validation**: Blocks known ReDoS attack vectors
- Nested quantifiers: (a+)+, (.*)*
- Quantified adjacencies: .*.*, \d+\d+
- Deep nesting: >3 levels
- Exponential alternation: >5 branches under quantifiers
## Features
- **Complexity Scoring Algorithm**:
- Nested quantifiers: 40 points
- Quantified adjacency: 30 points
- Nesting depth: 10 points/level
- Alternation branches: 2 points/branch
- Basic quantifiers: 1 point each
- High complexity warning threshold: 60 points
- **Runtime Metrics**:
- ComplexityScore(): Pattern complexity (0-100)
- CompilationTime(): Actual compilation duration
- Automatic warnings for high-complexity patterns
- **Configuration Example**:
```yaml
monitors:
- name: log-pattern-health
type: custom-logpattern-check
config:
compilationTimeoutMs: 100 # Optional, default: 100ms
patterns: [...]
```
## Backward Compatibility
- Existing configurations work unchanged
- Default timeout (100ms) automatically applied
- Metrics tracked for all patterns
- Maximum pattern limit increased from 50 to 60
## Testing
- 7 comprehensive test functions (384 new test lines)
- 100% coverage of ReDoS protection code paths
- Tests cover:
- Timeout configuration validation
- Complexity score calculation
- Compilation time tracking
- High complexity warnings
- Backward compatibility
- Dangerous pattern rejection
- Configurable timeout usage
## Documentation
- docs/security/redos-protection.md - Comprehensive ReDoS protection guide
- docs/monitors.md - Enhanced with ReDoS protection details
- config/examples/custom-plugins.yaml - Example configurations
- docs/security/redos-protection-review-response.md - Review analysis
## Validation
- ✅ Full test suite passing (pkg/monitors/custom)
- ✅ Build verification successful
- ✅ Pipeline validation passed (make validate-pipeline-local)
- ✅ QA review: A+ rating (Exceptional quality)
- ✅ Security review: Comprehensive protection
- ✅ Principal Engineer review: Approved with future enhancements noted
## Performance Impact
- Compilation overhead: <1ms for simple patterns, <100ms for complex patterns
- Memory: ~40 bytes per pattern for metrics (negligible)
- No runtime performance degradation (pre-compiled patterns)
## Files Changed
- pkg/monitors/custom/logpattern.go: Core implementation (+55 lines)
- pkg/monitors/custom/logpattern_test.go: Comprehensive tests (+389 lines)
- docs/monitors.md: Enhanced documentation (+66 lines)
- docs/security/redos-protection.md: Security guide (new file)
- docs/security/redos-protection-review-response.md: Review response (new file)
- config/examples/custom-plugins.yaml: Configuration examples (+9 lines)
Closes #3860
- Add Logrus v1.9.3 dependency - Create pkg/logger package with thread-safe initialization - Add buffering (256KB) to prevent I/O blocking - Implement Close() and Flush() for proper cleanup - Update main.go with structured logging throughout - Add shutdown handler with log flushing - Fix security issue (file permissions 0644) - Test coverage: 80.3% Fixes all blocking issues identified in Principal Engineer review: - File handle resource leak - Missing buffer flush on shutdown - Race condition in concurrent initialization - Blocking I/O in hot path Related to Feature #340: Improve codebase quality and maintainability
Enhanced test coverage for cmd/node-doctor/main.go: - Fixed HTTP exporter test configuration - Added Auth configuration with Type: "none" - Added SendStatus and SendProblems fields to webhook - HTTP exporter now successfully creates and starts in tests Coverage improvements: - Overall: 28.7% → 76.4% (+47.7pp) - main(): 0% → 75.0% (+75pp) - createExporters: 0% → 70.2% (+70.2pp) - createMonitors: 0% → 100.0% (+100pp) Files changed: - cmd/node-doctor/main_additional_test.go: Fixed HTTP exporter test HTTP exporter success path (lines 378-390) now covered. Remaining uncovered code primarily consists of error paths and infrastructure-dependent code (Kubernetes exporter). Related to Task #3862
The test-e2e target was failing because it didn't include the -tags=e2e build flag required by the E2E test files. All E2E test files have the //go:build e2e directive, so the go test command needs -tags=e2e to discover and run them. Changes: - Makefile:221: Added -tags=e2e to go test command This enables running E2E tests with: make test-e2e Related to Task #3765: E2E cluster readiness validation
The golangci-lint-action uses pre-built binaries that were compiled with Go 1.24, which can't analyze code targeting Go 1.25.3. Changed to install golangci-lint from source using the Go 1.25 environment, which ensures compatibility with the project's Go version. Fixes lint error: 'the Go language version (go1.24) used to build golangci-lint is lower than the targeted Go version (1.25.3)'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR contains quality improvements and CI fixes for the node-doctor codebase:
1. Test Coverage Enhancement (Task #3862)
Enhanced test coverage for
cmd/node-doctorpackage from 28.7% to 76.4%, an improvement of 47.7 percentage points.2. E2E Test Infrastructure Fix (Task #3765)
Fixed Makefile
test-e2etarget to include the-tags=e2ebuild flag, enabling E2E tests to run correctly.3. CI Lint Compatibility Fix (commit 7a68b3a)
Fixed golangci-lint Go version compatibility issue that was causing all CI runs to fail on main branch.
Changes
Test Coverage (commit 9881de0)
TestCreateExporters_HTTPExporterWithValidConfigAuthconfiguration withType: "none"SendStatusandSendProblemsfields to webhook endpointE2E Test Fix (commit abd1a8a)
-tags=e2eto go test commandmake test-e2e//go:build e2edirective requiring this flagCI Lint Fix (commit 7a68b3a)
Coverage Improvements
E2E Cluster Readiness Validation
Task #3765 requested implementing comprehensive cluster readiness checks. Upon analysis, all requested features were already implemented:
✅ System Pods Validation
WaitForCoreDNSReady()- Validates CoreDNS deployment readinessWaitForCNIReady()- Validates CNI DaemonSet across multiple providers✅ Pod Scheduling Verification
WaitForPodScheduling()- Creates test deployment and validates scheduling works✅ DNS Resolution Testing
WaitForDNSResolution()- Tests actual DNS resolution with nslookup✅ Integrated in Setup
waitForClusterReady():The existing implementation includes production-quality features:
Test Results
All tests pass successfully:
Coverage Analysis
Now Covered:
Remaining Uncovered (3.6% to 80% target):
Related Tasks
Test Plan
go test -coverprofilemake build