First attempt at kea dhcp support#1636
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a Kea DHCP API device-scanner plugin: plugin config/schema, a Python importer that fetches and imports IPv4 leases from Kea via its control-agent API, and README documentation for setup and required settings. ChangesKea API Plugin Feature
sequenceDiagram
participant Importer as Kea Importer
participant KeaAPI as Kea Control Agent API
participant Validator as is_mac
participant PluginObjects as Plugin_Objects
Importer->>Importer: Read KEALSS_URL/USER/PASS and timeout
Importer->>KeaAPI: POST {"command":"lease4-get-all","service":"dhcp4"} with BasicAuth
KeaAPI-->>Importer: JSON response (result, leases, text)
alt result == 0
loop leases
Importer->>Validator: validate hw-address
Validator-->>Importer: valid/invalid
Importer->>PluginObjects: add_object(primaryId=MAC, secondaryId=IP, watched fields, foreignKey=MAC)
end
Importer->>PluginObjects: write_result_file(RESULT_FILE)
else result == 1
Importer->>Importer: log API error
else result == 3
Importer->>Importer: log no entries found
end
Importer->>Importer: log completion/count/text
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
front/plugins/kea_api/README.md (1)
70-71: 💤 Low valueConsider adding a troubleshooting section.
The documentation could benefit from a brief troubleshooting section covering common issues such as connection failures, authentication errors, or timeout problems.
📚 Example section to add
### Troubleshooting **Connection errors:** - Verify the Kea Control Agent is running and accessible at the configured URL - Check firewall rules if connecting to a remote host - Confirm the control socket path matches between `kea-dhcp4` and `kea-ctrl-agent` configurations **Authentication failures:** - Ensure the username and password match the values in the password file - Verify the password file path is correct in the ctrl-agent configuration **Timeout issues:** - If imports are failing, increase the `RUN_TIMEOUT` setting in the plugin configuration🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/README.md` around lines 70 - 71, Add a "Troubleshooting" section to front/plugins/kea_api/README.md that covers common connection, authentication, and timeout issues: explain verifying the Kea Control Agent is running and reachable at the configured URL, checking firewall rules and control socket path consistency between kea-dhcp4 and kea-ctrl-agent, validating username/password and the kea-api-password file path used by the ctrl-agent, and advise increasing the plugin RUN_TIMEOUT setting for import/timeouts; keep the section brief with bullet-style checks and concrete configuration symbols (kea-dhcp4, kea-ctrl-agent, kea-api-password, RUN_TIMEOUT) for easy discovery.front/plugins/kea_api/script.py (3)
40-40: ⚡ Quick winRename the loop variable for clarity.
The variable
lis ambiguous. Use a descriptive name likeleaseto improve readability.♻️ Proposed fix
- for l in leases: - mac = l['hw-address'] - state = l['state'] + for lease in leases: + mac = lease['hw-address'] + state = lease['state'] if is_mac(mac): plugin_objects.add_object( primaryId = mac, - secondaryId = l['ip-address'], + secondaryId = lease['ip-address'], # Active or not, similar to watched 1 of DHCPLSS plugin watched1 = state == 0, - watched2 = l['hostname'], + watched2 = lease['hostname'],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/script.py` at line 40, The loop variable `l` in the iteration `for l in leases:` is ambiguous; rename it to a descriptive name like `lease` throughout that loop (change `l` references inside the loop body to `lease`) to improve readability and maintainability; ensure any inner references, function calls, or assignments that used `l` are updated to `lease` so behavior remains unchanged.
23-25: ⚡ Quick winConsider validating the configuration values.
The script retrieves
URL,USER, andPASSbut does not validate them. Empty or malformed values could cause runtime errors.🛡️ Proposed validation
url = get_setting_value(f'{pluginName}_URL') user = get_setting_value(f'{pluginName}_USER') password = get_setting_value(f'{pluginName}_PASS') + + if not url or not user or not password: + mylog('none', [f'[{pluginName}] ⚠ ERROR: Missing required configuration (URL, USER, or PASS)']) + return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/script.py` around lines 23 - 25, The code fetches plugin config values (url, user, password) via get_setting_value but does not validate them; add validation after those calls to ensure url, user, and password are non-empty and that url is a well-formed URL (e.g., parse with a URL parser and check scheme/netloc) and if invalid, log a clear error (including pluginName) and raise/exit so the caller is not given malformed config; update the block around get_setting_value(...) to perform these checks and fail-fast with descriptive messages.
57-58: ⚡ Quick winInclude error details from the Kea API response.
When
result == 1, the Kea API response may include additional error information in thetextorargumentsfields. Logging these details would help with troubleshooting.📝 Proposed enhancement
elif entry.get("result") == 1: - mylog('none', [f'[{pluginName}] ⚠ ERROR: Kea API indicated error']) + error_text = entry.get("text", "unknown error") + mylog('none', [f'[{pluginName}] ⚠ ERROR: Kea API indicated error: {error_text}'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/script.py` around lines 57 - 58, In the branch checking elif entry.get("result") == 1, update the mylog call so it includes the Kea API error details from the response (e.g., entry.get("text") and entry.get("arguments")) alongside pluginName; locate the conditional on entry.get("result") in script.py and change the log invocation in that block (where mylog is called) to concatenate or format a message that includes the text and arguments values when present so the log shows both the original warning and the API-provided error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/kea_api/config.json`:
- Line 309: The RUN_TIMEOUT default in front/plugins/kea_api/config.json is too
low (currently "default_value": 5) and will kill the plugin before its 10s HTTP
request finishes; update the default_value for RUN_TIMEOUT to a larger value
(e.g., 15) so it comfortably exceeds the 10-second HTTP timeout plus processing
overhead, ensuring the plugin's HTTP requests can complete without premature
termination.
- Line 332: In the plugin config JSON, update the elementOptions entries so the
key reads "ordeable" instead of "orderable" to match frontend compatibility;
locate each elementOptions object that currently contains { "multiple": "true",
"orderable": "true" } and change the "orderable" key to "ordeable" for all
occurrences (including the instance shown in the diff and the other similar
elementOptions blocks present elsewhere in the file).
In `@front/plugins/kea_api/script.py`:
- Line 31: The requests.post call currently uses a hardcoded timeout of 10
seconds which exceeds the default RUN_TIMEOUT in config.json; change the timeout
to respect the configured run timeout (e.g., read RUN_TIMEOUT from configuration
and pass a timeout value safely below it, or set a smaller fixed timeout such as
3–4 seconds) so the HTTP request in the line invoking requests.post (the
assignment to response) completes within the process run budget; ensure you
reference the RUN_TIMEOUT config key when computing the timeout and adjust any
callers that assume a 10s timeout.
---
Nitpick comments:
In `@front/plugins/kea_api/README.md`:
- Around line 70-71: Add a "Troubleshooting" section to
front/plugins/kea_api/README.md that covers common connection, authentication,
and timeout issues: explain verifying the Kea Control Agent is running and
reachable at the configured URL, checking firewall rules and control socket path
consistency between kea-dhcp4 and kea-ctrl-agent, validating username/password
and the kea-api-password file path used by the ctrl-agent, and advise increasing
the plugin RUN_TIMEOUT setting for import/timeouts; keep the section brief with
bullet-style checks and concrete configuration symbols (kea-dhcp4,
kea-ctrl-agent, kea-api-password, RUN_TIMEOUT) for easy discovery.
In `@front/plugins/kea_api/script.py`:
- Line 40: The loop variable `l` in the iteration `for l in leases:` is
ambiguous; rename it to a descriptive name like `lease` throughout that loop
(change `l` references inside the loop body to `lease`) to improve readability
and maintainability; ensure any inner references, function calls, or assignments
that used `l` are updated to `lease` so behavior remains unchanged.
- Around line 23-25: The code fetches plugin config values (url, user, password)
via get_setting_value but does not validate them; add validation after those
calls to ensure url, user, and password are non-empty and that url is a
well-formed URL (e.g., parse with a URL parser and check scheme/netloc) and if
invalid, log a clear error (including pluginName) and raise/exit so the caller
is not given malformed config; update the block around get_setting_value(...) to
perform these checks and fail-fast with descriptive messages.
- Around line 57-58: In the branch checking elif entry.get("result") == 1,
update the mylog call so it includes the Kea API error details from the response
(e.g., entry.get("text") and entry.get("arguments")) alongside pluginName;
locate the conditional on entry.get("result") in script.py and change the log
invocation in that block (where mylog is called) to concatenate or format a
message that includes the text and arguments values when present so the log
shows both the original warning and the API-provided error details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4fcbc007-3562-4014-ac25-4a0429e3b765
📒 Files selected for processing (3)
front/plugins/kea_api/README.mdfront/plugins/kea_api/config.jsonfront/plugins/kea_api/script.py
|
Thanks a lot! Can you also add a line in the plugins table? https://github.com/netalertx/NetAlertX/blob/main/docs/PLUGINS.md#available-plugins |
jokob-sk
left a comment
There was a problem hiding this comment.
looking good, some small improvements and we are good to merge I think
Use 'l' instead of 'lease' (I see you little rabbit) Use [] over get in most places, if expected fields are missing an error is an acceptable outcome. Include 'text' field in logging. Add logging for result '3'
match request timeout to script timeout -1 second. Always use at least 1 sec. timeout for request.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
front/plugins/kea_api/script.py (1)
59-59: ⚡ Quick winConsider moving
write_result_file()outside the loop.Currently,
write_result_file()is called inside thefor entry in data:loop, which means the result file is written once for each entry withresult == 0. While Kea typically returns a single entry forlease4-get-all, if multiple entries are present, this results in redundant I/O. Moving the call outside the loop would be more efficient.♻️ Proposed refactor
count += 1 - plugin_objects.write_result_file() mylog('verbose', [f'[{pluginName}] Kea API response: {text}']) - mylog('verbose', [f'[{pluginName}] Successfully imported {count} devices reported by Kea API']) elif entry['result'] == 1: mylog('none', [f'[{pluginName}] ⚠ ERROR: Kea API indicated error: {text}']) elif entry['result'] == 3: mylog('verbose', [f'[{pluginName}] Kea API indicates no entries found: {text}']) - + + # Write results once after processing all entries + if count > 0: + plugin_objects.write_result_file() + mylog('verbose', [f'[{pluginName}] Successfully imported {count} devices reported by Kea API']) except Exception as e:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/script.py` at line 59, The loop iterates over "for entry in data:" and calls plugin_objects.write_result_file() inside the branch that checks entry["result"] == 0, causing repeated writes; move the single call to plugin_objects.write_result_file() out of the loop so the file is written once after processing all entries (keep the existing per-entry processing and any accumulation of results, then invoke write_result_file() once after the loop completes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/kea_api/script.py`:
- Line 38: Remove the unnecessary semicolon at the end of the assignment to the
variable `text` (the statement using `entry.get('text', '[API provided no
text]')`) in front/plugins/kea_api/script.py so the line reads as a normal
Python assignment without a trailing semicolon; locate the assignment to `text`
and delete the semicolon character only.
- Around line 40-41: The code directly indexes entry['result'] and
entry['arguments']['leases'] which can KeyError on malformed API responses;
before accessing, validate the top-level keys exist (e.g. check 'result' in
entry and that 'arguments' is a dict containing 'leases') and only then perform
the comparison and assign leases; on missing/invalid structure, handle
gracefully (skip this entry and log/debug a warning) so the loop using entry
remains robust.
- Around line 23-32: The code reads url, user, password, and timeout without
validation before calling requests.post; add checks after retrieving settings
(url, user, password, timeout) to ensure url/user/password are non-empty strings
and to coerce/validate timeout to a numeric value (int or float) with a safe
default (e.g., DEFAULT_TIMEOUT) if None or invalid; on invalid required settings
call mylog('error', ...) and abort/return before the requests.post call, and for
timeout use a validated numeric value (ensure >=2 or clamp) when computing
timeout=max(1, timeout-1) so the arithmetic cannot raise a TypeError. Ensure the
changes are applied around the variables named url, user, password, timeout and
before the requests.post(...) invocation.
---
Nitpick comments:
In `@front/plugins/kea_api/script.py`:
- Line 59: The loop iterates over "for entry in data:" and calls
plugin_objects.write_result_file() inside the branch that checks entry["result"]
== 0, causing repeated writes; move the single call to
plugin_objects.write_result_file() out of the loop so the file is written once
after processing all entries (keep the existing per-entry processing and any
accumulation of results, then invoke write_result_file() once after the loop
completes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8e5fe09-60a1-4bc0-9519-40ed1c9372e4
📒 Files selected for processing (3)
front/plugins/kea_api/README.mdfront/plugins/kea_api/config.jsonfront/plugins/kea_api/script.py
✅ Files skipped from review due to trivial changes (1)
- front/plugins/kea_api/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- front/plugins/kea_api/config.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/kea_api/script.py`:
- Around line 59-62: The plugin currently calls
plugin_objects.write_result_file() and logs success via mylog('verbose', [...])
inside the for entry in data: loop causing repeated writes/logs; move the
plugin_objects.write_result_file() call and the success mylog lines (the two
mylog calls that reference Kea API response and "Successfully imported {count}
devices") out of the loop so they execute once after the loop completes,
ensuring the result file is written once and the final success log reflects the
total processed count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6ec11ac-fc4b-4d4e-bde9-cd17ba0dade9
📒 Files selected for processing (1)
front/plugins/kea_api/script.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
front/plugins/kea_api/script.py (2)
23-32: 💤 Low valueConsider logging a warning when using HTTP instead of HTTPS.
Basic authentication sends credentials base64-encoded in headers, which are transmitted in plaintext over HTTP. While some deployments may use HTTP in trusted networks, logging a warning when the URL scheme is
http://would alert users to this security risk.🔒 Optional: Add HTTPS check
url = get_setting_value(f'{pluginName}_URL') user = get_setting_value(f'{pluginName}_USER') password = get_setting_value(f'{pluginName}_PASS') timeout = get_setting_value(f'{pluginName}_RUN_TIMEOUT') + + if url and not url.startswith('https://'): + mylog('none', [f'[{pluginName}] ⚠ WARNING: Using HTTP instead of HTTPS may expose credentials']) mylog('verbose', [f'[{pluginName}] Querying Kea API at {url}'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/script.py` around lines 23 - 32, The code calls requests.post with basic auth using the variable url (constructed from get_setting_value with f'{pluginName}_URL') but does not warn when the URL uses plain HTTP; add a check of the url's scheme (e.g., inspect url.startswith('http://') or parse with urllib.parse.urlparse) before making the request and call mylog('warning', [...]) with a clear message referencing pluginName and url if the scheme is "http" to alert about plaintext credential transmission; keep the existing request flow (requests.post(..., auth=(user, password), ...)) but ensure the warning is logged prior to the request.
45-58: 💤 Low valueOptionally log when invalid MAC addresses are skipped.
The code silently skips leases with invalid MAC addresses. Adding a verbose log message would help operators understand if any leases are being filtered out during import.
📝 Optional: Log skipped MACs
for lease in leases: mac = lease['hw-address'] state = lease['state'] if is_mac(mac): plugin_objects.add_object( primaryId = mac, secondaryId = lease['ip-address'], # Active or not, similar to watched1 of DHCPLSS plugin watched1 = state == 0, watched2 = lease['hostname'], watched3 = None, # Default (or assigned) (0), declined (1), expired-reclaimed (2), released (3), and registered (4)). watched4 = state, extra = None, foreignKey = mac ) count += 1 + else: + mylog('verbose', [f'[{pluginName}] Skipping lease with invalid MAC: {mac}'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@front/plugins/kea_api/script.py` around lines 45 - 58, The loop that calls is_mac(mac) currently silently skips invalid MACs; add an else branch next to the is_mac(mac) check to log skipped leases (include mac value, lease['ip-address'] and lease.get('hostname')) so operators can see which MACs were filtered out; use the module or plugin logger (e.g., an existing plugin logger or logging.getLogger(__name__)) and emit a debug/info log before continuing, leaving plugin_objects.add_object, count increment, and the rest unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/plugins/kea_api/script.py`:
- Line 51: The code accesses lease['hostname'] directly (seen in the assignment
to watched2) which can raise KeyError because Kea's lease 'hostname' is
optional; change the access to use a safe lookup like lease.get('hostname', '')
where watched2 is assigned (the lease processing block that sets watched2 =
lease['hostname']) so missing hostnames are handled gracefully without crashing
the import.
---
Nitpick comments:
In `@front/plugins/kea_api/script.py`:
- Around line 23-32: The code calls requests.post with basic auth using the
variable url (constructed from get_setting_value with f'{pluginName}_URL') but
does not warn when the URL uses plain HTTP; add a check of the url's scheme
(e.g., inspect url.startswith('http://') or parse with urllib.parse.urlparse)
before making the request and call mylog('warning', [...]) with a clear message
referencing pluginName and url if the scheme is "http" to alert about plaintext
credential transmission; keep the existing request flow (requests.post(...,
auth=(user, password), ...)) but ensure the warning is logged prior to the
request.
- Around line 45-58: The loop that calls is_mac(mac) currently silently skips
invalid MACs; add an else branch next to the is_mac(mac) check to log skipped
leases (include mac value, lease['ip-address'] and lease.get('hostname')) so
operators can see which MACs were filtered out; use the module or plugin logger
(e.g., an existing plugin logger or logging.getLogger(__name__)) and emit a
debug/info log before continuing, leaving plugin_objects.add_object, count
increment, and the rest unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9128ee6b-f410-435a-9cef-d0bb806b4630
📒 Files selected for processing (1)
front/plugins/kea_api/script.py
| secondaryId = lease['ip-address'], | ||
| # Active or not, similar to watched1 of DHCPLSS plugin | ||
| watched1 = state == 0, | ||
| watched2 = lease['hostname'], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is the hostname field always present in Kea DHCP lease4-get-all API responses, or can it be missing?
💡 Result:
The hostname field can be missing in Kea DHCP lease4-get-all API responses. The Lease4 structure includes hostname_ as std::string, with constructors defaulting it to an empty string ("")[1][2]. Database schemas define hostname as VARCHAR(255), allowing NULL or empty values[3][4]. Memfile CSV format supports empty hostname fields[3]. API responses mirror lease data, including hostname only if set; otherwise, it is absent or empty[2]. Commands like lease4-add accept optional hostname[5][6]. No sources indicate hostname is always present; it is populated from client DHCP requests or reservations but absent if unspecified[1].
Citations:
- 1: https://reports.kea.isc.org/dev_guide/d2/d08/structisc_1_1dhcp_1_1Lease4.html
- 2: https://context7.com/isc-projects/kea/llms.txt
- 3: https://reports.kea.isc.org/dev_guide/d6/dbd/dhcpDatabaseBackends.html
- 4: https://github.com/isc-projects/kea/blob/master/src/share/database/scripts/mysql/dhcpdb_create.mysql
- 5: https://reports.kea.isc.org/dev_guide/de/ddf/classisc_1_1lease__cmds_1_1LeaseCmds.html
- 6: https://lists.isc.org/pipermail/kea-users/2024-January/004518.html
Use defensive access for the optional 'hostname' field in Kea lease responses.
The code directly accesses lease['hostname'] at line 51, which will raise KeyError if the field is missing. According to the Kea API specification, the hostname field in lease4-get-all responses is optional and can be absent when not specified in client DHCP requests or reservations. This will cause the entire import to fail when processing a lease without a hostname. Use lease.get('hostname', '') instead to handle missing hostnames gracefully.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@front/plugins/kea_api/script.py` at line 51, The code accesses
lease['hostname'] directly (seen in the assignment to watched2) which can raise
KeyError because Kea's lease 'hostname' is optional; change the access to use a
safe lookup like lease.get('hostname', '') where watched2 is assigned (the lease
processing block that sets watched2 = lease['hostname']) so missing hostnames
are handled gracefully without crashing the import.
This is code that's working for me currently.
README.md included with very basic instructions.
Tested on debian13 with kea 2.6.3 (current version in debian stable repos).
Summary by CodeRabbit
New Features
Documentation