Resolve "Alpamon Agent Refactoring" #164

geunwoonoh · 2025-12-29T08:49:13Z

Summary

This PR merges the complete agent refactoring work from 131-alpamon-agent-refactoring branch into main. The refactoring addresses critical architectural issues, including the God Object anti-pattern, uncontrolled goroutine spawning, and circular dependencies.

Key Achievements

Metric	Before	After	Improvement
command.go size	2,363 lines	121 lines	95% reduction
Handler separation	1 monolithic file	9 specialized handlers	Clear SoC
Goroutine management	Unlimited spawning	Pool-limited (10 workers)	Controlled
Context propagation	Partial	100% coverage	Complete
Circular dependencies	Present	None	Eliminated

Architecture Changes

1. Handler Pattern Implementation

Decomposed the monolithic command.go into specialized handlers:

pkg/executor/handlers/
├── common/        # Shared interfaces and utilities
├── system/        # upgrade, restart, reboot, shutdown, quit
├── user/          # adduser, deluser, moduser
├── group/         # addgroup, delgroup
├── firewall/      # firewall, firewall-rollback, reorder operations
├── file/          # upload, download
├── shell/         # shell command execution
├── terminal/      # openpty, openftp, resizepty
├── tunnel/        # opentunnel, closetunnel (NEW)
└── info/          # ping, help, commit, sync

2. Goroutine Pool & Context Management

Worker Pool (internal/pool/pool.go):

Fixed worker count (default: 10)
Job queue with configurable capacity (default: 100)
Panic recovery and graceful shutdown
Non-blocking job submission

Context Manager (pkg/agent/context.go):

Centralized context creation
Hierarchical context tree from root
Coordinated shutdown across all components

3. Command Dispatcher System

pkg/executor/
├── dispatcher.go   # Routes commands to appropriate handlers
├── registry.go     # Handler registration and lookup
├── factory.go      # Dependency injection and handler creation
└── executor.go     # System command execution

4. Main Branch Features Integrated

