Skip to content

feat: add admin API endpoints for monitoring and auditing#158

Merged
rshade merged 1 commit intomasterfrom
auditing
Jun 10, 2025
Merged

feat: add admin API endpoints for monitoring and auditing#158
rshade merged 1 commit intomasterfrom
auditing

Conversation

@rshade
Copy link
Owner

@rshade rshade commented Jun 9, 2025

Add comprehensive admin API endpoints to enable programmatic monitoring and auditing of WStunnel server operations:

  • /admin/monitoring: Provides aggregate statistics (unique tunnels, connections, request counts) for dashboards and alerting systems

  • /admin/auditing: Provides detailed tunnel information including active connections, client details, and request history for security auditing

  • Consolidated admin functionality in tunnel/admin_service.go for maintainability and extensibility

  • SQLite-based persistence with automatic cleanup of records older than 7 days

  • Thread-safe operations with proper mutex usage for concurrent access

  • Extensible design allowing easy addition of new admin endpoints

  • Real-time tunnel monitoring: Track active tunnels, connections, and request volumes

  • Request lifecycle tracking: Complete audit trail from request start to completion/error

  • Client identification: IP addresses, reverse DNS, WHOIS data, and client version tracking

  • Base path support: Full compatibility with existing base path configuration

  • JSON API responses: Machine-readable format suitable for automation and web UIs

  • request_events table: Tracks all HTTP requests with status, timing, and client information

  • tunnel_events table: Records tunnel lifecycle events (connect/disconnect/errors)

  • Automatic indexing for performance on common queries

  • Built-in cleanup to prevent unbounded growth

curl http://localhost:8080/admin/monitoring
{
  "timestamp": "2025-01-09T15:30:45Z",
  "unique_tunnels": 3,
  "tunnel_connections": 3,
  "pending_requests": 5,
  "completed_requests": 1247,
  "errored_requests": 23
}
curl http://localhost:8080/admin/auditing
{
  "timestamp": "2025-01-09T15:30:45Z",
  "tunnels": {
    "my_secret_token": {
      "token": "my_secret_token",
      "remote_addr": "192.168.1.100:54321",
      "remote_name": "client.example.com",
      "client_version": "wstunnel v1.0.0",
      "last_activity": "2025-01-09T15:30:40Z",
      "active_connections": [...],
      "last_error_time": "2025-01-09T15:25:00Z",
      "last_success_time": "2025-01-09T15:30:35Z",
      "pending_requests": 1
    }
  }
}
  • Comprehensive test coverage including lifecycle testing, error handling, and edge cases

  • Integration tests for HTTP handlers with proper status codes and JSON validation

  • Mock server testing to ensure compatibility with existing WSTunnelServer architecture

  • Base path integration testing to verify proper path handling

  • User documentation added to README.md with API examples and field descriptions

  • Operations guide in docs/ADMIN_API.md with monitoring scripts, alerting rules, and troubleshooting

  • Security recommendations for production deployment

  • Monitoring dashboards: Integrate with Prometheus, Grafana, or custom monitoring systems

  • Security auditing: Track client connections, identify suspicious activity, generate compliance reports

  • Debugging and troubleshooting: Inspect active connections, view recent errors, identify performance issues

  • Capacity planning: Monitor usage patterns and plan infrastructure scaling

  • Web UI development: JSON APIs provide foundation for future web-based admin interfaces

  • No breaking changes to existing functionality

  • Optional initialization - admin service gracefully handles initialization failures

  • Existing endpoints unchanged - all current APIs remain fully functional

  • Configuration compatibility - works with all existing server configuration options

Resolves #143 #144

Summary by CodeRabbit

  • New Features
    • Introduced admin monitoring and auditing API endpoints, providing real-time statistics and detailed tunnel activity in JSON format for easier integration with monitoring and security tools.
  • Documentation
    • Added comprehensive guides and examples for using the new admin API endpoints, including operational scenarios, security recommendations, and troubleshooting tips.
  • Tests
    • Implemented thorough tests for admin service functionality, ensuring reliability and correctness of monitoring and auditing features.
  • Chores
    • Added a Makefile target to automate formatting of Go source files.
    • Updated dependencies to support new features.

Copilot AI review requested due to automatic review settings June 9, 2025 23:38
@coderabbitai
Copy link

coderabbitai bot commented Jun 9, 2025

Walkthrough

A new administrative API layer was introduced to the WebSocket tunnel server, enabling programmatic auditing and monitoring via HTTP endpoints. Supporting documentation and usage guides were added. The server and its request structures were extended to record request timing, and corresponding tests were implemented for the new admin service and endpoints. Dependency and build configuration updates support these changes.

Changes

File(s) Change Summary
Makefile Added format target to run gofmt on all Go files.
README.md Documented new Admin API endpoints, usage, and security notes.
docs/ADMIN_API.md Added comprehensive operational guide for Admin API usage, scripts, and best practices.
go.mod Added dependencies for SQLite and supporting libraries.
tunnel/admin_service.go Introduced AdminService with persistent event tracking, monitoring, and auditing HTTP endpoints.
tunnel/admin_service_test.go Added unit and integration tests for AdminService and its endpoints.
tunnel/wstunsrv.go Extended server/request structs, initialized admin service, registered endpoints, lifecycle handling.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HTTPServer
    participant AdminService
    participant SQLiteDB
    participant TunnelRegistry

    Client->>HTTPServer: GET /admin/monitoring
    HTTPServer->>AdminService: GetMonitoringStats()
    AdminService->>TunnelRegistry: Query active tunnels/connections
    AdminService->>SQLiteDB: Query request/tunnel event stats
    AdminService-->>HTTPServer: MonitoringResponse (JSON)
    HTTPServer-->>Client: 200 OK (JSON)

    Client->>HTTPServer: GET /admin/auditing
    HTTPServer->>AdminService: GetAuditingData()
    AdminService->>TunnelRegistry: Query tunnel/connection details
    AdminService->>SQLiteDB: Query recent request events
    AdminService-->>HTTPServer: AuditingResponse (JSON)
    HTTPServer-->>Client: 200 OK (JSON)
