Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option helper for AutoConfigure #525

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

candlerb
Copy link
Contributor

This isn't really a fully-fledged pull request: I'm seeking advice on how best to expose the option AutoConfigure from RFC 2563.

What the proposed patch does is implement a Byte option, based on Uint16 from dhcpv4/option_maximum_dhcp_message_size.go

However, other possibilities would be:

  1. Implement a Bool option
  2. Implement an enumeration which is specific to AutoConfigure - so that for example, the String() method would return "Auto-Configure: DoNotAutoConfigure" or "Auto-Configure: AutoConfigure"
  3. Do nothing and require clients to use GenericOption directly. However there could be a small helper for setting this generic option, along the lines of OptClientIdentifier() in dhcpv4/option_misc.go

Based on the registry, some other examples of single-byte DHCP options are:

  • 19 Forward On/Off (RFC 2132)
  • 20 SrcRte On/Off (RFC 2132)
  • 23 Default IP TTL (RFC 2132)
  • ... various other obsolete-looking options with length 1
  • Weirdly, option 116 (Auto-Config) is described as having a length of N, not 1.

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

Attention: Patch coverage is 96.77419% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.35%. Comparing base (3c11703) to head (45ea332).

Files Patch % Lines
dhcpv4/option_autoconfigure.go 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
+ Coverage   73.21%   73.35%   +0.14%     
==========================================
  Files          80       81       +1     
  Lines        5073     5104      +31     
==========================================
+ Hits         3714     3744      +30     
- Misses       1216     1217       +1     
  Partials      143      143              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@candlerb candlerb force-pushed the candlerb/autoconfigure branch 3 times, most recently from de3c46c to 05f007c Compare February 26, 2024 05:06
dhcpv4/dhcpv4.go Outdated Show resolved Hide resolved
@candlerb candlerb force-pushed the candlerb/autoconfigure branch 3 times, most recently from 77bb6b9 to 1192794 Compare February 26, 2024 08:20
@candlerb
Copy link
Contributor Author

Ready for review

@candlerb candlerb changed the title Option helper for Byte Option helper for AutoConfigure Feb 26, 2024
@candlerb
Copy link
Contributor Author

I have changed the AutoConfigure() helper to return (value, bool) instead of (value, error). I do need this one for coredhcp.

Please could you kick off the CI, and if it's happy, I'm good to merge this now.

dhcpv4/options.go Outdated Show resolved Hide resolved
@pmazzini pmazzini merged commit c728f5d into insomniacslk:master Feb 27, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants