Merge pull request #80 from varshavaradarajan/varsha/add-details-to-diagnostics

Add additional details to diagnostics, provide specific labels and taint keys  for nodeLabelsTaints check

checks.md

@@ -588,6 +588,8 @@ spec:
 
 When a DOKS cluster is upgraded, all worker nodes are replaced, and replacement nodes do not retain any custom labels or taints that were previously set by the user on the nodes. This check reports any labels or taints that will be lost on upgrade.
 
+DOKS provides persistent node pool labels. Adding a custom label to a node pool will ensure that the label is propagated to the worker nodes in the node pool after replacement or upgrade.
+
 ### How to Fix
 
 ```bash

checks/basic/bare_pods.go

@@ -73,9 +73,9 @@ func (b *barePodCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) {
 }
 
 func isStaticPod(pod corev1.Pod, nodeList []corev1.Node) bool {
-	for _,node := range nodeList {
+	for _, node := range nodeList {
 		// https://github.com/kubernetes/kubernetes/blob/b409073e99695ea35642a8194b9285ac12fd0cf8/pkg/kubelet/config/common.go#L51
-		if strings.HasSuffix(pod.Name, "-" + strings.ToLower(node.Name)) {
+		if strings.HasSuffix(pod.Name, "-"+strings.ToLower(node.Name)) {
 			return true
 		}
 	}

checks/diagnostic.go

@@ -30,6 +30,7 @@ type Diagnostic struct {
 	Kind     Kind
 	Object   *metav1.ObjectMeta
 	Owners   []metav1.OwnerReference
+	Details  string
 }
 
 func (d Diagnostic) String() string {

checks/doks/node_labels_taints.go

@@ -17,6 +17,7 @@ limitations under the License.
 package doks
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/digitalocean/clusterlint/checks"
@@ -48,34 +49,38 @@ func (*nodeLabelsTaintsCheck) Description() string {
 // Run runs the check.
 func (c *nodeLabelsTaintsCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, error) {
 	var diagnostics []checks.Diagnostic
-
 	for _, node := range objects.Nodes.Items {
+		var customLabels, customTaints []string
 		for labelKey := range node.Labels {
 			if !isKubernetesLabel(labelKey) && !isDOKSLabel(labelKey) {
-				d := checks.Diagnostic{
-					Severity: checks.Warning,
-					Message:  "Custom node labels will be lost if node is replaced or upgraded.",
-					Kind:     checks.Node,
-					Object:   &node.ObjectMeta,
-				}
-				diagnostics = append(diagnostics, d)
-				// Produce only one label diagnostic per node.
-				break
+				customLabels = append(customLabels, labelKey)
 			}
 		}
+		if len(customLabels) > 0 {
+			d := checks.Diagnostic{
+				Severity: checks.Warning,
+				Message:  "Custom node labels will be lost if node is replaced or upgraded. Add custom labels on node pools instead.",
+				Kind:     checks.Node,
+				Object:   &node.ObjectMeta,
+				Details:  fmt.Sprintf("Custom node labels: %s", customLabels),
+			}
+			diagnostics = append(diagnostics, d)
+		}
 		for _, taint := range node.Spec.Taints {
 			if !isDOKSTaint(taint) {
-				d := checks.Diagnostic{
-					Severity: checks.Warning,
-					Message:  "Custom node taints will be lost if node is replaced or upgraded.",
-					Kind:     checks.Node,
-					Object:   &node.ObjectMeta,
-				}
-				diagnostics = append(diagnostics, d)
-				// Produce only one taint diagnostic per node.
-				break
+				customTaints = append(customTaints, taint.Key)
 			}
 		}
+		if len(customTaints) > 0 {
+			d := checks.Diagnostic{
+				Severity: checks.Warning,
+				Message:  "Custom node taints will be lost if node is replaced or upgraded.",
+				Kind:     checks.Node,
+				Object:   &node.ObjectMeta,
+				Details:  fmt.Sprintf("Custom node taints: %s", customTaints),
+			}
+			diagnostics = append(diagnostics, d)
+		}
 	}
 
 	return diagnostics, nil

checks/doks/node_labels_taints_test.go

@@ -75,8 +75,9 @@ func TestNodeLabels(t *testing.T) {
 			},
 			expectedDiagnostics: []checks.Diagnostic{{
 				Severity: checks.Warning,
-				Message:  "Custom node labels will be lost if node is replaced or upgraded.",
+				Message:  "Custom node labels will be lost if node is replaced or upgraded. Add custom labels on node pools instead.",
 				Kind:     checks.Node,
+				Details:  "Custom node labels: [example.com/custom-label example.com/another-label]",
 				Object: &metav1.ObjectMeta{
 					Labels: map[string]string{
 						"doks.digitalocean.com/foo":                "bar",
@@ -134,6 +135,7 @@ func TestNodeTaints(t *testing.T) {
 			}},
 			expectedDiagnostics: []checks.Diagnostic{{
 				Severity: checks.Warning,
+				Details:  "Custom node taints: [example.com/my-taint]",
 				Message:  "Custom node taints will be lost if node is replaced or upgraded.",
 				Kind:     checks.Node,
 				Object:   &metav1.ObjectMeta{},

checks/run_checks.go

@@ -28,7 +28,7 @@ import (
 
 // Run applies the filters and runs the resultant check list in parallel
 func Run(ctx context.Context, client *kube.Client, checkFilter CheckFilter, diagnosticFilter DiagnosticFilter, objectFilter kube.ObjectFilter) (*CheckResult, error) {
-	objects, err := client.FetchObjects(ctx,objectFilter)
+	objects, err := client.FetchObjects(ctx, objectFilter)
 	if err != nil {
 		return nil, err
 	}

checks/run_checks_test.go

@@ -45,7 +45,7 @@ func TestRun(t *testing.T) {
 	alwaysFailCheck, err := Get("always-fail")
 	assert.NoError(t, err)
 
-	result, err := Run(context.Background(), client, filter, DiagnosticFilter{},kube.ObjectFilter{})
+	result, err := Run(context.Background(), client, filter, DiagnosticFilter{}, kube.ObjectFilter{})
 	assert.NoError(t, err)
 	assert.Len(t, result.Diagnostics, 1)
 	assert.Equal(t, alwaysFailCheck.Name(), result.Diagnostics[0].Check)

cmd/clusterlint/main.go

@@ -190,9 +190,8 @@ func runChecks(c *cli.Context) error {
 	if err != nil {
 		return err
 	}
-	write(output, c)
-
-	return nil
+	err = write(output, c)
+	return err
 }
 
 func write(checkResult *checks.CheckResult, c *cli.Context) error {
@@ -220,7 +219,7 @@ func write(checkResult *checks.CheckResult, c *cli.Context) error {
 			case checks.Suggestion:
 				s.Println(d)
 			default:
-				fmt.Printf("%s\n", d)
+				fmt.Println(d)
 			}
 		}
 	}

kube/object_filter.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/fields"
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
 )
+
 // ObjectFilter stores k8s object's fields that needs to be included or excluded while running checks
 type ObjectFilter struct {
 	IncludeNamespace string
@@ -43,10 +44,10 @@ func NewObjectFilter(includeNamespace, excludeNamespace string) (ObjectFilter, e
 // NamespaceOptions returns ListOptions for filtering by namespace
 func (f ObjectFilter) NamespaceOptions(opts metav1.ListOptions) metav1.ListOptions {
 	if len(f.IncludeNamespace) > 0 {
-		opts.FieldSelector = fields.OneTermEqualSelector("metadata.namespace",f.IncludeNamespace).String()
+		opts.FieldSelector = fields.OneTermEqualSelector("metadata.namespace", f.IncludeNamespace).String()
 	}
 	if len(f.ExcludeNamespace) > 0 {
-		opts.FieldSelector = fields.OneTermNotEqualSelector("metadata.namespace",f.ExcludeNamespace).String()
+		opts.FieldSelector = fields.OneTermNotEqualSelector("metadata.namespace", f.ExcludeNamespace).String()
 	}
 	return opts
-}
+}

kube/object_filter_test.go

@@ -33,17 +33,17 @@ func TestNamespaceError(t *testing.T) {
 }
 
 func TestNamespaceOptions(t *testing.T) {
-	filter, err := NewObjectFilter("namespace-1","")
+	filter, err := NewObjectFilter("namespace-1", "")
 	assert.NoError(t, err)
 	assert.Equal(t,
-		metav1.ListOptions{FieldSelector: fields.OneTermEqualSelector("metadata.namespace","namespace-1").String()},
+		metav1.ListOptions{FieldSelector: fields.OneTermEqualSelector("metadata.namespace", "namespace-1").String()},
 		filter.NamespaceOptions(metav1.ListOptions{}),
 	)
 
-	filter, err = NewObjectFilter("","namespace-2")
+	filter, err = NewObjectFilter("", "namespace-2")
 	assert.NoError(t, err)
 	assert.Equal(t,
-		metav1.ListOptions{FieldSelector: fields.OneTermNotEqualSelector("metadata.namespace","namespace-2").String()},
+		metav1.ListOptions{FieldSelector: fields.OneTermNotEqualSelector("metadata.namespace", "namespace-2").String()},
 		filter.NamespaceOptions(metav1.ListOptions{}),
 	)
-}
+}

kube/objects.go

@@ -64,7 +64,7 @@ type Client struct {
 
 // FetchObjects returns the objects from a Kubernetes cluster.
 // ctx is currently unused during API calls. More info: https://github.com/kubernetes/community/pull/1166
-func (c *Client) FetchObjects(ctx context.Context,filter ObjectFilter) (*Objects, error) {
+func (c *Client) FetchObjects(ctx context.Context, filter ObjectFilter) (*Objects, error) {
 	client := c.KubeClient.CoreV1()
 	admissionControllerClient := c.KubeClient.AdmissionregistrationV1beta1()
 	opts := metav1.ListOptions{}

kube/objects_test.go

@@ -40,7 +40,7 @@ func TestFetchObjects(t *testing.T) {
 			Labels: map[string]string{"doks_key": "bar"}},
 	})
 
-	actual, err := api.FetchObjects(context.Background(),ObjectFilter{})
+	actual, err := api.FetchObjects(context.Background(), ObjectFilter{})
 	assert.NoError(t, err)
 
 	assert.NotNil(t, actual.Nodes)