From 2b4ac9d07b2080f737c17e04822e7335650838b4 Mon Sep 17 00:00:00 2001 From: shivdudhani Date: Tue, 7 May 2019 13:26:54 -0700 Subject: [PATCH] code review changes --- controller/controller.go | 40 +++++++++---------- .../controller_interfaces.go | 11 +++-- kubeclient/kubeclient.go | 4 +- pkg/event/builder.go | 10 ++--- .../builder_interfaces.go | 6 +-- pkg/violation/builder.go | 18 ++++----- .../interfaces/violation_interfaces.go | 11 +++++ .../violation_interfaces.go | 11 ----- pkg/violation/utils/util.go | 10 ++--- webhooks/admission.go | 2 +- webhooks/mutation.go | 13 ++++-- 11 files changed, 70 insertions(+), 66 deletions(-) rename controller/{internalinterfaces => interfaces}/controller_interfaces.go (69%) rename pkg/event/{internalinterfaces => interfaces}/builder_interfaces.go (52%) create mode 100644 pkg/violation/interfaces/violation_interfaces.go delete mode 100644 pkg/violation/internalinterfaces/violation_interfaces.go diff --git a/controller/controller.go b/controller/controller.go index 3145b07785..3ccde81b97 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -8,7 +8,7 @@ import ( "sort" "time" - internalinterfaces "github.com/nirmata/kube-policy/controller/internalinterfaces" + controllerinterfaces "github.com/nirmata/kube-policy/controller/interfaces" kubeClient "github.com/nirmata/kube-policy/kubeclient" types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" clientset "github.com/nirmata/kube-policy/pkg/client/clientset/versioned" @@ -16,10 +16,10 @@ import ( informers "github.com/nirmata/kube-policy/pkg/client/informers/externalversions" lister "github.com/nirmata/kube-policy/pkg/client/listers/policy/v1alpha1" event "github.com/nirmata/kube-policy/pkg/event" - eventinternalinterfaces "github.com/nirmata/kube-policy/pkg/event/internalinterfaces" + eventinterfaces "github.com/nirmata/kube-policy/pkg/event/interfaces" eventutils "github.com/nirmata/kube-policy/pkg/event/utils" violation "github.com/nirmata/kube-policy/pkg/violation" - violationinternalinterfaces "github.com/nirmata/kube-policy/pkg/violation/internalinterfaces" + violationinterfaces "github.com/nirmata/kube-policy/pkg/violation/interfaces" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" mergetypes "k8s.io/apimachinery/pkg/types" @@ -30,11 +30,9 @@ import ( // PolicyController API type PolicyController interface { - internalinterfaces.PolicyGetter - createPolicyHandler(resource interface{}) - updatePolicyHandler(oldResource, newResource interface{}) - deletePolicyHandler(resource interface{}) - getResourceKey(resource interface{}) string + controllerinterfaces.PolicyGetter + controllerinterfaces.PolicyHandlers + Run(stopCh <-chan struct{}) } //policyController for CRD @@ -43,8 +41,8 @@ type policyController struct { policyLister lister.PolicyLister policiesInterface policies.PolicyInterface logger *log.Logger - violationBuilder violationinternalinterfaces.ViolationGenerator - eventBuilder eventinternalinterfaces.BuilderInternal + violationBuilder violationinterfaces.ViolationGenerator + eventBuilder eventinterfaces.BuilderInternal } // NewPolicyController from cmd args @@ -83,9 +81,9 @@ func NewPolicyController(config *rest.Config, logger *log.Logger, kubeClient *ku eventBuilder: eventBuilder, } policyInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: controller.createPolicyHandler, - UpdateFunc: controller.updatePolicyHandler, - DeleteFunc: controller.deletePolicyHandler, + AddFunc: controller.CreatePolicyHandler, + UpdateFunc: controller.UpdatePolicyHandler, + DeleteFunc: controller.DeletePolicyHandler, }) // Set the controller eventBuilder.SetController(controller) @@ -164,23 +162,23 @@ func (c *policyController) addPolicyLog(name, text string) { } } -func (c *policyController) createPolicyHandler(resource interface{}) { - key := c.getResourceKey(resource) +func (c *policyController) CreatePolicyHandler(resource interface{}) { + key := c.GetResourceKey(resource) c.logger.Printf("Policy created: %s", key) } -func (c *policyController) updatePolicyHandler(oldResource, newResource interface{}) { - oldKey := c.getResourceKey(oldResource) - newKey := c.getResourceKey(newResource) +func (c *policyController) UpdatePolicyHandler(oldResource, newResource interface{}) { + oldKey := c.GetResourceKey(oldResource) + newKey := c.GetResourceKey(newResource) c.logger.Printf("Policy %s updated to %s", oldKey, newKey) } -func (c *policyController) deletePolicyHandler(resource interface{}) { - key := c.getResourceKey(resource) +func (c *policyController) DeletePolicyHandler(resource interface{}) { + key := c.GetResourceKey(resource) c.logger.Printf("Policy deleted: %s", key) } -func (c *policyController) getResourceKey(resource interface{}) string { +func (c *policyController) GetResourceKey(resource interface{}) string { if key, err := cache.MetaNamespaceKeyFunc(resource); err != nil { c.logger.Fatalf("Error retrieving policy key: %v", err) } else { diff --git a/controller/internalinterfaces/controller_interfaces.go b/controller/interfaces/controller_interfaces.go similarity index 69% rename from controller/internalinterfaces/controller_interfaces.go rename to controller/interfaces/controller_interfaces.go index 2352077394..8b3911bab2 100755 --- a/controller/internalinterfaces/controller_interfaces.go +++ b/controller/interfaces/controller_interfaces.go @@ -1,4 +1,4 @@ -package internalinterfaces +package interfaces import ( policytypes "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" @@ -6,14 +6,19 @@ import ( "k8s.io/client-go/tools/cache" ) -// PolicyGetter interface for external API type PolicyGetter interface { GetPolicies() ([]policytypes.Policy, error) GetPolicy(name string) (*policytypes.Policy, error) GetCacheInformerSync() cache.InformerSynced PatchPolicy(policy string, pt types.PatchType, data []byte) (*policytypes.Policy, error) UpdatePolicyViolations(updatedPolicy *policytypes.Policy) error - Run(stopCh <-chan struct{}) LogPolicyError(name, text string) LogPolicyInfo(name, text string) } + +type PolicyHandlers interface { + CreatePolicyHandler(resource interface{}) + UpdatePolicyHandler(oldResource, newResource interface{}) + DeletePolicyHandler(resource interface{}) + GetResourceKey(resource interface{}) string +} diff --git a/kubeclient/kubeclient.go b/kubeclient/kubeclient.go index 978fb75b1e..36436d608d 100644 --- a/kubeclient/kubeclient.go +++ b/kubeclient/kubeclient.go @@ -218,8 +218,8 @@ func (kc *KubeClient) GetResource(kind string, resource string) (runtime.Object, return rMapper[kind](kc.client, namespace, name) } -//GetSupportedResourceTypes provides list of supported types -func GetSupportedResourceTypes() (rTypes []string) { +//GetSupportedKinds provides list of supported types +func GetSupportedKinds() (rTypes []string) { for k := range rMapper { rTypes = append(rTypes, k) } diff --git a/pkg/event/builder.go b/pkg/event/builder.go index f43231b42b..93db5795ec 100644 --- a/pkg/event/builder.go +++ b/pkg/event/builder.go @@ -6,11 +6,11 @@ import ( "log" "time" - controllerinternalinterfaces "github.com/nirmata/kube-policy/controller/internalinterfaces" + controllerinterfaces "github.com/nirmata/kube-policy/controller/interfaces" kubeClient "github.com/nirmata/kube-policy/kubeclient" "github.com/nirmata/kube-policy/pkg/client/clientset/versioned/scheme" policyscheme "github.com/nirmata/kube-policy/pkg/client/clientset/versioned/scheme" - "github.com/nirmata/kube-policy/pkg/event/internalinterfaces" + eventinterfaces "github.com/nirmata/kube-policy/pkg/event/interfaces" utils "github.com/nirmata/kube-policy/pkg/event/utils" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -24,7 +24,7 @@ import ( type builder struct { kubeClient *kubeClient.KubeClient - controller controllerinternalinterfaces.PolicyGetter + controller controllerinterfaces.PolicyGetter workqueue workqueue.RateLimitingInterface recorder record.EventRecorder logger *log.Logger @@ -32,7 +32,7 @@ type builder struct { } type Builder interface { - internalinterfaces.BuilderInternal + eventinterfaces.BuilderInternal SyncHandler(key utils.EventInfo) error ProcessNextWorkItem() bool RunWorker() @@ -70,7 +70,7 @@ func initWorkqueue() workqueue.RateLimitingInterface { return workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), utils.EventWorkQueueName) } -func (b *builder) SetController(controller controllerinternalinterfaces.PolicyGetter) { +func (b *builder) SetController(controller controllerinterfaces.PolicyGetter) { b.controller = controller b.policySynced = controller.GetCacheInformerSync() } diff --git a/pkg/event/internalinterfaces/builder_interfaces.go b/pkg/event/interfaces/builder_interfaces.go similarity index 52% rename from pkg/event/internalinterfaces/builder_interfaces.go rename to pkg/event/interfaces/builder_interfaces.go index b020824df5..94a685f719 100644 --- a/pkg/event/internalinterfaces/builder_interfaces.go +++ b/pkg/event/interfaces/builder_interfaces.go @@ -1,12 +1,12 @@ -package internalinterfaces +package interfaces import ( - internalinterfaces "github.com/nirmata/kube-policy/controller/internalinterfaces" + controllerinterfaces "github.com/nirmata/kube-policy/controller/interfaces" utils "github.com/nirmata/kube-policy/pkg/event/utils" ) type BuilderInternal interface { - SetController(controller internalinterfaces.PolicyGetter) + SetController(controller controllerinterfaces.PolicyGetter) Run(threadiness int, stopCh <-chan struct{}) error AddEvent(info utils.EventInfo) } diff --git a/pkg/violation/builder.go b/pkg/violation/builder.go index 09ae07c4e0..26acc643c6 100644 --- a/pkg/violation/builder.go +++ b/pkg/violation/builder.go @@ -6,13 +6,12 @@ import ( "log" jsonpatch "github.com/evanphx/json-patch" - controllerinternalinterfaces "github.com/nirmata/kube-policy/controller/internalinterfaces" + controllerinterfaces "github.com/nirmata/kube-policy/controller/interfaces" kubeClient "github.com/nirmata/kube-policy/kubeclient" types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" - "github.com/nirmata/kube-policy/pkg/event/internalinterfaces" - eventinternalinterfaces "github.com/nirmata/kube-policy/pkg/event/internalinterfaces" + eventinterfaces "github.com/nirmata/kube-policy/pkg/event/interfaces" eventutils "github.com/nirmata/kube-policy/pkg/event/utils" - violationinternalinterfaces "github.com/nirmata/kube-policy/pkg/violation/internalinterfaces" + violationinterfaces "github.com/nirmata/kube-policy/pkg/violation/interfaces" utils "github.com/nirmata/kube-policy/pkg/violation/utils" mergetypes "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -20,13 +19,13 @@ import ( type builder struct { kubeClient *kubeClient.KubeClient - controller controllerinternalinterfaces.PolicyGetter - eventBuilder eventinternalinterfaces.BuilderInternal + controller controllerinterfaces.PolicyGetter + eventBuilder eventinterfaces.BuilderInternal logger *log.Logger } type Builder interface { - violationinternalinterfaces.ViolationGenerator + violationinterfaces.ViolationGenerator ProcessViolation(info utils.ViolationInfo) error Patch(policy *types.Policy, updatedPolicy *types.Policy) error IsActive(kind string, resource string) (bool, error) @@ -34,7 +33,7 @@ type Builder interface { func NewViolationBuilder( kubeClient *kubeClient.KubeClient, - eventBuilder internalinterfaces.BuilderInternal, + eventBuilder eventinterfaces.BuilderInternal, logger *log.Logger) (Builder, error) { builder := &builder{ @@ -53,7 +52,7 @@ func (b *builder) Create(info utils.ViolationInfo) error { return nil } -func (b *builder) SetController(controller controllerinternalinterfaces.PolicyGetter) { +func (b *builder) SetController(controller controllerinterfaces.PolicyGetter) { b.controller = controller } @@ -115,7 +114,6 @@ func (b *builder) IsActive(kind string, resource string) (bool, error) { return true, nil } -// ProcessViolation(info utils.ViolationInfo) error func (b *builder) Patch(policy *types.Policy, updatedPolicy *types.Policy) error { originalData, err := json.Marshal(policy) if err != nil { diff --git a/pkg/violation/interfaces/violation_interfaces.go b/pkg/violation/interfaces/violation_interfaces.go new file mode 100644 index 0000000000..f74cd28c6f --- /dev/null +++ b/pkg/violation/interfaces/violation_interfaces.go @@ -0,0 +1,11 @@ +package interfaces + +import ( + controllerinterfaces "github.com/nirmata/kube-policy/controller/interfaces" + utils "github.com/nirmata/kube-policy/pkg/violation/utils" +) + +type ViolationGenerator interface { + SetController(controller controllerinterfaces.PolicyGetter) + Create(info utils.ViolationInfo) error +} diff --git a/pkg/violation/internalinterfaces/violation_interfaces.go b/pkg/violation/internalinterfaces/violation_interfaces.go deleted file mode 100644 index 7863bc8906..0000000000 --- a/pkg/violation/internalinterfaces/violation_interfaces.go +++ /dev/null @@ -1,11 +0,0 @@ -package internalinterfaces - -import ( - "github.com/nirmata/kube-policy/controller/internalinterfaces" - utils "github.com/nirmata/kube-policy/pkg/violation/utils" -) - -type ViolationGenerator interface { - SetController(controller internalinterfaces.PolicyGetter) - Create(info utils.ViolationInfo) error -} diff --git a/pkg/violation/utils/util.go b/pkg/violation/utils/util.go index b200752345..1d3db344f4 100644 --- a/pkg/violation/utils/util.go +++ b/pkg/violation/utils/util.go @@ -1,10 +1,8 @@ package utils +import policytype "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" + type ViolationInfo struct { - Kind string - Resource string - Policy string - Rule string - Reason string - Message string + Policy string + policytype.Violation } diff --git a/webhooks/admission.go b/webhooks/admission.go index 8e7f94bd61..652dcc3c34 100644 --- a/webhooks/admission.go +++ b/webhooks/admission.go @@ -9,7 +9,7 @@ import ( ) func kindIsSupported(kind string) bool { - for _, k := range kubeclient.GetSupportedResourceTypes() { + for _, k := range kubeclient.GetSupportedKinds() { if k == kind { return true } diff --git a/webhooks/mutation.go b/webhooks/mutation.go index 5b29b031be..ad651c329d 100644 --- a/webhooks/mutation.go +++ b/webhooks/mutation.go @@ -6,11 +6,12 @@ import ( "log" "os" - controllerinternalinterfaces "github.com/nirmata/kube-policy/controller/internalinterfaces" + controllerinterfaces "github.com/nirmata/kube-policy/controller/interfaces" kubeclient "github.com/nirmata/kube-policy/kubeclient" types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" v1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" rest "k8s.io/client-go/rest" ) @@ -18,13 +19,13 @@ import ( // business logic for resource mutation type MutationWebhook struct { kubeclient *kubeclient.KubeClient - controller controllerinternalinterfaces.PolicyGetter + controller controllerinterfaces.PolicyGetter registration *MutationWebhookRegistration logger *log.Logger } // Registers mutation webhook in cluster and creates object for this webhook -func CreateMutationWebhook(clientConfig *rest.Config, kubeclient *kubeclient.KubeClient, controller controllerinternalinterfaces.PolicyGetter, logger *log.Logger) (*MutationWebhook, error) { +func CreateMutationWebhook(clientConfig *rest.Config, kubeclient *kubeclient.KubeClient, controller controllerinterfaces.PolicyGetter, logger *log.Logger) (*MutationWebhook, error) { if clientConfig == nil || kubeclient == nil || controller == nil { return nil, errors.New("Some parameters are not set") } @@ -55,7 +56,11 @@ func (mw *MutationWebhook) Mutate(request *v1beta1.AdmissionRequest) *v1beta1.Ad 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) - policies, _ := mw.controller.GetPolicies() + policies, err := mw.controller.GetPolicies() + if err != nil { + utilruntime.HandleError(err) + return nil + } if len(policies) == 0 { return nil }