Skip to content

Commit 93e782b

Browse files
justinsbjustinsb
committed
gce: use set type where order does not matter
Starting small with FirewallRule Allowed and SourceRanges, but this should be easier than explicitly handling values where order does not matter.
1 parent 78d4757 commit 93e782b

File tree

6 files changed

+79
-61
lines changed

6 files changed

+79
-61
lines changed

‎pkg/model/gcemodel/api_loadbalancer.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strconv"
2222

2323
"golang.org/x/exp/slices"
24+
"k8s.io/apimachinery/pkg/util/sets"
2425
"k8s.io/kops/pkg/apis/kops"
2526
"k8s.io/kops/pkg/wellknownports"
2627
"k8s.io/kops/pkg/wellknownservices"
@@ -101,9 +102,9 @@ func (b *APILoadBalancerBuilder) addFirewallRules(c *fi.CloudupModelBuilderConte
101102
b.AddFirewallRulesTasks(c, "https-api", &gcetasks.FirewallRule{
102103
Lifecycle: b.Lifecycle,
103104
Network: network,
104-
SourceRanges: b.Cluster.Spec.API.Access,
105+
SourceRanges: sets.New(b.Cluster.Spec.API.Access...),
105106
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane)},
106-
Allowed: []string{"tcp:" + strconv.Itoa(wellknownports.KubeAPIServer)},
107+
Allowed: sets.New("tcp:" + strconv.Itoa(wellknownports.KubeAPIServer)),
107108
})
108109

109110
if b.NetworkingIsIPAlias() {
@@ -112,19 +113,19 @@ func (b *APILoadBalancerBuilder) addFirewallRules(c *fi.CloudupModelBuilderConte
112113
Lifecycle: b.Lifecycle,
113114
Network: network,
114115
Family: gcetasks.AddressFamilyIPv4, // ip alias is always ipv4
115-
SourceRanges: []string{b.Cluster.Spec.Networking.PodCIDR},
116+
SourceRanges: sets.New(b.Cluster.Spec.Networking.PodCIDR),
116117
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane)},
117-
Allowed: []string{"tcp:" + strconv.Itoa(wellknownports.KubeAPIServer)},
118+
Allowed: sets.New("tcp:" + strconv.Itoa(wellknownports.KubeAPIServer)),
118119
})
119120
}
120121

121122
if b.Cluster.UsesNoneDNS() {
122123
b.AddFirewallRulesTasks(c, "kops-controller", &gcetasks.FirewallRule{
123124
Lifecycle: b.Lifecycle,
124125
Network: network,
125-
SourceRanges: b.Cluster.Spec.API.Access,
126+
SourceRanges: sets.New(b.Cluster.Spec.API.Access...),
126127
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane)},
127-
Allowed: []string{"tcp:" + strconv.Itoa(wellknownports.KopsControllerPort)},
128+
Allowed: sets.New("tcp:" + strconv.Itoa(wellknownports.KopsControllerPort)),
128129
})
129130
}
130131
}

