Project

General

Profile

Bug #4655

IPsec: Enable bypass for LAN interface IP behaviour is reversed

Added by Kill Bill over 4 years ago. Updated about 4 years ago.

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

0%

Estimated time:
Affected Version:
2.2.3
Affected Architecture:
All

Description

Before this gets lost again: After the commits related to Bug #4640, the checkbox does the exact opposite of what'd described in the GUI. To enable bypass for LAN interface IP, you need to uncheck the checkbox. Can be trivially verified by reading the resulting /var/etc/ipsec/ipsec.conf. When the checkbox is checked, the conn bypasslan is missing.

Associated revisions

Revision ee72e2ca (diff)
Added by Chris Buechler over 4 years ago

Show correct selection for noshuntlan option. Ticket #4655

Revision 43f83ab4 (diff)
Added by Chris Buechler over 4 years ago

Show correct selection for noshuntlan option. Ticket #4655

Revision 1c7fd09e (diff)
Added by Ermal Luçi about 4 years ago

Ticket #4655 Do not behave against the logic of checkbox and description.

Revision 71f29f44 (diff)
Added by Ermal Luçi about 4 years ago

Ticket #4655 Do not behave against the logic of checkbox and description.

Revision da6faa2b (diff)
Added by Ermal Luçi about 4 years ago

Ticket #4655 well manually merge pull/1715. Thanks: Phil

Revision d26955ff (diff)
Added by Ermal Luçi about 4 years ago

Ticket #4655 well manually merge pull/1715. Thanks: Phil

History

#1 Updated by Chris Buechler over 4 years ago

  • Status changed from New to Resolved
  • Affected Version changed from 2.2.2 to 2.2.3

fixed, thanks

#2 Updated by Kill Bill over 4 years ago

No, sorry, sir. We are back to the flip every save. This no logic MUST go away.

#3 Updated by Phillip Davis over 4 years ago

Yes, Chris seems to have reversed the fix I made for the "flip every save" problem. The whole thing needs some re-engineering.

#4 Updated by Kill Bill over 4 years ago

Yes. The only way to fix this without reversing the evil logic would be reverting this commit and changing the description to the equally evil "Check this checkbox to disable this" in the GUI.

#5 Updated by Chris Buechler over 4 years ago

  • Status changed from Resolved to New
  • Assignee set to Chris Buechler

I shouldn't test at 2 AM apparently.

#6 Updated by Kill Bill about 4 years ago

Can we please revert the broken commit and fix the description until this is recoded properly?

#7 Updated by Ermal Luçi about 4 years ago

  • Status changed from New to Feedback

Fixed to be natural to the checkbox and comment.

#8 Updated by Kill Bill about 4 years ago

This does NOT work. Keep clicking Save and watch the checkbox and ipsec.conf flip. Not really sure what to say here. Fix the description if unable to recode the double negative bullshit. Sorry but this is getting absurd.

#9 Updated by Kill Bill about 4 years ago

And let me say, it was just fine until this evil commit that twisted the logic into this stupidity.

https://github.com/pfsense/pfsense/commit/0a9e6c85f05b1027156618a9ccf1e1b12f31683e

#10 Updated by Phillip Davis about 4 years ago

https://github.com/pfsense/pfsense/pull/1715
Ermal's change/fix seems good (although the whole thing screws with people's brains, but that is another issue). The "aim" is that the user interface checkbox displays the boolean value the opposite way around to what is actually in the $config. That means:
1) Read config into $pconfig array
2) If $pconfig value is false then set the checkbox, otherwise clear it (code by cmb)
3) When processing $POST value on save, if $POST value is "yes" then unset in config, otherwise set in config (Ermal's fix a few hours back)
4) Set the $pconfig value to be the opposite of what came in $POST - otherwise the immediately redisplayed checkbox value is actually the opposite of the way it should be - my pull request now.

