Fix Lint warning for missing Deployment spec.template.metadata.labels (…

…istio#27312) * Fix Lint warning for missing Deployment spec.template.labels * Add test * Add error handling * remove warn and use writer * check for panic in test
sel · Sep 22, 2020 · e9a02da · e9a02da
1 parent 0a653e0
commit e9a02da
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 13 deletions.
diff --git a/istioctl/pkg/validate/validate.go b/istioctl/pkg/validate/validate.go
@@ -39,7 +39,6 @@ import (
 	"istio.io/istio/pkg/config/schema/collection"
 	"istio.io/istio/pkg/config/schema/collections"
 	"istio.io/istio/pkg/url"
-	"istio.io/pkg/log"
 )
 
 var (
@@ -76,7 +75,7 @@ func checkFields(un *unstructured.Unstructured) error {
 	return errs
 }
 
-func (v *validator) validateResource(istioNamespace string, un *unstructured.Unstructured) error {
+func (v *validator) validateResource(istioNamespace string, un *unstructured.Unstructured, writer io.Writer) error {
 	gvk := config.GroupVersionKind{
 		Group:   un.GroupVersionKind().Group,
 		Version: un.GroupVersionKind().Version,
@@ -112,7 +111,10 @@ func (v *validator) validateResource(istioNamespace string, un *unstructured.Uns
 				}
 			}
 			if castItem.GetKind() == name.DeploymentStr {
-				v.validateDeploymentLabel(istioNamespace, castItem)
+				err := v.validateDeploymentLabel(istioNamespace, castItem, writer)
+				if err != nil {
+					errs = multierror.Append(errs, err)
+				}
 			}
 			return nil
 		})
@@ -126,7 +128,10 @@ func (v *validator) validateResource(istioNamespace string, un *unstructured.Uns
 	}
 
 	if un.GetKind() == name.DeploymentStr {
-		v.validateDeploymentLabel(istioNamespace, un)
+		err := v.validateDeploymentLabel(istioNamespace, un, writer)
+		if err != nil {
+			return err
+		}
 		return nil
 	}
 
@@ -184,20 +189,39 @@ func (v *validator) validateServicePortPrefix(istioNamespace string, un *unstruc
 	return nil
 }
 
-func (v *validator) validateDeploymentLabel(istioNamespace string, un *unstructured.Unstructured) {
+func (v *validator) validateDeploymentLabel(istioNamespace string, un *unstructured.Unstructured, writer io.Writer) error {
 	if un.GetNamespace() == handleNamespace(istioNamespace) {
-		return
+		return nil
 	}
-	labels := un.GetLabels()
+	labels, err := GetTemplateLabels(un)
+	if err != nil {
+		return err
+	}
+	url := fmt.Sprintf("See %s\n", url.DeploymentRequirements)
 	for _, l := range istioDeploymentLabel {
 		if _, ok := labels[l]; !ok {
-			log.Warnf("deployment %q may not provide Istio metrics and telemetry without label %q."+
-				" See "+url.DeploymentRequirements, fmt.Sprintf("%s/%s:", un.GetName(), un.GetNamespace()), l)
+			fmt.Fprintf(writer, "deployment %q may not provide Istio metrics and telemetry without label %q. "+url,
+				fmt.Sprintf("%s/%s:", un.GetName(), un.GetNamespace()), l)
+		}
+	}
+	return nil
+}
+
+// GetTemplateLabels returns spec.template.metadata.labels from Deployment
+func GetTemplateLabels(u *unstructured.Unstructured) (map[string]string, error) {
+	if spec, ok := u.Object["spec"].(map[string]interface{}); ok {
+		if template, ok := spec["template"].(map[string]interface{}); ok {
+			m, _, err := unstructured.NestedStringMap(template, "metadata", "labels")
+			if err != nil {
+				return nil, err
+			}
+			return m, nil
 		}
 	}
+	return nil, nil
 }
 
-func (v *validator) validateFile(istioNamespace *string, reader io.Reader) error {
+func (v *validator) validateFile(istioNamespace *string, reader io.Reader, writer io.Writer) error {
 	decoder := yaml.NewDecoder(reader)
 	decoder.SetStrict(true)
 	var errs error
@@ -217,7 +241,7 @@ func (v *validator) validateFile(istioNamespace *string, reader io.Reader) error
 		}
 		out := transformInterfaceMap(raw)
 		un := unstructured.Unstructured{Object: out}
-		err = v.validateResource(*istioNamespace, &un)
+		err = v.validateResource(*istioNamespace, &un, writer)
 		if err != nil {
 			errs = multierror.Append(errs, multierror.Prefix(err, fmt.Sprintf("%s/%s/%s:",
 				un.GetKind(), un.GetNamespace(), un.GetName())))
@@ -244,7 +268,7 @@ func validateFiles(istioNamespace *string, filenames []string, writer io.Writer)
 			errs = multierror.Append(errs, fmt.Errorf("cannot read file %q: %v", filename, err))
 			continue
 		}
-		err = v.validateFile(istioNamespace, reader)
+		err = v.validateFile(istioNamespace, reader, writer)
 		if err != nil {
 			errs = multierror.Append(errs, err)
 		}

diff --git a/istioctl/pkg/validate/validate_test.go b/istioctl/pkg/validate/validate_test.go
@@ -24,6 +24,7 @@ import (
 	"testing"
 
 	"github.com/ghodss/yaml"
+	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -278,6 +279,38 @@ trafficPolicy: {}
 trafficPolicy:
   tls:
     mode: ISTIO_MUTUAL
+`
+	validDeployment = `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: helloworld-v1
+  labels:
+    app: helloworld
+    version: v1
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: helloworld
+      version: v1
+  template:
+    metadata:
+      annotations:
+        sidecar.istio.io/bootstrapOverride: "istio-custom-bootstrap-config"
+      labels:
+        app: helloworld
+        version: v1
+    spec:
+      containers:
+        - name: helloworld
+          image: docker.io/istio/examples-helloworld-v1
+          resources:
+            requests:
+              cpu: "100m"
+          imagePullPolicy: IfNotPresent
+          ports:
+            - containerPort: 5000
 `
 )
 
@@ -374,8 +407,10 @@ func TestValidateResource(t *testing.T) {
 
 	for i, c := range cases {
 		t.Run(fmt.Sprintf("[%v] %v ", i, c.name), func(tt *testing.T) {
+			defer func() { recover() }()
 			v := &validator{}
-			err := v.validateResource("istio-system", fromYAML(c.in))
+			var writer io.Writer
+			err := v.validateResource("istio-system", fromYAML(c.in), writer)
 			if (err == nil) != c.valid {
 				tt.Fatalf("unexpected validation result: got %v want %v: err=%v", err == nil, c.valid, err)
 			}
@@ -546,3 +581,16 @@ $`),
 		})
 	}
 }
+
+func TestGetTemplateLabels(t *testing.T) {
+	assert := assert.New(t)
+	un := fromYAML(validDeployment)
+
+	labels, err := GetTemplateLabels(un)
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.NotEmpty(t, labels)
+	assert.Contains(labels, "app")
+	assert.Contains(labels, "version")
+}