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

Track6_Allow_DHCPD6 #2460

Closed
wants to merge 1 commit into from
Closed

Track6_Allow_DHCPD6 #2460

wants to merge 1 commit into from

Conversation

marjohn56
Copy link
Member

This is the initial commit. There is much more work to do, dhcpd6.conf will need to be re-writen on a prefix change is the major omission at this point, but this commit will allow the user to manually set dhcpd6 range and RADVD options. To enable, go to the System->Settings->General Page and enable Manual IPv6 DHCPD and RA,

@fichtner
Copy link
Member

download

Nice, looks simple enough, but review will have to wait a couple of days.

@marjohn56
Copy link
Member Author

marjohn56 commented Jun 13, 2018

There's no rush on this, need to get this one right. pf had it but it used to fail when prefix changed, that I want to make sure does not happen here. We'll need a catch somewhere that tells us the prefix has changed and then a call to re-write the dhcpd6.conf.

  1. How do we signal the clients that the prefix has changed and force them to change?
  2. We will need to find a way to update the statically assigned addresses
  3. We will need to update Unbound to reflect those static changes if needed.

@marjohn56
Copy link
Member Author

After looking at this, I am inclined to disable the subnet prefix ability when in this mode. The issue being that if I request a /56 prefix, I have no way of saying where in the /56 subnet range my LAN interface is going to get an address, and as I cannot have the ranges crossing it makes it possible that a clash can happen.

Thoughts..

@marjohn56
Copy link
Member Author

marjohn56 commented Jun 24, 2018

Well it does work, the PD delegation appears to work fine. dhcpd6,conf needs to be re-written and dhcpd needs to be restarted on a WAN event in case the prefix has changed. Would a call to services_dhcpd6_configure() suffice?

@fichtner
Copy link
Member

Do we need to restart from WAN on IPv6 change? Something similar to this? 3b7c0bc#diff-1332c372788c9e1a8c6c9bae9ebb55a5R3818

I recently removed a side effect here, maybe it made it worse... eb1e396#diff-1332c372788c9e1a8c6c9bae9ebb55a5L2595

<tr>
<td><a id="help_for_dhcpd6_opt" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("Manual IPv6 DHCPD and RA"); ?></td>
<td>
<input name="dhcpd6track6allowoverride" type="checkbox" value="yes" <?= $pconfig['dhcpd6track6allowoverride'] ? 'checked="checked"' : '' ?>/>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be global. Either we set this in the interface, or we don't set anything and figure it out from the dhcpv6 server side

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add it to the interface itself I suppose.

@@ -307,7 +309,7 @@
<tr>
<td><a id="help_for_theme" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("Theme"); ?></td>
<td>
<select name="theme" class="selectpicker">
<select name="theme" class="selectpicker" data-size="10" data-width="auto">
Copy link
Member

Choose a reason for hiding this comment

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

merge issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Need to do a pull... my bad, sorry!

