NK-23: Implemented Validate() methods for structures in types.go.

Implemented tests for added methods.
Added usage of Validate() methods to the code, removed old checks.
Fixed some comments, added new comments.

Gopkg.toml

@@ -14,4 +14,8 @@ required = ["k8s.io/code-generator/cmd/client-gen"]
 
 [[constraint]]
   name = "github.com/golang/protobuf"
+  branch = "master"
+
+[[constraint]]
+  name = "github.com/gotestyourself/gotest.tools"
   branch = "master"

controller/controller_test.go

@@ -0,0 +1,149 @@
+package controller_test
+
+import (
+	"gotest.tools/assert"
+	"testing"
+
+	types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestPolicyCopyFrom_Validate(t *testing.T) {
+	copyFrom := types.PolicyCopyFrom{}
+	assert.Assert(t, copyFrom.Validate() != nil)
+	copyFrom.Name = "name"
+	assert.Assert(t, copyFrom.Validate() != nil)
+	copyFrom.Namespace = "ns"
+	assert.Assert(t, copyFrom.Validate() == nil)
+}
+
+func TestPolicyConfigGenerator_Validate(t *testing.T) {
+	// Not valid
+	generator := types.PolicyConfigGenerator{}
+	assert.Assert(t, generator.Validate() != nil)
+	generator.Name = "generator-name"
+	assert.Assert(t, generator.Validate() != nil)
+	generator.Data = make(map[string]string)
+	assert.Assert(t, generator.Validate() != nil)
+	// Valid
+	generator.Data["field"] = "value"
+	assert.Assert(t, generator.Validate() == nil)
+	generator.CopyFrom = &types.PolicyCopyFrom{
+		Name:      "config-map-name",
+		Namespace: "custom-ns",
+	}
+	assert.Assert(t, generator.Validate() == nil)
+	generator.Data = nil
+	assert.Assert(t, generator.Validate() == nil)
+	// Not valid again
+	generator.CopyFrom = nil
+}
+
+func TestPolicyPatch_Validate(t *testing.T) {
+	// Not valid
+	patch := types.PolicyPatch{}
+	assert.Assert(t, patch.Validate() != nil)
+	patch.Path = "/path"
+	assert.Assert(t, patch.Validate() != nil)
+	patch.Operation = "add"
+	assert.Assert(t, patch.Validate() != nil)
+	// Valid
+	patch.Value = "some-value"
+	assert.Assert(t, patch.Validate() == nil)
+	patch.Operation = "replace"
+	assert.Assert(t, patch.Validate() == nil)
+	patch.Operation = "remove"
+	assert.Assert(t, patch.Validate() == nil)
+	// Valid without a value
+	patch.Value = ""
+	assert.Assert(t, patch.Validate() == nil)
+	// Not valid again
+	patch.Operation = "unknown"
+	assert.Assert(t, patch.Validate() != nil)
+	patch.Value = "some-another-value"
+	assert.Assert(t, patch.Validate() != nil)
+}
+
+func TestPolicyResource_Validate_Name(t *testing.T) {
+	// Not valid
+	resource := types.PolicyResource{}
+	assert.Assert(t, resource.Validate() != nil)
+	resource.Kind = "Deployment"
+	assert.Assert(t, resource.Validate() != nil)
+	// Valid
+	resourceName := "nginx"
+	resource.Name = &resourceName
+	assert.Assert(t, resource.Validate() == nil)
+}
+
+func TestPolicyResource_Validate_Selector(t *testing.T) {
+	// Not valid
+	resource := types.PolicyResource{
+		Kind:     "ConfigMap",
+		Selector: new(metav1.LabelSelector),
+	}
+	assert.Assert(t, resource.Validate() != nil)
+	resource.Selector.MatchLabels = make(map[string]string)
+	assert.Assert(t, resource.Validate() != nil)
+	// Valid
+	resource.Selector.MatchLabels["new-label"] = "new-value"
+	assert.Assert(t, resource.Validate() == nil)
+}
+
+func makeValidRuleResource() types.PolicyResource {
+	resourceName := "test-deployment"
+	return types.PolicyResource{
+		Kind: "Deployment",
+		Name: &resourceName,
+	}
+}
+
+func TestPolicyRule_Validate_Resource(t *testing.T) {
+	// Not valid
+	rule := types.PolicyRule{}
+	assert.Assert(t, rule.Validate() != nil)
+	// Empty
+	rule.Resource = makeValidRuleResource()
+	// Validate resource toi ensure that it is the only valid field
+	assert.Assert(t, rule.Resource.Validate() == nil)
+	assert.Assert(t, rule.Validate() != nil)
+}
+
+func TestPolicyRule_Validate_Patches(t *testing.T) {
+	rule := types.PolicyRule{
+		Resource: makeValidRuleResource(),
+	}
+	// Not empty, but not valid
+	patch := types.PolicyPatch{}
+	rule.Patches = append(rule.Patches, patch)
+	// Not empty and valid
+	assert.Assert(t, rule.Validate() != nil)
+	rule.Patches[0] = types.PolicyPatch{
+		Path:      "/",
+		Operation: "add",
+		Value:     "some",
+	}
+	assert.Assert(t, rule.Validate() == nil)
+}
+
+func TestPolicyRule_Validate_ConfigGenerators(t *testing.T) {
+	rule := types.PolicyRule{
+		Resource: makeValidRuleResource(),
+	}
+	// Not empty, but not valid
+	rule.ConfigMapGenerator = &types.PolicyConfigGenerator{
+		Name: "test-generator",
+	}
+	assert.Assert(t, rule.Validate() != nil)
+	// Not empty and valid
+	rule.ConfigMapGenerator.Data = make(map[string]string)
+	rule.ConfigMapGenerator.Data["some-data"] = "some-value"
+	assert.Assert(t, rule.Validate() == nil)
+	rule.SecretGenerator = rule.ConfigMapGenerator
+	assert.Assert(t, rule.Validate() == nil)
+	rule.ConfigMapGenerator = nil
+	assert.Assert(t, rule.Validate() == nil)
+	// Not valid again
+	rule.SecretGenerator.Name = ""
+	assert.Assert(t, rule.Validate() != nil)
+}

pkg/apis/policy/v1alpha1/types.go

@@ -1,6 +1,9 @@
 package v1alpha1
 
 import (
+	"errors"
+	"fmt"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -31,6 +34,43 @@ type PolicyRule struct {
 	SecretGenerator    *PolicyConfigGenerator `json:"secretGenerator,omitempty"`
 }
 
+// Checks if rule is not empty and all substructures are valid
+func (pr *PolicyRule) Validate() error {
+	err := pr.Resource.Validate()
+	if err != nil {
+		return err
+	}
+
+	if len(pr.Patches) == 0 && pr.ConfigMapGenerator == nil && pr.SecretGenerator == nil {
+		return errors.New("The rule is empty")
+	}
+
+	if len(pr.Patches) > 0 {
+		for _, patch := range pr.Patches {
+			err = patch.Validate()
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	if pr.ConfigMapGenerator != nil {
+		err = pr.ConfigMapGenerator.Validate()
+		if err != nil {
+			return err
+		}
+	}
+
+	if pr.SecretGenerator != nil {
+		err = pr.SecretGenerator.Validate()
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // Describes the resource to which the PolicyRule will apply.
 // Either the name or selector must be specified.
 // IMPORTANT: If neither is specified, the policy rule will not apply (TBD).
@@ -40,6 +80,30 @@ type PolicyResource struct {
 	Selector *metav1.LabelSelector `json:"selector,omitempty"`
 }
 
+// Checks if all necesarry fields are present and have values. Also checks a Selector.
+// Returns error if resource definition is invalid.
+func (pr *PolicyResource) Validate() error {
+	// TBD: selector or name MUST be specified
+	if pr.Kind == "" {
+		return errors.New("The Kind is not specified")
+	} else if pr.Name == nil && pr.Selector == nil {
+		return errors.New("Neither Name nor Selector is specified")
+	}
+
+	if pr.Selector != nil {
+		selector, err := metav1.LabelSelectorAsSelector(pr.Selector)
+		if err != nil {
+			return err
+		}
+		requirements, _ := selector.Requirements()
+		if len(requirements) == 0 {
+			return errors.New("The requirements are not specified in selector")
+		}
+	}
+
+	return nil
+}
+
 // PolicyPatch declares patch operation for created object according to the JSONPatch spec:
 // http://jsonpatch.com/
 type PolicyPatch struct {
@@ -48,6 +112,23 @@ type PolicyPatch struct {
 	Value     string `json:"value"`
 }
 
+func (pp *PolicyPatch) Validate() error {
+	if pp.Path == "" {
+		return errors.New("JSONPatch field 'path' is mandatory")
+	}
+
+	if pp.Operation == "add" || pp.Operation == "replace" {
+		if pp.Value == "" {
+			return errors.New(fmt.Sprintf("JSONPatch field 'value' is mandatory for operation '%s'", pp.Operation))
+		}
+		return nil
+	} else if pp.Operation == "remove" {
+		return nil
+	}
+
+	return errors.New(fmt.Sprintf("Unsupported JSONPatch operation '%s'", pp.Operation))
+}
+
 // The declaration for a Secret or a ConfigMap, which will be created in the new namespace.
 // Can be applied only when PolicyRule.Resource.Kind is "Namespace".
 type PolicyConfigGenerator struct {
@@ -56,12 +137,33 @@ type PolicyConfigGenerator struct {
 	Data     map[string]string `json:"data"`
 }
 
+// Returns error if generator is configured incompletely
+func (pcg *PolicyConfigGenerator) Validate() error {
+	if pcg.Name == "" {
+		return errors.New("The generator is unnamed")
+	} else if len(pcg.Data) == 0 && pcg.CopyFrom == nil {
+		return errors.New("Neither Data nor CopyFrom (source) is specified")
+	}
+	if pcg.CopyFrom != nil {
+		return pcg.CopyFrom.Validate()
+	}
+	return nil
+}
+
 // Location of a Secret or a ConfigMap which will be used as source when applying PolicyConfigGenerator
 type PolicyCopyFrom struct {
 	Namespace string `json:"namespace"`
 	Name      string `json:"name"`
 }
 
+// Returns error if Name or namespace is not cpecified
+func (pcf *PolicyCopyFrom) Validate() error {
+	if pcf.Name == "" || pcf.Namespace == "" {
+		return errors.New("Name or/and Namespace is not specified")
+	}
+	return nil
+}
+
 // Contains logs about policy application
 type PolicyStatus struct {
 	Logs []string `json:"log"`

webhooks/admission.go

@@ -39,19 +39,14 @@ func kindIsSupported(kind string) bool {
 	return false
 }
 
-// AdmissionIsRequired checks for admission if kind is supported
+// Checks for admission if kind is supported
 func AdmissionIsRequired(request *v1beta1.AdmissionRequest) bool {
 	// Here you can make additional hardcoded checks
 	return kindIsSupported(request.Kind.Kind)
 }
 
-// IsRuleApplicableToRequest checks requests kind, name and labels to fit the policy
+// Checks requests kind, name and labels to fit the policy
 func IsRuleApplicableToRequest(policyResource types.PolicyResource, request *v1beta1.AdmissionRequest) bool {
-	if policyResource.Selector == nil && policyResource.Name == nil {
-		// TBD: selector or name MUST be specified
-		return false
-	}
-
 	if policyResource.Kind != request.Kind.Kind {
 		return false
 	}
@@ -68,7 +63,6 @@ func IsRuleApplicableToRequest(policyResource types.PolicyResource, request *v1b
 			selector, err := metav1.LabelSelectorAsSelector(policyResource.Selector)
 
 			if err != nil {
-				// TODO: log that selector is invalid
 				return false
 			}
 

webhooks/mutation.go

@@ -1,132 +1,158 @@
 package webhooks
 
 import (
-    "encoding/json"
-    "errors"
-    "log"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"log"
 
-    types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1"
-    v1beta1 "k8s.io/api/admission/v1beta1"
-    metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1"
+	v1beta1 "k8s.io/api/admission/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // MutationWebhook is a data type that represents
 // buisness logic for resource mutation
 type MutationWebhook struct {
-    logger *log.Logger
+	logger *log.Logger
 }
 
 // NewMutationWebhook is a method that returns new instance
 // of MutationWebhook struct
 func NewMutationWebhook(logger *log.Logger) (*MutationWebhook, error) {
-    if logger == nil {
-        return nil, errors.New("Logger must be set for the mutation webhook")
-    }
-    return &MutationWebhook{logger: logger}, nil
+	if logger == nil {
+		return nil, errors.New("Logger must be set for the mutation webhook")
+	}
+	return &MutationWebhook{logger: logger}, nil
 }
 
 // Mutate applies admission to request
 func (mw *MutationWebhook) Mutate(request *v1beta1.AdmissionRequest, policies []types.Policy) *v1beta1.AdmissionResponse {
-    mw.logger.Printf("AdmissionReview for Kind=%v, Namespace=%v Name=%v UID=%v patchOperation=%v UserInfo=%v",
-        request.Kind.Kind, request.Namespace, request.Name, request.UID, request.Operation, request.UserInfo)
+	mw.logger.Printf("AdmissionReview for Kind=%v, Namespace=%v Name=%v UID=%v patchOperation=%v UserInfo=%v",
+		request.Kind.Kind, request.Namespace, request.Name, request.UID, request.Operation, request.UserInfo)
 
-    if len(policies) == 0 {
-        return nil
-    }
+	if len(policies) == 0 {
+		return nil
+	}
 
-    var allPatches []types.PolicyPatch
-    for _, policy := range policies {
-        stopOnError := true
-        if policy.Spec.FailurePolicy != nil && *policy.Spec.FailurePolicy == "continueOnError" {
-            stopOnError = false
-        }
+	var allPatches []types.PolicyPatch
+	for _, policy := range policies {
+		stopOnError := true
+		if policy.Spec.FailurePolicy != nil && *policy.Spec.FailurePolicy == "continueOnError" {
+			stopOnError = false
+		}
 
-        for ruleIdx, rule := range policy.Spec.Rules {
-            if IsRuleApplicableToRequest(rule.Resource, request) {
-                mw.logger.Printf("Applying policy %v, rule index = %v", policy.ObjectMeta.Name, ruleIdx)
-                rulePatches, err := mw.applyPolicyRule(request, rule)
-                /*
-                 * If at least one error is detected in the rule, the entire rule will not be applied.
-                 * This may be changed in the future by varying the policy.Spec.FailurePolicy values.
-                 */
-                if err != nil {
-                    mw.logger.Printf("Error occurred while applying the policy: %v", err)
-                    if stopOnError {
-                        mw.logger.Printf("/!\\ Denying the request according to FailurePolicy spec /!\\")
-                        return errorToResponse(err, false)
-                    }
-                } else {
-                    mw.logger.Printf("Prepared %v patches", len(rulePatches))
-                    allPatches = append(allPatches, rulePatches...)
-                }
-            }
-        }
-    }
+		for ruleIdx, rule := range policy.Spec.Rules {
+			err := rule.Validate()
+			if err != nil {
+				mw.logger.Printf("Invalid rule detected: #%d in policy %s", ruleIdx, policy.ObjectMeta.Name)
+				continue
+			}
 
-    patchesBytes, err := SerializePatches(allPatches)
-    if err != nil {
-        mw.logger.Printf("Error occerred while serializing JSONPathch: %v", err)
-        return errorToResponse(err, true)
-    }
+			if IsRuleApplicableToRequest(rule.Resource, request) {
+				mw.logger.Printf("Applying policy %s, rule index = %s", policy.ObjectMeta.Name, ruleIdx)
+				rulePatches, err := mw.applyPolicyRule(request, rule)
+				/*
+				 * If at least one error is detected in the rule, the entire rule will not be applied.
+				 * This may be changed in the future by varying the policy.Spec.FailurePolicy values.
+				 */
+				if err != nil {
+					mw.logger.Printf("Error occurred while applying the policy: %v", err)
+					if stopOnError {
+						mw.logger.Printf("/!\\ Denying the request according to FailurePolicy spec /!\\")
+						return errorToResponse(err, false)
+					}
+				} else {
+					mw.logger.Printf("Prepared %v patches", len(rulePatches))
+					allPatches = append(allPatches, rulePatches...)
+				}
+			}
+		}
+	}
 
-    return &v1beta1.AdmissionResponse{
-        Allowed: true,
-        Patch:   patchesBytes,
-        PatchType: func() *v1beta1.PatchType {
-            pt := v1beta1.PatchTypeJSONPatch
-            return &pt
-        }(),
-    }
+	patchesBytes, err := SerializePatches(allPatches)
+	if err != nil {
+		mw.logger.Printf("Error occerred while serializing JSONPathch: %v", err)
+		return errorToResponse(err, true)
+	}
+
+	return &v1beta1.AdmissionResponse{
+		Allowed: true,
+		Patch:   patchesBytes,
+		PatchType: func() *v1beta1.PatchType {
+			pt := v1beta1.PatchTypeJSONPatch
+			return &pt
+		}(),
+	}
 }
 
 // Applies all possible patches in a rule
 func (mw *MutationWebhook) applyPolicyRule(request *v1beta1.AdmissionRequest, rule types.PolicyRule) ([]types.PolicyPatch, error) {
-    var allPatches []types.PolicyPatch
-    if rule.Patches == nil && rule.ConfigMapGenerator == nil && rule.SecretGenerator == nil {
-        return nil, errors.New("The rule is empty")
-    }
+	var allPatches []types.PolicyPatch
+	allPatches = append(allPatches, rule.Patches...)
 
-    allPatches = append(allPatches, rule.Patches...)
+	// configMapGenerator and secretGenerator can be applied only to namespaces
+	if request.Kind.Kind == "Namespace" {
+		if rule.ConfigMapGenerator != nil {
+			err := mw.applyConfigGenerator(request, *rule.ConfigMapGenerator, "ConfigMap")
+			if err != nil {
+				mw.logger.Printf("Unable to apply configMapGenerator: %s", err)
+			}
+		}
 
-    if rule.ConfigMapGenerator != nil {
-        // TODO: Make patches from configMapGenerator and add them to returned array
-    }
+		if rule.SecretGenerator != nil {
+			err := mw.applyConfigGenerator(request, *rule.SecretGenerator, "Secret")
+			if err != nil {
+				mw.logger.Printf("Unable to apply secretGenerator: %s", err)
+			}
+		}
+	}
 
-    if rule.SecretGenerator != nil {
-        // TODO: Make patches from secretGenerator and add them to returned array
-    }
+	return allPatches, nil
+}
 
-    return allPatches, nil
+// Creates resourceKind (ConfigMap or Secret) with parameters specified in generator in cluster specified in request
+func (mw *MutationWebhook) applyConfigGenerator(request *v1beta1.AdmissionRequest, generator types.PolicyConfigGenerator, resourceKind string) error {
+	err := generator.Validate()
+	if err != nil {
+		return errors.New(fmt.Sprintf("Generator for %s is invalid: %s", resourceKind, err))
+	}
+
+	if generator.CopyFrom != nil {
+		// TODO: Implement copying of the object to another namespace
+	}
+	// TODO: Implement filling of existing object with data from generator
+	return nil
 }
 
 // SerializePatches converts JSON patches to byte array
 func SerializePatches(patches []types.PolicyPatch) ([]byte, error) {
-    var result []byte
-    result = append(result, []byte("[\n")...)
-    for index, patch := range patches {
-        if patch.Operation == "" || patch.Path == "" {
-            return nil, errors.New("JSONPatch doesn't contain mandatory fields 'path' or 'op'")
-        }
+	var result []byte
+	result = append(result, []byte("[\n")...)
+	for index, patch := range patches {
+		if patch.Operation == "" || patch.Path == "" {
+			return nil, errors.New("JSONPatch doesn't contain mandatory fields 'path' or 'op'")
+		}
 
-        patchBytes, err := json.Marshal(patch)
-        if err != nil {
-            return nil, err
-        }
+		patchBytes, err := json.Marshal(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
+		result = append(result, patchBytes...)
+		if index != (len(patches) - 1) {
+			result = append(result, []byte(",\n")...)
+		}
+	}
+	result = append(result, []byte("\n]")...)
+	return result, nil
 }
 
 func errorToResponse(err error, allowed bool) *v1beta1.AdmissionResponse {
-    return &v1beta1.AdmissionResponse{
-        Result: &metav1.Status{
-            Message: err.Error(),
-        },
-        Allowed: allowed,
-    }
+	return &v1beta1.AdmissionResponse{
+		Result: &metav1.Status{
+			Message: err.Error(),
+		},
+		Allowed: allowed,
+	}
 }

webhooks/mutation_test.go

@@ -1,48 +1,46 @@
 package webhooks_test
 
 import (
-    "testing"
+	"testing"
 
-    "github.com/nirmata/kube-policy/webhooks"
+	"github.com/nirmata/kube-policy/webhooks"
 
-    //v1beta1 "k8s.io/api/admission/v1beta1"
-    //metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-    types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1"
+	//v1beta1 "k8s.io/api/admission/v1beta1"
+	//metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	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)
-    assertEq(t, nil, err)
-    assertEqStringAndData(t, "[\n\n]", bytes)
+	var patches []types.PolicyPatch
+	bytes, err := webhooks.SerializePatches(patches)
+	assertEq(t, nil, err)
+	assertEqStringAndData(t, "[\n\n]", bytes)
 }
 
 func TestSerializePatches_SingleValid(t *testing.T) {
-    patch := types.PolicyPatch{
-        Path:      "/metadata/labels/is-mutated",
-        Operation: "add",
-        Value:     "true",
-    }
-    patches := []types.PolicyPatch{patch}
-    bytes, err := webhooks.SerializePatches(patches)
-    assertEq(t, nil, err)
-    assertEqStringAndData(t, `[
+	patch := types.PolicyPatch{
+		Path:      "/metadata/labels/is-mutated",
+		Operation: "add",
+		Value:     "true",
+	}
+	patches := []types.PolicyPatch{patch}
+	bytes, err := webhooks.SerializePatches(patches)
+	assertEq(t, nil, err)
+	assertEqStringAndData(t, `[
 {"path":"/metadata/labels/is-mutated","op":"add","value":"true"}
 ]`, bytes)
 }
 
 func TestSerializePatches_SingleInvalid(t *testing.T) {
-    patch := types.PolicyPatch{
-        Path:  "/metadata/labels/is-mutated",
-        Value: "true",
-    }
-    patches := []types.PolicyPatch{patch}
-    _, err := webhooks.SerializePatches(patches)
-    assertNe(t, nil, err)
-    patches[0].Path = ""
-    patches[0].Operation = "delete"
-    _, err = webhooks.SerializePatches(patches)
-    assertNe(t, nil, err)
+	patch := types.PolicyPatch{
+		Path:  "/metadata/labels/is-mutated",
+		Value: "true",
+	}
+	patches := []types.PolicyPatch{patch}
+	_, err := webhooks.SerializePatches(patches)
+	assertNe(t, nil, err)
+	patches[0].Path = ""
+	patches[0].Operation = "delete"
+	_, err = webhooks.SerializePatches(patches)
+	assertNe(t, nil, err)
 }
-
-// patch := `[ {"op":"add","path":"/metadata/labels","value":{"is-mutated":"true"}} ]`