NK-31: SerializePatches decomposed to ProcessPatches and JoinPatches. Implemented new tests for ProcessPatches, used it in mutation webhook. Added options for behavior on errors while patching. Improved and refactored code for mutation webhook. Added dependency from json-patch lib.

2025-03-13 19:28:55 +00:00 · 2019-03-12 22:02:39 +02:00 · 2019-03-12 22:02:39 +02:00 · 85c84046af
commit 85c84046af
parent e2b7444271
4 changed files with 170 additions and 247 deletions
--- a/Gopkg.toml
+++ b/Gopkg.toml
@ -18,4 +18,8 @@ required = ["k8s.io/code-generator/cmd/client-gen"]

 [[constraint]]
  name = "github.com/gotestyourself/gotest.tools"
+  branch = "master"
+
+[[constraint]]
+  name = "github.com/evanphx/json-patch"
  branch = "master"
--- a/webhooks/mutation.go
+++ b/webhooks/mutation.go
@ -1,7 +1,6 @@
 package webhooks

 import (
-	"encoding/json"
 	"errors"
 	"fmt"
 	"log"
@ -40,12 +39,9 @@ func (mw *MutationWebhook) Mutate(request *v1beta1.AdmissionRequest) *v1beta1.Ad
 		return nil
 	}

-	var allPatches []types.PolicyPatch
+	var allPatches []PatchBytes
 	for _, policy := range policies {
-		stopOnError := true
-		if policy.Spec.FailurePolicy != nil && *policy.Spec.FailurePolicy == "continueOnError" {
-			stopOnError = false
-		}
+		patchingSets := getPolicyPatchingSets(policy)

 		for ruleIdx, rule := range policy.Spec.Rules {
 			err := rule.Validate()
@ -54,36 +50,29 @@ func (mw *MutationWebhook) Mutate(request *v1beta1.AdmissionRequest) *v1beta1.Ad
 				continue
 			}

-			if IsRuleApplicableToRequest(rule.Resource, request) {
-				mw.logger.Printf("Applying policy %s, rule #%d", policy.ObjectMeta.Name, ruleIdx)
-				rulePatches, err := mw.applyRule(request, rule, stopOnError)
-				// If at least one error is detected in the rule, the entire rule will not be applied:
-				// it can be changed in the future by varying the policy.Spec.FailurePolicy values.
-				if err != nil {
-					errStr := fmt.Sprintf("Unable to apply rule #%d: %s", ruleIdx, err)
-					mw.logger.Print(errStr)
-					mw.controller.LogPolicyError(policy.Name, errStr)
-					if stopOnError {
-						mw.logger.Printf("/!\\ Denying the request according to FailurePolicy spec /!\\")
-					}
-					return errorToAdmissionResponse(err, !stopOnError)
-				}
-				if rulePatches != nil {
-					allPatches = append(allPatches, rulePatches...)
-				}
+			mw.logger.Printf("Applying policy %s, rule #%d", policy.ObjectMeta.Name, ruleIdx)
+			rulePatches, err := mw.applyRule(request, rule, patchingSets)
+
+			if err != nil {
+				errStr := fmt.Sprintf("Unable to apply rule #%d: %s", ruleIdx, err)
+				mw.logger.Printf("Denying the request because of error: %s", errStr)
+				mw.controller.LogPolicyError(policy.Name, errStr)
+				return errorToAdmissionResponse(err, true)
+			}
+
+			rulePatchesProcessed, err := ProcessPatches(rulePatches, request.Object.Raw, patchingSets)
+			if rulePatches != nil {
+				allPatches = append(allPatches, rulePatchesProcessed...)
+				mw.logger.Printf("Prepared %d patches", len(rulePatchesProcessed))
+			} else {
+				mw.logger.Print("No patches prepared")
 			}
 		}
 	}

-	patchesBytes, err := SerializePatches(allPatches)
-	if err != nil {
-		mw.logger.Printf("Error occerred while serializing JSONPathch: %v", err)
-		return errorToAdmissionResponse(err, true)
-	}
-
 	return &v1beta1.AdmissionResponse{
 		Allowed: true,
-		Patch:   patchesBytes,
+		Patch:   JoinPatches(allPatches),
 		PatchType: func() *v1beta1.PatchType {
 			pt := v1beta1.PatchTypeJSONPatch
 			return &pt
@ -91,28 +80,28 @@ func (mw *MutationWebhook) Mutate(request *v1beta1.AdmissionRequest) *v1beta1.Ad
 	}
 }

