Validate machineset before reconciling

Even thought the MachineSet type implements Validate() function, it's not called by default. The validation function is responsible for making sure every machine set matchLabels selector is matched with machine template labels. Given the validation is not performed by default, it is possible to create an invalid machineset that causes the machineset controller to start creating machine object one by one without any upper bound. Causing the machine controller to launch as many instances as there is machine objects.
kubernetes-sigs · Jan 14, 2019 · 7ee9873 · 7ee9873
1 parent 633970e
commit 7ee9873
Show file tree

Hide file tree

Showing 2 changed files with 137 additions and 61 deletions.
diff --git a/pkg/controller/machineset/controller.go b/pkg/controller/machineset/controller.go
@@ -150,6 +150,14 @@ func (r *ReconcileMachineSet) Reconcile(request reconcile.Request) (reconcile.Re
 	}
 
 	klog.V(4).Infof("Reconcile machineset %v", machineSet.Name)
+	fmt.Printf("Reconcile machineset %v\n", machineSet.Name)
+
+	if errList := machineSet.Validate(); len(errList) > 0 {
+		err := fmt.Errorf("%q machineset validation failed: %v", machineSet.Name, errList.ToAggregate().Error())
+		klog.Error(err)
+		return reconcile.Result{}, err
+	}
+
 	allMachines := &clusterv1alpha1.MachineList{}
 
 	err = r.Client.List(context.Background(), client.InNamespace(machineSet.Namespace), allMachines)

diff --git a/pkg/controller/machineset/machineset_controller_test.go b/pkg/controller/machineset/machineset_controller_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package machineset
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -36,19 +37,6 @@ var expectedRequest = reconcile.Request{NamespacedName: types.NamespacedName{Nam
 const timeout = time.Second * 5
 
 func TestReconcile(t *testing.T) {
-	replicas := int32(2)
-	instance := &clusterv1alpha1.MachineSet{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: clusterv1alpha1.MachineSetSpec{
-			Replicas: &replicas,
-			Template: clusterv1alpha1.MachineTemplateSpec{
-				Spec: clusterv1alpha1.MachineSpec{
-					Versions: clusterv1alpha1.MachineVersionInfo{Kubelet: "1.10.3"},
-				},
-			},
-		},
-	}
-
 	// Setup the Manager and Controller.  Wrap the Controller Reconcile function so it writes each request to a
 	// channel when it is finished.
 	mgr, err := manager.New(cfg, manager.Options{})
@@ -64,61 +52,141 @@ func TestReconcile(t *testing.T) {
 	}
 	defer close(StartTestManager(mgr, t))
 
-	// Create the MachineSet object and expect Reconcile to be called and the Machines to be created.
-	if err := c.Create(context.TODO(), instance); err != nil {
-		t.Errorf("error creating instance: %v", err)
-	}
-	defer c.Delete(context.TODO(), instance)
-	select {
-	case recv := <-requests:
-		if recv != expectedRequest {
-			t.Error("received request does not match expected request")
-		}
-	case <-time.After(timeout):
-		t.Error("timed out waiting for request")
+	replicas := int32(2)
+	labels := map[string]string{"foo": "bar"}
+
+	testCases := []struct {
+		name            string
+		instance        *clusterv1alpha1.MachineSet
+		expectedRequest reconcile.Request
+		verifyFnc       func()
+	}{
+		{
+			name: "Refuse invalid machineset (with invalid matching labels)",
+			instance: &clusterv1alpha1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "invalidfoo", Namespace: "default"},
+				Spec: clusterv1alpha1.MachineSetSpec{
+					Replicas: &replicas,
+					Selector: metav1.LabelSelector{
+						MatchLabels: map[string]string{"foo": "bar"},
+					},
+					Template: clusterv1alpha1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{"foo": "bar2"},
+						},
+						Spec: clusterv1alpha1.MachineSpec{
+							Versions: clusterv1alpha1.MachineVersionInfo{Kubelet: "1.10.3"},
+						},
+					},
+				},
+			},
+			expectedRequest: reconcile.Request{NamespacedName: types.NamespacedName{Name: "invalidfoo", Namespace: "default"}},
+			verifyFnc: func() {
+				// expecting machineset validation error
+				if _, err := r.Reconcile(reconcile.Request{
+					NamespacedName: types.NamespacedName{Name: "invalidfoo", Namespace: "default"},
+				}); err == nil {
+					t.Errorf("expected validation error did not occur")
+				}
+			},
+		},
+		{
+			name: "Create the MachineSet object and expect Reconcile to be called and the Machines to be created",
+			instance: &clusterv1alpha1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
+				Spec: clusterv1alpha1.MachineSetSpec{
+					Replicas: &replicas,
+					Selector: metav1.LabelSelector{
+						MatchLabels: labels,
+					},
+					Template: clusterv1alpha1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: labels,
+						},
+						Spec: clusterv1alpha1.MachineSpec{
+							Versions: clusterv1alpha1.MachineVersionInfo{Kubelet: "1.10.3"},
+						},
+					},
+				},
+			},
+			expectedRequest: reconcile.Request{NamespacedName: types.NamespacedName{Name: "foo", Namespace: "default"}},
+			// Verify machines are created and recreated after deletion
+			verifyFnc: func() {
+				machines := &clusterv1alpha1.MachineList{}
+
+				// TODO(joshuarubin) there seems to be a race here. If expectInt sleeps
+				// briefly, even 10ms, the number of replicas is 4 and not 2 as expected
+				expectInt(t, int(replicas), func(ctx context.Context) int {
+					if err := c.List(ctx, &client.ListOptions{}, machines); err != nil {
+						return -1
+					}
+					return len(machines.Items)
+				})
+
+				// Verify that each machine has the desired kubelet version.
+				for _, m := range machines.Items {
+					if k := m.Spec.Versions.Kubelet; k != "1.10.3" {
+						t.Errorf("kubelet was %q not '1.10.3'", k)
+					}
+				}
+
+				// Delete a Machine and expect Reconcile to be called to replace it.
+				m := machines.Items[0]
+				if err := c.Delete(context.TODO(), &m); err != nil {
+					t.Errorf("error deleting machine: %v", err)
+				}
+				select {
+				case recv := <-requests:
+					if recv != expectedRequest {
+						t.Error("received request does not match expected request")
+					}
+				case <-time.After(timeout):
+					t.Error("timed out waiting for request")
+				}
+
+				// TODO (robertbailey): Figure out why the control loop isn't working as expected.
+				/*
+					g.Eventually(func() int {
+						if err := c.List(context.TODO(), &client.ListOptions{}, machines); err != nil {
+							return -1
+						}
+						return len(machines.Items)
+					}, timeout).Should(gomega.BeEquivalentTo(replicas))
+				*/
+			},
+		},
 	}
 