Loading

Assessment against linked issues

Objective Addressed Explanation
List of unique tunnels (#143)
List of active connections for each tunnel with IP addresses (#143)
Time of last errored request with IP address (#143)
Time of last successful request with IP address (#143)

Poem

A rabbit hopped to the tunnel’s side,
And saw new admin APIs open wide!
With stats and audits, JSON flows,
Now every tunnel’s secret shows.
Monitoring made easy, oh what fun—
Well done, dear devs, the work is done!
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds new admin API endpoints for programmatic monitoring and auditing of WStunnel operations, wires up an in‐memory SQLite persistence layer, and updates documentation and tooling.

  • Introduces AdminService in WSTunnelServer with /admin/monitoring and /admin/auditing handlers
  • Updates module dependencies for SQLite persistence
  • Provides comprehensive docs (docs/ADMIN_API.md, README.md, CLAUDE.md) and adds a Makefile format target

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tunnel/wstunsrv.go Added AdminService field, initialization (NewAdminService(t, ":memory:")), handler registration, startTime tracking, and cleanup on stop
go.mod Added modernc.org/sqlite and several indirect deps for SQLite support
docs/ADMIN_API.md New operational guide for monitoring and auditing endpoints
README.md Documented /admin/monitoring and /admin/auditing endpoints
Makefile Added format target to run gofmt on Go sources
CLAUDE.md Introduced format commands and updated style reminders
Comments suppressed due to low confidence (4)

tunnel/wstunsrv.go:345

  • New HTTP handlers for /admin/auditing and /admin/monitoring were added but there are no corresponding integration or unit tests in this diff. Please add tests to verify response codes and JSON schema.
httpMux.HandleFunc(buildPath(t.BasePath, "/admin/auditing"), t.adminService.HandleAuditing)

docs/ADMIN_API.md:71

  • The header is written after the curl command, which will overwrite any data. Swap the order or use >> appropriately so that the header appears once at the top.
echo "Date,Token,RemoteAddr,RemoteName,ActiveConnections" > "/var/log/wstunnel/audit-$DATE.csv"

CLAUDE.md:23

  • [nitpick] The make format instruction appears twice in this document. Remove the duplicate to keep the guide concise.
- `make format` - Format all Go files with gofmt

go.mod:15

  • Several indirect dependencies were added; verify each is required by the admin service to avoid module bloat. Consider pruning unused imports.
github.com/dustin/go-humanize v1.0.1 // indirect


// Initialize admin service if not already set
if t.adminService == nil {
adminService, err := NewAdminService(t, ":memory:")
Copy link

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an in-memory SQLite database (":memory:") means audit data isn't persisted across restarts, which contradicts the PR's persistence goal. Consider making the database path configurable or defaulting to a file-backed DB.

Copilot uses AI. Check for mistakes.
fi

# Format all Go files
format:
Copy link

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’ve added a format target—consider integrating it into the CI pipeline or make lint to enforce formatting automatically.

Copilot uses AI. Check for mistakes.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13d32db and 55ee16a.

⛔ Files ignored due to path filters (2)
  • CLAUDE.md is excluded by !CLAUDE.md
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • Makefile (1 hunks)
  • README.md (1 hunks)
  • docs/ADMIN_API.md (1 hunks)
  • go.mod (1 hunks)
  • tunnel/admin_service.go (1 hunks)
  • tunnel/admin_service_test.go (1 hunks)
  • tunnel/wstunsrv.go (6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`go.mod`: - Ensure go.mod uses correct format (e.g., `go 1.24` not `go 1.24.0`)

go.mod: - Ensure go.mod uses correct format (e.g., go 1.24 not go 1.24.0)

  • go.mod
`**/*.go`: - Ensure code follows gofmt formatting standards - Check that imports are properly organized (standard library first, third-party after) - Verify error handling follows ...

**/*.go: - Ensure code follows gofmt formatting standards

  • Check that imports are properly organized (standard library first, third-party after)
  • Verify error handling follows the pattern: check immediately, return or log with context
  • Ensure logging uses log15 with structured key-value pairs
  • Verify naming conventions: PascalCase for exported, camelCase for unexported
  • Check for unhandled errors
  • Verify proper use of contexts
  • tunnel/admin_service_test.go
  • tunnel/wstunsrv.go
  • tunnel/admin_service.go
`tunnel/**/*.go`: - Check WebSocket handling follows established patterns in tunnel/ws.go - Verify concurrent request handling uses goroutine-per-request pattern - Check for securi...

tunnel/**/*.go: - Check WebSocket handling follows established patterns in tunnel/ws.go

  • Verify concurrent request handling uses goroutine-per-request pattern
  • Check for security issues: no logging of passwords/tokens, proper certificate validation
  • Ensure no unused blank identifiers (like var _ fmt.Formatter)
  • Verify token authentication is properly implemented
  • Check for timing attacks in authentication
  • Ensure errors don't leak sensitive information
  • tunnel/admin_service_test.go
  • tunnel/wstunsrv.go
  • tunnel/admin_service.go
`**/*_test.go`: - Confirm tests follow standard Go testing patterns - Verify test coverage for edge cases and error conditions - Prefer table-driven tests - Test both success and e...

**/*_test.go: - Confirm tests follow standard Go testing patterns

  • Verify test coverage for edge cases and error conditions
  • Prefer table-driven tests
  • Test both success and error paths
  • Mock external dependencies appropriately
  • tunnel/admin_service_test.go
🧬 Code Graph Analysis (2)
tunnel/wstunsrv.go (1)
tunnel/admin_service.go (2)
  • AdminService (19-24)
  • NewAdminService (94-117)
tunnel/admin_service.go (1)
tunnel/wstunsrv.go (1)
  • WSTunnelServer (115-132)
🪛 LanguageTool
docs/ADMIN_API.md

[uncategorized] ~319-~319: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... secured 5. Rate Limiting: Consider rate limiting admin endpoint access ## Troubleshooti...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[uncategorized] ~325-~325: You might be missing the article “the” here.
Context: ...n Endpoints Not Responding 1. Check if WStunnel server is running: `ps aux | grep wstun...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

README.md

[uncategorized] ~362-~362: You might be missing the article “the” here.
Context: ...r of tunnels. ### Admin API Endpoints WStunnel server provides two JSON API endpoints ...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~454-~454: Loose punctuation mark.
Context: ...: HTTP method (GET, POST, etc.) - uri: The requested URI path - remote_addr`:...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 markdownlint-cli2 (0.17.2)
docs/ADMIN_API.md

342-342: Files should end with a single newline character
null

(MD047, single-trailing-newline)

README.md

370-370: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


377-377: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


389-389: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


400-400: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


407-407: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


438-438: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


452-452: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


459-459: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test
  • GitHub Check: Analyze (go)
🔇 Additional comments (14)
Makefile (1)

154-159: LGTM! Well-implemented formatting target.

The new format target follows the same file exclusion pattern as the lint target and properly uses gofmt -w for in-place formatting. This will help maintain consistent code formatting across the project.

go.mod (2)

11-11: LGTM! Appropriate dependency addition for admin service.

The addition of modernc.org/sqlite provides the necessary SQLite support for the new admin monitoring and auditing functionality. The version format follows the correct go.mod conventions.


15-30: LGTM! Required indirect dependencies for SQLite support.

These indirect dependencies are automatically managed by Go modules and provide the necessary supporting libraries for the SQLite functionality. All packages appear to be legitimate components of the modernc.org SQLite ecosystem.

README.md (2)

367-466: Excellent comprehensive API documentation.

The documentation provides clear examples, field descriptions, use cases, and important security considerations. The JSON response examples and curl commands make it easy for users to understand and implement the admin API endpoints.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~454-~454: Loose punctuation mark.
Context: ...: HTTP method (GET, POST, etc.) - uri: The requested URI path - remote_addr`:...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 markdownlint-cli2 (0.17.2)

370-370: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


377-377: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


389-389: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


400-400: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


407-407: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


438-438: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


452-452: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


459-459: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


361-366: 🧹 Nitpick (assertive)

Consider adding article for better readability.

The documentation is excellent, but consider adding "The" at the beginning for better readability: "The WStunnel server provides two JSON API endpoints..."

-WStunnel server provides two JSON API endpoints for programmatic monitoring and auditing:
+The WStunnel server provides two JSON API endpoints for programmatic monitoring and auditing:

Likely an incorrect or invalid review comment.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~362-~362: You might be missing the article “the” here.
Context: ...r of tunnels. ### Admin API Endpoints WStunnel server provides two JSON API endpoints ...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

tunnel/wstunsrv.go (6)

63-63: Good addition for request timing tracking.

Adding the startTime field enables proper request lifecycle monitoring in the admin service, complementing the existing deadline field.


131-131: Clean integration of admin service.

The adminService field is properly added to the WSTunnelServer struct to support the new monitoring and auditing functionality.


295-303: Excellent graceful initialization pattern.

The admin service initialization follows good practices:

  • Only initializes if not already set (supports dependency injection)
  • Uses in-memory SQLite for simplicity
  • Handles initialization errors gracefully without failing server startup
  • Follows structured logging patterns with proper error context

343-347: Proper conditional endpoint registration.

The admin endpoints are correctly registered only when the admin service is available, and they properly use the base path handling consistent with other endpoints in the server.


388-392: Good resource cleanup with error handling.

The admin service cleanup in the Stop() method follows proper resource management patterns with appropriate error logging.


869-869: Nice optimization for consistent timing.

Capturing time.Now() once and reusing it for both deadline and startTime is more efficient and ensures both fields use the exact same timestamp, which is important for accurate request timing measurements.

Also applies to: 875-876

tunnel/admin_service.go (3)

368-368:

Details

✅ Verification successful

Undefined type safeResponseWriter and function safeError.

The handlers use safeResponseWriter type and safeError function which are not defined in this file.

Also applies to: 371-371


🏁 Script executed:

#!/bin/bash
# Search for safeResponseWriter type definition
rg -A 5 "type safeResponseWriter" --type go

# Search for safeError function definition
rg -A 3 "func safeError" --type go

Length of output: 515


No undefined references – types and functions are defined in tunnel/ws.go

Both safeResponseWriter and safeError are implemented in tunnel/ws.go, so there’s no missing definition in admin_service.go.

type safeResponseWriter at tunnel/ws.go
func safeError at tunnel/ws.go


228-228:

Details

❌ Incorrect review comment

Undefined constant tunnelInactiveKillTimeout.

The code references tunnelInactiveKillTimeout which is not defined in this file.


🏁 Script executed:

#!/bin/bash
# Search for the definition of tunnelInactiveKillTimeout
rg -A 2 "tunnelInactiveKillTimeout\s*=" --type go

Length of output: 274


Constant tunnelInactiveKillTimeout is defined elsewhere
The tunnelInactiveKillTimeout constant is declared in tunnel/wstunsrv.go (as const tunnelInactiveKillTimeout = 60 * time.Minute). No changes are needed here.

Likely an incorrect or invalid review comment.


306-306:

Details

❌ Incorrect review comment

Undefined function cutToken.

The code uses cutToken function which is not defined in this file.

Also applies to: 317-317


🏁 Script executed:

#!/bin/bash
# Search for the definition of cutToken function
rg -A 3 "func cutToken" --type go

Length of output: 181


🏁 Script executed:

#!/bin/bash
# Check package declarations in both files
awk 'FNR==1{print FILENAME ": " $0}' tunnel/admin_service.go tunnel/wstunsrv.go

# Search for the definition of the custom type `token`
rg -n "type token" --type go

Length of output: 303


🏁 Script executed:

#!/bin/bash
# Check package declarations in both files to confirm they share the same package
rg -n "^package " tunnel/admin_service.go tunnel/wstunsrv.go

Length of output: 139


Ignore undefined function cutToken
The call to cutToken in tunnel/admin_service.go is valid—this helper is defined in tunnel/wstunsrv.go (same tunnel package). No changes required.

Likely an incorrect or invalid review comment.

Comment on lines +93 to +119
// NewAdminService creates a new admin service with SQLite tracking
func NewAdminService(server *WSTunnelServer, dbPath string) (*AdminService, error) {
db, err := sql.Open("sqlite", dbPath)
if err != nil {
return nil, fmt.Errorf("failed to open database: %w", err)
}

service := &AdminService{
db: db,
server: server,
log: server.Log.New("component", "admin"),
}

if err := service.initDB(); err != nil {
if closeErr := db.Close(); closeErr != nil {
service.log.Error("failed to close database after init error", "error", closeErr)
}
return nil, fmt.Errorf("failed to initialize database: %w", err)
}

// Start cleanup goroutine
go service.cleanupOldRecords()

return service, nil
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding context support for database operations.

The current implementation doesn't support context-based cancellation, which is a Go best practice for long-running operations and graceful shutdowns.

Consider accepting context.Context in your database operation methods to support cancellation:

func (as *AdminService) RecordRequestStart(ctx context.Context, token, method, uri, remoteAddr string) (int64, error) {
    // Use ctx for database operations
    result, err := as.db.ExecContext(ctx, `
        INSERT INTO request_events (token, method, uri, remote_addr, status, start_time)
        VALUES (?, ?, ?, ?, ?, ?)
    `, token, method, uri, remoteAddr, "pending", time.Now())
    // ...
}

This would allow callers to cancel operations during shutdown or implement timeouts.

Also applies to: 167-214

🤖 Prompt for AI Agents
In tunnel/admin_service.go between lines 93 and 117, the NewAdminService
function and related database methods lack context.Context support, which is
important for enabling cancellation and timeouts in database operations. To fix
this, update the signatures of database-related methods in AdminService to
accept a context.Context parameter, and use ExecContext, QueryContext, or
similar context-aware database calls inside these methods. This change should
also be applied to the methods spanning lines 167 to 214 to ensure consistent
context usage across all database interactions.

Add comprehensive admin API endpoints to enable programmatic monitoring and auditing of
WStunnel server operations:

- **`/admin/monitoring`**: Provides aggregate statistics (unique tunnels, connections,
  request counts) for dashboards and alerting systems
- **`/admin/auditing`**: Provides detailed tunnel information including active connections,
  client details, and request history for security auditing

- **Consolidated admin functionality** in `tunnel/admin_service.go` for maintainability
  and extensibility
- **SQLite-based persistence** with automatic cleanup of records older than 7 days
- **Thread-safe operations** with proper mutex usage for concurrent access
- **Extensible design** allowing easy addition of new admin endpoints

- **Real-time tunnel monitoring**: Track active tunnels, connections, and request volumes
- **Request lifecycle tracking**: Complete audit trail from request start to
  completion/error
- **Client identification**: IP addresses, reverse DNS, WHOIS data, and client version
  tracking
- **Base path support**: Full compatibility with existing base path configuration
- **JSON API responses**: Machine-readable format suitable for automation and web UIs

- `request_events` table: Tracks all HTTP requests with status, timing, and client
  information
- `tunnel_events` table: Records tunnel lifecycle events (connect/disconnect/errors)
- Automatic indexing for performance on common queries
- Built-in cleanup to prevent unbounded growth

```bash
curl http://localhost:8080/admin/monitoring
```
```json
{
  "timestamp": "2025-01-09T15:30:45Z",
  "unique_tunnels": 3,
  "tunnel_connections": 3,
  "pending_requests": 5,
  "completed_requests": 1247,
  "errored_requests": 23
}
```

```bash
curl http://localhost:8080/admin/auditing
```
```json
{
  "timestamp": "2025-01-09T15:30:45Z",
  "tunnels": {
    "my_secret_token": {
      "token": "my_secret_token",
      "remote_addr": "192.168.1.100:54321",
      "remote_name": "client.example.com",
      "client_version": "wstunnel v1.0.0",
      "last_activity": "2025-01-09T15:30:40Z",
      "active_connections": [...],
      "last_error_time": "2025-01-09T15:25:00Z",
      "last_success_time": "2025-01-09T15:30:35Z",
      "pending_requests": 1
    }
  }
}
```

- **Comprehensive test coverage** including lifecycle testing, error handling, and edge
  cases
- **Integration tests** for HTTP handlers with proper status codes and JSON validation
- **Mock server testing** to ensure compatibility with existing WSTunnelServer
  architecture
- **Base path integration testing** to verify proper path handling

- **User documentation** added to README.md with API examples and field descriptions
- **Operations guide** in `docs/ADMIN_API.md` with monitoring scripts, alerting rules,
  and troubleshooting
- **Security recommendations** for production deployment

- **Monitoring dashboards**: Integrate with Prometheus, Grafana, or custom monitoring
  systems
- **Security auditing**: Track client connections, identify suspicious activity, generate
  compliance reports
- **Debugging and troubleshooting**: Inspect active connections, view recent errors,
  identify performance issues
- **Capacity planning**: Monitor usage patterns and plan infrastructure scaling
- **Web UI development**: JSON APIs provide foundation for future web-based admin
  interfaces

- **No breaking changes** to existing functionality
- **Optional initialization** - admin service gracefully handles initialization failures
- **Existing endpoints unchanged** - all current APIs remain fully functional
- **Configuration compatibility** - works with all existing server configuration
  options

Resolves #143 #144
@codecov
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 76.22150% with 73 lines in your changes missing coverage. Please review.

Project coverage is 57.86%. Comparing base (13d32db) to head (ee97427).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
tunnel/admin_service.go 75.98% 52 Missing and 15 partials ⚠️
tunnel/wstunsrv.go 78.57% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   55.14%   57.86%   +2.71%     
==========================================
  Files          13       14       +1     
  Lines        2205     2511     +306     
==========================================
+ Hits         1216     1453     +237     
- Misses        863      915      +52     
- Partials      126      143      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (3)
Makefile (1)

154-158: Well-implemented formatting target.

The format target correctly excludes vendor directories and version.go while formatting all other Go files. The implementation follows best practices.

As noted in previous reviews, consider integrating this into your CI pipeline or make lint target to enforce formatting automatically.

docs/ADMIN_API.md (1)

1-342: Outstanding operational documentation.

This comprehensive guide provides excellent value for operations teams with practical scripts, monitoring examples, and troubleshooting guidance.

As noted in previous reviews, add a trailing newline at the end of the file to fix the markdown linting issue.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~319-~319: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... secured 5. Rate Limiting: Consider rate limiting admin endpoint access ## Troubleshooti...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[uncategorized] ~325-~325: You might be missing the article “the” here.
Context: ...n Endpoints Not Responding 1. Check if WStunnel server is running: `ps aux | grep wstun...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

tunnel/wstunsrv.go (1)

296-306: ⚠️ Potential issue

Address the in-memory database persistence issue.

Using ":memory:" contradicts the PR's goal of providing persistent audit data across server restarts.

As noted in previous reviews, using an in-memory SQLite database means audit data isn't persisted across restarts, which contradicts the PR's persistence goal. Consider making the database path configurable or defaulting to a file-backed DB.

-		adminService, err := NewAdminService(t, ":memory:")
+		adminService, err := NewAdminService(t, "/var/lib/wstunnel/admin.db")

Or make it configurable via command-line flag.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55ee16a and ee97427.

⛔ Files ignored due to path filters (2)
  • CLAUDE.md is excluded by !CLAUDE.md
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • Makefile (1 hunks)
  • README.md (1 hunks)
  • docs/ADMIN_API.md (1 hunks)
  • go.mod (1 hunks)
  • tunnel/admin_service.go (1 hunks)
  • tunnel/admin_service_test.go (1 hunks)
  • tunnel/wstunsrv.go (6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`go.mod`: - Ensure go.mod uses correct format (e.g., `go 1.24` not `go 1.24.0`)

go.mod: - Ensure go.mod uses correct format (e.g., go 1.24 not go 1.24.0)

  • go.mod
`**/*.go`: - Ensure code follows gofmt formatting standards - Check that imports are properly organized (standard library first, third-party after) - Verify error handling follows ...

**/*.go: - Ensure code follows gofmt formatting standards

  • Check that imports are properly organized (standard library first, third-party after)
  • Verify error handling follows the pattern: check immediately, return or log with context
  • Ensure logging uses log15 with structured key-value pairs
  • Verify naming conventions: PascalCase for exported, camelCase for unexported
  • Check for unhandled errors
  • Verify proper use of contexts
  • tunnel/wstunsrv.go
  • tunnel/admin_service_test.go
  • tunnel/admin_service.go
`tunnel/**/*.go`: - Check WebSocket handling follows established patterns in tunnel/ws.go - Verify concurrent request handling uses goroutine-per-request pattern - Check for securi...

tunnel/**/*.go: - Check WebSocket handling follows established patterns in tunnel/ws.go

  • Verify concurrent request handling uses goroutine-per-request pattern
  • Check for security issues: no logging of passwords/tokens, proper certificate validation
  • Ensure no unused blank identifiers (like var _ fmt.Formatter)
  • Verify token authentication is properly implemented
  • Check for timing attacks in authentication
  • Ensure errors don't leak sensitive information
  • tunnel/wstunsrv.go
  • tunnel/admin_service_test.go
  • tunnel/admin_service.go
`**/*_test.go`: - Confirm tests follow standard Go testing patterns - Verify test coverage for edge cases and error conditions - Prefer table-driven tests - Test both success and e...

**/*_test.go: - Confirm tests follow standard Go testing patterns

  • Verify test coverage for edge cases and error conditions
  • Prefer table-driven tests
  • Test both success and error paths
  • Mock external dependencies appropriately
  • tunnel/admin_service_test.go
🧬 Code Graph Analysis (1)
tunnel/wstunsrv.go (1)
tunnel/admin_service.go (2)
  • AdminService (19-25)
  • NewAdminService (95-119)
🪛 LanguageTool
docs/ADMIN_API.md

[uncategorized] ~319-~319: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... secured 5. Rate Limiting: Consider rate limiting admin endpoint access ## Troubleshooti...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[uncategorized] ~325-~325: You might be missing the article “the” here.
Context: ...n Endpoints Not Responding 1. Check if WStunnel server is running: `ps aux | grep wstun...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

README.md

[uncategorized] ~454-~454: Loose punctuation mark.
Context: ...: HTTP method (GET, POST, etc.) - uri: The requested URI path - remote_addr`:...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 markdownlint-cli2 (0.17.2)
README.md

370-370: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


377-377: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


389-389: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


400-400: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


407-407: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


438-438: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


452-452: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


459-459: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: test
🔇 Additional comments (19)
go.mod (2)

11-11: LGTM: SQLite dependency addition is appropriate.

The addition of modernc.org/sqlite aligns with the admin service requirements for persistent data storage.


15-29: LGTM: Indirect dependencies are properly managed.

The indirect dependencies for SQLite and related utilities are correctly added by the Go module system.

tunnel/wstunsrv.go (5)

63-63: Good addition for request timing.

Adding startTime to track request lifecycle supports the admin service's auditing capabilities.


131-132: Proper field additions for admin service integration.

The adminService field and corresponding mutex follow good concurrency practices.


347-352: Excellent admin endpoint registration with proper synchronization.

The conditional registration with read lock protection ensures thread-safe access to the admin service.


393-399: Proper cleanup in Stop method.

The admin service cleanup follows best practices with error logging and proper synchronization.


876-883: Nice optimization: single time capture.

Capturing time once and using it for both deadline and startTime is more efficient and ensures consistency.

tunnel/admin_service_test.go (4)

17-62: Well-structured test with proper resource management.

The test properly creates and cleans up temporary resources, and validates specific table creation as suggested in the previous review.


200-280: Comprehensive monitoring stats test coverage.

The test properly validates all monitoring statistics including unique tunnels, connections, and various request states.


282-410: Excellent comprehensive auditing data test.

The test thoroughly validates all aspects of the auditing response including active connections, request history, and error tracking.


556-601: Good integration test for base path handling.

This test ensures the admin service functions correctly when the server has a base path configured, which is important for deployment flexibility.

tunnel/admin_service.go (8)

18-92: Well-designed type definitions with proper JSON serialization.

The types are comprehensive with appropriate use of optional fields and proper JSON tags for API responses.


94-119: Properly implemented constructor with comprehensive error handling.

The constructor correctly initializes all components and ensures proper cleanup on initialization failure.


130-171: Well-designed database schema with appropriate indexes.

The schema properly handles all required fields and includes indexes for common query patterns.


173-196: Proper input validation and thread-safe implementation.

The method correctly validates inputs and enforces reasonable length limits as suggested in the previous review.


230-271: Robust monitoring stats implementation with graceful error handling.

The method properly handles concurrent access and continues providing partial data even if some queries fail, which is appropriate for monitoring.


273-355: Comprehensive auditing implementation with proper data aggregation.

The method effectively combines real-time server state with historical database records, handles edge cases well, and uses safe token logging.


357-384: Well-implemented cleanup routine with proper termination handling.

The cleanup goroutine correctly implements graceful shutdown via the done channel as requested in previous reviews.


386-407: Properly implemented HTTP handler with safe error handling.

The handler correctly uses safeResponseWriter and handles errors appropriately with proper HTTP status codes.

Comment on lines +361 to +465
### Admin API Endpoints

WStunnel server provides two JSON API endpoints for programmatic monitoring and auditing:

#### `/admin/monitoring` - Aggregate Statistics

Returns high-level statistics in JSON format, suitable for monitoring dashboards and alerting systems.

**Example Request:**
```bash
curl http://localhost:8080/admin/monitoring
# With base path:
curl http://localhost:8080/wstunnel/admin/monitoring
```

**Example Response:**
```json
{
"timestamp": "2025-01-09T15:30:45Z",
"unique_tunnels": 3,
"tunnel_connections": 3,
"pending_requests": 5,
"completed_requests": 1247,
"errored_requests": 23
}
```

**Fields:**
- `unique_tunnels`: Number of unique tunnel tokens registered
- `tunnel_connections`: Number of active tunnel connections
- `pending_requests`: Current number of requests waiting for response
- `completed_requests`: Total successful requests since server start
- `errored_requests`: Total failed requests since server start

#### `/admin/auditing` - Detailed Tunnel Information

Returns comprehensive details about all active tunnels and their connections, suitable for security auditing and detailed analysis.

**Example Request:**
```bash
curl http://localhost:8080/admin/auditing
# With base path:
curl http://localhost:8080/wstunnel/admin/auditing
```

**Example Response:**
```json
{
"timestamp": "2025-01-09T15:30:45Z",
"tunnels": {
"my_secret_token": {
"token": "my_secret_token",
"remote_addr": "192.168.1.100:54321",
"remote_name": "client.example.com",
"remote_whois": "Example Corp",
"client_version": "wstunnel v1.0.0",
"last_activity": "2025-01-09T15:30:40Z",
"active_connections": [
{
"request_id": 123,
"method": "GET",
"uri": "/api/data",
"remote_addr": "10.0.0.5",
"start_time": "2025-01-09T15:30:30Z"
}
],
"last_error_time": "2025-01-09T15:25:00Z",
"last_error_addr": "10.0.0.3",
"last_success_time": "2025-01-09T15:30:35Z",
"last_success_addr": "10.0.0.5",
"pending_requests": 1
}
}
}
```

**Tunnel Fields:**
- `token`: The tunnel token (first 8 characters shown in logs)
- `remote_addr`: IP address and port of the tunnel client
- `remote_name`: Reverse DNS lookup of the client IP
- `remote_whois`: WHOIS information for the client IP (if available)
- `client_version`: Version string reported by the tunnel client
- `last_activity`: Timestamp of last tunnel activity
- `active_connections`: Array of currently active HTTP requests
- `last_error_time`: Timestamp of most recent failed request (optional)
- `last_error_addr`: IP address of most recent failed request (optional)
- `last_success_time`: Timestamp of most recent successful request (optional)
- `last_success_addr`: IP address of most recent successful request (optional)
- `pending_requests`: Number of requests currently pending

**Active Connection Fields:**
- `request_id`: Unique identifier for the request
- `method`: HTTP method (GET, POST, etc.)
- `uri`: The requested URI path
- `remote_addr`: IP address of the client making the request
- `start_time`: When the request was initiated

**Use Cases:**
- **Monitoring**: Use `/admin/monitoring` for dashboards, alerting, and performance tracking
- **Security Auditing**: Use `/admin/auditing` to track which clients are connecting from where
- **Debugging**: Use `/admin/auditing` to see active requests and recent errors
- **Capacity Planning**: Monitor request volumes and tunnel usage patterns
- **Web UI Integration**: Both endpoints return JSON suitable for web-based admin interfaces

**Security Note:** These endpoints are accessible without authentication. In production environments, consider placing them behind a reverse proxy with appropriate access controls.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Excellent admin API documentation with minor formatting issues.

The documentation comprehensively covers both endpoints with practical examples, field descriptions, and appropriate security warnings. The content accurately reflects the implementation.

Fix the markdown formatting issues identified by static analysis:

+
 **Example Request:**
+
 ```bash
 curl http://localhost:8080/admin/monitoring

Apply similar fixes for other code blocks and lists that need blank line separation as indicated by the static analysis hints.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~454-~454: Loose punctuation mark.
Context: ...: HTTP method (GET, POST, etc.) - uri: The requested URI path - remote_addr`:...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 markdownlint-cli2 (0.17.2)

370-370: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


377-377: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


389-389: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


400-400: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


407-407: Fenced code blocks should be surrounded by blank lines
null

(MD031, blanks-around-fences)


438-438: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


452-452: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)


459-459: Lists should be surrounded by blank lines
null

(MD032, blanks-around-lists)

🤖 Prompt for AI Agents
In README.md from lines 361 to 465, fix markdown formatting issues by ensuring
there are blank lines before and after all code blocks and lists. Specifically,
add a blank line before the opening triple backticks and after the closing
triple backticks in all bash and json code blocks. Also, ensure lists have blank
lines separating them from surrounding text or code blocks to improve
readability and comply with markdown standards.

Comment on lines +412 to +482
func TestHandleAuditing(t *testing.T) {
// Create temporary database file
tmpfile, err := os.CreateTemp("", "admin_test*.db")
if err != nil {
t.Fatal(err)
}
defer func() {
if err := os.Remove(tmpfile.Name()); err != nil {
t.Logf("Failed to remove temp file: %v", err)
}
}()
if err := tmpfile.Close(); err != nil {
t.Fatalf("Failed to close temp file: %v", err)
}

// Create mock server
server := &WSTunnelServer{
Log: log15.New("pkg", "test"),
serverRegistry: make(map[token]*remoteServer),
tokenClients: make(map[token]int),
}

// Create admin service
adminService, err := NewAdminService(server, tmpfile.Name())
if err != nil {
t.Fatalf("Failed to create admin service: %v", err)
}
defer func() {
if err := adminService.Close(); err != nil {
t.Logf("Failed to close admin service: %v", err)
}
}()

// Test GET request
req := httptest.NewRequest("GET", "/admin/auditing", nil)
w := httptest.NewRecorder()

adminService.HandleAuditing(w, req)

resp := w.Result()
if resp.StatusCode != http.StatusOK {
t.Errorf("Expected status 200, got %d", resp.StatusCode)
}

contentType := resp.Header.Get("Content-Type")
if !strings.Contains(contentType, "application/json") {
t.Errorf("Expected JSON content type, got %s", contentType)
}

// Verify JSON response
var auditingResp AuditingResponse
err = json.NewDecoder(resp.Body).Decode(&auditingResp)
if err != nil {
t.Fatalf("Failed to decode JSON response: %v", err)
}

if auditingResp.Tunnels == nil {
t.Error("Expected tunnels map to be initialized")
}

// Test non-GET request
req = httptest.NewRequest("POST", "/admin/auditing", nil)
w = httptest.NewRecorder()

adminService.HandleAuditing(w, req)

resp = w.Result()
if resp.StatusCode != http.StatusMethodNotAllowed {
t.Errorf("Expected status 405, got %d", resp.StatusCode)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding error scenario testing.

The HTTP handler tests are good, but consider adding a test case where GetAuditingData returns an error to ensure proper error handling and HTTP 500 response.

🤖 Prompt for AI Agents
In tunnel/admin_service_test.go around lines 412 to 482, the test for
HandleAuditing lacks coverage for error scenarios. Add a test case where the
GetAuditingData method is mocked or configured to return an error, then verify
that the HTTP handler responds with a 500 Internal Server Error status. This
will ensure proper error handling is tested in the adminService.HandleAuditing
method.

Comment on lines +64 to +95
func TestRecordRequestLifecycle(t *testing.T) {
// Create temporary database file
tmpfile, err := os.CreateTemp("", "admin_test*.db")
if err != nil {
t.Fatal(err)
}
defer func() {
if err := os.Remove(tmpfile.Name()); err != nil {
t.Logf("Failed to remove temp file: %v", err)
}
}()
if err := tmpfile.Close(); err != nil {
t.Fatalf("Failed to close temp file: %v", err)
}

// Create mock server
server := &WSTunnelServer{
Log: log15.New("pkg", "test"),
serverRegistry: make(map[token]*remoteServer),
tokenClients: make(map[token]int),
}

// Create admin service
adminService, err := NewAdminService(server, tmpfile.Name())
if err != nil {
t.Fatalf("Failed to create admin service: %v", err)
}
defer func() {
if err := adminService.Close(); err != nil {
t.Logf("Failed to close admin service: %v", err)
}
}()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Extract common test setup into a helper function.

The temporary file creation and admin service initialization code is duplicated across all test functions. Consider extracting this into a helper function to improve maintainability.

Add a helper function at the beginning of the test file:

+func setupTestAdminService(t *testing.T) (*AdminService, func()) {
+	// Create temporary database file
+	tmpfile, err := os.CreateTemp("", "admin_test*.db")
+	if err != nil {
+		t.Fatal(err)
+	}
+	
+	cleanupFunc := func() {
+		if err := os.Remove(tmpfile.Name()); err != nil {
+			t.Logf("Failed to remove temp file: %v", err)
+		}
+	}
+	
+	if err := tmpfile.Close(); err != nil {
+		cleanupFunc()
+		t.Fatalf("Failed to close temp file: %v", err)
+	}
+	
+	// Create mock server
+	server := &WSTunnelServer{
+		Log:            log15.New("pkg", "test"),
+		serverRegistry: make(map[token]*remoteServer),
+		tokenClients:   make(map[token]int),
+	}
+	
+	// Create admin service
+	adminService, err := NewAdminService(server, tmpfile.Name())
+	if err != nil {
+		cleanupFunc()
+		t.Fatalf("Failed to create admin service: %v", err)
+	}
+	
+	return adminService, func() {
+		if err := adminService.Close(); err != nil {
+			t.Logf("Failed to close admin service: %v", err)
+		}
+		cleanupFunc()
+	}
+}

Then simplify each test:

 func TestRecordRequestLifecycle(t *testing.T) {
-	// Create temporary database file
-	tmpfile, err := os.CreateTemp("", "admin_test*.db")
-	// ... (lines 66-95)
+	adminService, cleanup := setupTestAdminService(t)
+	defer cleanup()

Also applies to: 142-173, 200-241, 282-335, 412-443, 484-515, 556-588

🤖 Prompt for AI Agents
In tunnel/admin_service_test.go around lines 64 to 95 and similarly in lines
142-173, 200-241, 282-335, 412-443, 484-515, and 556-588, the temporary file
creation and admin service initialization code is duplicated across multiple
test functions. Extract this repeated setup code into a single helper function
that creates the temp file, initializes the admin service, and returns them
along with any errors. Then update each test to call this helper function to
obtain the setup objects, reducing duplication and improving maintainability.

Comment on lines +121 to +128
// Close closes the admin service and database connection
func (as *AdminService) Close() error {
// Signal cleanup goroutine to stop
close(as.done)
// Give it time to finish current operation
time.Sleep(100 * time.Millisecond)
return as.db.Close()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider using sync.WaitGroup for more robust goroutine synchronization.

While the current implementation works, using a fixed sleep is fragile. Consider using sync.WaitGroup for deterministic cleanup.

Add a WaitGroup to the AdminService struct:

 type AdminService struct {
 	db     *sql.DB
 	server *WSTunnelServer
 	log    log15.Logger
 	mu     sync.RWMutex
 	done   chan struct{}
+	wg     sync.WaitGroup
 }

Update NewAdminService:

 	// Start cleanup goroutine
+	service.wg.Add(1)
 	go service.cleanupOldRecords()

Update Close method:

 func (as *AdminService) Close() error {
 	// Signal cleanup goroutine to stop
 	close(as.done)
-	// Give it time to finish current operation
-	time.Sleep(100 * time.Millisecond)
+	// Wait for cleanup goroutine to finish
+	as.wg.Wait()
 	return as.db.Close()
 }

Update cleanupOldRecords:

 func (as *AdminService) cleanupOldRecords() {
+	defer as.wg.Done()
 	ticker := time.NewTicker(1 * time.Hour)
🤖 Prompt for AI Agents
In tunnel/admin_service.go around lines 121 to 128, replace the fixed time.Sleep
with sync.WaitGroup to synchronize the cleanup goroutine deterministically. Add
a sync.WaitGroup field to the AdminService struct, increment it when starting
the cleanup goroutine in NewAdminService, and call Done inside the goroutine
when finished. In the Close method, call Wait on the WaitGroup after signaling
the goroutine to stop, before closing the database. Also update the
cleanupOldRecords method to call Done on the WaitGroup when the goroutine exits.

Comment on lines +198 to +215
// RecordRequestComplete records the completion of a request
func (as *AdminService) RecordRequestComplete(requestID int64, success bool, errorMsg string) error {
as.mu.Lock()
defer as.mu.Unlock()

status := "completed"
if !success {
status = "errored"
}

_, err := as.db.Exec(`
UPDATE request_events
SET status = ?, end_time = ?, error = ?
WHERE id = ?
`, status, time.Now(), errorMsg, requestID)

return err
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider validating request existence before update.

The method doesn't verify that the requestID exists before updating. While not critical, checking RowsAffected could help detect invalid IDs.

-	_, err := as.db.Exec(`
+	result, err := as.db.Exec(`
 		UPDATE request_events 
 		SET status = ?, end_time = ?, error = ?
 		WHERE id = ?
 	`, status, time.Now(), errorMsg, requestID)
+	
+	if err != nil {
+		return err
+	}
+	
+	rowsAffected, err := result.RowsAffected()
+	if err != nil {
+		return err
+	}
+	
+	if rowsAffected == 0 {
+		return fmt.Errorf("request ID %d not found", requestID)
+	}

 	return err
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// RecordRequestComplete records the completion of a request
func (as *AdminService) RecordRequestComplete(requestID int64, success bool, errorMsg string) error {
as.mu.Lock()
defer as.mu.Unlock()
status := "completed"
if !success {
status = "errored"
}
_, err := as.db.Exec(`
UPDATE request_events
SET status = ?, end_time = ?, error = ?
WHERE id = ?
`, status, time.Now(), errorMsg, requestID)
return err
}
func (as *AdminService) RecordRequestComplete(requestID int64, success bool, errorMsg string) error {
as.mu.Lock()
defer as.mu.Unlock()
status := "completed"
if !success {
status = "errored"
}
result, err := as.db.Exec(`
UPDATE request_events
SET status = ?, end_time = ?, error = ?
WHERE id = ?
`, status, time.Now(), errorMsg, requestID)
if err != nil {
return err
}
rowsAffected, err := result.RowsAffected()
if err != nil {
return err
}
if rowsAffected == 0 {
return fmt.Errorf("request ID %d not found", requestID)
}
return err
}
🤖 Prompt for AI Agents
In tunnel/admin_service.go around lines 198 to 215, the RecordRequestComplete
method updates a request without verifying if the requestID exists. Modify the
code to check the result of the Exec call for RowsAffected; if zero rows are
affected, return an error indicating the requestID was not found. This
validation helps detect invalid IDs before completing the request update.

@rshade rshade merged commit d69e61c into master Jun 10, 2025
9 checks passed
@rshade rshade deleted the auditing branch June 10, 2025 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http request for auditing

2 participants