Skip to content

Commit 0664d08

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

3 files changed

Lines changed: 155 additions & 61 deletions

File tree

‎drivers/google/compute_util.go‎

Lines changed: 55 additions & 33 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,40 @@ 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)
194-
}
198+
// if all the ports match then there is no need
199+
// to update / recreate the rule.
200+
if reflect.DeepEqual(requestedPorts, opened) {
201+
return false
195202
}
196-
if len(missing) > 0 {
197-
log.Warnf("found missing ports for firewall rule '%s': %v", rule.Name, missing)
203+
204+
rule.Allowed = []*raw.FirewallAllowed{}
205+
for proto, ports := range requestedPorts {
206+
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
207+
IPProtocol: proto,
208+
// note that Ports can only include numbers, and not
209+
// number protocol pairs.
210+
Ports: ports,
211+
})
198212
}
199-
return missing
213+
214+
return true
200215
}
201216

202217
func (c *ComputeUtil) portsUsed() ([]string, error) {
@@ -233,8 +248,6 @@ func (c *ComputeUtil) openInternalFirewallPorts(d *Driver) error {
233248
"6443",
234249
"2379",
235250
"2380",
236-
"4789",
237-
"2376",
238251
"9345",
239252
"9796",
240253
"8472/udp",
@@ -256,17 +269,20 @@ func (c *ComputeUtil) openInternalFirewallPorts(d *Driver) error {
256269
}
257270
}
258271

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
272+
// ensure an existing firewall rule properly points to the specified network
273+
networkChanged := false
274+
desiredNet := c.globalURL + "/networks/" + d.Network
275+
if !create {
276+
if desiredNet != rule.Network {
277+
networkChanged = true
278+
rule.Network = desiredNet
279+
}
263280
}
264281

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

272288
var err error
@@ -298,7 +314,6 @@ func (c *ComputeUtil) openPublicFirewallPorts(d *Driver) error {
298314
Description: "rancher-machine managed external firewall rule",
299315
Allowed: []*raw.FirewallAllowed{},
300316
SourceRanges: []string{"0.0.0.0/0"},
301-
SourceTags: []string{c.externalFirewallRuleName()},
302317
TargetTags: []string{c.externalFirewallRuleName()},
303318
Network: c.globalURL + "/networks/" + d.Network,
304319
}
@@ -309,16 +324,20 @@ func (c *ComputeUtil) openPublicFirewallPorts(d *Driver) error {
309324
return err
310325
}
311326

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
327+
// ensure an existing firewall rule properly points to the specified network
328+
networkChanged := false
329+
desiredNet := c.globalURL + "/networks/" + d.Network
330+
if !create {
331+
if desiredNet != rule.Network {
332+
networkChanged = true
333+
rule.Network = desiredNet
334+
}
316335
}
317-
for proto, ports := range missingPorts {
318-
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
319-
IPProtocol: proto,
320-
Ports: ports,
321-
})
336+
337+
// ensure the rule is specifying only the requested ports
338+
if !updatePorts(rule, portsUsed) && !networkChanged {
339+
log.Debugf("Do not need to update internal firewall rule '%s' as all ports are configured", rule.Name)
340+
return nil
322341
}
323342

324343
var op *raw.Operation
@@ -361,6 +380,9 @@ func (c *ComputeUtil) CleanUpFirewallRule(rule *raw.Firewall, labelKey string) e
361380
log.Infof("Removing rancher-machine managed firewall rule '%s' from project as no instances are using it", rule.Name)
362381
op, err := c.service.Firewalls.Delete(c.project, rule.Name).Do()
363382
if err != nil {
383+
if isNotFound(err) {
384+
return nil
385+
}
364386
return fmt.Errorf("failed to remove rancher-machine managed firewall rule '%s': %w", rule.Name, err)
365387
}
366388

‎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)