Repository: aquasecurity/starboard
Pull request #1010 (2022-03-09 09:34:46)

refactor: decouple policy pkg from starboard apis
This will allow us to use the aquasecurity/defsec module
to load and evaluate Rego policies.

Signed-off-by: Daniel Pacak <pacak.daniel@gmail.com>

danielpacak: It seems that we have a flaky test. Can you check why it's failing @chen-keinan ?

```
--- FAIL: TestControlChecksByScannerChecks (0.00s)
[95](https://github.com/aquasecurity/starboard/runs/5477898995?check_suite_focus=true#step:5:95)
    --- FAIL: TestControlChecksByScannerChecks/_control_checks_by_scanner_checks (0.00s)
```
chen-keinan: I have pushed the fix for flaky test in #1009 
codecov[bot]: # [Codecov](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity) Report
> Merging [#1010](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity) (29ef8e9) into [main](https://codecov.io/gh/aquasecurity/starboard/commit/0f837424d9505036015389e4815e08fa60a6d8b9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity) (0f83742) will **decrease** coverage by `0.10%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/aquasecurity/starboard/pull/1010/graphs/tree.svg?width=650&height=150&src=pr&token=qqTGKAJ0Wb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity)](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity)

```diff
@@            Coverage Diff             @@
##             main    #1010      +/-   ##
==========================================
- Coverage   57.98%   57.88%   -0.11%     
==========================================
  Files          70       70              
  Lines        9095     9098       +3     
==========================================
- Hits         5274     5266       -8     
- Misses       3296     3304       +8     
- Partials      525      528       +3     
```


| [Impacted Files](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity) | Coverage Δ | |
|---|---|---|
| [pkg/configauditreport/controller.go](https://codecov.io/gh/aquasecurity/starboard/pull/1010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity#diff-cGtnL2NvbmZpZ2F1ZGl0cmVwb3J0L2NvbnRyb2xsZXIuZ28=) | `61.41% <100.00%> (+0.64%)` | :arrow_up: |
| [pkg/policy/policy.go](https://codecov.io/gh/aquasecurity/starboard/pull/1010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity#diff-cGtnL3BvbGljeS9wb2xpY3kuZ28=) | `80.00% <100.00%> (-0.76%)` | :arrow_down: |
| [pkg/operator/controller/vulnerabilityreport.go](https://codecov.io/gh/aquasecurity/starboard/pull/1010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity#diff-cGtnL29wZXJhdG9yL2NvbnRyb2xsZXIvdnVsbmVyYWJpbGl0eXJlcG9ydC5nbw==) | `55.79% <0.00%> (-3.05%)` | :arrow_down: |
| [pkg/operator/controller/ciskubebenchreport.go](https://codecov.io/gh/aquasecurity/starboard/pull/1010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity#diff-cGtnL29wZXJhdG9yL2NvbnRyb2xsZXIvY2lza3ViZWJlbmNocmVwb3J0Lmdv) | `53.38% <0.00%> (-1.20%)` | :arrow_down: |
| [pkg/operator/controller/configauditreport.go](https://codecov.io/gh/aquasecurity/starboard/pull/1010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity#diff-cGtnL29wZXJhdG9yL2NvbnRyb2xsZXIvY29uZmlnYXVkaXRyZXBvcnQuZ28=) | `54.03% <0.00%> (+1.39%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity). Last update [b1fd2cb...29ef8e9](https://codecov.io/gh/aquasecurity/starboard/pull/1010?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aquasecurity).


Patch:
diff --git a/pkg/configauditreport/controller.go b/pkg/configauditreport/controller.go
index 1cc5982..bb15659 100644
--- a/pkg/configauditreport/controller.go
+++ b/pkg/configauditreport/controller.go
@@ -269,11 +269,25 @@ func (r *ResourceController) policies(ctx context.Context) (*policy.Policies, er
 }
 
 func (r *ResourceController) evaluate(ctx context.Context, policies *policy.Policies, resource client.Object) (v1alpha1.ConfigAuditReportData, error) {
-	checks, err := policies.Eval(ctx, resource)
+	results, err := policies.Eval(ctx, resource)
 	if err != nil {
 		return v1alpha1.ConfigAuditReportData{}, err
 	}
 
+	checks := make([]v1alpha1.Check, len(results))
+	for i, result := range results {
+		checks[i] = v1alpha1.Check{
+			ID:          result.Metadata.ID,
+			Title:       result.Metadata.Title,
+			Description: result.Metadata.Description,
+			Severity:    result.Metadata.Severity,
+			Category:    result.Metadata.Type,
+
+			Success:  result.Success,
+			Messages: result.Messages,
+		}
+	}
+
 	return v1alpha1.ConfigAuditReportData{
 		Scanner: v1alpha1.Scanner{
 			Name:    "Starboard",
diff --git a/pkg/policy/policy.go b/pkg/policy/policy.go
index 8157b6d..fe618f4 100644
--- a/pkg/policy/policy.go
+++ b/pkg/policy/policy.go
@@ -25,6 +25,17 @@ const (
 	kindWorkload = "Workload"
 )
 
+const (
+	// varMessage is the name of Rego variable used to bind deny or warn
+	// messages.
+	varMessage = "msg"
+	// varMetadata is the name of Rego variable used to bind policy metadata.
+	varMetadata = "md"
+	// varResult is the name of Rego variable used to bind result of evaluating
+	// deny or warn rules.
+	varResult = "res"
+)
+
 // Metadata describes policy metadata.
 type Metadata struct {
 	ID          string
@@ -73,23 +84,31 @@ func NewMetadata(values map[string]interface{}) (Metadata, error) {
 	}, nil
 }
 
-// Result describes result of evaluating a policy.
+// Result describes result of evaluating a Rego policy that defines `deny` or
+// `warn` rules.
 type Result struct {
-	Message string
+	// Metadata describes Rego policy metadata.
+	Metadata Metadata
+
+	// Success represents the status of evaluating Rego policy.
+	Success bool
+
+	// Messages deny or warning messages.
+	Messages []string
 }
 
-// NewResult constructs new Result based on raw values.
-func NewResult(values map[string]interface{}) (Result, error) {
+type Results []Result
+
+// NewMessage constructs new message string based on raw values.
+func NewMessage(values map[string]interface{}) (string, error) {
 	if values == nil {
-		return Result{}, errors.New("values must not be nil")
+		return "", errors.New("values must not be nil")
 	}
-	message, err := requiredStringValue(values, "msg")
+	message, err := requiredStringValue(values, varMessage)
 	if err != nil {
-		return Result{}, err
+		return "", err
 	}
-	return Result{
-		Message: message,
-	}, nil
+	return message, nil
 }
 
 type Policies struct {
@@ -188,7 +207,7 @@ func (p *Policies) Applicable(resource client.Object) (bool, string, error) {
 //
 // TODO(danielpacak) Compile and cache prepared queries to make Eval more efficient.
 //                   We can reuse prepared queries so long policies do not change.
-func (p *Policies) Eval(ctx context.Context, resource client.Object) ([]v1alpha1.Check, error) {
+func (p *Policies) Eval(ctx context.Context, resource client.Object) (Results, error) {
 	if resource == nil {
 		return nil, fmt.Errorf("resource must not be nil")
 	}
@@ -197,7 +216,7 @@ func (p *Policies) Eval(ctx context.Context, resource client.Object) ([]v1alpha1
 		return nil, fmt.Errorf("resource kind must not be blank")
 	}
 
-	checks := make([]v1alpha1.Check, 0)
+	var results Results
 
 	policies, err := p.PoliciesByKind(resourceKind)
 	if err != nil {
@@ -237,7 +256,7 @@ func (p *Policies) Eval(ctx context.Context, resource client.Object) ([]v1alpha1
 			return nil, fmt.Errorf("failed evaluating Rego metadata rule: %s: %w", metadataQuery, err)
 		}
 
-		metadataResult, hasMetadataResult := hasBinding(metadata, "md")
+		metadataResult, hasMetadataResult := hasBinding(metadata, varMetadata)
 
 		if !hasMetadataResult {
 			return nil, fmt.Errorf("failed parsing policy metadata: %s", policyName)
@@ -259,13 +278,13 @@ func (p *Policies) Eval(ctx context.Context, resource client.Object) ([]v1alpha1
 			return nil, fmt.Errorf("failed evaluating Rego deny rule: %s: %w", denyQuery, err)
 		}
 
-		denyResults, hasDenyResults := hasBindings(deny, "res")
-		if hasDenyResults {
-			denyChecks, err := valuesToChecks(md, denyResults)
+		denyValues, hasDenyValues := hasBindings(deny, varResult)
+		if hasDenyValues {
+			denyResults, err := valuesToResults(md, denyValues)
 			if err != nil {
 				return nil, fmt.Errorf("failed parsing deny rule result: %s: %w", denyQuery, err)
 			}
-			checks = append(checks, denyChecks...)
+			results = append(results, denyResults...)
 			continue
 		}
 
@@ -280,27 +299,23 @@ func (p *Policies) Eval(ctx context.Context, resource client.Object) ([]v1alpha1
 			return nil, fmt.Errorf("failed evaluating Rego warn rule: %s: %w", warnQuery, err)
 		}
 
-		warnResults, hasWarnResults := hasBindings(warn, "res")
-		if hasWarnResults {
-			warnChecks, err := valuesToChecks(md, warnResults)
+		warnValues, hasWarnValues := hasBindings(warn, varResult)
+		if hasWarnValues {
+			warnResults, err := valuesToResults(md, warnValues)
 			if err != nil {
 				return nil, fmt.Errorf("failed parsing warn rule result: %s: %w", warnQuery, err)
 			}
-			checks = append(checks, warnChecks...)
+			results = append(results, warnResults...)
 			continue
 		}
 
-		checks = append(checks, v1alpha1.Check{
-			Success:     true,
-			ID:          md.ID,
-			Title:       md.Title,
-			Severity:    md.Severity,
-			Category:    md.Type,
-			Description: md.Description,
+		results = append(results, Result{
+			Metadata: md,
+			Success:  true,
 		})
 	}
 
-	return checks, nil
+	return results, nil
 }
 
 func hasBinding(rs rego.ResultSet, key string) (map[string]interface{}, bool) {
@@ -349,26 +364,22 @@ func requiredStringValue(values map[string]interface{}, key string) (string, err
 	return valueString, nil
 }
 
-func valuesToChecks(md Metadata, values []map[string]interface{}) ([]v1alpha1.Check, error) {
-	var checks []v1alpha1.Check
+func valuesToResults(md Metadata, values []map[string]interface{}) (Results, error) {
+	var results Results
 	var messages []string
 
 	for _, value := range values {
-		result, err := NewResult(value)
+		message, err := NewMessage(value)
 		if err != nil {
 			return nil, err
 		}
-		messages = append(messages, result.Message)
+		messages = append(messages, message)
 	}
 
-	checks = append(checks, v1alpha1.Check{
-		Success:     false,
-		ID:          md.ID,
-		Title:       md.Title,
-		Severity:    md.Severity,
-		Category:    md.Type,
-		Description: md.Description,
-		Messages:    messages,
+	results = append(results, Result{
+		Metadata: md,
+		Success:  false,
+		Messages: messages,
 	})
-	return checks, nil
+	return results, nil
 }


Test patch:
diff --git a/pkg/policy/policy_test.go b/pkg/policy/policy_test.go
index b53669d..18f25d0 100644
--- a/pkg/policy/policy_test.go
+++ b/pkg/policy/policy_test.go
@@ -14,7 +14,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-func TestConfig_PoliciesByKind(t *testing.T) {
+func TestPolicies_PoliciesByKind(t *testing.T) {
 
 	t.Run("Should return error when kinds are not defined for policy", func(t *testing.T) {
 		g := NewGomegaWithT(t)
@@ -72,7 +72,7 @@ func TestConfig_PoliciesByKind(t *testing.T) {
 	})
 }
 
-func TestConfig_Applicable(t *testing.T) {
+func TestPolicies_Applicable(t *testing.T) {
 
 	testCases := []struct {
 		name     string
@@ -130,12 +130,12 @@ deny[res] {
 
 }
 
-func TestConfig_Eval(t *testing.T) {
+func TestPolicies_Eval(t *testing.T) {
 	testCases := []struct {
 		name          string
 		resource      client.Object
 		policies      map[string]string
-		checks        []v1alpha1.Check
+		results       policy.Results
 		expectedError string
 	}{
 		{
@@ -191,15 +191,17 @@ deny[res] {
 }
 `,
 			},
-			checks: []v1alpha1.Check{
+			results: []policy.Result{
 				{
-					Success:     false,
-					ID:          "KSV014",
-					Title:       "Root file system is not read-only",
-					Description: "An immutable root file system prevents applications from writing to their local disk",
-					Severity:    v1alpha1.SeverityLow,
-					Category:    "Kubernetes Security Check",
-					Messages:    []string{"Containers must not run as root"},
+					Success: false,
+					Metadata: policy.Metadata{
+						ID:          "KSV014",
+						Title:       "Root file system is not read-only",
+						Description: "An immutable root file system prevents applications from writing to their local disk",
+						Severity:    v1alpha1.SeverityLow,
+						Type:        "Kubernetes Security Check",
+					},
+					Messages: []string{"Containers must not run as root"},
 				},
 			},
 		},
@@ -259,14 +261,16 @@ deny[res] {
 }
 `,
 			},
-			checks: []v1alpha1.Check{
+			results: []policy.Result{
 				{
-					Success:     true,
-					ID:          "KSV014",
-					Severity:    v1alpha1.SeverityLow,
-					Title:       "Root file system is not read-only",
-					Description: "An immutable root file system prevents applications from writing to their local disk",
-					Category:    "Kubernetes Security Check",
+					Success: true,
+					Metadata: policy.Metadata{
+						ID:          "KSV014",
+						Severity:    v1alpha1.SeverityLow,
+						Title:       "Root file system is not read-only",
+						Description: "An immutable root file system prevents applications from writing to their local disk",
+						Type:        "Kubernetes Security Check",
+					},
 				},
 			},
 		},
@@ -323,15 +327,17 @@ warn[res] {
 }
 `,
 			},
-			checks: []v1alpha1.Check{
+			results: []policy.Result{
 				{
-					Success:     false,
-					ID:          "KSV014",
-					Title:       "Root file system is not read-only",
-					Description: "An immutable root file system prevents applications from writing to their local disk",
-					Severity:    v1alpha1.SeverityMedium,
-					Category:    "Kubernetes Security Check",
-					Messages:    []string{"Containers must not run as root"},
+					Success: false,
+					Metadata: policy.Metadata{
+						ID:          "KSV014",
+						Title:       "Root file system is not read-only",
+						Description: "An immutable root file system prevents applications from writing to their local disk",
+						Severity:    v1alpha1.SeverityMedium,
+						Type:        "Kubernetes Security Check",
+					},
+					Messages: []string{"Containers must not run as root"},
 				},
 			},
 		},
@@ -391,14 +397,16 @@ warn[res] {
 }
 `,
 			},
-			checks: []v1alpha1.Check{
+			results: []policy.Result{
 				{
-					Success:     true,
-					ID:          "KSV014",
-					Severity:    v1alpha1.SeverityLow,
-					Title:       "Root file system is not read-only",
-					Description: "An immutable root file system prevents applications from writing to their local disk",
-					Category:    "Kubernetes Security Check",
+					Success: true,
+					Metadata: policy.Metadata{
+						ID:          "KSV014",
+						Severity:    v1alpha1.SeverityLow,
+						Title:       "Root file system is not read-only",
+						Description: "An immutable root file system prevents applications from writing to their local disk",
+						Type:        "Kubernetes Security Check",
+					},
 				},
 			},
 		},
@@ -628,15 +636,17 @@ deny[res] {
 	}
 }`,
 			},
-			checks: []v1alpha1.Check{
+			results: []policy.Result{
 				{
-					ID:          "KSV013",
-					Title:       "Image tag ':latest' used",
-					Description: "It is best to avoid using the ':latest' image tag when deploying containers in production. Doing so makes it hard to track which version of the image is running, and hard to roll back the version.",
-					Severity:    "LOW",
-					Category:    "Kubernetes Security Check",
-					Messages:    []string{"msg1", "msg2"},
-					Success:     false,
+					Metadata: policy.Metadata{
+						ID:          "KSV013",
+						Title:       "Image tag ':latest' used",
+						Description: "It is best to avoid using the ':latest' image tag when deploying containers in production. Doing so makes it hard to track which version of the image is running, and hard to roll back the version.",
+						Severity:    "LOW",
+						Type:        "Kubernetes Security Check",
+					},
+					Messages: []string{"msg1", "msg2"},
+					Success:  false,
 				},
 			},
 		},
@@ -692,15 +702,17 @@ deny[res] {
 	}
 }`,
 			},
-			checks: []v1alpha1.Check{
+			results: []policy.Result{
 				{
-					ID:          "KSV013",
-					Title:       "Image tag ':latest' used",
-					Description: "It is best to avoid using the ':latest' image tag when deploying containers in production. Doing so makes it hard to track which version of the image is running, and hard to roll back the version.",
-					Severity:    "LOW",
-					Category:    "Kubernetes Security Check",
-					Messages:    []string{"msg1", "msg2"},
-					Success:     false,
+					Metadata: policy.Metadata{
+						ID:          "KSV013",
+						Title:       "Image tag ':latest' used",
+						Description: "It is best to avoid using the ':latest' image tag when deploying containers in production. Doing so makes it hard to track which version of the image is running, and hard to roll back the version.",
+						Severity:    "LOW",
+						Type:        "Kubernetes Security Check",
+					},
+					Messages: []string{"msg1", "msg2"},
+					Success:  false,
 				},
 			},
 		},
@@ -714,7 +726,7 @@ deny[res] {
 				g.Expect(err).To(MatchError(tc.expectedError))
 			} else {
 				g.Expect(err).ToNot(HaveOccurred())
-				g.Expect(checks).To(Equal(tc.checks))
+				g.Expect(checks).To(Equal(tc.results))
 			}
 		})
 	}
@@ -923,11 +935,11 @@ func TestNewMetadata(t *testing.T) {
 
 }
 
-func TestNewResult(t *testing.T) {
+func TestNewMessage(t *testing.T) {
 	testCases := []struct {
 		name           string
 		values         map[string]interface{}
-		expectedResult policy.Result
+		expectedResult string
 		expectedError  string
 	}{
 		{
@@ -959,16 +971,14 @@ func TestNewResult(t *testing.T) {
 			values: map[string]interface{}{
 				"msg": "some message",
 			},
-			expectedResult: policy.Result{
-				Message: "some message",
-			},
+			expectedResult: "some message",
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewGomegaWithT(t)
-			result, err := policy.NewResult(tc.values)
+			result, err := policy.NewMessage(tc.values)
 			if tc.expectedError != "" {
 				g.Expect(err).To(MatchError(tc.expectedError))
 			} else {