Skip to content

Commit cf849bb

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#5810 from killianmuldoon/webhook/cluster-index
🌱 Use ClusterClass name index in ClusterClass webhook
2 parents 013ff86 + 7726885 commit cf849bb

File tree

7 files changed

+176
-288
lines changed

7 files changed

+176
-288
lines changed

‎internal/controllers/clusterclass/clusterclass_controller_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,14 @@ import (
2525
. "github.com/onsi/gomega"
2626
corev1 "k8s.io/api/core/v1"
2727
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
28-
utilfeature "k8s.io/component-base/featuregate/testing"
2928
"sigs.k8s.io/controller-runtime/pkg/client"
3029

3130
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
32-
"sigs.k8s.io/cluster-api/feature"
3331
tlog "sigs.k8s.io/cluster-api/internal/log"
3432
"sigs.k8s.io/cluster-api/internal/test/builder"
3533
)
3634

3735
func TestClusterClassReconciler_reconcile(t *testing.T) {
38-
defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)()
3936
g := NewWithT(t)
4037
timeout := 30 * time.Second
4138

‎internal/controllers/clusterclass/suite_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,14 @@ import (
3030
"k8s.io/apimachinery/pkg/runtime"
3131
"k8s.io/apimachinery/pkg/runtime/schema"
3232
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
33+
"k8s.io/component-base/featuregate"
3334
ctrl "sigs.k8s.io/controller-runtime"
3435
"sigs.k8s.io/controller-runtime/pkg/client"
3536
"sigs.k8s.io/controller-runtime/pkg/controller"
3637

3738
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3839
"sigs.k8s.io/cluster-api/api/v1beta1/index"
40+
"sigs.k8s.io/cluster-api/feature"
3941
"sigs.k8s.io/cluster-api/internal/test/envtest"
4042
)
4143

