Skip to content

Commit

Permalink
remove getMachineDeployment and address nits
Browse files Browse the repository at this point in the history
  • Loading branch information
@killianmuldoon
killianmuldoon committed Nov 22, 2021
1 parent b546898 commit f2fc925
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 129 deletions.
2 changes: 1 addition & 1 deletion internal/builder/builders.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func Cluster(namespace, name string) *ClusterBuilder {
}
}

// WithLabels sets the labels for the MachineDeploymentClassBuilder.
// WithLabels sets the labels for the ClusterBuilder.
func (c *ClusterBuilder) WithLabels(labels map[string]string) *ClusterBuilder {
c.labels = labels
return c
Expand Down
74 changes: 20 additions & 54 deletions internal/webhooks/clusterclass.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,14 @@ func (webhook *ClusterClass) ValidateDelete(ctx context.Context, obj runtime.Obj
if !ok {
return apierrors.NewBadRequest(fmt.Sprintf("expected a ClusterClass but got a %T", obj))
}
clusters, err := webhook.clustersUsingClusterClass(clusterClass)

clusters, err := webhook.getClustersUsingClusterClass(clusterClass)
if err != nil {
return err.ToAggregate()
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), clusterClass.Name, err)
}

if len(clusters) != 0 {
// TODO: Improve error here to include the names of some clusters using the clusterClass.
// 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))
}
Expand Down Expand Up @@ -141,95 +143,60 @@ func (webhook *ClusterClass) validate(old, new *clusterv1.ClusterClass) error {
allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...)

// Ensure no MachineDeploymentClass currently in use has been removed from the ClusterClass.
allErrs = append(allErrs, webhook.validateLivingMachineDeploymentClassesNotRemoved(old, new)...)
allErrs = append(allErrs, webhook.validateRemovedMachineDeploymentClassesAreNotUsed(old, new)...)

if len(allErrs) > 0 {
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs)
}
return nil
}

func (webhook *ClusterClass) validateLivingMachineDeploymentClassesNotRemoved(old, new *clusterv1.ClusterClass) field.ErrorList {
func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(old, new *clusterv1.ClusterClass) field.ErrorList {
var allErrs field.ErrorList
if old == nil {
return allErrs
}
missingClasses := webhook.removedMachineClasses(old, new)
removedClasses := webhook.removedMachineClasses(old, new)

// If no classes have been removed return early as no further checks are needed.
if len(missingClasses) == 0 {
if len(removedClasses) == 0 {
return allErrs
}

// TODO: The first part of this method will need to be broken out to properly account for changes when also validating variables.
// Retrieve all clusters using the ClusterClass.
clusters, errs := webhook.clustersUsingClusterClass(old)
if len(errs) > 0 {
allErrs = append(allErrs, errs...)
return allErrs
}

// Retrieve all the MachineDeployments in the clusterClass namespace using a managed topology.
machineDeployments, errs := webhook.topologyManagedMachineDeployments(new.Namespace)
clusters, errs := webhook.getClustersUsingClusterClass(old)
if len(errs) > 0 {
allErrs = append(allErrs, errs...)
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.
mdcRecords := map[string]string{}
for _, c := range clusters {
for _, machineDeploymentClass := range c.Spec.Topology.Workers.MachineDeployments {
if missingClasses.Has(machineDeploymentClass.Class) {
mdcRecords[fmt.Sprintf("%v/%v", c.Name, machineDeploymentClass.Name)] = machineDeploymentClass.Class
for _, 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),
))
}
}
}

// For each machineDeployment using a managed topology return an error if it is using a class that has been removed in the ClusterClass change.
for _, md := range machineDeployments {
mdName := fmt.Sprintf("%v/%v", md.Labels[clusterv1.ClusterLabelName], md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName])
if _, ok := mdcRecords[mdName]; ok {
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",
mdcRecords[mdName], md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName], md.Labels[clusterv1.ClusterLabelName], old.Name),
))
}
}

return allErrs
}

func (webhook *ClusterClass) topologyManagedMachineDeployments(namespace string) ([]clusterv1.MachineDeployment, field.ErrorList) {
var allErrs field.ErrorList

// List all the MachineDeployments in the current cluster using a managed topology.
machineDeployments := &clusterv1.MachineDeploymentList{}
err := webhook.Client.List(context.Background(), machineDeployments,
client.MatchingLabels{
clusterv1.ClusterTopologyOwnedLabel: "",
},
// TODO: Should this be namespaced in future?
client.InNamespace(namespace),
)
if err != nil {
allErrs = append(allErrs, field.InternalError(field.NewPath(""), err))
return nil, allErrs
}
return machineDeployments.Items, nil
}