Tunneling (PR feat: Support for Websh based TCP-tunneling #156): smux-multiplexed WebSocket tunnels for TCP forwarding
Auth Manager (PR feat: implement MFA-based sudo approval system #101): Sudo approval workflow via Unix socket
Control Client: Secondary WebSocket for control plane operations
Logger refactoring (PR refactor(logger): remove lumberjack, use systemd journald for logging #160): Removed lumberjack, uses stdout/systemd
Ping/Pong (PR feat(runner): add ping/pong handler for WebSocket heartbeat #158): WebSocket heartbeat support

Design Patterns Applied

Pattern	Location	Purpose
Handler	`pkg/executor/handlers/`	Command execution isolation
Factory	`pkg/executor/factory.go`	Centralized handler creation
Registry	`pkg/executor/registry.go`	Handler lookup by command
Strategy	`handlers/firewall/backend.go`	iptables/nftables abstraction
Object Pool	`internal/pool/pool.go`	Goroutine management
Context	`pkg/agent/context.go`	Lifecycle management

Breaking Changes

API Changes

NewCommandRunner() now requires dispatcher parameter
NewWebsocketClient() now requires ctxManager and workerPool parameters

Configuration

New [pool] section in alpamon.conf:

[pool]
max_workers = 10
queue_size = 100
default_timeout = 300

Dependencies

Added: github.com/xtaci/smux (tunnel multiplexing)
Removed: gopkg.in/natefinch/lumberjack.v2 (log rotation)

Test Results

$ go build -v ./...
# Build successful

$ go test -v ./... -p 1
# All tests pass

Test Coverage

38 test files across all packages
Unit tests for each handler
Integration tests for dispatcher system
Goroutine leak detection tests
Benchmark tests for pool and context manager

Files Changed (Key Files)

Core Refactoring

pkg/runner/command.go - Reduced from 2,363 to 121 lines
pkg/runner/client.go - Added pool integration and ping/pong
cmd/alpamon/command/root.go - Updated initialization sequence

New Components

internal/pool/pool.go - Worker pool implementation
pkg/agent/context.go - Context manager
pkg/executor/dispatcher.go - Command dispatcher
pkg/executor/factory.go - Handler factory
pkg/executor/handlers/* - 9 specialized handlers

Integrated from Main

pkg/runner/tunnel_client.go - smux tunnel client
pkg/runner/auth_manager.go - Sudo approval manager
pkg/runner/control_client.go - Control WebSocket client

Verification Checklist

Build passes on Linux and macOS
All existing tests pass
No goroutine leaks detected
No circular dependencies
Backward compatible with existing server API
Configuration changes documented

Deployment Notes

Configuration Update: Add [pool] section to existing configs
systemd: Service file updated for journald logging
Monitoring: Watch goroutine count and memory usage post-deployment

Related Issues/PRs

Closes Alpamon Agent Refactoring #131 (Agent refactoring)
Includes feat: Support for Websh based TCP-tunneling #156 (Tunneling)
Includes feat: implement MFA-based sudo approval system #101 (Auth Manager)
Includes refactor(logger): remove lumberjack, use systemd journald for logging #160 (Logger refactoring)
Includes feat(runner): add ping/pong handler for WebSocket heartbeat #158 (Ping/Pong)

Add a Pool struct that implements the Goroutine Worker Pool pattern to solve the problem of potentially having an unlimited number of goroutines.

There was a potential for goroutines to not shut down gracefully upon alpamon termination because their contexts were not propagated. Furthermore, the unrestrained use of contexts resulted in poor context management. To address these problems, add a ContextManager to globally manage contexts within alpamon.

As the Pool was added, add test cases for the functionalities it provides.

As the ContextManager was added, add test cases for the functionalities it provides.

As the management of goroutines and contexts was changed to be handled globally by the Pool and ContextManager, respectively, refactor the following components: - Collector(pkg/collector/collector.go) - WebsocketClient(pkg/runner/client.go) - CommandRunner(pkg/runner/command.go) - CommitAsync()(pkg/runner/commit.go) - Reporter(pkg/scheduler/reporter.go) - LogServer(pkg/logger/server.go)

If multiple PtyClients attempt to use the terminals variable simultaneously, a race condition could occur. To solve this, use sync.RWMutex to prevent the race condition.

Add a pool section to alpamon.conf to allow users to dynamically configure parameters such as the maximum number of workers and the queue size for the Pool.

Refactore the newly added pool section in alpamon.conf to be stored and used in Settings, similar to the other existing sections in alpamon.conf.

Delete unused NewContextManagerWithRoot().

Adjust golint to pool_test.go.

Add a ReporterManager to manage the goroutine lifecycle of the Reporters.

Fix a context leak by modifying the code to use the ReporterManager for managing the goroutine and context lifecycle of the Reporters.

As mentioned in the code review, fix the code to use a cancel function to prevent context leaks and memory leaks in the restart, quit, reboot, and shutdown operation cases.

As mentioned in the code review, fix the code to use a cancel function to prevent context leaks and memory leaks in LogServer and WebsocketClient.

Replace unconventional context timeout as sleep mechanism with standard time.After + select pattern for better readability and Go idioms.

Use pointer type for DefaultTimeout to distinguish between "not configured" and "explicitly set to 0", enabling users to disable timeout via config.

…ase-1-goroutine-pool-and-context-management-implementation Resolve "[Alpamon Agent Refactoring] Phase 1: Goroutine Pool and Context Management Implementation"

…actoring

Add executor components to resolve the God Object Anti-pattern in pkg/runner/command.go Add Handler, CommandExecutor, and WSClient interfaces considering polymorphism and extensibility. Executor is the component that executes the actual command, responsible for receiving the command data from Handler, managing privilege demotion, and executing the command.

Add Registry, which is responsible for registering and unregistering handlers, and managing the registered handlers.

Add CommandDispatcher component, which is responsible for routing the Command received from the CommandRunner to the correct handler.

Add BaseHandler to provide functionality commonly used by all handlers.

The previous use of maps for passing CommandData between the Dispatcher, Handler, and Executor resulted in reduced type safety. This was resolved by adding a separate CommandArgs struct to manage the data used by these components.

Update the CommandDispatcher following the addition of CommandArgs.

Update Interfaces following the addition of CommandArgs at pkg/executor/handlers/common/interfaces.go

Integrate NewBaseHandlerWithTypes() into NewBaseHandler().

Refactor pkg/runner/command.go and pkg/runner/command\_types.go following the addition of executor-related components to resolve the God Object Anti-pattern in pkg/runner/command.go. Refactor the CommandRunner so that its sole responsibility is to parse the incoming CommandData into CommandArgs, route the command to the Dispatcher for execution by the Executor, and send the execution result back to the WebsocketClient. Firewall logic was kept for compatibility and is scheduled for refactoring in Phase 3.

Add dispatcher field to WebsocketClient to handle command with CommandRunner and CommandDispatcher. Separate the logic for the command cases into a method named handleCommand()

Minor fix.

Add a SystemHandler to process system-related commands. This handler takes over the following cases previously handled by CommandRunner: upgrade, restart, quit, reboot, shutdown, update, byebye

Update pty.go to import and use protocol.CommandData directly. Update terminal.go to use protocol.CommandData instead of runner.CommandData. Update client.go, command.go to use types in internal/protocol instead of types in pkg/runner.

…-phase4-transport-layer-separation Resolve "[Alpamon Agent Refactoring] Phase4: Transport Layer Separation"

Add InfoHandler tests (ping, help, commit, sync) Add ShellHandler tests (operators, user/group, env, timeout) Add SystemHandler tests (upgrade, restart, quit, reboot, shutdown)

Add pool leak tests (normal ops, panic, context cancel, queue full). Add context manager leak tests (create/cancel, child cleanup). Verify zero goroutine leaks after operations.

Add pool benchmarks (worker scaling, queue throughput, concurrent submit). Add context manager benchmarks (create, timeout, concurrent). Add dispatcher/registry benchmarks (get, list, concurrent). Fix benchmark deadlock by increasing queue size.

Add integration tests for registry and handler execution. Add regression tests for all handler/command types. Add resource usage tests (memory, startup time, overhead).

Apply golint.

…ase-5-testing-and-validation Resolve "[Alpamon Agent Refactoring] Phase 5: Testing and Validation"

…actoring-integrate-main-branch-changes-into-refactored-architecture

Add TunnelHandler to handle the following commands: opentunnel, closetunnel Define types and structs related to TunnelHandler.

Add fields related to tunneling functionality in conjunction with TunnelHandler.

…ation Update RegisterAll() to include TunnelHandler registration.

…tegrate-main-branch-changes-into-refactored-architecture Resolve "[Alpamon Agent Refactoring] Integrate Main Branch Changes into Refactored Architecture"

pkg/executor/executor.go

pkg/executor/handlers/file/file.go

pkg/executor/handlers/user/user.go

pkg/utils/fs.go

pkg/utils/http_client.go

geunwoonoh · 2025-12-29T09:04:19Z

@junho226 , @jisung-02 First of all, I apologize for requesting a review on such a large amount of code.
However, I believe it should be manageable as the code has already undergone review and testing in the following PRs:

During the refactoring process, I have also incorporated the latest changes merged into the main branch.
Therefore, I would appreciate it if you could focus your review on the following areas:

@junho226 : pkg/runner/auth_manager.go, pkg/runner/control_client.go, cmd/alpamon/command/root.go
@jisung-02 : pkg/executor/handlers/tunnel

Inline suppressions added for intentional behaviors (9 locations): - executor.go: CWE-78 command injection (core agent functionality) - file.go: CWE-22 path traversal, CWE-918 SSRF (admin file operations) - user.go: CWE-22 path traversal (home directory management) - fs.go: CWE-22 path traversal (file existence check) - http_client.go: CWE-918 SSRF (admin-specified URL requests) Alpamon is a server management agent equivalent to SSH server. Commands are received only from authenticated Alpacon administrators.

Add CodeQL security scanning for Go codebase with custom configuration. New files: - .github/workflows/codeql.yml: Weekly CodeQL analysis on main branch - .github/codeql/codeql-config.yml: Security model documentation

Remove custom CodeQL configuration to resolve conflict with GitHub's Default Setup which is already enabled on this repository. Deleted files: - .github/workflows/codeql.yml - .github/codeql/codeql-config.yml Inline suppression comments in source files (executor.go, file.go, user.go, fs.go, http_client.go) remain intact and will work with GitHub's Default Setup.

Add // lgtm[...] inline comments to suppress CodeQL security alerts that are false positives for this server management agent. Alpamon is designed to execute admin commands from authenticated Alpacon console, similar to an SSH server. These operations are intentional features, not vulnerabilities: - go/command-injection: executor.go (admin command execution) - go/path-injection: file.go, user.go, fs.go (file operations) - go/request-forgery: file.go, http_client.go (URL fetching) Existing // codeql[...] comments retained for documentation.

pkg/executor/executor.go

pkg/executor/handlers/file/file.go

pkg/executor/handlers/user/user.go

pkg/utils/fs.go

geunwoonoh and others added 30 commits November 24, 2025 12:02

feat(pool): add Pool struct to manage goroutine

c153122

Add a Pool struct that implements the Goroutine Worker Pool pattern to solve the problem of potentially having an unlimited number of goroutines.

feat(pool): add test cases for Pool

b862cd2

As the Pool was added, add test cases for the functionalities it provides.

feat(agent): add test cases for ContextManager

fc9c1aa

As the ContextManager was added, add test cases for the functionalities it provides.

fix(pty): fix race condition in PtyClient

123366c

If multiple PtyClients attempt to use the terminals variable simultaneously, a race condition could occur. To solve this, use sync.RWMutex to prevent the race condition.

feat(config): add pool section to alpamon.conf

0ee4daa

Add a pool section to alpamon.conf to allow users to dynamically configure parameters such as the maximum number of workers and the queue size for the Pool.

refactor(config): update due to apply changes in alpamon.conf

a35efa3

Refactore the newly added pool section in alpamon.conf to be stored and used in Settings, similar to the other existing sections in alpamon.conf.

chore(agent): Delete unused function

6633f4b

Delete unused NewContextManagerWithRoot().

fix(pool): adjust golint to pool_test.go

261d75b

Adjust golint to pool_test.go.

feat(scheduler): add ReportManager to manage reporter goroutine

52eb1a6

Add a ReporterManager to manage the goroutine lifecycle of the Reporters.

fix(scheduler): fix context leak

d3bb4d7

Fix a context leak by modifying the code to use the ReporterManager for managing the goroutine and context lifecycle of the Reporters.

fix(command): fix context leaks in specific cases

01adca2

As mentioned in the code review, fix the code to use a cancel function to prevent context leaks and memory leaks in the restart, quit, reboot, and shutdown operation cases.

fix: fix context leaks in specific component

41eead2

As mentioned in the code review, fix the code to use a cancel function to prevent context leaks and memory leaks in LogServer and WebsocketClient.

refactor(commit): use time.After for delayed execution in CommitAsync

94bb1ef

Replace unconventional context timeout as sleep mechanism with standard time.After + select pattern for better readability and Go idioms.

fix(config): allow explicit zero timeout in pool configuration

c750986

Use pointer type for DefaultTimeout to distinguish between "not configured" and "explicitly set to 0", enabling users to disable timeout via config.

Merge pull request #138 from alpacax/132-alpamon-agent-refactoring-ph…

bf7506c

…ase-1-goroutine-pool-and-context-management-implementation Resolve "[Alpamon Agent Refactoring] Phase 1: Goroutine Pool and Context Management Implementation"

Merge remote-tracking branch 'origin/main' into 131-alpamon-agent-ref…

bde6d2b

…actoring

feat(executor): add Registry component

9817ffa

Add Registry, which is responsible for registering and unregistering handlers, and managing the registered handlers.

feat(executor): add CommandDispatcher component

1dd19d7

Add CommandDispatcher component, which is responsible for routing the Command received from the CommandRunner to the correct handler.

feat(handlers): Add BaseHandler

eed68e6

Add BaseHandler to provide functionality commonly used by all handlers.

feat(executor): add CommandArgs to provide type safety

10dcc7a

The previous use of maps for passing CommandData between the Dispatcher, Handler, and Executor resulted in reduced type safety. This was resolved by adding a separate CommandArgs struct to manage the data used by these components.

refactor(executor): update CommandDispatcher to apply changes

f44e207

Update the CommandDispatcher following the addition of CommandArgs.

refactor(executor): update Interfaces to apply changes

ac3802e

Update Interfaces following the addition of CommandArgs at pkg/executor/handlers/common/interfaces.go

chore(handler): Delete unused function

e0ee9f6

Integrate NewBaseHandlerWithTypes() into NewBaseHandler().

refactor(client): update following the addition of Executor

6a69b91

Add dispatcher field to WebsocketClient to handle command with CommandRunner and CommandDispatcher. Separate the logic for the command cases into a method named handleCommand()

chore(executor): minor fix

9597127

Minor fix.

feat(handler): add SystemHandler

648de3a

Add a SystemHandler to process system-related commands. This handler takes over the following cases previously handled by CommandRunner: upgrade, restart, quit, reboot, shutdown, update, byebye

geunwoonoh and others added 15 commits December 16, 2025 15:18

refactor: update to apply changes

2254dfa

Update pty.go to import and use protocol.CommandData directly. Update terminal.go to use protocol.CommandData instead of runner.CommandData. Update client.go, command.go to use types in internal/protocol instead of types in pkg/runner.

Merge pull request #153 from geunwoonoh/135-alpamon-agent-refactoring…

1c9120d

…-phase4-transport-layer-separation Resolve "[Alpamon Agent Refactoring] Phase4: Transport Layer Separation"

test(handlers): add unit tests for info, shell, and system handlers

a0e30ec

Add InfoHandler tests (ping, help, commit, sync) Add ShellHandler tests (operators, user/group, env, timeout) Add SystemHandler tests (upgrade, restart, quit, reboot, shutdown)

test: add goroutine leak detection tests

9d79f9f

Add pool leak tests (normal ops, panic, context cancel, queue full). Add context manager leak tests (create/cancel, child cleanup). Verify zero goroutine leaks after operations.

test(executor): add integration, regression, and resource tests

04fc496

Add integration tests for registry and handler execution. Add regression tests for all handler/command types. Add resource usage tests (memory, startup time, overhead).

chore: apply golint

0511174

Apply golint.

chore: apply golint

c1f5a54

Apply golint.

chore: apply golint

d5882f5

Apply golint.

Merge pull request #155 from alpacax/136-alpamon-agent-refactoring-ph…

2f46385

…ase-5-testing-and-validation Resolve "[Alpamon Agent Refactoring] Phase 5: Testing and Validation"

Merge remote-tracking branch 'origin/main' into 162-alpamon-agent-ref…

cd8ada4

…actoring-integrate-main-branch-changes-into-refactored-architecture

feat(handlers): implement TunnelHandler

ea4d9be

Add TunnelHandler to handle the following commands: opentunnel, closetunnel Define types and structs related to TunnelHandler.

feat(protocol): add tunnel specific field

469b2ca

Add fields related to tunneling functionality in conjunction with TunnelHandler.

feat(executor): update RegisterAll() to include TunnelHandler registr…

0052267

…ation Update RegisterAll() to include TunnelHandler registration.

Merge pull request #163 from alpacax/162-alpamon-agent-refactoring-in…

4cf6ff7

…tegrate-main-branch-changes-into-refactored-architecture Resolve "[Alpamon Agent Refactoring] Integrate Main Branch Changes into Refactored Architecture"

geunwoonoh requested review from jisung-02 and junho226 December 29, 2025 08:49

geunwoonoh self-assigned this Dec 29, 2025

geunwoonoh linked an issue Dec 29, 2025 that may be closed by this pull request

Alpamon Agent Refactoring #131

Open

14 tasks

github-advanced-security bot found potential problems Dec 29, 2025

View reviewed changes

geunwoonoh added 2 commits December 29, 2025 19:18

feat(ci): add CodeQL security scanning workflow and configuration

d0c97c9

Add CodeQL security scanning for Go codebase with custom configuration. New files: - .github/workflows/codeql.yml: Weekly CodeQL analysis on main branch - .github/codeql/codeql-config.yml: Security model documentation

geunwoonoh marked this pull request as draft December 29, 2025 10:25

junho226 previously approved these changes Dec 29, 2025

View reviewed changes

geunwoonoh dismissed junho226’s stale review via f65f8d7 December 29, 2025 10:50

github-advanced-security bot found potential problems Dec 29, 2025

View reviewed changes

geunwoonoh marked this pull request as ready for review December 29, 2025 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Resolve "Alpamon Agent Refactoring" #164

Resolve "Alpamon Agent Refactoring" #164

Uh oh!

geunwoonoh commented Dec 29, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geunwoonoh commented Dec 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Resolve "Alpamon Agent Refactoring" #164

Are you sure you want to change the base?

Resolve "Alpamon Agent Refactoring" #164

Uh oh!

Conversation

geunwoonoh commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Achievements

Architecture Changes

1. Handler Pattern Implementation

2. Goroutine Pool & Context Management

3. Command Dispatcher System

4. Main Branch Features Integrated

Design Patterns Applied

Breaking Changes

API Changes

Configuration

Dependencies

Test Results

Test Coverage

Files Changed (Key Files)

Core Refactoring

New Components

Integrated from Main

Verification Checklist

Deployment Notes

Related Issues/PRs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geunwoonoh commented Dec 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geunwoonoh commented Dec 29, 2025 •

edited

Loading