@@ -51,6 +53,9 @@ func init() {
5153
_ = apiextensionsv1.AddToScheme(fakeScheme)
5254
}
5355
func TestMain(m *testing.M) {
56+
if err := feature.Gates.(featuregate.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", feature.ClusterTopology, true)); err != nil {
57+
panic(fmt.Sprintf("unable to set ClusterTopology feature gate: %v", err))
58+
}
5459
setupIndexes := func(ctx context.Context, mgr ctrl.Manager) {
5560
if err := index.AddDefaultIndexes(ctx, mgr); err != nil {
5661
panic(fmt.Sprintf("unable to setup index: %v", err))

‎internal/controllers/topology/cluster/suite_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,11 @@ func TestMain(m *testing.M) {
5353
if err := index.AddDefaultIndexes(ctx, mgr); err != nil {
5454
panic(fmt.Sprintf("unable to setup index: %v", err))
5555
}
56+
// Set up the ClusterClassName index explicitly here. This index is ordinarily created in
57+
// index.AddDefaultIndexes. That doesn't happen here because the ClusterClass feature flag is not set.
58+
if err := index.ByClusterClassName(ctx, mgr); err != nil {
59+
panic(fmt.Sprintf("unable to setup index: %v", err))
60+
}
5661
}
5762
setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
5863
unstructuredCachingClient, err := client.NewDelegatingClient(

‎internal/webhooks/clusterclass.go‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
"sigs.k8s.io/controller-runtime/pkg/webhook"
3333

3434
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
35+
"sigs.k8s.io/cluster-api/api/v1beta1/index"
3536
"sigs.k8s.io/cluster-api/feature"
3637
"sigs.k8s.io/cluster-api/internal/topology/check"
3738
"sigs.k8s.io/cluster-api/internal/topology/variables"
@@ -440,22 +441,14 @@ func (webhook *ClusterClass) classNamesFromWorkerClass(w clusterv1.WorkersClass)
440441

441442
func (webhook *ClusterClass) getClustersUsingClusterClass(ctx context.Context, clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, error) {
442443
clusters := &clusterv1.ClusterList{}
443-
clustersUsingClusterClass := []clusterv1.Cluster{}
444444
err := webhook.Client.List(ctx, clusters,
445-
client.MatchingLabels{
446-
clusterv1.ClusterTopologyOwnedLabel: "",
447-
},
445+
client.MatchingFields{index.ClusterClassNameField: clusterClass.Name},
448446
client.InNamespace(clusterClass.Namespace),
449447
)
450448
if err != nil {
451449
return nil, err
452450
}
453-
for _, c := range clusters.Items {
454-
if c.Spec.Topology.Class == clusterClass.Name {
455-
clustersUsingClusterClass = append(clustersUsingClusterClass, c)
456-
}
457-
}
458-
return clustersUsingClusterClass, nil
451+
return clusters.Items, nil
459452
}
460453

461454
func getClusterClassVariablesMapWithReverseIndex(clusterClassVariables []clusterv1.ClusterClassVariable) (map[string]*clusterv1.ClusterClassVariable, map[string]int) {

‎internal/webhooks/clusterclass_test.go‎

Lines changed: 19 additions & 211 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,7 +1148,7 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
11481148
expectErr bool
11491149
}{
11501150
{
1151-
name: "error if a MachineDeploymentClass in use gets removed",
1151+
name: "pass if a MachineDeploymentClass not in use gets removed",
11521152
clusters: []client.Object{
11531153
builder.Cluster(metav1.NamespaceDefault, "cluster1").
11541154
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
@@ -1190,53 +1190,19 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
11901190
builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1").
11911191
Build()).
11921192
WithWorkerMachineDeploymentClasses(
1193-
*builder.MachineDeploymentClass("aa").
1193+
*builder.MachineDeploymentClass("bb").
11941194
WithInfrastructureTemplate(
11951195
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
11961196
WithBootstrapTemplate(
11971197
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
11981198
Build()).
11991199
Build(),
1200-
expectErr: true,
1200+
expectErr: false,
12011201
},
12021202
{
1203-
name: "error if many MachineDeploymentClasses, used in multiple Clusters using the modified ClusterClass, are removed",
1203+
name: "error if a MachineDeploymentClass in use gets removed",
12041204
clusters: []client.Object{
12051205
builder.Cluster(metav1.NamespaceDefault, "cluster1").
1206-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
1207-
WithTopology(
1208-
builder.ClusterTopology().
1209-
WithClass("class1").
1210-
WithMachineDeployment(
1211-
builder.MachineDeploymentTopology("workers1").
1212-
WithClass("bb").
1213-
Build(),
1214-
).
1215-
WithMachineDeployment(
1216-
builder.MachineDeploymentTopology("workers2").
1217-
WithClass("aa").
1218-
Build(),
1219-
).
1220-
Build()).
1221-
Build(),
1222-
builder.Cluster(metav1.NamespaceDefault, "cluster2").
1223-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
1224-
WithTopology(
1225-
builder.ClusterTopology().
1226-
WithClass("class1").
1227-
WithMachineDeployment(
1228-
builder.MachineDeploymentTopology("workers1").
1229-
WithClass("aa").
1230-
Build(),
1231-
).
1232-
WithMachineDeployment(
1233-
builder.MachineDeploymentTopology("workers2").
1234-
WithClass("aa").
1235-
Build(),
1236-
).
1237-
Build()).
1238-
Build(),
1239-
builder.Cluster(metav1.NamespaceDefault, "cluster3").
12401206
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
12411207
WithTopology(
12421208
builder.ClusterTopology().
@@ -1276,7 +1242,7 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
12761242
builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1").
12771243
Build()).
12781244
WithWorkerMachineDeploymentClasses(
1279-
*builder.MachineDeploymentClass("bb").
1245+
*builder.MachineDeploymentClass("aa").
12801246
WithInfrastructureTemplate(
12811247
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
12821248
WithBootstrapTemplate(
@@ -1286,7 +1252,7 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
12861252
expectErr: true,
12871253
},
12881254
{
1289-
name: "pass if a similar MachineDeploymentClass is deleted when it is only used in Clusters not belonging to the ClusterClass",
1255+
name: "error if many MachineDeploymentClasses, used in multiple Clusters using the modified ClusterClass, are removed",
12901256
clusters: []client.Object{
12911257
builder.Cluster(metav1.NamespaceDefault, "cluster1").
12921258
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
@@ -1298,64 +1264,31 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
12981264
WithClass("bb").
12991265
Build(),
13001266
).
1267+
WithMachineDeployment(
1268+
builder.MachineDeploymentTopology("workers2").
1269+
WithClass("aa").
1270+
Build(),
1271+
).
13011272
Build()).
13021273
Build(),
13031274
builder.Cluster(metav1.NamespaceDefault, "cluster2").
13041275
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
13051276
WithTopology(
13061277
builder.ClusterTopology().
1307-
WithClass("class2").
1278+
WithClass("class1").
13081279
WithMachineDeployment(
13091280
builder.MachineDeploymentTopology("workers1").
1310-
1311-
// A MachineDeploymentClass with the same name is in ClusterClass "class1" but
1312-
// this cluster is based on ClusterClass "class2" and does not impact deletion.
1281+
WithClass("aa").
1282+
Build(),
1283+
).
1284+
WithMachineDeployment(
1285+
builder.MachineDeploymentTopology("workers2").
13131286
WithClass("aa").
13141287
Build(),
13151288
).
13161289
Build()).
13171290
Build(),
1318-
},
1319-
oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
1320-
WithInfrastructureClusterTemplate(
1321-
builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()).
1322-
WithControlPlaneTemplate(
1323-
builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1").
1324-
Build()).
1325-
WithWorkerMachineDeploymentClasses(
1326-
*builder.MachineDeploymentClass("aa").
1327-
WithInfrastructureTemplate(
1328-
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
1329-
WithBootstrapTemplate(
1330-
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
1331-
Build(),
1332-
*builder.MachineDeploymentClass("bb").
1333-
WithInfrastructureTemplate(
1334-
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
1335-
WithBootstrapTemplate(
1336-
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
1337-
Build()).
1338-
Build(),
1339-
newClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
1340-
WithInfrastructureClusterTemplate(
1341-
builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "inf").Build()).
1342-
WithControlPlaneTemplate(
1343-
builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1").
1344-
Build()).
1345-
WithWorkerMachineDeploymentClasses(
1346-
*builder.MachineDeploymentClass("bb").
1347-
WithInfrastructureTemplate(
1348-
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
1349-
WithBootstrapTemplate(
1350-
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
1351-
Build()).
1352-
Build(),
1353-
expectErr: false,
1354-
},
1355-
{
1356-
name: "pass if a MachineDeploymentClass not in use gets removed",
1357-
clusters: []client.Object{
1358-
builder.Cluster(metav1.NamespaceDefault, "cluster1").
1291+
builder.Cluster(metav1.NamespaceDefault, "cluster3").
13591292
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
13601293
WithTopology(
13611294
builder.ClusterTopology().
@@ -1402,7 +1335,7 @@ func TestClusterClassValidationWithClusterAwareChecks(t *testing.T) {
14021335
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
14031336
Build()).
14041337
Build(),
1405-
expectErr: false,
1338+
expectErr: true,
14061339
},
14071340
}
14081341

@@ -2450,128 +2383,3 @@ func TestClusterClassValidationWithVariableChecks(t *testing.T) {
24502383
})
24512384
}
24522385
}
2453-
2454-
func TestClusterClass_ValidateDelete(t *testing.T) {
2455-
class := builder.ClusterClass(metav1.NamespaceDefault, "class1").Build()
2456-
2457-
tests := []struct {
2458-
name string
2459-
clusters []client.Object
2460-
expectErr bool
2461-
}{
2462-
{
2463-
name: "allow deletion if a cluster exists but does not reference the ClusterClass for deletion",
2464-
clusters: []client.Object{
2465-
builder.Cluster(metav1.NamespaceDefault, "cluster1").
2466-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2467-
WithTopology(
2468-
builder.ClusterTopology().
2469-
WithClass("class2").
2470-
Build()).
2471-
Build(),
2472-
},
2473-
expectErr: false,
2474-
},
2475-
{
2476-
name: "error if cluster exists with a reference to the ClusterClass for deletion",
2477-
clusters: []client.Object{
2478-
builder.Cluster(metav1.NamespaceDefault, "cluster1").
2479-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2480-
WithTopology(
2481-
builder.ClusterTopology().
2482-
WithClass("class1").
2483-
Build()).
2484-
Build(),
2485-
},
2486-
expectErr: true,
2487-
},
2488-
{
2489-
name: "error if multiple clusters exist and at least one references to the ClusterClass for deletion",
2490-
clusters: []client.Object{
2491-
builder.Cluster(metav1.NamespaceDefault, "cluster1").
2492-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2493-
WithTopology(
2494-
builder.ClusterTopology().
2495-
WithClass("class1").
2496-
Build()).
2497-
Build(),
2498-
builder.Cluster(metav1.NamespaceDefault, "cluster2").
2499-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2500-
WithTopology(
2501-
builder.ClusterTopology().
2502-
WithClass("class2").
2503-
Build()).
2504-
Build(),
2505-
builder.Cluster(metav1.NamespaceDefault, "cluster3").
2506-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2507-
WithTopology(
2508-
builder.ClusterTopology().
2509-
WithClass("class3").
2510-
Build()).
2511-
Build(),
2512-
builder.Cluster(metav1.NamespaceDefault, "cluster4").
2513-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2514-
WithTopology(
2515-
builder.ClusterTopology().
2516-
WithClass("class4").
2517-
Build()).
2518-
Build(),
2519-
},
2520-
expectErr: true,
2521-
},
2522-
{
2523-
name: "allow deletion if multiple clusters exist and none of them references to the ClusterClass for deletion",
2524-
clusters: []client.Object{
2525-
builder.Cluster(metav1.NamespaceDefault, "cluster1").
2526-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2527-
WithTopology(
2528-
builder.ClusterTopology().
2529-
WithClass("class5").
2530-
Build()).
2531-
Build(),
2532-
builder.Cluster(metav1.NamespaceDefault, "cluster2").
2533-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2534-
WithTopology(
2535-
builder.ClusterTopology().
2536-
WithClass("class2").
2537-
Build()).
2538-
Build(),
2539-
builder.Cluster(metav1.NamespaceDefault, "cluster3").
2540-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2541-
WithTopology(
2542-
builder.ClusterTopology().
2543-
WithClass("class3").
2544-
Build()).
2545-
Build(),
2546-
builder.Cluster(metav1.NamespaceDefault, "cluster4").
2547-
WithLabels(map[string]string{clusterv1.ClusterTopologyOwnedLabel: ""}).
2548-
WithTopology(
2549-
builder.ClusterTopology().
2550-
WithClass("class4").
2551-
Build()).
2552-
Build(),
2553-
},
2554-
expectErr: false,
2555-
},
2556-
}
2557-
2558-
for _, tt := range tests {
2559-
t.Run(tt.name, func(t *testing.T) {
2560-
g := NewWithT(t)
2561-
// Sets up the fakeClient for the test case.
2562-
fakeClient := fake.NewClientBuilder().
2563-
WithObjects(tt.clusters...).
2564-
WithScheme(fakeScheme).
2565-
Build()
2566-
2567-
// Create the webhook and add the fakeClient as its client.
2568-
webhook := &ClusterClass{Client: fakeClient}
2569-
err := webhook.ValidateDelete(ctx, class)
2570-
if tt.expectErr {
2571-
g.Expect(err).To(HaveOccurred())
2572-
return
2573-
}
2574-
g.Expect(err).ToNot(HaveOccurred())
2575-
})
2576-
}
2577-
}

0 commit comments

Comments
 (0)