Skip to content

Commit 00132ed

Browse files
mgajdamgajda
committed
refactor: reimplement firewall rule enable/disable to move rules relative to catch-all
- delete command now supports filters to delete multiple rules - enable command moves rules before catch-all drop rule (activates them) - disable command moves rules after catch-all drop rule (deactivates them) - consistent filter flags across delete/enable/disable commands This addresses PR review feedback to make command semantics clearer: - create/delete for rule CRUD operations - enable/disable for rule activation by position relative to catch-all
1 parent 8480631 commit 00132ed

3 files changed

Lines changed: 355 additions & 25 deletions

File tree

‎internal/commands/server/firewall/delete.go‎

Lines changed: 99 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,21 @@ package serverfirewall
22

33
import (
44
"fmt"
5+
"sort"
56

67
"github.com/UpCloudLtd/upcloud-cli/v3/internal/commands"
78
"github.com/UpCloudLtd/upcloud-cli/v3/internal/completion"
89
"github.com/UpCloudLtd/upcloud-cli/v3/internal/output"
910
"github.com/UpCloudLtd/upcloud-cli/v3/internal/resolver"
11+
"github.com/UpCloudLtd/upcloud-cli/v3/internal/ui"
12+
"github.com/UpCloudLtd/upcloud-go-api/v8/upcloud"
1013
"github.com/UpCloudLtd/upcloud-go-api/v8/upcloud/request"
11-
"github.com/spf13/cobra"
1214
"github.com/spf13/pflag"
1315
)
1416

1517
type deleteCommand struct {
1618
*commands.BaseCommand
17-
rulePosition int
19+
params ruleModifyParams
1820
completion.Server
1921
resolver.CachingServer
2022
}
@@ -24,39 +26,118 @@ func DeleteCommand() commands.Command {
2426
return &deleteCommand{
2527
BaseCommand: commands.New(
2628
"delete",
27-
"Removes a firewall rule from a server. Firewall rules must be removed individually. The positions of remaining firewall rules will be adjusted after a rule is removed.",
29+
"Delete firewall rules from a server. Rules can be deleted by position or by using filters.",
2830
"upctl server firewall delete 00038afc-d526-4148-af0e-d2f1eeaded9b --position 1",
31+
"upctl server firewall delete myserver --comment \"temporary rule\"",
32+
"upctl server firewall delete myserver --direction in --protocol tcp --dest-port 8080",
2933
),
34+
params: ruleModifyParams{
35+
skipConfirmation: 1,
36+
},
3037
}
3138
}
3239

3340
// InitCommand implements Command.InitCommand
3441
func (s *deleteCommand) InitCommand() {
3542
flagSet := &pflag.FlagSet{}
36-
flagSet.IntVar(&s.rulePosition, "position", 0, "Rule position. Available: 1-1000")
43+
addRuleFilterFlags(flagSet, &s.params, s.Cobra())
3744
s.AddFlags(flagSet)
38-
39-
commands.Must(s.Cobra().MarkFlagRequired("position"))
40-
commands.Must(s.Cobra().RegisterFlagCompletionFunc("position", cobra.NoFileCompletions))
45+
configureRuleFilterFlagsPostAdd(s.Cobra())
4146
}
4247

