feat(dhcp-inform): add provider architecture for flexible route sources#13
Merged
feat(dhcp-inform): add provider architecture for flexible route sources#13
Conversation
BREAKING: Remove hard PostgreSQL dependency - plugin now works without database.
Route sources (in priority order):
1. Traffic Selectors (EXCLUSIVE mode) - for Windows 7 compatibility
2. Database (PostgreSQL/MySQL/SQLite) - if configured
3. Static configuration from strongswan.conf
New files:
- dhcp_inform_provider.h: Route provider interface
- dhcp_inform_ts_provider: Extract routes from IKE SA traffic selectors
- dhcp_inform_db_provider: Database routes (refactored from responder)
- dhcp_inform_static_provider: Config-based routes with per-pool support
Configuration changes:
- use_ts_routes: Enable exclusive TS mode
- routes{}: Global static routes for all clients
- pools{name}{cidr,routes{}}: Per-pool route overrides
RPM: strongswan-pgsql changed from Requires to Recommends
- Use ts->to_subnet() instead of non-existent ts->get_netmask() - Cast const char* to char* for host_create_from_string() calls - Remove unused parse_cidr() function from responder
There was a problem hiding this comment.
Pull request overview
This PR introduces a provider architecture to the dhcp-inform plugin, enabling flexible route sources through three mutually exclusive modes: Traffic Selectors (for Windows 7 compatibility), Database (PostgreSQL/MySQL/SQLite), and Static configuration. The refactoring extracts route retrieval logic into separate provider modules while maintaining backward compatibility.
Changes:
- Adds provider interface abstraction (
dhcp_inform_provider.h) with three implementations: TS, DB, and static - Refactors responder to use provider pattern with priority-based route source selection
- Updates build system and documentation to reflect new multi-mode architecture
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
dhcp_inform_provider.h |
New interface defining provider contract for route retrieval |
dhcp_inform_ts_provider.h/c |
Traffic selector provider extracting routes from IKE SAs |
dhcp_inform_static_provider.h/c |
Static configuration provider with per-pool route support |
dhcp_inform_db_provider.h/c |
Database provider for SQL-based route lookups |
dhcp_inform_responder.h/c |
Refactored to use provider pattern with mode selection logic |
dhcp_inform_plugin.h/c |
Updated dependencies to make database optional |
dhcp-inform.conf |
Comprehensive configuration documentation for all three modes |
Makefile.am |
Added new source files to build |
strongswan-sw.spec |
Updated package description and made PostgreSQL optional |
- Add NULL checks after all linked_list_create() calls - Replace atoi() with strtol() for safer CIDR prefix parsing - Add MAX_CIDR_LEN constant (43) with documentation - Use portable SQL for database provider (works with pgsql/mysql/sqlite) - Do IP-in-CIDR filtering in C code for database portability - Add comments explaining intentional code patterns: - Code duplication across providers (self-contained compilation) - Memory ownership transfer in route deduplication - Mutual exclusivity handled by priority order - Add buffer truncation check for pool names - Update documentation to reflect multi-database support
- Update RPM spec comment to clarify multi-database support (PostgreSQL/MySQL/SQLite via strongSwan SQL plugins) - Fix snprintf negative return check in static_provider (len < 0 indicates encoding error per POSIX) - Clarify IPv4-only limitation in MAX_CIDR_LEN comments (DHCP option 121/249 classless static routes is IPv4-only) - Fix misleading comment in responder: providers are mutually exclusive (TS OR DB OR static), not combined
- ts_provider: document that empty list return is intentional - dhcp-inform.conf: clarify priority selection behavior - static_provider: document prefix 0 handling and code duplication - db_provider: add duplication note and prefix 0 documentation
src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c
Outdated
Show resolved
Hide resolved
src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c
Outdated
Show resolved
Hide resolved
- Add @param this documentation to all provider.h function pointers - Add strdup NULL check in load_pools (memory allocation failure) - Add NULL check for entry->routes before calling get_count - Add NULL check for global_routes in get_routes and constructor
- Add NULL check for ts->clone() in ts_provider - Add NULL check for ts in responder enumerate loop - Add NULL check for pools linked_list_create in constructor - Fix clone_routes to handle ts->clone() failure gracefully
src/libcharon/plugins/dhcp_inform/dhcp_inform_static_provider.c
Outdated
Show resolved
Hide resolved
All three clone_routes call sites now check for NULL and return an empty list on allocation failure, preventing NULL pointer dereference in the responder. Note: Provider NULL checks already exist in responder (lines 919, 925, 931 use && short-circuit evaluation).
Rule 18 (Interface Contract): - Update provider.h get_routes docs to clarify NULL vs empty return Rule 20 (Cascading Failure): - ts_provider: handle NULL from extract_ts_from_ike_sa - Document that fallback linked_list_create may also fail (OOM) Rule 21 (Terminology): - Change "MUTUALLY EXCLUSIVE" to "PRIORITY-BASED ROUTE SELECTION" - Update dhcp-inform.conf and responder.c comments - Clarifies that multiple sources = graceful fallback, not error
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan