Repository: FairwindsOps/polaris
Pull request #278 (2020-04-30 19:39:05)

Improve performance
Improves the performance of Polaris by caching results and relying on Lists for the most part instead of get to find parent objects.

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/FairwindsOps/polaris?pullRequest=278) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our [Contributor License Agreement](https://cla-assistant.io/FairwindsOps/polaris?pullRequest=278) before we can accept your contribution.<br/>**2** out of **3** committers have signed the CLA.<br/><br/>:white_check_mark: rbren<br/>:white_check_mark: baderbuddy<br/>:x: Robert Brennan<br/><hr/>**Robert Brennan** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/FairwindsOps/polaris?pullRequest=278) it.</sub>
codecov[bot]: # [Codecov](https://codecov.io/gh/FairwindsOps/polaris/pull/278?src=pr&el=h1) Report
> Merging [#278](https://codecov.io/gh/FairwindsOps/polaris/pull/278?src=pr&el=desc) into [master](https://codecov.io/gh/FairwindsOps/polaris/commit/4cf3d15af008ed768155d502042a933719e289ae&el=desc) will **decrease** coverage by `2.81%`.
> The diff coverage is `23.52%`.

[![Impacted file tree graph](https://codecov.io/gh/FairwindsOps/polaris/pull/278/graphs/tree.svg?width=650&height=150&src=pr&token=RR1ySHa61e)](https://codecov.io/gh/FairwindsOps/polaris/pull/278?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
- Coverage   55.29%   52.48%   -2.82%     
==========================================
  Files          12       12              
  Lines         604      644      +40     
==========================================
+ Hits          334      338       +4     
- Misses        237      273      +36     
  Partials       33       33              
```


| [Impacted Files](https://codecov.io/gh/FairwindsOps/polaris/pull/278?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/kube/workload.go](https://codecov.io/gh/FairwindsOps/polaris/pull/278/diff?src=pr&el=tree#diff-cGtnL2t1YmUvd29ya2xvYWQuZ28=) | `22.80% <18.75%> (-9.46%)` | :arrow_down: |
| [pkg/kube/resources.go](https://codecov.io/gh/FairwindsOps/polaris/pull/278/diff?src=pr&el=tree#diff-cGtnL2t1YmUvcmVzb3VyY2VzLmdv) | `65.21% <31.57%> (-5.54%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/FairwindsOps/polaris/pull/278?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/FairwindsOps/polaris/pull/278?src=pr&el=footer). Last update [4cf3d15...540c0ff](https://codecov.io/gh/FairwindsOps/polaris/pull/278?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/kube/resources.go b/pkg/kube/resources.go
index ebd2e03f..235f4dcf 100644
--- a/pkg/kube/resources.go
+++ b/pkg/kube/resources.go
@@ -2,6 +2,7 @@ package kube
 
 import (
 	"bytes"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -14,6 +15,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	k8sYaml "k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
@@ -143,6 +145,9 @@ func CreateResourceProviderFromAPI(kube kubernetes.Interface, clusterName string
 		return nil, err
 	}
 	restMapper := restmapper.NewDiscoveryRESTMapper(resources)
+
+	objectCache := map[string]metav1.Object{}
+
 	api := ResourceProvider{
 		ServerVersion: serverVersion.Major + "." + serverVersion.Minor,
 		SourceType:    "Cluster",
@@ -150,13 +155,32 @@ func CreateResourceProviderFromAPI(kube kubernetes.Interface, clusterName string
 		CreationTime:  time.Now(),
 		Nodes:         nodes.Items,
 		Namespaces:    namespaces.Items,
-		Controllers:   LoadControllers(pods.Items, dynamic, &restMapper),
+		Controllers:   LoadControllers(pods.Items, dynamic, &restMapper, objectCache),
 	}
 	return &api, nil
 }
 
+func cacheAllObjectsOfKind(dynamicClient dynamic.Interface, groupVersionResource schema.GroupVersionResource, objectCache map[string]metav1.Object) error {
+	objects, err := dynamicClient.Resource(groupVersionResource).Namespace("").List(metav1.ListOptions{})
+	if err != nil {
+		logrus.Warnf("Error retrieving parent object API %s and Kind %s because of error: %v ", groupVersionResource.Version, groupVersionResource.Resource, err)
+		return err
+	}
+	for idx, object := range objects.Items {
+
+		key := fmt.Sprintf("%s/%s/%s", object.GetKind(), object.GetNamespace(), object.GetName())
+		objMeta, err := meta.Accessor(&objects.Items[idx])
+		if err != nil {
+			logrus.Warnf("Error converting object to meta object %s %v", object.GetName(), err)
+			return err
+		}
+		objectCache[key] = objMeta
+	}
+	return nil
+}
+
 // LoadControllers loads a list of controllers from the kubeResources Pods
-func LoadControllers(pods []corev1.Pod, dynamicClientPointer *dynamic.Interface, restMapperPointer *meta.RESTMapper) []GenericWorkload {
+func LoadControllers(pods []corev1.Pod, dynamicClientPointer *dynamic.Interface, restMapperPointer *meta.RESTMapper, objectCache map[string]metav1.Object) []GenericWorkload {
 	interfaces := []GenericWorkload{}
 	deduped := map[string]corev1.Pod{}
 	for _, pod := range pods {
@@ -168,7 +192,7 @@ func LoadControllers(pods []corev1.Pod, dynamicClientPointer *dynamic.Interface,
 		deduped[pod.ObjectMeta.Namespace+"/"+owners[0].Kind+"/"+owners[0].Name] = pod
 	}
 	for _, pod := range deduped {
-		interfaces = append(interfaces, NewGenericWorkload(pod, dynamicClientPointer, restMapperPointer))
+		interfaces = append(interfaces, NewGenericWorkload(pod, dynamicClientPointer, restMapperPointer, objectCache))
 	}
 	return deduplicateControllers(interfaces)
 }
@@ -219,7 +243,7 @@ func addResourceFromString(contents string, resources *ResourceProvider) error {
 	} else if resource.Kind == "Pod" {
 		pod := corev1.Pod{}
 		err = decoder.Decode(&pod)
-		resources.Controllers = append(resources.Controllers, NewGenericWorkload(pod, nil, nil))
+		resources.Controllers = append(resources.Controllers, NewGenericWorkloadFromPod(pod))
 	} else {
 		yamlNode := make(map[string]interface{})
 		err = yaml.Unmarshal(contentBytes, &yamlNode)
@@ -240,7 +264,7 @@ func addResourceFromString(contents string, resources *ResourceProvider) error {
 		decoder := k8sYaml.NewYAMLOrJSONDecoder(bytes.NewReader(marshaledYaml), 1000)
 		pod := corev1.Pod{}
 		err = decoder.Decode(&pod)
-		newController := NewGenericWorkload(pod, nil, nil)
+		newController := NewGenericWorkloadFromPod(pod)
 		newController.Kind = resource.Kind
 		resources.Controllers = append(resources.Controllers, newController)
 	}
diff --git a/pkg/kube/workload.go b/pkg/kube/workload.go
index 40fe5ba7..d847f0a8 100644
--- a/pkg/kube/workload.go
+++ b/pkg/kube/workload.go
@@ -18,17 +18,21 @@ type GenericWorkload struct {
 	ObjectMeta kubeAPIMetaV1.Object
 }
 
-// NewGenericWorkload builds a new workload for a given Pod
-func NewGenericWorkload(originalResource kubeAPICoreV1.Pod, dynamicClientPointer *dynamic.Interface, restMapperPointer *meta.RESTMapper) GenericWorkload {
+// NewGenericWorkloadFromPod builds a new workload for a given Pod without looking at parents
+func NewGenericWorkloadFromPod(originalResource kubeAPICoreV1.Pod) GenericWorkload {
 	workload := GenericWorkload{}
 	workload.PodSpec = originalResource.Spec
 	workload.ObjectMeta = originalResource.ObjectMeta.GetObjectMeta()
 	workload.Kind = "Pod"
-	fmt.Println("get workload", workload.ObjectMeta.GetNamespace(), workload.ObjectMeta.GetName())
+	return workload
+}
 
-	if dynamicClientPointer == nil || restMapperPointer == nil {
-		return workload
-	}
+// NewGenericWorkload builds a new workload for a given Pod
+func NewGenericWorkload(originalResource kubeAPICoreV1.Pod, dynamicClientPointer *dynamic.Interface, restMapperPointer *meta.RESTMapper, objectCache map[string]kubeAPIMetaV1.Object) GenericWorkload {
+	workload := NewGenericWorkloadFromPod(originalResource)
+
+	dynamicClient := *dynamicClientPointer
+	restMapper := *restMapperPointer
 	// If an owner exists then set the name to the workload.
 	// This allows us to handle CRDs creating Workloads or DeploymentConfigs in OpenShift.
 	owners := workload.ObjectMeta.GetOwnerReferences()
@@ -41,22 +45,47 @@ func NewGenericWorkload(originalResource kubeAPICoreV1.Pod, dynamicClientPointer
 			break
 		}
 		workload.Kind = firstOwner.Kind
+		key := fmt.Sprintf("%s/%s/%s", firstOwner.Kind, workload.ObjectMeta.GetNamespace(), firstOwner.Name)
+		objMeta, ok := objectCache[key]
+		if ok {
+			workload.ObjectMeta = objMeta
+			owners = objMeta.GetOwnerReferences()
 
-		dynamicClient := *dynamicClientPointer
-		restMapper := *restMapperPointer
+			continue
+		}
 		fqKind := schema.FromAPIVersionAndKind(firstOwner.APIVersion, firstOwner.Kind)
 		mapping, err := restMapper.RESTMapping(fqKind.GroupKind(), fqKind.Version)
 		if err != nil {
 			logrus.Warnf("Error retrieving mapping %s of API %s and Kind %s because of error: %v ", firstOwner.Name, firstOwner.APIVersion, firstOwner.Kind, err)
 			return workload
 		}
+		err = cacheAllObjectsOfKind(dynamicClient, mapping.Resource, objectCache)
+		if err != nil {
+			logrus.Warnf("Error getting objects of Kind %s %v", firstOwner.Kind, err)
+			return workload
+		}
+
+		objMeta, ok = objectCache[key]
+		if ok {
+			workload.ObjectMeta = objMeta
+			owners = objMeta.GetOwnerReferences()
+
+			continue
+		} else {
+			logrus.Errorf("Cache missed again %s", key)
+		}
 		parent, err := dynamicClient.Resource(mapping.Resource).Namespace(workload.ObjectMeta.GetNamespace()).Get(firstOwner.Name, kubeAPIMetaV1.GetOptions{})
 		if err != nil {
 			logrus.Warnf("Error retrieving parent object %s of API %s and Kind %s because of error: %v ", firstOwner.Name, firstOwner.APIVersion, firstOwner.Kind, err)
 			return workload
 		}
-		objMeta, err := meta.Accessor(parent)
+		objMeta, err = meta.Accessor(parent)
+		if err != nil {
+			logrus.Warnf("Error retrieving parent metadata %s of API %s and Kind %s because of error: %v ", firstOwner.Name, firstOwner.APIVersion, firstOwner.Kind, err)
+			return workload
+		}
 		workload.ObjectMeta = objMeta
+		objectCache[key] = objMeta
 		owners = parent.GetOwnerReferences()
 	}
 
diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go
index 0f7b63b1..0fac0d37 100644
--- a/pkg/webhook/webhook.go
+++ b/pkg/webhook/webhook.go
@@ -104,7 +104,7 @@ func (v *Validator) handleInternal(ctx context.Context, req types.Request) (*val
 			return nil, err
 		}
 	}
-	controller := kube.NewGenericWorkload(pod, nil, nil)
+	controller := kube.NewGenericWorkloadFromPod(pod)
 	controller.Kind = req.AdmissionRequest.Kind.Kind
 	controllerResult, err := validator.ValidateController(&v.Config, controller)
 	if err != nil {


Test patch:
diff --git a/pkg/validator/container_test.go b/pkg/validator/container_test.go
index 65720909..a8aac77e 100644
--- a/pkg/validator/container_test.go
+++ b/pkg/validator/container_test.go
@@ -51,11 +51,11 @@ exemptions:
 `
 
 func getEmptyWorkload(name string) kube.GenericWorkload {
-	workload := kube.NewGenericWorkload(corev1.Pod{
+	workload := kube.NewGenericWorkloadFromPod(corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
 		},
-	}, nil, nil)
+	})
 	return workload
 }
 
@@ -919,7 +919,7 @@ func TestValidateSecurity(t *testing.T) {
 
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			workload := kube.NewGenericWorkload(corev1.Pod{Spec: *tt.pod}, nil, nil)
+			workload := kube.NewGenericWorkloadFromPod(corev1.Pod{Spec: *tt.pod})
 			results, err := applyContainerSchemaChecks(&conf.Configuration{Checks: tt.securityConf}, workload, tt.container, false)
 			if err != nil {
 				panic(err)
@@ -1063,7 +1063,7 @@ func TestValidateRunAsRoot(t *testing.T) {
 	}
 	for idx, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			workload := kube.NewGenericWorkload(corev1.Pod{Spec: *tt.pod}, nil, nil)
+			workload := kube.NewGenericWorkloadFromPod(corev1.Pod{Spec: *tt.pod})
 			results, err := applyContainerSchemaChecks(&config, workload, tt.container, false)
 			if err != nil {
 				panic(err)
@@ -1164,7 +1164,7 @@ func TestValidateResourcesEmptyContainerCPURequestsExempt(t *testing.T) {
 
 	expectedSuccesses := []ResultMessage{}
 
-	workload := kube.NewGenericWorkload(corev1.Pod{
+	workload := kube.NewGenericWorkloadFromPod(corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "foo",
 			Annotations: map[string]string{
@@ -1172,6 +1172,6 @@ func TestValidateResourcesEmptyContainerCPURequestsExempt(t *testing.T) {
 				"polaris.fairwinds.com/memoryRequestsMissing-exempt": "truthy", // Don't actually exempt this controller from memoryRequestsMissing
 			},
 		},
-	}, nil, nil)
+	})
 	testValidateWithWorkload(t, &container, &resourceConfMinimal, workload, expectedErrors, expectedWarnings, expectedSuccesses)
 }
diff --git a/pkg/validator/controller_test.go b/pkg/validator/controller_test.go
index 9d722ba9..cc9a9ed3 100644
--- a/pkg/validator/controller_test.go
+++ b/pkg/validator/controller_test.go
@@ -32,7 +32,7 @@ func TestValidateController(t *testing.T) {
 			"hostPIDSet": conf.SeverityError,
 		},
 	}
-	deployment := kube.NewGenericWorkload(test.MockPod(), nil, nil)
+	deployment := kube.NewGenericWorkloadFromPod(test.MockPod())
 	deployment.Kind = "Deployment"
 	expectedSum := CountSummary{
 		Successes: uint(2),
@@ -65,7 +65,7 @@ func TestSkipHealthChecks(t *testing.T) {
 	}
 	pod := test.MockPod()
 	pod.Spec.InitContainers = []corev1.Container{test.MockContainer("test")}
-	deployment := kube.NewGenericWorkload(pod, nil, nil)
+	deployment := kube.NewGenericWorkloadFromPod(pod)
 	deployment.Kind = "Deployment"
 	expectedSum := CountSummary{
 		Successes: uint(0),
@@ -86,7 +86,7 @@ func TestSkipHealthChecks(t *testing.T) {
 	assert.EqualValues(t, ResultSet{}, actualResult.PodResult.ContainerResults[0].Results)
 	assert.EqualValues(t, expectedResults, actualResult.PodResult.ContainerResults[1].Results)
 
-	job := kube.NewGenericWorkload(test.MockPod(), nil, nil)
+	job := kube.NewGenericWorkloadFromPod(test.MockPod())
 	job.Kind = "Job"
 	expectedSum = CountSummary{
 		Successes: uint(0),
@@ -103,7 +103,7 @@ func TestSkipHealthChecks(t *testing.T) {
 	assert.EqualValues(t, expectedSum, actualResult.GetSummary())
 	assert.EqualValues(t, expectedResults, actualResult.PodResult.ContainerResults[0].Results)
 
-	cronjob := kube.NewGenericWorkload(test.MockPod(), nil, nil)
+	cronjob := kube.NewGenericWorkloadFromPod(test.MockPod())
 	cronjob.Kind = "CronJob"
 	expectedSum = CountSummary{
 		Successes: uint(0),
@@ -129,7 +129,7 @@ func TestControllerExemptions(t *testing.T) {
 		},
 	}
 	pod := test.MockPod()
-	workload := kube.NewGenericWorkload(pod, nil, nil)
+	workload := kube.NewGenericWorkloadFromPod(pod)
 	workload.Kind = "Deployment"
 	resources := &kube.ResourceProvider{
 		Controllers: []kube.GenericWorkload{workload},
diff --git a/pkg/validator/pod_test.go b/pkg/validator/pod_test.go
index 5b103590..4c5f0454 100644
--- a/pkg/validator/pod_test.go
+++ b/pkg/validator/pod_test.go
@@ -38,7 +38,7 @@ func TestValidatePod(t *testing.T) {
 	k8s, _ := test.SetupTestAPI()
 	k8s = test.SetupAddControllers(k8s, "test")
 	p := test.MockPod()
-	deployment := kube.NewGenericWorkload(p, nil, nil)
+	deployment := kube.NewGenericWorkloadFromPod(p)
 
 	expectedSum := CountSummary{
 		Successes: uint(4),
@@ -76,7 +76,7 @@ func TestInvalidIPCPod(t *testing.T) {
 	k8s = test.SetupAddControllers(k8s, "test")
 	p := test.MockPod()
 	p.Spec.HostIPC = true
-	workload := kube.NewGenericWorkload(p, nil, nil)
+	workload := kube.NewGenericWorkloadFromPod(p)
 
 	expectedSum := CountSummary{
 		Successes: uint(3),
@@ -113,7 +113,7 @@ func TestInvalidNeworkPod(t *testing.T) {
 	k8s = test.SetupAddControllers(k8s, "test")
 	p := test.MockPod()
 	p.Spec.HostNetwork = true
-	workload := kube.NewGenericWorkload(p, nil, nil)
+	workload := kube.NewGenericWorkloadFromPod(p)
 
 	expectedSum := CountSummary{
 		Successes: uint(3),
@@ -151,7 +151,7 @@ func TestInvalidPIDPod(t *testing.T) {
 	k8s = test.SetupAddControllers(k8s, "test")
 	p := test.MockPod()
 	p.Spec.HostPID = true
-	workload := kube.NewGenericWorkload(p, nil, nil)
+	workload := kube.NewGenericWorkloadFromPod(p)
 
 	expectedSum := CountSummary{
 		Successes: uint(3),
@@ -198,7 +198,7 @@ func TestExemption(t *testing.T) {
 	p.ObjectMeta = metav1.ObjectMeta{
 		Name: "foo",
 	}
-	workload := kube.NewGenericWorkload(p, nil, nil)
+	workload := kube.NewGenericWorkloadFromPod(p)
 
 	expectedSum := CountSummary{
 		Successes: uint(3),