Skip to content

fix(firewall): preserve whitelist priority and rule ordering #13845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ssongliu merged 1 commit into from
Sep 17, 2026
Merged

fix(firewall): preserve whitelist priority and rule ordering #13845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 55 additions & 60 deletions agent/app/service/firewall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,7 +1015,9 @@ func (s *FirewallService) Create(
return dto.FirewallRuleCreateResponse{}, err
}
taskItem.AddSubTaskWithOps(i18n.GetMsgByKey("FirewallCreateRulesStep"), func(t *task.Task) error {
_, err := s.runCreateTask(t, request)
firewallRuleMutationMu.Lock()
defer firewallRuleMutationMu.Unlock()
_, err := s.createRules(t.TaskCtx, request, t)
return err
}, nil, 0, 0)
if err := repo.NewITaskRepo().Save(context.Background(), taskItem.Task); err != nil {
Expand All @@ -1031,12 +1033,6 @@ func (s *FirewallService) Create(
return dto.FirewallRuleCreateResponse{TaskID: taskItem.TaskID, Queued: true}, nil
}

func (s *FirewallService) runCreateTask(t *task.Task, request dto.FirewallRuleCreate) (dto.FirewallRuleCreateResponse, error) {
firewallRuleMutationMu.Lock()
defer firewallRuleMutationMu.Unlock()
return s.createRules(t.TaskCtx, request, t)
}

func (s *FirewallService) createRules(ctx context.Context, request dto.FirewallRuleCreate, t *task.Task) (result dto.FirewallRuleCreateResponse, taskErr error) {
var firstFailure error
defer func() {
Expand Down Expand Up @@ -1412,7 +1408,13 @@ func (s *FirewallService) Delete(ctx context.Context, request dto.FirewallRuleDe
return dto.FirewallRuleDeleteResponse{}, err
}
taskItem.AddSubTaskWithOps(taskItem.Name, func(t *task.Task) error {
_, err := s.runDeleteTask(t, request)
t.Logf("rules=%d", len(request.UUIDs)+len(request.BeforeRules))
firewallRuleMutationMu.Lock()
defer firewallRuleMutationMu.Unlock()
if err := t.TaskCtx.Err(); err != nil {
return err
}
_, err := s.deleteRules(t.TaskCtx, request, t)
return err
}, nil, 0, 0)
if err := repo.NewITaskRepo().Save(context.Background(), taskItem.Task); err != nil {
Expand All @@ -1428,16 +1430,6 @@ func (s *FirewallService) Delete(ctx context.Context, request dto.FirewallRuleDe
return dto.FirewallRuleDeleteResponse{TaskID: taskItem.TaskID, Queued: true}, nil
}

func (s *FirewallService) runDeleteTask(t *task.Task, request dto.FirewallRuleDelete) (dto.FirewallRuleDeleteResponse, error) {
t.Logf("rules=%d", len(request.UUIDs)+len(request.BeforeRules))
firewallRuleMutationMu.Lock()
defer firewallRuleMutationMu.Unlock()
if err := t.TaskCtx.Err(); err != nil {
return dto.FirewallRuleDeleteResponse{}, err
}
return s.deleteRules(t.TaskCtx, request, t)
}

func (s *FirewallService) deleteRules(ctx context.Context, request dto.FirewallRuleDelete, t *task.Task) (result dto.FirewallRuleDeleteResponse, taskErr error) {
var firstFailure error
defer func() {
Expand Down Expand Up @@ -1619,7 +1611,7 @@ func (s *FirewallService) prepareDelete(
}
return preparedFirewallRuleDelete{}, err
}
if err := checkFirewallRuleWhitelistProtection(ctx, stored); err != nil {
if err := checkFirewallRuleWhitelistProtection(selectedProvider, stored); err != nil {
return preparedFirewallRuleDelete{}, err
}
if stored.Origin != constant.FirewallRuleOriginCreated && stored.Origin != constant.FirewallRuleOriginAdopted {
Expand Down Expand Up @@ -1754,7 +1746,11 @@ func (s *FirewallService) updateRuleDescription(ctx context.Context, ruleUUID, d
if err != nil {
return err
}
if err := checkFirewallRuleWhitelistProtection(ctx, stored); err != nil {
selected, err := s.selectedProviderForStoredRule(ctx, stored)
if err != nil {
return err
}
if err := checkFirewallRuleWhitelistProtection(selected, stored); err != nil {
return err
}
if stored.Origin != constant.FirewallRuleOriginCreated && stored.Origin != constant.FirewallRuleOriginAdopted {
Expand Down Expand Up @@ -1900,7 +1896,7 @@ func (s *FirewallService) deleteRule(ctx context.Context, ruleUUID string) error
if err != nil {
return err
}
if err := checkFirewallRuleWhitelistProtection(ctx, stored); err != nil {
if err := checkFirewallRuleWhitelistProtection(selected, stored); err != nil {
return err
}
type appliedDelete struct {
Expand Down Expand Up @@ -2212,9 +2208,6 @@ func (s *FirewallService) loadManagedMutation(
if err != nil {
return model.FirewallRule{}, filter.DesiredRule{}, filter.Snapshot{}, filter.ObservedRule{}, nil, err
}
if err := checkFirewallRuleWhitelistProtection(ctx, stored); err != nil {
return model.FirewallRule{}, filter.DesiredRule{}, filter.Snapshot{}, filter.ObservedRule{}, nil, err
}
if stored.Origin != constant.FirewallRuleOriginCreated && stored.Origin != constant.FirewallRuleOriginAdopted {
return model.FirewallRule{}, filter.DesiredRule{}, filter.Snapshot{}, filter.ObservedRule{}, nil,
fmt.Errorf("%w: only created or adopted rules can be changed", filter.ErrInvalidRule)
Expand All @@ -2223,6 +2216,9 @@ func (s *FirewallService) loadManagedMutation(
if err != nil {
return model.FirewallRule{}, filter.DesiredRule{}, filter.Snapshot{}, filter.ObservedRule{}, nil, err
}
if err := checkFirewallRuleWhitelistProtection(selected, stored); err != nil {
return model.FirewallRule{}, filter.DesiredRule{}, filter.Snapshot{}, filter.ObservedRule{}, nil, err
}
desiredRules, err := s.compileStoredFirewallRules(ctx, stored, selected)
if err != nil {
return model.FirewallRule{}, filter.DesiredRule{}, filter.Snapshot{}, filter.ObservedRule{}, nil, err
Expand Down Expand Up @@ -2617,16 +2613,27 @@ func (s *FirewallService) sequenceForFirewallRulePosition(
byUUID := make(map[string]model.FirewallRule, len(stored))
for _, record := range stored {
byUUID[record.UUID] = record
compiled, err := s.compileStoredFirewallRules(ctx, record, snapshot.Scope.Provider)
if err != nil {
if isFirewallPolicyIncompatible(err) {
continue
}
return 0, err
}
for _, desired := range compiled {
if desired.Rule.Scope.Key() == snapshot.Scope.Key() {
byUUID[desired.Rule.UUID] = record
}
}
}
var previous, next *model.FirewallRule
needsRebalance := false
for _, observed := range snapshot.Rules {
uuid := strings.TrimPrefix(observed.Marker, "1panel-rule:")
if observed.Marker == uuid || uuid == excludedUUID || observed.Locator.Position == nil {
continue
}
record, exists := byUUID[uuid]
if !exists {
if !exists || record.UUID == excludedUUID {
continue
}
position := *observed.Locator.Position
Expand All @@ -2640,30 +2647,24 @@ func (s *FirewallService) sequenceForFirewallRulePosition(
}
}
if previous != nil && previous.Sequence == nil || next != nil && next.Sequence == nil {
needsRebalance = true
return s.rebalanceFirewallRuleSequences(ctx, snapshot, targetPosition, excludedUUID, byUUID)
}
if !needsRebalance && current != nil &&
if current != nil &&
(previous == nil || *previous.Sequence < *current) && (next == nil || *current < *next.Sequence) {
return *current, nil
}
if !needsRebalance {
switch {
case previous == nil && next == nil:
return model.FirewallRuleSequenceStep, nil
case previous == nil:
return *next.Sequence - model.FirewallRuleSequenceStep, nil
case next == nil:
return *previous.Sequence + model.FirewallRuleSequenceStep, nil
case *next.Sequence-*previous.Sequence > 1:
return *previous.Sequence + (*next.Sequence-*previous.Sequence)/2, nil
default:
needsRebalance = true
}
}
if needsRebalance {
switch {
case previous == nil && next == nil:
return model.FirewallRuleSequenceStep, nil
case previous == nil:
return *next.Sequence - model.FirewallRuleSequenceStep, nil
case next == nil:
return *previous.Sequence + model.FirewallRuleSequenceStep, nil
case *next.Sequence-*previous.Sequence > 1:
return *previous.Sequence + (*next.Sequence-*previous.Sequence)/2, nil
default:
return s.rebalanceFirewallRuleSequences(ctx, snapshot, targetPosition, excludedUUID, byUUID)
}
return 0, fmt.Errorf("%w: cannot allocate firewall rule sequence", filter.ErrRuleOperation)
}

func (s *FirewallService) rebalanceFirewallRuleSequences(
Expand All @@ -2674,6 +2675,7 @@ func (s *FirewallService) rebalanceFirewallRuleSequences(
byUUID map[string]model.FirewallRule,
) (int64, error) {
targetSequence := int64(targetPosition) * model.FirewallRuleSequenceStep
updated := make(map[string]bool)
for _, observed := range snapshot.Rules {
if observed.Locator.Position == nil {
continue
Expand All @@ -2683,9 +2685,10 @@ func (s *FirewallService) rebalanceFirewallRuleSequences(
continue
}
record, exists := byUUID[uuid]
if !exists {
if !exists || record.UUID == excludedUUID || updated[record.UUID] {
continue
}
updated[record.UUID] = true
position := *observed.Locator.Position
if excludedUUID == "" && position >= targetPosition {
position++
Expand Down Expand Up @@ -2825,7 +2828,11 @@ func firewallRuleSnapshotPolicy(ctx context.Context, snapshot filter.Snapshot) (
}

func firewallRuleSelectedProvider(context.Context) (filter.Provider, error) {
return selectedRuleProvider()
provider, err := selectedSystemFirewallProvider()
if err != nil {
return "", fmt.Errorf("%w: %v", filter.ErrProviderUnavailable, err)
}
return filter.Provider(provider), nil
}

func rollbackFirewallPlan(ctx context.Context, runtime *filterruntime.Engine, plan filter.BackendPlan, cause error) error {
Expand All @@ -2838,14 +2845,6 @@ func rollbackFirewallPlan(ctx context.Context, runtime *filterruntime.Engine, pl
return cause
}

func selectedRuleProvider() (filter.Provider, error) {
provider, err := selectedSystemFirewallProvider()
if err != nil {
return "", fmt.Errorf("%w: %v", filter.ErrProviderUnavailable, err)
}
return filter.Provider(provider), nil
}

func ensureFirewallPorts(ports []int) error {
filterruntime.InvalidateInventory()
defer filterruntime.InvalidateInventory()
Expand All @@ -2858,7 +2857,7 @@ func ensureFirewallPorts(ports []int) error {
return err
}
if state.Name == constant.FirewallProviderIptables || state.Name == constant.FirewallProviderNftables {
isInit, _, err := loadDirectFirewallInitStatus(state.Name)
isInit, _, err := loadFirewallInitStatus(state.Name, "base")
if err != nil {
return err
}
Expand Down Expand Up @@ -2934,10 +2933,6 @@ func newNftablesHelperManager() *nftables_helper.Manager {
}
}

func loadDirectFirewallInitStatus(provider string) (bool, bool, error) {
return loadFirewallInitStatus(provider, "base")
}

func loadFirewallInitStatus(provider, tab string) (bool, bool, error) {
switch provider {
case constant.FirewallProviderNftables:
Expand Down Expand Up @@ -2968,7 +2963,7 @@ func (s *FirewallService) restoreFirewallAfterStart(client lifecycle.Client) err
}
}
if provider == filter.ProviderIptables || provider == filter.ProviderNftables {
isInit, _, err := loadDirectFirewallInitStatus(string(provider))
isInit, _, err := loadFirewallInitStatus(string(provider), "base")
if err != nil {
recordFailure("load managed chain status", err)
return errors.Join(recoveryErrors...)
Expand Down
4 changes: 2 additions & 2 deletions agent/app/service/firewall_setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,12 @@ func savePortWhitelist(ctx context.Context, change func([]firewall.PortWhitelist
})
}

func checkFirewallRuleWhitelistProtection(ctx context.Context, record model.FirewallRule) error {
func checkFirewallRuleWhitelistProtection(provider filter.Provider, record model.FirewallRule) error {
ports, err := loadFirewallPortWhiteList()
if err != nil {
return err
}
rules, err := record.RulesForProvider(filter.ProviderIptables)
rules, err := record.RulesForProvider(provider)
if err != nil {
return err
}
Expand Down
Loading
Loading