Skip to content

Conversation

davi2367
Copy link
Contributor

@davi2367 davi2367 commented May 16, 2025

Change summary

The change adds support for VRF in the KEA DHCP-server

Change implemtes desired config from T6211

set interfaces ethernet eth0 address '192.168.1.1/24'
set interfaces ethernet eth0 vrf foo
set vrf name foo service dhcp-server shared-network-name eth1 option default-router '192.168.1.1'
set vrf name foo service dhcp-server shared-network-name eth1 subnet 192.168.1.0/24 lease '300'
set vrf name foo service dhcp-server shared-network-name eth1 subnet 192.168.1.0/24 range default start '192.168.1.10'
set vrf name foo service dhcp-server shared-network-name eth1 subnet 192.168.1.0/24 range default stop '192.168.1.100'
set vrf name foo service dhcp-server shared-network-name eth1 subnet 192.168.1.0/24 subnet-id '1'
set vrf name foo table '101'

Unsure if tests should be placed in the VRF segment or in the DHCP segment - therefor no tests are added for the VRF part - yet

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T6211

Related PR(s)

vyos/vyos-documentation#1637

How to test / Smoketest result

ran tests locally and the pass - a new test was added for vrf to ensure it is checked

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

Copy link

github-actions bot commented May 16, 2025

👍
No issues in PR Title / Commit Title

@davi2367 davi2367 marked this pull request as ready for review May 20, 2025 00:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds VRF support to the KEA DHCP server by extending control-socket calls, CLI commands, templates, systemd units, and configuration mode scripts to accept and propagate a --vrf parameter.

  • Extended dhcp.py and underlying Kea API (vyos/kea.py) to accept vrf/vrf_name
  • Updated service definitions, systemd overrides, op-mode XML, interface definitions, and Jinja templates for VRF support
  • Adapted smoke tests and configuration mode scripts to inject VRF context via argv

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/op_mode/dhcp.py Updated commands to accept --vrf, modified function signatures and service names, but introduced mis-wired pool sizing
python/vyos/kea.py Extended Kea API calls (_ctrl_socket_command, lease/config getters) to include vrf_name
src/conf_mode/service_dhcp-server.py & service_dhcpv6-… Detect VRF context via argv and set config/control-socket paths accordingly
src/etc/systemd/system/kea-dhcp*[email protected]/… Added ip vrf exec overrides for DHCP4, DHCP6, and DDNS units
op-mode-definitions/.xml.in & interface-definitions/ Added <tagNode name="vrf"> blocks for new CLI commands and VRF under interface definitions
data/templates/dhcp-server/kea-dhcp*.j2 Conditionally generate control-socket paths based on vrf_context
Comments suppressed due to low confidence (5)

src/op_mode/dhcp.py:177

  • config is not defined inside _get_pool_size. You need to pass the active configuration object into _get_pool_size or capture it from the outer scope.
subnets = config.list_nodes(f'{base} subnet')

src/op_mode/dhcp.py:196

  • The call to _get_pool_size does not pass the vrf argument, so pool sizes will always be computed for the global context rather than the selected VRF. Change to _get_pool_size(pool=p, family=family, vrf=vrf).
size = _get_pool_size(family=family, pool=p)

src/op_mode/dhcp.py:85

  • [nitpick] The parameter name sorted shadows the built-in Python sorted function. Consider renaming it to sort_key or sort_by for clarity.
def _get_raw_server_leases(config, family='inet', vrf='', pool=None, sorted=None, state=[], origin=None)

smoketest/scripts/cli/test_service_dhcp-server.py:100

  • [nitpick] There are no smoke tests for the new VRF-aware show_server_leases commands. Add tests for show dhcp-server leases vrf <name> to validate CLI and config output.
def verify_service_running(self):

src/system/on-dhcp-event.sh:33

  • [nitpick] Indentation around this line is inconsistent with surrounding lines, which could affect readability or heredoc parsing. Align it with the other commands in this block.
config = kea_get_active_config('4', '')

@sever-sever
Copy link
Member

Please combine ten commits into a single commit, e.g. with git rebase -i HEAD~10

Copy link
Member

@sarthurdev sarthurdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI tests failing:

