Skip to content

Conversation

mzpqnxow
Copy link

@mzpqnxow mzpqnxow commented Aug 18, 2025

Change summary

Support DHCP v4 option #26 - interface MTU

  • Add interface-mtu to option-v4.xml.i
  • Add interface_mtu -> interface-mtu translation in python/vyos/kea.py
  • Add to smoke tests

Note, I have not actually run the smoke tests yet, I need to set up an environment to test in

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)

Related PR(s)

None

How to test / Smoketest result

Enhancements were made to the existing test_service_dhcp-server.py script. I have not tested yet as I don't have an environment set up

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 Aug 18, 2025

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

Copy link

github-actions bot commented Aug 18, 2025


PR title does not match the required format

@mzpqnxow mzpqnxow changed the title T7723: Support DHCP option 26 in DHCP v4 server (client interface mtu) dhcp-server: T7723: Added support for DHCP Option 26 current Aug 18, 2025
@mzpqnxow mzpqnxow changed the title dhcp-server: T7723: Added support for DHCP Option 26 current T7723: dhcp-server add DHCP Option 26 current Aug 18, 2025
@mzpqnxow mzpqnxow changed the title T7723: dhcp-server add DHCP Option 26 current T7723: dhcp-server add DHCP Option 26 Aug 18, 2025
@mzpqnxow
Copy link
Author

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

@mzpqnxow
Copy link
Author

recheck

vyosbot added a commit to vyos/vyos-cla-signatures that referenced this pull request Aug 18, 2025
@mzpqnxow
Copy link
Author

Looks like I may need to amend the actual commit message, not the PR description ...

Copy link
Member

@sever-sever sever-sever left a comment

Choose a reason for hiding this comment

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

Change the commit message
The format should be like in the PR message

<properties>
<help>Interface MTU</help>
<valueHelp>
<format>u32:1-16</format>
Copy link
Member

Choose a reason for hiding this comment

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

Why 1-16?

Copy link
Author

Choose a reason for hiding this comment

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

Per the RFC, it's a 16bit value. But looking at it now, I think I misread how the notation in that file works and it should probably just be u16?

Copy link
Author

Choose a reason for hiding this comment

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

As far as the values chosen for the bounds - I'm open to any ideas on the best values to use.

I admit they're somewhat arbitrary choices (though I'm sure you recognize the upper bound as Jumbo)

In terms of standards, we could use 576 for the minimum. It's unreasonably small, but mentioned in the IPv4 RFC (RFC 791 section 3.1):

All hosts must be prepared
to accept datagrams of up to 576 octets (whether they arrive whole
or in fragments). It is recommended that hosts only send datagrams
larger than 576 octets if they have assurance that the destination
is prepared to accept the larger datagrams.

The IPv6 RFC RFC 8200, section 5 requires a minimum MTU of 1280 - but this is DHCP IPv4, so I don't think we need to be troubled with that

I can't imagine 576 actually being used anywhere. At the same time, I've actually come close to needing to go below 1280 before on IPv4 interfaces (due to use of multiple layers of tunnels). So 576 seems reasonable.

@mzpqnxow mzpqnxow requested a review from sever-sever August 29, 2025 22:24
Copy link

CI integration ❌ failed!

Details

CI logs

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

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.

2 participants