@@ -152,8 +152,10 @@ function gen_subnet($ipaddr, $bits)
/* return the subnet address given a host address and a subnet bit count */
function gen_subnetv6($ipaddr, $bits)
{
if (!is_ipaddrv6($ipaddr) || !is_numeric($bits)) {
if(!isset($config['system']['dhcpd6track6allowoverride'])) {
Copy link
Member

Choose a reason for hiding this comment

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

if we need to push such high level override logic into these innocent functions the whole approach is flawed ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well if we do it by interface then we'll still have to do this.

Copy link
Member

Choose a reason for hiding this comment

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

makes zero sense to me, the caller of gen_subnetv6() should do what is necessary. I'm guessing that's in the radvd or dhcp6 code?

Copy link
Member Author

Choose a reason for hiding this comment

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

The key there is the line at 155, if we don't override it there and let the function continue then we return nothing, and we need a range for dhcp6.conf

Copy link
Member

Choose a reason for hiding this comment

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

sure, but that's dhcp's problem, not gen_subnetv6(). this code here merely introduces a multitude of regressions into unrelated code if dhcpd6track6allowoverride were set, like OpenVPN...

Copy link
Member

Choose a reason for hiding this comment

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

yes and no, we need to address this specifically in "Line 1249 onward in services.inc..."

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use the same lines 161 to 163 in from util.inc - the Net_IPV6:: calls and place them directly into services.inc..

Copy link
Member

Choose a reason for hiding this comment

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

sooo you want to bypass the sanity check to generate a subnet anyway? why not

function gen_subnetv6($ipaddr, $bits, $validate = true)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no sanity on my desk.. :) Sorry I got distracted setting up my new new Qotom.

OK, so we call the gen_subnetv6() which by default is validate true, but we use the status of the validate flag instead of the config->dhcpd6track6allowoverride. Is that your thought?

Copy link
Member

Choose a reason for hiding this comment

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

yep, sounds like a step forward. as far as semantics go "dhcpd6track6allowoverride" doesn't really tell us why we override it so we also need a comment there why we disable validation in this particular case, but first things first...

@marjohn56
Copy link
Member Author

OK... let's see where this takes us. :)

@@ -137,7 +137,7 @@
}

$config['system']['dnsallowoverride'] = !empty($pconfig['dnsallowoverride']);

Copy link
Member

Choose a reason for hiding this comment

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

small merge hiccup still, shouldn't touch this file :)

$dhcpdv6cfg[$ifname]['range']['from'] = Net_IPv6::compress(implode(":", $ifcfgipv6arr));
$ifcfgipv6arr[7] = "2000";
$dhcpdv6cfg[$ifname]['range']['to'] = Net_IPv6::compress(implode(":", $ifcfgipv6arr));
if(!isset($config['interfaces'][$ifname]['dhcpd6track6allowoverride']) || (isset($config['interfaces'][$ifname]['dhcpd6track6allowoverride']) && !isset($config['dhcpdv6'][$ifname]['enable']))) {
Copy link
Member

Choose a reason for hiding this comment

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

this whole block could be clearer, let me try to merge tomorrow. nice work so far!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure there's a better way, but I can't see it at present.

@@ -598,7 +599,7 @@ function get_wireless_channel_info($interface) {
if (isset($config['dhcpd']) && isset($config['dhcpd'][$if]['enable']) && (! preg_match("/^staticv4/", $pconfig['type']))) {
$input_errors[] = gettext("The DHCP Server is active on this interface and it can be used only with a static IP configuration. Please disable the DHCP Server service on this interface first, then change the interface configuration.");
}
if (isset($config['dhcpdv6']) && isset($config['dhcpdv6'][$if]['enable']) && (! preg_match("/^staticv6/", $pconfig['type6']))) {
if (isset($config['dhcpdv6']) && isset($config['dhcpdv6'][$if]['enable']) && (! preg_match("/^staticv6/", $pconfig['type6'])) && !isset($config['system']['dhcpd6track6allowoverride'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: this validation is very strange. we should opt for a clean service change, not block a reconfigure. Also, this might have been fixed via a0bb26b

Copy link
Member Author

Choose a reason for hiding this comment

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

What's even stranger is this one has sneaked through with $config['system'], so much for a global file search!

@marjohn56
Copy link
Member Author

Heavy mods to services_dhcpv6.php to attempt to make it foolproof. No override of any sanity checks.

@marjohn56
Copy link
Member Author

More cleanup... much tidier

@fichtner
Copy link
Member

I like it ❤️

Merge coming up soon...

@@ -113,7 +113,7 @@ function services_radvd_configure($blacklist = array())

/* handle manually configured DHCP6 server settings first */
foreach ($config['dhcpdv6'] as $dhcpv6if => $dhcpv6ifconf) {
if (isset($config['interfaces'][$dhcpv6if]['track6-interface'])) {
if (isset($config['interfaces'][$dhcpv6if]['track6-interface']) && isset($config['interfaces'][$dhcpv6if]['dhcpd6track6allowoverride'])) {
Copy link
Member

Choose a reason for hiding this comment

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

@marjohn56 trying to make sense of this. can you walk me through it please?

Copy link
Member

Choose a reason for hiding this comment

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

to be more precise, shouldn't it be !isset() ?

@@ -134,7 +134,7 @@ function services_radvd_configure($blacklist = array())
}

$ifcfgipv6 = get_interface_ipv6($dhcpv6if);
if (!is_ipaddrv6($ifcfgipv6)) {
if (!is_ipaddrv6($ifcfgipv6) && !isset($config['interfaces'][$dhcpv6if]['dhcpd6track6allowoverride'])) {
Copy link
Member

Choose a reason for hiding this comment

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

especially with this in mind...

Copy link
Member Author

Choose a reason for hiding this comment

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

Out earning pennies at moment
I'll have a look when I get home.

@fichtner fichtner closed this in 733f505 Jun 27, 2018
@marjohn56 marjohn56 deleted the DHCPD6 branch July 1, 2018 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants