Skip to content

Commit deff2da

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

File tree

5 files changed

+53
-34
lines changed

5 files changed

+53
-34
lines changed

‎pkg/model/gcemodel/api_loadbalancer.go‎

Lines changed: 4 additions & 3 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"
@@ -103,7 +104,7 @@ func (b *APILoadBalancerBuilder) addFirewallRules(c *fi.CloudupModelBuilderConte
103104
Network: network,
104105
SourceRanges: 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() {
@@ -114,7 +115,7 @@ func (b *APILoadBalancerBuilder) addFirewallRules(c *fi.CloudupModelBuilderConte
114115
Family: gcetasks.AddressFamilyIPv4, // ip alias is always ipv4
115116
SourceRanges: []string{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

@@ -124,7 +125,7 @@ func (b *APILoadBalancerBuilder) addFirewallRules(c *fi.CloudupModelBuilderConte
124125
Network: network,
125126
SourceRanges: 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: 12 additions & 11 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+
Allowed: sets.New("tcp:22"),
6263
SourceRanges: 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,15 +85,15 @@ 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+
Allowed: sets.New("tcp:22"),
8889
SourceRanges: 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+
Allowed: sets.New("tcp:22"),
9697
SourceRanges: b.Cluster.Spec.SSHAccess,
9798
Network: network,
9899
})
@@ -113,10 +114,10 @@ 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-
},
117+
Allowed: sets.New(
118+
"tcp:"+nodePortRangeString,
119+
"udp:"+nodePortRangeString,
120+
),
120121
SourceRanges: b.Cluster.Spec.NodePortAccess,
121122
Network: network,
122123
})
@@ -136,7 +137,7 @@ 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+
Allowed: sets.New("tcp:443"),
140141
SourceRanges: b.Cluster.Spec.API.Access,
141142
Network: network,
142143
})
@@ -149,7 +150,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.CloudupModelBuilderContext) err
149150
Family: gcetasks.AddressFamilyIPv4, // ip alias is always ipv4
150151
SourceRanges: []string{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: 18 additions & 17 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"
@@ -66,7 +67,7 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
6667
"209.85.152.0/22",
6768
},
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)
@@ -176,7 +177,7 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
176177
Network: network,
177178
SourceRanges: []string{b.Cluster.Spec.Networking.PodCIDR},
178179
TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)},
179-
Allowed: allProtocols,
180+
Allowed: sets.New(allProtocols...),
180181
})
181182
}
182183
}
@@ -230,13 +231,13 @@ func (b *GCEModelContext) AddFirewallRulesTasks(c *fi.CloudupModelBuilderContext
230231
ipv6.SourceRanges = []string{"::/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/cloudup/gcetasks/firewallrule.go‎

Lines changed: 7 additions & 3 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"
@@ -44,7 +45,7 @@ type FirewallRule struct {
4445
SourceTags []string
4546
SourceRanges []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
@@ -79,7 +80,10 @@ func (e *FirewallRule) Find(c *fi.CloudupContext) (*FirewallRule, error) {
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
@@ -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 e.Allowed {
186190
p, err := parseFirewallAllowed(a)
187191
if err != nil {
188192
return nil, err

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