feat: Add rate limiting, configurable workers and network optimizations#24
Conversation
| totalLive += len(hosts) | ||
| } | ||
| } | ||
| logrus.Infof("Port scan complete: found %d hosts with port %s open across all networks", totalLive, config.ExpoterExporterPort) |
There was a problem hiding this comment.
ostrukturerad log, kanske nåt sånt här:
logrus.WithFields(logrus.Fields{
"hosts_with_open_port": totalLive,
"port": config.ExpoterExporterPort,
"scan_type": "port_scan",
"status": "complete",
}).Info("Port scan completed")
Samma för alla andra loggrader
There was a problem hiding this comment.
Pull request overview
This PR replaces Layer 2 ARP scanning with Layer 3 TCP port scanning to enable network discovery across routers. It introduces a two-phase discovery approach: first scanning for open TCP ports, then probing only those hosts with HTTP requests to reduce network traffic by 80-90%.
Changes:
- Added TCP port scanner with no external dependencies (new
portscan.go) - Implemented two-phase discovery: TCP connect scan followed by selective HTTP probing
- Added network optimization features including rate limiting, reduced default workers (128→16), and automatic network/broadcast address skipping
- Added custom DNS server support for PTR lookups
- Enhanced logging with discovery summaries and phase-specific messages
- Updated build configuration (.goreleaser.yaml v2, .env support in Makefile)
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| portscan.go | New TCP port scanner implementation with concurrent worker pool and context support |
| models.go | Added configuration fields for workers, rate limiting, port scanning, and DNS server |
| main.go | Integrated two-phase discovery, rate limiting, custom DNS lookups, and helper functions for network/broadcast filtering |
| go.mod, go.sum | Added golang.org/x/time dependency for rate limiting |
| README.md | Comprehensive documentation of new features, configuration options, and usage examples |
| Makefile | Simplified with .env file support for environment variables |
| .goreleaser.yaml | Updated to v2 format with cosign bundle support |
| .gitignore | Added .env, output/, and build artifacts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return d.DialContext(ctx, "udp", dnsServer) | ||
| }, | ||
| } | ||
| return resolver.LookupAddr(context.Background(), ip) |
There was a problem hiding this comment.
The lookupPTR function creates a new background context instead of accepting a context parameter. This prevents proper cancellation propagation - if the parent context is cancelled, DNS lookups will continue running. Consider adding a context parameter to the function and using it for the LookupAddr call.
| return resolver.LookupAddr(context.Background(), ip) | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| return resolver.LookupAddr(ctx, ip) |
| # Sets DOCKER_HOST to the Podman socket if it exists, otherwise defaults to the Docker socket | ||
| export DOCKER_HOST := unix://$(or $(wildcard /run/user/$(shell id -u)/podman/podman.sock),/var/run/docker.sock) | ||
|
|
||
| include .env |
There was a problem hiding this comment.
The Makefile unconditionally includes .env file with 'include .env', but .env is in .gitignore and may not exist. This will cause make to fail if the .env file doesn't exist. Consider using '-include .env' (with a hyphen) to make the include optional and prevent errors when the file is missing.
| include .env | |
| -include .env |
| // SkipNetworkBroadcast skips network and broadcast addresses. Default true | ||
| SkipNetworkBroadcast bool `default:"true"` |
There was a problem hiding this comment.
The SkipNetworkBroadcast configuration field is defined but never used in the code. Network and broadcast addresses are always skipped in both discoverNetwork and ScanNetwork functions. Either remove this unused configuration field or implement conditional skipping behavior based on its value.
| // SkipNetworkBroadcast skips network and broadcast addresses. Default true | |
| SkipNetworkBroadcast bool `default:"true"` |
| skippedCount := 0 | ||
| for ip := networkip.Mask(ipnet.Mask); ipnet.Contains(ip); inc(ip) { | ||
| // Skip network and broadcast addresses - these will never have hosts | ||
| // and cause unnecessary ARP requests |
There was a problem hiding this comment.
The comment references "unnecessary ARP requests" but this PR replaces ARP scanning with TCP port scanning. Update the comment to be accurate, such as "these will never have hosts and cause unnecessary connection attempts" or simply remove the reference to ARP requests.
| // and cause unnecessary ARP requests | |
| // and cause unnecessary connection attempts |
| | `-skipnetworkbroadcast` | true | Skip network and broadcast addresses | | ||
| | `-exporterexporterport` | 9999 | Port where exporter_exporter is listening | | ||
| | `-port` | 8080 | Port for the internal webserver | | ||
| | `-dnsserver` | (empty) | DNS server for PTR lookups (e.g., "8.8.8.8:53"), empty uses OS default | | ||
|
|
There was a problem hiding this comment.
This documentation describes the skipnetworkbroadcast flag, but the corresponding SkipNetworkBroadcast configuration field is not used in the code. Network and broadcast addresses are always skipped unconditionally. Either remove this documentation or implement the conditional behavior in the code.
| | `-skipnetworkbroadcast` | true | Skip network and broadcast addresses | | |
| | `-exporterexporterport` | 9999 | Port where exporter_exporter is listening | | |
| | `-port` | 8080 | Port for the internal webserver | | |
| | `-dnsserver` | (empty) | DNS server for PTR lookups (e.g., "8.8.8.8:53"), empty uses OS default | | |
| | `-exporterexporterport` | 9999 | Port where exporter_exporter is listening | | |
| | `-port` | 8080 | Port for the internal webserver | | |
| | `-dnsserver` | (empty) | DNS server for PTR lookups (e.g., "8.8.8.8:53"), empty uses OS default | |
| -skipnetworkbroadcast | ||
| Change value of SkipNetworkBroadcast. (default true) |
There was a problem hiding this comment.
This documentation describes the skipnetworkbroadcast flag, but the corresponding SkipNetworkBroadcast configuration field is not used in the code. Network and broadcast addresses are always skipped unconditionally. Either remove this documentation or implement the conditional behavior in the code.
| // isNetworkOrBroadcast checks if the IP is the network address or broadcast address of the given subnet | ||
| func isNetworkOrBroadcast(ip net.IP, ipnet *net.IPNet) bool { | ||
| ip = ip.To4() | ||
| if ip == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
The isNetworkOrBroadcast function only handles IPv4 addresses and returns false for IPv6. This means IPv6 networks won't have the network address filtered out. Consider documenting this IPv4-only limitation in the function comment or adding IPv6 support if needed.
| } | ||
| } | ||
|
|
||
| // discoverHosts probes a specific list of IPs (from ARP scan results) |
There was a problem hiding this comment.
The comment references "ARP scan results" but this PR replaces ARP scanning with TCP port scanning. The comment should be updated to reference "TCP port scan results" or "port scan results" for accuracy.
| // discoverHosts probes a specific list of IPs (from ARP scan results) | |
| // discoverHosts probes a specific list of IPs (from port scan results) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
main.go:373
- The logic inside the queue function for discoverHosts (lines 332-373) is nearly identical to the logic in discoverNetwork (lines 278-320). This code duplication makes maintenance harder. Consider extracting the common logic into a separate function that both can call, passing only the necessary parameters like ip, port, network, ctx, config, and exporter channel.
queue <- func(ctx context.Context) {
for _, data := range exporterConfig {
if ctx.Err() != nil {
return
}
port := data.port
logrus.Debugf("probing live host: %s:%s", ip, port)
exporters, err := checkExporterExporter(ctx, ip, port)
if err != nil {
if !errors.Is(err, context.DeadlineExceeded) && !IsTimeout(err) {
logrus.Debugf("error fetching from exporter_exporter: %s", err)
}
logrus.Debugf("%s:%s exporter_exporter not responding", ip, port)
continue
}
logrus.Info(net.JoinHostPort(ip, port), " is alive")
addr, _ := lookupPTR(ctx, ip, config.DNSServer) // #nosec
hostname := strings.TrimRight(getFirst(addr), ".")
if hostname == "" {
logrus.Debugf("skipping %s: missing reverse record", ip)
continue
}
if isVip(hostname) && !strings.HasPrefix(hostname, "k8s-") {
logrus.Info("skipping vip ", hostname, ip)
continue
}
if len(exporters) > 0 {
for _, filename := range exporters {
a := Address{
IP: strings.TrimSpace(ip),
Hostname: strings.TrimSpace(hostname),
Subnet: strings.TrimSpace(network),
Exporter: filename,
Port: port,
}
exporter <- &a
}
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use custom DNS server | ||
| resolver := &net.Resolver{ | ||
| PreferGo: true, | ||
| Dial: func(ctx context.Context, network, address string) (net.Conn, error) { |
There was a problem hiding this comment.
The context parameter 'network' in the Dial function shadows the 'network' parameter from the outer scope. While this code works correctly (the inner 'network' parameter is intentionally unused as you're always using UDP), consider renaming one of them to avoid confusion and follow best practices. For example, rename the inner parameter to underscore or netType.
| Dial: func(ctx context.Context, network, address string) (net.Conn, error) { | |
| Dial: func(ctx context.Context, _ , address string) (net.Conn, error) { |
| logrus.Debugf("exporter %s: %d targets", name, len(addresses)) | ||
| totalTargets += len(addresses) | ||
| } | ||
| logrus.Debugf("found %d unique exporters with %d total targets", len(exporters), totalTargets) |
There was a problem hiding this comment.
The summary logging uses Debug level when it should arguably be Info level. This information about the total number of discovered exporters and targets is valuable operational data that operators would want to see by default. Consider changing these from Debugf to Infof to make them visible at the default log level.
| logrus.Debugf("exporter %s: %d targets", name, len(addresses)) | |
| totalTargets += len(addresses) | |
| } | |
| logrus.Debugf("found %d unique exporters with %d total targets", len(exporters), totalTargets) | |
| logrus.Infof("exporter %s: %d targets", name, len(addresses)) | |
| totalTargets += len(addresses) | |
| } | |
| logrus.Infof("found %d unique exporters with %d total targets", len(exporters), totalTargets) |
| for ip := ipnet.IP.Mask(ipnet.Mask); ipnet.Contains(ip); inc(ip) { | ||
| ipCopy := make(net.IP, len(ip)) | ||
| copy(ipCopy, ip) | ||
| if skipNetworkBroadcast && isNetworkOrBroadcast(ipCopy, ipnet) { | ||
| continue | ||
| } | ||
| ips = append(ips, ipCopy.String()) |
There was a problem hiding this comment.
The inc function mutates the IP address in-place. Since you're creating a copy in the next lines, consider moving the inc(ip) call after the copy to avoid potential confusion. The current code is correct but the order could be clearer: first copy the current IP, check and add it, then increment the original for the next iteration.
| for ip := ipnet.IP.Mask(ipnet.Mask); ipnet.Contains(ip); inc(ip) { | |
| ipCopy := make(net.IP, len(ip)) | |
| copy(ipCopy, ip) | |
| if skipNetworkBroadcast && isNetworkOrBroadcast(ipCopy, ipnet) { | |
| continue | |
| } | |
| ips = append(ips, ipCopy.String()) | |
| for ip := ipnet.IP.Mask(ipnet.Mask); ipnet.Contains(ip); { | |
| ipCopy := make(net.IP, len(ip)) | |
| copy(ipCopy, ip) | |
| if skipNetworkBroadcast && isNetworkOrBroadcast(ipCopy, ipnet) { | |
| inc(ip) | |
| continue | |
| } | |
| ips = append(ips, ipCopy.String()) | |
| inc(ip) |
| var resultsMu sync.Mutex | ||
|
|
||
| // Worker pool | ||
| ipChan := make(chan string, len(ips)) |
There was a problem hiding this comment.
The channel ipChan is created with a fixed size equal to len(ips). For large CIDR blocks (e.g., /16 or /8), this could allocate a very large buffer in memory. Consider using a smaller fixed buffer size (e.g., 1000) or using s.concurrency * 2 to limit memory usage while still allowing efficient work distribution.
| ipChan := make(chan string, len(ips)) | |
| bufferSize := s.concurrency * 2 | |
| if bufferSize < 1 { | |
| bufferSize = 1 | |
| } | |
| if bufferSize > len(ips) { | |
| bufferSize = len(ips) | |
| } | |
| ipChan := make(chan string, bufferSize) |
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "net" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // PortScanner provides methods for discovering live hosts by checking if a TCP port is open | ||
| type PortScanner struct { | ||
| timeout time.Duration | ||
| port string | ||
| concurrency int | ||
| } | ||
|
|
||
| // NewPortScanner creates a new TCP port scanner | ||
| func NewPortScanner(timeout time.Duration, port string, concurrency int) *PortScanner { | ||
| return &PortScanner{ | ||
| timeout: timeout, | ||
| port: port, | ||
| concurrency: concurrency, | ||
| } | ||
| } | ||
|
|
||
| // ScanNetwork performs a TCP port scan on the given network CIDR and returns IPs with open port | ||
| func (s *PortScanner) ScanNetwork(ctx context.Context, network string, skipNetworkBroadcast bool) ([]string, error) { | ||
| _, ipnet, err := net.ParseCIDR(network) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Generate all IPs to scan | ||
| var ips []string | ||
| for ip := ipnet.IP.Mask(ipnet.Mask); ipnet.Contains(ip); inc(ip) { | ||
| ipCopy := make(net.IP, len(ip)) | ||
| copy(ipCopy, ip) | ||
| if skipNetworkBroadcast && isNetworkOrBroadcast(ipCopy, ipnet) { | ||
| continue | ||
| } | ||
| ips = append(ips, ipCopy.String()) | ||
| } | ||
|
|
||
| // Results | ||
| var results []string | ||
| var resultsMu sync.Mutex | ||
|
|
||
| // Worker pool | ||
| ipChan := make(chan string, len(ips)) | ||
| var wg sync.WaitGroup | ||
|
|
||
| // Start workers | ||
| for i := 0; i < s.concurrency; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| dialer := &net.Dialer{Timeout: s.timeout} | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case ip, ok := <-ipChan: | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| addr := net.JoinHostPort(ip, s.port) | ||
| conn, err := dialer.DialContext(ctx, "tcp", addr) | ||
| if err != nil { | ||
| // Port closed or host unreachable - this is expected for most IPs | ||
| continue | ||
| } | ||
| conn.Close() | ||
|
|
||
| // Port is open! | ||
| resultsMu.Lock() | ||
| results = append(results, ip) | ||
| resultsMu.Unlock() | ||
| logrus.Debugf("TCP port %s open on %s", s.port, ip) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| // Feed IPs to workers | ||
| for _, ip := range ips { | ||
| select { | ||
| case <-ctx.Done(): | ||
| close(ipChan) | ||
| wg.Wait() | ||
| return results, ctx.Err() | ||
| case ipChan <- ip: | ||
| } | ||
| } | ||
| close(ipChan) | ||
| wg.Wait() | ||
|
|
||
| return results, nil | ||
| } | ||
|
|
||
| // DiscoverLiveHosts scans multiple networks and returns hosts with the target port open | ||
| func (s *PortScanner) DiscoverLiveHosts(ctx context.Context, networks []string, skipNetworkBroadcast bool) map[string][]string { | ||
| results := make(map[string][]string) | ||
|
|
||
| for _, network := range networks { | ||
| network = strings.TrimSpace(network) | ||
| if network == "" { | ||
| continue | ||
| } | ||
|
|
||
| logrus.Infof("TCP scanning port %s on network: %s", s.port, network) | ||
| hosts, err := s.ScanNetwork(ctx, network, skipNetworkBroadcast) | ||
| if err != nil { | ||
| logrus.Warnf("TCP scan failed for %s: %v", network, err) | ||
| results[network] = nil | ||
| } else { | ||
| logrus.Infof("Found %d hosts with port %s open in %s", len(hosts), s.port, network) | ||
| results[network] = hosts | ||
| } | ||
| } | ||
|
|
||
| return results | ||
| } |
There was a problem hiding this comment.
The new PortScanner functionality lacks test coverage. Consider adding tests for ScanNetwork and DiscoverLiveHosts methods, including edge cases like empty networks, invalid CIDRs, context cancellation, and network/broadcast address skipping logic.
| go run . | ||
|
|
||
| test: | ||
| go test ./... -count=1 -cover |
There was a problem hiding this comment.
The lint and test-coverage targets were removed from the Makefile. While these may have been unused, removing lint checks could reduce code quality enforcement. Consider keeping the lint target or documenting why it was removed in the PR description.
| go test ./... -count=1 -cover | |
| go test ./... -count=1 -cover | |
| lint: | |
| go vet ./... | |
| test-coverage: | |
| go test ./... -count=1 -coverprofile=coverage.out |
| d := net.Dialer{Timeout: 5 * time.Second} | ||
| return d.DialContext(ctx, "udp", dnsServer) |
There was a problem hiding this comment.
The DNS query timeout is hardcoded to 5 seconds. Consider making this configurable via a Config field, especially since other timeouts like PortScanTimeout are already configurable. Some networks may need shorter or longer DNS timeouts depending on their DNS infrastructure.
|
Jag förstår inte riktigt vad AIn har gjort eller varför? Dessutom säger den att den ARP scannade förut? det gjorde den ju inte? Den anslöt med TCP vilket i sin tur såklart gör ett arp uppslag ifall kerneln inte känner till mac adressen för IPt. Dvs det inte finns. Men BARA på det lokala layer2'et. Så resultatet betyder att den kommer anropa alla hostar som har porten öppen dubbelt nu istället? En gång i tcp och en gång med http klienten? AIn har alltså bara gjort koden sämre? |
5a740c5 to
e433656
Compare
curantes
changed the title
Tack Jonas för en bra review. Din observation är helt korrekt. Tog bort de saker som gjorde koden sämre och behöll de verkliga förbättringarna. Är tacksam för ett bar nya ögon på den senaste force pushen. |
- Add rate limiting for HTTP probes (default 50/sec via -scanratelimit) - Make worker count configurable (default 16 via -workers) - Skip network and broadcast addresses by default (-skipnetworkbroadcast) - Use context-aware DNS lookups for proper cancellation - Change missing PTR record log from error to debug level - Add summary logging of discovered exporters - Fix typo in configuration field (Expoter -> Exporter) - Update .goreleaser.yaml to v2 format - Update README with new configuration options
e433656 to
4d0ac8a
Compare
| return ipv4.Equal(networkIP) || ipv4.Equal(broadcast) | ||
| } | ||
|
|
||
| // For IPv6, check if it's the network address (IPv6 doesn't have broadcast) |
|
Inte bara vi som använder detta nödvändigtvis.
Mvh Jonas
…On Thu, 22 Jan 2026, 14:56 Matthias Eliasson, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In main.go
<#24 (comment)>
:
> + // Try IPv4 first
+ ipv4 := ip.To4()
+ if ipv4 != nil {
+ // Get network address
+ networkIP := ipv4.Mask(ipnet.Mask)
+
+ // Calculate broadcast address
+ broadcast := make(net.IP, len(networkIP))
+ for i := range networkIP {
+ broadcast[i] = networkIP[i] | ^ipnet.Mask[i]
+ }
+
+ return ipv4.Equal(networkIP) || ipv4.Equal(broadcast)
+ }
+
+ // For IPv6, check if it's the network address (IPv6 doesn't have broadcast)
nej bara ipv4
—
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIX4FKDSGDNR74ESXWOTOT4IDJH7AVCNFSM6AAAAACSMJ5SZGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMOJSGU4DIMZVGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Uh oh!
There was an error while loading. Please reload this page.