Skip to content

Commit e9faacc

Browse files
committed
Switching from comparison on Role to helper.
Preperation for something like #18495. Moving away from direct comparison (== or !=) on IG role. Using helper methods such as HasNode() or HasControlPlane(). Also added a hack test so we don't backtrack. Should help prepare for supporting more control plane roles.
1 parent 01499f4 commit e9faacc

48 files changed

Lines changed: 229 additions & 150 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

Makefile

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ govet:
339339

340340
# verify is ran by the pull-kops-verify prow job
341341
.PHONY: verify
342-
verify: quick-ci verify-gofmt
342+
verify: quick-ci verify-gofmt verify-ig-role-comparisons
343343

344344
.PHONY: verify-boilerplate
345345
verify-boilerplate:
@@ -386,10 +386,14 @@ verify-terraform:
386386
verify-hashes:
387387
hack/verify-hashes.sh
388388

389+
.PHONY: verify-ig-role-comparisons
390+
verify-ig-role-comparisons:
391+
hack/verify-ig-role-comparisons.sh
392+
389393
# ci target is for developers, it aims to cover all the CI jobs
390394
# verify-gendocs will call kops target
391395
.PHONY: ci
392-
ci: govet verify-gofmt verify-crds verify-gomod verify-goimports verify-boilerplate verify-versions verify-misspelling verify-shellcheck verify-golangci-lint verify-terraform nodeup examples test | verify-gendocs verify-apimachinery verify-codegen
396+
ci: govet verify-gofmt verify-ig-role-comparisons verify-crds verify-gomod verify-goimports verify-boilerplate verify-versions verify-misspelling verify-shellcheck verify-golangci-lint verify-terraform nodeup examples test | verify-gendocs verify-apimachinery verify-codegen
393397
echo "Done!"
394398

395399
# we skip tasks that are covered by other jobs

