Skip to content

Commit 4aa8fdb

Browse files
HarrisonWAffelHarrisonWAffel
committed
ensure ports and networks are properly updated, return errors when failing to delete firewall rules.
1 parent 3180e5d commit 4aa8fdb

3 files changed

Lines changed: 170 additions & 60 deletions

File tree

‎drivers/google/compute_util.go‎

Lines changed: 70 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http"
99
"net/url"
1010
"os"
11+
"reflect"
1112
"regexp"
1213
"strings"
1314
"time"
@@ -177,26 +178,59 @@ func (c *ComputeUtil) internalFirewallRuleName() string {
177178
return name.SafeConcatName(c.internalFirewallRulePrefix, internalFirewallRuleSuffix)
178179
}
179180

180-
func missingOpenedPorts(rule *raw.Firewall, ports []string) map[string][]string {
181-
missing := map[string][]string{}
182-
opened := map[string]bool{}
181+
// updatePorts compares the provided firewall rule against the list of provided ports
182+
// and returns a boolean indicating if the provided rule has been updated to only include
183+
// the provided ports.
184+
func updatePorts(rule *raw.Firewall, ports []string) bool {
185+
requestedPorts := map[string][]string{}
186+
for _, p := range ports {
187+
port, proto := driverutil.SplitPortProto(p)
188+
requestedPorts[proto] = append(requestedPorts[proto], port)
189+
}
183190

191+
opened := map[string][]string{}
184192
for _, allowed := range rule.Allowed {
185193
for _, allowedPort := range allowed.Ports {
186-
opened[allowedPort+"/"+allowed.IPProtocol] = true
194+
opened[allowed.IPProtocol] = append(opened[allowed.IPProtocol], allowedPort)
187195
}
188196
}
189197

190-
for _, p := range ports {
191-
port, proto := driverutil.SplitPortProto(p)
192-
if !opened[port+"/"+proto] {
193-
missing[proto] = append(missing[proto], port)
198+
// if there is a mismatch between the currently opened
199+
// ports and existing ports, recreate the rule with only
200+
// the requested ports.
201+
recreate := false
202+
203+
for proto, ports := range opened {
204+
if !reflect.DeepEqual(requestedPorts[proto], ports) {
205+
recreate = true
206+
break
207+
}
208+
}
209+
210+
if !recreate {
211+
for proto, ports := range requestedPorts {
212+
if !reflect.DeepEqual(requestedPorts[proto], ports) {
213+
recreate = true
214+
break
215+
}
194216
}
195217
}
196-
if len(missing) > 0 {
197-
log.Warnf("found missing ports for firewall rule '%s': %v", rule.Name, missing)
218+
219+
if !recreate {
220+
return false
221+
}
222+
223+
rule.Allowed = []*raw.FirewallAllowed{}
224+
for proto, ports := range requestedPorts {
225+
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
226+
IPProtocol: proto,
227+
// note that Ports can only include numbers, and not
228+
// number protocol pairs.
229+
Ports: ports,
230+
})
198231
}
199-
return missing
232+
233+
return true
200234
}
201235

202236
func (c *ComputeUtil) portsUsed() ([]string, error) {
@@ -233,8 +267,6 @@ func (c *ComputeUtil) openInternalFirewallPorts(d *Driver) error {
233267
"6443",
234268
"2379",
235269
"2380",
236-
"4789",
237-
"2376",
238270
"9345",
239271
"9796",
240272
"8472/udp",
@@ -256,17 +288,20 @@ func (c *ComputeUtil) openInternalFirewallPorts(d *Driver) error {
256288
}
257289
}
258290

259-
missingPorts := missingOpenedPorts(rule, expectedPorts)
260-
if len(missingPorts) == 0 {
261-
log.Debugf("Do not need to update internal firewall rule '%s' as all ports are configured", rule.Name)
262-
return nil
291+
// ensure an existing firewall rule properly points to the specified network
292+
networkChanged := false
293+
desiredNet := c.globalURL + "/networks/" + d.Network
294+
if !create {
295+
if desiredNet != rule.Network {
296+
networkChanged = true
297+
rule.Network = desiredNet
298+
}
263299
}
264300

265-
for proto, ports := range missingPorts {
266-
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
267-
IPProtocol: proto,
268-
Ports: ports,
269-
})
301+
// ensure the rule is specifying only the expected ports
302+
if !updatePorts(rule, expectedPorts) && !networkChanged {
303+
log.Debugf("Do not need to update internal firewall rule '%s' as all ports are configured", rule.Name)
304+
return nil
270305
}
271306

272307
var err error
@@ -298,7 +333,6 @@ func (c *ComputeUtil) openPublicFirewallPorts(d *Driver) error {
298333
Description: "rancher-machine managed external firewall rule",
299334
Allowed: []*raw.FirewallAllowed{},
300335
SourceRanges: []string{"0.0.0.0/0"},
301-
SourceTags: []string{c.externalFirewallRuleName()},
302336
TargetTags: []string{c.externalFirewallRuleName()},
303337
Network: c.globalURL + "/networks/" + d.Network,
304338
}
@@ -309,16 +343,20 @@ func (c *ComputeUtil) openPublicFirewallPorts(d *Driver) error {
309343
return err
310344
}
311345

312-
missingPorts := missingOpenedPorts(rule, portsUsed)
313-
if len(missingPorts) == 0 {
314-
log.Infof("Do not need to update external firewall rule '%s' as all ports are configured", rule.Name)
315-
return nil
346+
// ensure an existing firewall rule properly points to the specified network
347+
networkChanged := false
348+
desiredNet := c.globalURL + "/networks/" + d.Network
349+
if !create {
350+
if desiredNet != rule.Network {
351+
networkChanged = true
352+
rule.Network = desiredNet
353+
}
316354
}
317-
for proto, ports := range missingPorts {
318-
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
319-
IPProtocol: proto,
320-
Ports: ports,
321-
})
355+
356+
// ensure the rule is specifying only the requested ports
357+
if !updatePorts(rule, portsUsed) && !networkChanged {
358+
log.Debugf("Do not need to update internal firewall rule '%s' as all ports are configured", rule.Name)
359+
return nil
322360
}
323361

324362
var op *raw.Operation

‎drivers/google/compute_util_test.go‎

Lines changed: 75 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
package google
22

33
import (
4+
"slices"
45
"testing"
56

7+
"github.com/rancher/machine/drivers/driverutil"
68
"github.com/rancher/wrangler/v3/pkg/name"
79
"github.com/stretchr/testify/assert"
810
raw "google.golang.org/api/compute/v1"
@@ -75,28 +77,83 @@ func TestPortsUsed(t *testing.T) {
7577
}
7678
}
7779

78-
func TestMissingOpenedPorts(t *testing.T) {
80+
func TestUpdatePorts(t *testing.T) {
81+
7982
var tests = []struct {
80-
description string
81-
allowed []*raw.FirewallAllowed
82-
ports []string
83-
expectedMissing map[string][]string
83+
name string
84+
rule *raw.Firewall
85+
incomingPorts []string
86+
diffExpected bool
8487
}{
85-
{"no port opened", []*raw.FirewallAllowed{}, []string{"2376"}, map[string][]string{"tcp": {"2376"}}},
86-
{"docker port opened", []*raw.FirewallAllowed{{IPProtocol: "tcp", Ports: []string{"2376"}}}, []string{"2376"}, map[string][]string{}},
87-
{"missing swarm port", []*raw.FirewallAllowed{{IPProtocol: "tcp", Ports: []string{"2376"}}}, []string{"2376", "3376"}, map[string][]string{"tcp": {"3376"}}},
88-
{"missing docker port", []*raw.FirewallAllowed{{IPProtocol: "tcp", Ports: []string{"3376"}}}, []string{"2376", "3376"}, map[string][]string{"tcp": {"2376"}}},
89-
{"both ports opened", []*raw.FirewallAllowed{{IPProtocol: "tcp", Ports: []string{"2376", "3376"}}}, []string{"2376", "3376"}, map[string][]string{}},
90-
{"more ports opened", []*raw.FirewallAllowed{{IPProtocol: "tcp", Ports: []string{"2376", "3376", "22", "1024-2048"}}}, []string{"2376", "3376"}, map[string][]string{}},
91-
{"additional missing", []*raw.FirewallAllowed{{IPProtocol: "tcp", Ports: []string{"2376", "2377/tcp"}}}, []string{"2377/udp", "80/tcp", "2376"}, map[string][]string{"tcp": {"80"}, "udp": {"2377"}}},
88+
{
89+
name: "no change",
90+
rule: &raw.Firewall{
91+
Allowed: []*raw.FirewallAllowed{
92+
{
93+
IPProtocol: "tcp",
94+
Ports: []string{"80"},
95+
},
96+
},
97+
},
98+
incomingPorts: []string{"80"},
99+
diffExpected: false,
100+
},
101+
{
102+
name: "add ports",
103+
rule: &raw.Firewall{
104+
Allowed: []*raw.FirewallAllowed{
105+
{
106+
IPProtocol: "tcp",
107+
Ports: []string{"80"},
108+
},
109+
},
110+
},
111+
incomingPorts: []string{"80/tcp", "443/tcp", "123/udp"},
112+
diffExpected: true,
113+
},
114+
{
115+
name: "remove ports",
116+
rule: &raw.Firewall{
117+
Allowed: []*raw.FirewallAllowed{
118+
{
119+
IPProtocol: "tcp",
120+
Ports: []string{"80", "443"},
121+
},
122+
{
123+
IPProtocol: "udp",
124+
Ports: []string{"123"},
125+
},
126+
},
127+
},
128+
incomingPorts: []string{"80/tcp"},
129+
diffExpected: true,
130+
},
92131
}
93132

94-
for _, test := range tests {
95-
firewall := &raw.Firewall{Allowed: test.allowed}
96-
97-
missingPorts := missingOpenedPorts(firewall, test.ports)
98-
99-
assert.Equal(t, test.expectedMissing, missingPorts, test.description)
133+
for _, tt := range tests {
134+
t.Run(tt.name, func(t *testing.T) {
135+
diff := updatePorts(tt.rule, tt.incomingPorts)
136+
if diff && !tt.diffExpected {
137+
t.Logf("expected change to be %t, but got %t", tt.diffExpected, diff)
138+
t.Fail()
139+
}
140+
141+
for _, p := range tt.incomingPorts {
142+
port, proto := driverutil.SplitPortProto(p)
143+
switch proto {
144+
case "udp":
145+
if !slices.Contains(tt.rule.Allowed[1].Ports, port) {
146+
t.Logf("expected port %s to be in allowed list", port)
147+
t.Fail()
148+
}
149+
default:
150+
if !slices.Contains(tt.rule.Allowed[0].Ports, port) {
151+
t.Logf("expected port %s to be in allowed list", port)
152+
t.Fail()
153+
}
154+
}
155+
}
156+
})
100157
}
101158
}
102159

‎drivers/google/google.go‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -503,26 +503,41 @@ func (d *Driver) Remove() error {
503503
}
504504
}
505505

506+
// collect all errors and only return them
507+
// later. If we fail to destroy one firewall,
508+
// we should still attempt to remove the other.
509+
var errs []error
506510
if len(d.OpenPorts) > 0 {
507511
fwRule, err := c.externalFirewallRule()
508512
if err != nil {
509-
log.Warnf("failed to get external firewall rule '%s' while deleting VM: %v", c.externalFirewallRuleName(), err)
510-
}
511-
if err := c.CleanUpFirewallRule(fwRule, externalFirewallRuleLabelKey); err != nil {
512-
log.Errorf("failed remove external firewall rule '%s': %v", c.externalFirewallRuleName(), err)
513+
if !isNotFound(err) {
514+
log.Warnf("failed to get external firewall rule '%s' while deleting VM: %v", c.externalFirewallRuleName(), err)
515+
errs = append(errs, err)
516+
}
517+
log.Infof("external firewall rule '%s' does not exist, nothing to do", c.externalFirewallRuleName())
518+
} else {
519+
if err := c.CleanUpFirewallRule(fwRule, externalFirewallRuleLabelKey); err != nil {
520+
log.Errorf("failed remove external firewall rule '%s': %v", c.externalFirewallRuleName(), err)
521+
errs = append(errs, err)
522+
}
513523
}
514524
}
515525

516526
if c.internalFirewallRulePrefix != "" {
517527
internalFwRule, err := c.internalFirewallRule()
518528
if err != nil {
519-
log.Warnf("failed to get internal firewall rule '%s' while delete VM: %v", c.internalFirewallRuleName(), err)
520-
}
521-
522-
if err := c.CleanUpFirewallRule(internalFwRule, internalFirewallRuleLabelKey); err != nil {
523-
log.Errorf("failed to remove internal firewall rule '%s': %v", c.internalFirewallRuleName(), err)
529+
if !isNotFound(err) {
530+
log.Warnf("failed to get internal firewall rule '%s' while delete VM: %v", c.internalFirewallRuleName(), err)
531+
errs = append(errs, err)
532+
}
533+
log.Infof("internal firewall rule '%s' does not exist, nothing to do", c.internalFirewallRuleName())
534+
} else {
535+
if err := c.CleanUpFirewallRule(internalFwRule, internalFirewallRuleLabelKey); err != nil {
536+
log.Errorf("failed to remove internal firewall rule '%s': %v", c.internalFirewallRuleName(), err)
537+
errs = append(errs, err)
538+
}
524539
}
525540
}
526541

527-
return nil
542+
return errors.Join(errs...)
528543
}

0 commit comments

Comments
 (0)