Repository: aquasecurity/starboard
Pull request #230 (2020-10-25 20:28:28)

refactor: Save VulnerabilityReports method
Signed-off-by: Daniel Pacak <pacak.daniel@gmail.com>

codecov[bot]: # [Codecov](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=h1) Report
> Merging [#230](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=desc) into [master](https://codecov.io/gh/aquasecurity/starboard/commit/8a909afbe5da6873f63da6eb1d31e413016170ca?el=desc) will **decrease** coverage by `16.88%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/aquasecurity/starboard/pull/230/graphs/tree.svg?width=650&height=150&src=pr&token=qqTGKAJ0Wb)](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           master     #230       +/-   ##
===========================================
- Coverage   35.16%   18.28%   -16.89%     
===========================================
  Files          40       40               
  Lines        1945     1903       -42     
===========================================
- Hits          684      348      -336     
- Misses       1154     1523      +369     
+ Partials      107       32       -75     
```


| [Impacted Files](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/vulnerabilityreport/store.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL3Z1bG5lcmFiaWxpdHlyZXBvcnQvc3RvcmUuZ28=) | `0.00% <0.00%> (ø)` | |
| [pkg/cmd/get.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9nZXQuZ28=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/cmd/find.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9maW5kLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/cmd/version.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC92ZXJzaW9uLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [pkg/cmd/root.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9yb290Lmdv) | `3.22% <0.00%> (-90.33%)` | :arrow_down: |
| [pkg/kube/runnable\_job.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2t1YmUvcnVubmFibGVfam9iLmdv) | `0.00% <0.00%> (-77.15%)` | :arrow_down: |
| [pkg/cmd/init.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9pbml0Lmdv) | `0.00% <0.00%> (-76.93%)` | :arrow_down: |
| [pkg/cmd/find\_vulnerabilities.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9maW5kX3Z1bG5lcmFiaWxpdGllcy5nbw==) | `0.00% <0.00%> (-68.97%)` | :arrow_down: |
| [pkg/cmd/cleanup.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9jbGVhbnVwLmdv) | `0.00% <0.00%> (-62.50%)` | :arrow_down: |
| [pkg/cmd/kube\_bench.go](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree#diff-cGtnL2NtZC9rdWJlX2JlbmNoLmdv) | `0.00% <0.00%> (-53.66%)` | :arrow_down: |
| ... and [9 more](https://codecov.io/gh/aquasecurity/starboard/pull/230/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=footer). Last update [8a909af...a3b1a31](https://codecov.io/gh/aquasecurity/starboard/pull/230?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/operator/controller/job/job_controller.go b/pkg/operator/controller/job/job_controller.go
index 096a0ba..8d680b5 100644
--- a/pkg/operator/controller/job/job_controller.go
+++ b/pkg/operator/controller/job/job_controller.go
@@ -4,6 +4,10 @@ import (
 	"context"
 	"fmt"
 
+	appsv1 "k8s.io/api/apps/v1"
+	"k8s.io/api/batch/v1beta1"
+	"k8s.io/apimachinery/pkg/types"
+
 	"github.com/aquasecurity/starboard/pkg/vulnerabilityreport"
 
 	"github.com/aquasecurity/starboard/pkg/operator/resources"
@@ -103,12 +107,18 @@ func (r *JobController) processCompleteScanJob(ctx context.Context, scanJob *bat
 		return r.Client.Delete(ctx, scanJob, client.PropagationPolicy(metav1.DeletePropagationBackground))
 	}
 
+	owner, err := r.getRuntimeObjectFor(ctx, workload)
+	if err != nil {
+		return err
+	}
+
 	pod, err := r.GetPodControlledBy(ctx, scanJob)
 	if err != nil {
 		return fmt.Errorf("getting pod controlled by %s/%s: %w", scanJob.Namespace, scanJob.Name, err)
 	}
 
-	vulnerabilityReports := make(map[string]v1alpha1.VulnerabilityScanResult)
+	var vulnerabilityReports []v1alpha1.VulnerabilityReport
+
 	for _, container := range pod.Spec.Containers {
 		logsReader, err := r.LogsReader.GetLogsForPod(ctx, client.ObjectKey{Namespace: pod.Namespace, Name: pod.Name}, &corev1.PodLogOptions{
 			Container: container.Name,
@@ -117,15 +127,34 @@ func (r *JobController) processCompleteScanJob(ctx context.Context, scanJob *bat
 		if err != nil {
 			return fmt.Errorf("getting logs for pod %s/%s: %w", pod.Namespace, pod.Name, err)
 		}
-		vulnerabilityReports[container.Name], err = r.Scanner.ParseVulnerabilityScanResult(containerImages[container.Name], logsReader)
+		scanResult, err := r.Scanner.ParseVulnerabilityScanResult(containerImages[container.Name], logsReader)
 		if err != nil {
 			return err
 		}
 		_ = logsReader.Close()
+
+		reportName, err := vulnerabilityreport.NewNameBuilder(r.Scheme).
+			Owner(owner).
+			Container(container.Name).Get()
+		if err != nil {
+			return err
+		}
+
+		report, err := vulnerabilityreport.NewBuilder(r.Scheme).
+			Owner(owner).
+			Container(container.Name).
+			ReportName(reportName).
+			ScanResult(scanResult).
+			PodSpecHash(hash).Get()
+		if err != nil {
+			return err
+		}
+
+		vulnerabilityReports = append(vulnerabilityReports, report)
 	}
 
 	log.Info("Writing VulnerabilityReports", "owner", workload)
-	err = r.Store.SaveVulnerabilityReports(ctx, vulnerabilityReports, workload, hash)
+	err = r.Store.Save(ctx, vulnerabilityReports)
 	if err != nil {
 		return fmt.Errorf("writing vulnerability reports: %w", err)
 	}
@@ -133,6 +162,35 @@ func (r *JobController) processCompleteScanJob(ctx context.Context, scanJob *bat
 	return r.Client.Delete(ctx, scanJob, client.PropagationPolicy(metav1.DeletePropagationBackground))
 }
 
+func (r *JobController) getRuntimeObjectFor(ctx context.Context, workload kube.Object) (metav1.Object, error) {
+	var obj runtime.Object
+	switch workload.Kind {
+	case kube.KindPod:
+		obj = &corev1.Pod{}
+	case kube.KindReplicaSet:
+		obj = &appsv1.ReplicaSet{}
+	case kube.KindReplicationController:
+		obj = &corev1.ReplicationController{}
+	case kube.KindDeployment:
+		obj = &appsv1.Deployment{}
+	case kube.KindStatefulSet:
+		obj = &appsv1.StatefulSet{}
+	case kube.KindDaemonSet:
+		obj = &appsv1.DaemonSet{}
+	case kube.KindCronJob:
+		obj = &v1beta1.CronJob{}
+	case kube.KindJob:
+		obj = &batchv1.Job{}
+	default:
+		return nil, fmt.Errorf("unknown workload kind: %s", workload.Kind)
+	}
+	err := r.Client.Get(ctx, types.NamespacedName{Name: workload.Name, Namespace: workload.Namespace}, obj)
+	if err != nil {
+		return nil, err
+	}
+	return obj.(metav1.Object), nil
+}
+
 func (r *JobController) GetPodControlledBy(ctx context.Context, job *batchv1.Job) (*corev1.Pod, error) {
 	controllerUID, ok := job.Spec.Selector.MatchLabels["controller-uid"]
 	if !ok {
diff --git a/pkg/vulnerabilityreport/store.go b/pkg/vulnerabilityreport/store.go
index 4dd2614..99a8e09 100644
--- a/pkg/vulnerabilityreport/store.go
+++ b/pkg/vulnerabilityreport/store.go
@@ -2,32 +2,19 @@ package vulnerabilityreport
 
 import (
 	"context"
-	"fmt"
 	"reflect"
 
 	"k8s.io/apimachinery/pkg/api/errors"
-	ctrl "sigs.k8s.io/controller-runtime"
-
-	batchv1 "k8s.io/api/batch/v1"
-	"k8s.io/api/batch/v1beta1"
-	corev1 "k8s.io/api/core/v1"
-
-	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 
 	starboardv1alpha1 "github.com/aquasecurity/starboard/pkg/apis/aquasecurity/v1alpha1"
 	"github.com/aquasecurity/starboard/pkg/kube"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-var (
-	log = ctrl.Log.WithName("store")
-)
-
 type StoreInterface interface {
-	SaveVulnerabilityReports(ctx context.Context, reports WorkloadVulnerabilities, owner kube.Object, hash string) error
+	Save(context.Context, []starboardv1alpha1.VulnerabilityReport) error
 	GetVulnerabilityReportsByOwnerAndHash(ctx context.Context, owner kube.Object, hash string) (WorkloadVulnerabilities, error)
 	HasVulnerabilityReports(ctx context.Context, owner kube.Object, hash string, containerImages kube.ContainerImages) (bool, error)
 }
@@ -44,14 +31,9 @@ func NewStore(client client.Client, scheme *runtime.Scheme) StoreInterface {
 	}
 }
 
-func (s *store) SaveVulnerabilityReports(ctx context.Context, reports WorkloadVulnerabilities, owner kube.Object, hash string) error {
-	ownerObject, err := s.getRuntimeObjectFor(ctx, owner)
-	if err != nil {
-		return err
-	}
-
-	for container, report := range reports {
-		err := s.createOrUpdate(ctx, container, report, ownerObject, hash)
+func (s *store) Save(ctx context.Context, reports []starboardv1alpha1.VulnerabilityReport) error {
+	for _, report := range reports {
+		err := s.createOrUpdate(ctx, report)
 		if err != nil {
 			return err
 		}
@@ -59,47 +41,23 @@ func (s *store) SaveVulnerabilityReports(ctx context.Context, reports WorkloadVu
 	return nil
 }
 
-func (s *store) createOrUpdate(ctx context.Context, containerName string, report starboardv1alpha1.VulnerabilityScanResult, owner metav1.Object, hash string) error {
-	namespace := owner.GetNamespace()
-
-	reportName, err := NewNameBuilder(s.scheme).
-		Owner(owner).
-		Container(containerName).Get()
-
-	if err != nil {
-		return err
-	}
-
-	vulnerabilityReport := starboardv1alpha1.VulnerabilityReport{}
-
-	err = s.client.Get(ctx, types.NamespacedName{Name: reportName, Namespace: namespace}, &vulnerabilityReport)
+func (s *store) createOrUpdate(ctx context.Context, report starboardv1alpha1.VulnerabilityReport) error {
+	var existing starboardv1alpha1.VulnerabilityReport
+	err := s.client.Get(ctx, types.NamespacedName{
+		Name:      report.Name,
+		Namespace: report.Namespace,
+	}, &existing)
 
 	if err == nil {
-		log.Info("Updating VulnerabilityReport",
-			"report", fmt.Sprintf("%s/%s", namespace, reportName),
-			"hash", hash)
+		copied := existing.DeepCopy()
+		copied.Labels = report.Labels
+		copied.Report = report.Report
 
-		// Do not modify the object that might be cached.
-		cloned := vulnerabilityReport.DeepCopy()
-		cloned.Labels[kube.LabelPodSpecHash] = hash
-		cloned.Report = report
-
-		return s.client.Update(ctx, cloned)
+		return s.client.Update(ctx, copied)
 	}
 
 	if errors.IsNotFound(err) {
-		log.Info("Creating VulnerabilityReport",
-			"report", fmt.Sprintf("%s/%s", namespace, reportName),
-			"hash", hash)
-
-		vulnerabilityReport, err = NewBuilder(s.scheme).
-			Owner(owner).
-			Container(containerName).
-			ScanResult(report).
-			ReportName(reportName).
-			PodSpecHash(hash).Get()
-
-		return s.client.Create(ctx, &vulnerabilityReport)
+		return s.client.Create(ctx, &report)
 	}
 
 	return err
@@ -127,35 +85,6 @@ func (s *store) GetVulnerabilityReportsByOwnerAndHash(ctx context.Context, workl
 	return reports, nil
 }
 
-func (s *store) getRuntimeObjectFor(ctx context.Context, workload kube.Object) (metav1.Object, error) {
-	var obj runtime.Object
-	switch workload.Kind {
-	case kube.KindPod:
-		obj = &corev1.Pod{}
-	case kube.KindReplicaSet:
-		obj = &appsv1.ReplicaSet{}
-	case kube.KindReplicationController:
-		obj = &corev1.ReplicationController{}
-	case kube.KindDeployment:
-		obj = &appsv1.Deployment{}
-	case kube.KindStatefulSet:
-		obj = &appsv1.StatefulSet{}
-	case kube.KindDaemonSet:
-		obj = &appsv1.DaemonSet{}
-	case kube.KindCronJob:
-		obj = &v1beta1.CronJob{}
-	case kube.KindJob:
-		obj = &batchv1.Job{}
-	default:
-		return nil, fmt.Errorf("unknown workload kind: %s", workload.Kind)
-	}
-	err := s.client.Get(ctx, types.NamespacedName{Name: workload.Name, Namespace: workload.Namespace}, obj)
-	if err != nil {
-		return nil, err
-	}
-	return obj.(metav1.Object), nil
-}
-
 func (s *store) HasVulnerabilityReports(ctx context.Context, owner kube.Object, hash string, containerImages kube.ContainerImages) (bool, error) {
 	vulnerabilityReports, err := s.GetVulnerabilityReportsByOwnerAndHash(ctx, owner, hash)
 	if err != nil {


Test patch:
diff --git a/pkg/vulnerabilityreport/store_test.go b/pkg/vulnerabilityreport/store_test.go
index d36e439..5d3e4db 100644
--- a/pkg/vulnerabilityreport/store_test.go
+++ b/pkg/vulnerabilityreport/store_test.go
@@ -1,1 +1,165 @@
 package vulnerabilityreport_test
+
+import (
+	"context"
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/types"
+	"testing"
+
+	"github.com/aquasecurity/starboard/pkg/apis/aquasecurity/v1alpha1"
+	"github.com/aquasecurity/starboard/pkg/vulnerabilityreport"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func TestStore_Save(t *testing.T) {
+
+	kubernetesScheme := scheme.Scheme
+
+	t.Run("Should create VulnerabilityReports", func(t *testing.T) {
+
+		client := fake.NewFakeClientWithScheme(kubernetesScheme)
+		store := vulnerabilityreport.NewStore(client, kubernetesScheme)
+		err := store.Save(context.TODO(), []v1alpha1.VulnerabilityReport{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "deployment-app1-container1",
+					Namespace: "qa",
+					Labels: map[string]string{
+						"starboard.resource.kind":      "Deployment",
+						"starboard.resource.name":      "app1",
+						"starboard.resource.namespace": "qa",
+						"starboard.container.name":     "container1",
+						"pod-spec-hash":                "h1",
+					},
+				},
+			},
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "deployment-app1-container2",
+					Namespace: "qa",
+					Labels: map[string]string{
+						"starboard.resource.kind":      "Deployment",
+						"starboard.resource.name":      "app1",
+						"starboard.resource.namespace": "qa",
+						"starboard.container.name":     "container2",
+						"pod-spec-hash":                "h1",
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+
+	})
+
+	t.Run("Should update VulnerabilityReports", func(t *testing.T) {
+		client := fake.NewFakeClientWithScheme(kubernetesScheme,
+			&v1alpha1.VulnerabilityReport{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "deployment-app1-container1",
+					Namespace: "qa",
+					Labels: map[string]string{
+						"starboard.resource.kind":      "Deployment",
+						"starboard.resource.name":      "app1",
+						"starboard.resource.namespace": "qa",
+						"starboard.container.name":     "container1",
+						"pod-spec-hash":                "h1",
+					},
+				},
+			},
+			&v1alpha1.VulnerabilityReport{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "deployment-app1-container2",
+					Namespace: "qa",
+					Labels: map[string]string{
+						"starboard.resource.kind":      "Deployment",
+						"starboard.resource.name":      "app1",
+						"starboard.resource.namespace": "qa",
+						"starboard.container.name":     "container2",
+						"pod-spec-hash":                "h2",
+					},
+				},
+			})
+		store := vulnerabilityreport.NewStore(client, kubernetesScheme)
+		err := store.Save(context.TODO(), []v1alpha1.VulnerabilityReport{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "deployment-app1-container1",
+					Namespace: "qa",
+					Labels: map[string]string{
+						"starboard.resource.kind":      "Deployment",
+						"starboard.resource.name":      "app1",
+						"starboard.resource.namespace": "qa",
+						"starboard.container.name":     "container1",
+						"pod-spec-hash":                "h2",
+					},
+				},
+			},
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "deployment-app1-container2",
+					Namespace: "qa",
+					Labels: map[string]string{
+						"starboard.resource.kind":      "Deployment",
+						"starboard.resource.name":      "app1",
+						"starboard.resource.namespace": "qa",
+						"starboard.container.name":     "container2",
+						"pod-spec-hash":                "h2",
+					},
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		var found v1alpha1.VulnerabilityReport
+		err = client.Get(context.TODO(), types.NamespacedName{
+			Namespace: "qa",
+			Name:      "deployment-app1-container1",
+		}, &found)
+		require.NoError(t, err)
+		assert.Equal(t, v1alpha1.VulnerabilityReport{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "aquasecurity.github.io/v1alpha1",
+				Kind:       "VulnerabilityReport",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				ResourceVersion: "1",
+				Name:            "deployment-app1-container1",
+				Namespace:       "qa",
+				Labels: map[string]string{
+					"starboard.resource.kind":      "Deployment",
+					"starboard.resource.name":      "app1",
+					"starboard.resource.namespace": "qa",
+					"starboard.container.name":     "container1",
+					"pod-spec-hash":                "h2",
+				},
+			},
+		}, found)
+
+		err = client.Get(context.TODO(), types.NamespacedName{
+			Namespace: "qa",
+			Name:      "deployment-app1-container2",
+		}, &found)
+		require.NoError(t, err)
+		assert.Equal(t, v1alpha1.VulnerabilityReport{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "aquasecurity.github.io/v1alpha1",
+				Kind:       "VulnerabilityReport",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				ResourceVersion: "1",
+				Name:            "deployment-app1-container2",
+				Namespace:       "qa",
+				Labels: map[string]string{
+					"starboard.resource.kind":      "Deployment",
+					"starboard.resource.name":      "app1",
+					"starboard.resource.namespace": "qa",
+					"starboard.container.name":     "container2",
+					"pod-spec-hash":                "h2",
+				},
+			},
+		}, found)
+	})
+}