Project

General

Profile

Actions
Copy link

Bug #9887

closed
JP CC

Rule separator positions change when deleting multiple rules

Bug #9887: Rule separator positions change when deleting multiple rules

Added by Jim Pingle almost 7 years ago. Updated almost 4 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

Updated by Anonymous almost 6 years ago Actions
Copy link
 #1

  • Assignee set to Anonymous

Updated by Anonymous almost 6 years ago Actions
Copy link
 #2

  • Priority changed from Normal to Low

Updated by Anonymous almost 6 years ago Actions
Copy link
 #3

  • Target version changed from 2.5.0 to CE-Next

DM Updated by Dan Mackie over 5 years ago Actions
Copy link Download all files
 #5

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.

RB Updated by Renato Botelho over 5 years ago Actions
Copy link
 #6

  • Status changed from New to Feedback

PR has been merged. Thanks!

DM Updated by Dan Mackie over 5 years ago Actions
Copy link
 #7

  • % Done changed from 0 to 100

Applied in changeset commit:3e7a04be6ce4530bbb37b3c312fd2239a61967db.

JP Updated by Jim Pingle over 5 years ago Actions
Copy link
 #8

  • Target version changed from CE-Next to 2.6.0

JP Updated by Jim Pingle over 5 years ago Actions
Copy link
 #9

  • Plus Target Version set to 21.05

JP Updated by Jim Pingle over 5 years ago Actions
Copy link
 #10

Already in 21.05 branch.

JP Updated by Jim Pingle over 5 years ago Actions
Copy link
 #11

  • 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.

JP Updated by Jim Pingle about 5 years ago Actions
Copy link
 #12

  • Plus Target Version changed from 21.09 to 22.01

Moving ahead

JP Updated by Jim Pingle almost 5 years ago Actions
Copy link
 #13

  • Target version changed from 2.6.0 to CE-Next
  • Plus Target Version changed from 22.01 to 22.05

JP Updated by Jim Pingle over 4 years ago Actions
Copy link
 #14

  • Plus Target Version changed from 22.05 to 22.09

JP Updated by Jim Pingle about 4 years ago Actions
Copy link
 #15

  • Plus Target Version changed from 22.09 to 22.11

CC Updated by Christopher Cope about 4 years ago Actions
Copy link
 #16

  • 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.

JP Updated by Jim Pingle about 4 years ago Actions
Copy link
 #17

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.

CC Updated by Christopher Cope about 4 years ago Actions
Copy link
 #18

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

CC Updated by Christopher Cope about 4 years ago Actions
Copy link
 #19

  • File deleted (0001-Separators-position-fix.-Issue-9887.patch)

JP Updated by Jim Pingle about 4 years ago Actions
Copy link
 #20

  • 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.

CC Updated by Christopher Cope about 4 years ago Actions
Copy link
 #21

  • Status changed from Pull Request Review to Feedback

Fix merged

LP Updated by Lev Prokofev about 4 years ago Actions
Copy link Download all files
 #22

Tested, and it works for me.

JP Updated by Jim Pingle about 4 years ago Actions
Copy link
 #23

  • Status changed from Feedback to Resolved

Looks good on the latest snapshot.

JP Updated by Jim Pingle almost 4 years ago Actions
Copy link
 #24

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

Also available in: Atom