Remove and prevent unchecked type assertions (#1245)

* Enable errcheck with check-type-assertions.
* Also add a test for the checked type assertions in LoadYAML.

.golangci.yml

@@ -1,6 +1,7 @@
 linters:
   auto-fix: false
   enable:
+    - errcheck
     - goimports
     - golint
     - gosec
@@ -11,6 +12,8 @@ run:
     # autogenerated clientset by client-gen
     - pkg/client
 linters-settings:
+  errcheck:
+    check-type-assertions: true
   goimports:
     # Don't use 'github.com/kudobuilder/kudo', it'll result in unreliable output!
     local-prefixes: github.com/kudobuilder

pkg/engine/renderer/enhancer.go

@@ -83,8 +83,7 @@ func (k *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 	return objs, nil
 }
 
-func setControllerReference(owner v1.Object, obj runtime.Object, scheme *runtime.Scheme) error {
-	object := obj.(v1.Object)
+func setControllerReference(owner v1.Object, object *unstructured.Unstructured, scheme *runtime.Scheme) error {
 	ownerNs := owner.GetNamespace()
 	if ownerNs != "" {
 		objNs := object.GetNamespace()

pkg/engine/task/task_pipe.go

@@ -104,7 +104,10 @@ func (pt PipeTask) Run(ctx Context) (bool, error) {
 	// 8. - Copy out the pipe files -
 	log.Printf("PipeTask: %s/%s copying pipe files", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName)
 	fs := afero.NewMemMapFs()
-	pipePod := podObj[0].(*corev1.Pod)
+	pipePod, ok := podObj[0].(*corev1.Pod)
+	if !ok {
+		return false, errors.New("internal error: pipe pod changed type after enhance and apply")
+	}
 
 	err = copyFiles(fs, pt.PipeFiles, pipePod, ctx)
 	if err != nil {

pkg/test/step.go

@@ -441,7 +441,11 @@ func (s *Step) LoadYAML(file string) error {
 
 	for _, obj := range s.Asserts {
 		if obj.GetObjectKind().GroupVersionKind().Kind == "TestAssert" {
-			s.Assert = obj.(*kudo.TestAssert)
+			if testAssert, ok := obj.(*kudo.TestAssert); ok {
+				s.Assert = testAssert
+			} else {
+				return fmt.Errorf("failed to load TestAssert object from %s: it contains an object of type %T", file, obj)
+			}
 		} else {
 			asserts = append(asserts, obj)
 		}
@@ -451,7 +455,11 @@ func (s *Step) LoadYAML(file string) error {
 
 	for _, obj := range s.Apply {
 		if obj.GetObjectKind().GroupVersionKind().Kind == "TestStep" {
-			s.Step = obj.(*kudo.TestStep)
+			if testStep, ok := obj.(*kudo.TestStep); ok {
+				s.Step = testStep
+			} else {
+				return fmt.Errorf("failed to load TestStep object from %s: it contains an object of type %T", file, obj)
+			}
 			s.Step.Index = s.Index
 			if s.Step.Name != "" {
 				s.Name = s.Step.Name

pkg/test/step_integration_test.go

@@ -322,3 +322,22 @@ func TestStepDeleteExistingLabelMatch(t *testing.T) {
 	assert.True(t, k8serrors.IsNotFound(testenv.Client.Get(context.TODO(), testutils.ObjectKey(podToDelete), podToDelete)))
 	assert.True(t, k8serrors.IsNotFound(testenv.Client.Get(context.TODO(), testutils.ObjectKey(podToDelete2), podToDelete2)))
 }
+
+func TestCheckedTypeAssertions(t *testing.T) {
+	tests := []struct {
+		name     string
+		typeName string
+	}{
+		{"assert", "TestAssert"},
+		{"apply", "TestStep"},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			step := Step{}
+			path := fmt.Sprintf("step_integration_test_data/00-%s.yaml", test.name)
+			assert.EqualError(t, step.LoadYAML(path),
+				fmt.Sprintf("failed to load %s object from %s: it contains an object of type *unstructured.Unstructured",
+					test.typeName, path))
+		})
+	}
+}

pkg/test/step_integration_test_data/00-apply.yaml

@@ -0,0 +1,4 @@
+apiVersion: v1
+kind: TestStep
+metadata:
+  name: problem

pkg/test/step_integration_test_data/00-assert.yaml

@@ -0,0 +1,4 @@
+apiVersion: v1
+kind: TestAssert
+metadata:
+  name: problem

pkg/test/step_test.go

@@ -54,10 +54,11 @@ func TestStepCreate(t *testing.T) {
 	pod := testutils.NewPod("hello", "")
 	podWithNamespace := testutils.NewPod("hello2", "different-namespace")
 	clusterScopedResource := testutils.NewResource("v1", "Namespace", "my-namespace", "")
-	podToUpdate := testutils.NewPod("update-me", "").(*unstructured.Unstructured)
-	updateToApply := testutils.WithSpec(t, podToUpdate, map[string]interface{}{
-		"replicas": 2,
-	}).(*unstructured.Unstructured)
+	podToUpdate := testutils.NewPod("update-me", "")
+	specToApply := map[string]interface{}{
+		"replicas": int64(2),
+	}
+	updateToApply := testutils.WithSpec(t, podToUpdate, specToApply)
 
 	cl := fake.NewFakeClient(testutils.WithNamespace(podToUpdate, "world"))
 
@@ -75,8 +76,9 @@ func TestStepCreate(t *testing.T) {
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(pod), pod))
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(clusterScopedResource), clusterScopedResource))
 
-	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podToUpdate), podToUpdate))
-	assert.Equal(t, updateToApply.Object["spec"], podToUpdate.Object["spec"])
+	updatedPod := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "v1", "kind": "Pod"}}
+	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podToUpdate), updatedPod))
+	assert.Equal(t, specToApply, updatedPod.Object["spec"])
 
 	assert.Nil(t, cl.Get(context.TODO(), testutils.ObjectKey(podWithNamespace), podWithNamespace))
 	actual := testutils.NewPod("hello2", namespace)

pkg/test/utils/subset.go

@@ -73,9 +73,12 @@ func IsSubset(expected, actual interface{}) error {
 			}
 
 			if err := IsSubset(iter.Value().Interface(), actualValue.Interface()); err != nil {
-				subsetErr := err.(*SubsetError)
-				subsetErr.AppendPath(iter.Key().String())
-				return subsetErr
+				subsetErr, ok := err.(*SubsetError)
+				if ok {
+					subsetErr.AppendPath(iter.Key().String())
+					return subsetErr
+				}
+				return err
 			}
 		}
 	} else {