Merge pull request #532 from nirmata/bug

fix mutation patches

pkg/engine/mutation.go

@@ -26,7 +26,7 @@ func Mutate(policyContext PolicyContext) (response EngineResponse) {
 	defer func() {
 		response.PolicyResponse.ProcessingTime = time.Since(startTime)
 		glog.V(4).Infof("finished applying mutation rules policy %v (%v)", policy.Name, response.PolicyResponse.ProcessingTime)
-		glog.V(4).Infof("Mutation Rules appplied succesfully count %v for policy %q", response.PolicyResponse.RulesAppliedCount, policy.Name)
+		glog.V(4).Infof("Mutation Rules appplied count %v for policy %q", response.PolicyResponse.RulesAppliedCount, policy.Name)
 	}()
 	incrementAppliedRuleCount := func() {
 		// rules applied succesfully count

pkg/engine/overlay.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"reflect"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/golang/glog"
@@ -31,6 +32,12 @@ func processOverlay(rule kyverno.Rule, resource unstructured.Unstructured) (resp
 	// resource does not satisfy the overlay pattern, we don't apply this rule
 	if !reflect.DeepEqual(overlayerr, overlayError{}) {
 		switch overlayerr.statusCode {
+		// condition key is not present in the resource, don't apply this rule
+		// consider as success
+		case conditionNotPresent:
+			glog.V(3).Infof("Resource %s/%s/%s: %s", resource.GetKind(), resource.GetNamespace(), resource.GetName(), overlayerr.ErrorMsg())
+			response.Success = true
+			return response, resource
 		// conditions are not met, don't apply this rule
 		// consider as failure
 		case conditionFailure:
@@ -71,7 +78,7 @@ func processOverlay(rule kyverno.Rule, resource unstructured.Unstructured) (resp
 	patchResource, err = ApplyPatches(resourceRaw, patches)
 	if err != nil {
 		msg := fmt.Sprintf("failed to apply JSON patches: %v", err)
-		glog.V(2).Info(msg)
+		glog.V(2).Infof("%s, patches=%s", msg, string(JoinPatches(patches)))
 		response.Success = false
 		response.Message = msg
 		return response, resource
@@ -95,7 +102,13 @@ func processOverlay(rule kyverno.Rule, resource unstructured.Unstructured) (resp
 
 func processOverlayPatches(resource, overlay interface{}) ([][]byte, overlayError) {
 	if path, overlayerr := meetConditions(resource, overlay); !reflect.DeepEqual(overlayerr, overlayError{}) {
-		if overlayerr.statusCode == conditionFailure {
+		switch overlayerr.statusCode {
+		// anchor key does not exist in the resource, skip applying policy
+		case conditionNotPresent:
+			glog.V(4).Infof("Mutate rule: skip applying policy: %v at %s", overlayerr, path)
+			return nil, newOverlayError(overlayerr.statusCode, fmt.Sprintf("policy not applied: %v at %s", overlayerr.ErrorMsg(), path))
+		// anchor key is not satisfied in the resource, skip applying policy
+		case conditionFailure:
 			// anchor key is not satisfied in the resource, skip applying policy
 			glog.V(4).Infof("Mutate rule: failed to validate condition at %s, err: %v", path, overlayerr)
 			return nil, newOverlayError(overlayerr.statusCode, fmt.Sprintf("Conditions are not met at %s, %v", path, overlayerr))
@@ -333,10 +346,7 @@ func processSubtree(overlay interface{}, path string, op string) ([]byte, error)
 		path = path[:len(path)-1]
 	}
 
-	if path == "" {
-		path = "/"
-	}
-
+	path = preparePath(path)
 	value := prepareJSONValue(overlay)
 	patchStr := fmt.Sprintf(`{ "op": "%s", "path": "%s", "value": %s }`, op, path, value)
 
@@ -350,6 +360,20 @@ func processSubtree(overlay interface{}, path string, op string) ([]byte, error)
 	return []byte(patchStr), nil
 }
 
+func preparePath(path string) string {
+	if path == "" {
+		path = "/"
+	}
+
+	annPath := "/metadata/annotations/"
+	// escape slash in annotation patch
+	if strings.Contains(path, annPath) {
+		p := path[len(annPath):]
+		path = annPath + strings.ReplaceAll(p, "/", "~1")
+	}
+	return path
+}
+
 // converts overlay to JSON string to be inserted into the JSON Patch
 func prepareJSONValue(overlay interface{}) string {
 	var err error

pkg/engine/overlayCondition.go

@@ -98,7 +98,7 @@ func validateConditionAnchorMap(resourceMap, anchors map[string]interface{}, pat
 			}
 		} else {
 			// noAnchorKey doesn't exist in resource
-			continue
+			return curPath, newOverlayError(conditionNotPresent, fmt.Sprintf("resource field is not present %s", noAnchorKey))
 		}
 	}
 	return "", overlayError{}
@@ -156,11 +156,13 @@ func validateNonAnchorOverlayMap(resourceMap, overlayWithoutAnchor map[string]in
 		curPath := path + key + "/"
 		resourceValue, ok := resourceMap[key]
 		if !ok {
-			// policy: 		"(image)": "*:latest",
-			//				"imagePullPolicy": "IfNotPresent",
-			// resource:	"(image)": "*:latest",
-			// the above case should be allowed
-			continue
+			if !hasNestedAnchors(overlayValue) {
+				// policy: 		"(image)": "*:latest",
+				//				"imagePullPolicy": "IfNotPresent",
+				// resource:	"(image)": "*:latest",
+				// the above case should be allowed
+				continue
+			}
 		}
 		if newPath, err := checkConditions(resourceValue, overlayValue, curPath); !reflect.DeepEqual(err, overlayError{}) {
 			return newPath, err

pkg/engine/overlayError.go

@@ -6,6 +6,7 @@ type codeKey int
 
 const (
 	conditionFailure codeKey = iota
+	conditionNotPresent
 	overlayFailure
 )
 

pkg/testrunner/scenario.go

@@ -250,7 +250,8 @@ func validateResponse(t *testing.T, er engine.PolicyResponse, expected engine.Po
 
 	// rules
 	if len(er.Rules) != len(expected.Rules) {
-		t.Error("rule count: error")
+		t.Errorf("rule count error, er.Rules=%d, expected.Rules=%d", len(er.Rules), len(expected.Rules))
+		return
 	}
 	if len(er.Rules) == len(expected.Rules) {
 		// if there are rules being applied then we compare the rule response

pkg/testrunner/testrunner_test.go

@@ -112,6 +112,10 @@ func Test_add_safe_to_evict_annotation2(t *testing.T) {
 	testScenario(t, "test/scenarios/samples/best_practices/add_safe_to_evict2.yaml")
 }
 
+func Test_add_safe_to_evict_annotation3(t *testing.T) {
+	testScenario(t, "test/scenarios/samples/best_practices/add_safe_to_evict3.yaml")
+}
+
 func Test_validate_restrict_automount_sa_token_pass(t *testing.T) {
 	testScenario(t, "test/scenarios/samples/more/restrict_automount_sa_token.yaml")
 }

test/output/pod-with-default-volume.yaml

@@ -0,0 +1,17 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-with-default-volume
+spec:
+  containers:
+  - image: k8s.gcr.io/test-webserver
+    name: test-container
+    volumeMounts:
+    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
+      name: default-token-wkknl
+      readOnly: true
+  volumes:
+  - name: default-token-wkknl
+    secret:
+      defaultMode: 420
+      secretName: default-token-wkknl

test/resources/pod-with-default-volume.yaml

@@ -0,0 +1,17 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-with-default-volume
+spec:
+  containers:
+  - image: k8s.gcr.io/test-webserver
+    name: test-container
+    volumeMounts:
+    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
+      name: default-token-wkknl
+      readOnly: true
+  volumes:
+  - name: default-token-wkknl
+    secret:
+      defaultMode: 420
+      secretName: default-token-wkknl

test/scenarios/samples/best_practices/add_safe_to_evict2.yaml

@@ -13,7 +13,7 @@ expected:
         namespace: ''
         name: pod-with-hostpath
       rules:
-        - name: annotate-empty-dir
+        - name: annotate-host-path
           type: Mutation
           success: true
           message: "successfully processed overlay"

test/scenarios/samples/best_practices/add_safe_to_evict3.yaml

@@ -0,0 +1,15 @@
+# file path is relative to project root
+input:
+  policy: samples/best_practices/add_safe_to_evict.yaml
+  resource: test/resources/pod-with-default-volume.yaml
+expected:
+  mutation:
+    patchedresource: test/resources/pod-with-default-volume.yaml
+    policyresponse:
+      policy: add-safe-to-evict
+      resource:
+        kind: Pod
+        apiVersion: v1
+        namespace: ''
+        name: pod-with-default-volume
+      rules: