Conversation
glaslos
commented
May 24, 2025
- Refactored storing data on disc through a single helper
- Ticker to clean up old connections in the tracker
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how binary data is stored and embeds default configuration data while also introducing a periodic cleanup of stale connections.
- Refactored helper function usage for storing payloads across protocols.
- Embedded a default configuration file in Glutton and updated connection management with a ticker-based flush.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| protocols/udp/udp.go | Updated function call to use unified Store helper with "payloads" folder. |
| protocols/tcp/telnet.go | Refactored sample fetching to use the new Store helper and improved error messaging. |
| protocols/tcp/tcp.go | Replaced deprecated StorePayload with new Store helper. |
| protocols/helpers/helpers.go | Renamed StorePayload to Store and generalized folder handling. |
| glutton.go | Embedded default configuration and modified initConfig logic. |
| connection/connection_test.go | Updated tests to pass context when creating connection table. |
| connection/connection.go | Modified New() to accept context and added a periodic flush ticker. |
| app/server.go | Updated SSH port default from 0 to 22. |
|
@namay26 and @Boolean-Autocrat can you give this a code review? |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors payload storage into a unified helper and adjusts configuration loading by embedding a default configuration. Key changes include:
- Changing payload storage from StorePayload to a more flexible Store function.
- Refactoring the Telnet sample retrieval with improved error messages and inline error handling.
- Updating connection management to use context for periodic cleanup and embedding a default configuration in glutton.go.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| protocols/udp/udp.go | Updated payload logging and storage using a min-based slicing logic. |
| protocols/tcp/telnet.go | Improved error messages, URL trimming, and inline error handling in sample retrieval. |
| protocols/tcp/tcp.go | Updated payload storing API call to use the new Store function. |
| protocols/helpers/helpers.go | Renamed StorePayload to Store with added folder flexibility. |
| glutton.go | Embedded a default configuration and updated connection table usage. |
| connection/connection.go | Modified New to accept context and added a ticker for periodic cleanup. |
| connection/connection_test.go | Updated tests to pass context when creating connection tables. |
| app/server.go | Revised default SSH port flag. |
Comments suppressed due to low confidence (2)
connection/connection.go:65
- Ensure that the context provided to connection.New is appropriately canceled when no longer needed to prevent potential goroutine leaks from the ticker loop.
go func() { ... for { select { case <-ctx.Done(): return ... } } }()
protocols/udp/udp.go:16
- Ensure that the helper function 'min' is defined in the current scope and correctly returns the minimum of two int values. If not, consider adding a simple implementation or utility function to replace its usage.
log.Info(fmt.Sprintf("UDP payload:\n%s", hex.Dump(data[:min(len(data), 1024)])))
|
Hi! So this mostly looks good; embedding the default config is a nice approach to provide a fallback. I have a small question/concern in the way the context lifecycle is working in For now, in Glutton the process exits immediately after shutdown but if (maybe in the future) we decide to introduce lifecycle changes (like a restart without process exit), it will probably create issues. We could add a |
|
I agree with the analysis by @Boolean-Autocrat. If we do decide on adding the Restart process, the current setup would create memory leaks. |
|
@Boolean-Autocrat that is an interesting edge case. We can address this when we introduce a restart feature. I would assume during restart you wouldn't cancel the main context. |