commands/.../scorecard: improve crd resources check (#1027)

**Description of the change:** Changes the crd resources check so that the command
actually verifies what resources the operator touches. It also makes a separate function to get the proxy logs so that multiple tests can more easily use proxy logs.


**Motivation for the change:** This makes the test more accurate and can tell the user if they missed a resource that the operator interacts with.

Author: AlexNPavel

commands/operator-sdk/cmd/scorecard/basic_tests.go

@@ -15,7 +15,6 @@
 package scorecard
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -24,17 +23,10 @@ import (
 	"strings"
 	"time"
 
-	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
-
-	log "github.com/sirupsen/logrus"
 	"github.com/spf13/viper"
-	appsv1 "k8s.io/api/apps/v1"
-	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -172,38 +164,10 @@ func modifySpecAndCheck(specMap map[string]interface{}, obj *unstructured.Unstru
 // and/or POST requests to the API server, which should mean that it is creating or modifying resources.
 func writingIntoCRsHasEffect(obj *unstructured.Unstructured) (string, error) {
 	test := scorecardTest{testType: basicOperator, name: "Writing into CRs has an effect", maximumPoints: 1}
-	kubeclient, err := kubernetes.NewForConfig(kubeconfig)
-	if err != nil {
-		return "", fmt.Errorf("failed to create kubeclient: %v", err)
-	}
-	dep := &appsv1.Deployment{}
-	err = runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: deploymentName}, dep)
-	if err != nil {
-		return "", fmt.Errorf("failed to get newly created operator deployment: %v", err)
-	}
-	set := labels.Set(dep.Spec.Selector.MatchLabels)
-	pods := &v1.PodList{}
-	err = runtimeClient.List(context.TODO(), &client.ListOptions{LabelSelector: set.AsSelector()}, pods)
-	if err != nil {
-		return "", fmt.Errorf("failed to get list of pods in deployment: %v", err)
-	}
-	proxyPod = &pods.Items[0]
-	req := kubeclient.CoreV1().Pods(obj.GetNamespace()).GetLogs(proxyPod.GetName(), &v1.PodLogOptions{Container: "scorecard-proxy"})
-	readCloser, err := req.Stream()
-	if err != nil {
-		return "", fmt.Errorf("failed to get logs: %v", err)
-	}
-	defer func() {
-		if err := readCloser.Close(); err != nil && !fileutil.IsClosedError(err) {
-			log.Errorf("Failed to close pod log reader: (%v)", err)
-		}
-	}()
-	buf := new(bytes.Buffer)
-	_, err = buf.ReadFrom(readCloser)
+	logs, err := getProxyLogs()
 	if err != nil {
-		return "", fmt.Errorf("test failed and failed to read pod logs: %v", err)
+		return "", err
 	}
-	logs := buf.String()
 	msgMap := make(map[string]interface{})
 	for _, msg := range strings.Split(logs, "\n") {
 		if err := json.Unmarshal([]byte(msg), &msgMap); err != nil {
@@ -222,5 +186,5 @@ func writingIntoCRsHasEffect(obj *unstructured.Unstructured) (string, error) {
 	if test.earnedPoints != 1 {
 		scSuggestions = append(scSuggestions, "The operator should write into objects to update state. No PUT or POST requests from you operator were recorded by the scorecard.")
 	}
-	return buf.String(), nil
+	return logs, nil
 }

commands/operator-sdk/cmd/scorecard/olm_tests.go

@@ -16,6 +16,7 @@ package scorecard
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -25,6 +26,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -110,12 +112,38 @@ func crdsHaveValidation(crdsDir string, runtimeClient client.Client, obj *unstru
 }
 
 // crdsHaveResources checks to make sure that all owned CRDs have resources listed
-func crdsHaveResources(csv *olmapiv1alpha1.ClusterServiceVersion) {
+// Until there is full support for multiple CRs, we will only be able to check the
+// actual used resources of one CRD, but only the existence of a resources section
+// for other CRDs
+func crdsHaveResources(obj *unstructured.Unstructured, csv *olmapiv1alpha1.ClusterServiceVersion) {
 	test := scorecardTest{testType: olmIntegration, name: "Owned CRDs have resources listed"}
 	for _, crd := range csv.Spec.CustomResourceDefinitions.Owned {
 		test.maximumPoints++
-		if len(crd.Resources) > 0 {
-			test.earnedPoints++
+		gvk := obj.GroupVersionKind()
+		if strings.EqualFold(crd.Version, gvk.Version) && matchKind(gvk.Kind, crd.Kind) {
+			resources, err := getUsedResources()
+			if err != nil {
+				log.Warningf("getUsedResource failed: %v", err)
+			}
+			allResourcesListed := true
+			for _, resource := range resources {
+				foundResource := false
+				for _, listedResource := range crd.Resources {
+					if matchKind(resource.Kind, listedResource.Kind) && strings.EqualFold(resource.Version, listedResource.Version) {
+						foundResource = true
+					}
+				}
+				if foundResource == false {
+					allResourcesListed = false
+				}
+			}
+			if allResourcesListed {
+				test.earnedPoints++
+			}
+		} else {
+			if len(crd.Resources) > 0 {
+				test.earnedPoints++
+			}
 		}
 	}
 	scTests = append(scTests, test)
@@ -124,6 +152,91 @@ func crdsHaveResources(csv *olmapiv1alpha1.ClusterServiceVersion) {
 	}
 }
 
+func getUsedResources() ([]schema.GroupVersionKind, error) {
+	logs, err := getProxyLogs()
+	if err != nil {
+		return nil, err
+	}
+	resources := map[schema.GroupVersionKind]bool{}
+	for _, line := range strings.Split(logs, "\n") {
+		logMap := make(map[string]interface{})
+		err := json.Unmarshal([]byte(line), &logMap)
+		if err != nil {
+			// it is very common to get "unexpected end of JSON input", so we'll leave this at the debug level
+			log.Debugf("could not unmarshal line: %v", err)
+			continue
+		}
+		/*
+			There are 6 formats a resource uri can have:
+			Cluster-Scoped:
+				Collection: /apis/GROUP/VERSION/KIND
+				Individual: /apis/GROUP/VERSION/KIND/NAME
+				Core:       /api/v1/KIND
+			Namespaces:
+				All Namespaces:          /apis/GROUP/VERSION/KIND (same as cluster collection)
+				Collection in Namespace: /apis/GROUP/VERSION/namespaces/NAMESPACE/KIND
+				Individual:              /apis/GROUP/VERSION/namespaces/NAMESPACE/KIND/NAME
+				Core:                    /api/v1/namespaces/NAMESPACE/KIND
+
+			These urls are also often appended with options, which are denoted by the '?' symbol
+		*/
+		if msg, ok := logMap["msg"].(string); !ok || msg != "Request Info" {
+			continue
+		}
+		uri, ok := logMap["uri"].(string)
+		if !ok {
+			log.Warn("URI type is not string")
+			continue
+		}
+		removedOptions := strings.Split(uri, "?")[0]
+		splitURI := strings.Split(removedOptions, "/")
+		// first string is empty string ""
+		if len(splitURI) < 2 {
+			log.Warnf("Invalid URI: \"%s\"", uri)
+			continue
+		}
+		splitURI = splitURI[1:]
+		switch len(splitURI) {
+		case 3:
+			if splitURI[0] == "api" {
+				resources[schema.GroupVersionKind{Version: splitURI[1], Kind: splitURI[2]}] = true
+				break
+			} else if splitURI[0] == "apis" {
+				// this situation happens when the client enumerates the available resources of the server
+				// Example: "/apis/apps/v1?timeout=32s"
+				break
+			}
+			log.Warnf("Invalid URI: \"%s\"", uri)
+		case 4:
+			if splitURI[0] == "apis" {
+				resources[schema.GroupVersionKind{Group: splitURI[1], Version: splitURI[2], Kind: splitURI[3]}] = true
+				break
+			}
+			log.Warnf("Invalid URI: \"%s\"", uri)
+		case 5:
+			if splitURI[0] == "api" {
+				resources[schema.GroupVersionKind{Version: splitURI[1], Kind: splitURI[4]}] = true
+				break
+			} else if splitURI[0] == "apis" {
+				resources[schema.GroupVersionKind{Group: splitURI[1], Version: splitURI[2], Kind: splitURI[3]}] = true
+				break
+			}
+			log.Warnf("Invalid URI: \"%s\"", uri)
+		case 6, 7:
+			if splitURI[0] == "apis" {
+				resources[schema.GroupVersionKind{Group: splitURI[1], Version: splitURI[2], Kind: splitURI[5]}] = true
+				break
+			}
+			log.Warnf("Invalid URI: \"%s\"", uri)
+		}
+	}
+	var resourcesArr []schema.GroupVersionKind
+	for gvk := range resources {
+		resourcesArr = append(resourcesArr, gvk)
+	}
+	return resourcesArr, nil
+}
+
 // annotationsContainExamples makes sure that the CSVs list at least 1 example for the CR
 func annotationsContainExamples(csv *olmapiv1alpha1.ClusterServiceVersion) {
 	test := scorecardTest{testType: olmIntegration, name: "CRs have at least 1 example", maximumPoints: 1}

commands/operator-sdk/cmd/scorecard/resource_handler.go

@@ -27,6 +27,7 @@ import (
 	proxyConf "github.com/operator-framework/operator-sdk/pkg/ansible/proxy/kubeconfig"
 	"github.com/operator-framework/operator-sdk/pkg/k8sutil"
 	"github.com/spf13/viper"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/ghodss/yaml"
 	log "github.com/sirupsen/logrus"
@@ -35,9 +36,11 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/kubernetes"
 )
 
 // yamlToUnstructured decodes a yaml file into an unstructured object
@@ -126,6 +129,37 @@ func createFromYAMLFile(yamlPath string) error {
 			}
 		}
 		addResourceCleanup(obj, types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()})
+		if obj.GetKind() == "Deployment" {
+			dep := &appsv1.Deployment{}
+			err = runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: viper.GetString(NamespaceOpt), Name: deploymentName}, dep)
+			if err != nil {
+				return fmt.Errorf("failed to get newly created deployment: %v", err)
+			}
+			set := labels.Set(dep.Spec.Selector.MatchLabels)
+			// in some cases, the pod from the old deployment will be picked up instead of the new one
+			err = wait.PollImmediate(time.Second*1, time.Second*60, func() (bool, error) {
+				pods := &v1.PodList{}
+				err = runtimeClient.List(context.TODO(), &client.ListOptions{LabelSelector: set.AsSelector()}, pods)
+				if err != nil {
+					return false, fmt.Errorf("failed to get list of pods in deployment: %v", err)
+				}
+				// make sure the pods exist
+				// there should only be 1 pod per deployment
+				if len(pods.Items) == 1 {
+					// if the pod has a deletion timestamp, it is the old pod; wait for pod with no deletion timestamp
+					if pods.Items[0].GetDeletionTimestamp() == nil {
+						proxyPod = &pods.Items[0]
+						return true, nil
+					}
+				} else {
+					log.Debug("Operator deployment has more than 1 pod")
+				}
+				return false, nil
+			})
+			if err != nil {
+				return fmt.Errorf("failed to get proxyPod: %s", err)
+			}
+		}
 	}
 	if err := scanner.Err(); err != nil {
 		return fmt.Errorf("failed to scan %s: (%v)", yamlPath, err)
@@ -299,3 +333,23 @@ func addResourceCleanup(obj runtime.Object, key types.NamespacedName) {
 		return nil
 	})
 }
+
+func getProxyLogs() (string, error) {
+	// need a standard kubeclient for pod logs
+	kubeclient, err := kubernetes.NewForConfig(kubeconfig)
+	if err != nil {
+		return "", fmt.Errorf("failed to create kubeclient: %v", err)
+	}
+	req := kubeclient.CoreV1().Pods(proxyPod.GetNamespace()).GetLogs(proxyPod.GetName(), &v1.PodLogOptions{Container: "scorecard-proxy"})
+	readCloser, err := req.Stream()
+	if err != nil {
+		return "", fmt.Errorf("failed to get logs: %v", err)
+	}
+	defer readCloser.Close()
+	buf := new(bytes.Buffer)
+	_, err = buf.ReadFrom(readCloser)
+	if err != nil {
+		return "", fmt.Errorf("test failed and failed to read pod logs: %v", err)
+	}
+	return buf.String(), nil
+}

commands/operator-sdk/cmd/scorecard/scorecard.go

@@ -241,7 +241,7 @@ func ScorecardTests(cmd *cobra.Command, args []string) error {
 			return err
 		}
 		fmt.Println("Checking for CRD resources")
-		crdsHaveResources(csv)
+		crdsHaveResources(obj, csv)
 		fmt.Println("Checking for existence of example CRs")
 		annotationsContainExamples(csv)
 		fmt.Println("Checking spec descriptors")

hack/tests/scorecard-subcommand.sh

@@ -18,11 +18,11 @@ commandoutput="$(operator-sdk scorecard \
   --proxy-image "$DEST_IMAGE" \
   --proxy-pull-policy Never \
   2>&1)"
-echo $commandoutput | grep "Total Score: 6/8 points"
+echo $commandoutput | grep "Total Score: 5/8 points"
 
 # test config file
 commandoutput2="$(operator-sdk scorecard \
   --proxy-image "$DEST_IMAGE" \
   --config "$CONFIG_PATH")"
-echo $commandoutput2 | grep "Total Score: 6/8 points"
+echo $commandoutput2 | grep "Total Score: 5/8 points"
 popd

test/test-framework/.test-osdk-scorecard.yaml

@@ -3,3 +3,4 @@ init-timeout: 60
 csv-path: "deploy/memcachedoperator.0.0.2.csv.yaml"
 proxy-image: "scorecard-proxy"
 proxy-pull-policy: "Never"
+verbose: true

test/test-framework/deploy/memcachedoperator.0.0.2.csv.yaml

@@ -58,7 +58,7 @@ spec:
         version: v1alpha1
         resources:
           - kind: Deployment
-            version: v1beta2
+            version: v1
           - kind: ReplicaSet
             version: v1beta2
           - kind: Pod