From 38baee7660def8275bebc9e9a828fd262f5ffb8f Mon Sep 17 00:00:00 2001 From: shivdudhani Date: Tue, 21 May 2019 09:27:04 -0700 Subject: [PATCH] PR code review changes --- client/certificates.go | 30 +-- client/client.go | 6 +- client/utils.go | 16 +- .../policy-cm-test.yaml | 20 +- .../policy-namespace-patch-cmgCG-sgCG.yaml | 52 +++--- main.go | 2 +- pkg/controller/controller.go | 2 - pkg/controller/processPolicy.go | 175 ------------------ pkg/event/controller.go | 13 +- pkg/violation/builder.go | 2 - 10 files changed, 69 insertions(+), 249 deletions(-) delete mode 100644 pkg/controller/processPolicy.go diff --git a/client/certificates.go b/client/certificates.go index 0549eef489..0aa9578f5b 100644 --- a/client/certificates.go +++ b/client/certificates.go @@ -42,23 +42,18 @@ func (c *Client) GenerateTlsPemPair(props tls.TlsCertificateProps) (*tls.TlsPemP // Submits and approves certificate request, returns request which need to be fetched func (c *Client) submitAndApproveCertificateRequest(req *certificates.CertificateSigningRequest) (*certificates.CertificateSigningRequest, error) { - //TODO: using the CSR interface from the kubeclient certClient, err := c.GetCSRInterface() if err != nil { return nil, err } - // certClient := kc.client.CertificatesV1beta1().CertificateSigningRequests() - csrList, err := c.ListResource("certificatesigningrequests", "") - // csrList, err := certClient.List(metav1.ListOptions{}) + csrList, err := c.ListResource(CSR, "") if err != nil { return nil, errors.New(fmt.Sprintf("Unable to list existing certificate requests: %v", err)) } for _, csr := range csrList.Items { - csr.GetName() if csr.GetName() == req.ObjectMeta.Name { - // Delete - err := c.DeleteResouce("certificatesigningrequests", "", csr.GetName()) + err := c.DeleteResouce(CSR, "", csr.GetName()) if err != nil { return nil, errors.New(fmt.Sprintf("Unable to delete existing certificate request: %v", err)) } @@ -67,9 +62,7 @@ func (c *Client) submitAndApproveCertificateRequest(req *certificates.Certificat } } - // Create - unstrRes, err := c.CreateResource("certificatesigningrequests", "", req) - // res, err := certClient.Create(req) + unstrRes, err := c.CreateResource(CSR, "", req) if err != nil { return nil, err } @@ -93,15 +86,12 @@ func (c *Client) submitAndApproveCertificateRequest(req *certificates.Certificat return res, nil } -const certificateFetchWaitInterval time.Duration = 200 * time.Millisecond - // Fetches certificate from given request. Tries to obtain certificate for maxWaitSeconds func (c *Client) fetchCertificateFromRequest(req *certificates.CertificateSigningRequest, maxWaitSeconds uint8) ([]byte, error) { // TODO: react of SIGINT and SIGTERM timeStart := time.Now() - c.GetResource("certificatesigningrequests", "", req.ObjectMeta.Name) for time.Now().Sub(timeStart) < time.Duration(maxWaitSeconds)*time.Second { - unstrR, err := c.GetResource("certificatesigningrequests", "", req.ObjectMeta.Name) + unstrR, err := c.GetResource(CSR, "", req.ObjectMeta.Name) if err != nil { return nil, err } @@ -129,7 +119,7 @@ const certificateField string = "certificate" // Reads the pair of TLS certificate and key from the specified secret. func (c *Client) ReadTlsPair(props tls.TlsCertificateProps) *tls.TlsPemPair { name := generateSecretName(props) - unstrSecret, err := c.GetResource("secrets", props.Namespace, name) + unstrSecret, err := c.GetResource(Secret, props.Namespace, name) if err != nil { c.logger.Printf("Unable to get secret %s/%s: %s", props.Namespace, name, err) return nil @@ -157,8 +147,8 @@ func (c *Client) ReadTlsPair(props tls.TlsCertificateProps) *tls.TlsPemPair { // Updates existing secret or creates new one. func (c *Client) WriteTlsPair(props tls.TlsCertificateProps, pemPair *tls.TlsPemPair) error { name := generateSecretName(props) - unstrSecret, err := c.GetResource("secrets", props.Namespace, name) - if err == nil { // Update existing secret + unstrSecret, err := c.GetResource(Secret, props.Namespace, name) + if err == nil { secret, err := convertToSecret(unstrSecret) if err != nil { return nil @@ -169,12 +159,12 @@ func (c *Client) WriteTlsPair(props tls.TlsCertificateProps, pemPair *tls.TlsPem } secret.Data[certificateField] = pemPair.Certificate secret.Data[privateKeyField] = pemPair.PrivateKey - c.UpdateResource("secrets", props.Namespace, secret) + _, err = c.UpdateResource(Secret, props.Namespace, secret) if err == nil { c.logger.Printf("Secret %s is updated", name) } - } else { // Create new secret + } else { secret := &v1.Secret{ TypeMeta: metav1.TypeMeta{ @@ -191,7 +181,7 @@ func (c *Client) WriteTlsPair(props tls.TlsCertificateProps, pemPair *tls.TlsPem }, } - _, err := c.CreateResource("secrets", props.Namespace, secret) + _, err := c.CreateResource(Secret, props.Namespace, secret) if err == nil { c.logger.Printf("Secret %s is created", name) } diff --git a/client/client.go b/client/client.go index 960a4f7a8d..83b75ad4f0 100644 --- a/client/client.go +++ b/client/client.go @@ -29,7 +29,7 @@ type Client struct { clientConfig *rest.Config } -func NewDynamicClient(config *rest.Config, logger *log.Logger) (*Client, error) { +func NewClient(config *rest.Config, logger *log.Logger) (*Client, error) { client, err := dynamic.NewForConfig(config) if err != nil { return nil, err @@ -179,7 +179,7 @@ func (c *Client) GenerateSecret(generator types.Generation, namespace string) er // if generator.CopyFrom != nil { c.logger.Printf("Copying data from secret %s/%s", generator.CopyFrom.Namespace, generator.CopyFrom.Name) // Get configMap resource - unstrSecret, err := c.GetResource("secrets", generator.CopyFrom.Namespace, generator.CopyFrom.Name) + unstrSecret, err := c.GetResource(Secret, generator.CopyFrom.Namespace, generator.CopyFrom.Name) if err != nil { return err } @@ -286,7 +286,7 @@ func (c *Client) createConfigMapAfterNamespaceIsCreated(configMap v1.ConfigMap, func (c *Client) createSecretAfterNamespaceIsCreated(secret v1.Secret, namespace string) { err := c.waitUntilNamespaceIsCreated(namespace) if err == nil { - _, err = c.CreateResource("secrets", namespace, secret) + _, err = c.CreateResource(Secret, namespace, secret) } if err != nil { c.logger.Printf("Can't create a secret: %s", err) diff --git a/client/utils.go b/client/utils.go index be0244e096..d768ddda78 100644 --- a/client/utils.go +++ b/client/utils.go @@ -11,10 +11,15 @@ import ( "k8s.io/client-go/rest" ) +const ( + CSR string = "certificatesigningrequests" + Secret string = "secrets" +) const namespaceCreationMaxWaitTime time.Duration = 30 * time.Second const namespaceCreationWaitInterval time.Duration = 100 * time.Millisecond var groupVersionMapper map[string]schema.GroupVersionResource +var kubeClient *kubernetes.Clientset func getGrpVersionMapper(kind string, clientConfig *rest.Config, refresh bool) schema.GroupVersionResource { // build the GVK mapper @@ -83,9 +88,12 @@ func refreshRegisteredResources(mapper map[string]schema.GroupVersionResource, c } func newKubeClient(clientConfig *rest.Config) (*kubernetes.Clientset, error) { - client, err := kubernetes.NewForConfig(clientConfig) - if err != nil { - return nil, err + var err error + if kubeClient == nil { + kubeClient, err = kubernetes.NewForConfig(clientConfig) + if err != nil { + return nil, err + } } - return client, nil + return kubeClient, nil } diff --git a/examples/ConfigMapGenerator-SecretGenerator/policy-cm-test.yaml b/examples/ConfigMapGenerator-SecretGenerator/policy-cm-test.yaml index 9e1056611b..3a0ce26477 100644 --- a/examples/ConfigMapGenerator-SecretGenerator/policy-cm-test.yaml +++ b/examples/ConfigMapGenerator-SecretGenerator/policy-cm-test.yaml @@ -4,15 +4,17 @@ metadata : name: "policy-configmapgenerator-test" spec: rules: - - name: "Policy ConfigMap sample rule" - resource: + - name: "copyCM" + resource : kind : Namespace - name: "ns2" - generate: + selector: + matchLabels: + LabelForSelector : "namespace2" + generate : kind: ConfigMap - name: copied-cm - copyFrom: - namespace: default - name: game-config - data: + name : copied-cm + copyFrom : + namespace : default + name : game-config + data : secretData: "data from cmg" \ No newline at end of file diff --git a/examples/ConfigMapGenerator-SecretGenerator/policy-namespace-patch-cmgCG-sgCG.yaml b/examples/ConfigMapGenerator-SecretGenerator/policy-namespace-patch-cmgCG-sgCG.yaml index 922eeee05a..56e21a4598 100644 --- a/examples/ConfigMapGenerator-SecretGenerator/policy-namespace-patch-cmgCG-sgCG.yaml +++ b/examples/ConfigMapGenerator-SecretGenerator/policy-namespace-patch-cmgCG-sgCG.yaml @@ -3,42 +3,47 @@ # To apply this policy you need to create secret and configMap in "default" namespace # and then create a namespace -apiVersion : policy.nirmata.io/v1alpha1 +apiVersion : kubepolicy.nirmata.io/v1alpha1 kind : Policy metadata : name : "policy-ns-patch-cmg-sg" spec : - failurePolicy: stopOnError rules: - - resource : + - name: "patchNamespace2" + resource : kind : Namespace selector: matchLabels: LabelForSelector : "namespace2" - patch: - - path: "/metadata/labels/isMutatedByPolicy" - op: add - value: "true" + mutate: + patches: + - path: "/metadata/labels/isMutatedByPolicy" + op: add + value: "true" - - resource : + - name: "copyCM" + resource : kind : Namespace selector: matchLabels: LabelForSelector : "namespace2" - configMapGenerator : + generate : + kind: ConfigMap name : copied-cm copyFrom : namespace : default name : game-config data : secretData: "data from cmg" - - - resource : + + - name: "generateCM" + resource : kind : Namespace selector: matchLabels: LabelForSelector : "namespace2" - configMapGenerator : + generate : + kind: ConfigMap name : generated-cm data : secretData: "very sensitive data from cmg" @@ -49,13 +54,12 @@ spec : image.public.key=771 rsa.public.key=42 - - resource : + - name: "generateSecret" + resource : kind : Namespace - selector: - matchLabels: - LabelForSelector : "namespace2" - - secretGenerator : + name: ns2 + generate : + kind: Secret name : generated-secrets data : foo : bar @@ -66,13 +70,12 @@ spec : foo1=bar1 foo2=bar2 - - resource : + - name: "copySecret" + resource : kind : Namespace - selector: - matchLabels: - LabelForSelector : "namespace2" - - secretGenerator : + name: ns2 + generate : + kind: Secret name : copied-secrets copyFrom : namespace : default @@ -80,4 +83,3 @@ spec : data : foo : bar secretData: "data from sg" - diff --git a/main.go b/main.go index 4e73cc1404..eac3f1f685 100644 --- a/main.go +++ b/main.go @@ -26,7 +26,7 @@ func main() { log.Fatalf("Error building kubeconfig: %v\n", err) } - client, err := client.NewDynamicClient(clientConfig, nil) + client, err := client.NewClient(clientConfig, nil) if err != nil { log.Fatalf("Error creating client: %v\n", err) } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 71660ac779..572e287a2b 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -160,8 +160,6 @@ func (pc *PolicyController) syncHandler(obj interface{}) error { if key, ok = obj.(string); !ok { return fmt.Errorf("expected string in workqueue but got %#v", obj) } - // convert the namespace/name string into distinct namespace and name - //TODO: currently policies are clustered, but the above code is to support namespaced as well _, name, err := cache.SplitMetaNamespaceKey(key) if err != nil { utilruntime.HandleError(fmt.Errorf("invalid policy key: %s", key)) diff --git a/pkg/controller/processPolicy.go b/pkg/controller/processPolicy.go deleted file mode 100644 index e32b0327df..0000000000 --- a/pkg/controller/processPolicy.go +++ /dev/null @@ -1,175 +0,0 @@ -package controller - -import ( - "fmt" - - types "github.com/nirmata/kube-policy/pkg/apis/policy/v1alpha1" - engine "github.com/nirmata/kube-policy/pkg/engine" - "github.com/nirmata/kube-policy/pkg/engine/mutation" - event "github.com/nirmata/kube-policy/pkg/event" - violation "github.com/nirmata/kube-policy/pkg/violation" - "k8s.io/apimachinery/pkg/labels" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" -) - -func (pc *PolicyController) runForPolicy(key string) { - - policy, err := pc.getPolicyByKey(key) - if err != nil { - utilruntime.HandleError(fmt.Errorf("invalid resource key: %s, err: %v", key, err)) - return - } - - if policy == nil { - pc.logger.Printf("Could not find policy by key %s", key) - return - } - - violations, events, err := pc.processPolicy(*policy) - if err != nil { - // add Error processing policy event - } - - pc.logger.Printf("%v, %v", violations, events) - // TODO: - // create violations - // pc.violationBuilder.Add() - // create events - // pc.eventBuilder.Add() - -} - -// processPolicy process the policy to all the matched resources -func (pc *PolicyController) processPolicy(policy types.Policy) ( - violations []violation.Info, events []event.Info, err error) { - - for _, rule := range policy.Spec.Rules { - resources, err := pc.filterResourceByRule(rule) - if err != nil { - pc.logger.Printf("Failed to filter resources by rule %s, err: %v\n", rule.Name, err) - } - - for _, resource := range resources { - if err != nil { - pc.logger.Printf("Failed to marshal resources map to rule %s, err: %v\n", rule.Name, err) - continue - } - - violation, eventInfos, err := engine.ProcessExisting(policy, resource) - if err != nil { - pc.logger.Printf("Failed to process rule %s, err: %v\n", rule.Name, err) - continue - } - - violations = append(violations, violation...) - events = append(events, eventInfos...) - } - } - return violations, events, nil -} - -func (pc *PolicyController) filterResourceByRule(rule types.Rule) ([][]byte, error) { - var targetResources [][]byte - // TODO: make this namespace all - var namespace = "default" - if err := rule.Validate(); err != nil { - return nil, fmt.Errorf("invalid rule detected: %s, err: %v", rule.Name, err) - } - - // Get the resource list from kind - resources, err := pc.client.ListResource(rule.ResourceDescription.Kind, namespace) - if err != nil { - return nil, err - } - - for _, resource := range resources.Items { - // TODO: - //rawResource, err := json.Marshal(resource) - // objKind := resource.GetObjectKind() - // codecFactory := serializer.NewCodecFactory(runtime.NewScheme()) - // codecFactory.EncoderForVersion() - - if err != nil { - pc.logger.Printf("failed to marshal object %v", resource) - continue - } - - // filter the resource by name and label - //if ok, _ := mutation.ResourceMeetsRules(rawResource, rule.ResourceDescription); ok { - // targetResources = append(targetResources, resource) - //} - } - return targetResources, nil -} - -func (pc *PolicyController) getPolicyByKey(key string) (*types.Policy, error) { - // Create nil Selector to grab all the policies - selector := labels.NewSelector() - cachedPolicies, err := pc.policyLister.List(selector) - if err != nil { - return nil, err - } - - for _, elem := range cachedPolicies { - if elem.Name == key { - return elem, nil - } - } - return nil, nil -} - -//TODO wrap the generate, mutation & validation functions for the existing resources -//ProcessExisting processes the policy rule types for the existing resources -func (pc *PolicyController) processExisting(policy types.Policy, rawResource []byte) ([]violation.Info, []event.Info, error) { - // Generate - // generatedDataList := engine.Generate(pc.logger, policy, rawResource) - // // apply the generateData using the kubeClient - // err = pc.applyGenerate(generatedDataList) - // if err != nil { - // return nil, nil, err - // } - // // Mutation - // mutationPatches, err := engine.Mutation(pc.logger, policy, rawResource) - // if err != nil { - // return nil, nil, err - // } - // // Apply mutationPatches on the rawResource - // err = pc.applyPatches(mutationPatches, rawResource) - // if err != nil { - // return nil, nil, err - // } - // //Validation - // validate, _, _ := engine.Validation(policy, rawResource) - // if !validate { - // // validation has errors -> so there will be violations - // // call the violatio builder to apply the violations - // } - // // Generate events - - return nil, nil, nil -} - -//TODO: return events and policy violations -// func (pc *PolicyController) applyGenerate(generatedDataList []engine.GenerationResponse) error { -// // for _, generateData := range generatedDataList { -// // switch generateData.Generator.Kind { -// // case "ConfigMap": -// // err := pc.client.GenerateConfigMap(generateData.Generator, generateData.Namespace) -// // if err != nil { -// // return err -// // } -// // case "Secret": -// // err := pc.client.GenerateSecret(generateData.Generator, generateData.Namespace) -// // if err != nil { -// // return err -// // } -// // default: -// // return errors.New("Unsuported config kind") -// // } -// // } -// return nil -// } - -func (pc *PolicyController) applyPatches([]mutation.PatchBytes, []byte) error { - return nil -} diff --git a/pkg/event/controller.go b/pkg/event/controller.go index e0f011ef0e..22103993bc 100644 --- a/pkg/event/controller.go +++ b/pkg/event/controller.go @@ -62,7 +62,11 @@ func NewEventController(client *client.Client, func initRecorder(client *client.Client) record.EventRecorder { // Initliaze Event Broadcaster - policyscheme.AddToScheme(scheme.Scheme) + err := policyscheme.AddToScheme(scheme.Scheme) + if err != nil { + utilruntime.HandleError(err) + return nil + } eventBroadcaster := record.NewBroadcaster() eventBroadcaster.StartLogging(log.Printf) eventInterface, err := client.GetEventsInterface() @@ -151,13 +155,6 @@ func (c *controller) SyncHandler(key Info) error { if err != nil { return err } - //TODO: Test if conversion from unstructured to runtime.Object is implicit or explicit conversion is required - // resourceobj, err := client.ConvertToRuntimeObject(resource) - // if err != nil { - // return err - // } - - // resource, err = c.kubeClient.GetResource(key.Kind, key.Resource) if err != nil { utilruntime.HandleError(fmt.Errorf("unable to create event for resource %s, will retry ", key.Resource)) return err diff --git a/pkg/violation/builder.go b/pkg/violation/builder.go index 283027ff2e..faa8d9313d 100644 --- a/pkg/violation/builder.go +++ b/pkg/violation/builder.go @@ -104,9 +104,7 @@ func (b *builder) isActive(kind string, resource string) (bool, error) { return false, err } // Generate Merge Patch - //TODO: test for namspace and clustered object _, err = b.client.GetResource(kind, namespace, name) - // _, err := b.kubeClient.GetResource(kind, resource) if err != nil { utilruntime.HandleError(fmt.Errorf("unable to get resource %s ", resource)) return false, err