Feature/pfdhcp optimization

[fdurand]

Description

Fixes various issues in pfdhcp code

Impacts

DHCP server

Delete branch after merge

YES

Checklist

(REQUIRED) - [yes, no or n/a]

• Document the feature
• Add OpenAPI specification
• Add unit tests
• Add acceptance tests (TestLink)

NEWS file entries

Bug Fixes

Critical Severity - Crash/Panic Issues (11 bugs):
Bug #1 - Resource leak in keysoption.go MysqlGet/MysqlDel (defer ordering + wrong SQL method)
Bug #5 - Resource leak in utils.go NodeInformation (defer before error check)
Bug #9 - Panic in HTTP handlers api.go (2 locations - should return errors not panic)
Bug #12 - Nil pointer from unchecked net.ParseCIDR in utils.go detectVIP
Bug #14 - Index out of bounds in utils.go IpRange (accessing ips[1] without length check)
Bug #15 - Nil pointer in utils.go Shuffle (ParseIP().To4() passed to len())
Bug #16 - Nil pointer in utils.go ShuffleNetIP (array[0].To4() passed to len())
Bug #17 - Unchecked regex match in utils.go AssignIP (accessing result[2] without validation)
Bug #18 - Nil pointer in utils.go AssignIP (ParseIP().To4() to Uint32)
Bug #19 - Nil pointer in config.go (ParseIP().To4() to Uint32 for DHCP range validation)
Bug #20 - Nil pointer in utils.go cacheWarmup (ip.To4() to Uint32)
High Severity - Data Corruption/Security (3 bugs):
Bug #2 - Race condition in main.go handleRequest (lock/unlock ordering issue)
Bug #4 - Missing lock error checks in main.go (3 locations - lock failures ignored)
Bug #6 - Integer overflow in utils.go cryptoShuffle (uint64 to int conversion)
Medium Severity - Resource/Context Leaks (3 bugs):
Bug #3 - Context leak in main.go handleDecline/handleRelease (2 goroutines with 30s sleep)
Bug #8 - Resource leak in main.go pingDB (missing defer resp.Body.Close)
Bug #13 - Context leak in main.go handleDiscover (goroutine with 10min sleep)
Low Severity - Error Handling/Code Quality (3 bugs):
Bug #7 - Typo in api.go ("betwork" → "network")
Bug #10 - Ignored MysqlInsert errors in api.go (2 locations)
Bug #11 - Ignored errors in workers_pool.go (SplitHostPort, Atoi, NewRawClient)

[Copilot]

Pull Request Overview

This PR addresses critical bug fixes and optimization improvements in the pfdhcp (PacketFence DHCP server) code, focusing on resource management, error handling, and potential crash scenarios. The changes include fixing resource leaks, race conditions, nil pointer dereferences, and adding comprehensive unit test coverage.

Key changes:

• Fixed 20 critical bugs including resource leaks, nil pointer dereferences, and race conditions
• Added proper error handling throughout the codebase with checks for lock failures, parsing errors, and context cancellations
• Introduced comprehensive unit test coverage across multiple components (40+ tests)

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
workers_pool.go	Added error handling for SplitHostPort, NewRawClient, and Atoi operations (Bug #11)
workers_pool_test.go	New test file covering job structure creation and context handling
utils.go	Fixed nil pointer dereferences in IP parsing operations and improved integer overflow handling (Bugs #6, #12, #14-20)
utils_helpers_test.go	New test file for utility helper functions
main.go	Fixed context leaks in goroutines, lock error handling, and defer placement (Bugs #2-4, #8, #13)
main_test.go	Added integration test placeholder documentation
keysoption.go	Fixed defer ordering and incorrect SQL method usage (Bug #1)
keysoption_test.go	New test file documenting MySQL operation interfaces
external_test.go	Added external tests for constants and configurations
constants_test.go	New test file for constant validation
config.go	Added nil checks for DHCP range IP validation (Bug #19)
config_test.go	New test file for configuration structures
api.go	Fixed panic statements, ignored errors, and typo correction (Bugs #7, #9, #10)
api_test.go	New comprehensive test file for API endpoints and structures
README_TESTS.md	New documentation for test suite organization and execution

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The inline anonymous function to check handler.ip length is unnecessarily complex. Use a ternary-style approach or extract to a variable before the log statement for better readability.

Suggested change

	log.LoggerWContext(ctx).Debug(fmt.Sprintf(
	"ServeDHCP: MAC=%s giaddr=%s ciaddr=%s msgType=%s found=%v handlerIP=%s NetScope=%v",
	answer.MAC.String(), p.GIAddr().String(), p.CIAddr().String(), msgType.String(), found, func() string {
	if len(handler.ip) > 0 {
	return handler.ip.String()
	} else {
	return ""
	}
	}(), NetScope))
	if !found || len(handler.ip) == 0 {
	log.LoggerWContext(ctx).Info(fmt.Sprintf(
	"Ignored DHCP request: MAC=%s giaddr=%s ciaddr=%s msgType=%s (no handler/network found)",
	answer.MAC.String(), p.GIAddr().String(), p.CIAddr().String(), msgType.String()))
	return answer
	}
	var handlerIPStr string
	if len(handler.ip) > 0 {
	handlerIPStr = handler.ip.String()
	} else {
	handlerIPStr = ""
	}
	log.LoggerWContext(ctx).Debug(fmt.Sprintf(
	"ServeDHCP: MAC=%s giaddr=%s ciaddr=%s msgType=%s found=%v handlerIP=%s NetScope=%v",
	answer.MAC.String(), p.GIAddr().String(), p.CIAddr().String(), msgType.String(), found, handlerIPStr, NetScope))

[Copilot]

[nitpick] The order of operations has changed: Reply = false is now set after unlocking (line 440) instead of before (old line 438). While functionally correct since we return immediately, maintaining the original order would make the intent clearer that we're rejecting the request before releasing resources.

Suggested change

	GlobalTransactionLock.Unlock(id)
	Reply = false
	Reply = false
	GlobalTransactionLock.Unlock(id)