-	machines := &clusterv1alpha1.MachineList{}
-
-	// TODO(joshuarubin) there seems to be a race here. If expectInt sleeps
-	// briefly, even 10ms, the number of replicas is 4 and not 2 as expected
-	expectInt(t, int(replicas), func(ctx context.Context) int {
-		if err := c.List(ctx, &client.ListOptions{}, machines); err != nil {
-			return -1
+	for _, tc := range testCases {
+		if err := c.Create(context.TODO(), tc.instance); err != nil {
+			t.Errorf("error creating instance: %v", err)
 		}
-		return len(machines.Items)
-	})
 
-	// Verify that each machine has the desired kubelet version.
-	for _, m := range machines.Items {
-		if k := m.Spec.Versions.Kubelet; k != "1.10.3" {
-			t.Errorf("kubelet was %q not '1.10.3'", k)
-		}
-	}
+		defer func() {
+			c.Delete(context.TODO(), tc.instance)
+			select {
+			case recv := <-requests:
+				fmt.Printf("delete.recv: %#v\n", recv)
+				if recv != tc.expectedRequest {
+					t.Error("received request does not match expected request")
+				}
+			case <-time.After(timeout):
+				t.Error("timed out waiting for request")
+			}
+		}()
 
-	// Delete a Machine and expect Reconcile to be called to replace it.
-	m := machines.Items[0]
-	if err := c.Delete(context.TODO(), &m); err != nil {
-		t.Errorf("error deleting machine: %v", err)
-	}
-	select {
-	case recv := <-requests:
-		if recv != expectedRequest {
-			t.Error("received request does not match expected request")
+		select {
+		case recv := <-requests:
+			fmt.Printf("recv: %#v\n", recv)
+			if recv != tc.expectedRequest {
+				t.Error("received request does not match expected request")
+			}
+		case <-time.After(timeout):
+			t.Error("timed out waiting for request")
 		}
-	case <-time.After(timeout):
-		t.Error("timed out waiting for request")
-	}
 
-	// TODO (robertbailey): Figure out why the control loop isn't working as expected.
-	/*
-		g.Eventually(func() int {
-			if err := c.List(context.TODO(), &client.ListOptions{}, machines); err != nil {
-				return -1
-			}
-			return len(machines.Items)
-		}, timeout).Should(gomega.BeEquivalentTo(replicas))
-	*/
+		tc.verifyFnc()
+	}
 }
 
 func expectInt(t *testing.T, expect int, fn func(context.Context) int) {