‎pkg/model/gcemodel/external_access.go

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package gcemodel
1919
import (
2020
"strconv"
2121

22+
"k8s.io/apimachinery/pkg/util/sets"
2223
"k8s.io/klog/v2"
2324
"k8s.io/kops/pkg/apis/kops"
2425
"k8s.io/kops/pkg/wellknownports"
@@ -58,21 +59,21 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.CloudupModelBuilderContext) err
5859
b.AddFirewallRulesTasks(c, "ssh-external-to-bastion", &gcetasks.FirewallRule{
5960
Lifecycle: b.Lifecycle,
6061
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleBastion)},
61-
Allowed: []string{"tcp:22"},
62-
SourceRanges: b.Cluster.Spec.SSHAccess,
62+
Allowed: sets.New("tcp:22"),
63+
SourceRanges: sets.New(b.Cluster.Spec.SSHAccess...),
6364
Network: network,
6465
})
6566
b.AddFirewallRulesTasks(c, "bastion-to-master-ssh", &gcetasks.FirewallRule{
6667
Lifecycle: b.Lifecycle,
6768
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
68-
Allowed: []string{"tcp:22"},
69+
Allowed: sets.New("tcp:22"),
6970
SourceTags: []string{b.GCETagForRole(kops.InstanceGroupRoleBastion)},
7071
Network: network,
7172
})
7273
b.AddFirewallRulesTasks(c, "bastion-to-node-ssh", &gcetasks.FirewallRule{
7374
Lifecycle: b.Lifecycle,
7475
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
75-
Allowed: []string{"tcp:22"},
76+
Allowed: sets.New("tcp:22"),
7677
SourceTags: []string{b.GCETagForRole(kops.InstanceGroupRoleBastion)},
7778
Network: network,
7879
})
@@ -84,16 +85,16 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.CloudupModelBuilderContext) err
8485
b.AddFirewallRulesTasks(c, "ssh-external-to-master", &gcetasks.FirewallRule{
8586
Lifecycle: b.Lifecycle,
8687
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
87-
Allowed: []string{"tcp:22"},
88-
SourceRanges: b.Cluster.Spec.SSHAccess,
88+
Allowed: sets.New("tcp:22"),
89+
SourceRanges: sets.New(b.Cluster.Spec.SSHAccess...),
8990
Network: network,
9091
})
9192

9293
b.AddFirewallRulesTasks(c, "ssh-external-to-node", &gcetasks.FirewallRule{
9394
Lifecycle: b.Lifecycle,
9495
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
95-
Allowed: []string{"tcp:22"},
96-
SourceRanges: b.Cluster.Spec.SSHAccess,
96+
Allowed: sets.New("tcp:22"),
97+
SourceRanges: sets.New(b.Cluster.Spec.SSHAccess...),
9798
Network: network,
9899
})
99100
}
@@ -113,11 +114,11 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.CloudupModelBuilderContext) err
113114
b.AddFirewallRulesTasks(c, "nodeport-external-to-node", &gcetasks.FirewallRule{
114115
Lifecycle: b.Lifecycle,
115116
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
116-
Allowed: []string{
117-
"tcp:" + nodePortRangeString,
118-
"udp:" + nodePortRangeString,
119-
},
120-
SourceRanges: b.Cluster.Spec.NodePortAccess,
117+
Allowed: sets.New(
118+
"tcp:"+nodePortRangeString,
119+
"udp:"+nodePortRangeString,
120+
),
121+
SourceRanges: sets.New(b.Cluster.Spec.NodePortAccess...),
121122
Network: network,
122123
})
123124
}
@@ -136,8 +137,8 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.CloudupModelBuilderContext) err
136137
b.AddFirewallRulesTasks(c, "kubernetes-master-https", &gcetasks.FirewallRule{
137138
Lifecycle: b.Lifecycle,
138139
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
139-
Allowed: []string{"tcp:443"},
140-
SourceRanges: b.Cluster.Spec.API.Access,
140+
Allowed: sets.New("tcp:443"),
141+
SourceRanges: sets.New(b.Cluster.Spec.API.Access...),
141142
Network: network,
142143
})
143144

@@ -147,9 +148,9 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.CloudupModelBuilderContext) err
147148
Lifecycle: b.Lifecycle,
148149
Network: network,
149150
Family: gcetasks.AddressFamilyIPv4, // ip alias is always ipv4
150-
SourceRanges: []string{b.Cluster.Spec.Networking.PodCIDR},
151+
SourceRanges: sets.New(b.Cluster.Spec.Networking.PodCIDR),
151152
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane)},
152-
Allowed: []string{"tcp:" + strconv.Itoa(wellknownports.KubeAPIServer)},
153+
Allowed: sets.New("tcp:" + strconv.Itoa(wellknownports.KubeAPIServer)),
153154
})
154155
}
155156
}

‎pkg/model/gcemodel/firewall.go

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"net"
2222
"strings"
2323

