Project

General

Profile

Bug #4568

mlppp settings lost after save on interface page

Added by Bianco Veigel over 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Category:
Interfaces
Target version:
Start date:
04/01/2015
Due date:
% Done:

100%

Estimated time:
Affected Version:
2.2.x
Affected Architecture:

Description

Everytime I click save on the interface page, the second Gateway-Address for the corresponding l2tp interface are lost. So after saving the interface page I have to go to Interfaces->(assign)->PPPs and add the Gateway Address for the second link interface.

l2tp-bug.png (22.4 KB) l2tp-bug.png Bianco Veigel, 04/01/2015 08:33 AM

Associated revisions

Revision a344af88 (diff)
Added by Phillip Davis about 4 years ago

pptp_local and pptp_remote are arrays

I believe these should reference the [0] array element of pptp_local and pptp_remote. The code in interfaces.php only presents the [0] (first) array element to the user for editing.
The current code also does not preserve any additional pptp_local, pptp_subnet or pptp_remote entries in the coma-separated list in the config. That is redmine bug #4568.
I am currently integrating the fix for Redmine #4568 Preserve MLPPP settings when saving interface settings. The fix for that bug will add more code to preserve additional pptp_local, pptp_subnet or pptp_remote entries.
For now it would be good to have this little fix to the existing code - then the fix for redmine bug #4568 will sit nicely on top of it.

History

#1 Updated by Bianco Veigel over 4 years ago

May be the same as #4378

#2 Updated by Phillip Davis over 4 years ago

Note: both Local IP, subnet and Gateway fields are lost for all but the first of multiple interfaces selected for MLPPP.
That data is stored in comma-separated lists 'localip' 'subnet' and 'gateway'. interfaces_ppps_edit.php correctly handles unpacking and packing the lists to/from the fields in the UI.
interfaces.php is too stupid. It only displays the first in the list, even having code that explicitly picks out the first (0) element of the arrays:
htmlspecialchars($pconfig['pptp_local'][0])
$pconfig['pptp_subnet'][0]
htmlspecialchars($pconfig['pptp_remote'][0])

And then that is saved back without putting back any of the other entries that might have been there.

The Local IP, Subnet and Gateway details already managed by interfaces_ppps_edit.

What to do in interfaces.php?
a) Remove the fields from interfaces.php and just make sure to preserve the existing data without ever showing it to the user, or;
b) Display all the data from the 3 fields as text information to the user and point them to where it can be edited in interfaces_ppps_edit.php. Make sure to preserve it correctly on save, or;
c) Enhance interfaces.php to display multiple sets of the fields, accept and validate any changes and correctly build the comma-separated lists again, or;
d) Just display the first set of fields as they are now, allowing the user to edit that as they wish. If there are multiple MLPPP defined underneath then indicate that somewhere (so the user can realise that), point them to where the proper editing of that is done, and underneath in the code make sure to preserve that data on save, or;
e) Something else.

What is the preferred option?

#3 Updated by Phillip Davis over 4 years ago

Actually I looked at the code more and option (d) was easy to do and seemed reasonable. Pull request https://github.com/pfsense/pfsense/pull/1780 should fix this bug.

#4 Updated by Chris Buechler over 4 years ago

  • Target version set to 2.2.5

thought I'd submitted this yesterday but was still sitting here.
Thanks Phil. Assuming testing checks out fine, we're at 2.2.4-REL now and that's more of a change than I want to risk at the moment for an atypical edge case that's been broken for a long time (and maybe always), but we'll get that merged post-2.2.4.

#5 Updated by Phillip Davis over 4 years ago

Yes, you are right. There is a bit of duck-and-weave in the changes there to save the previous strings, get the $POST values and merge them back in at the start of the previous data... And it really needs testing that there are no unforeseen side-effects when other PP* variants are selected for an interface. Not good to just dump in as the last commit for a formal release.

I see you have admitted to the idea that 2.2.5 might happen - good. We can keep fixing the steady flow of things that seem to pop up. I keep thinking I have got near the end of things I care about, then I press a button somewhere and think "Oh, that should validate better or..."
For things that change/fix/enhance the GUI modules (/usr/local/www) it will be a challenge to make sure that everything that is done is also migrated into effectively the same change/fix/enhancement in the bootstrap code.

#6 Updated by David Burgess over 4 years ago

Is there a workaround for this? I just upgraded from 2.1.3 > 2.2.4 and now have use of only one of my MLPPP members.

#7 Updated by Phillip Davis over 4 years ago

Apply the patch to usr/local/www/interfaces.php from https://github.com/pfsense/pfsense/pull/1780
My commit https://github.com/phil-davis/pfsense/commit/da71a2c06e9b955cf482ebfa8081616dc94a7efe
Note: After the pull request is merged, that temporary branch and patch will be deleted. But at that time you will be able to get the change from the pfSense RELENG_2_2 branch anyway.

#8 Updated by Phillip Davis over 4 years ago

I have never been able to edit my posts in Redmine. Sorry - that post above has the pull request and commit for master.
The ones for RELENG_2_2 to go on top of 2.2.4 are:
https://github.com/pfsense/pfsense/pull/1781
https://github.com/phil-davis/pfsense/commit/6db49ea6cb14ac80adb9f1b800780117fa3dd78a

#9 Updated by Renato Botelho about 4 years ago

  • Assignee set to Renato Botelho

#10 Updated by Phillip Davis about 4 years ago

New pull request for RELENG_2_2 that should be bug free :) https://github.com/pfsense/pfsense/pull/1928

The master version was committed as https://github.com/pfsense/pfsense/commit/66fd7b47679187ddcfaf5852a55b2af15394f341
and there is a followup dumb bug fix for that https://github.com/pfsense/pfsense/pull/1929

Sorry for all the noise. My brain is going to mush with recent merging and manual integration.

#11 Updated by Renato Botelho about 4 years ago

  • Status changed from New to Feedback
  • % Done changed from 0 to 100

Pull requests have been merged. Thank you Phil!

#12 Updated by Chris Buechler about 4 years ago

  • Status changed from Feedback to Resolved
  • Affected Version changed from 2.2 to 2.2.x

fixed

Also available in: Atom PDF