Dhcp failover config generation buggy



  • I was just wondering, why both my primary and secondary pfsense boxes put "primary" into their dhcpd.conf
    files. IMHO there's a bug in /etc/inc/services.inc (pfSense-Full-Update-2.1-RC2-amd64-20130904-2355.tgz):

    –- services.inc.orig  2013-09-05 13:23:20.000000000 +0200
    +++ services.inc        2013-09-05 15:04:00.000000000 +0200
    @@ -451,7 +451,7 @@
                            $a_vip = &$config['virtualip']['vip'];
                            if(is_array($a_vip)) {
                                    foreach ($a_vip as $vipent) {
    -                                      if($int == $real_dhcpif) {
    +                                      if (convert_friendly_interface_to_real_interface_name($vipent['interface']) == $real_dhcpif) {
                                                    /* this is the interface! */
                                                    if(is_numeric($vipent['advskew']) && ($vipent['advskew'] < "20"))
                                                            $skew = 0;

    with this fix, the secondary really does put "secondary" into dhcpd.conf, without, both configs
    wind up as "primary" here, and won't go past "recovery" state.

    Cheers,
    Markus

    EDIT: my original fix was wrong, sorry. Don't know whether this is the best approach, but it seems to work.



  • Your diff is not entirely correct either.
    I pushed a fix https://github.com/pfsense/pfsense/commit/25f9f3322863eae12f705137d8d414643d7b853f that should do the right thing.

    Can you confirm it fixes for you?



  • I changed my patch, sorry.

    I'm not sure your fix would work here. I did some logging for troubleshooting, and the comparison that
    is supposed to match looks like this here. I have to add that this specific firewall is running
    2.1-RC0 (i386) built on Sun Jun 2, so in case something in the upstream code changed, your change
    might work:

    php: : Array ( [0] => em1 [1] => Array ( [mode] => carp [interface] => lan [vhid] => 99 [advskew] => 100 [advbase] => 1 [password] => XXXX [descr] => MGMT [type] => single [subnet_bits] => 24 [subnet] => 10.99.1.1 ) [2] => em1 )

    with elements being:
    [0] $real_dhcpif
    [1] $vipent
    [2] $int

    so, the way I see it, guess_interface_from_ip($dhcpifconf['failover_peerip']) yields the physical interface, not
    the CARP interface, so the $intip = find_interface_ip($int) will give the IP address of the physical interface
    too. Your proposed fix compares $vipent['subnet'] == $intip, but that won't be the case because the VIP
    IP is not the same as the IP of the physical interface.

    Cheers,
    Markus



  • Another potential issue: the isc dhcpd.conf manual says:


          The mclt statement

    mclt seconds;

    The  mclt statement defines the Maximum Client Lead Time.  It must be
            specified on the primary, and may not be specified on the  secondary.
            ..

    ...

    however, pfsense currently sets mclt on both the primary and secondary, but only
    specifies the split value on the primary.

    Was this intentional, or did the two get mixed up?



  • I think the original intent may have been for guess_interface_from_ip() to return the actual vip interface with that specific IP.

    --- interfaces.inc.orig Fri Sep  6 00:27:12 2013
    +++ interfaces.inc      Fri Sep  6 00:31:27 2013
    @@ -4015,6 +4015,14 @@
                    /* create a route table we can search */
                    exec("netstat -rnWf inet", $output, $ret);
                    foreach($output as $line) {
    +                       if(preg_match("/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+[ ]+link[#]/", $line)) {
    +                               $fields = preg_split("/[ ]+/", $line);
    +                               if($ipaddress==$fields[0]) {
    +                                       return $fields[6];
    +                               }
    +                       }
    +               }
    +               foreach($output as $line) {
                            if(preg_match("/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+\/[0-9]+[ ]+link[#]/", $line)) {
                                    $fields = preg_split("/[ ]+/", $line);
                                    if(ip_in_subnet($ipaddress, $fields[0])) {
    

    I didn't do anything to the IPv6 side.



  • I don't think guess_interface_from_ip() returning the VIP interface would work, since the carp
    interface doesn't appear in the routing table on the BACKUP node, and even on the MASTER
    node, the lookup is done on the failover_peerip, which is not the /32 of the MASTER:

    
    MASTER:
    10.99.1.0/24       link#2             U           0 544689315   1500         em1
    10.99.1.1          link#13            UH          0         0   1500   lan_vip99
    10.99.1.2          link#2             UHS         0         0  16384         lo0
    
    SLAVE:
    10.99.1.0/24       link#2             U           0     2362   1500      em1
    10.99.1.3          link#2             UHS         0        0  16384      lo0
    
    

    Cheers,


  • Rebel Alliance Developer Netgate

    The fixes made based on this thread were backed out because they broke previously-functional failover setups.

    The code that is in place is correct for most setups, perhaps there is something else unique/incorrect about your configuration contributing to the detection failure, but we had some fallout from this while testing the -RELEASE images.

    So the code is back to how it was before, and it is confirmed working on several very large CARP+DHCP Failover setups. If it does not work for you on 2.1-RELEASE, I'd take a closer look at your configuration first.



  • @jimp:

    The code that is in place is correct for most setups, perhaps there is something else unique/incorrect about your configuration contributing to the detection failure, but we had some fallout from this while testing the -RELEASE images.

    So the code is back to how it was before, and it is confirmed working on several very large CARP+DHCP Failover setups. If it does not work for you on 2.1-RELEASE, I'd take a closer look at your configuration first.

    Okay. I don't see how the original code could be correct, since the loop variable to
    check for "a defined vip" (according to the commentary) isn't actually used for
    searching, and I pointed out that your fix was not correct either.
    But I understand you don't want to break things just before release. My later
    version seems to work correctly for me, so I'll just work with a local patch then.
    I suggest to have a look at this again in 2.1-stable .

    Cheers,
    Markus

    PS: for illustration (original):

    
                                    foreach ($a_vip as $vipent) {
                                            if($int == $real_dhcpif) {
                                                    /* this is the interface! */
                                                    if(is_numeric($vipent['advskew']) && ($vipent['advskew'] < "20"))
                                                            $skew = 0;
                                            }
                                    }
    
    

    can be transformed into:

    
               if($int == $real_dhcpif) {
                           foreach ($a_vip as $vipent) {
                                        /* this is the interface! */
                                        if(is_numeric($vipent['advskew']) && ($vipent['advskew'] < "20"))
                                                 $skew = 0;
                           }
               }
    
    

    and this just doesn't look correct to me.


Log in to reply