DEBUG - Running Testcase: /usr/libexec/vyos/tests/smoke/cli/test_service_dhcp-server.py
DEBUG - test_dhcp_dynamic_dns_update (__main__.TestServiceDHCPServer.test_dhcp_dynamic_dns_update) ... ERROR
DEBUG - test_dhcp_exclude_in_range (__main__.TestServiceDHCPServer.test_dhcp_exclude_in_range) ... ERROR
DEBUG - test_dhcp_exclude_not_in_range (__main__.TestServiceDHCPServer.test_dhcp_exclude_not_in_range) ... ERROR
DEBUG - test_dhcp_high_availability (__main__.TestServiceDHCPServer.test_dhcp_high_availability) ... ERROR
DEBUG - test_dhcp_high_availability_standby (__main__.TestServiceDHCPServer.test_dhcp_high_availability_standby) ... ERROR
DEBUG - test_dhcp_hostsd_lease_sync (__main__.TestServiceDHCPServer.test_dhcp_hostsd_lease_sync) ... ERROR
DEBUG - test_dhcp_multiple_pools (__main__.TestServiceDHCPServer.test_dhcp_multiple_pools) ... ERROR
DEBUG - test_dhcp_on_interface_with_vrf (__main__.TestServiceDHCPServer.test_dhcp_on_interface_with_vrf) ... ERROR
DEBUG - test_dhcp_relay_server (__main__.TestServiceDHCPServer.test_dhcp_relay_server) ... ERROR
DEBUG - test_dhcp_single_pool_options (__main__.TestServiceDHCPServer.test_dhcp_single_pool_options) ... ERROR
DEBUG - test_dhcp_single_pool_options_scoped (__main__.TestServiceDHCPServer.test_dhcp_single_pool_options_scoped) ... ERROR
DEBUG - test_dhcp_single_pool_range (__main__.TestServiceDHCPServer.test_dhcp_single_pool_range) ... ERROR
DEBUG - test_dhcp_single_pool_static_mapping (__main__.TestServiceDHCPServer.test_dhcp_single_pool_static_mapping) ... ERROR
DEBUG - 
DEBUG - ======================================================================
DEBUG - ERROR: test_dhcp_dynamic_dns_update (__main__.TestServiceDHCPServer.test_dhcp_dynamic_dns_update)
DEBUG - ----------------------------------------------------------------------
DEBUG - Traceback (most recent call last):
DEBUG -   File "/usr/libexec/vyos/tests/smoke/cli/test_service_dhcp-server.py", line 1262, in test_dhcp_dynamic_dns_update
DEBUG -     config = read_file(KEA4_CONF)
DEBUG -              ^^^^^^^^^^^^^^^^^^^^
DEBUG -   File "/usr/lib/python3/dist-packages/vyos/utils/file.py", line 44, in read_file
DEBUG -     raise e
DEBUG -   File "/usr/lib/python3/dist-packages/vyos/utils/file.py", line 38, in read_file
DEBUG -     with open(fname, 'r') as f:
DEBUG -          ^^^^^^^^^^^^^^^^

@dmbaturin dmbaturin changed the title kea: T6211: add vrf support for KEA dhcp server kea: T6211: add VRF support for KEA dhcp server May 20, 2025
@davi2367 davi2367 force-pushed the vrf-dhcp branch 9 times, most recently from 512455c to 3fd44d7 Compare May 21, 2025 14:54
@davi2367 davi2367 force-pushed the vrf-dhcp branch 2 times, most recently from 1184bef to 1aca411 Compare May 23, 2025 10:42
@davi2367
Copy link
Contributor Author

davi2367 commented May 23, 2025

Current failing tests seem to be in conntrack-sync witht the firewall

DEBUG - ======================================================================
DEBUG - ERROR: test_conntrack_ignore (__main__.TestSystemConntrack.test_conntrack_ignore)
DEBUG - ----------------------------------------------------------------------
DEBUG - Traceback (most recent call last):
DEBUG -   File "/usr/libexec/vyos/tests/smoke/cli/test_system_conntrack.py", line 235, in test_conntrack_ignore
DEBUG -     self.cli_commit()
DEBUG -   File "/usr/libexec/vyos/tests/smoke/cli/base_vyostest_shim.py", line 103, in cli_commit
DEBUG -     out = self._session.commit()
DEBUG -           ^^^^^^^^^^^^^^^^^^^^^^
DEBUG -   File "/usr/lib/python3/dist-packages/vyos/configsession.py", line 280, in commit
DEBUG -     out = self.__run_command([COMMIT])
DEBUG -           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
DEBUG -   File "/usr/lib/python3/dist-packages/vyos/configsession.py", line 210, in __run_command
DEBUG -     raise ConfigSessionError(output)
DEBUG - vyos.configsession.ConfigSessionError: [ system conntrack ]
DEBUG - 
DEBUG - WARNING: It is prefered to define ipv4 conntrack ignore rules in
DEBUG - <firewall ipv4 prerouting raw> section
DEBUG - 
DEBUG - 
DEBUG - WARNING: It is prefered to define ipv6 conntrack ignore rules in
DEBUG - <firewall ipv6 prerouting raw> section
DEBUG - 
DEBUG - Failed to apply configuration: /run/nftables-ct.conf:12:56-58: Error:
DEBUG - Basetype of type internet network service is not bitmask         meta
DEBUG - l4proto udp ip saddr  192.0.2.1 th dport  500,4500 counter notrack
DEBUG - comment "ignore-3"
DEBUG - ^^^
DEBUG - [[system conntrack]] failed
DEBUG - [ firewall ]
DEBUG - 
DEBUG - WARNING: It is prefered to define ipv4 conntrack ignore rules in
DEBUG - <firewall ipv4 prerouting raw> section
DEBUG - 
DEBUG - 
DEBUG - WARNING: It is prefered to define ipv6 conntrack ignore rules in
DEBUG - <firewall ipv6 prerouting raw> section
DEBUG - 
DEBUG - dependent nat: dependent system_conntrack: Failed to apply
DEBUG - configuration: /run/nftables-ct.conf:12:56-58: Error: Basetype of type
DEBUG - internet network service is not bitmask         meta l4proto udp ip
DEBUG - saddr  192.0.2.1 th dport  500,4500 counter notrack comment "ignore-3"
DEBUG - ^^^
DEBUG - [[firewall]] failed
DEBUG - Commit failed

