Skip to content

Commit

Permalink
pr responses
Browse files Browse the repository at this point in the history
  • Loading branch information
@killianmuldoon
killianmuldoon committed Nov 23, 2021
1 parent f2fc925 commit dc5bb91
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 11 deletions.
78 changes: 78 additions & 0 deletions internal/topology/check/compatibility_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,45 @@ func TestClusterClassesAreCompatible(t *testing.T) {
Build(),
wantErr: false,
},
{
name: "pass if machineDeploymentClass is removed from ClusterClass",
current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(
builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithControlPlaneTemplate(
refToUnstructured(ref)).
WithControlPlaneInfrastructureMachineTemplate(
refToUnstructured(ref)).
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
WithInfrastructureTemplate(
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithBootstrapTemplate(
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
Build(),
*builder.MachineDeploymentClass("bb").
WithInfrastructureTemplate(
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithBootstrapTemplate(
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
Build()).
Build(),
desired: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(
builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithControlPlaneTemplate(
refToUnstructured(ref)).
WithControlPlaneInfrastructureMachineTemplate(
refToUnstructured(ref)).
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
WithInfrastructureTemplate(
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithBootstrapTemplate(
refToUnstructured(incompatibleRef)).Build()).
Build(),
wantErr: false,
},
}
for _, tt := range tests {
g := NewWithT(t)
Expand Down Expand Up @@ -590,6 +629,45 @@ func TestMachineDeploymentClassesAreCompatible(t *testing.T) {
Build(),
wantErr: false,
},
{
name: "pass if machineDeploymentClass is removed from ClusterClass",
current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(
builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithControlPlaneTemplate(
refToUnstructured(ref)).
WithControlPlaneInfrastructureMachineTemplate(
refToUnstructured(ref)).
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
WithInfrastructureTemplate(
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithBootstrapTemplate(
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
Build(),
*builder.MachineDeploymentClass("bb").
WithInfrastructureTemplate(
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithBootstrapTemplate(
builder.BootstrapTemplate(metav1.NamespaceDefault, "bootstrap1").Build()).
Build()).
Build(),
desired: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(
builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithControlPlaneTemplate(
refToUnstructured(ref)).
WithControlPlaneInfrastructureMachineTemplate(
refToUnstructured(ref)).
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
WithInfrastructureTemplate(
builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").Build()).
WithBootstrapTemplate(
refToUnstructured(incompatibleRef)).Build()).
Build(),
wantErr: false,
},
{
name: "error if machineDeploymentClass has multiple incompatible references",
current: builder.ClusterClass(metav1.NamespaceDefault, "class1").
Expand Down
30 changes: 30 additions & 0 deletions internal/webhooks/cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,36 @@ func TestClusterTopologyValidationForTopologyClassChange(t *testing.T) {
Build(),
wantErr: false,
},
{
name: "Accept cluster.topology.class change with a deleted MachineDeploymentClass",
firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(refToUnstructured(ref)).
WithControlPlaneTemplate(refToUnstructured(ref)).
WithControlPlaneInfrastructureMachineTemplate(refToUnstructured(ref)).
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
WithInfrastructureTemplate(refToUnstructured(ref)).
WithBootstrapTemplate(refToUnstructured(ref)).
Build(),
*builder.MachineDeploymentClass("bb").
WithInfrastructureTemplate(refToUnstructured(ref)).
WithBootstrapTemplate(refToUnstructured(ref)).
Build(),
).
Build(),
secondClass: builder.ClusterClass(metav1.NamespaceDefault, "class2").
WithInfrastructureClusterTemplate(refToUnstructured(ref)).
WithControlPlaneTemplate(refToUnstructured(ref)).
WithControlPlaneInfrastructureMachineTemplate(refToUnstructured(ref)).
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
WithInfrastructureTemplate(refToUnstructured(ref)).
WithBootstrapTemplate(refToUnstructured(ref)).
Build(),
).
Build(),
wantErr: false,
},
{
name: "Accept cluster.topology.class change with an added MachineDeploymentClass",
firstClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
Expand Down
20 changes: 9 additions & 11 deletions internal/webhooks/clusterclass.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ func (webhook *ClusterClass) ValidateDelete(ctx context.Context, obj runtime.Obj
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), clusterClass.Name, err)
}

if len(clusters) != 0 {
if len(clusters) > 0 {
// TODO(killianmuldoon): Improve error here to include the names of some clusters using the clusterClass.
return field.Forbidden(field.NewPath("clusterClass"),
fmt.Sprintf("clusterClass %v can not be deleted as it is still in use by some clusters", clusterClass.Name))
return apierrors.NewForbidden(clusterv1.GroupVersion.WithResource("ClusterClass").GroupResource(), clusterClass.Name,
fmt.Errorf("cannot be deleted. %v clusters still using the ClusterClass", len(clusters)))
}
return nil
}
Expand Down Expand Up @@ -170,26 +170,24 @@ func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(o
return allErrs
}

// Create a set of records of machineDeploymentClass information where the name is a fully qualified name for the
// MachineDeployment topology in the form cluster.Name/machineDeploymentTopology.Name and links to the machineDeploymentClass Name.
// Error if any Cluster using the ClusterClass uses a MachineDeploymentClass that has been removed.
for _, c := range clusters {
for _, machineDeploymentTopology := range c.Spec.Topology.Workers.MachineDeployments {
for i, machineDeploymentTopology := range c.Spec.Topology.Workers.MachineDeployments {
if removedClasses.Has(machineDeploymentTopology.Class) {
// TODO(killianmuldoon): Improve error printing here so large scale changes don't flood the error log e.g. deduplication, only example usages given.
allErrs = append(allErrs, field.Forbidden(field.NewPath(""), fmt.Sprintf("MachineDeploymentClass %v is in use in MachineDeployment %v in Cluster %v. ClusterClass %v modification not allowed",
machineDeploymentTopology.Class, machineDeploymentTopology.Name, c.Name, old.Name),
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("class"),
fmt.Sprintf("MachineDeploymentClass %v is in use in MachineDeploymentTopology %v in Cluster %v. ClusterClass %v modification not allowed",
machineDeploymentTopology.Class, machineDeploymentTopology.Name, c.Name, old.Name),
))
}
}
}

return allErrs
}

func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String {
removedClasses := sets.NewString()

// Ensure no MachineDeploymentClass in use has been removed.
classes := webhook.classNamesFromWorkerClass(new.Spec.Workers)
for _, oldClass := range old.Spec.Workers.MachineDeployments {
if !classes.Has(oldClass.Class) {
Expand All @@ -199,7 +197,7 @@ func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterCl
return removedClasses
}

// classNames returns the set of MachineDeployment class names.
// classNamesFromWorkerClass returns the set of MachineDeployment class names.
func (webhook *ClusterClass) classNamesFromWorkerClass(w clusterv1.WorkersClass) sets.String {
classes := sets.NewString()
for _, class := range w.MachineDeployments {
Expand Down

0 comments on commit dc5bb91

Please sign in to comment.