This change to address point (4) finishes making the UI processing consistently display the opposite truth value to what is stored in the config. At least now the front end works consistently (even if backwards and you don't like the name 'noshuntlaninterfaces' :)

Now it works consistently, someone can check the back-end implementation and if that does the wrong/opposite thing then that can be fixed in the back-end without needing to touch the front-end UI logic.

#11 Updated by Kill Bill about 4 years ago

Phillip Davis wrote:

https://github.com/pfsense/pfsense/pull/1715

Applied your pull request on top of the whole mess without reverting anything.

- The GUI now really shows what's configured in ipsec.conf, not the opposite.
- Does not flip on save.
- Ticking/unticking the checkbox does enable/disable conn bypasslan as expected.

Thanks, Phil!!!

#12 Updated by Ermal Luçi about 4 years ago

Kill Bill wrote:

And let me say, it was just fine until this evil commit that twisted the logic into this stupidity.

https://github.com/pfsense/pfsense/commit/0a9e6c85f05b1027156618a9ccf1e1b12f31683e

It was a game for you to test you communication skills.
I thought you have to taste what happens on this side :)

Anyway its fixed now let me know if its still broken.

#13 Updated by Kill Bill about 4 years ago

It works now... (The time wasted here would be enough of a hint to not ever do things like this again. There's a forum thread dedicated to this coding style, so leaving it at that... :-P)

Thanks.

#14 Updated by Chris Buechler about 4 years ago

  • Status changed from Feedback to Resolved

Thanks Phil!

We were heading out to dinner shortly after Ermal's commit yesterday, and came up with the idea on the way we'd make a couple more broken commits here since it makes doktornotor so happy. Commit log "Maybe this works? Ticket #4655. special-credit-to: doktornotor"

but actually fixing it is better. :)

#16 Updated by Phillip Davis about 4 years ago

The fish-slapping drama continues here https://github.com/pfsense/pfsense/pull/1715
That pull request is closed, but so far the changes Ermal has made fix parts of the remaining issue or introduce other side-effects. Let's see how long we can keep this one going - it helps distract all of us from attending to other more worthy bugs :)

#17 Updated by Kill Bill about 4 years ago

I still would love to hear why exactly do we desperately need to spare one line in default config.xml and why the setting needs to be like <noshuntlaninterfaces /> instead of <shuntlaninterfaces>no<shuntlaninterfaces/>

#18 Updated by Chris Buechler about 4 years ago

Top secret reasons, sorry. The NSA won't let us tell.

I'm kidding...point being, why would you want additional lines in the default config that are unnecessary? Sure, a single line is no big deal, but it's a matter of principle. If every single line were no big deal, we'd have hundreds of lines for the same reason. Every default config option shouldn't require a config.xml parameter to be set to use the default. The absence of a config.xml setting means it's at its default, not the presence of it.

#19 Updated by Chris Buechler about 4 years ago

  • Status changed from Resolved to Feedback

Phil, thanks for the additional pull request. Putting this back to Feedback to review later. It's BSDCan and 2 AM which means I've had too many beers to merge or review pull requests at this point.

#20 Updated by Kill Bill about 4 years ago

Chris Buechler wrote:

I'm kidding...point being, why would you want additional lines in the default config that are unnecessary? Sure, a single line is no big deal, but it's a matter of principle. If every single line were no big deal, we'd have hundreds of lines for the same reason. Every default config option shouldn't require a config.xml parameter to be set to use the default. The absence of a config.xml setting means it's at its default, not the presence of it.

Chris, when you no longer have defaults in the config, you don't know what the default is (without digging into the code). You don't even know that such configuration option exists. Having even the defaults in the config.xml makes it self-documenting. You can grep the code and see what that config option does, if non-obvious. You also can change those easily when manually imporing the config.xml on the same/different box or whatever. I cannot see how hundreds of lines do any harm in the config.xml. (The empty tags style does not do much good here either.)

This results in extremely messy code that "screws with people's brains", as Phil noted.

There's a forum thread for this, better move the debate there though. https://forum.pfsense.org/index.php?topic=92631.0

#21 Updated by Phillip Davis about 4 years ago

Ermal merge pull 1715, and as far as I can see that resolves the remaining issue.

#22 Updated by Chris Buechler about 4 years ago

  • Status changed from Feedback to Resolved

fixed

Also available in: Atom PDF