cmd/kops/create_cluster.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -585,10 +585,10 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr
585585
var controlPlanes []*api.InstanceGroup
586586
var nodes []*api.InstanceGroup
587587
for _, ig := range instanceGroups {
588-
switch ig.Spec.Role {
589-
case api.InstanceGroupRoleControlPlane:
588+
switch {
589+
case ig.Spec.Role.HasControlPlane():
590590
controlPlanes = append(controlPlanes, ig)
591-
case api.InstanceGroupRoleNode:
591+
case ig.Spec.Role.HasNode():
592592
nodes = append(nodes, ig)
593593
}
594594
}

cmd/kops/create_instancegroup.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ func NewCmdCreateInstanceGroup(f *util.Factory, out io.Writer) *cobra.Command {
126126

127127
allRoles := make([]string, 0, len(kopsapi.AllInstanceGroupRoles))
128128
for _, r := range kopsapi.AllInstanceGroupRoles {
129-
if r == kopsapi.InstanceGroupRoleAPIServer && !featureflag.APIServerNodes.Enabled() {
129+
if r.HasAPIServer() && !featureflag.APIServerNodes.Enabled() {
130130
continue
131131
}
132132
allRoles = append(allRoles, r.ToLowerString())

cmd/kops/delete_instancegroup.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,15 +147,15 @@ func RunDeleteInstanceGroup(ctx context.Context, f *util.Factory, out io.Writer,
147147

148148
fmt.Fprintf(out, "InstanceGroup %q found for deletion\n", groupName)
149149

150-
if group.Spec.Role == kops.InstanceGroupRoleControlPlane {
150+
if group.Spec.Role.HasControlPlane() {
151151
groups, err := clientset.InstanceGroupsFor(cluster).List(ctx, metav1.ListOptions{})
152152
if err != nil {
153153
return fmt.Errorf("listing InstanceGroups: %v", err)
154154
}
155155

156156
onlyMaster := true
157157
for _, ig := range groups.Items {
158-
if ig.Name != groupName && ig.Spec.Role == kops.InstanceGroupRoleControlPlane {
158+
if ig.Name != groupName && ig.Spec.Role.HasControlPlane() {
159159
onlyMaster = false
160160
break
161161
}

cmd/kops/reconcile_cluster.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ func RunReconcileCluster(ctx context.Context, f *util.Factory, out io.Writer, op
160160

161161
// filter the instance group to only include the control plane
162162
opt.filterInstanceGroups = func(ig *kops.InstanceGroup) bool {
163-
return ig.Spec.Role == kops.InstanceGroupRoleAPIServer || ig.Spec.Role == kops.InstanceGroupRoleControlPlane
163+
return ig.Spec.Role.HasAPIServer() || ig.Spec.Role.HasControlPlane()
164164
}
165165

166166
// Ignore all pods, we just want to check the control plane is responding

cmd/kops/toolbox_instance-selector_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ func TestValidateUserSubnetsWithClusterSubnets(t *testing.T) {
143143
func TestCreateInstanceGroup(t *testing.T) {
144144
zones := []string{"us-east-2a", "us-east-2b", "us-east-2c"}
145145
actualIG := createInstanceGroup("testGroup", "clusterTest", zones)
146-
if actualIG.Spec.Role != kops.InstanceGroupRoleNode {
146+
if !actualIG.Spec.Role.HasNode() {
147147
t.Fatalf("instance group should have the \"%s\" role but got %s", kops.InstanceGroupRoleNode, actualIG.Spec.Role)
148148
}
149149
if !reflect.DeepEqual(actualIG.Spec.Subnets, zones) {

cmd/kops/update_cluster.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ func parseLifecycle(lifecycle string) (fi.Lifecycle, error) {
510510

511511
func usesBastion(instanceGroups []*kops.InstanceGroup) bool {
512512
for _, ig := range instanceGroups {
513-
if ig.Spec.Role == kops.InstanceGroupRoleBastion {
513+
if ig.Spec.Role.HasBastion() {
514514
return true
515515
}
516516
}

hack/verify-ig-role-comparisons.sh

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
#!/usr/bin/env bash
2+
3+
# Copyright 2026 The Kubernetes Authors.
4+
#
5+
# Licensed under the Apache License, Version 2.0 (the "License");
6+
# you may not use this file except in compliance with the License.
7+
# You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing, software
12+
# distributed under the License is distributed on an "AS IS" BASIS,
13+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
# See the License for the specific language governing permissions and
15+
# limitations under the License.
16+
17+
set -o errexit
18+
set -o nounset
19+
set -o pipefail
20+
21+
KOPS_ROOT=$(git rev-parse --show-toplevel)
22+
cd "${KOPS_ROOT}"
23+
24+
errors=0
25+
26+
# Find all .go files, excluding vendor, .build, and the file where roles are defined.
27+
# We also exclude pkg/apis/kops/instancegroup.go
28+
files=$(find . -name "*.go" -not -path "./vendor/*" -not -path "./.build/*" -not -path "./pkg/apis/kops/instancegroup.go")
29+
30+
# Regex to match == or != with InstanceGroupRole constants
31+
# We check for constants with or without package prefixes (e.g. kops., api., unversioned.)
32+
# We match both:
33+
# variable == constant
34+
# constant == variable
35+
# variable != constant
36+
# constant != variable
37+
REGEX='==[[:space:]]*([a-zA-Z0-9_]+\.)?InstanceGroupRole(ControlPlane|Node|Bastion|APIServer)\b|\b([a-zA-Z0-9_]+\.)?InstanceGroupRole(ControlPlane|Node|Bastion|APIServer)[[:space:]]*==|!=[[:space:]]*([a-zA-Z0-9_]+\.)?InstanceGroupRole(ControlPlane|Node|Bastion|APIServer)\b|\b([a-zA-Z0-9_]+\.)?InstanceGroupRole(ControlPlane|Node|Bastion|APIServer)[[:space:]]*!='
38+
39+
for file in $files; do
40+
if grep -E "${REGEX}" "${file}" > /dev/null; then
41+
echo "Verification failed in ${file}: direct comparison with InstanceGroupRole constant found:"
42+
grep -n -E "${REGEX}" "${file}"
43+
errors=$((errors + 1))
44+
fi
45+
done
46+
47+
if [ "${errors}" -ne 0 ]; then
48+
echo "Error: Found ${errors} files with direct InstanceGroupRole comparisons (== or !=). Use HasControlPlane(), HasNode(), HasBastion(), or HasAPIServer() instead."
49+
exit 1
50+
fi
51+
52+
echo "InstanceGroupRole comparison verification passed."
53+
exit 0

nodeup/pkg/model/context.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,11 @@ func (c *NodeupModelContext) Init() error {
100100

101101
role := c.BootConfig.InstanceGroupRole
102102

103-
if role == kops.InstanceGroupRoleControlPlane {
103+
if role.HasControlPlane() {
104104
c.IsMaster = true
105105
}
106106

107-
if role == kops.InstanceGroupRoleControlPlane || role == kops.InstanceGroupRoleAPIServer {
107+
if role.HasControlPlane() || role.HasAPIServer() {
108108
c.HasAPIServer = true
109109
}
110110

@@ -566,7 +566,7 @@ func (c *NodeupModelContext) InstallNvidiaRuntime() bool {
566566
// InstallGVisorRuntime returns true if the gVisor (runsc) runtime should be installed.
567567
func (c *NodeupModelContext) InstallGVisorRuntime() bool {
568568
return c.BootConfig != nil &&
569-
c.BootConfig.InstanceGroupRole == kops.InstanceGroupRoleNode &&
569+
c.BootConfig.InstanceGroupRole.HasNode() &&
570570
c.NodeupConfig.GVisor != nil &&
571571
fi.ValueOf(c.NodeupConfig.GVisor.Enabled)
572572
}

pkg/apis/kops/cluster.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1179,7 +1179,7 @@ func (in *WarmPoolSpec) IsEnabled() bool {
11791179
func (in *WarmPoolSpec) ResolveDefaults(ig *InstanceGroup) *WarmPoolSpec {
11801180
igWarmPool := ig.Spec.WarmPool
11811181
if igWarmPool == nil {
1182-
if in == nil || (ig.Spec.Role == InstanceGroupRoleControlPlane || ig.Spec.Role == InstanceGroupRoleBastion) {
1182+
if in == nil || (ig.Spec.Role.HasControlPlane() || ig.Spec.Role.HasBastion()) {
11831183
var zero int64
11841184
return &WarmPoolSpec{
11851185
MaxSize: &zero,
@@ -1188,7 +1188,7 @@ func (in *WarmPoolSpec) ResolveDefaults(ig *InstanceGroup) *WarmPoolSpec {
11881188
return in
11891189
}
11901190

1191-
if in == nil || (ig.Spec.Role == InstanceGroupRoleControlPlane || ig.Spec.Role == InstanceGroupRoleBastion) {
1191+
if in == nil || (ig.Spec.Role.HasControlPlane() || ig.Spec.Role.HasBastion()) {
11921192
return igWarmPool
11931193
}
11941194

0 commit comments

Comments
 (0)