@davi2367 davi2367 requested a review from sarthurdev May 26, 2025 07:06
@alexandr-san4ez
Copy link
Contributor

@davi2367 I still see quite a lot of changes made by the linter in the file python/vyos/template.py. Could you please revert the linter's changes to make this PR more convenient to work with?

Copy link

github-actions bot commented Jul 28, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@davi2367
Copy link
Contributor Author

I have removed all linting corrections made by ruff to lines i havent added, or altered.

Please note that large sectons of the the files below, are moved into "include" in order to reuse them in the VRF submeny, similar to how eg. BGP works.

interface-definitions/service_dhcp-server.xml.in

the large section is moved to "interface-definitions/include/dhcp/dhcp-server-common-config.xml.i" in order to avoid duplicate code

interface-definitions/service_dhcpv6-server.xml.in

the same applies to this file, the large section is moved to "interface-definitions/include/dhcp/dhcpv6-server-common-config.xml.i"

@davi2367
Copy link
Contributor Author

recheck

@davi2367
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@sever-sever
Copy link
Member

recheck

vyosbot added a commit to vyos/vyos-cla-signatures that referenced this pull request Jul 28, 2025
@davi2367
Copy link
Contributor Author

Thanks for the review alexandr-san4ez

I have fixed the issues pointed out :)

@davi2367 davi2367 reopened this Aug 15, 2025
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests ❌ failed

Copy link
Contributor

@alexandr-san4ez alexandr-san4ez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition of VRF support in the KEA DHCP server looks ready.
Noting the CI TPM test failures, but local tests passed, everything looks solid for merging.

@l0crian1
Copy link
Contributor

Sorry to bring this up on the 11th hour.

I think there was an error in logic when configuration elements were moved under the vrf node (in general; not just this PR specifically). By doing this, the configuration script for a given section would be run once per VRF. This causes a ballooning of the commit and boot times of the installation.

For instance, if you have 100 VRFs with BGP configured, it will run protocols_bgp.py 100 times. In this case, service_dhcp-server.py would be run once per VRF.

I think it may be better to simply add a vrf node to the service dhcp(v6)-server path. So it'd look like this:

set service dhcp-server shared-network-name eth1 vrf foo

This would probably also greatly simplify the implementation of this PR.

Just my thoughts.

@sever-sever
Copy link
Member

Sorry to bring this up on the 11th hour.

I think there was an error in logic when configuration elements were moved under the vrf node (in general; not just this PR specifically). By doing this, the configuration script for a given section would be run once per VRF. This causes a ballooning of the commit and boot times of the installation.

For instance, if you have 100 VRFs with BGP configured, it will run protocols_bgp.py 100 times. In this case, service_dhcp-server.py would be run once per VRF.

I think it may be better to simply add a vrf node to the service dhcp(v6)-server path. So it'd look like this:

set service dhcp-server shared-network-name eth1 vrf foo

This would probably also greatly simplify the implementation of this PR.

Just my thoughts.

As it is a separate instance with its own config, it is better to start it separately under its own VRF context
So each VRF has its own dictionary with its own lease files. The implementation looks logical.
I can have the same IP ranges/pools in the different VRFs.
The commit/boot time is a separate question that requires discussion. I do not want to prevent the current implementation.

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fine now, all concerns are addressed. Let's give it a go in rolling.

@dmbaturin dmbaturin dismissed sarthurdev’s stale review August 19, 2025 14:25

Change requests are addressed

@dmbaturin dmbaturin merged commit 731e219 into vyos:current Aug 19, 2025
24 of 28 checks passed
@vyosbot vyosbot added mirror-initiated This PR initiated for mirror sync workflow mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels Aug 19, 2025
@MattKobayashi
Copy link
Contributor

MattKobayashi commented Aug 19, 2025

This has broken vyos-1x package builds.

if not _lease_valid(inet, address):
is missing the vrf parameter in the function call.

@davi2367
Copy link
Contributor Author

I've created a new PR(don't know if i could edit it in this pr/reopen it somehow), fixing the issue pointed out by MattKobayashi

#4671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

8 participants