Skip to content

Commit

Permalink
Validate MachineSet and MachineDeployment labels (#739)
Browse files Browse the repository at this point in the history
Signed-off-by: Vince Prignano <[email protected]>
  • Loading branch information
vincepri authored and k8s-ci-robot committed Feb 11, 2019
1 parent a3f503f commit a9f44d0
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 1 deletion.
1 change: 1 addition & 0 deletions cmd/clusterctl/clusterdeployer/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ go_library(
"//cmd/clusterctl/phases:go_default_library",
"//cmd/clusterctl/providercomponents:go_default_library",
"//pkg/apis/cluster/v1alpha1:go_default_library",
"//pkg/util:go_default_library",
"//vendor/github.com/pkg/errors:go_default_library",
"//vendor/k8s.io/client-go/kubernetes:go_default_library",
"//vendor/k8s.io/klog:go_default_library",
Expand Down
2 changes: 1 addition & 1 deletion cmd/clusterctl/clusterdeployer/clusterdeployer.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (d *ClusterDeployer) Create(cluster *clusterv1.Cluster, machines []*cluster
}
}

klog.Info("Creating namespace %q on target cluster", cluster.Namespace)
klog.Infof("Creating namespace %q on target cluster", cluster.Namespace)
addNamespaceToTarget := func() (bool, error) {
err = targetClient.EnsureNamespace(cluster.Namespace)
if err != nil {
Expand Down
11 changes: 11 additions & 0 deletions pkg/controller/machinedeployment/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,17 @@ func (r *ReconcileMachineDeployment) Reconcile(request reconcile.Request) (recon
return reconcile.Result{}, nil
}

// Make sure that label selector can match template's labels.
// TODO(vincepri): Move to a validation (admission) webhook when supported.
selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)
if err != nil {
return reconcile.Result{}, errors.Wrapf(err, "failed to parse MachineDeployment %q label selector", d.Name)
}

if !selector.Matches(labels.Set(d.Spec.Template.Labels)) {
return reconcile.Result{}, errors.Errorf("failed validation on MachineDeployment %q label selector, cannot match any machines ", d.Name)
}

msList, err := r.getMachineSetsForDeployment(d)
if err != nil {
return reconcile.Result{}, err
Expand Down
12 changes: 12 additions & 0 deletions pkg/controller/machineset/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/klog"
clusterv1alpha1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
Expand Down Expand Up @@ -158,6 +159,17 @@ func (r *ReconcileMachineSet) Reconcile(request reconcile.Request) (reconcile.Re
return reconcile.Result{}, errors.Wrap(err, "failed to list machines")
}

// Make sure that label selector can match template's labels.

This comment has been minimized.

Copy link
@enxebre

enxebre Feb 11, 2019

Member

hey @vincepri Wouldn't it make sense for this to go right before the api call to get allMachines?
Also any motivation to prefer this approach over using validate function as per #669 which also included unit tests :)?
We should probably remove Validate() function now since it does not seem to be called anywhere https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/apis/cluster/v1alpha1/machineset_types.go#L139

// TODO(vincepri): Move to a validation (admission) webhook when supported.
selector, err := metav1.LabelSelectorAsSelector(&machineSet.Spec.Selector)
if err != nil {
return reconcile.Result{}, errors.Wrapf(err, "failed to parse MachineSet %q label selector", machineSet.Name)
}

if !selector.Matches(labels.Set(machineSet.Spec.Template.Labels)) {
return reconcile.Result{}, errors.Errorf("failed validation on MachineSet %q label selector, cannot match any machines ", machineSet.Name)
}

// Filter out irrelevant machines (deleting/mismatch labels) and claim orphaned machines.
filteredMachines := make([]*clusterv1alpha1.Machine, 0, len(allMachines.Items))
for idx := range allMachines.Items {
Expand Down

0 comments on commit a9f44d0

Please sign in to comment.