openwrt / odhcpd

This repository is a mirror of https://git.openwrt.org/?p=project/odhcpd.git. Pull requests will be accepted which will be merged in odhcpd.git
GNU General Public License v2.0
162 stars 98 forks source link

odhcpd: add support for dhcpv6_pd_min parameter #184

Closed jtkohl closed 1 year ago

jtkohl commented 2 years ago

the dhcpv6_pd_min option clamps the requested prefix delegation to be at least as big as the option. This allows a router to manage the size of each downstream router's prefix delegation length independently from the delegating interface's prefix length.

Signed-off-by: John Kohl jtk.git@bostonpog.org

jtkohl commented 2 years ago

@dedeckeh

jtkohl commented 2 years ago

@dedeckeh

Hi, any chance to review this/comments/suggestions?

gtxaspec commented 1 year ago

waiting on this to be merged too... looking forward to support for this!

jonathanunderwood commented 1 year ago

@Ansuel @dedeckeh Any chance we can get this into 23.05?

Ansuel commented 1 year ago

Also is this supported or do we have and RFC for this feature?

jtkohl commented 1 year ago

Also is this supported or do we have and RFC for this feature?

I think we end up in the "implementer's option" spaces of RFC3633 and RFC8168, where the behavior can be chosen to meet our desires.

RFC8168 section 3.2 and 3.3 seems on point here. 3.2 starts:

[RFC3633] allows a client to include a prefix-length hint in the Solicit message to signal its preference to the server. How the prefix-length hint should be handled by the server is unclear. The client might want a different prefix length due to configuration changes or it might just want the same prefix again after reboot. The server should interpret these cases differently. Many servers are configured to provide only prefixes of specific lengths to the client, for example, if the client requested for a /54 but the server could only provide /30, /48, and /56. How should these servers decide which prefix to give to the client based on the prefix-length hint?

RFC3633 section 11.2 describes the delegating router's behavior, in part:

If the requesting router includes an IA_PD Prefix option in the IA_PD option in its Solicit message, the delegating router MAY choose to use the information in that option to select the prefix(es) or prefix size to be delegated to the requesting router. The delegating router sends an Advertise message to the requesting router in the same way as described in section, "Creation and transmission of Advertise messages" of RFC 3315. The delegating router MUST include an IA_PD option, identifying any prefix(es) that the delegating router will delegate to the requesting router.

I read this to indicate that the delegating router is free to make its own choices about how to respond to an IA_PD, including altering the delegated prefix length to differ from the requested prefix length hint

Ansuel commented 1 year ago

I think we end up in the "implementer's option" spaces of RFC3633 and RFC8168, where the behavior can be chosen to meet our desires.

I know it will be a PITA but I would ask you to add all these reference to justify this feature...

Anyway on to the real discussion of this. (thanks for the RFC reference so we know what this is)

This thing is really just another ""hint"" that can be communicated to the server. So what you actually get is quite different. Because of this I would rename the variables to some thing like dhcpdv6_pd_hint_min and in commit description and in the comments stress that it's really an hint to the remote server and what you actually get can be different than what you requested. (as described in the RFC)

I would move the check condition in dhcpv6-ia at line 1324 and I would operate directly in the reqlen value (it's local and used only for this task)

Also I would drop the MIN_DEFAULT define and just use MAX directly (that from what I can see should be 62 and not 64) looking at the code in dhcpdv6-ia for reqlen.

Also about formal issue. I would ask you to force push and keep everything in one single commit and to drop any merge commit. I you need help with git commands feel free to ask but in short

And yes I would ask you to add the info pointed in the RFC about this feature.

Again thanks for helping me in having this feature merged.

jtkohl commented 1 year ago

I know it will be a PITA but I would ask you to add all these reference to justify this feature...

Not a problem.

Anyway on to the real discussion of this. (thanks for the RFC reference so we know what this is)

This thing is really just another ""hint"" that can be communicated to the server. So what you actually get is quite different. Because of this I would rename the variables to some thing like dhcpdv6_pd_hint_min and in commit description and in the comments stress that it's really an hint to the remote server and what you actually get can be different than what you requested. (as described in the RFC)

Keep in mind that odhcpd in this case is the delegating server. This setting is not changing the hint that the requesting router sent--that router can send whatever it likes. What the setting changes is what prefix length is assigned and delegated. It's setting a policy that the delegating server (us) will only delegate prefix lengths >= the specified value. I don't think pd_hint_min describes that. Maybe dhcpdv6_pd_min_len ?

I would move the check condition in dhcpv6-ia at line 1324 and I would operate directly in the reqlen value (it's local and used only for this task)

Also I would drop the MIN_DEFAULT define and just use MAX directly (that from what I can see should be 62 and not 64) looking at the code in dhcpdv6-ia for reqlen.

Let me re-read all the related code and then look at this suggestion.

Also about formal issue. I would ask you to force push and keep everything in one single commit and to drop any merge commit.

Sure, I'll do that at the end, once we have an agreed implementation. (I come from the philosophy that intermediate development history is important to preserve. I believe I can do that at least in my own repository on a branch, and do the equivalent of a squash commit to prepare the final pull request content.)

And yes I would ask you to add the info pointed in the RFC about this feature.

Sure thing, in the code comments and in commit comments.

Ansuel commented 1 year ago

@jtkohl Thanks a lot for the fixup, I fixed some formal issue and merged. I will not proceed with bumping the package in main and backport to 23.05

Ansuel commented 1 year ago

@jtkohl also would really love if you could add support for this in luci...

gtxaspec commented 1 year ago

+1 for luci support! =D

jtkohl commented 1 year ago

Not sure when I will have time to get to LuCI support, but yes that's the right next step!

jtkohl commented 1 year ago

LuCI PR: https://github.com/openwrt/luci/pull/6443