From 16d5570fbf4f85cb8a066580a5c37737ec8144ae Mon Sep 17 00:00:00 2001
From: Shuting Zhao <shutting06@gmail.com>
Date: Wed, 13 Nov 2019 15:45:36 -0800
Subject: [PATCH] - return detailed error message; - set pv name with old pv
 when updates the pv

---
 pkg/policyviolation/generator.go    | 10 +++++----
 pkg/policyviolation/namespacedpv.go | 32 +++++++++++++++++------------
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/pkg/policyviolation/generator.go b/pkg/policyviolation/generator.go
index 483d7dad0d..ca3429b478 100644
--- a/pkg/policyviolation/generator.go
+++ b/pkg/policyviolation/generator.go
@@ -1,6 +1,7 @@
 package policyviolation
 
 import (
+	"fmt"
 	"reflect"
 	"strconv"
 	"strings"
@@ -252,20 +253,20 @@ func createPVNew(dclient *client.Client, pv kyverno.ClusterPolicyViolation, pvLi
 	ePV, err := getExistingPVIfAny(pvLister, pv)
 	if err != nil {
 		glog.Error(err)
-		return err
+		return fmt.Errorf("failed to get existing pv on resource '%s': %v", pv.Spec.ResourceSpec.ToKey(), err)
 	}
 	if ePV == nil {
 		// Create a New PV
 		glog.V(4).Infof("creating new policy violation for policy %s & resource %s/%s/%s", pv.Spec.Policy, pv.Spec.ResourceSpec.Kind, pv.Spec.ResourceSpec.Namespace, pv.Spec.ResourceSpec.Name)
 		err := retryGetResource(dclient, pv.Spec.ResourceSpec)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to retry getting resource for policy violation %s/%s: %v", pv.Name, pv.Spec.Policy, err)
 		}
 
 		_, err = pvInterface.ClusterPolicyViolations().Create(&pv)
 		if err != nil {
 			glog.Error(err)
-			return err
+			return fmt.Errorf("failed to create cluster policy violation: %v", err)
 		}
 		glog.Infof("policy violation created for resource %v", pv.Spec.ResourceSpec)
 		return nil
@@ -276,10 +277,11 @@ func createPVNew(dclient *client.Client, pv kyverno.ClusterPolicyViolation, pvLi
 		return nil
 	}
 
+	pv.SetName(ePV.Name)
 	_, err = pvInterface.ClusterPolicyViolations().Update(&pv)
 	if err != nil {
 		glog.Error(err)
-		return err
+		return fmt.Errorf("failed to update cluster polciy violation: %v", err)
 	}
 	glog.Infof("policy violation updated for resource %v", pv.Spec.ResourceSpec)
 	return nil
diff --git a/pkg/policyviolation/namespacedpv.go b/pkg/policyviolation/namespacedpv.go
index 70a21b8239..cba216400d 100644
--- a/pkg/policyviolation/namespacedpv.go
+++ b/pkg/policyviolation/namespacedpv.go
@@ -68,23 +68,25 @@ func createNamespacedPV(dclient *dclient.Client, pvLister kyvernolister.Namespac
 		// check if there was a previous policy voilation for policy & resource combination
 		curPv, err := getExistingNamespacedPVIfAny(pvLister, newPv)
 		if err != nil {
-			glog.Error(err)
-			continue
+			return fmt.Errorf("failed to get existing namespaced pv on resource '%s': %v", newPv.Spec.ResourceSpec.ToKey(), err)
 		}
 
-		// no existing policy violation, create a new one
 		if reflect.DeepEqual(curPv, kyverno.NamespacedPolicyViolation{}) {
-			glog.V(4).Infof("creating new namespaced policy violation for policy %s & resource %s", newPv.Spec.Policy, newPv.Spec.ResourceSpec.ToKey())
+			// no existing policy violation, create a new one
+			if reflect.DeepEqual(curPv, kyverno.NamespacedPolicyViolation{}) {
+				glog.V(4).Infof("creating new namespaced policy violation for policy %s & resource %s", newPv.Spec.Policy, newPv.Spec.ResourceSpec.ToKey())
 
-			if err := retryGetResource(dclient, newPv.Spec.ResourceSpec); err != nil {
-				return err
+				if err := retryGetResource(dclient, newPv.Spec.ResourceSpec); err != nil {
+					return fmt.Errorf("failed to get resource for policy violation '%s': %v", curPv.Name, err)
+				}
+
+				if _, err := pvInterface.NamespacedPolicyViolations(newPv.Spec.ResourceSpec.Namespace).Create(&newPv); err != nil {
+					return fmt.Errorf("failed to create namespaced policy violation: %v", err)
+				}
+
+				glog.Infof("namespaced policy violation created for resource %s", newPv.Spec.ResourceSpec.ToKey())
 			}
-
-			if _, err := pvInterface.NamespacedPolicyViolations(newPv.Spec.ResourceSpec.Namespace).Create(&newPv); err != nil {
-				return err
-			}
-
-			glog.Infof("namespaced policy violation created for resource %s", newPv.Spec.ResourceSpec.ToKey())
+			return nil
 		}
 
 		// compare the policyviolation spec for existing resource if present else
@@ -94,12 +96,16 @@ func createNamespacedPV(dclient *dclient.Client, pvLister kyvernolister.Namespac
 			glog.V(4).Infof("namespaced policy violation spec %v did not change so not updating it", newPv.Spec)
 			continue
 		}
+
+		// set newPv name with curPv, as we are updating the resource itself
+		newPv.SetName(curPv.Name)
+
 		// spec changed so update the policyviolation
 		glog.V(4).Infof("creating new policy violation for policy %s & resource %s", curPv.Spec.Policy, curPv.Spec.ResourceSpec.ToKey())
 		//TODO: using a generic name, but would it be helpful to have naming convention for policy violations
 		// as we can only have one policy violation for each (policy + resource) combination
 		if _, err = pvInterface.NamespacedPolicyViolations(newPv.Spec.ResourceSpec.Namespace).Update(&newPv); err != nil {
-			return err
+			return fmt.Errorf("failed to update namespaced policy violation: %v", err)
 		}
 		glog.Infof("namespaced policy violation updated for resource %s", newPv.Spec.ResourceSpec.ToKey())
 	}