Skip to content

Commit 902d6f4

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

3 files changed

Lines changed: 181 additions & 62 deletions

File tree

‎drivers/google/compute_util.go‎

Lines changed: 78 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"net/url"
1010
"os"
1111
"regexp"
12+
"slices"
1213
"strings"
1314
"time"
1415

@@ -47,7 +48,7 @@ type ComputeUtil struct {
4748

4849
const (
4950
apiURL = "https://www.googleapis.com/compute/v1/projects/"
50-
externalFirewallRuleSuffix = "external-rancher-node"
51+
externalFirewallRuleSuffix = "external-rancher-nodes"
5152
internalFirewallRuleSuffix = "internal-rancher-nodes"
5253
externalFirewallRuleLabelKey = "rancher-external-fw-rule"
5354
internalFirewallRuleLabelKey = "rancher-internal-fw-rule"
@@ -177,26 +178,63 @@ 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+
for _, p := range ports {
205+
// a port needs to be closed
206+
if !slices.Contains(requestedPorts[proto], p) {
207+
recreate = true
208+
break
209+
}
194210
}
195211
}
196-
if len(missing) > 0 {
197-
log.Warnf("found missing ports for firewall rule '%s': %v", rule.Name, missing)
212+
213+
for proto, ports := range requestedPorts {
214+
for _, p := range ports {
215+
// a port needs to be opened
216+
if !slices.Contains(opened[proto], p) {
217+
recreate = true
218+
break
219+
}
220+
}
198221
}
199-
return missing
222+
223+
if !recreate {
224+
return false
225+
}
226+
227+
rule.Allowed = []*raw.FirewallAllowed{}
228+
for proto, ports := range requestedPorts {
229+
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
230+
IPProtocol: proto,
231+
// note that Ports can only include numbers, and not
232+
// number protocol pairs.
233+
Ports: ports,
234+
})
235+
}
236+
237+
return true
200238
}
201239

202240
func (c *ComputeUtil) portsUsed() ([]string, error) {
@@ -233,8 +271,6 @@ func (c *ComputeUtil) openInternalFirewallPorts(d *Driver) error {
233271
"6443",
234272
"2379",
235273
"2380",
236-
"4789",
237-
"2376",
238274
"9345",
239275
"9796",
240276
"8472/udp",
@@ -256,17 +292,20 @@ func (c *ComputeUtil) openInternalFirewallPorts(d *Driver) error {
256292
}
257293
}
258294

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
295+
// ensure an existing firewall rule properly points to the specified network
296+
networkChanged := false
297+
desiredNet := c.globalURL + "/networks/" + d.Network
298+
if !create {
299+
if desiredNet != rule.Network {
300+
networkChanged = true
301+
rule.Network = desiredNet
302+
}
263303
}
264304

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

272311
var err error
@@ -298,7 +337,6 @@ func (c *ComputeUtil) openPublicFirewallPorts(d *Driver) error {
298337
Description: "rancher-machine managed external firewall rule",
299338
Allowed: []*raw.FirewallAllowed{},
300339
SourceRanges: []string{"0.0.0.0/0"},
301-
SourceTags: []string{c.externalFirewallRuleName()},
302340
TargetTags: []string{c.externalFirewallRuleName()},
303341
Network: c.globalURL + "/networks/" + d.Network,
304342
}
@@ -309,16 +347,20 @@ func (c *ComputeUtil) openPublicFirewallPorts(d *Driver) error {
309347
return err
310348
}
311349

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
350+
// ensure an existing firewall rule properly points to the specified network
351+
networkChanged := false
352+
desiredNet := c.globalURL + "/networks/" + d.Network
353+
if !create {
354+
if desiredNet != rule.Network {
355+
networkChanged = true
356+
rule.Network = desiredNet
357+
}
316358
}
317-
for proto, ports := range missingPorts {
318-
rule.Allowed = append(rule.Allowed, &raw.FirewallAllowed{
319-
IPProtocol: proto,
320-
Ports: ports,
321-
})
359+
360+
// ensure the rule is specifying only the requested ports
361+
if !updatePorts(rule, portsUsed) && !networkChanged {
362+
log.Debugf("Do not need to update internal firewall rule '%s' as all ports are configured", rule.Name)
363+
return nil
322364
}
323365

324366
var op *raw.Operation
@@ -361,6 +403,9 @@ func (c *ComputeUtil) CleanUpFirewallRule(rule *raw.Firewall, labelKey string) e
361403
log.Infof("Removing rancher-machine managed firewall rule '%s' from project as no instances are using it", rule.Name)
362404
op, err := c.service.Firewalls.Delete(c.project, rule.Name).Do()
363405
if err != nil {
406+
if isNotFound(err) {
407+
return nil
408+
}
364409
return fmt.Errorf("failed to remove rancher-machine managed firewall rule '%s': %w", rule.Name, err)
365410
}
366411

‎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", "443"},
95+
},
96+
},
97+
},
98+
incomingPorts: []string{"443", "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: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func (d *Driver) GetCreateFlags() []mcnflag.Flag {
176176
},
177177
mcnflag.StringFlag{
178178
Name: "google-labels",
179-
Usage: "labels to add onto the created node",
179+
Usage: "labels to add onto the created virtual machine",
180180
EnvVar: "GOOGLE_LABELS",
181181
Value: "",
182182
},
@@ -503,26 +503,43 @@ 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+
} else {
517+
log.Infof("external firewall rule '%s' does not exist, nothing to do", c.externalFirewallRuleName())
518+
}
519+
} else {
520+
if err := c.CleanUpFirewallRule(fwRule, externalFirewallRuleLabelKey); err != nil {
521+
log.Errorf("failed remove external firewall rule '%s': %v", c.externalFirewallRuleName(), err)
522+
errs = append(errs, err)
523+
}
513524
}
514525
}
515526

516527
if c.internalFirewallRulePrefix != "" {
517528
internalFwRule, err := c.internalFirewallRule()
518529
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)
530+
if !isNotFound(err) {
531+
log.Warnf("failed to get internal firewall rule '%s' while delete VM: %v", c.internalFirewallRuleName(), err)
532+
errs = append(errs, err)
533+
} else {
534+
log.Infof("internal firewall rule '%s' does not exist, nothing to do", c.internalFirewallRuleName())
535+
}
536+
} else {
537+
if err := c.CleanUpFirewallRule(internalFwRule, internalFirewallRuleLabelKey); err != nil {
538+
log.Errorf("failed to remove internal firewall rule '%s': %v", c.internalFirewallRuleName(), err)
539+
errs = append(errs, err)
540+
}
524541
}
525542
}
526543

527-
return nil
544+
return errors.Join(errs...)
528545
}

0 commit comments

Comments
 (0)