4348
// Execute implements commands.MultipleArgumentCommand
4449
func (s *deleteCommand) Execute(exec commands.Executor, arg string) (output.Output, error) {
45-
if s.rulePosition < 1 || s.rulePosition > 1000 {
46-
return nil, fmt.Errorf("invalid position (1-1000 allowed)")
47-
}
48-
msg := fmt.Sprintf("Deleting firewall rule %d from server %v", s.rulePosition, arg)
49-
exec.PushProgressStarted(msg)
50-
51-
err := exec.Firewall().DeleteFirewallRule(exec.Context(), &request.DeleteFirewallRuleRequest{
52-
ServerUUID: arg,
53-
Position: s.rulePosition,
50+
// Get current firewall rules
51+
server, err := exec.Server().GetServerDetails(exec.Context(), &request.GetServerDetailsRequest{
52+
UUID: arg,
5453
})
5554
if err != nil {
56-
return commands.HandleError(exec, msg, err)
55+
return nil, err
56+
}
57+
58+
// Find matching rules
59+
matchedIndices := findMatchingRules(server.FirewallRules, &s.params)
60+
61+
if len(matchedIndices) == 0 {
62+
return nil, fmt.Errorf("no firewall rules matched the specified filters")
5763
}
5864

59-
exec.PushProgressSuccess(msg)
65+
// Confirm if multiple rules or if confirmation required
66+
if len(matchedIndices) > s.params.skipConfirmation {
67+
exec.PushProgressUpdate(fmt.Sprintf("Found %d matching firewall rules:", len(matchedIndices)))
68+
for _, idx := range matchedIndices {
69+
rule := &server.FirewallRules[idx]
70+
exec.PushProgressUpdate(fmt.Sprintf(" Position %d: %s %s %s -> %s",
71+
rule.Position, rule.Direction, rule.Protocol,
72+
formatRuleAddress(rule, true), formatRuleAddress(rule, false)))
73+
}
74+
75+
if !ui.Confirm(fmt.Sprintf("Delete %d firewall rules?", len(matchedIndices))) {
76+
return output.None{}, nil
77+
}
78+
}
79+
80+
// Sort indices in descending order to delete from highest position first
81+
// This prevents position shifts affecting subsequent deletions
82+
sort.Sort(sort.Reverse(sort.IntSlice(matchedIndices)))
83+
84+
// Delete each matched rule
85+
deletedCount := 0
86+
for _, idx := range matchedIndices {
87+
rule := &server.FirewallRules[idx]
88+
msg := fmt.Sprintf("Deleting firewall rule at position %d", rule.Position)
89+
exec.PushProgressStarted(msg)
90+
91+
err := exec.Firewall().DeleteFirewallRule(exec.Context(), &request.DeleteFirewallRuleRequest{
92+
ServerUUID: arg,
93+
Position: rule.Position,
94+
})
95+
if err != nil {
96+
exec.PushProgressFailed(msg)
97+
if deletedCount > 0 {
98+
exec.PushProgressUpdate(fmt.Sprintf("Successfully deleted %d rules before error", deletedCount))
99+
}
100+
return nil, err
101+
}
102+
103+
exec.PushProgressSuccess(msg)
104+
deletedCount++
105+
106+
// Adjust positions of remaining rules in our local copy
107+
for i := range server.FirewallRules {
108+
if server.FirewallRules[i].Position > rule.Position {
109+
server.FirewallRules[i].Position--
110+
}
111+
}
112+
}
60113

61114
return output.None{}, nil
62115
}
116+
117+
func formatRuleAddress(rule *upcloud.FirewallRule, source bool) string {
118+
var addr, port string
119+
if source {
120+
addr = rule.SourceAddressStart
121+
if rule.SourceAddressEnd != "" && rule.SourceAddressEnd != rule.SourceAddressStart {
122+
addr = fmt.Sprintf("%s-%s", addr, rule.SourceAddressEnd)
123+
}
124+
port = rule.SourcePortStart
125+
if rule.SourcePortEnd != "" && rule.SourcePortEnd != rule.SourcePortStart {
126+
port = fmt.Sprintf("%s-%s", port, rule.SourcePortEnd)
127+
}
128+
} else {
129+
addr = rule.DestinationAddressStart
130+
if rule.DestinationAddressEnd != "" && rule.DestinationAddressEnd != rule.DestinationAddressStart {
131+
addr = fmt.Sprintf("%s-%s", addr, rule.DestinationAddressEnd)
132+
}
133+
port = rule.DestinationPortStart
134+
if rule.DestinationPortEnd != "" && rule.DestinationPortEnd != rule.DestinationPortStart {
135+
port = fmt.Sprintf("%s-%s", port, rule.DestinationPortEnd)
136+
}
137+
}
138+
139+
if port != "" {
140+
return fmt.Sprintf("%s:%s", addr, port)
141+
}
142+
return addr
143+
}

‎internal/commands/server/firewall/rule_disable.go‎

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
11
package serverfirewall
22

33
import (
4+
"fmt"
5+
"sort"
6+
47
"github.com/UpCloudLtd/upcloud-cli/v3/internal/commands"
58
"github.com/UpCloudLtd/upcloud-cli/v3/internal/completion"
69
"github.com/UpCloudLtd/upcloud-cli/v3/internal/output"
710
"github.com/UpCloudLtd/upcloud-cli/v3/internal/resolver"
11+
"github.com/UpCloudLtd/upcloud-cli/v3/internal/ui"
812
"github.com/UpCloudLtd/upcloud-go-api/v8/upcloud"
13+
"github.com/UpCloudLtd/upcloud-go-api/v8/upcloud/request"
914
"github.com/spf13/pflag"
1015
)
1116

@@ -21,10 +26,9 @@ func RuleDisableCommand() commands.Command {
2126
return &ruleDisableCommand{
2227
BaseCommand: commands.New(
2328
"disable",
24-
"Disable firewall rules by changing their action to drop",
29+
"Disable firewall rules by moving them after the catch-all drop rule",
2530
"upctl server firewall rule disable myserver --dest-port 80",
2631
"upctl server firewall rule disable myserver --comment \"Dev ports\"",
27-
"upctl server firewall rule disable myserver --direction out --protocol udp --dest-port 53",
2832
"upctl server firewall rule disable myserver --position 5",
2933
),
3034
params: ruleModifyParams{
@@ -43,5 +47,112 @@ func (s *ruleDisableCommand) InitCommand() {
4347

4448
// Execute implements commands.MultipleArgumentCommand
4549
func (s *ruleDisableCommand) Execute(exec commands.Executor, arg string) (output.Output, error) {
46-
return modifyFirewallRules(exec, arg, &s.params, upcloud.FirewallRuleActionDrop, "disable")
50+
// Get current firewall rules
51+
server, err := exec.Server().GetServerDetails(exec.Context(), &request.GetServerDetailsRequest{
52+
UUID: arg,
53+
})
54+
if err != nil {
55+
return nil, err
56+
}
57+
58+
// Find the catch-all drop rule position
59+
catchAllPosition := findCatchAllDropRule(server.FirewallRules)
60+
if catchAllPosition == 0 {
61+
return nil, fmt.Errorf("no catch-all drop rule found in firewall rules")
62+
}
63+
64+
// Find matching rules that are currently before the catch-all
65+
matchedIndices := findMatchingRules(server.FirewallRules, &s.params)
66+
var rulesToMove []int
67+
for _, idx := range matchedIndices {
68+
if server.FirewallRules[idx].Position < catchAllPosition {
69+
rulesToMove = append(rulesToMove, idx)
70+
}
71+
}
72+
73+
if len(rulesToMove) == 0 {
74+
return nil, fmt.Errorf("no enabled firewall rules matched the specified filters (rules before catch-all at position %d)", catchAllPosition)
75+
}
76+
77+
// Confirm if multiple rules or if confirmation required
78+
if len(rulesToMove) > s.params.skipConfirmation {
79+
exec.PushProgressUpdate(fmt.Sprintf("Found %d enabled firewall rules to disable:", len(rulesToMove)))
80+
for _, idx := range rulesToMove {
81+
rule := &server.FirewallRules[idx]
82+
exec.PushProgressUpdate(fmt.Sprintf(" Position %d: %s %s %s",
83+
rule.Position, rule.Direction, rule.Protocol, rule.Comment))
84+
}
85+
86+
if !ui.Confirm(fmt.Sprintf("Disable %d firewall rules?", len(rulesToMove))) {
87+
return output.None{}, nil
88+
}
89+
}
90+
91+
// Sort by position (descending) to move rules from highest position first
92+
sort.Slice(rulesToMove, func(i, j int) bool {
93+
return server.FirewallRules[rulesToMove[i]].Position > server.FirewallRules[rulesToMove[j]].Position
94+
})
95+
96+
// Move each rule to after the catch-all
97+
movedCount := 0
98+
for _, idx := range rulesToMove {
99+
rule := &server.FirewallRules[idx]
100+
oldPosition := rule.Position
101+
// Move to just after catch-all
102+
newPosition := catchAllPosition + 1
103+
104+
msg := fmt.Sprintf("Disabling rule (moving from position %d to %d)", oldPosition, newPosition)
105+
exec.PushProgressStarted(msg)
106+
107+
// Create the modified rule at the new position
108+
newRule := *rule
109+
newRule.Position = newPosition
110+
111+
// Delete the old rule first
112+
err := exec.Firewall().DeleteFirewallRule(exec.Context(), &request.DeleteFirewallRuleRequest{
113+
ServerUUID: arg,
114+
Position: oldPosition,
115+
})
116+
if err != nil {
117+
exec.PushProgressFailed(msg)
118+
if movedCount > 0 {
119+
exec.PushProgressUpdate(fmt.Sprintf("Successfully disabled %d rules before error", movedCount))
120+
}
121+
return nil, err
122+
}
123+
124+
// Create the rule at the new position (after catch-all)
125+
_, err = exec.Firewall().CreateFirewallRule(exec.Context(), &request.CreateFirewallRuleRequest{
126+
ServerUUID: arg,
127+
FirewallRule: request.FirewallRule{
128+
Direction: newRule.Direction,
129+
Action: newRule.Action,
130+
Family: newRule.Family,
131+
Protocol: newRule.Protocol,
132+
ICMPType: newRule.ICMPType,
133+
DestinationAddressStart: newRule.DestinationAddressStart,
134+
DestinationAddressEnd: newRule.DestinationAddressEnd,
135+
DestinationPortStart: newRule.DestinationPortStart,
136+
DestinationPortEnd: newRule.DestinationPortEnd,
137+
SourceAddressStart: newRule.SourceAddressStart,
138+
SourceAddressEnd: newRule.SourceAddressEnd,
139+
SourcePortStart: newRule.SourcePortStart,
140+
SourcePortEnd: newRule.SourcePortEnd,
141+
Comment: newRule.Comment,
142+
},
143+
Position: newPosition,
144+
})
145+
if err != nil {
146+
exec.PushProgressFailed(msg)
147+
return nil, fmt.Errorf("failed to recreate rule at position %d: %w", newPosition, err)
148+
}
149+
150+
exec.PushProgressSuccess(msg)
151+
movedCount++
152+
153+
// The catch-all position shifts down by 1 after each move (since we're moving from before it)
154+
catchAllPosition--
155+
}
156+
157+
return output.None{}, nil
47158
}

0 commit comments

Comments
 (0)