From ac38530e462a9e162fd6620ec550a3d9ab6cb2ca Mon Sep 17 00:00:00 2001 From: Varsha Varadarajan Date: Tue, 26 May 2020 14:03:14 -0700 Subject: [PATCH 1/2] Add additional details to diagnostics, provide specific labels and taint keys for nodeLabelsTaints check --- checks/basic/bare_pods.go | 4 +-- checks/diagnostic.go | 1 + checks/doks/node_labels_taints.go | 43 ++++++++++++++------------ checks/doks/node_labels_taints_test.go | 2 ++ checks/run_checks.go | 2 +- checks/run_checks_test.go | 2 +- cmd/clusterlint/main.go | 7 ++--- kube/object_filter.go | 7 +++-- kube/object_filter_test.go | 10 +++--- kube/objects.go | 2 +- kube/objects_test.go | 2 +- 11 files changed, 45 insertions(+), 37 deletions(-) diff --git a/checks/basic/bare_pods.go b/checks/basic/bare_pods.go index f9d904b..8ec4dee 100644 --- a/checks/basic/bare_pods.go +++ b/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 } } diff --git a/checks/diagnostic.go b/checks/diagnostic.go index 0684d5a..64eada7 100644 --- a/checks/diagnostic.go +++ b/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 { diff --git a/checks/doks/node_labels_taints.go b/checks/doks/node_labels_taints.go index 64321ff..55aad62 100644 --- a/checks/doks/node_labels_taints.go +++ b/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.", + 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 diff --git a/checks/doks/node_labels_taints_test.go b/checks/doks/node_labels_taints_test.go index fd87efd..9e84a9e 100644 --- a/checks/doks/node_labels_taints_test.go +++ b/checks/doks/node_labels_taints_test.go @@ -77,6 +77,7 @@ func TestNodeLabels(t *testing.T) { Severity: checks.Warning, Message: "Custom node labels will be lost if node is replaced or upgraded.", 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{}, diff --git a/checks/run_checks.go b/checks/run_checks.go index 1a25171..6ec8083 100644 --- a/checks/run_checks.go +++ b/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 } diff --git a/checks/run_checks_test.go b/checks/run_checks_test.go index 8b44ca9..a1dda67 100644 --- a/checks/run_checks_test.go +++ b/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) diff --git a/cmd/clusterlint/main.go b/cmd/clusterlint/main.go index 7d95332..3c1aac2 100644 --- a/cmd/clusterlint/main.go +++ b/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) } } } diff --git a/kube/object_filter.go b/kube/object_filter.go index 72d5093..9ca6859 100644 --- a/kube/object_filter.go +++ b/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 -} \ No newline at end of file +} diff --git a/kube/object_filter_test.go b/kube/object_filter_test.go index 95fd950..4fab868 100644 --- a/kube/object_filter_test.go +++ b/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{}), ) -} \ No newline at end of file +} diff --git a/kube/objects.go b/kube/objects.go index ac4701e..5aea097 100644 --- a/kube/objects.go +++ b/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{} diff --git a/kube/objects_test.go b/kube/objects_test.go index 31fb736..e2ef52b 100644 --- a/kube/objects_test.go +++ b/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) From aa7e326e5abd019c7031a8f858aafbdd811fc166 Mon Sep 17 00:00:00 2001 From: Varsha Varadarajan Date: Thu, 28 May 2020 09:10:24 -0700 Subject: [PATCH 2/2] Update node labels diagnostics to mention persistent node pool labels --- checks.md | 2 ++ checks/doks/node_labels_taints.go | 2 +- checks/doks/node_labels_taints_test.go | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/checks.md b/checks.md index 2143e52..b603e9e 100644 --- a/checks.md +++ b/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 diff --git a/checks/doks/node_labels_taints.go b/checks/doks/node_labels_taints.go index 55aad62..a52454d 100644 --- a/checks/doks/node_labels_taints.go +++ b/checks/doks/node_labels_taints.go @@ -59,7 +59,7 @@ func (c *nodeLabelsTaintsCheck) Run(objects *kube.Objects) ([]checks.Diagnostic, if len(customLabels) > 0 { d := 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, Object: &node.ObjectMeta, Details: fmt.Sprintf("Custom node labels: %s", customLabels), diff --git a/checks/doks/node_labels_taints_test.go b/checks/doks/node_labels_taints_test.go index 9e84a9e..a07a049 100644 --- a/checks/doks/node_labels_taints_test.go +++ b/checks/doks/node_labels_taints_test.go @@ -75,7 +75,7 @@ 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{