24+
"k8s.io/apimachinery/pkg/util/sets"
2425
"k8s.io/klog/v2"
2526
"k8s.io/kops/pkg/apis/kops"
2627
"k8s.io/kops/pkg/apis/kops/model"
@@ -57,16 +58,16 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
5758
Lifecycle: b.Lifecycle,
5859
Network: network,
5960
Family: gcetasks.AddressFamilyIPv4,
60-
SourceRanges: []string{
61+
SourceRanges: sets.New(
6162
// IP ranges for load balancer health checks
6263
// https://cloud.google.com/load-balancing/docs/health-checks
6364
"35.191.0.0/16",
6465
"130.211.0.0/22",
6566
"209.85.204.0/22",
6667
"209.85.152.0/22",
67-
},
68+
),
6869
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane)},
69-
Allowed: []string{"tcp"},
70+
Allowed: sets.New("tcp"),
7071
})
7172
}
7273

@@ -82,7 +83,7 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
8283
Network: network,
8384
SourceTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
8485
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
85-
Allowed: allProtocols,
86+
Allowed: sets.New(allProtocols...),
8687
}
8788
c.AddTask(t)
8889
}
@@ -99,7 +100,7 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
99100
Network: network,
100101
SourceTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
101102
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
102-
Allowed: allProtocols,
103+
Allowed: sets.New(allProtocols...),
103104
}
104105
c.AddTask(t)
105106
}
@@ -116,7 +117,7 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
116117
Network: network,
117118
SourceTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
118119
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
119-
Allowed: allProtocols,
120+
Allowed: sets.New(allProtocols...),
120121
}
121122
c.AddTask(t)
122123
}
@@ -133,25 +134,25 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
133134
Network: network,
134135
SourceTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
135136
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleControlPlane), b.GCETagForRole("Master")},
136-
Allowed: []string{
137+
Allowed: sets.New(
137138
fmt.Sprintf("tcp:%d", wellknownports.KubeAPIServer),
138139
fmt.Sprintf("tcp:%d", wellknownports.KubeletAPI),
139140
fmt.Sprintf("tcp:%d", wellknownports.KopsControllerPort),
140-
},
141+
),
141142
}
142143
if b.Cluster.UsesLegacyGossip() {
143-
t.Allowed = append(t.Allowed, fmt.Sprintf("udp:%d", wellknownports.DNSControllerGossipMemberlist))
144-
t.Allowed = append(t.Allowed, fmt.Sprintf("tcp:%d", wellknownports.DNSControllerGossipMemberlist))
145-
t.Allowed = append(t.Allowed, fmt.Sprintf("udp:%d", wellknownports.ProtokubeGossipMemberlist))
146-
t.Allowed = append(t.Allowed, fmt.Sprintf("tcp:%d", wellknownports.ProtokubeGossipMemberlist))
144+
t.Allowed.Insert(fmt.Sprintf("udp:%d", wellknownports.DNSControllerGossipMemberlist))
145+
t.Allowed.Insert(fmt.Sprintf("tcp:%d", wellknownports.DNSControllerGossipMemberlist))
146+
t.Allowed.Insert(fmt.Sprintf("udp:%d", wellknownports.ProtokubeGossipMemberlist))
147+
t.Allowed.Insert(fmt.Sprintf("tcp:%d", wellknownports.ProtokubeGossipMemberlist))
147148
}
148149
if b.NetworkingIsCalico() {
149-
t.Allowed = append(t.Allowed, "ipip")
150+
t.Allowed.Insert("ipip")
150151
}
151152
if b.NetworkingIsCilium() {
152-
t.Allowed = append(t.Allowed, fmt.Sprintf("udp:%d", wellknownports.VxlanUDP))
153+
t.Allowed.Insert(fmt.Sprintf("udp:%d", wellknownports.VxlanUDP))
153154
if model.UseCiliumEtcd(b.Cluster) {
154-
t.Allowed = append(t.Allowed, fmt.Sprintf("tcp:%d", wellknownports.EtcdCiliumClientPort))
155+
t.Allowed.Insert(fmt.Sprintf("tcp:%d", wellknownports.EtcdCiliumClientPort))
155156
}
156157
}
157158
c.AddTask(t)
@@ -174,9 +175,9 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
174175
b.AddFirewallRulesTasks(c, "pod-cidrs-to-node", &gcetasks.FirewallRule{
175176
Lifecycle: b.Lifecycle,
176177
Network: network,
177-
SourceRanges: []string{b.Cluster.Spec.Networking.PodCIDR},
178+
SourceRanges: sets.New(b.Cluster.Spec.Networking.PodCIDR),
178179
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
179-
Allowed: allProtocols,
180+
Allowed: sets.New(allProtocols...),
180181
})
181182
}
182183
}
@@ -189,19 +190,19 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
189190
// Furthermore, an empty SourceRange with empty SourceTags is interpreted as allow-everything,
190191
// but we intend for it to block everything; so we can Disabled to achieve the desired blocking.
191192
func (b *GCEModelContext) AddFirewallRulesTasks(c *fi.CloudupModelBuilderContext, name string, rule *gcetasks.FirewallRule) {
192-
var ipv4SourceRanges []string
193-
var ipv6SourceRanges []string
194-
for _, sourceRange := range rule.SourceRanges {
193+
ipv4SourceRanges := sets.New[string]()
194+
ipv6SourceRanges := sets.New[string]()
195+
for sourceRange := range rule.SourceRanges {
195196
_, cidr, err := net.ParseCIDR(sourceRange)
196197
if err != nil {
197198
klog.Fatalf("failed to parse invalid sourceRange %q", sourceRange)
198199
}
199200

200201
// Split into ipv4s and ipv6s, but treat IPv4-mapped IPv6 addresses as IPv6
201202
if cidr.IP.To4() != nil && !strings.Contains(sourceRange, ":") {
202-
ipv4SourceRanges = append(ipv4SourceRanges, sourceRange)
203+
ipv4SourceRanges.Insert(sourceRange)
203204
} else {
204-
ipv6SourceRanges = append(ipv6SourceRanges, sourceRange)
205+
ipv6SourceRanges.Insert(sourceRange)
205206
}
206207
}
207208

@@ -214,7 +215,7 @@ func (b *GCEModelContext) AddFirewallRulesTasks(c *fi.CloudupModelBuilderContext
214215
// This is helpful because empty SourceRanges and SourceTags are interpreted as allow everything,
215216
// but the intent is usually to block everything, which can be achieved with Disabled=true.
216217
ipv4.Disabled = true
217-
ipv4.SourceRanges = []string{"0.0.0.0/0"}
218+
ipv4.SourceRanges = sets.New("0.0.0.0/0")
218219
}
219220
}
220221
c.AddTask(&ipv4)
@@ -227,16 +228,16 @@ func (b *GCEModelContext) AddFirewallRulesTasks(c *fi.CloudupModelBuilderContext
227228
if len(ipv6.SourceRanges) == 0 {
228229
// We specify explicitly so the rule is in IPv6 mode
229230
ipv6.Disabled = true
230-
ipv6.SourceRanges = []string{"::/0"}
231+
ipv6.SourceRanges = sets.New("::/0")
231232
}
232233
}
233-
var ipv6Allowed []string
234-
for _, allowed := range ipv6.Allowed {
234+
ipv6Allowed := sets.New[string]()
235+
for allowed := range ipv6.Allowed {
235236
// Map icmp to icmpv6; easier than maintaining separate lists
236237
if allowed == "icmp" {
237238
allowed = "58" // 58 == the IANA protocol number for ICMPv6
238239
}
239-
ipv6Allowed = append(ipv6Allowed, allowed)
240+
ipv6Allowed.Insert(allowed)
240241
}
241242
ipv6.Allowed = ipv6Allowed
242243
c.AddTask(&ipv6)

‎upup/pkg/fi/changes.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,11 @@ func equalFieldValues(a, e reflect.Value) bool {
138138

139139
// equalMapValues performs a deep-equality check on a map, but using our custom comparison logic (equalFieldValues)
140140
func equalMapValues(a, e reflect.Value) bool {
141-
if a.IsNil() != e.IsNil() {
141+
aIsEmpty := a.IsNil() || a.Len() == 0
142+
eIsEmpty := e.IsNil() || e.Len() == 0
143+
if aIsEmpty != eIsEmpty {
142144
return false
143145
}
144-
if a.IsNil() && e.IsNil() {
145-
return true
146-
}
147146
if a.Len() != e.Len() {
148147
return false
149148
}

‎upup/pkg/fi/cloudup/gcetasks/firewallrule.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"strings"
2323

2424
compute "google.golang.org/api/compute/v1"
25+
"k8s.io/apimachinery/pkg/util/sets"
2526
"k8s.io/kops/upup/pkg/fi"
2627
"k8s.io/kops/upup/pkg/fi/cloudup/gce"
2728
"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
@@ -42,9 +43,9 @@ type FirewallRule struct {
4243

4344
Network *Network
4445
SourceTags []string
45-
SourceRanges []string
46+
SourceRanges sets.Set[string]
4647
TargetTags []string
47-
Allowed []string
48+
Allowed sets.Set[string]
4849

4950
// Disabled: Denotes whether the firewall rule is disabled. When set to
5051
// true, the firewall rule is not enforced and the network behaves as if
@@ -75,11 +76,14 @@ func (e *FirewallRule) Find(c *fi.CloudupContext) (*FirewallRule, error) {
7576
actual.Name = &r.Name
7677
actual.Network = &Network{Name: fi.PtrTo(lastComponent(r.Network))}
7778
actual.TargetTags = r.TargetTags
78-
actual.SourceRanges = r.SourceRanges
79+
actual.SourceRanges = sets.New(r.SourceRanges...)
7980
actual.SourceTags = r.SourceTags
8081
actual.Disabled = r.Disabled
8182
for _, a := range r.Allowed {
82-
actual.Allowed = append(actual.Allowed, serializeFirewallAllowed(a))
83+
if actual.Allowed == nil {
84+
actual.Allowed = sets.New[string]()
85+
}
86+
actual.Allowed.Insert(serializeFirewallAllowed(a))
8387
}
8488

8589
// Ignore "system" fields
@@ -114,7 +118,7 @@ func (e *FirewallRule) Normalize(c *fi.CloudupContext) error {
114118

115119
// Make sure we've split the ipv4 / ipv6 addresses.
116120
// A single firewall rule can't mix ipv4 and ipv6 addresses, so we split them into two rules.
117-
for _, sourceRange := range e.SourceRanges {
121+
for sourceRange := range e.SourceRanges {
118122
_, cidr, err := net.ParseCIDR(sourceRange)
119123
if err != nil {
120124
return fmt.Errorf("sourceRange %q is not valid: %w", sourceRange, err)
@@ -182,7 +186,7 @@ func serializeFirewallAllowed(r *compute.FirewallAllowed) string {
182186
func (e *FirewallRule) mapToGCE(project string) (*compute.Firewall, error) {
183187
var allowed []*compute.FirewallAllowed
184188
if e.Allowed != nil {
185-
for _, a := range e.Allowed {
189+
for _, a := range sets.List(e.Allowed) {
186190
p, err := parseFirewallAllowed(a)
187191
if err != nil {
188192
return nil, err
@@ -194,7 +198,7 @@ func (e *FirewallRule) mapToGCE(project string) (*compute.Firewall, error) {
194198
Name: *e.Name,
195199
Network: e.Network.URL(project),
196200
SourceTags: e.SourceTags,
197-
SourceRanges: e.SourceRanges,
201+
SourceRanges: sets.List(e.SourceRanges),
198202
TargetTags: e.TargetTags,
199203
Allowed: allowed,
200204
Disabled: e.Disabled,

‎upup/pkg/fi/topological_sort.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,18 @@ func getDependencies[T SubContext](tasks map[string]Task[T], v reflect.Value) []
125125
return nil
126126
}
127127

128+
// Ignore empty struct (struct{}) and other non-addressable types
129+
if !v.CanAddr() {
130+
typeName := v.Type().PkgPath() + "/" + v.Type().Name()
131+
switch typeName {
132+
case "k8s.io/apimachinery/pkg/util/sets/Empty":
133+
// known
134+
default:
135+
klog.Warningf("skipping non-addressable type %v name=%q", v.Type(), typeName)
136+
}
137+
return nil
138+
}
139+
128140
// TODO: Can we / should we use a type-switch statement
129141
intf := v.Addr().Interface()
130142
if hd, ok := intf.(HasDependencies[T]); ok {

0 commit comments

Comments
 (0)