Skip to content

Commit 681ddea

Browse files
killianmuldoonkillianmuldoon
committed
Add webhook warning for missing ClusterClass
Signed-off-by: killianmuldoon <[email protected]>
1 parent 58a7df0 commit 681ddea

File tree

2 files changed

+101
-43
lines changed

2 files changed

+101
-43
lines changed

‎internal/webhooks/cluster.go‎

Lines changed: 40 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ func (webhook *Cluster) ValidateDelete(_ context.Context, _ runtime.Object) (adm
139139

140140
func (webhook *Cluster) validate(ctx context.Context, oldCluster, newCluster *clusterv1.Cluster) (admission.Warnings, error) {
141141
var allErrs field.ErrorList
142+
var allWarnings admission.Warnings
142143
// The Cluster name is used as a label value. This check ensures that names which are not valid label values are rejected.
143144
if errs := validation.IsValidLabelValue(newCluster.Name); len(errs) != 0 {
144145
for _, err := range errs {
@@ -191,7 +192,9 @@ func (webhook *Cluster) validate(ctx context.Context, oldCluster, newCluster *cl
191192

192193
// Validate the managed topology, if defined.
193194
if newCluster.Spec.Topology != nil {
194-
allErrs = append(allErrs, webhook.validateTopology(ctx, oldCluster, newCluster, topologyPath)...)
195+
topologyWarnings, topologyErrs := webhook.validateTopology(ctx, oldCluster, newCluster, topologyPath)
196+
allWarnings = append(allWarnings, topologyWarnings...)
197+
allErrs = append(allErrs, topologyErrs...)
195198
}
196199

197200
// On update.
@@ -206,16 +209,18 @@ func (webhook *Cluster) validate(ctx context.Context, oldCluster, newCluster *cl
206209
}
207210

208211
if len(allErrs) > 0 {
209-
return nil, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), newCluster.Name, allErrs)
212+
return allWarnings, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), newCluster.Name, allErrs)
210213
}
211-
return nil, nil
214+
return allWarnings, nil
212215
}
213216

214-
func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newCluster *clusterv1.Cluster, fldPath *field.Path) field.ErrorList {
217+
func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newCluster *clusterv1.Cluster, fldPath *field.Path) (admission.Warnings, field.ErrorList) {
218+
var allWarnings admission.Warnings
219+
215220
// NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the web hook
216221
// must prevent the usage of Cluster.Topology in case the feature flag is disabled.
217222
if !feature.Gates.Enabled(feature.ClusterTopology) {
218-
return field.ErrorList{
223+
return allWarnings, field.ErrorList{
219224
field.Forbidden(
220225
fldPath,
221226
"can be set only if the ClusterTopology feature flag is enabled",
@@ -234,6 +239,8 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
234239
"class cannot be empty",
235240
),
236241
)
242+
// Return early if there is no defined class to validate.
243+
return allWarnings, allErrs
237244
}
238245

239246
// version should be valid.
@@ -268,18 +275,11 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
268275
}
269276

270277
// Get the ClusterClass referenced in the Cluster.
271-
clusterClass, clusterClassPollErr := webhook.pollClusterClassForCluster(ctx, newCluster)
272-
if clusterClassPollErr != nil &&
273-
// If the error is anything other than "NotFound" or "NotReconciled" return all errors at this point.
274-
!(apierrors.IsNotFound(clusterClassPollErr) || errors.Is(clusterClassPollErr, errClusterClassNotReconciled)) {
275-
allErrs = append(
276-
allErrs, field.InternalError(
277-
fldPath.Child("class"),
278-
clusterClassPollErr))
279-
return allErrs
280-
}
278+
clusterClass, warnings, clusterClassPollErr := webhook.validateClusterClassExistsAndIsReconciled(ctx, newCluster)
279+
allWarnings = append(allWarnings, warnings...)
280+
281+
// If there's no error validate the Cluster based on the ClusterClass.
281282
if clusterClassPollErr == nil {
282-
// If there's no error validate the Cluster based on the ClusterClass.
283283
allErrs = append(allErrs, ValidateClusterForClusterClass(newCluster, clusterClass)...)
284284
}
285285
if oldCluster != nil { // On update
@@ -290,13 +290,13 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
290290
allErrs, field.InternalError(
291291
fldPath.Child("class"),
292292
clusterClassPollErr))
293-
return allErrs
293+
return allWarnings, allErrs
294294
}
295295

