commands/.../scorecard: modularize tests and implement weights

Author: AlexNPavel

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

@@ -37,39 +37,35 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-// checkSpecAndStat checks that the spec and status blocks exist. If noStore is set to true, this function
-// will not store the result of the test in scTests and will instead just wait until the spec and
-// status blocks exist or return an error after the timeout.
-func checkSpecAndStat(runtimeClient client.Client, obj *unstructured.Unstructured, noStore bool) error {
-	testSpec := scorecardTest{testType: basicOperator, name: "Spec Block Exists", maximumPoints: 1}
-	testStat := scorecardTest{testType: basicOperator, name: "Status Block Exist", maximumPoints: 1}
-	err := wait.Poll(time.Second*1, time.Second*time.Duration(SCConf.InitTimeout), func() (bool, error) {
-		err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, obj)
-		if err != nil {
-			return false, fmt.Errorf("error getting custom resource: %v", err)
-		}
-		var specPass, statusPass bool
-		if obj.Object["spec"] != nil {
-			testSpec.earnedPoints = 1
-			specPass = true
-		}
-
-		if obj.Object["status"] != nil {
-			testStat.earnedPoints = 1
-			statusPass = true
-		}
-		return statusPass && specPass, nil
-	})
-	if !noStore {
-		scTests = append(scTests, testSpec, testStat)
+// checkSpec checks that the spec block exists
+func checkSpec(test *Test, vars ScorecardVars) error {
+	score := Score{maximumPoints: 1}
+	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: vars.crObj.GetNamespace(), Name: vars.crObj.GetName()}, vars.crObj)
+	if err != nil {
+		return fmt.Errorf("error getting custom resource: %v", err)
 	}
