From b08357a1702c911447b060991fcead4c63a280d2 Mon Sep 17 00:00:00 2001 From: shuting Date: Wed, 15 May 2019 11:45:16 -0700 Subject: [PATCH 1/4] update pkg/engine/mutation/patches_test.go --- pkg/engine/mutation/patches.go | 9 ++-- pkg/engine/mutation/patches_test.go | 77 +++++++++++++---------------- 2 files changed, 40 insertions(+), 46 deletions(-) diff --git a/pkg/engine/mutation/patches.go b/pkg/engine/mutation/patches.go index bf86a73418..2694a8d5ab 100644 --- a/pkg/engine/mutation/patches.go +++ b/pkg/engine/mutation/patches.go @@ -3,6 +3,7 @@ package mutation import ( "encoding/json" "errors" + "log" jsonpatch "github.com/evanphx/json-patch" kubepolicy "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" @@ -10,7 +11,7 @@ import ( type PatchBytes []byte -// Returns array from separate patches that can be applied to the document +// ProcessPatches Returns array from separate patches that can be applied to the document // Returns error ONLY in case when creation of resource should be denied. func ProcessPatches(patches []kubepolicy.Patch, resource []byte) ([]PatchBytes, error) { if len(resource) == 0 { @@ -18,7 +19,7 @@ func ProcessPatches(patches []kubepolicy.Patch, resource []byte) ([]PatchBytes, } var appliedPatches []PatchBytes - for _, patch := range patches { + for i, patch := range patches { patchRaw, err := json.Marshal(patch) if err != nil { return nil, err @@ -26,7 +27,9 @@ func ProcessPatches(patches []kubepolicy.Patch, resource []byte) ([]PatchBytes, _, err = applyPatch(resource, patchRaw) if err != nil { - return nil, err + // TODO: continue on error if one of the patches fails, will add the failure event in such case + log.Printf("Patch failed: patch number = %d, patch Operation = %s, err: %v", i, patch.Operation, err) + continue } appliedPatches = append(appliedPatches, patchRaw) diff --git a/pkg/engine/mutation/patches_test.go b/pkg/engine/mutation/patches_test.go index 986b15594f..60ee4d2835 100644 --- a/pkg/engine/mutation/patches_test.go +++ b/pkg/engine/mutation/patches_test.go @@ -34,14 +34,14 @@ const endpointsDocument string = `{ }` func TestProcessPatches_EmptyPatches(t *testing.T) { - var empty []types.PolicyPatch - patches, err := ProcessPatches(empty, []byte(endpointsDocument), PatchingSetsDefault) + var empty []types.Patch + patches, err := ProcessPatches(empty, []byte(endpointsDocument)) assert.NilError(t, err) assert.Assert(t, len(patches) == 0) } -func makeAddIsMutatedLabelPatch() types.PolicyPatch { - return types.PolicyPatch{ +func makeAddIsMutatedLabelPatch() types.Patch { + return types.Patch{ Path: "/metadata/labels/is-mutated", Operation: "add", Value: "true", @@ -49,78 +49,69 @@ func makeAddIsMutatedLabelPatch() types.PolicyPatch { } func TestProcessPatches_EmptyDocument(t *testing.T) { - var patches []types.PolicyPatch + var patches []types.Patch patches = append(patches, makeAddIsMutatedLabelPatch()) - patchesBytes, err := ProcessPatches(patches, nil, PatchingSetsDefault) + patchesBytes, err := ProcessPatches(patches, nil) assert.Assert(t, err != nil) assert.Assert(t, len(patchesBytes) == 0) } func TestProcessPatches_AllEmpty(t *testing.T) { - patchesBytes, err := ProcessPatches(nil, nil, PatchingSetsDefault) + patchesBytes, err := ProcessPatches(nil, nil) assert.Assert(t, err != nil) assert.Assert(t, len(patchesBytes) == 0) } -func TestProcessPatches_AddPathDoesntExist_StopOnError(t *testing.T) { +func TestProcessPatches_AddPathDoesntExist(t *testing.T) { patch := makeAddIsMutatedLabelPatch() patch.Path = "/metadata/additional/is-mutated" - patches := []types.PolicyPatch{patch} - patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument), 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 := ProcessPatches(patches, []byte(endpointsDocument), PatchingSetsContinueAlways) + patches := []types.Patch{patch} + patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument)) 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 := ProcessPatches(patches, []byte(endpointsDocument), PatchingSetsStopOnError) - assert.Assert(t, err != nil) +func TestProcessPatches_RemovePathDoesntExist(t *testing.T) { + patch := types.Patch{Path: "/metadata/labels/is-mutated", Operation: "remove"} + patches := []types.Patch{patch} + patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument)) + assert.NilError(t, err) 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 := ProcessPatches(patches, []byte(endpointsDocument), PatchingSetsContinueAlways) +func TestProcessPatches_AddAndRemovePathsDontExist_EmptyResult(t *testing.T) { + patch1 := types.Patch{Path: "/metadata/labels/is-mutated", Operation: "remove"} + patch2 := types.Patch{Path: "/spec/labels/label3", Operation: "add", Value: "label3Value"} + patches := []types.Patch{patch1, patch2} + patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument)) 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 := ProcessPatches(patches, []byte(endpointsDocument), PatchingSetsContinueAlways) + patch1 := types.Patch{Path: "/metadata/labels/is-mutated", Operation: "remove"} + patch2 := types.Patch{Path: "/spec/labels/label2", Operation: "remove", Value: "label2Value"} + patch3 := types.Patch{Path: "/metadata/labels/label3", Operation: "add", Value: "label3Value"} + patches := []types.Patch{patch1, patch2, patch3} + patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument)) 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 := ProcessPatches(patches, []byte(endpointsDocument), PatchingSetsContinueOnRemoveFailure) +func TestProcessPatches_RemovePathDoesntExist_EmptyResult(t *testing.T) { + patch := types.Patch{Path: "/metadata/labels/is-mutated", Operation: "remove"} + patches := []types.Patch{patch} + patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument)) 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 := ProcessPatches(patches, []byte(endpointsDocument), PatchingSetsContinueOnRemoveFailure) +func TestProcessPatches_RemovePathDoesntExist_NotEmptyResult(t *testing.T) { + patch1 := types.Patch{Path: "/metadata/labels/is-mutated", Operation: "remove"} + patch2 := types.Patch{Path: "/metadata/labels/label2", Operation: "add", Value: "label2Value"} + patches := []types.Patch{patch1, patch2} + patchesBytes, err := ProcessPatches(patches, []byte(endpointsDocument)) assert.NilError(t, err) assert.Assert(t, len(patchesBytes) == 1) assertEqStringAndData(t, `{"path":"/metadata/labels/label2","op":"add","value":"label2Value"}`, patchesBytes[0]) From 35f829e4b6767fe02df1c523b02de3d06d343d5d Mon Sep 17 00:00:00 2001 From: shuting Date: Wed, 15 May 2019 11:47:38 -0700 Subject: [PATCH 2/4] remove pkg/engine/mutation/checkRules.go since the logic is moved to /pkg/engine/mutation/utils.go --- pkg/engine/mutation/checkRules.go | 44 ------------------------------- 1 file changed, 44 deletions(-) delete mode 100644 pkg/engine/mutation/checkRules.go diff --git a/pkg/engine/mutation/checkRules.go b/pkg/engine/mutation/checkRules.go deleted file mode 100644 index bcd73a0840..0000000000 --- a/pkg/engine/mutation/checkRules.go +++ /dev/null @@ -1,44 +0,0 @@ -package mutation - -import ( - "github.com/minio/minio/pkg/wildcard" - types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -// kind is the type of object being manipulated -// Checks requests kind, name and labels to fit the policy -func IsRuleApplicableToResource(resourceRaw []byte, description types.ResourceDescription) (bool, error) { - kind := ParseKindFromObject(resourceRaw) - if description.Kind != kind { - return false, nil - } - - if resourceRaw != nil { - meta := ParseMetadataFromObject(resourceRaw) - name := ParseNameFromObject(resourceRaw) - - if description.Name != nil { - - if !wildcard.Match(*description.Name, name) { - return false, nil - } - } - - if description.Selector != nil { - selector, err := metav1.LabelSelectorAsSelector(description.Selector) - - if err != nil { - return false, err - } - - labelMap := ParseLabelsFromMetadata(meta) - - if !selector.Matches(labelMap) { - return false, nil - } - - } - } - return true, nil -} From 9956f3ee1207893c3388f93a91da9884e09d7569 Mon Sep 17 00:00:00 2001 From: shuting Date: Wed, 15 May 2019 18:27:02 -0700 Subject: [PATCH 3/4] add unit test pkg/apis/policy/v1alpha1/utils_test.go --- pkg/apis/policy/v1alpha1/utils_test.go | 87 ++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 pkg/apis/policy/v1alpha1/utils_test.go diff --git a/pkg/apis/policy/v1alpha1/utils_test.go b/pkg/apis/policy/v1alpha1/utils_test.go new file mode 100644 index 0000000000..2d050ce3dc --- /dev/null +++ b/pkg/apis/policy/v1alpha1/utils_test.go @@ -0,0 +1,87 @@ +package v1alpha1 + +import ( + "testing" + + "gotest.tools/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var defaultResourceDescriptionName = "defaultResourceDescription" +var defaultResourceDescription = ResourceDescription{ + Kind: "Deployment", + Name: &defaultResourceDescriptionName, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"LabelForSelector": "defaultResourceDescription"}, + }, +} + +func Test_EmptyRule(t *testing.T) { + emptyRule := Rule{ + Name: "defaultRule", + ResourceDescription: defaultResourceDescription, + } + err := emptyRule.Validate() + assert.Assert(t, err != nil) +} + +func Test_ResourceDescription(t *testing.T) { + err := defaultResourceDescription.Validate() + assert.NilError(t, err) +} + +func Test_ResourceDescription_EmptyKind(t *testing.T) { + resourceDescription := ResourceDescription{ + Name: &defaultResourceDescriptionName, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"LabelForSelector": "defaultResourceDescription"}, + }, + } + err := resourceDescription.Validate() + assert.Assert(t, err != nil) +} + +func Test_ResourceDescription_EmptyNameAndSelector(t *testing.T) { + resourceDescription := ResourceDescription{ + Kind: "Deployment", + } + err := resourceDescription.Validate() + assert.Assert(t, err != nil) +} + +func Test_Patch_EmptyPath(t *testing.T) { + patch := Patch{ + Operation: "add", + Value: "true", + } + err := patch.Validate() + assert.Assert(t, err != nil) +} + +func Test_Patch_EmptyValueWithAdd(t *testing.T) { + patch := Patch{ + Path: "/metadata/labels/is-mutated", + Operation: "add", + } + err := patch.Validate() + assert.Assert(t, err != nil) +} + +func Test_Patch_UnsupportedOperation(t *testing.T) { + patch := Patch{ + Path: "/metadata/labels/is-mutated", + Operation: "overwrite", + Value: "true", + } + err := patch.Validate() + assert.Assert(t, err != nil) +} + +func Test_Generation_EmptyCopyFrom(t *testing.T) { + generation := Generation{ + Kind: "ConfigMap", + Name: "comfigmapGenerator", + } + err := generation.Validate() + assert.Assert(t, err != nil) +} From b58e4f50262d90f3b29b2032874fd0775fe86bc0 Mon Sep 17 00:00:00 2001 From: shuting Date: Wed, 15 May 2019 18:53:45 -0700 Subject: [PATCH 4/4] Format project with gofmt, govet, misspell --- definitions/policy-example.yaml | 2 +- init.go | 16 +++++++--------- main.go | 2 +- pkg/apis/policy/v1alpha1/register.go | 28 ++++++++++++++-------------- pkg/engine/mutation/patches.go | 2 +- pkg/event/controller.go | 2 +- pkg/event/msgbuilder.go | 2 +- 7 files changed, 26 insertions(+), 28 deletions(-) diff --git a/definitions/policy-example.yaml b/definitions/policy-example.yaml index 385c35ffb1..327cc97586 100644 --- a/definitions/policy-example.yaml +++ b/definitions/policy-example.yaml @@ -78,7 +78,7 @@ spec : data: # data is optional status: events: - # log of applied policies. We will need a way to distingush between failed + # log of applied policies. We will need a way to distinguish between failed # and succeeded operations diff --git a/init.go b/init.go index 358e7b2d29..edb1f64f53 100644 --- a/init.go +++ b/init.go @@ -17,13 +17,12 @@ func createClientConfig(kubeconfig string) (*rest.Config, error) { if kubeconfig == "" { log.Printf("Using in-cluster configuration") return rest.InClusterConfig() - } else { - log.Printf("Using configuration from '%s'", kubeconfig) - return clientcmd.BuildConfigFromFlags("", kubeconfig) } + log.Printf("Using configuration from '%s'", kubeconfig) + return clientcmd.BuildConfigFromFlags("", kubeconfig) } -func initTlsPemPair(certFile, keyFile string, clientConfig *rest.Config, kubeclient *kubeclient.KubeClient) (*tls.TlsPemPair, error) { +func initTLSPemPair(certFile, keyFile string, clientConfig *rest.Config, kubeclient *kubeclient.KubeClient) (*tls.TlsPemPair, error) { var tlsPair *tls.TlsPemPair if certFile != "" || keyFile != "" { tlsPair = tlsPairFromFiles(certFile, keyFile) @@ -32,10 +31,9 @@ func initTlsPemPair(certFile, keyFile string, clientConfig *rest.Config, kubecli var err error if tlsPair != nil { return tlsPair, nil - } else { - tlsPair, err = tlsPairFromCluster(clientConfig, kubeclient) - return tlsPair, err } + tlsPair, err = tlsPairFromCluster(clientConfig, kubeclient) + return tlsPair, err } // Loads PEM private key and TLS certificate from given files @@ -66,14 +64,14 @@ func tlsPairFromFiles(certFile, keyFile string) *tls.TlsPemPair { // Created pair is stored in cluster's secret. // Returns struct with key/certificate pair. func tlsPairFromCluster(configuration *rest.Config, client *kubeclient.KubeClient) (*tls.TlsPemPair, error) { - apiServerUrl, err := url.Parse(configuration.Host) + apiServerURL, err := url.Parse(configuration.Host) if err != nil { return nil, err } certProps := tls.TlsCertificateProps{ Service: config.WebhookServiceName, Namespace: config.KubePolicyNamespace, - ApiServerHost: apiServerUrl.Hostname(), + ApiServerHost: apiServerURL.Hostname(), } tlsPair := client.ReadTlsPair(certProps) diff --git a/main.go b/main.go index 3fd428f433..abf9b47711 100644 --- a/main.go +++ b/main.go @@ -54,7 +54,7 @@ func main() { log.Fatalf("Error creating mutation webhook: %v\n", err) } - tlsPair, err := initTlsPemPair(cert, key, clientConfig, kubeclient) + tlsPair, err := initTLSPemPair(cert, key, clientConfig, kubeclient) if err != nil { log.Fatalf("Failed to initialize TLS key/certificate pair: %v\n", err) } diff --git a/pkg/apis/policy/v1alpha1/register.go b/pkg/apis/policy/v1alpha1/register.go index cd8c355461..a207396860 100644 --- a/pkg/apis/policy/v1alpha1/register.go +++ b/pkg/apis/policy/v1alpha1/register.go @@ -1,11 +1,11 @@ package v1alpha1 import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" - "github.com/nirmata/kube-policy/pkg/apis/policy" + "github.com/nirmata/kube-policy/pkg/apis/policy" ) // SchemeGroupVersion is group version used to register these objects @@ -13,25 +13,25 @@ var SchemeGroupVersion = schema.GroupVersion{Group: policy.GroupName, Version: " // Kind takes an unqualified kind and returns back a Group qualified GroupKind func Kind(kind string) schema.GroupKind { - return SchemeGroupVersion.WithKind(kind).GroupKind() + return SchemeGroupVersion.WithKind(kind).GroupKind() } // Resource takes an unqualified resource and returns a Group qualified GroupResource func Resource(resource string) schema.GroupResource { - return SchemeGroupVersion.WithResource(resource).GroupResource() + return SchemeGroupVersion.WithResource(resource).GroupResource() } var ( - SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes) - AddToScheme = SchemeBuilder.AddToScheme + SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes) + AddToScheme = SchemeBuilder.AddToScheme ) // Adds the list of known types to Scheme. func addKnownTypes(scheme *runtime.Scheme) error { - scheme.AddKnownTypes(SchemeGroupVersion, - &Policy{}, - &PolicyList{}, - ) - metav1.AddToGroupVersion(scheme, SchemeGroupVersion) - return nil + scheme.AddKnownTypes(SchemeGroupVersion, + &Policy{}, + &PolicyList{}, + ) + metav1.AddToGroupVersion(scheme, SchemeGroupVersion) + return nil } diff --git a/pkg/engine/mutation/patches.go b/pkg/engine/mutation/patches.go index 2694a8d5ab..0c298352aa 100644 --- a/pkg/engine/mutation/patches.go +++ b/pkg/engine/mutation/patches.go @@ -47,7 +47,7 @@ func JoinPatches(patches []PatchBytes) PatchBytes { result = append(result, []byte("[\n")...) for index, patch := range patches { result = append(result, patch...) - if index != (len(patches) - 1) { + if index != len(patches)-1 { result = append(result, []byte(",\n")...) } } diff --git a/pkg/event/controller.go b/pkg/event/controller.go index e0507af49f..6d2144314b 100644 --- a/pkg/event/controller.go +++ b/pkg/event/controller.go @@ -115,7 +115,7 @@ func (c *controller) processNextWorkItem() bool { }(obj) if err != nil { - log.Println((err)) + log.Println(err) } return true } diff --git a/pkg/event/msgbuilder.go b/pkg/event/msgbuilder.go index b38d9327ac..08703fba53 100644 --- a/pkg/event/msgbuilder.go +++ b/pkg/event/msgbuilder.go @@ -10,7 +10,7 @@ func (k MsgKey) String() string { "Failed to satisfy policy on resource %s.The following rules %s failed to apply. Created Policy Violation", "Failed to process rule %s of policy %s. Created Policy Violation %s", "Policy applied successfully on the resource %s", - "Rule %s of Policy %s applied successfull", + "Rule %s of Policy %s applied successful", "Failed to apply policy, blocked creation of resource %s. The following rules %s failed to apply", "Failed to apply rule %s of policy %s Blocked update of the resource", "Failed to apply policy on resource %s.Blocked update of the resource. The following rules %s failed to apply",