Project

General

Profile

Actions
Copy link

Bug #9887

closed

Rule separator positions change when deleting multiple rules

Added by Jim Pingle about 5 years ago. Updated about 2 years ago.

Status:
Resolved
Priority:
Low
Assignee:
Christopher Cope
Category:
Rules / NAT
Target version:
2.7.0
Start date:
11/07/2019
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
23.01
Release Notes:
Default
Affected Version:
All
Affected Architecture:
All

Description

When deleting rules around a separator at the end of the ruleset, separator positions can change unintentionally. Similar to #6801 but doesn't appear to be identical.

I was only able to reproduce it when removing the last few rules of an interface ruleset. If I deleted single rules or multiples around/between higher separator bars, there was no issue and things stayed in the expected places.

See attached before/after photos to see how to reproduce it.

Files

Download all files
sep-test2-after.jpg (123 KB) sep-test2-after.jpg Test #2 - After Delete Jim Pingle, 11/07/2019 09:46 AM
sep-test2-before.jpg (157 KB) sep-test2-before.jpg Test #2 - Before Delete Jim Pingle, 11/07/2019 09:46 AM
sep-test1-before.jpg (159 KB) sep-test1-before.jpg Test #1 - Before Delete Jim Pingle, 11/07/2019 09:46 AM
sep-test1-after.jpg (137 KB) sep-test1-after.jpg Test #1 - After Delete Jim Pingle, 11/07/2019 09:46 AM
sep-test2-after.JPG (28.8 KB) sep-test2-after.JPG Dan Mackie, 12/17/2020 02:19 AM
sep-test2-before.jpg (37.9 KB) sep-test2-before.jpg Dan Mackie, 12/17/2020 02:19 AM
sep-test1-after.JPG (31.2 KB) sep-test1-after.JPG Dan Mackie, 12/17/2020 02:19 AM
sep-test1-before.jpg (38.1 KB) sep-test1-before.jpg Dan Mackie, 12/17/2020 02:19 AM
sep-test4-after.JPG (47.1 KB) sep-test4-after.JPG Dan Mackie, 12/17/2020 04:26 AM
sep-test4-before.JPG (57.5 KB) sep-test4-before.JPG Dan Mackie, 12/17/2020 04:26 AM
sep-test3-after.JPG (72.5 KB) sep-test3-after.JPG Test for #6801 - After Dan Mackie, 12/17/2020 04:26 AM
sep-test3-before.JPG (91.1 KB) sep-test3-before.JPG Test for #6801 - Before Dan Mackie, 12/17/2020 04:26 AM
sep-test2-after-830-fail.png (43.3 KB) sep-test2-after-830-fail.png Jim Pingle, 07/05/2022 09:41 AM
9887.patch (3 KB) 9887.patch Christopher Cope, 07/07/2022 12:18 PM
before.png (32.4 KB) before.png Lev Prokofev, 07/09/2022 01:23 AM
after.png (26.7 KB) after.png Lev Prokofev, 07/09/2022 01:23 AM
Actions
Copy link
 #1

Updated by Anonymous about 4 years ago

  • Assignee set to Anonymous
Actions
Copy link
 #2

Updated by Anonymous about 4 years ago

  • Priority changed from Normal to Low
Actions
Copy link
 #3

Updated by Anonymous about 4 years ago

  • Target version changed from 2.5.0 to CE-Next
Actions
Copy link
 #5

Updated by Dan Mackie almost 4 years ago

PR: https://github.com/pfsense/pfsense/pull/4491

Fix allows both Jim Pingle's screenshots from this issue and those in #6801 to work as expected.

Note - If possible, delete the images from comment No. 4 above as they are before I realised I had not actually fixed the issue correctly.

Actions
Copy link
 #6

Updated by Renato Botelho over 3 years ago

  • Status changed from New to Feedback

PR has been merged. Thanks!

Actions
Copy link
 #7

Updated by Dan Mackie over 3 years ago

  • % Done changed from 0 to 100
Actions
Copy link
 #8

Updated by Jim Pingle over 3 years ago

  • Target version changed from CE-Next to 2.6.0
Actions
Copy link
 #9

Updated by Jim Pingle over 3 years ago

  • Plus Target Version set to 21.05
Actions
Copy link
 #10

Updated by Jim Pingle over 3 years ago

Already in 21.05 branch.

Actions
Copy link
 #11

Updated by Jim Pingle over 3 years ago

  • Status changed from Feedback to New
  • % Done changed from 100 to 50
  • Plus Target Version changed from 21.05 to 21.09

Still broken but not a blocker so moving forward. The scenario in my first test "sep-test1" is OK. The second scenario "sep-test2" still breaks. The behavior is different, however, as now the second separator "Sep 2" has moved to the top of the interface rules instead of staying at the bottom.

Actions
Copy link
 #12

Updated by Jim Pingle about 3 years ago

  • Plus Target Version changed from 21.09 to 22.01

Moving ahead

Actions
Copy link
 #13

Updated by Jim Pingle about 3 years ago

  • Target version changed from 2.6.0 to CE-Next
  • Plus Target Version changed from 22.01 to 22.05
Actions
Copy link
 #14

Updated by Jim Pingle over 2 years ago

  • Plus Target Version changed from 22.05 to 22.09
Actions
Copy link
 #15

Updated by Jim Pingle over 2 years ago

  • Plus Target Version changed from 22.09 to 22.11
Actions
Copy link
 #16

Updated by Christopher Cope over 2 years ago

  • File 0001-Separators-position-fix.-Issue-9887.patch added
  • Status changed from New to Pull Request Review

https://gitlab.netgate.com/pfSense/pfSense/-/merge_requests/830

All tests in the original ticket worked as expected with these changes. I'd appreciate an extra pair of eyes, though.

Actions
Copy link
 #17

Updated by Jim Pingle over 2 years ago

This fails in a new/different way when applied. When attempting "test 2" from my original attachments, it puts the separators into an even different (and wrong) layout.

Looking at my original attachments, "sep-test1" worked even before this patch now, but "sep-test2" failed in the way shown in the new image attached.

Actions
Copy link
 #18

Updated by Christopher Cope over 2 years ago

Here's a new patch with missing fixes. Seems to pass all tests this time.

Actions
Copy link
 #19

Updated by Christopher Cope over 2 years ago

  • File deleted (0001-Separators-position-fix.-Issue-9887.patch)
Actions
Copy link
 #20

Updated by Jim Pingle over 2 years ago

  • Assignee set to Christopher Cope
  • Target version changed from CE-Next to 2.7.0
  • % Done changed from 50 to 100

Latest patch tests OK for me.

Actions
Copy link
 #21

Updated by Christopher Cope over 2 years ago

  • Status changed from Pull Request Review to Feedback

Fix merged

Actions
Copy link
 #22

Updated by Lev Prokofev over 2 years ago

Tested, and it works for me.

Actions
Copy link
 #23

Updated by Jim Pingle over 2 years ago

  • Status changed from Feedback to Resolved

Looks good on the latest snapshot.

Actions
Copy link
 #24

Updated by Jim Pingle about 2 years ago

  • Plus Target Version changed from 22.11 to 23.01
Actions
Copy link

Also available in: Atom PDF