296296
// Topology or Class can not be added on update unless ClusterTopologyUnsafeUpdateClassNameAnnotation is set.
297297
if oldCluster.Spec.Topology == nil || oldCluster.Spec.Topology.Class == "" {
298298
if _, ok := newCluster.Annotations[clusterv1.ClusterTopologyUnsafeUpdateClassNameAnnotation]; ok {
299-
return allErrs
299+
return allWarnings, allErrs
300300
}
301301

302302
allErrs = append(
@@ -307,7 +307,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
307307
),
308308
)
309309
// return early here if there is no class to compare.
310-
return allErrs
310+
return allWarnings, allErrs
311311
}
312312

313313
// Version could only be increased.
@@ -372,14 +372,14 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
372372
oldCluster.Spec.Topology.Class, newCluster.Spec.Topology.Class)))
373373

374374
// Return early with errors if the ClusterClass can't be retrieved.
375-
return allErrs
375+
return allWarnings, allErrs
376376
}
377377

378378
// Check if the new and old ClusterClasses are compatible with one another.
379379
allErrs = append(allErrs, check.ClusterClassesAreCompatible(oldClusterClass, clusterClass)...)
380380
}
381381
}
382-
return allErrs
382+
return allWarnings, allErrs
383383
}
384384

385385
func validateMachineHealthChecks(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList {
@@ -551,11 +551,29 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl
551551
return allErrs
552552
}
553553

554+
// validateClusterClassExistsAndIsReconciled will try to get the ClusterClass referenced in the Cluster. If it does not exist or is not reconciled it will add a warning.
555+
// In any other case it will return an error.
556+
func (webhook *Cluster) validateClusterClassExistsAndIsReconciled(ctx context.Context, newCluster *clusterv1.Cluster) (*clusterv1.ClusterClass, admission.Warnings, error) {
557+
var allWarnings admission.Warnings
558+
clusterClass, clusterClassPollErr := webhook.pollClusterClassForCluster(ctx, newCluster)
559+
if clusterClassPollErr != nil {
560+
// Add a warning if the Class does not exist or if it has not been successfully reconciled.
561+
switch {
562+
case apierrors.IsNotFound(clusterClassPollErr):
563+
allWarnings = append(allWarnings,
564+
fmt.Sprintf("Cluster specifies ClusterClass %s in the topology but it does not exist. Cluster variables have not been fully validated. The ClusterClass must be created to reconcile the Cluster", newCluster.Spec.Topology.Class))
565+
case errors.Is(clusterClassPollErr, errClusterClassNotReconciled):
566+
allWarnings = append(allWarnings,
567+
fmt.Sprintf("Cluster specifies ClusterClass %s in `topology.spec.class` but it has not successfully been reconciled. Cluster variables have not been fully validated.", newCluster.Spec.Topology.Class))
568+
}
569+
}
570+
return clusterClass, allWarnings, clusterClassPollErr
571+
}
572+
554573
// pollClusterClassForCluster will retry getting the ClusterClass referenced in the Cluster for two seconds.
555574
func (webhook *Cluster) pollClusterClassForCluster(ctx context.Context, cluster *clusterv1.Cluster) (*clusterv1.ClusterClass, error) {
556575
clusterClass := &clusterv1.ClusterClass{}
557576
var clusterClassPollErr error
558-
// TODO: Add a webhook warning if the ClusterClass is not up to date or not found.
559577
_ = wait.PollUntilContextTimeout(ctx, 200*time.Millisecond, 2*time.Second, true, func(ctx context.Context) (bool, error) {
560578
if clusterClassPollErr = webhook.Client.Get(ctx, client.ObjectKey{Namespace: cluster.Namespace, Name: cluster.Spec.Topology.Class}, clusterClass); clusterClassPollErr != nil {
561579
return false, nil //nolint:nilerr

‎internal/webhooks/cluster_test.go‎

Lines changed: 61 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,7 +1004,8 @@ func TestClusterValidation(t *testing.T) {
10041004
// Create the webhook.
10051005
webhook := &Cluster{}
10061006

1007-
_, err := webhook.validate(ctx, tt.old, tt.in)
1007+
warnings, err := webhook.validate(ctx, tt.old, tt.in)
1008+
g.Expect(warnings).To(BeNil())
10081009
if tt.expectErr {
10091010
g.Expect(err).To(HaveOccurred())
10101011
return
@@ -1268,7 +1269,8 @@ func TestClusterTopologyValidation(t *testing.T) {
12681269
// Create the webhook and add the fakeClient as its client. This is required because the test uses a Managed Topology.
12691270
webhook := &Cluster{Client: fakeClient}
12701271

1271-
_, err := webhook.validate(ctx, tt.old, tt.in)
1272+
warnings, err := webhook.validate(ctx, tt.old, tt.in)
1273+
g.Expect(warnings).To(BeNil())
12721274
if tt.expectErr {
12731275
g.Expect(err).To(HaveOccurred())
12741276
return
@@ -1284,11 +1286,13 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
12841286
g := NewWithT(t)
12851287

12861288
tests := []struct {
1287-
name string
1288-
cluster *clusterv1.Cluster
1289-
class *clusterv1.ClusterClass
1290-
objects []client.Object
1291-
wantErr bool
1289+
name string
1290+
cluster *clusterv1.Cluster
1291+
class *clusterv1.ClusterClass
1292+
classReconciled bool
1293+
objects []client.Object
1294+
wantErr bool
1295+
wantWarnings bool
12921296
}{
12931297
{
12941298
name: "Accept a cluster with an existing ClusterClass named in cluster.spec.topology.class",
@@ -1302,10 +1306,11 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
13021306
Build(),
13031307
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
13041308
Build(),
1305-
wantErr: false,
1309+
classReconciled: true,
1310+
wantErr: false,
13061311
},
13071312
{
1308-
name: "Accept a cluster with non-existent ClusterClass referenced cluster.spec.topology.class",
1313+
name: "Warning for a cluster with non-existent ClusterClass referenced cluster.spec.topology.class",
13091314
cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
13101315
WithTopology(
13111316
builder.ClusterTopology().
@@ -1316,7 +1321,26 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
13161321
Build(),
13171322
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
13181323
Build(),
1319-
wantErr: false,
1324+
// There should be a warning for a ClusterClass which can not be found.
1325+
wantWarnings: true,
1326+
wantErr: false,
1327+
},
1328+
{
1329+
name: "Warning for a cluster with an unreconciled ClusterClass named in cluster.spec.topology.class",
1330+
cluster: builder.Cluster(metav1.NamespaceDefault, "cluster1").
1331+
WithTopology(
1332+
builder.ClusterTopology().
1333+
WithClass("clusterclass").
1334+
WithVersion("v1.22.2").
1335+
WithControlPlaneReplicas(3).
1336+
Build()).
1337+
Build(),
1338+
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
1339+
Build(),
1340+
classReconciled: false,
1341+
// There should be a warning for a ClusterClass which is not yet reconciled.
1342+
wantWarnings: true,
1343+
wantErr: false,
13201344
},
13211345
{
13221346
name: "Reject a cluster that has MHC enabled for control plane but is missing MHC definition in cluster topology and clusterclass",
@@ -1333,7 +1357,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
13331357
Build(),
13341358
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
13351359
Build(),
1336-
wantErr: true,
1360+
classReconciled: true,
1361+
wantErr: true,
13371362
},
13381363
{
13391364
name: "Reject a cluster that MHC override defined for control plane but is missing unhealthy conditions",
@@ -1352,7 +1377,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
13521377
Build(),
13531378
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
13541379
Build(),
1355-
wantErr: true,
1380+
classReconciled: true,
1381+
wantErr: true,
13561382
},
13571383
{
13581384
name: "Reject a cluster that MHC override defined for control plane but is set when control plane is missing machineInfrastructure",
@@ -1376,7 +1402,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
13761402
Build(),
13771403
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
13781404
Build(),
1379-
wantErr: true,
1405+
classReconciled: true,
1406+
wantErr: true,
13801407
},
13811408
{
13821409
name: "Accept a cluster that has MHC enabled for control plane with control plane MHC defined in ClusterClass",
@@ -1394,7 +1421,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
13941421
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
13951422
WithControlPlaneMachineHealthCheck(&clusterv1.MachineHealthCheckClass{}).
13961423
Build(),
1397-
wantErr: false,
1424+
classReconciled: true,
1425+
wantErr: false,
13981426
},
13991427
{
14001428
name: "Accept a cluster that has MHC enabled for control plane with control plane MHC defined in cluster topology",
@@ -1420,7 +1448,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
14201448
class: builder.ClusterClass(metav1.NamespaceDefault, "clusterclass").
14211449
WithControlPlaneInfrastructureMachineTemplate(&unstructured.Unstructured{}).
14221450
Build(),
1423-
wantErr: false,
1451+
classReconciled: true,
1452+
wantErr: false,
14241453
},
14251454
{
14261455
name: "Reject a cluster that has MHC enabled for machine deployment but is missing MHC definition in cluster topology and ClusterClass",
@@ -1445,7 +1474,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
14451474
*builder.MachineDeploymentClass("worker-class").Build(),
14461475
).
14471476
Build(),
1448-
wantErr: true,
1477+
classReconciled: true,
1478+
wantErr: true,
14491479
},
14501480
{
14511481
name: "Reject a cluster that has MHC override defined for machine deployment but is missing unhealthy conditions",
@@ -1472,7 +1502,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
14721502
*builder.MachineDeploymentClass("worker-class").Build(),
14731503
).
14741504
Build(),
1475-
wantErr: true,
1505+
classReconciled: true,
1506+
wantErr: true,
14761507
},
14771508
{
14781509
name: "Accept a cluster that has MHC enabled for machine deployment with machine deployment MHC defined in ClusterClass",
@@ -1499,7 +1530,8 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
14991530
Build(),
15001531
).
15011532
Build(),
1502-
wantErr: false,
1533+
classReconciled: true,
1534+
wantErr: false,
15031535
},
15041536
{
15051537
name: "Accept a cluster that has MHC enabled for machine deployment with machine deployment MHC defined in cluster topology",
@@ -1532,13 +1564,16 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
15321564
*builder.MachineDeploymentClass("worker-class").Build(),
15331565
).
15341566
Build(),
1535-
wantErr: false,
1567+
classReconciled: true,
1568+
wantErr: false,
15361569
},
15371570
}
15381571
for _, tt := range tests {
15391572
t.Run(tt.name, func(t *testing.T) {
15401573
// Mark this condition to true so the webhook sees the ClusterClass as up to date.
1541-
conditions.MarkTrue(tt.class, clusterv1.ClusterClassVariablesReconciledCondition)
1574+
if tt.classReconciled {
1575+
conditions.MarkTrue(tt.class, clusterv1.ClusterClassVariablesReconciledCondition)
1576+
}
15421577
// Sets up the fakeClient for the test case.
15431578
fakeClient := fake.NewClientBuilder().
15441579
WithObjects(tt.class).
@@ -1549,12 +1584,17 @@ func TestClusterTopologyValidationWithClient(t *testing.T) {
15491584
c := &Cluster{Client: fakeClient}
15501585

15511586
// Checks the return error.
1552-
_, err := c.ValidateCreate(ctx, tt.cluster)
1587+
warnings, err := c.ValidateCreate(ctx, tt.cluster)
15531588
if tt.wantErr {
15541589
g.Expect(err).To(HaveOccurred())
15551590
} else {
15561591
g.Expect(err).NotTo(HaveOccurred())
15571592
}
1593+
if tt.wantWarnings {
1594+
g.Expect(warnings).ToNot(BeNil())
1595+
} else {
1596+
g.Expect(warnings).To(BeNil())
1597+
}
15581598
})
15591599
}
15601600
}

0 commit comments

Comments
 (0)