func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String {
missingClasses := sets.NewString()
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) {
missingClasses.Insert(oldClass.Class)
removedClasses.Insert(oldClass.Class)
}
}
return missingClasses
return removedClasses
}

// classNames returns the set of MachineDeployment class names.
Expand All @@ -241,15 +208,14 @@ func (webhook *ClusterClass) classNamesFromWorkerClass(w clusterv1.WorkersClass)
return classes
}

func (webhook *ClusterClass) clustersUsingClusterClass(clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, field.ErrorList) {
func (webhook *ClusterClass) getClustersUsingClusterClass(clusterClass *clusterv1.ClusterClass) ([]clusterv1.Cluster, field.ErrorList) {
var allErrs field.ErrorList
clusters := &clusterv1.ClusterList{}
clustersUsingClusterClass := []clusterv1.Cluster{}
err := webhook.Client.List(context.Background(), clusters,
client.MatchingLabels{
clusterv1.ClusterTopologyOwnedLabel: "",
},
//TODO: This will be an issue when doing clusterClass across namespaces.
client.InNamespace(clusterClass.Namespace),
)
if err != nil {
Expand Down
76 changes: 2 additions & 74 deletions internal/webhooks/clusterclass_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1049,15 +1049,6 @@ func TestClusterClass_ValidateMachineDeploymentClassChanges(t *testing.T) {
Build()).
Build(),
},
machineDeployments: []client.Object{
builder.MachineDeployment(metav1.NamespaceDefault, "md1").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster1",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
},
oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
Expand Down Expand Up @@ -1088,15 +1079,6 @@ func TestClusterClass_ValidateMachineDeploymentClassChanges(t *testing.T) {
Build()).
Build(),
},
machineDeployments: []client.Object{
builder.MachineDeployment(metav1.NamespaceDefault, "md1").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster1",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
},
oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
Expand Down Expand Up @@ -1140,22 +1122,6 @@ func TestClusterClass_ValidateMachineDeploymentClassChanges(t *testing.T) {
Build()).
Build(),
},
machineDeployments: []client.Object{
builder.MachineDeployment(metav1.NamespaceDefault, "md1").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster1",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
builder.MachineDeployment(metav1.NamespaceDefault, "md2").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster2",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
},
oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
Expand Down Expand Up @@ -1220,44 +1186,6 @@ func TestClusterClass_ValidateMachineDeploymentClassChanges(t *testing.T) {
Build()).
Build(),
},
machineDeployments: []client.Object{
builder.MachineDeployment(metav1.NamespaceDefault, "md1").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster1",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
builder.MachineDeployment(metav1.NamespaceDefault, "md2").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster1",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers2"},
).
Build(),

builder.MachineDeployment(metav1.NamespaceDefault, "md3").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster2",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
builder.MachineDeployment(metav1.NamespaceDefault, "md4").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster2",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers2"},
).
Build(),
builder.MachineDeployment(metav1.NamespaceDefault, "md5").
WithLabels(map[string]string{
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterLabelName: "cluster3",
clusterv1.ClusterTopologyMachineDeploymentLabelName: "workers1"},
).
Build(),
},
oldClusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithWorkerMachineDeploymentClasses(
*builder.MachineDeploymentClass("aa").
Expand Down Expand Up @@ -1288,9 +1216,9 @@ func TestClusterClass_ValidateMachineDeploymentClassChanges(t *testing.T) {
webhook := &ClusterClass{Client: fakeClient}

if tt.expectErr {
g.Expect(webhook.validateLivingMachineDeploymentClassesNotRemoved(tt.oldClusterClass, tt.newClusterClass).ToAggregate()).NotTo(Succeed())
g.Expect(webhook.validateRemovedMachineDeploymentClassesAreNotUsed(tt.oldClusterClass, tt.newClusterClass).ToAggregate()).NotTo(Succeed())
} else {
g.Expect(webhook.validateLivingMachineDeploymentClassesNotRemoved(tt.oldClusterClass, tt.newClusterClass).ToAggregate()).To(Succeed())
g.Expect(webhook.validateRemovedMachineDeploymentClassesAreNotUsed(tt.oldClusterClass, tt.newClusterClass).ToAggregate()).To(Succeed())
}
})
}
Expand Down

0 comments on commit f2fc925

Please sign in to comment.