-	if err != nil && err != wait.ErrWaitTimeout {
-		return err
+	if vars.crObj.Object["spec"] != nil {
+		score.earnedPoints = 1
 	}
-	if testSpec.earnedPoints != 1 {
+	test.scores = append(test.scores, score)
+	if score.earnedPoints != 1 {
 		scSuggestions = append(scSuggestions, "Add a 'spec' field to your Custom Resource")
 	}
-	if testStat.earnedPoints != 1 {
+	return nil
+}
+
+// checkStat checks that the stat block exists
+func checkStat(test *Test, vars ScorecardVars) error {
+	score := Score{maximumPoints: 1}
+	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: vars.crObj.GetNamespace(), Name: vars.crObj.GetName()}, vars.crObj)
+	if err != nil {
+		return fmt.Errorf("error getting custom resource: %v", err)
+	}
+	if vars.crObj.Object["status"] != nil {
+		score.earnedPoints = 1
+	}
+	test.scores = append(test.scores, score)
+	if score.earnedPoints != 1 {
 		scSuggestions = append(scSuggestions, "Add a 'status' field to your Custom Resource")
 	}
 	return nil
@@ -81,30 +77,29 @@ func checkSpecAndStat(runtimeClient client.Client, obj *unstructured.Unstructure
 // see if the status changes as a result. This is a bit prone to breakage as this is a black box test and we don't
 // know much about how the operators we are testing actually work and may pass an invalid value. In the future, we
 // should use user-specified tests
-func checkStatusUpdate(runtimeClient client.Client, obj *unstructured.Unstructured) error {
-	test := scorecardTest{testType: basicOperator, name: "Operator actions are reflected in status", maximumPoints: 1}
-	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, obj)
+func checkStatusUpdate(test *Test, vars ScorecardVars) error {
+	score := Score{maximumPoints: 1}
+	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: vars.crObj.GetNamespace(), Name: vars.crObj.GetName()}, vars.crObj)
 	if err != nil {
 		return fmt.Errorf("error getting custom resource: %v", err)
 	}
-	if obj.Object["status"] == nil || obj.Object["spec"] == nil {
-		scTests = append(scTests, test)
+	if vars.crObj.Object["status"] == nil || vars.crObj.Object["spec"] == nil {
 		return nil
 	}
 	statCopy := make(map[string]interface{})
-	for k, v := range obj.Object["status"].(map[string]interface{}) {
+	for k, v := range vars.crObj.Object["status"].(map[string]interface{}) {
 		statCopy[k] = v
 	}
-	specMap := obj.Object["spec"].(map[string]interface{})
-	err = modifySpecAndCheck(specMap, obj)
+	specMap := vars.crObj.Object["spec"].(map[string]interface{})
+	err = modifySpecAndCheck(specMap, vars.crObj)
 	if err != nil {
-		test.earnedPoints = 0
+		score.earnedPoints = 0
+		test.scores = append(test.scores, score)
 		scSuggestions = append(scSuggestions, "Make sure that the 'status' block is always updated to reflect changes after the 'spec' block is changed")
-		scTests = append(scTests, test)
 		return nil
 	}
-	test.earnedPoints = 1
-	scTests = append(scTests, test)
+	score.earnedPoints = 1
+	test.scores = append(test.scores, score)
 	return nil
 }
 
@@ -169,28 +164,27 @@ func modifySpecAndCheck(specMap map[string]interface{}, obj *unstructured.Unstru
 
 // wiritingIntoCRsHasEffect simply looks at the proxy logs and verifies that the operator is sending PUT
 // 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}
+func writingIntoCRsHasEffect(test *Test, vars ScorecardVars) error {
 	kubeclient, err := kubernetes.NewForConfig(kubeconfig)
 	if err != nil {
-		return "", fmt.Errorf("failed to create kubeclient: %v", err)
+		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)
+	err = runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: vars.crObj.GetNamespace(), Name: deploymentName}, dep)
 	if err != nil {
-		return "", fmt.Errorf("failed to get newly created operator deployment: %v", err)
+		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)
+		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"})
+	req := kubeclient.CoreV1().Pods(vars.crObj.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)
+		return fmt.Errorf("failed to get logs: %v", err)
 	}
 	defer func() {
 		if err := readCloser.Close(); err != nil && !fileutil.IsClosedError(err) {
@@ -200,10 +194,11 @@ func writingIntoCRsHasEffect(obj *unstructured.Unstructured) (string, error) {
 	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 fmt.Errorf("test failed and failed to read pod logs: %v", err)
 	}
 	logs := buf.String()
 	msgMap := make(map[string]interface{})
+	score := Score{maximumPoints: 1}
 	for _, msg := range strings.Split(logs, "\n") {
 		if err := json.Unmarshal([]byte(msg), &msgMap); err != nil {
 			continue
@@ -213,13 +208,13 @@ func writingIntoCRsHasEffect(obj *unstructured.Unstructured) (string, error) {
 			continue
 		}
 		if method == "PUT" || method == "POST" {
-			test.earnedPoints = 1
+			score.earnedPoints = 1
 			break
 		}
 	}
-	scTests = append(scTests, test)
-	if test.earnedPoints != 1 {
+	test.scores = append(test.scores, score)
+	if score.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 nil
 }

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

@@ -18,116 +18,112 @@ import (
 	"context"
 
 	olmAPI "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // crdsHaveResources checks to make sure that all owned CRDs have resources listed
-func crdsHaveResources(csv *olmAPI.ClusterServiceVersion) {
-	test := scorecardTest{testType: olmIntegration, name: "Owned CRDs have resources listed"}
-	for _, crd := range csv.Spec.CustomResourceDefinitions.Owned {
-		test.maximumPoints++
+func crdsHaveResources(test *Test, vars ScorecardVars) error {
+	score := Score{}
+	for _, crd := range vars.csvObj.Spec.CustomResourceDefinitions.Owned {
+		score.maximumPoints++
 		if len(crd.Resources) > 0 {
-			test.earnedPoints++
+			score.earnedPoints++
 		}
 	}
-	scTests = append(scTests, test)
-	if test.earnedPoints == 0 {
+	test.scores = append(test.scores, score)
+	if score.earnedPoints < score.maximumPoints {
 		scSuggestions = append(scSuggestions, "Add resources to owned CRDs")
 	}
+	return nil
 }
 
 // annotationsContainExamples makes sure that the CSVs list at least 1 example for the CR
-func annotationsContainExamples(csv *olmAPI.ClusterServiceVersion) {
-	test := scorecardTest{testType: olmIntegration, name: "CRs have at least 1 example", maximumPoints: 1}
-	if csv.Annotations != nil && csv.Annotations["alm-examples"] != "" {
-		test.earnedPoints = 1
+func annotationsContainExamples(test *Test, vars ScorecardVars) error {
+	score := Score{maximumPoints: 1}
+	if vars.csvObj.Annotations != nil && vars.csvObj.Annotations["alm-examples"] != "" {
+		score.earnedPoints = 1
 	}
-	scTests = append(scTests, test)
-	if test.earnedPoints == 0 {
-		scSuggestions = append(scSuggestions, "Add an alm-examples annotation to your CSV to pass the " + test.name + " test")
+	test.scores = append(test.scores, score)
+	if score.earnedPoints == 0 {
+		scSuggestions = append(scSuggestions, "Add an alm-examples annotation to your CSV to pass the "+test.name+" test")
 	}
+	return nil
 }
 
 // statusDescriptors makes sure that all status fields found in the created CR has a matching descriptor in the CSV
-func statusDescriptors(csv *olmAPI.ClusterServiceVersion, runtimeClient client.Client, obj *unstructured.Unstructured) error {
-	test := scorecardTest{testType: olmIntegration, name: "Status fields with descriptors"}
-	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, obj)
+func statusDescriptors(test *Test, vars ScorecardVars) error {
+	score := Score{}
+	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: vars.crObj.GetNamespace(), Name: vars.crObj.GetName()}, vars.crObj)
 	if err != nil {
 		return err
 	}
-	if obj.Object["status"] == nil {
+	if vars.crObj.Object["status"] == nil {
 		// what should we do if there is no status block? Maybe some kind of N/A type output?
-		scTests = append(scTests, test)
 		return nil
 	}
-	statusBlock := obj.Object["status"].(map[string]interface{})
-	test.maximumPoints = len(statusBlock)
+	statusBlock := vars.crObj.Object["status"].(map[string]interface{})
+	score.maximumPoints = len(statusBlock)
 	var crd *olmAPI.CRDDescription
-	for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
-		if owned.Kind == obj.GetKind() {
+	for _, owned := range vars.csvObj.Spec.CustomResourceDefinitions.Owned {
+		if owned.Kind == vars.crObj.GetKind() {
 			crd = &owned
 			break
 		}
 	}
 	if crd == nil {
-		scTests = append(scTests, test)
 		return nil
 	}
 	for key := range statusBlock {
 		for _, statDesc := range crd.StatusDescriptors {
 			if statDesc.Path == key {
-				test.earnedPoints++
+				score.earnedPoints++
 				delete(statusBlock, key)
 				break
 			}
 		}
 	}
-	scTests = append(scTests, test)
+	test.scores = append(test.scores, score)
 	for key := range statusBlock {
 		scSuggestions = append(scSuggestions, "Add a status descriptor for "+key)
 	}
 	return nil
 }
 
 // specDescriptors makes sure that all spec fields found in the created CR has a matching descriptor in the CSV
-func specDescriptors(csv *olmAPI.ClusterServiceVersion, runtimeClient client.Client, obj *unstructured.Unstructured) error {
-	test := scorecardTest{testType: olmIntegration, name: "Spec fields with descriptors"}
-	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, obj)
+func specDescriptors(test *Test, vars ScorecardVars) error {
+	score := Score{}
+	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: vars.crObj.GetNamespace(), Name: vars.crObj.GetName()}, vars.crObj)
 	if err != nil {
 		return err
 	}
-	if obj.Object["spec"] == nil {
+	if vars.crObj.Object["spec"] == nil {
 		// what should we do if there is no spec block? Maybe some kind of N/A type output?
-		scTests = append(scTests, test)
 		return nil
 	}
-	specBlock := obj.Object["spec"].(map[string]interface{})
-	test.maximumPoints = len(specBlock)
+	specBlock := vars.crObj.Object["spec"].(map[string]interface{})
+	score.maximumPoints = len(specBlock)
 	var crd *olmAPI.CRDDescription
-	for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
-		if owned.Kind == obj.GetKind() {
+	for _, owned := range vars.csvObj.Spec.CustomResourceDefinitions.Owned {
+		if owned.Kind == vars.crObj.GetKind() {
 			crd = &owned
 			break
 		}
 	}
 	if crd == nil {
-		scTests = append(scTests, test)
 		return nil
 	}
 	for key := range specBlock {
 		for _, specDesc := range crd.SpecDescriptors {
 			if specDesc.Path == key {
-				test.earnedPoints++
+				score.earnedPoints++
 				delete(specBlock, key)
 				break
 			}
 		}
 	}
-	scTests = append(scTests, test)
+	test.scores = append(test.scores, score)
 	for key := range specBlock {
-		scSuggestions = append(scSuggestions, "Add a spec descriptor for " + key)
+		scSuggestions = append(scSuggestions, "Add a spec descriptor for "+key)
 	}
 	return nil
 }

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

@@ -38,6 +38,27 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 )
 
+type cleanupFn func() error
+
+// waitUntilReady waits until the status block of the CR currently being tested exists. If the timeout
+// is reached, it simply continues and assumes there is no status block
+func waitUntilReady(obj *unstructured.Unstructured) error {
+	err := wait.Poll(time.Second*1, time.Second*time.Duration(SCConf.InitTimeout), func() (bool, error) {
+		err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, obj)
+		if err != nil {
+			return false, fmt.Errorf("error getting custom resource: %v", err)
+		}
+		if obj.Object["status"] != nil {
+			return true, nil
+		}
+		return false, nil
+	})
+	if err != nil && err != wait.ErrWaitTimeout {
+		return err
+	}
+	return nil
+}
+
 // yamlToUnstructured decodes a yaml file into an unstructured object
 func yamlToUnstructured(yamlPath string) (*unstructured.Unstructured, error) {
 	yamlFile, err := ioutil.ReadFile(yamlPath)