dhcp-server: T9202: add client identifier to DHCPv4 lease output - #5394
dhcp-server: T9202: add client identifier to DHCPv4 lease output#5394allanice001 wants to merge 1 commit into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used🔍 Remote MCP vyos.devAdditional review context
📝 WalkthroughSummary by CodeRabbit
WalkthroughDHCPv4 lease output now includes Kea client IDs. IPv4 lease formatting displays a ChangesDHCPv4 client ID
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@python/vyos/kea.py`:
- Around line 686-690: Update the client identifier assignment in the DHCPv4
lease handling to use '-' whenever lease.get('client-id') is missing, empty, or
None, while preserving non-empty Kea-provided identifiers unchanged.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c626ec32-2587-40e7-99e4-cc2dcfd96dbf
📒 Files selected for processing (2)
python/vyos/kea.pysrc/op_mode/dhcp.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Actions: CLA Check / 0_call-cla-assistant _ cla_assistant.txt: dhcp-server: T9202: add client identifier to DHCPv4 lease output
Conclusion: failure
##[group]Run contributor-assistant/github-action@ca4a40a7d1004f18d9960b404b97e5f30a505a08
with:
path-to-signatures: signatures/version1/cla.json
path-to-document: https://github.com/vyos/vyos-cla-signatures/blob/production/README.md
branch: production
allowlist: github-actions[bot], dependabot-preview[bot], insights-engineering-bot, dependabot[bot], copilot, github-copilot[bot], copilot[bot], Copilot, vyosbot, pre-commit-ci, pre-commit-ci[bot], codecov, codecov[bot], mergify, mergify[bot], netlify, netlify[bot], claude, claude[bot], coderabbitai, coderabbitai[bot]
remote-organization-name: vyos
remote-repository-name: vyos-cla-signatures
lock-pullrequest-aftermerge: false
use-dco-flag: false
suggest-recheck: true
env:
GITHUB_***REDACTED_SECRET_ASSIGNMENT***
PERSONAL_ACCESS_***REDACTED_SECRET_ASSIGNMENT***
##[endgroup]
CLA Assistant GitHub Action bot has started the process
(node:2265) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:2265) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
##[error]Committers of Pull Request number 5394 have to sign the CLA 📝
GitHub Actions: CLA Check / call-cla-assistant _ cla_assistant: dhcp-server: T9202: add client identifier to DHCPv4 lease output
Conclusion: failure
##[group]Run contributor-assistant/github-action@ca4a40a7d1004f18d9960b404b97e5f30a505a08
with:
path-to-signatures: signatures/version1/cla.json
path-to-document: https://github.com/vyos/vyos-cla-signatures/blob/production/README.md
branch: production
allowlist: github-actions[bot], dependabot-preview[bot], insights-engineering-bot, dependabot[bot], copilot, github-copilot[bot], copilot[bot], Copilot, vyosbot, pre-commit-ci, pre-commit-ci[bot], codecov, codecov[bot], mergify, mergify[bot], netlify, netlify[bot], claude, claude[bot], coderabbitai, coderabbitai[bot]
remote-organization-name: vyos
remote-repository-name: vyos-cla-signatures
lock-pullrequest-aftermerge: false
use-dco-flag: false
suggest-recheck: true
env:
GITHUB_***REDACTED_SECRET_ASSIGNMENT***
PERSONAL_ACCESS_***REDACTED_SECRET_ASSIGNMENT***
##[endgroup]
CLA Assistant GitHub Action bot has started the process
(node:2265) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:2265) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
##[error]Committers of Pull Request number 5394 have to sign the CLA 📝
🧰 Additional context used
📓 Path-based instructions (3)
python/vyos/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Python version must be >=3.11 for all code in the
vyos.*library
Files:
python/vyos/kea.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use ruff 0.6.4 for Python linting with configuration inruff.tomlat repository root
Use pylint to check for W0611 (unused imports) violations in Python code
Use darker for code formatting in Python files
Use nose2 for Python testing with configuration innose2.cfgat repository root
Files:
python/vyos/kea.pysrc/op_mode/dhcp.py
src/op_mode/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Op-mode scripts must be located in
src/op_mode/
Files:
src/op_mode/dhcp.py
🧠 Learnings (1)
📚 Learning: 2026-06-29T12:13:51.293Z
Learnt from: andamasov
Repo: vyos/vyos-1x PR: 5298
File: smoketest/scripts/cli/test_vpp.py:0-0
Timestamp: 2026-06-29T12:13:51.293Z
Learning: When reviewing vyos-1x code that parses or asserts VPP CLI output (e.g., smoketest CLI tests and VPP op-mode code), do not flag the token spelling "Forwrd" / "U-Forwrd" as a typo. It is intentionally preserved verbatim from the upstream VPP CLI text shown by commands like `vppctl show bridge-domain ... detail`. This misspelling is centrally allowlisted (vyos/.github#153) for that specific VPP-CLI context, so typo-review comments should exclude "Forwrd" when it originates from that VPP output.
Applied to files:
src/op_mode/dhcp.py
🔍 Remote MCP Context7, vyos.dev
Relevant review context
- Broad VyOS search found task T9202, titled “DHCPv4 lease output omits the client identifier”. It is Open, a feature with Requires assessment priority, targeting both VyOS 1.4 and 1.5.
- T9202 specifies the exact intended behavior: copy Kea’s
client-idfor DHCPv4, preserve its colon-separated representation, use-when absent, append aClient IDcolumn, and addclient_idsorting. - A related task shows Kea lease data can contain an empty
client_id, confirming the missing-identifier fallback is a real case worth testing. - Kea documents
lease4-get-alllease objects as containing a stringclient-id; its example uses the colon-separated format01:00:0c:01:02:03:04. - T9202 has no comment history adding further requirements.
🔇 Additional comments (3)
src/op_mode/dhcp.py (3)
54-54: LGTM!
115-128: LGTM!
141-141: LGTM!
|
I have read the CLA Document and I hereby sign the CLA |
15c8f4b to
ff22985
Compare
kea_get_server_leases() copies the DHCPv6 DUID out of the lease Kea returns but drops the DHCPv4 client identifier, so `show dhcp server leases` cannot show it and neither can anything built on the op-mode API. Kea itself has it: lease4-get-all returns 'client-id' alongside 'hw-address'. This matters wherever a reservation is matched on the client identifier rather than the MAC. Hosts using systemd-networkd send an RFC 4361 identifier -- ff:<IAID>:<DUID> -- instead of the traditional 01:<MAC>, and Kea matches a static-mapping 'duid' against the DUID embedded in it. Without the identifier in the lease output there is no way to read the value a reservation has to be written with, and a static-mapping carrying the wrong one is accepted, rendered into the Kea configuration, and silently never matches: the host keeps taking a dynamic address while every screen reports it as reserved. Kea returns client-id already colon separated, so unlike the DHCPv6 DUID it is not passed through _format_hex_string(). IP Address MAC address Hostname Client ID 192.168.10.60 00:e0:4c:68:4e:0d arya 01:00:e0:4c:68:4e:0d 192.168.10.40 52:54:00:32:6a:fa ppp-server ff:00:32:6a:fa:00:01:00:01:32:0d:2a:4c:52:54:00:32:6a:fa 192.168.10.92 52:54:00:e3:be:41 ember-u26 ff:b5:5e:67:ff:00:02:00:00:ab:11:68:46:79:5a:ab:74:cd:25 The column is appended rather than inserted next to the MAC, matching where DHCPv6 already puts its DUID and leaving the existing column order untouched. 'client_id' is also accepted as a sort key and offered in the completion list for the inet variants of 'show dhcp server leases sort'. Signed-off-by: allanice001 <allanice001@gmail.com>
ff22985 to
b16f604
Compare
Taken, thanks — pushed. For the record, since it affects how the fallback should be read: Kea does not appear to emit an empty client identifier. So this is defensive rather than a fix for observed behaviour. Worth doing anyway: it costs nothing, it survives a future change on Kea's side, and |
|
Four workflows are sitting at No rush, and no obligation to approve on my account. To make that decision cheaper, I ran what I could locally against Darker — clean, no reformatting proposed. Graylint / ruff — clean, no findings on changed lines. Typos — clean on the three files this PR touches. Unrelated to this PR, but noting it since I saw it: ISO Integration Test — can't build an ISO here, but the part of it that covers this component was run directly, on a throwaway CodeQL — not reproducible locally, and the least interesting of the four here: the change adds no new input handling, reading a field Kea already returns and placing it in an output table. Happy to rebase or adjust anything if that helps. |
|
CI integration 👍 passed! Details
|
There was a problem hiding this comment.
Pull request overview
Adds DHCPv4 client identifier visibility to the VyOS op-mode DHCP server lease output by propagating Kea’s client-id (Option 61) into the lease data model and rendering it as an additional column. This improves operational troubleshooting and enables operators to correctly match/verify static mappings that rely on client identifiers (e.g., RFC 4361-style IDs from systemd-networkd).
Changes:
- Include Kea DHCPv4
client-idin the lease dict returned bykea_get_server_leases(), defaulting to-when absent. - Extend
show dhcp server leasesoutput (and sortable keys) to exposeclient_idand render a “Client ID” column. - Update op-mode CLI completion lists to include
client_idas a valid sort key for DHCPv4 lease views.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/op_mode/dhcp.py | Adds client_id as a sortable key and appends a “Client ID” column to DHCPv4 lease table output. |
| python/vyos/kea.py | Copies Kea’s DHCPv4 client-id into the normalized lease dict (with - fallback). |
| op-mode-definitions/dhcp.xml.in | Updates show dhcp server leases ... sort completion lists to include client_id for DHCPv4. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Change summary
kea_get_server_leases()copies the DHCPv6 DUID out of the lease Kea returns, but drops the DHCPv4 client identifier.show dhcp server leasestherefore cannot show it, and neither can anything built on the op-mode API. Kea itself has the value —lease4-get-allreturnsclient-idalongsidehw-address.Two lines put it in the lease dict and a column puts it on screen.
Why it matters. A DHCPv4 static-mapping can be matched on the client identifier rather than the MAC —
static-mappingincludesinterface/duid.xml.i. Hosts running systemd-networkd send an RFC 4361 identifier,ff:<IAID>:<DUID>, rather than the traditional01:<MAC>, and with nohost-reservation-identifiersconfigured Kea matches aduidreservation against the DUID embedded in it:So the value a reservation must carry is a substring of one the operator cannot see anywhere in the CLI. A static-mapping written with the wrong one is accepted by the CLI, rendered into the Kea configuration, and then silently never matches: the host keeps taking a dynamic address while
show dhcp server static-mappingsreports it as reserved. The only way to find the real value today issudoand the lease file.The two identifier styles are visible side by side once the column exists:
Two notes on the implementation:
client-idalready colon separated, so unlike the DHCPv6duidit is not passed through_format_hex_string()— doing so would insert a second colon after every existing one.DUIDand leaves the existing column order untouched for anything parsing the text output.client_idis also accepted as asortedkey, asduidalready is for inet6.Clients that send no option 61 get
-, consistent with howhostnameandmacare handled a few lines above.Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
The patched
kea_get_server_leases()and_get_formatted_server_leases()were loaded in-process on a2026.08.05-0033-rollingdevice carrying 26 live DHCPv4 leases, against the reallease4-get-allcontrol-socket output — no system files modified:Both identifier forms render, and the DHCPv6 path is untouched — the change is inside
if inet == '4'.ruff format(line-length 88, single quotes) reports no change to any line this PR touches.Smoketest
test_service_dhcp-server.py, on a throwaway1.5-rolling-202608071457VM with this change applied:Worth being precise about what that does and does not show. The suite never calls
kea_get_server_leases()— it exercises configuration generation, not op-mode output — so it is a regression check on the path that importsvyos.kea, not evidence for the feature. The evidence for the feature is the live lease output above.Checklist
test_service_dhcp-server.py, 16 tests, OK. See above.