Add check names to diagnostics from the check runner

Rather than relying on each check to fill in its name correctly when
producing diagnostics, fill in the name in the check runner after
running the check. This reduces the likelihood that a check gets its
name wrong or forgets to fill it in.

This also fixes a bug where the admission control webhook check was not
filling in its name at all.

checks/basic/bare_pods.go

@@ -50,7 +50,6 @@ func (b *barePodCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) {
 		pod := pod
 		if len(pod.ObjectMeta.OwnerReferences) == 0 {
 			d := checks.Diagnostic{
-				Check:    b.Name(),
 				Severity: checks.Warning,
 				Message:  "Avoid using bare pods in clusters",
 				Kind:     checks.Pod,

checks/basic/bare_pods_test.go

@@ -67,7 +67,6 @@ func TestBarePodError(t *testing.T) {
 			expected: []checks.Diagnostic{
 				{
 					Severity: "warning",
-					Check:    "bare-pods",
 					Kind:     checks.Pod,
 					Message:  "Avoid using bare pods in clusters",
 					Object:   GetObjectMeta(),
@@ -81,7 +80,6 @@ func TestBarePodError(t *testing.T) {
 			expected: []checks.Diagnostic{
 				{
 					Severity: "warning",
-					Check:    "bare-pods",
 					Kind:     checks.Pod,
 					Message:  "Avoid using bare pods in clusters",
 					Object:   &metav1.ObjectMeta{Name: "pod_1", Namespace: "k8s"},
@@ -89,7 +87,6 @@ func TestBarePodError(t *testing.T) {
 				},
 				{
 					Severity: "warning",
-					Check:    "bare-pods",
 					Kind:     checks.Pod,
 					Message:  "Avoid using bare pods in clusters",
 					Object:   &metav1.ObjectMeta{Name: "pod_2", Namespace: "k8s"},

checks/basic/fully_qualified_image.go

@@ -71,7 +71,6 @@ func (fq *fullyQualifiedImageCheck) checkImage(containers []corev1.Container, po
 		value, err := reference.ParseAnyReference(container.Image)
 		if err != nil {
 			d := checks.Diagnostic{
-				Check:    fq.Name(),
 				Severity: checks.Warning,
 				Message:  fmt.Sprintf("Malformed image name for container '%s'", container.Name),
 				Kind:     checks.Pod,
@@ -82,7 +81,6 @@ func (fq *fullyQualifiedImageCheck) checkImage(containers []corev1.Container, po
 		} else {
 			if value.String() != container.Image {
 				d := checks.Diagnostic{
-					Check:    fq.Name(),
 					Severity: checks.Warning,
 					Message:  fmt.Sprintf("Use fully qualified image for container '%s'", container.Name),
 					Kind:     checks.Pod,

checks/basic/helper_test.go

@@ -92,7 +92,6 @@ func initContainer(image string) *kube.Objects {
 func issues(severity checks.Severity, message string, kind checks.Kind, check string) []checks.Diagnostic {
 	d := []checks.Diagnostic{
 		{
-			Check:    check,
 			Severity: severity,
 			Message:  message,
 			Kind:     kind,

checks/basic/hostpath.go

@@ -55,7 +55,6 @@ func (h *hostPathCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error)
 			pod := pod
 			if volume.VolumeSource.HostPath != nil {
 				d := checks.Diagnostic{
-					Check:    h.Name(),
 					Severity: checks.Warning,
 					Message:  fmt.Sprintf("Avoid using hostpath for volume '%s'.", volume.Name),
 					Kind:     checks.Pod,

checks/basic/hostpath_test.go

@@ -69,7 +69,6 @@ func TestHostpathVolumeError(t *testing.T) {
 			}),
 			expected: []checks.Diagnostic{
 				{
-					Check:    "hostpath-volume",
 					Severity: checks.Warning,
 					Message:  "Avoid using hostpath for volume 'bar'.",
 					Kind:     checks.Pod,

checks/basic/latest_tag.go

@@ -70,7 +70,6 @@ func (l *latestTagCheck) checkTags(containers []corev1.Container, pod corev1.Pod
 		tagNameOnly := reference.TagNameOnly(namedRef)
 		if strings.HasSuffix(tagNameOnly.String(), ":latest") {
 			d := checks.Diagnostic{
-				Check:    l.Name(),
 				Severity: checks.Warning,
 				Message:  fmt.Sprintf("Avoid using latest tag for container '%s'", container.Name),
 				Kind:     checks.Pod,

checks/basic/namespace.go

@@ -50,7 +50,6 @@ func (alert *alert) SetDiagnostics(d []checks.Diagnostic) {
 // warn adds warnings for k8s objects that should not be in the default namespace
 func (alert *alert) warn(k8stype checks.Kind, itemMeta metav1.ObjectMeta) {
 	d := checks.Diagnostic{
-		Check:    "default-namespace",
 		Severity: checks.Warning,
 		Message:  "Avoid using the default namespace",
 		Kind:     k8stype,

checks/basic/namespace_test.go

@@ -100,7 +100,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 	d := []checks.Diagnostic{
 
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.Pod,
@@ -108,7 +107,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 			Owners:   pod.ObjectMeta.GetOwnerReferences(),
 		},
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.PodTemplate,
@@ -116,7 +114,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 			Owners:   template.ObjectMeta.GetOwnerReferences(),
 		},
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.PersistentVolumeClaim,
@@ -124,7 +121,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 			Owners:   pvc.ObjectMeta.GetOwnerReferences(),
 		},
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.ConfigMap,
@@ -132,7 +128,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 			Owners:   cm.ObjectMeta.GetOwnerReferences(),
 		},
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.Service,
@@ -140,7 +135,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 			Owners:   service.ObjectMeta.GetOwnerReferences(),
 		},
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.Secret,
@@ -148,7 +142,6 @@ func errors(n defaultNamespaceCheck) []checks.Diagnostic {
 			Owners:   secret.ObjectMeta.GetOwnerReferences(),
 		},
 		{
-			Check:    n.Name(),
 			Severity: checks.Warning,
 			Message:  "Avoid using the default namespace",
 			Kind:     checks.ServiceAccount,

checks/basic/pod_status.go

@@ -55,7 +55,6 @@ func (p *podStatusCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error)
 	for _, pod := range objects.Pods.Items {
 		if corev1.PodFailed == pod.Status.Phase || corev1.PodUnknown == pod.Status.Phase {
 			d := checks.Diagnostic{
-				Check:    p.Name(),
 				Severity: checks.Warning,
 				Message:  fmt.Sprintf("Unhealthy pod. State: `%s`. Pod state should be `Running`, `Pending` or `Succeeded`.", pod.Status.Phase),
 				Kind:     checks.Pod,

checks/basic/pod_status_test.go

@@ -71,7 +71,6 @@ func TestPodStateError(t *testing.T) {
 			objs: status(corev1.PodFailed),
 			expected: []checks.Diagnostic{
 				{
-					Check:    podStatusCheck.Name(),
 					Severity: checks.Warning,
 					Message:  "Unhealthy pod. State: `Failed`. Pod state should be `Running`, `Pending` or `Succeeded`.",
 					Kind:     checks.Pod,
@@ -85,7 +84,6 @@ func TestPodStateError(t *testing.T) {
 			objs: status(corev1.PodUnknown),
 			expected: []checks.Diagnostic{
 				{
-					Check:    podStatusCheck.Name(),
 					Severity: checks.Warning,
 					Message:  "Unhealthy pod. State: `Unknown`. Pod state should be `Running`, `Pending` or `Succeeded`.",
 					Kind:     checks.Pod,

checks/basic/resource_requests.go

@@ -69,7 +69,6 @@ func (r *resourceRequirementsCheck) checkResourceRequirements(containers []corev
 	for _, container := range containers {
 		if container.Resources.Size() == 0 {
 			d := checks.Diagnostic{
-				Check:    r.Name(),
 				Severity: checks.Warning,
 				Message:  fmt.Sprintf("Set resource requests and limits for container `%s` to prevent resource contention", container.Name),
 				Kind:     checks.Pod,

checks/basic/resource_requests_test.go

@@ -60,7 +60,6 @@ func TestResourceRequestsWarning(t *testing.T) {
 			objs: container("alpine"),
 			expected: []checks.Diagnostic{
 				{
-					Check:    resourceRequirementsCheck.Name(),
 					Severity: checks.Warning,
 					Message:  message,
 					Kind:     checks.Pod,
@@ -74,7 +73,6 @@ func TestResourceRequestsWarning(t *testing.T) {
 			objs: initContainer("alpine"),
 			expected: []checks.Diagnostic{
 				{
-					Check:    resourceRequirementsCheck.Name(),
 					Severity: checks.Warning,
 					Message:  message,
 					Kind:     checks.Pod,

checks/basic/unused_config_map.go

@@ -71,7 +71,6 @@ func (c *unusedCMCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error)
 		if _, ok := used[kube.Identifier{Name: cm.GetName(), Namespace: cm.GetNamespace()}]; !ok {
 			cm := cm
 			d := checks.Diagnostic{
-				Check:    c.Name(),
 				Severity: checks.Warning,
 				Message:  "Unused config map",
 				Kind:     checks.ConfigMap,

checks/basic/unused_config_map_test.go

@@ -80,7 +80,6 @@ func TestUnusedConfigMapWarning(t *testing.T) {
 			objs: initConfigMap(),
 			expected: []checks.Diagnostic{
 				{
-					Check:    unusedCMCheck.Name(),
 					Severity: checks.Warning,
 					Message:  "Unused config map",
 					Kind:     checks.ConfigMap,

checks/basic/unused_pv.go

@@ -53,7 +53,6 @@ func (p *unusedPVCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error)
 	for _, pv := range objects.PersistentVolumes.Items {
 		if pv.Spec.ClaimRef == nil {
 			d := checks.Diagnostic{
-				Check:    p.Name(),
 				Severity: checks.Warning,
 				Message:  fmt.Sprintf("Unused Persistent Volume '%s'.", pv.GetName()),
 				Kind:     checks.PersistentVolume,

checks/basic/unused_pv_test.go

@@ -63,7 +63,6 @@ func TestUnusedPVWarning(t *testing.T) {
 			objs: unused(),
 			expected: []checks.Diagnostic{
 				{
-					Check:    unusedPVCheck.Name(),
 					Severity: checks.Warning,
 					Message:  "Unused Persistent Volume 'pv_foo'.",
 					Kind:     checks.PersistentVolume,

checks/basic/unused_pvc.go

@@ -62,7 +62,6 @@ func (c *unusedClaimCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, erro
 	for _, claim := range objects.PersistentVolumeClaims.Items {
 		if _, ok := used[kube.Identifier{Name: claim.GetName(), Namespace: claim.GetNamespace()}]; !ok {
 			d := checks.Diagnostic{
-				Check:    c.Name(),
 				Severity: checks.Warning,
 				Message:  "Unused persistent volume claim",
 				Kind:     checks.PersistentVolumeClaim,

checks/basic/unused_pvc_test.go

@@ -67,7 +67,6 @@ func TestUnusedPVCWarning(t *testing.T) {
 			objs: initPVC(),
 			expected: []checks.Diagnostic{
 				{
-					Check:    unusedClaimCheck.Name(),
 					Severity: checks.Warning,
 					Message:  "Unused persistent volume claim",
 					Kind:     checks.PersistentVolumeClaim,

checks/basic/unused_secrets.go

@@ -66,7 +66,6 @@ func (s *unusedSecretCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, err
 		if _, ok := used[kube.Identifier{Name: secret.GetName(), Namespace: secret.GetNamespace()}]; !ok {
 			secret := secret
 			d := checks.Diagnostic{
-				Check:    s.Name(),
 				Severity: checks.Warning,
 				Message:  "Unused secret",
 				Kind:     checks.Secret,

checks/basic/unused_secrets_test.go

@@ -78,7 +78,6 @@ func TestUnusedSecretWarning(t *testing.T) {
 			objs: initSecret(),
 			expected: []checks.Diagnostic{
 				{
-					Check:    unusedSecretCheck.Name(),
 					Severity: checks.Warning,
 					Message:  "Unused secret",
 					Kind:     checks.Secret,

checks/doks/node_labels_taints.go

@@ -53,7 +53,6 @@ func (c *nodeLabelsTaintsCheck) Run(objects *kube.Objects) ([]checks.Diagnostic,
 		for labelKey := range node.Labels {
 			if !isKubernetesLabel(labelKey) && !isDOKSLabel(labelKey) {
 				d := checks.Diagnostic{
-					Check:    c.Name(),
 					Severity: checks.Warning,
 					Message:  "Custom node labels will be lost if node is replaced or upgraded.",
 					Kind:     checks.Node,
@@ -67,7 +66,6 @@ func (c *nodeLabelsTaintsCheck) Run(objects *kube.Objects) ([]checks.Diagnostic,
 		for _, taint := range node.Spec.Taints {
 			if !isDOKSTaint(taint) {
 				d := checks.Diagnostic{
-					Check:    c.Name(),
 					Severity: checks.Warning,
 					Message:  "Custom node taints will be lost if node is replaced or upgraded.",
 					Kind:     checks.Node,

checks/doks/node_labels_taints_test.go

@@ -74,7 +74,6 @@ func TestNodeLabels(t *testing.T) {
 				"region": "tor1",
 			},
 			expectedDiagnostics: []checks.Diagnostic{{
-				Check:    "node-labels-and-taints",
 				Severity: checks.Warning,
 				Message:  "Custom node labels will be lost if node is replaced or upgraded.",
 				Kind:     checks.Node,
@@ -134,7 +133,6 @@ func TestNodeTaints(t *testing.T) {
 				Effect: corev1.TaintEffectNoSchedule,
 			}},
 			expectedDiagnostics: []checks.Diagnostic{{
-				Check:    "node-labels-and-taints",
 				Severity: checks.Warning,
 				Message:  "Custom node taints will be lost if node is replaced or upgraded.",
 				Kind:     checks.Node,

checks/doks/node_name_pod_selector.go

@@ -53,7 +53,6 @@ func (p *podSelectorCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, erro
 		nodeSelectorMap := pod.Spec.NodeSelector
 		if _, ok := nodeSelectorMap[corev1.LabelHostname]; ok {
 			d := checks.Diagnostic{
-				Check:    p.Name(),
 				Severity: checks.Warning,
 				Message:  "Avoid node name label for node selector.",
 				Kind:     checks.Pod,

checks/doks/node_name_pod_selector_test.go

@@ -93,7 +93,6 @@ func expectedWarnings(objs *kube.Objects, name string) []checks.Diagnostic {
 	pod := objs.Pods.Items[0]
 	diagnostics := []checks.Diagnostic{
 		{
-			Check:    name,
 			Severity: checks.Warning,
 			Message:  "Avoid node name label for node selector.",
 			Kind:     checks.Pod,

checks/run_checks.go

@@ -54,6 +54,12 @@ func Run(ctx context.Context, client *kube.Client, checkFilter CheckFilter, diag
 				return err
 			}
 			mu.Lock()
+			// Fill in the check names for the diagnostics. Doing this here
+			// absolves checks of needing to do it and also ensures they're
+			// consistent.
+			for i := 0; i < len(d); i++ {
+				d[i].Check = check.Name()
+			}
 			diagnostics = append(diagnostics, d...)
 			checkDuration[check.Name()] = elapsed
 			mu.Unlock()

checks/run_checks_test.go

@@ -0,0 +1,82 @@
+/*
+Copyright 2019 DigitalOcean
+
+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 checks
+
+import (
+	"context"
+	"testing"
+
+	"github.com/digitalocean/clusterlint/kube"
+	"github.com/stretchr/testify/assert"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
+)
+
+func TestRun(t *testing.T) {
+	Register(&alwaysFail{})
+
+	filter := CheckFilter{
+		IncludeChecks: []string{"always-fail"},
+	}
+	client := &kube.Client{
+		KubeClient: fake.NewSimpleClientset(),
+	}
+	client.KubeClient.CoreV1().Namespaces().Create(&corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "kube-system",
+		},
+	})
+
+	alwaysFailCheck, err := Get("always-fail")
+	assert.NoError(t, err)
+
+	result, err := Run(context.Background(), client, filter, DiagnosticFilter{})
+	assert.NoError(t, err)
+	assert.Len(t, result.Diagnostics, 1)
+	assert.Equal(t, alwaysFailCheck.Name(), result.Diagnostics[0].Check)
+}
+
+type alwaysFail struct{}
+
+// Name returns a unique name for this check.
+func (nc *alwaysFail) Name() string {
+	return "always-fail"
+}
+
+// Groups returns a list of group names this check should be part of.
+func (nc *alwaysFail) Groups() []string {
+	return nil
+}
+
+// Description returns a detailed human-readable description of what this check
+// does.
+func (nc *alwaysFail) Description() string {
+	return "Does not check anything. Always returns an error.."
+}
+
+// Run runs this check on a set of Kubernetes objects. It can return warnings
+// (low-priority problems) and errors (high-priority problems) as well as an
+// error value indicating that the check failed to run.
+func (nc *alwaysFail) Run(*kube.Objects) ([]Diagnostic, error) {
+	return []Diagnostic{{
+		Message:  "This check always produces an error.",
+		Severity: Error,
+		Kind:     Pod,
+		Object:   &metav1.ObjectMeta{},
+	}}, nil
+}

checks/security/privileged_containers.go

@@ -67,7 +67,6 @@ func (pc *privilegedContainerCheck) checkPrivileged(containers []corev1.Containe
 	for _, container := range containers {
 		if container.SecurityContext != nil && container.SecurityContext.Privileged != nil && *container.SecurityContext.Privileged {
 			d := checks.Diagnostic{
-				Check:    pc.Name(),
 				Severity: checks.Warning,
 				Message:  fmt.Sprintf("Privileged container '%s' found. Please ensure that the image is from a trusted source.", container.Name),
 				Kind:     checks.Pod,

checks/security/privileged_containers_test.go

@@ -106,7 +106,6 @@ func warnings(objs *kube.Objects, name string) []checks.Diagnostic {
 	pod := objs.Pods.Items[0]
 	d := []checks.Diagnostic{
 		{
-			Check:    name,
 			Severity: checks.Warning,
 			Message:  "Privileged container 'bar' found. Please ensure that the image is from a trusted source.",
 			Kind:     checks.Pod,

checks/security/run_as_non_root.go

@@ -63,7 +63,6 @@ func (nr *nonRootUserCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, err
 
 			if containerRunAsRoot && podRunAsRoot {
 				d := checks.Diagnostic{
-					Check:    nr.Name(),
 					Severity: checks.Warning,
 					Message:  fmt.Sprintf("Container `%s` can run as root user. Please ensure that the image is from a trusted source.", container.Name),
 					Kind:     checks.Pod,

checks/security/run_as_non_root_test.go

@@ -161,7 +161,6 @@ func diagnostic() []checks.Diagnostic {
 	pod := initPod().Pods.Items[0]
 	d := []checks.Diagnostic{
 		{
-			Check:    "non-root-user",
 			Severity: checks.Warning,
 			Message:  "Container `bar` can run as root user. Please ensure that the image is from a trusted source.",
 			Kind:     checks.Pod,