-// Applies all rule to the created object and returns list of JSON patches.
-// May return nil patches if it is not necessary to create patches for requested object.
-func (mw *MutationWebhook) applyRule(request *v1beta1.AdmissionRequest, rule types.PolicyRule, stopOnError bool) ([]types.PolicyPatch, error) {
-	rulePatches, err := mw.applyRulePatches(request, rule)
-	if err != nil {
-		mw.logger.Printf("Error occurred while applying patches according to the policy: %v", err)
-	} else {
-		mw.logger.Printf("Prepared %v patches", len(rulePatches))
+func getPolicyPatchingSets(policy types.Policy) PatchingSets {
+	// failurePolicy property is the only available way for now to define behavior on patching error.
+	// TODO: define new failurePolicy values specific for patching and other policy features.
+	if policy.Spec.FailurePolicy != nil && *policy.Spec.FailurePolicy == "continueOnError" {
+		return PatchingSetsContinueAlways
 	}
-
-	if err == nil || !stopOnError {
-		err = mw.applyRuleGenerators(request, rule)
-	}
-
-	return rulePatches, err
+	return PatchingSetsDefault
 }

-// Gets patches from "patch" section in PolicyRule
-func (mw *MutationWebhook) applyRulePatches(request *v1beta1.AdmissionRequest, rule types.PolicyRule) ([]types.PolicyPatch, error) {
-	var patches []types.PolicyPatch
-	patches = append(patches, rule.Patches...)
-	return patches, nil
+// Applies all rule to the created object and returns list of JSON patches.
+// May return nil patches if it is not necessary to create patches for requested object.
+func (mw *MutationWebhook) applyRule(request *v1beta1.AdmissionRequest, rule types.PolicyRule, errorBehavior PatchingSets) ([]types.PolicyPatch, error) {
+	if !IsRuleApplicableToRequest(rule.Resource, request) {
+		return nil, nil
+	}
+
+	err := mw.applyRuleGenerators(request, rule)
+	if err != nil && errorBehavior == PatchingSetsStopOnError {
+		return nil, err
+	} else {
+		return rule.Patches, nil
+	}
 }

 // Applies "configMapGenerator" and "secretGenerator" described in PolicyRule
@ -158,37 +147,6 @@ func (mw *MutationWebhook) applyConfigGenerator(generator *types.PolicyConfigGen
 	return nil
 }

-// Converts JSON patches to byte array
-func SerializePatches(patches []types.PolicyPatch) ([]byte, error) {
-	var result []byte
-	if len(patches) == 0 {
-		return result, nil
-	}
-
-	result = append(result, []byte("[\n")...)
-	for index, patch := range patches {
-		patchBytes, err := serializePatch(patch)
-		if err != nil {
-			return nil, err
-		}
-
-		result = append(result, patchBytes...)
-		if index != (len(patches) - 1) {
-			result = append(result, []byte(",\n")...)
-		}
-	}
-	result = append(result, []byte("\n]")...)
-	return result, nil
-}
-
-func serializePatch(patch types.PolicyPatch) ([]byte, error) {
-	err := patch.Validate()
-	if err != nil {
-		return nil, err
-	}
-	return json.Marshal(patch)
-}
-
 func errorToAdmissionResponse(err error, allowed bool) *v1beta1.AdmissionResponse {
 	return &v1beta1.AdmissionResponse{
 		Result: &metav1.Status{
--- a/webhooks/mutation_test.go
+++ b/webhooks/mutation_test.go
@ -1,167 +0,0 @@
-package webhooks_test
-
-import (
-	"gotest.tools/assert"
-	"testing"
-
-	"github.com/nirmata/kube-policy/webhooks"
-
-	types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1"
-)
-
-func TestSerializePatches_Empty(t *testing.T) {
-	var patches []types.PolicyPatch
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.Assert(t, nil == err)
-	assert.Assert(t, 0 == len(bytes))
-}
-
-func TestSerializePatches_SingleStringValid(t *testing.T) {
-	patch := types.PolicyPatch{
-		Path:      "/metadata/labels/is-mutated",
-		Operation: "add",
-		Value:     "true",
-	}
-	patches := []types.PolicyPatch{patch}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.Assert(t, nil == err)
-	assertEqStringAndData(t, `[
-{"path":"/metadata/labels/is-mutated","op":"add","value":"true"}
-]`, bytes)
-}
-
-func TestSerializePatches_SingleStringInvalid(t *testing.T) {
-	patch := types.PolicyPatch{
-		Path:  "/metadata/labels/is-mutated",
-		Value: "true",
-	}
-	patches := []types.PolicyPatch{patch}
-	_, err := webhooks.SerializePatches(patches)
-	assert.Assert(t, nil != err)
-	patches[0].Path = ""
-	patches[0].Operation = "delete"
-	_, err = webhooks.SerializePatches(patches)
-	assert.Assert(t, nil != err)
-}
-
-func TestSerializePatches_MultipleStringsValid(t *testing.T) {
-	patch1 := types.PolicyPatch{
-		Path:      "/metadata/labels/is-mutated",
-		Operation: "add",
-		Value:     "true",
-	}
-	patch2 := types.PolicyPatch{
-		Path:      "/metadata/labels/newLabel",
-		Operation: "add",
-		Value:     "newValue",
-	}
-	patches := []types.PolicyPatch{patch1, patch2}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.Assert(t, nil == err)
-	assertEqStringAndData(t, `[
-{"path":"/metadata/labels/is-mutated","op":"add","value":"true"},
-{"path":"/metadata/labels/newLabel","op":"add","value":"newValue"}
-]`, bytes)
-}
-
-func TestSerializePatches_SingleIntegerValid(t *testing.T) {
-	const ordinaryInt int = 42
-	patch := types.PolicyPatch{
-		Path:      "/spec/replicas",
-		Operation: "add",
-		Value:     ordinaryInt,
-	}
-	patches := []types.PolicyPatch{patch}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.NilError(t, err)
-	assertEqStringAndData(t, `[
-{"path":"/spec/replicas","op":"add","value":42}
-]`, bytes)
-}
-
-func TestSerializePatches_SingleIntegerBigValid(t *testing.T) {
-	const bigInt uint64 = 100500100500
-	patch := types.PolicyPatch{
-		Path:      "/spec/somethingHuge",
-		Operation: "add",
-		Value:     bigInt,
-	}
-	patches := []types.PolicyPatch{patch}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.NilError(t, err)
-	assertEqStringAndData(t, `[
-{"path":"/spec/somethingHuge","op":"add","value":100500100500}
-]`, bytes)
-}
-
-func TestSerializePatches_SingleFloatValid(t *testing.T) {
-	const ordinaryFloat float32 = 2.71828
-	patch := types.PolicyPatch{
-		Path:      "/spec/consts/e",
-		Operation: "add",
-		Value:     ordinaryFloat,
-	}
-	patches := []types.PolicyPatch{patch}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.NilError(t, err)
-	assertEqStringAndData(t, `[
-{"path":"/spec/consts/e","op":"add","value":2.71828}
-]`, bytes)
-}
-
-func TestSerializePatches_SingleFloatBigValid(t *testing.T) {
-	const bigFloat float64 = 3.1415926535
-	patch := types.PolicyPatch{
-		Path:      "/spec/consts/pi",
-		Operation: "add",
-		Value:     bigFloat,
-	}
-	patches := []types.PolicyPatch{patch}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.NilError(t, err)
-	assertEqStringAndData(t, `[
-{"path":"/spec/consts/pi","op":"add","value":3.1415926535}
-]`, bytes)
-}
-
-func TestSerializePatches_MultipleBoolValid(t *testing.T) {
-	patch1 := types.PolicyPatch{
-		Path:      "/status/is-mutated",
-		Operation: "add",
-		Value:     true,
-	}
-	patch2 := types.PolicyPatch{
-		Path:      "/status/is-unreal",
-		Operation: "add",
-		Value:     false,
-	}
-	patches := []types.PolicyPatch{patch1, patch2}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.NilError(t, err)
-	assertEqStringAndData(t, `[
-{"path":"/status/is-mutated","op":"add","value":true},
-{"path":"/status/is-unreal","op":"add","value":false}
-]`, bytes)
-}
-
-func TestSerializePatches_MultitypeMap(t *testing.T) {
-	valuesMap := make(map[string]interface{})
-	valuesMap["some_string"] = "value1"
-	valuesMap["number"] = 42
-	nestedMap := make(map[string]interface{})
-	nestedMap["label"] = "other value"
-	nestedMap["nested"] = true
-	valuesMap["additional"] = nestedMap
-
-	patch := types.PolicyPatch{
-		Path:      "/spec/values",
-		Operation: "add",
-		Value:     valuesMap,
-	}
-	patches := []types.PolicyPatch{patch}
-	bytes, err := webhooks.SerializePatches(patches)
-	assert.NilError(t, err)
-	assertEqStringAndData(t, `[
-{"path":"/spec/values","op":"add","value":{"additional":{"label":"other value","nested":true},"number":42,"some_string":"value1"}}
-]`, bytes)
-}
--- a/webhooks/patches_test.go
+++ b/webhooks/patches_test.go
@ -0,0 +1,128 @@
+package webhooks_test
+
+import (
+	"gotest.tools/assert"
+	"testing"
+
+	"github.com/nirmata/kube-policy/webhooks"
+
+	types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1"
+)
+
+const endpointsDocument string = `{
+	"kind": "Endpoints",
+	"apiVersion": "v1",
+	"metadata": {
+		"name": "my-endpoint-service",
+		"labels": {
+			"originalLabel": "isHere"
+		}
+	},
+	"subsets": [
+		{
+			"addresses": [
+				{
+					"ip": "1.2.3.4"
+				}
+			],
+			"ports": [
+				{
+					"port": 9376
+				}
+			]
+		}
+	]
+}`
+
+func TestProcessPatches_EmptyPatches(t *testing.T) {
+	var empty []types.PolicyPatch
+	patches, err := webhooks.ProcessPatches(empty, []byte(endpointsDocument), webhooks.PatchingSetsDefault)
+	assert.NilError(t, err)
+	assert.Assert(t, len(patches) == 0)
+}
+
+func makeAddIsMutatedLabelPatch() types.PolicyPatch {
+	return types.PolicyPatch{
+		Path:      "/metadata/labels/is-mutated",
+		Operation: "add",
+		Value:     "true",
+	}
+}
+
+func TestProcessPatches_EmptyDocument(t *testing.T) {
+	var patches []types.PolicyPatch
+	patches = append(patches, makeAddIsMutatedLabelPatch())
+	patchesBytes, err := webhooks.ProcessPatches(patches, nil, webhooks.PatchingSetsDefault)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_AllEmpty(t *testing.T) {
+	patchesBytes, err := webhooks.ProcessPatches(nil, nil, webhooks.PatchingSetsDefault)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_AddPathDoesntExist_StopOnError(t *testing.T) {
+	patch := makeAddIsMutatedLabelPatch()
+	patch.Path = "/metadata/additional/is-mutated"
+	patches := []types.PolicyPatch{patch}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsStopOnError)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_AddPathDoesntExist_ContinueOnError(t *testing.T) {
+	patch := makeAddIsMutatedLabelPatch()
+	patch.Path = "/metadata/additional/is-mutated"
+	patches := []types.PolicyPatch{patch}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsContinueAlways)
+	assert.NilError(t, err)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_RemovePathDoesntExist_StopOnError(t *testing.T) {
+	patch := types.PolicyPatch{Path: "/metadata/labels/is-mutated", Operation: "remove"}
+	patches := []types.PolicyPatch{patch}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsStopOnError)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_AddAndRemovePathsDontExist_ContinueOnError_EmptyResult(t *testing.T) {
+	patch1 := types.PolicyPatch{Path: "/metadata/labels/is-mutated", Operation: "remove"}
+	patch2 := types.PolicyPatch{Path: "/spec/labels/label3", Operation: "add", Value: "label3Value"}
+	patches := []types.PolicyPatch{patch1, patch2}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsContinueAlways)
+	assert.NilError(t, err)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_AddAndRemovePathsDontExist_ContinueOnError_NotEmptyResult(t *testing.T) {
+	patch1 := types.PolicyPatch{Path: "/metadata/labels/is-mutated", Operation: "remove"}
+	patch2 := types.PolicyPatch{Path: "/spec/labels/label2", Operation: "remove", Value: "label2Value"}
+	patch3 := types.PolicyPatch{Path: "/metadata/labels/label3", Operation: "add", Value: "label3Value"}
+	patches := []types.PolicyPatch{patch1, patch2, patch3}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsContinueAlways)
+	assert.NilError(t, err)
+	assert.Assert(t, len(patchesBytes) == 1)
+	assertEqStringAndData(t, `{"path":"/metadata/labels/label3","op":"add","value":"label3Value"}`, patchesBytes[0])
+}
+
+func TestProcessPatches_RemovePathDoesntExist_IgnoreRemoveFailures_EmptyResult(t *testing.T) {
+	patch := types.PolicyPatch{Path: "/metadata/labels/is-mutated", Operation: "remove"}
+	patches := []types.PolicyPatch{patch}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsContinueOnRemoveFailure)
+	assert.NilError(t, err)
+	assert.Assert(t, len(patchesBytes) == 0)
+}
+
+func TestProcessPatches_RemovePathDoesntExist_IgnoreRemoveFailures_NotEmptyResult(t *testing.T) {
+	patch1 := types.PolicyPatch{Path: "/metadata/labels/is-mutated", Operation: "remove"}
+	patch2 := types.PolicyPatch{Path: "/metadata/labels/label2", Operation: "add", Value: "label2Value"}
+	patches := []types.PolicyPatch{patch1, patch2}
+	patchesBytes, err := webhooks.ProcessPatches(patches, []byte(endpointsDocument), webhooks.PatchingSetsContinueOnRemoveFailure)
+	assert.NilError(t, err)
+	assert.Assert(t, len(patchesBytes) == 1)
+	assertEqStringAndData(t, `{"path":"/metadata/labels/label2","op":"add","value":"label2Value"}`, patchesBytes[0])
+}