Don't compare interface values in dynamic config controller

This previously caused a panic when moving lastKnownGood between two
non-nil values, because we were comparing the interface wrapper instead
of comparing the NodeConfigSources. The case of moving from one non-nil
lastKnownGood config to another doesn't appear to be tested by the e2e
node tests. I added a unit test and an e2e node test to help catch bugs
with this case in the future.

pkg/kubelet/kubeletconfig/BUILD

@@ -1,9 +1,6 @@
 package(default_visibility = ["//visibility:public"])
 
-load(
-    "@io_bazel_rules_go//go:def.bzl",
-    "go_library",
-)
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -60,3 +57,15 @@ filegroup(
     ],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["controller_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//pkg/kubelet/kubeletconfig/checkpoint:go_default_library",
+        "//pkg/kubelet/kubeletconfig/checkpoint/store:go_default_library",
+        "//pkg/kubelet/kubeletconfig/status:go_default_library",
+        "//staging/src/k8s.io/api/core/v1:go_default_library",
+    ],
+)

pkg/kubelet/kubeletconfig/checkpoint/store/fakestore.go

@@ -32,6 +32,11 @@ type fakeStore struct {
 
 var _ Store = (*fakeStore)(nil)
 
+// NewFakeStore constructs a fake Store
+func NewFakeStore() Store {
+	return &fakeStore{}
+}
+
 func (s *fakeStore) Initialize() error {
 	return fmt.Errorf("Initialize method not supported")
 }

pkg/kubelet/kubeletconfig/controller.go

@@ -309,7 +309,7 @@ func (cc *Controller) graduateAssignedToLastKnownGood() error {
 	}
 	// if the sources are equal, no need to change
 	if assigned == lastKnownGood ||
-		assigned != nil && lastKnownGood != nil && apiequality.Semantic.DeepEqual(assigned, lastKnownGood) {
+		assigned != nil && lastKnownGood != nil && apiequality.Semantic.DeepEqual(assigned.NodeConfigSource(), lastKnownGood.NodeConfigSource()) {
 		return nil
 	}
 	// update last-known-good

pkg/kubelet/kubeletconfig/controller_test.go

@@ -0,0 +1,81 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubeletconfig
+
+import (
+	"testing"
+
+	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/kubernetes/pkg/kubelet/kubeletconfig/checkpoint"
+	"k8s.io/kubernetes/pkg/kubelet/kubeletconfig/checkpoint/store"
+	"k8s.io/kubernetes/pkg/kubelet/kubeletconfig/status"
+)
+
+func TestGraduateAssignedToLastKnownGood(t *testing.T) {
+	realSource1, _, err := checkpoint.NewRemoteConfigSource(&apiv1.NodeConfigSource{
+		ConfigMap: &apiv1.ConfigMapNodeConfigSource{
+			Namespace:        "foo",
+			Name:             "1",
+			KubeletConfigKey: "kubelet",
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	realSource2, _, err := checkpoint.NewRemoteConfigSource(&apiv1.NodeConfigSource{
+		ConfigMap: &apiv1.ConfigMapNodeConfigSource{
+			Namespace:        "foo",
+			Name:             "2",
+			KubeletConfigKey: "kubelet",
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	cases := []struct {
+		name     string
+		assigned checkpoint.RemoteConfigSource
+		lkg      checkpoint.RemoteConfigSource
+	}{
+		{
+			name:     "nil lkg to non-nil lkg",
+			assigned: realSource1,
+			lkg:      nil,
+		},
+		{
+			name:     "non-nil lkg to non-nil lkg",
+			assigned: realSource2,
+			lkg:      realSource1,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			controller := &Controller{
+				configStatus:    status.NewNodeConfigStatus(),
+				checkpointStore: store.NewFakeStore(),
+			}
+			controller.checkpointStore.SetLastKnownGood(tc.lkg)
+			controller.checkpointStore.SetAssigned(tc.assigned)
+			if err := controller.graduateAssignedToLastKnownGood(); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}

test/e2e_node/dynamic_kubelet_config_test.go

@@ -418,6 +418,70 @@ var _ = framework.KubeDescribe("[Feature:DynamicKubeletConfig][NodeFeature:Dynam
 			})
 		})
 
+		// previously, we missed a panic because we were not exercising this path
+		Context("update Node.Spec.ConfigSource: non-nil last-known-good to a new non-nil last-known-good", func() {
+			It(itDescription, func() {
+				var err error
+				// we base the "lkg" configmap off of the configuration from before the test
+				lkgKC := beforeKC.DeepCopy()
+				lkgConfigMap1 := newKubeletConfigMap("dynamic-kubelet-config-test-lkg-1", lkgKC)
+				lkgConfigMap1, err = f.ClientSet.CoreV1().ConfigMaps("kube-system").Create(lkgConfigMap1)
+				framework.ExpectNoError(err)
+
+				lkgSource1 := &apiv1.NodeConfigSource{ConfigMap: &apiv1.ConfigMapNodeConfigSource{
+					Namespace:        lkgConfigMap1.Namespace,
+					Name:             lkgConfigMap1.Name,
+					KubeletConfigKey: "kubelet",
+				}}
+				lkgStatus1 := lkgSource1.DeepCopy()
+				lkgStatus1.ConfigMap.UID = lkgConfigMap1.UID
+				lkgStatus1.ConfigMap.ResourceVersion = lkgConfigMap1.ResourceVersion
+
+				lkgConfigMap2 := newKubeletConfigMap("dynamic-kubelet-config-test-lkg-2", lkgKC)
+				lkgConfigMap2, err = f.ClientSet.CoreV1().ConfigMaps("kube-system").Create(lkgConfigMap2)
+				framework.ExpectNoError(err)
+
+				lkgSource2 := &apiv1.NodeConfigSource{ConfigMap: &apiv1.ConfigMapNodeConfigSource{
+					Namespace:        lkgConfigMap2.Namespace,
+					Name:             lkgConfigMap2.Name,
+					KubeletConfigKey: "kubelet",
+				}}
+				lkgStatus2 := lkgSource2.DeepCopy()
+				lkgStatus2.ConfigMap.UID = lkgConfigMap2.UID
+				lkgStatus2.ConfigMap.ResourceVersion = lkgConfigMap2.ResourceVersion
+
+				// cases
+				first := nodeConfigTestCase{
+					desc:         "last-known-good-1",
+					configSource: lkgSource1,
+					configMap:    lkgConfigMap1,
+					expectConfigStatus: expectNodeConfigStatus{
+						lastKnownGood: lkgStatus1,
+					},
+					expectConfig: lkgKC,
+					event:        true,
+				}
+
+				second := nodeConfigTestCase{
+					desc:         "last-known-good-2",
+					configSource: lkgSource2,
+					configMap:    lkgConfigMap2,
+					expectConfigStatus: expectNodeConfigStatus{
+						lastKnownGood: lkgStatus2,
+					},
+					expectConfig: lkgKC,
+					event:        true,
+				}
+
+				// Manually actuate this to ensure we wait for each case to become the last-known-good
+				const lkgDuration = 12 * time.Minute
+				By(fmt.Sprintf("setting initial state %q", first.desc))
+				first.run(f, setConfigSourceFunc, true, lkgDuration)
+				By(fmt.Sprintf("from %q to %q", first.desc, second.desc))
+				second.run(f, setConfigSourceFunc, true, lkgDuration)
+			})
+		})
+
 		// exposes resource leaks across config changes
 		Context("update Node.Spec.ConfigSource: 100 update stress test:", func() {
 			It(itDescription, func() {